Page MenuHomePhabricator

[AArch64] Add -mmark-bti-property flag.

Authored by danielkiss on Jun 16 2020, 5:28 AM.



Writing the manually is error prone and hard to
maintain in the assembly files.
The -mmark-bti-property is for the assembler to emit the section with the
GNU_PROPERTY_AARCH64_FEATURE_1_BTI. To be used when C/C++ is compiled
with -mbranch-protection=bti.

This patch refactors the handling.

Diff Detail

Event Timeline

danielkiss created this revision.Jun 16 2020, 5:28 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 16 2020, 5:28 AM
danielkiss planned changes to this revision.Jun 18 2020, 6:44 AM

I got some comment internally, the -mbti might suggest it enables the bti feature so I'm going to rename the flag to -mmark-bti-property.

danielkiss retitled this revision from [AArch64] Add -mbti flag. to [AArch64] Add -mmark-bti-property flag..
danielkiss edited the summary of this revision. (Show Details)

Rename the flag to -mmark-bti-property

chill requested changes to this revision.Aug 7 2020, 3:25 AM
chill added inline comments.
352 ↗(On Diff #272024)

No, this is an abuse of subtarget features. Subtarget features represent characteristics of the chip, they shouldn't be used to pass arbitrary bits of information.
Possible alternatives - TargetOptions (cf. BackendUtil.cpp:initTargetOptions()) or
LLVM command-line arguments (cf. BackendUtil.cpp:setCommandLineOpts().

This revision now requires changes to proceed.Aug 7 2020, 3:25 AM

Fix review comments.

danielkiss marked an inline comment as done.Aug 13 2020, 2:06 PM
danielkiss added inline comments.
352 ↗(On Diff #272024)

Thanks, now I learned how to pass flags across layers, that was not clear at the first time.

chill added inline comments.Aug 25 2020, 1:22 AM
106 ↗(On Diff #285492)

No need to the .getValue() part.

34 ↗(On Diff #285492)

Is there a need for this data member? The option value does not change over time, and the option can be defined in AArch64TargetStreamer.cpp.

danielkiss marked an inline comment as done.
danielkiss marked 2 inline comments as done.
chill accepted this revision.Sep 2 2020, 7:31 AM

LGTM. It'd be nice if we could get someone non-Arm to have a look too. though.

This revision is now accepted and ready to land.Sep 2 2020, 7:31 AM

I don't have any thoughts on the change per se, so just minor thoughts/generic code review.


it looks like A is unused. Should we be using Args.hasFlag instead of Args.getLastArg?


The coding style allows for the curly braces to be omitted for single statement bodies.




move this to just before the SwitchSection call below?


does this need to be virtual?

danielkiss marked 4 inline comments as done.Sep 15 2020, 7:49 AM

@nickdesaulniers Thanks for the review, comments are addressed.

nickdesaulniers accepted this revision.Sep 15 2020, 11:02 AM
This revision was automatically updated to reflect the committed changes.