[PATCH] D72221: Support function attribute patchable_function_entry

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


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


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4923
+  const llvm::Triple &T = S.Context.getTargetInfo().getTriple();
+  if (!T.isAArch64() && T.getArch() != llvm::Triple::x86 &&
+      T.getArch() != llvm::Triple::x86_64) {
----------------
If you make the attribute target-specific, this code can be removed.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4930
+  uint32_t Size = 0, Start = 0;
+  if (AL.getNumArgs()) {
+    if (!checkUInt32Argument(S, AL, AL.getArgAsExpr(0), Size, 0, true))
----------------
Should not be a need to check this -- it will always have at least one argument because the common attribute handling code will diagnose otherwise given the subject list.


================
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"
----------------
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
}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72221