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

[Vedant Kumar via Phabricator via llvm-commits]

vsk marked 2 inline comments as done.
vsk added a comment.

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

> (general caveat: this should be committed as separate patches - between llvm and llvm-dsymutil (much like llvm-dwarfdump and llvm codegen are usually committed separately) - but yeah, it's nice to see it all together for review))


Sounds good, I'll split this up before committing.

> Do you happen to have numbers for, say, clang, on the growth of object size because of this - specifically I'd be interested in the growth of the .rela.debug_addr section in a DWARFv5 build.

I looked at the size impact on a Darwin -gdwarf-4 stage2 clang build. The aggregate size of .o's post-patch grows 0.006% (8367046264 bytes -> 8367544576 bytes, a 486KB increase). Tail calls make up ~5% of all calls (in clang at least), so this is in line with what was measured in D72489 <https://reviews.llvm.org/D72489>. In D72489 <https://reviews.llvm.org/D72489>, I added a relocation to every non-tail call site entry, which increased the aggregate .o size by 0.04% (~3MB).

I don't think linker support for dwarf5 is mature enough on Darwin for me to measure. Perhaps that measurement wouldn't be very useful, as ELF relocations are different.

> Did I understand correctly from previous discussions that the goal was to have call_sites for /every/ call, at some point? (I'm concerned that'll be a /lot/ of addresses)

Yes. When tuning for lldb, we've been emitting call sites for every call for some time now.

Hm :(. Digging through the history, I see that I accidentally turned on DIFlagAllCallsDescribed when targeting -gdwarf-4 + GDB in D69743 <https://reviews.llvm.org/D69743> (November 2019). This was done prematurely, I apologize for this. It should not have happened until the entry values feature got enabled by default. I've checked in test coverage for -gdwarf-4 + -ggdb to clang/test/CodeGenCXX/dbg-info-all-calls-described.cpp in 47622efc <https://reviews.llvm.org/rG47622efc6f01532ef1c23490f516efb9081827f8>.

@dblaikie @djtodoro I'd prefer to leave things as they are, but am also open to disabling DIFlagAllCallsDescribed for -gdwarf-4 + -ggdb until entry values get re-enabled by default, let me know what you think.

> I suppose another question, I guess - how many more call_sites is this than currently/without this patch?

This patch doesn't change the number of call site entries (DW_TAG_call_site), it just adds relocations to ~3-5% of those entries.



================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:873
       const MCSymbol *PCAddr =
           (IsTail && !tuneForGDB())
               ? nullptr
----------------
djtodoro wrote:
> Looks like the `dwarf::DW_AT_call_return_pc` is enough for GDB to distinguish that even for tail calls. Can we do the same for LLDB and avoid the `call_pc`?
> 
> 
I don't think that would be valid DWARF. The return pc for a tail call isn't given by the address of the instruction after after the tail-calling branch.

Are you sure GDB uses DW_AT_call_return_pc to figure out the address where the tail call was made?


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1789
+  // TODO: Add support for targets with delay slots.
+  if (!NoDebug && SP->areAllCallsDescribed() && MI->isCall() &&
+      !MI->hasDelaySlot()) {
----------------
djtodoro wrote:
> Should we use the `isCandidateForCallSiteEntry()` instead?
Yes, thanks!


================
Comment at: llvm/test/DebugInfo/MIR/X86/call-site-gnu-vs-dwarf5-attrs.mir:17
 #
 # extern void fn();
 # extern void fn2(int x);
----------------
djtodoro wrote:
> GCC produces this:
>    <2><9a>: Abbrev Number: 9 (DW_TAG_GNU_call_site)
>       <9b>   DW_AT_low_pc      : 0x23
>       <a3>   DW_AT_GNU_tail_call: 1
>       <a3>   DW_AT_abstract_origin: <0xd4>
Is the DW_AT_low_pc the address of the tail-calling branch instruction, or is it the address of the next instruction?


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

https://reviews.llvm.org/D76336