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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/lib/IR/Verifier.cpp:1860
+                       .getValueAsString();
+    StringRef S = S0;
+    unsigned N;
----------------
nickdesaulniers wrote:
> Why do we make a copy of the `StringRef`? (Sorry I didn't notice in code review of the below test case previously). I assume this has something to do with avoiding "consuming" an attribute?
My fault. `StringRef::getAsInteger` does not consume the prefix in case of a parse error.

Updated.


================
Comment at: llvm/lib/Target/AArch64/AArch64AsmPrinter.cpp:1303
+  while (N--)
+    EmitToStreamer(*OutStreamer, MCInstBuilder(AArch64::HINT).addImm(0));
+}
----------------
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.)


================
Comment at: llvm/test/CodeGen/AArch64/patchable-function-entry.ll:80
+; CHECK:       .size f3_2, .Lfunc_end5-f3_2
+; NOFSECT      .section __patchable_function_entries,"awo", at progbits,f1,unique,0
+; FSECT:       .section __patchable_function_entries,"awo", at progbits,f3_2,unique,4
----------------
nickdesaulniers wrote:
> What is this `NOFSECT` checking? That there's no entry for `f1`? Why?
Added a comment above.

```
+;; Without -function-sections, f2 is in the same text section as f1.
+;; They share the __patchable_function_entries section.
+;; With -function-sections, f1 and f2 are in different text sections.
+;; Use separate __patchable_function_entries.
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73070