aaron.ballman requested changes to this revision.
aaron.ballman added a reviewer: erichkeane.
aaron.ballman added a comment.
This revision now requires changes to proceed.
I can see some minor utility to this for some kinds of libraries perhaps, but
I'm on the fence about adding the attribute. Is there a reason we need the user
to write this attribute at all? e.g., could we be smarter about inline function
definitions rather than making this the user's problem?
This is missing all the usual Sema tests (that the attribute is diagnosed when
not written on a function, that it diagnoses incorrect attribute arguments, and
so on), and should also have a release note.
================
Comment at: clang/include/clang/Basic/Attr.td:774
+def Trampoline : InheritableAttr {
+ let Spellings = [GNU<"trampoline">];
+ let Subjects = SubjectList<[Function, ObjCMethod]>;
----------------
Why does this not have a `[[]]` spelling? New attributes typically should be
using the `Clang` spelling unless there's a compelling reason not to.
Also, I'm a bit worried that the identifier `trampoline` is pretty generic and
can mean different things to different folks, so we might want a more debug
info-specific name for this
(https://en.wikipedia.org/wiki/Trampoline_(computing)).
================
Comment at: clang/include/clang/Basic/Attr.td:776
+ let Subjects = SubjectList<[Function, ObjCMethod]>;
+ let Args = [StringArgument<"Name">];
+ let Documentation = [TrampolineDocs];
----------------
This is quite fragile, for a few reasons.
1) It's too easy for the user to get undiagnosed typos. e.g., they want to
dispatch to `bar` but accidentally dispatch to `barf` or `bar()` instead.
2) How does this work with overloaded functions? Templated functions with
specializations?
It seems to me that this should be accepting an Expr argument that's either a
`CallExpr`/`MemberCallExpr` or is a `DeclRefExpr` that names a function, so you
could do:
```
void bar();
void baz(int), baz(float);
__attribute__((trampoline(bar))) void foo() { bar(); }
// or
__attribute__((trampoline(baz(12))) void foo() { baz(12); }
```
That said, this is still fragile in that it's easy to write the attribute and
then the function body drifts out of sync with the attribute over time. Given
that this only impacts debug information, that sort of latent issue is likely
to hide in the code for a long time. Should we consider a warning if the
function named by the attribute is not called within the function carrying the
attribute?
================
Comment at: clang/include/clang/Basic/AttrDocs.td:6905
+ let Category = DocCatFunction;
+ let Heading = "trampoline";
+ let Content = [{
----------------
No need to specify a heading, one is picked automatically because this
attribute only has one spelling.
================
Comment at: clang/include/clang/Basic/AttrDocs.td:6975-6976
}
+
+
----------------
Spurious whitespace changes.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:4885
+ if (const auto *PrevSNA = D->getAttr<TrampolineAttr>()) {
+ if (PrevSNA->getName() != Name && !PrevSNA->isImplicit()) {
+ Diag(PrevSNA->getLocation(), diag::err_attributes_are_not_compatible)
----------------
Nothing creates this attribute implicitly, so you should drop this bit.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D146595/new/
https://reviews.llvm.org/D146595
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
