[PATCH] D146595: [clang] Add clang trampoline attribute

[Aaron Ballman via Phabricator via llvm-commits]

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