This is an archive of the discontinued LLVM Phabricator instance.

[DWARF] - Emit the correct value for DW_AT_addr_base.
ClosedPublic

Authored by grimar on Sep 17 2018, 5:48 AM.

Details

Summary

Currently, we emit DW_AT_addr_base that points to the beginning of
the .debug_addr section. That is not correct for the DWARF5 case because address
table contains the header and the attribute should point to the first entry
following the header.

This is currently the reason why LLDB does not work with such executables correctly.

Diff Detail

Event Timeline

grimar created this revision.Sep 17 2018, 5:48 AM
dblaikie added inline comments.Sep 17 2018, 7:37 PM
lib/CodeGen/AsmPrinter/AddressPool.cpp
43–44

Is there any reason to support not labeling the address pool? It doesn't look like this should be conditional.

lib/CodeGen/AsmPrinter/DwarfFile.h
59

Since the DWO file can't have an address table itself (so its AddressTableBaseSym would always be null) - perhaps this symbol should be tracked in either the AddressPool itself, or in DwarfDebug? I'd slightly favor the AddressPool handling this. Using something like "DD->getAddressPool().getLabel()" to retrieve it in the DWARFCompileUnit & no need to pass it in when the AddressPool is emitting its header - it already has it/can attach it there.

lib/CodeGen/AsmPrinter/DwarfUnit.cpp
1655–1656

Could you explain what this case is for? I would (naively) have thought this case wouldn't come up once the label was being appropriately managed/emitted at the end of the header? In what situations is there no computed addrTableBase, but this fallback is correct?

grimar updated this revision to Diff 165933.Sep 18 2018, 4:21 AM
  • Addressed review comments.
grimar added inline comments.Sep 18 2018, 4:22 AM
lib/CodeGen/AsmPrinter/AddressPool.cpp
43–44

Labeling of the address pool really needed only for dwarf5 because dwarf4 does not have address table header.
For simplicity of the code, I changed this. With that, I had to change the 2 more tests though.
I am not sure what is better, for dwarf4 output labeling is not useful, but the code is a bit simpler now.

lib/CodeGen/AsmPrinter/DwarfFile.h
59

Nice, done. (Originally I put it here for consistency, near the StringOffsetsStartSym and RnglistsTableBaseSym).

lib/CodeGen/AsmPrinter/DwarfUnit.cpp
1655–1656

My code created a base symbol for dwarf5 only:

if (getDwarfVersion() >= 5 && useSplitDwarf())
  SkeletonHolder.setAddrTableBaseSym(
      Asm->createTempSymbol("addr_table_base"));

So this fallback was for dwarf4.

dblaikie added inline comments.Sep 18 2018, 10:52 AM
test/DebugInfo/X86/debug_addr.ll
34

This looks suspicious, I think? This test is for DWARF4 so why does it have a non-zero addr_base?

grimar added inline comments.Sep 19 2018, 12:49 AM
test/DebugInfo/X86/debug_addr.ll
34

No :) This test file is for both DWARF4 and DWARF5. See the corresponding check tags. For DWARF4 it is zero (line 20).
This place is exactly what needed to be fixed to show that this patch does the right thing now.

dblaikie accepted this revision.Sep 19 2018, 9:03 PM

Looks good - thanks!

This revision is now accepted and ready to land.Sep 19 2018, 9:03 PM
This revision was automatically updated to reflect the committed changes.