Page MenuHomePhabricator

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

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

Details

Summary

Writing the .note.gnu.property 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 .note.gnu.property 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.
llvm/lib/Target/AArch64/AArch64.td
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.
llvm/lib/Target/AArch64/AArch64.td
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
llvm/lib/Target/AArch64/MCTargetDesc/AArch64MCAsmInfo.cpp
106 ↗(On Diff #285492)

No need to the .getValue() part.

llvm/lib/Target/AArch64/MCTargetDesc/AArch64MCAsmInfo.h
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.

clang/lib/Driver/ToolChains/Clang.cpp
7024

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

llvm/lib/Target/AArch64/MCTargetDesc/AArch64TargetStreamer.cpp
56–58

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

64

ditto

68

move this to just before the SwitchSection call below?

llvm/lib/Target/AArch64/MCTargetDesc/AArch64TargetStreamer.h
43

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.