[PATCH] D73070: Add function attribute "patchable-function-prefix" to support -fpatchable-function-entry=N,M where M>0

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/AsmPrinter.h:456
+  /// Emit N NOP instructions.
+  void emitNops(int N);
+
----------------
`unsigned N`

Can we mark this method `const`?


================
Comment at: llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:2806
+void AsmPrinter::emitNops(int N) {
+  MCInst Noop;
+  MF->getSubtarget().getInstrInfo()->getNoop(Noop);
----------------
guard or `assert` that `N > 0`


================
Comment at: llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp:1303
+  while (N--)
+    EmitToStreamer(*OutStreamer, MCInstBuilder(AArch64::HINT).addImm(0));
+}
----------------
MaskRay wrote:
> nickdesaulniers wrote:
> > I think it would be better to use `TargetInstrInfo::getNoop()` via `MF->getSubtarget().getInstrInfo()->getNoop(Noop);`. Then you could make this a non-virtual function call in the base `AsmPrinter` class.  In fact, emitting nops is useful and could be a method on the base `AsmPrinter`, and that could be reused between `AsmPrinter::EmitFunctionHeader()` and `AsmPrinter::EmitFunctionBody()`.
> TIL getNoop. I was looking for such an operation bug did not find one. This is much more elegant! Added `AsmPrinter::emitNops`.
> 
> Unfortunately we cannot retire `LowerPATCHABLE_FUNCTION_ENTER`.
> 
> AsmPrinter (assembly printer/object file emitter) does the following:
> 
> 1. Emit data before the function entry
> 2. Emit the function entry label and the label for __patchable_function_entries
> 3. Emit the function body (which may have optional `bti c` or `endbr64`)
> 
> Due to `bti c` (AArch64) and `endbr64` (x86), we cannot make the handling of the function attribute "patchable-function-entry" generic.
> (-fpatchable-function-entry=, -mbranch-protection= (AArch64 BTI) and -fcf-protection= (Intel Indirect Branch Tracking) are implemented as passes which can prepend instructions to the function body.)
Looks like we can still modify `AsmPrinter::EmitFunctionBody()` and `ARMAsmPrinter::EmitSled()` to now make use of `AsmPrinter::emitNops()` in this change.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73070