[PATCH] D81930: [AArch64] Add -mmark-bti-property flag.

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added a comment.

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



================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:6995
+  case llvm::Triple::aarch64_be:
+    if (Arg *A = Args.getLastArg(options::OPT_mmark_bti_property)) {
+      CmdArgs.push_back("-mllvm");
----------------
it looks like `A` is unused.  Should we be using `Args.hasFlag` instead of `Args.getLastArg`?


================
Comment at: llvm/lib/Target/AArch64/MCTargetDesc/AArch64TargetStreamer.cpp:56-58
+  if (MarkBTIProperty) {
+    emitNoteSection(ELF::GNU_PROPERTY_AARCH64_FEATURE_1_BTI);
+  }
----------------
The coding style allows for the curly braces to be omitted for single statement bodies.


================
Comment at: llvm/lib/Target/AArch64/MCTargetDesc/AArch64TargetStreamer.cpp:64
+    return;
+  }
+  MCStreamer &OutStreamer = getStreamer();
----------------
ditto


================
Comment at: llvm/lib/Target/AArch64/MCTargetDesc/AArch64TargetStreamer.cpp:68
+  // Emit a .note.gnu.property section with the flags.
+  MCSection *Cur = OutStreamer.getCurrentSectionOnly();
+  MCSectionELF *Nt = Context.getELFSection(".note.gnu.property", ELF::SHT_NOTE,
----------------
move this to just before the SwitchSection call below?


================
Comment at: llvm/lib/Target/AArch64/MCTargetDesc/AArch64TargetStreamer.h:40
+  /// Callback used to implement the .note.gnu.property section.
+  virtual void emitNoteSection(unsigned Flags);
+
----------------
does this need to be virtual?


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

https://reviews.llvm.org/D81930