[PATCH] D146595: [clang] Add "debug_trampoline" attribute

Tue Apr 18 12:31:32 PDT 2023

dblaikie added a comment.

In D146595#4235340 <https://reviews.llvm.org/D146595#4235340>, @augusto2112 wrote:

> In D146595#4235333 <https://reviews.llvm.org/D146595#4235333>, @aprantl wrote:
>
>> 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`?

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?

In D146595#4241852 <https://reviews.llvm.org/D146595#4241852>, @aprantl wrote:

>> 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.

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146595