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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
