Page MenuHomePhabricator

[DWARF] Emit DW_AT_call_return_pc as an address
ClosedPublic

Authored by vsk on Jan 9 2020, 6:16 PM.

Details

Summary

This reverts D53469, which changed llvm's DWARF emission to emit
DW_AT_call_return_pc as a function-local offset. Such an encoding is not
compatible with post-link block re-ordering tools and isn't standards-
compliant.

In addition to reverting back to the original DW_AT_call_return_pc
encoding, teach lldb how to fix up DW_AT_call_return_pc when the address
comes from an object file pointed-to by a debug map. While doing this I
noticed that lldb's support for tail calls that cross a DSO/object file
boundary wasn't covered, so I added tests for that. This latter case
exercises the newly added return PC fixup.

The dsymutil changes in this patch were originall included in D49887:
the associated test should be sufficient to test DW_AT_call_return_pc
encoding purely on the llvm side.

Diff Detail

Event Timeline

vsk created this revision.Jan 9 2020, 6:16 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: hiraditya. · View Herald Transcript

Thanks for looking into this!

Could you measure the size of the object files of, for example, the clang binary before/after this change - and, if possible, on Linux (where relocations are required to fixup these addresses)? I'm concerned this might increase the size significantly.
Could you check if this does the right thing for Split DWARF or DWARFv5? (I suspect it does - if you're using the same API as is used for DW_TAG_label's low_pc, for instance - which I imagine you are) - in terms of adding the address to debug_addr, and using addrx encoding.

I was going to suggest we use a non-standard extension to preserve the subprogram-relative behavior, by using a different DW_FORM (such as data4) to communicate that this is a subprogram-relative value. But that won't be sufficient for PROPELLER, for instance, which fragments subprograms so there's no unique low_pc for it to be relative to. So that leaves us maybe really wanting the addr+offset encoding discussed here: http://lists.llvm.org/pipermail/llvm-dev/2020-January/138029.html

Thanks for fixing this. The lldb parts look good to me.

aprantl added inline comments.Jan 10 2020, 8:57 AM
lldb/packages/Python/lldbsuite/test/functionalities/tail_call_frames/cross_dso/Makefile
11

It seems like one of the lib_Two dependencies is redundant? Presumably the one in a.out?

aprantl accepted this revision.Jan 10 2020, 8:58 AM
This revision is now accepted and ready to land.Jan 10 2020, 8:58 AM

I ran some size analysis (& hadn't realized that this special case was only done in the DWARFv5 codepath... that leads me to wonder about size impact in DWARFv4, so maybe I'll need to go back and run some general size analysis cost of these call sites).

The .rela.debug_addr for a clang self host optimized (-O3) build, split DWARF, increased from 62.2MB to 70.5MB, which isn't great - but worse in the context of the further address reduction like the ones I've discussed on llvm-dev recently. "ranges everywhere" gets that 62.2MB down to 4.4MB, so growing it by a further 8MB would be pretty unfortunate.

llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
984–991

Side question: How'd this end up like this? Why all these GDB tuning checks? Seems like it'd add another layer of complexity/variety that'll make it harder for us to all be evaluating the same things.

vsk marked an inline comment as done.Jan 10 2020, 12:50 PM

Thanks for looking into this!

Could you measure the size of the object files of, for example, the clang binary before/after this change - and, if possible, on Linux (where relocations are required to fixup these addresses)? I'm concerned this might increase the size significantly.

Hm, there's something I'm missing here. I verified that this change causes one extra X86_64_RELOC_UNSIGNED relocation to be emitted per call site entry in a MachO. For a stage2 build of clang on Darwin (2281 .o's), this patch increases the total file size of the .o's by 0.04% (6,970,706,668 bytes -> 6,973,643,092 bytes, a ~3MB increase). That size increase is much smaller than expected, because clang has ~5.5 million call site entries, and (IIUC) an X86_64_RELOC_UNSIGNED relocation takes 8 bytes. So I'd expect a ~42MB size increase. Does anyone have any insight into this discrepancy? Are relocations compressed, or did the previous subtraction encoding take up space elsewhere?

Could you check if this does the right thing for Split DWARF or DWARFv5? (I suspect it does - if you're using the same API as is used for DW_TAG_label's low_pc, for instance - which I imagine you are) - in terms of adding the address to debug_addr, and using addrx encoding.

I've added a test in test/DebugInfo/X86/debug_addr.ll.

I was going to suggest we use a non-standard extension to preserve the subprogram-relative behavior, by using a different DW_FORM (such as data4) to communicate that this is a subprogram-relative value. But that won't be sufficient for PROPELLER, for instance, which fragments subprograms so there's no unique low_pc for it to be relative to. So that leaves us maybe really wanting the addr+offset encoding discussed here: http://lists.llvm.org/pipermail/llvm-dev/2020-January/138029.html

I don't really understand the addr+offset encoding, I'll follow up on llvm-dev.

lldb/packages/Python/lldbsuite/test/functionalities/tail_call_frames/cross_dso/Makefile
11

Ah, yep. If it's all right I'd rather just leave the redundant lib_Two dependency for a.out, to make it really explicit.

vsk updated this revision to Diff 237419.Jan 10 2020, 12:51 PM
vsk marked an inline comment as done.Jan 10 2020, 1:01 PM
vsk added a subscriber: djtodoro.
vsk added inline comments.
llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
984–991

+ @djtodoro, I'm not sure why AT_call_return_pc would be needed at a tail call site as the debugger must ignore it. As for emitting DW_AT_low_pc under gdb tuning, I think this might be an artifact from the original GNU implementation.

djtodoro added inline comments.Jan 13 2020, 12:18 AM
llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
984–991

I'm not sure why AT_call_return_pc would be needed at a tail call site as the debugger must ignore it. As for emitting DW_AT_low_pc under gdb tuning, I think this might be an artifact from the original GNU implementation.

Yes, that is the GNU implementation's heritage (I cannot remember why GCC generated the low_pc info in the case of the tail calls), but GNU GDB needs the low_pc (as an address) in order to handle the call_site and call_site_parameters debug info for non-tail calls. To avoiding the pc address info in the case of tail calls makes sense to me, since debuggers should avoid that info.

djtodoro added inline comments.Jan 13 2020, 12:20 AM
llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
990

Why don't we use the getDwarf5OrGNUAttr() for the return_pc/low_pc, since we use the address in both cases now?

dblaikie accepted this revision.Jan 14 2020, 1:50 PM

While the related design discussions continue - the patch itself is good/correct & there's nothing much to be done about the address pool size/relocations increase for now, for GDB at least, which is what I care about.

Perhaps it's best if I split off the design discussions into an llvm-dev thread.

llvm/test/tools/dsymutil/Inputs/call-site-entry.c
22–28

Would this be able to be simplified down to:

__attribute__((optnone)) void f() {
}
int main() {
  f();
}

(the attribute might be simpler than the command line argument to disable optimizations)

Or does the function need to return int to get a call_site?

vsk marked 2 inline comments as done.Jan 15 2020, 1:00 PM
vsk added inline comments.
llvm/lib/CodeGen/AsmPrinter/DwarfCompileUnit.cpp
990

I'll plan a follow-up for this, thanks.

llvm/test/tools/dsymutil/Inputs/call-site-entry.c
22–28

That's a nice simplification, I'll fold that in before committing.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJan 15 2020, 1:19 PM