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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.


================
Comment at: clang/include/clang/Basic/Attr.td:776
+  let Subjects = SubjectList<[Function, ObjCMethod]>;
+  let Args = [StringArgument<"Name">];
+  let Documentation = [TrampolineDocs];
----------------
augusto2112 wrote:
> aaron.ballman wrote:
> > 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?
> I understand the concerns you brought up as I didn't make it clear initially that this isn't supposed to be used by users, but meant to be used in compiler generated code. Given that, do you still think this should be an `Expr`? I think that'd be more error prone for tools to generate the correct C++ code to write, when compared with the mangled name.
Hyrem's law exists, and attributes are the proof of that one :)  Anything we provide in the attributes list gets used by users SOMEWHERE, and we need to do what we can to not have our interface be user hostile.

Also, this means that whatever code is generated needs to have the same mangled name... and then doesn't this get defeated by the inliner?  Can you name something that will later be removed?  What if you're naming something without linkage?


================
Comment at: clang/include/clang/Basic/AttrDocs.td:6923
+
+Indicates to debuggers that stepping into ``foo`` should jump directly to ``bar`` instead.
+  }];
----------------
I'm sure there is a compelling reason here, but if I ever saw this happen in a debugger, I'd get very confused.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6775
 
+static void handleTrampolineMangledTarget(Sema &S, Decl *D,
+                                          const ParsedAttr &AL) {
----------------
I don't see anything for instantiating this in a template? As it is, I think this attribute will be dropped if on a primary template. What about dependent arguments?

Should we prevent this from being placed on function templates?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146595