[PATCH] D71019: [AArch64] Emit PAC/BTI .note.gnu.property flags

Mon Dec 9 06:13:49 PST 2019

chill marked 4 inline comments as done.
chill added inline comments.

================
Comment at: llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp:190
+  // Assemble feature flags that may require creation of a note section.
+  unsigned Flags = ELF::GNU_PROPERTY_AARCH64_FEATURE_1_BTI |
+                   ELF::GNU_PROPERTY_AARCH64_FEATURE_1_PAC;
----------------
peter.smith wrote:
> There is a similar implementation in X86AsmPrinter that uses FeatureFlagsAnd rather than Flags, it may be worth using a similar naming convention as these are equivalent features. This isn't a strong opinion by any means, just a suggestion.
I don't see how that would be an improvement in readability. If the function manipulated several different kinds of flags, it'd be worth it renaming to, say, `FeatureFlags`, but then, if the function did more than one thing I'd separate the part that emitted the note section in a separate utility function ... and leave the name `Flags`. `FeatureFlagsAnd` is a rather unfortunate name, a) it is extremely long for a local variable name, and b) it makes me ask "... and what?" :D

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71019/new/

https://reviews.llvm.org/D71019