augusto2112 added a subscriber: DavidSpickett.
augusto2112 added a comment.
> I'm coming around to maybe this is the best tradeoff, though not /super/
> enthusiastic - if there was some way to annotate in a way that's easier for
> users (like a bit on the function - "transparent" or something - that lets
> you call the function, but is sort of nodebug-ish (skip any calls that don't
> have debug info, step further into other functions that have this transparent
> attribute on them until you reach something with debug info)) - because
> there's probably a lot of functions in things like the standard library
> that'd be nice to skip over - or have the debugger skip over them in the same
> way (currently I think lldb hardcodes skipping over things in the std
> namespace, but fails to step into user code inside those calls - like
> `make_unique` - you can't step into the user's ctor, it skips over th ewhole
> thing - admittedly there you'd still need the whole implementation call chain
> to be annotated with transparent so the debugger knew which bits to step into
> V over)
I think your idea makes sense, and we could potentially have that as separate
attribute, if we think there's interest from the stdlib folks to annotate their
functions with it. It could even to lowered to the "flag" version of
DW_AT_trampoline.
================
Comment at: clang/include/clang/Basic/Attr.td:776
+ let Subjects = SubjectList<[Function, ObjCMethod]>;
+ let Args = [StringArgument<"Name">];
+ let Documentation = [TrampolineDocs];
----------------
erichkeane wrote:
> 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?
> 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.
I think we need to make this attribute fit the use case (compiler generated
code which can provide an unambiguous target the debugger should trampoline to)
instead of making it half fit a second use case (people using this attribute in
their real code), which would make it a bad attribute that doesn't really fit
either use case.
I can make it super clear, either by name, documentation, examples, what this
is meant for, and we could discuss a different attribute, similar to what
@DavidSpickett was talking about, which means "step through this function, and
check after every function call if we're in regular code, stop if yes", which
would we intended for people to use, without the drawback this one would have
for people (implementation drifting, users accidentally naming the wrong
function, etc).
Also, I'm not 100% how using expressions here would work. Lets say you have:
```
struct S {};
void foo(S s) {
}
void foo(int i) {}
__attribute__((trampoline_mangled_target(foo(?))))
void bar() {
S s;
foo(s);
}
```
What would be in place of the "?".
> 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?
If the function was inlined, there's debug info to indicate that, and the
debugger should handle it accordingly. If the function is completely deleted,
the debugger would step over it, which is already the behavior you get when
your function is deleted in optimized code.
> What if you're naming something without linkage?
You mean calling a static function defined in the same function? If we have
debug info for it, I believe it should still work.
================
Comment at: clang/include/clang/Basic/AttrDocs.td:6923
+
+Indicates to debuggers that stepping into ``foo`` should jump directly to
``bar`` instead.
+ }];
----------------
erichkeane wrote:
> I'm sure there is a compelling reason here, but if I ever saw this happen in
> a debugger, I'd get very confused.
The motivation is for compiler generated thunks that the user shouldn't be
aware about. I could update this description/example to make the use case
clearer.
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6775
+static void handleTrampolineMangledTarget(Sema &S, Decl *D,
+ const ParsedAttr &AL) {
----------------
erichkeane wrote:
> 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?
I'll double check the template case!
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
