[PATCH] D76336: [DWARF] Emit DW_AT_call_pc for tail calls

[Vedant Kumar via Phabricator via llvm-commits]

vsk added a comment.

In D76336#1934743 <https://reviews.llvm.org/D76336#1934743>, @dblaikie wrote:

> In D76336#1934726 <https://reviews.llvm.org/D76336#1934726>, @vsk wrote:
>
> > In D76336#1934635 <https://reviews.llvm.org/D76336#1934635>, @dblaikie wrote:
> >
> > > In D76336#1934599 <https://reviews.llvm.org/D76336#1934599>, @vsk wrote:
> > >
> > > > Per @djtodoro's suggestion, avoid emitting DW_AT_call_pc when tuning for gdb. We already emit AT_call_return_pc in this case, which seems weird/wrong to me, but that's what gdb expects to see it seems.
> > >
> > >
> > > Does it work for GDB?
> >
> >
> > Yes, @djtodoro's second-to-last comment shows an example where gdb works out the address for an artificial frame using the "return pc".
> >
> > > If so, how? Please be sure there's clear documentation (probably in a somewhat long explanatory comment in the LLVM source itself) about why this divergence is justified
> >
> > I left a comment where DW_AT_call_pc is emitted explaining the situation: 'GDB prefers to work out what the call pc is by subtracting an offset from DW_AT_low_pc/DW_AT_call_return_pc'. I'll expand on this.
>
>
> Should we do that only in DWARFv4, and leave DWARFv5 standard & let GDB implement the missing functionality to understand the standard form?


Sure, I think this is a nice opportunity to reduce debugger-specific divergence. I'll include that in the next update.

> 
> 
>>> does GDB need this for providing some functionality, but less than is possible with DW_AT_call_pc, so LLDB wants that so it can provide the better experience, but providing only that would mean GDB would provide a worse experience than if it has return_pc?
>> 
>> gdb works backwards from non-standard usage of DW_AT_low_pc/DW_AT_call_return_pc at tail-call site entries to figure out the PC of tail-calling branch instructions. This means it doesn't need the compiler to emit DW_AT_call_pc, so we don't emit it (this change happened in my last patch update -- incidentally this means this patch no longer has any effect on debug info emission when tuning for gdb).
> 
> The language in the DWARF spec is, as always, pretty vague/general. But, yes, the call_pc wording in the spec (which includes a mention of jumps/tail calls) seems to be in contrast to the call_return_pc that only mentions calls, so I get where you're coming from.
> 
>> However, there isn't a good reason to tie non-gdb debuggers to this non-standardness, as it adds unnecessary complexity to the debugger. It forces the debugger to disassemble around the fake "return pc" to find the actual tail-calling branch. In the case where the tail-calling branch is the last instruction in the function, I'm not sure how that would work, as the fake "return pc" wouldn't necessarily point anywhere meaningful. To side-step that, we emit DW_AT_call_pc for non-gdb debuggers.
> 
> the "I'm not sure how that would work" bit is sort of concerning to me - it does work for GDB, right? (I'd guess it points to the end of the instruction (same as the "high_pc" of a function points to the end of (or, one passed the end in both cases) of the function))

I was hedging a bit because I haven't/can't read the gdb sources. I expect that the implementation does something to the effect of:

  tail_call_pc = call_return_pc-1;
  tail_call_pc -= sizeOfInstAt(tail_call_pc)

in which case handling a tail call at the end of a function shouldn't be problematic. Part of why I hedged is that I don't know what impact (if any) post-link function reordering tools might have on DW_AT_call_return_pc. Hypothetically, if the encoded address is one past the end of caller function, and the post-link tool fastidiously updates addresses in DWARF sections which point within moved functions, the non-standard trick gdb uses would stop working (otoh DW_AT_call_pc would get updated correctly).

> I'm not going to/don't mean to hold any of this work up, just curious.
> 
> I guess if GDB does add support for call_pc, it'll likely keep its extension support of call_return_pc as well so it'll keep working with the non-standard emission for GDB you're proposing, and then we can fix-forward & remove this workaround. Has someone filed the bug on GDB for this missing functionality?

I did a quick scan of the gdb bug database but couldn't find anything relevant. If anyone following along can confirm gdb hasn't added support for DW_AT_call_pc, I'd appreciate it if you could file a bug.


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

https://reviews.llvm.org/D76336