Page MenuHomePhabricator

[Codegen] - Implement basic .debug_loclists section emission (DWARF5).
ClosedPublic

Authored by grimar on Oct 17 2018, 3:58 AM.

Details

Summary

.debug_loclists is the DWARF 5 version of the .debug_loc.
With that patch, it will be emitted when DWARF 5 is used.

Diff Detail

Repository
rL LLVM

Event Timeline

grimar created this revision.Oct 17 2018, 3:58 AM
dblaikie added inline comments.Oct 19 2018, 10:24 AM
lib/CodeGen/AsmPrinter/DwarfDebug.cpp
889–895 ↗(On Diff #169984)

Maybe factor this like:

if (dwarf >= 5 && !split) {
  if (range lists)
    add rangelists base
  if (loc lists)
    add loclists base
}

Though, also - why does this check for !split DWARF? These features would be useful even with split-dwarf... well, I guess there aren't any location lists in the skeleton, but that should fall out naturally? (oh, I guess loclists being stored in DwarfDebug because there are never loclists in both skeleton and split file, they only ever go in one place - but I think it might be a nicer generalization to have it in the DwarfFile level)

& in fact I'm moving the ranges into DwarfFile (from DwarfCompileUnit) to support more rnglist functionality too.

1928–1962 ↗(On Diff #169984)

I'd probably factor this as 3 functions:

emitListsTableHeader(AsmPrinter, DwarfFile, TableStart, TableEnd, TableBase) {
  ...
}
emitRngListsTableHeader(AsmPrinter, DwarfFile) {
  emitListsTableHeader(AsmPrinter, DwarfFile, Asm->createTempSymbol("debug_rnglist_table_start"), Asm->createTempSymbol("debug_rnglist_table_end"), Holder.getRnglistsTableBaseSym());
}
/* similar for emitLocListsTableHeader(...) */

Alternatively, put the 3 list-specific parameters in the call sites, since they only each have one call site, I guess?

(I suppose you could pass in the "rng" or "loc" string and use that to create the name of the first two symbols to reduce repetition there)

1968 ↗(On Diff #169984)

Perhaps renamed to "IsLocLists"?

2011–2013 ↗(On Diff #169984)

Probably use base_addressx+offset_pair (note, addressx rather than address - using the address pool is handy) and startx_length if there's only a single range (which does happen - when the range is smaller than the range of the scope of the variable)

grimar updated this revision to Diff 170606.Oct 23 2018, 4:44 AM
grimar marked 2 inline comments as done.
  • Addressed review comments.
lib/CodeGen/AsmPrinter/DwarfDebug.cpp
889–895 ↗(On Diff #169984)

I applied suggested change.

Though, also - why does this check for !split DWARF?

I think I just misread the spec. I was sure DW_AT_loclists_base is not mentioned in allowed attributes lists for skeleton/split full unit for some reason.

but I think it might be a nicer generalization to have it in the DwarfFile level

I guess so. Are you ok if I try to do this change in a followup? (since I am not adding the DebugLocs to DwarfDebug in this patch).

1928–1962 ↗(On Diff #169984)

I can't pass the "rng"/"loc" now since the code was updated recently and now range list's header
contains the offsets table, while lists table does not (not implemented).
But I was still able to extract the common part using your first suggestion.

1968 ↗(On Diff #169984)

Oops. Done.

2011–2013 ↗(On Diff #169984)

I updated the comment. (I assume you meant that and not that you want to use these entry kinds in this patch?).

dblaikie added inline comments.Oct 23 2018, 11:25 AM
lib/CodeGen/AsmPrinter/DwarfDebug.cpp
889–895 ↗(On Diff #169984)

Sure - though you're right in some ways there.

So long as DebugLocs is at the DwarfDebug level, not the DebugFile level - then yeah, this should test for usingSplitDwarf.

Ranges are slightly different - a skeleton/split-full can each have ranges (hence having the ranges stored at the DebugFile level - some ranges in the .o, some in the .dwo). /if/ there are ranges in the skeleton (like if CU ranges are put there) then you want a ranges_base potentially.

But with loclist - this change as you have it now would put a loclists_base on the skeleton CU even though all the locations will go in the .dwo file anyway.

So, yeah:

  if (getDwarfVersion() >= 5) {
  if (U.hasRangeLists())
    U.addRnglistsBase();

  if (!DebugLocs.getLists().empty() && !usingSplitDwarf())
    U.addLoclistsBase();
}

Shoudl be right. And I wouldn't bother moving DebugLocs to DwarfFile for now - maybe one day we'll want that (to have locations both in the skeleton and the full-split CU), but I don't think there's any reason to frontload that work now.

1928–1962 ↗(On Diff #169984)

Looks good for now - but yeah, ongoing refactoring to help share more of these implementations as we go - probably resort to some templates, etc, eventually.

test/CodeGen/X86/debug-loclists.ll
7 ↗(On Diff #170606)

Probably use a -v dump to test the specific LLE opcodes, etc?

grimar updated this revision to Diff 170844.Oct 24 2018, 4:02 AM
  • Addressed review comments.
lib/CodeGen/AsmPrinter/DwarfDebug.cpp
889–895 ↗(On Diff #169984)

I see. Thanks for the explanation!

test/CodeGen/X86/debug-loclists.ll
7 ↗(On Diff #170606)

But I am already using llvm-dwarfdump -v. It shows DW_OP_breg3 RBX+0 already.

I added a piece from the .debug_info to the test to show how location is used here.

If you mean to test all possible LLE opcodes, that is probably should be in the patch for llvm-dwarfdump :)

dblaikie added inline comments.Oct 24 2018, 10:32 AM
test/CodeGen/X86/debug-loclists.ll
7 ↗(On Diff #170606)

I think in this case it's interesting/worth testing the particular choice of LLE encoding that LLVM used to emit this list. (see similar tests for RLE, I think?)

Other tests that are interested in testing the high level "this range has this location, this range has this location" would use the sort of checking you have here.

69–72 ↗(On Diff #170844)

Generally folks strip out the attribute lists since they aren't important to most debug info test cases and add some noise to trying to understand what's being tested.

grimar added inline comments.Oct 24 2018, 1:40 PM
test/CodeGen/X86/debug-loclists.ll
7 ↗(On Diff #170606)

Ah, I see what you mean now. Did not realize you're talking about LLE* entry kinds (that is how they are called in the standard it seems) and not about dumping the DW_OP* opcodes. That makes sense.

At a very quick look, I see tests dumping DW_RLE_* with -v option, but seems nothing dumps DW_LLE_*.
So seems it needs adding this functionality to the llvm-dwarfdump perhaps.
I'll take a look closer tomorrow and do that then. Thanks!

69–72 ↗(On Diff #170844)

Will do.

grimar updated this revision to Diff 171072.Oct 25 2018, 5:46 AM
  • Addressed review comments.
test/CodeGen/X86/debug-loclists.ll
7 ↗(On Diff #170606)

So. Currently, we dump range lists in dumpRnglistsSection here:
https://github.com/llvm-mirror/llvm/blob/master/lib/DebugInfo/DWARF/DWARFContext.cpp#L273

and locations list int dumpLoclistsSection here:
https://github.com/llvm-mirror/llvm/blob/master/lib/DebugInfo/DWARF/DWARFContext.cpp#L295
The latter does not implements dumping the LLE_* kinds.

Ranges list and locations list are very similar and can be dumped in a common way and share the code
probably. I believe that is what D51081 starts to generalize and its code can be used for implementing
locations list dumping (LLE*).

But it was not yet re-landed, though I assume it should happen soon
(https://reviews.llvm.org/D53364#1271569).
And I would prefer waiting for it before starting to implement LLE* dumping with llvm-dwarfdump.

For now, I suggest checking the asm code produced to verify the RLE entries. What do you think?

Looks good - thanks!

Eventually we'll see how all the loclists, rnglists, and pre-v5 debug_loc.dwo handling can all be merged/refactored to make the most sense, but seems OK to sort of pursue these separately-ish for now & refactor/common the parts as we find them/once we can see the overall picture.

test/CodeGen/X86/debug-loclists.ll
7 ↗(On Diff #170606)

Yeah, I just came across that myself while improving debug_loc.dwo encoding (using startx_length means using a lot of addresses in the address pool (in an optimized build) compared to using base_addressx+offset pairs mostly - same as the problems with the rnglist encoding choices)

Probably just skip checking the LLE encodings for now - don't worry about writing an assembly test, I think. (might've been worth doing that way if you were starting from scratch, or improving the dumping support first, etc - but not worth holding this up to rewrite the test (small hurdle, I realize - but also small benefit I think)

dblaikie accepted this revision.Oct 25 2018, 8:50 AM

Oops, meant to accept.

This revision is now accepted and ready to land.Oct 25 2018, 8:50 AM
grimar added inline comments.Oct 25 2018, 11:34 AM
test/CodeGen/X86/debug-loclists.ll
7 ↗(On Diff #170606)

Sorry, I was not clear. I was mean I already included this it in this version of the patch (see ASM tag check). I am testing that llc produce the DW_LLE_offset_pair tags now.

dblaikie added inline comments.Oct 25 2018, 1:21 PM
test/CodeGen/X86/debug-loclists.ll
26–45 ↗(On Diff #171072)

Maybe trim some of the whitespace from between the assembly and the comments so it's easier to read on a narrower screen? (I'd probably bring it in as close as possible while still keeping it vertically aligned)

7 ↗(On Diff #170606)

Ah, right, no worries - sorry I didn't look more closely.

grimar added inline comments.Oct 25 2018, 3:18 PM
test/CodeGen/X86/debug-loclists.ll
26–45 ↗(On Diff #171072)

Yeah, I am also was not happy to do that. Problem is on the line 24. Had to add spaces to keep the
vertical-align with "# Length".
I guess I'll just remove that single comment and trim the spaces then.

dblaikie added inline comments.Oct 25 2018, 3:30 PM
test/CodeGen/X86/debug-loclists.ll
26–45 ↗(On Diff #171072)

Ah, yeah - I'd let that one be unaligned - there are other test cases (& I think LLVM's own output) leaves some things on especially long lines unaligned.

This revision was automatically updated to reflect the committed changes.