The `debug_trampoline ` attribute is intended as a hint for debuggers that this function itself is not interesting, but it calls a function that might be. So, when stepping in arrives at a function with this attribute, debuggers should transparently step-in through it into the functions called by the annotated function (but not by subsequent calls made by those functions), stopping at the first one its normal rules for whether to stop says to stop at - or stepping out again if none qualify. Also, when stepping out arrives at a function with this attribute, the debugger should continue stepping out to its caller.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
So this attribute will lower into a DW_AT_trampoline("target_func_name") attribute on the DW_TAG_subprogram of the function definition?
The debug info parts LGTM. It would be nice to hear some extra confirmation that we're fine with this specific attribute name.
llvm/include/llvm/IR/DIBuilder.h | ||
---|---|---|
802 ↗ | (On Diff #507186) | Why did you need to add this? Does flang use a different createMethod() method? |
Exactly, there's already functionality to lower the targetFuncName in a DISubprogram into dwarf as a DW_AT_trampoline. This threads the clang attribute down to LLVM IR.
llvm/include/llvm/IR/DIBuilder.h | ||
---|---|---|
802 ↗ | (On Diff #507186) | Yes, there's an existing createFunction which already takes in a TargetFuncName, which I assume is all that flang uses. I'm extending this to createMethod so we support this attribute on methods as well. |
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.
clang/include/clang/Basic/Attr.td | ||
---|---|---|
775 | 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)). | |
777 | This is quite fragile, for a few reasons.
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? | |
clang/include/clang/Basic/AttrDocs.td | ||
6908 | No need to specify a heading, one is picked automatically because this attribute only has one spelling. | |
7013–7014 | Spurious whitespace changes. | |
clang/lib/Sema/SemaDeclAttr.cpp | ||
4885 ↗ | (On Diff #507422) | Nothing creates this attribute implicitly, so you should drop this bit. |
Yeah, this seems a bit burdensome for both the frontend (encoding more information, the author has to duplicate the name of the implementation function so there's a risk they get out of sync?) and the debugger (is the debugger expected to jump directly and not call the implementation? Is it expected to continue into the implementation and break at the function? What if the function isn't called, or there's more than one call in the function, etc?)...
What about function overloading & namespaces? Is the debugger expected to figure out which bar function the DWARF/user is referring to?
I'd expect attribute((nodebug)) on the function, maybe, or some other single attribute that doesn't require retyping the implementation detail - attribute((debug_strep_transparent)) or the like?
So looking at the DWARF spec for this - I see what it's going for, but yeah, I wouldn't want to use a textual representation either at the source or bitcode level, ideally... - perhaps this should just be implemented in LLVM when something's being lowered to a trampoline we could emit DWARF that describes that, without needing source annotations? Or are there situations where it depends on how the user wrote the code as to how we want the debugger to dehave?
If it's the latter, then, yeah, some "transparent to debugger" source attribute might be appropriate - that lowers to a bit on the DISubprogram and instructs LLVM to, if the function definition ends up lowering to a trampoline, mark that for the debugger so it's transparent - and if it lowers to a trampoline but doesn't have the source attribute, you can then still step into the function separately from stepping into the subsequent function?
This is quite fragile, for a few reasons.
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.
How does this work with overloaded functions? Templated functions with specializations?
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?
Yeah, this seems a bit burdensome for both the frontend (encoding more information, the author has to duplicate the name of the implementation function so there's a risk they get out of sync?)
So looking at the DWARF spec for this - I see what it's going for, but yeah, I wouldn't want to use a textual representation either at the source or bitcode level, ideally... - perhaps this should just be implemented in LLVM when something's being lowered to a trampoline we could emit DWARF that describes that, without needing source annotations? Or are there situations where it depends on how the user wrote the code as to how we want the debugger to dehave?
Right, I probably should've been clearer on this: the attribute is meant to be used mainly in compiler generated code, not really to be used directly by people (similar to how external_source_symbol is used). The main motivation is to annotate the C++ header file that the Swift compiler emits to enable interop with C++. I'd be happy to make the attribute name really specific (as it shouldn't be something users need to type very often), and make it clear on the documentation that this is meant to be used by tooling.
What about function overloading & namespaces? Is the debugger expected to figure out which bar function the DWARF/user is referring to?
The symbol name that's passed in the attribute is supposed to be the mangled name, so there shouldn't be any ambiguity to resolve.
is the debugger expected to jump directly and not call the implementation? Is it expected to continue into the implementation and break at the function? What if the function isn't called, or there's more than one call in the function, etc?)...
I'm also working on an lldb patch to support the DW_AT_trampoline attribute, where we could discuss the details on how the behavior of the debugger should be, but the way I'm imagining it right now would be that stepping into a trampoline should step through directly to the trampoline target instead, and stepping out of the trampoline target should also step out of the trampoline as well . If the function isn't called, maybe it should behave like it does for functions with no debug info, where we immediately step out of the trampoline. If there's more than one call to the trampoline target, I'm not sure what should happen, I guess we'd step into the trampoline target multiple times (that'd be a pretty odd trampoline though). We could also have a setting where you can enable/disable the trampoline stepping behavior.
clang/include/clang/Basic/Attr.td | ||
---|---|---|
775 |
Ok, I'll update it to the Clang spelling.
What about debugger_trampoline_target? debugger_trampoline_mangled_target? |
If it's the latter, then, yeah, some "transparent to debugger" source attribute might be appropriate - that lowers to a bit on the DISubprogram and instructs LLVM to, if the function definition ends up lowering to a trampoline, mark that for the debugger so it's transparent - and if it lowers to a trampoline but doesn't have the source attribute, you can then still step into the function separately from stepping into the subsequent function?
Since the glue code the Swift compiler emits may call an arbitrary number of functions before calling the one function the developer is interested in, a single bit would make it quite difficult for a debugger to understand which function is the "interesting" one. The advantage of DW_AT_trampoline is that it completely eliminates any ambiguity about where to continue stepping.
What about function overloading & namespaces? Is the debugger expected to figure out which bar function the DWARF/user is referring to?
The symbol name that's passed in the attribute is supposed to be the mangled name, so there shouldn't be any ambiguity to resolve.
Should there be some check that that symbol exists? (the current test uses the unmangled name "bar" for instance - which gives the wrong impression?)
is the debugger expected to jump directly and not call the implementation? Is it expected to continue into the implementation and break at the function? What if the function isn't called, or there's more than one call in the function, etc?)...
I'm also working on an lldb patch to support the DW_AT_trampoline attribute, where we could discuss the details on how the behavior of the debugger should be, but the way I'm imagining it right now would be that stepping into a trampoline should step through directly to the trampoline target instead, and stepping out of the trampoline target should also step out of the trampoline as well . If the function isn't called, maybe it should behave like it does for functions with no debug info, where we immediately step out of the trampoline. If there's more than one call to the trampoline target, I'm not sure what should happen, I guess we'd step into the trampoline target multiple times (that'd be a pretty odd trampoline though). We could also have a setting where you can enable/disable the trampoline stepping behavior.
Helpful to discuss the semantics here - or somewhere in a generic sense before we go adding the features to compiler or debugger, since what alternatives to consider depends on the desired functionality.
What would happen if, instead, these trampolining functions were annotated nodebug? I guess then you wouldn't have the top level one as an entry point for the user, as there would be no debug info for it?
Should there be some check that that symbol exists? (the current test uses the unmangled name "bar" for instance - which gives the wrong impression?)
You mean as part of the attribute validation?
What would happen if, instead, these trampolining functions were annotated nodebug? I guess then you wouldn't have the top level one as an entry point for the user, as there would be no debug info for it?
These trampoline functions can call other functions as part of their setup, so annotating them with nodebug wouldn't guarantee that the debugger would be able to step through to the correct place. Additionally, you wouldn't be able to refer to them in LLDB expression evaluation either.
Right
What would happen if, instead, these trampolining functions were annotated nodebug? I guess then you wouldn't have the top level one as an entry point for the user, as there would be no debug info for it?
These trampoline functions can call other functions as part of their setup, so annotating them with nodebug wouldn't guarantee that the debugger would be able to step through to the correct place. Additionally, you wouldn't be able to refer to them in LLDB expression evaluation either.
Oh, that's a bit worrying - that these aren't trampolines in the sort of lower-level sense, only in a high level sense.
Is the ability to debug these functions under some debugger flag ("show me intermediate steps/implementation details") a priority? Would it be OK if that were only available with a rebuild (ie: maybe didn't need to be represented in DWARF - this would also allow debug info to be smaller & not having to describe these transparent/implementation detail functions)?
For instance std::move/std::forward got turned into compiler intrinsics and so have no debug info impact anymore - you can't step into the std::move implementation at all, even at -O0. It'd be nice if some other standard library functions maybe could have this behavior - though the callability is an issue which would be different for different functions. (I guess non-callability is already covered by nodebug)
I guess the debug info size benefit couldn't be attained while providing callability...
Is there a single public entry point in most cases? How bad would it be if the implementation functions were all nodebug, but the public entry point and underlying function had debug info? Would that provide most/enough of the benefits you're after?
What would happen if, instead, these trampolining functions were annotated nodebug? I guess then you wouldn't have the top level one as an entry point for the user, as there would be no debug info for it?
These trampoline functions can call other functions as part of their setup, so annotating them with nodebug wouldn't guarantee that the debugger would be able to step through to the correct place. Additionally, you wouldn't be able to refer to them in LLDB expression evaluation either.
Oh, that's a bit worrying - that these aren't trampolines in the sort of lower-level sense, only in a high level sense.
Is the ability to debug these functions under some debugger flag ("show me intermediate steps/implementation details") a priority? Would it be OK if that were only available with a rebuild (ie: maybe didn't need to be represented in DWARF - this would also allow debug info to be smaller & not having to describe these transparent/implementation detail functions)?
You mean step in the trampoline? It'd be nice, but not absolutely required. The main issue would be not being able to evaluate expressions though.
Is there a single public entry point in most cases? How bad would it be if the implementation functions were all nodebug, but the public entry point and underlying function had debug info? Would that provide most/enough of the benefits you're after?
Let me make sure I understand your suggestion, you mean we'd have three functions?
void target() { // do stuff } // attribute (nodebug) void trampoline() { // do setup target() // do cleanup } void entrypoint() { trampoline() }
Wouldn't users still have to step into the compiler generated entrypoint, and step in more to get to target?
Also, depending the complexity of the trampoline algorithm, a step in still isn't guaranteed to stop in the target function: I think, as it's implemented right now, LLDB stepping into a function with no debug info will try to follow the first function call, and step out if that isn't somewhere with no debug info. We could change the step in algorithm, but a function with no debug info isn't necessarily a trampoline, and could have a chain of calls to other functions without debug info as well, and having the debugger stop at every one of those calls could be pretty expensive. This solution would be more performant, as we could set a private breakpoint in the target and continue the program.
Sorry if I misunderstood your suggestion.
clang/include/clang/Basic/Attr.td | ||
---|---|---|
777 | 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. |
Right - yep yep.
Is there a single public entry point in most cases? How bad would it be if the implementation functions were all nodebug, but the public entry point and underlying function had debug info? Would that provide most/enough of the benefits you're after?
Let me make sure I understand your suggestion, you mean we'd have three functions?
void target() { // do stuff } // attribute (nodebug) void trampoline() { // do setup target() // do cleanup } void entrypoint() { trampoline() }Wouldn't users still have to step into the compiler generated entrypoint, and step in more to get to target?
Yep - wondering just how bad that'd be - how much abstraction you're trying to get past, etc.
Also, depending the complexity of the trampoline algorithm, a step in still isn't guaranteed to stop in the target function: I think, as it's implemented right now, LLDB stepping into a function with no debug info will try to follow the first function call, and step out if that isn't somewhere with no debug info. We could change the step in algorithm, but a function with no debug info isn't necessarily a trampoline, and could have a chain of calls to other functions without debug info as well, and having the debugger stop at every one of those calls could be pretty expensive. This solution would be more performant, as we could set a private breakpoint in the target and continue the program.
True - being unable to tell whether there's something interesting in there or not does complicate things.
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)
clang/include/clang/Basic/Attr.td | ||
---|---|---|
777 | 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? | |
clang/include/clang/Basic/AttrDocs.td | ||
6926 | I'm sure there is a compelling reason here, but if I ever saw this happen in a debugger, I'd get very confused. | |
clang/lib/Sema/SemaDeclAttr.cpp | ||
6775 ↗ | (On Diff #507922) | 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'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.
clang/include/clang/Basic/Attr.td | ||
---|---|---|
777 |
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 "?".
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.
You mean calling a static function defined in the same function? If we have debug info for it, I believe it should still work. | |
clang/include/clang/Basic/AttrDocs.td | ||
6926 | 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. | |
clang/lib/Sema/SemaDeclAttr.cpp | ||
6775 ↗ | (On Diff #507922) | I'll double check the template case! |
clang/lib/Sema/SemaDeclAttr.cpp | ||
---|---|---|
6775 ↗ | (On Diff #507922) | Could you elaborate what potential problem you suspect with templates? I couldn't find any problems when using this in templated functions. The attribute should only work on functions/methods ("SubjectList<[Function]>") so we shouldn't need to worry about it being applied to other types. |
I had one question about this:
I think through the comments you've made it quite clear that the intent is for this to be used by compiler-generated code (i.e. "tooling"). Given that even tools have bugs, what does the debugging/remediation story look like when a tool generates a "bad" argument for trampoline_mangled_target? My understanding is that you can mark any arbitrary function as a trampoline for any other arbitrary symbol, even ones that don't exist in your compilation unit (so clang or llvm may not be able to verify that your trampoline "target" is correct). This may be even challenging for the dsymutil (and other debug info "linkers") because it's possible the symbol isn't described in debugging information. I suppose then it would fall on LLDB to detect these things? Since LLDB is meant to place a breakpoint on the trampoline target, would we emit a warning when LLDB has no idea where to place the breakpoint?
Not trying to cause trouble or disagree with the idea, but I would like to better understand what we can do to detect a failure.
I know this is a bit of a redirection/scope creep/etc - but I'd quite like to see a solution that is likely to be usable for the "std::function" problem (stepping into std::function should allow you to reach the underlying function - but lldb currently skips any call to a std-namespaced function, I think, so you step right over the whole op() call) that could also cover the Swift needs. Though perhaps they're just sufficiently different problems that there is no generalizing here.
This patch aims at exposing an existing LLVM IR & DWARF feature in clang. Having a generic solution for the std::function problem is definitely worthwhile investigating, but I'm not sure it needs to prevent supporting the existing mechanism in clang.
I understand that this is nowhere near as flexible as a source-level solution, but in case you weren't aware of this, LLDB implements a custom step through plan for std::function:
Why should this feature be limited to DWARF? Don't we have the same kind of stepping behavior issue with PDB files, for example?
Existing LLVM IR feature? *goes to check history* Ah, I see - hadn't realized at least at the IR level this was already implemented. (Anyone got experience with how this works in GCC/GDB then? I guess it was implemented there first/ported to clang with the fortran effort? So that might answer some of the questions about how we should implement it in LLVM and Clang, and about the precedent for behavior of the corner cases?)
Having a generic solution for the std::function problem is definitely worthwhile investigating, but I'm not sure it needs to prevent supporting the existing mechanism in clang.
I understand that this is nowhere near as flexible as a source-level solution, but in case you weren't aware of this, LLDB implements a custom step through plan for std::function:
Ah, might've been an overreach/misassumption on my part - I thought we came across this somewhere, but it might've been std::make_unique or some other standard library code that didn't have this special casing (can imagine various emplace functions, etc, where you might want to step into the ctor too).
There's already support to threading trampoline names from IR to DWARF. If PDB supports the same attribute in some form there's nothing stopping someone to adding support to thread the attribute from IR to PDB as well.
We don't want to have one attribute per debug format, because that's not going to scale. So how do we validate that this attribute should be exposed to users *now* before we know what the story is for other debug formats? The argument "this is for compiler-generated code" doesn't really address the concern I have here -- once the attribute exists, people will start using it if it does useful things. The problem this attribute solves is pretty general to a lot of different kinds of library facilities (at least in C++, a bit less so in C IMO), so it seems that hypothetical situation is plausible. We can document that this only works with DWARF, but that's still not actually solving the problem users have.
I guess what I'm trying to say is: this feels like a specific solution to a general problem and that makes me worried we're painting ourselves into a corner where we're going to need to deprecate this attribute and add the general one in the future. How likely do you think that is (might be more of a question for @dblaikie from the debug info side of things) and do you agree that it seems like users would want this functionality for their own libraries?
I guess it was implemented there first/ported to clang with the fortran effort
Yeah my understanding is that the LLVM feature was added for flang, and so I'm not sure what the targeted debugger is, I believe there are some non-GDB/LLDB debuggers targeting the HPC market that might implement this.
Why should this feature be limited to DWARF? Don't we have the same kind of stepping behavior issue with PDB files, for example?
That's not what I was trying to say. Yes, DW_AT_trampoline is a DWARF feature. But the "target function" LLVM IR feature is not necessarily limited to lowering into DWARF. If someone wants to implement lowering of the LLVM IR feature to PDB, and PDB supports some equivalent constructs, they can certainly do so.
I just wanted to point out that the goal of this patch is to give Clang users access to an already existing LLVM (& DWARF) feature that is being used by other frontends (flang) and supported by other debuggers. While I agree with David's general point that the feature is somewhat limited in its design, most of the design choices ultimately come from the DWARF specification.
We don't want to have one attribute per debug format, because that's not going to scale.
LLVM supports outputting a total of 2 debug info formats.
So how do we validate that this attribute should be exposed to users *now* before we know what the story is for other debug formats?
Do you have any other debug info formats in mind? To my knowledge LLVM doesn't support lowering all of the the debug info metadata into PDB because there are some corners where it is less expressive than DWARF.
I believe that the closest equivalent to this feature in PDB is a feature called "just my code". A web search came up with https://learn.microsoft.com/en-us/visualstudio/debugger/just-my-code?view=vs-2022 and it seems like it's powered by an external database of function names. Would be interesting to hear from someone with knowledge and an interest in PDB about this.
Would you say that we shouldn't create this attribute if it cannot be supported on Windows? Or would documenting that the attribute only has an effect on DWARF platforms be sufficient?
Why should the user have to write two separate attributes that do the same thing?
So how do we validate that this attribute should be exposed to users *now* before we know what the story is for other debug formats?
Do you have any other debug info formats in mind? To my knowledge LLVM doesn't support lowering all of the the debug info metadata into PDB because there are some corners where it is less expressive than DWARF.
I believe that the closest equivalent to this feature in PDB is a feature called "just my code". A web search came up with https://learn.microsoft.com/en-us/visualstudio/debugger/just-my-code?view=vs-2022 and it seems like it's powered by an external database of function names. Would be interesting to hear from someone with knowledge and an interest in PDB about this.
It's less about other debug formats and more about user experience. My two big concerns are: 1) writing multiple attributes to do the same thing is somewhat user-hostile unless "the same thing" is actually different in some way users will care about, 2) we have a policy that user-facing attributes that do nothing are diagnosed as being ignored, so if we don't support Windows for this attribute, then compiling on that platform is going to generate a pile of "attribute ignored" warnings. If we can support the attribute with PDB as well, that solves both of my big concerns because it makes this attribute generally useful (which is what we aim for with attributes when possible).
Would you say that we shouldn't create this attribute if it cannot be supported on Windows? Or would documenting that the attribute only has an effect on DWARF platforms be sufficient?
I don't want to sign y'all up for more work you can't easily do yourself, so I don't want to block on support for Windows unless it turns out to be completely trivial to support. But from a review POV, I'd like to hear back from someone who knows something about PDB to validate that the attribute doesn't require a breaking change for support there (like going from taking no args to requiring an arg for some reason, basically). If we don't think breaking changes should be needed, then I think documentation + diagnostics will be sufficient for me.
I did have two questions though:
What happens when stepping into the attributed function through a function pointer?
Are there any special situations where a function cannot/should not be marked with the attribute (virtual functions, lambda, blocks)?
That's a valid concern. However, in its current form it would still get lowered into LLVM IR, and LLVM may or may not silently ignore the attribute when lowering to PDB. It could be argued that that's even worse than warning about an ignored attribute. I just wanted to point this out.
I don't want to sign y'all up for more work you can't easily do yourself, so I don't want to block on support for Windows unless it turns out to be completely trivial to support. But from a review POV, I'd like to hear back from someone who knows something about PDB to validate that the attribute doesn't require a breaking change for support there (like going from taking no args to requiring an arg for some reason, basically). If we don't think breaking changes should be needed, then I think documentation + diagnostics will be sufficient for me.
I did have two questions though:
What happens when stepping into the attributed function through a function pointer?
Are there any special situations where a function cannot/should not be marked with the attribute (virtual functions, lambda, blocks)?
@rnk Do you happen to know anything about how PDB handles these?
That's the same for basically any LLVM IR metadata/attributes. We try our best from the FE to warn users if we know something is going to be ignored, but we don't have a diagnostic that is *guaranteed* to fire if the attribute doesn't do anything useful (then you run into all sorts of fun situations like: what if the attribute that does nothing is written on a function that is never called and so it's dead code stripped away?).
I don't want to sign y'all up for more work you can't easily do yourself, so I don't want to block on support for Windows unless it turns out to be completely trivial to support. But from a review POV, I'd like to hear back from someone who knows something about PDB to validate that the attribute doesn't require a breaking change for support there (like going from taking no args to requiring an arg for some reason, basically). If we don't think breaking changes should be needed, then I think documentation + diagnostics will be sufficient for me.
I did have two questions though:
What happens when stepping into the attributed function through a function pointer?
Are there any special situations where a function cannot/should not be marked with the attribute (virtual functions, lambda, blocks)?@rnk Do you happen to know anything about how PDB handles these?
CC @akhuang @zturner @bwyma as other folks who might have ideas here as well (or know folks who do).
How is this attribute going to handle a trampoline that performs a virtual dispatch from C++ call into Swift? In that case, the target is not known.
@aaron.ballman @dblaikie I've changed the patch to introduce a more general "transparent_stepping" attribute, which doesn't take in a string (similar to what David suggested), please take a look and let me know what you think.
I hope I'm not kicking off a long bike-shedding thread, but I would propose to either call the attribute transparent to match the DWARF attribute name, or if we want to be more descriptive, debug_transparent, or transparentdebug to fit better with other attributes such as nodebug. My vote is on transparent.
The dwarf attribute is actually called trampoline, not transparent. trampoline is a pretty generic name which could be used for other things other than debugging, so maybe trampolinedebug?
I think debug_trampoline both captures the semantics and makes it clear that this is related to debugging.
I'd be fine with this, or transparent_debug_stepping, etc. Just something other than transparent or trampoline as those are rather generic terms.
Additionally, should you be allowed to write this on a lambda to skip over the function call operator? (If so, you may need to move the attribute from the lambda declaration onto the function call operator.)
clang/include/clang/Basic/Attr.td | ||
---|---|---|
776 | ObjC method decls as well? | |
clang/lib/CodeGen/CGDebugInfo.cpp | ||
72 | How about something like this ("get is" just sounds weird to me)? | |
clang/lib/Sema/SemaDeclAttr.cpp | ||
6773–6777 ↗ | (On Diff #509810) | Can be removed entirely if we don't have any custom checking logic for it. Instead, do let SimpleHandler = 1; in the attribute definition in Attr.td. |
9090–9092 ↗ | (On Diff #509810) | This can also be removed. |
Is there some place (bug, discourse thread, etc) where the broader direction is discussed? I want to checkin on the design decisions/alternatives without fragmenting this across multiple reviews/losing context/etc?
(specifically - this started out with the trampoline attribute, then switched to this transparent idea (perhaps based on my feedback? Also other feedback? I'd like to know more about how that change in direction happened, what the tradeoffs were, etc - I don't think my suggestion alone was probably enough to make this direction clearly the right one (nor clearly the wrong one)), etc)
& there was some tangent about DWARF v COFF too, which I wouldn't mind weighing in on, but feel like it's all a bit fragmented, so not sure where all the discussions are/how to keep track of them.
Is there some place (bug, discourse thread, etc) where the broader direction is discussed? I want to checkin on the design decisions/alternatives without fragmenting this across multiple reviews/losing context/etc?
No, I believe that all the relevant discussion happened in this review. While there is a separate review for an LLDB implementation, there is no general direction discussion there, it's just implementing a DWARF feature.
(specifically - this started out with the trampoline attribute, then switched to this transparent idea (perhaps based on my feedback? Also other feedback? I'd like to know more about how that change in direction happened, what the tradeoffs were, etc - I don't think my suggestion alone was probably enough to make this direction clearly the right one (nor clearly the wrong one)), etc)
It was partially based on your feedback, but also on @arphaman pointing out that the DW_AT_trampoline("call_target") implementation wouldn't be able to deal with the jump target being a virtual function call. So @augusto2112
landed on implementing the "flag variant" of DW_AT_trampiline instead. This is also an existing DWARF feature, albeit not yet supported by LLVM.
& there was some tangent about DWARF v COFF too, which I wouldn't mind weighing in on, but feel like it's all a bit fragmented, so not sure where all the discussions are/how to keep track of them.
That was also in this review; @aaron.ballman pointed out that it would be best if new Clang attributes weren't targeting only DWARF, though I believe this request may run into some hard limitations of what CodeView/PDB can support.
@aaron.ballman thanks all the code reviews.
I checked this, and it already attaches the debug flag to the call operator in the IR, so there shouldn't be any changes necessary.
clang/include/clang/Basic/Attr.td | ||
---|---|---|
776 | Added objc methods (and tests) as well. |
The Clang attribute bits generally LG, but I don't know enough on the debug info bits to say. In terms of behavior in PDB files, I'd still like for this attribute to be supported across all debug info formats. If we know this attribute is going to be ignored for a particular debug format, I think we should issue the usual "attribute ignored" diagnostics so that users are not surprised the attribute does nothing (I think the metric for this should be "can the attribute ever do what the user expects on this target?" -- so not using -g wouldn't issue the attribute ignored diagnostic on a target using DWARF because the attribute could do what the user expects there if the user enabled debug info). But that said, I'll defer to what the debug info code owners think is the best approach to take.
clang/test/CodeGen/attr-debug-trampoline-method.cpp | ||
---|---|---|
15 | Can you also show a use on a lambda to demonstrate we codegen that properly? | |
clang/test/Sema/attr-debug-trampoline.c | ||
1 | I'd probably drop this test file entirely as the C++ file covers the testing needs (we don't usually ask for tests duplicated across spellings unless the spelling is important for some reason). | |
clang/test/SemaCXX/attr-debug-trampoline-method.cpp | ||
11 | Can you also add tests like: [[clang::debug_trampoline]] int var; // Error, not allowed on something other than a function [[clang::debug_trampoline]] void free_function(); // OK typedef void (*fp)() [[clang::debug_trampoline]]; // Error, not allowed on a type |
FWIW, trampoline seems weird to me (both for the DWARF attribute, and the source level attribute) - my understanding is that trampolines are small functions that just immediately jump to some other function. In this case the intermediate function can be arbitrarily complicated, but just not /interesting/ to the average developer. Doesn't seem like a trampoline to me. (though maybe I'm thinking of a "thunk" specifically - at least wikipedia's deefinition of a programming trampoline is fairly broad/involves some fairly higher level "do some non-trivial things" operations, I guess)
I guess an example of something that might be transparent but not trampoline-like would be std::sort - the ability to step into the comparison function, but skip over the actual sorting logic (or for_each, etc - you might want to step into your functor, but skip the intermediate logic that's handling the looping, etc). Those don't feel like trampolines, I guess?
Thanks, sorry, gets all a bit confusing sometimes :/
(specifically - this started out with the trampoline attribute, then switched to this transparent idea (perhaps based on my feedback? Also other feedback? I'd like to know more about how that change in direction happened, what the tradeoffs were, etc - I don't think my suggestion alone was probably enough to make this direction clearly the right one (nor clearly the wrong one)), etc)
It was partially based on your feedback, but also on @arphaman pointing out that the DW_AT_trampoline("call_target") implementation wouldn't be able to deal with the jump target being a virtual function call. So @augusto2112
landed on implementing the "flag variant" of DW_AT_trampiline instead. This is also an existing DWARF feature, albeit not yet supported by LLVM.
OK
& there was some tangent about DWARF v COFF too, which I wouldn't mind weighing in on, but feel like it's all a bit fragmented, so not sure where all the discussions are/how to keep track of them.
That was also in this review; @aaron.ballman pointed out that it would be best if new Clang attributes weren't targeting only DWARF, though I believe this request may run into some hard limitations of what CodeView/PDB can support.
Yeah, that seems like a bit of a big ask, since there's a specific DWARF feature we're lowering to here - feels out of scope to go trying to figure out how it might be implemented in CodeView. For features that are format-agnostic (like simple template names, no-standalone-debug (various type homing strategies), etc) yeah, makes total sense for them to be format-agnostic. This isn't one of those, though.
but, I guess, worth at least asking the question if there's an equivalent feature in CV already that this should be mapped to
I think I agree. If we go with some kind of transparent we have the options of
- transparent (which is quite broad)
- transparentdebug (which follows the schema started by nodebug) I think this is ugly
- transparent_debug (which is oddly inconsistent with nodebug but more readable)
- debug_transparent (which starts a new nomenclature)
My preferential vote is 4 (best), 2, 3, 1.
& there was some tangent about DWARF v COFF too, which I wouldn't mind weighing in on, but feel like it's all a bit fragmented, so not sure where all the discussions are/how to keep track of them.
That was also in this review; @aaron.ballman pointed out that it would be best if new Clang attributes weren't targeting only DWARF, though I believe this request may run into some hard limitations of what CodeView/PDB can support.
Yeah, that seems like a bit of a big ask, since there's a specific DWARF feature we're lowering to here - feels out of scope to go trying to figure out how it might be implemented in CodeView. For features that are format-agnostic (like simple template names, no-standalone-debug (various type homing strategies), etc) yeah, makes total sense for them to be format-agnostic. This isn't one of those, though.
but, I guess, worth at least asking the question if there's an equivalent feature in CV already that this should be mapped to
I already tried to research this — see my comment above "just my code" above. My layman understanding is that they use an external database of uninteresting symbols.
Now if we are going to say this explicitly DWARF only feature, that could be an argument to stick with the trampoline name. But I'd rather say this is a general feature that — at this point in time — is only implement on DWARF targets.
We do already have https://clang.llvm.org/docs/AttributeReference.html#standalone-debug - so maybe (3) is the way to go? but worst comes to worst we could rename standalone_debug to debug_standalone and deprecate the old spelling if it's worth it.
& there was some tangent about DWARF v COFF too, which I wouldn't mind weighing in on, but feel like it's all a bit fragmented, so not sure where all the discussions are/how to keep track of them.
That was also in this review; @aaron.ballman pointed out that it would be best if new Clang attributes weren't targeting only DWARF, though I believe this request may run into some hard limitations of what CodeView/PDB can support.
Yeah, that seems like a bit of a big ask, since there's a specific DWARF feature we're lowering to here - feels out of scope to go trying to figure out how it might be implemented in CodeView. For features that are format-agnostic (like simple template names, no-standalone-debug (various type homing strategies), etc) yeah, makes total sense for them to be format-agnostic. This isn't one of those, though.
but, I guess, worth at least asking the question if there's an equivalent feature in CV already that this should be mapped to
I already tried to research this — see my comment above "just my code" above. My layman understanding is that they use an external database of uninteresting symbols.
Now if we are going to say this explicitly DWARF only feature, that could be an argument to stick with the trampoline name. But I'd rather say this is a general feature that — at this point in time — is only implement on DWARF targets.
Yeah, I'd be inclined to think of this as not a DWARF-specific feature, but one only implemented in DWARF at the moment. If CV adds some way to inject names into the database of uninteresting symbols, they could use this too.
To clarify, my concerns with non-DWARF aren't of the "this needs to work with every debug format before we should accept it" variety. It's more of the "if other debug formats support this concept or conceivably would support this concept in the near future, would we need to materially change the attribute from what's proposed here?" kind. Basically, I want to try to avoid a situation where we have to introduce a second attribute for this same feature, or this attribute needs to be a decl attribute for DWARF and a type attribute for others, needs arguments for some reason, that sort of thing. I want to avoid having a set of attributes for DWARF and a different set of attribute code CodeView, basically.
Fair - we DWARF folks don't probably have enough context to answer that authoritatively. I'm not sure if the CV folks do either (in the sense of knowing all the corners of a less public format is harder - so the usual challenges of proving non-existence come up more), but @akhuang - do you happen to know anything about CV's ability to encode places not to stop when stepping through code?
If we can't find this information out in the next few days, then I'd say it's reasonable to move forward without worrying about the behavior of other debug formats. If we run into a problem in the future, we can deprecate the attribute and replace it with something sufficiently general then.
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)).