[PATCH] D72221: Support function attribute patchable_function_entry

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:686
 
+def PatchableFunctionEntry : InheritableAttr {
+  let Spellings = [GCC<"patchable_function_entry">];
----------------
aaron.ballman wrote:
> Should this be inheriting from `TargetSpecificAttr` as well given that the attribute is only supported on some architectures?
Thanks for the suggestion. Didn't know TargetSpecificAttr.


================
Comment at: clang/include/clang/Basic/AttrDocs.td:3995
+``__attribute__((patchable_function_entry(N,M)))`` is used to generate M NOPs
+before the function entry and N-M NOPs after the function entry. This attributes
+takes precedence over command line option ``-fpatchable-function-entry=N,M``.
----------------
ostannard wrote:
> Grammar nits:
> s/attributes/attribute/
> over _the_ command line option
Thanks. (I am a non-native English speaker and any advice will be highly appreciated.)

(So, I believe GCC's documentation misses _the_)


================
Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:826
+          "patchable-function-entry",
+          (Twine(Attr->getSize()) + "," + Twine(Attr->getStart())).str());
   }
----------------
nickdesaulniers wrote:
> ostannard wrote:
> > I think using two function attributes here would be better, to avoid needing to parse this again later.
> In that case, it would not make sense to have start without a size, and thus should be checked for in the verification.
Agreed. Updated D72215 (semantics of the IR function attribute "patchable-function-entry") as well.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4925
+      T.getArch() != llvm::Triple::x86_64) {
+    S.Diag(getAttrLoc(AL), diag::err_attribute_unsupported) << AL;
+    return;
----------------
nickdesaulniers wrote:
> nickdesaulniers wrote:
> > Why is the target arch also checked in `clang/lib/Driver/ToolChains/Clang.cpp` in https://reviews.llvm.org/D72222?
> Ah, https://reviews.llvm.org/D72221 is checking the `__attribute__` syntax, https://reviews.llvm.org/D72222 is checking the command line `-f` syntax.
Yes. D72221 is for the Clang function attribute while D72222 is for `-fpatchable-function-entry=`.




================
Comment at: clang/test/CodeGen/patchable-function-entry.c:20
+// CHECK: attributes #0 = { {{.*}} "patchable-function-entry"="0,0"
+// CHECK: attributes #1 = { {{.*}} "patchable-function-entry"="2,0"
----------------
aaron.ballman wrote:
> Can you also add a test verifying that this attribute works with redeclarations as well? Something like:
> ```
> [[gnu::patchable_function_entry(12, 4)]] void func(void);
> 
> void func(void) { // Definition should have the noop sled
> }
> ```
```
__attribute__((patchable_function_entry(2,0))) void f20decl();
__attribute__((noinline)) void f20decl() {}
```

Checked this case. I'll delete `__attribute__((noinline))` since it does not seem to be clear what it intends to do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72221