[PATCH] D68465: [DebugInfo] Trim call-clobbered location list entries when tuning for GDB

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

In D68465#1697407 <https://reviews.llvm.org/D68465#1697407>, @dstenb wrote:

> In D68465#1695482 <https://reviews.llvm.org/D68465#1695482>, @dblaikie wrote:
>
> > Thanks for bringing this up!
> >
> > A few thoughts from me:
> >
> > 1. Yeah, I tend to agree with the DWARF Committee folks & the fact that LLDB can do the right thing without this change sort of points to this being a "fix it in GDB" situation. Have you tried asking the GDB folks about it/submitting patches there rather than here?
>
>
> No, we have not done that yet.


I think it'd be worthwhile having at least a statement from GDB that they feel this should be the responsibility of the producer. Though even if that's the answer they provide - I think some amount of pushback (especially given the existence proof of LLDB's behavior, by the sounds of it/if I'm understanding you correctly) might be worthwhile.

>> 2. Do you have a small example of GCC producing this kind of output?
> 
> 
> 
>   extern int value(void);
>   extern void call(int);
>   
>   int main() {
>     int local = value();
>     call(local);
>     return 0;
>   }
> 
> 
> compiled using -O1 -g with GCC 8.3.0 gives:
> 
>   (gdb) info addr local
>   Symbol "local" is multi-location:
>     Range 0x5555555550ee-0x5555555550f4: a variable in $rax
>   .
>   (gdb) disas main
>   [...]
>      0x00005555555550f0 <+11>:	callq  0x5555555550ff <call>
>      0x00005555555550f5 <+16>:	mov    $0x0,%eax
>   [...]
> 
> 
> As seen, the location list entry ends one byte before the return address.

Cool - thanks!

>> 3. The ability to use an offset from a debug_addr entry is actually something that's quite desirable - but not quite in the way you're suggesting.

OK, I've looked through the code some more & understand a little better.

So your changes to the address pool don't actually cause the address pool to contain entries with offsets - it stores only the base address in the actual debug_addr pool output, but then uses the offset from there in the place that refers to the address pool.

So I think that would mean there would end up with duplicate entries in debug_addr, which would be a waste of space/relocations/etc.

So only the address should go in the pool - the pool shouldn't be aware of the offset. (this would mean the semantics of the in-memory data structures would match more closely to the output)

So the DebugLocStream::Entry's Begin/End could be SymbolWithOffset - without having the AddressPool having any knowledge of these offsets, just the symbol itself.

>   Actually the goal would be to not use another debug_addr entry, but the ability to refer to an addr entry + offset in a DIE. Of course this would require an extension to DWARF (non-standard, or eventually standard) which would also mean updating the DWARF consumer... which probably defeats the point of your work, which I imagine is intended to avoid changing the consumer. Though perhaps GDB would be more inclined to accept a patch for an addr+offset form compared to support for the register details.
>    
> 
> Okay! How would that look like, and what would that be used for?
> 
>> 4. If GDB can do the right thing when printing in a backtrace, then it seems like it should do the same/similar thing when in a frame
> 
> The same problem exists for backtraces. For example, in PR39752 the outer frames' parameters are printed using the inner-most register value:
> 
>   (gdb) bt
>   #0  fn3 (p3=<optimized out>) at test.c:11
>   #1  0x000000000040050f in fn2 (p2=999) at test.c:15
>   #2  0x000000000040052f in fn1 (p1=999) at test.c:21
>   #3  0x000000000040054e in main () at test.c:26
>    

Ah, sorry, I misunderstood your description.

In D68465#1697824 <https://reviews.llvm.org/D68465#1697824>, @probinson wrote:

> Debugger tuning should not be used directly this way.  There should be a DwarfDebug flag, and a CL option, and the default set in the DwarfDebug ctor based on tuning.  This allows the defaulting to work how you want, but can be overridden easily for experimentation and testing.  There are lots of examples of doing this in the ctor already.  Also, if it turns out some other debugger also needs this, it's trivial to fix up the ctor to handle it with no code changes needed elsewhere.
>
> @dblaikie I'm also not clear what you're suggestion about .debug_addr entry plus offset.  DW_LLE_offset_pair does this, derived from the base address, which ought to be available for any given function, assuming DWARF v5.  Can you explain more clearly what's missing?


Right - for loclists there's no need for new forms, etc. It was specifically related to the other review related to this that modifies the in-memory representation of the debug_addr (llvm::AddressPool) - which I assume meant a difference in output in the address pool, but seems it doesn't add the offset inside the pool (but may end up with redundant entries in the pool which should be fixed in any case).

My point was generally that the debug_addr section shouldn't be incnluding addresses with offsets, it should be the places that refer to debug_addr that use the offsets. The specific place I'd like to use offsets would be from FORM_addr in debug_info. But, yes, in this case the support for debug addr references from loclists, the forms are already sufficiently descriptive for this.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D68465