Page MenuHomePhabricator

[DebugInfo]: Support for debug_loclists.dwo section in llvm and llvm-dwarfdump.
Needs ReviewPublic

Authored by SouraVX on Sat, Oct 26, 3:26 AM.

Details

Summary

This patch adds support for debug_loclists.dwo section in llvm and llvm-dwarfdump.
Also Fixes PR43622, PR43623.

Diff Detail

Event Timeline

SouraVX created this revision.Sat, Oct 26, 3:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptSat, Oct 26, 3:26 AM
aprantl added inline comments.Mon, Oct 28, 12:29 PM
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
2462

This function looks like it may be more readable if it were organized like this:

if (getDwarfVersion() >= 5) {
  for (const auto &List : DebugLocs.getLists())
    ...
} else /* DWARF v2-4 */ {
  for (const auto &List : DebugLocs.getLists())
    ...
}
2500

Can you please make sure all comments are complete sentences ending in a .?

dblaikie added inline comments.Mon, Oct 28, 3:34 PM
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1140–1145

This is a bit of a surprising/uncommon piece of code (using a conditional operator without using its result, and using the comma operator). Probably best to use more common constructs for this sort of situation.

I don't think there's any need to name the label differently (loclists_table_base/loclists_dwo_table_base) depending on dwo usage. Probably just keep it with a single/the same name always (loclists_table_base).

2457

This function handles the pre-standard loclists format (which can't use some of the new/more compact encoding options available in DWARFv5 - as the comment inside the loop mentions)

It'd be better to reuse the emitDebugLoc() function (refactored to be able to use the appropriate .dwo section in some way (either add a function parameter, or choose the section based on whether Split DWARF is enabled, etc)) because it now has some advanced handling of choosing base address selection entries, etc.

llvm/lib/DebugInfo/DWARF/DWARFDie.cpp
96–98

I'm /guessing/ this is probably not compatible with DWP files (the DWARFUnit's "LocSectionData" is carefully populated from the DWP's cu_index - see DWARFUnit's ctor & try testing dumping of a DWP file (admittedly, there's no DWP tool that can produce a DWP file for DWARFv5 right now, so you'd have to hand-craft one/locally patch llvm-dwp).

llvm/test/DebugInfo/Inputs/dwarfdump-test-loclists-dwo.c
2–19

Probably easier/better to add extra RUN/check to test/DebugInfo/X86/sret.ll - see my changes in r374600: http://llvm.org/viewvc/llvm-project?view=revision&revision=374600

SouraVX marked an inline comment as done.Tue, Oct 29, 2:34 PM
SouraVX added inline comments.
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1140–1145

Yeah, I'll clean this up in next revision. basically I was only interested in creating symbol based on whether we are in split mode or not. So didn't bothered to check the result of ternary operator.

2457

Hi @dblaikie, Thanks for reviewing this.
Primary reason why I added {loclists.dwo} code to this function is because --
top level bifurcation of emission is based on "split or not"--

if (useSplitDwarf())

  emitDebugLocDWO();   -- this handles both v4 and v5 loc and loclists format
else
  // Emit info into a debug loc section.
  emitDebugLoc(); -- as you, mentioned this only handles pre-standard loclist format.. So I choose to add it here.

Now your suggestion is to add the v5 loclist.dwo format to emitDebugLoc() function -- as it can handle v4 and v5 both ??
I think that also implies to make emission decision based on version of dwarf ??
sort of --

if (getDwarfVersion() == 4 && UseSplitDwarf())
    emitDebugLocDWO()   -- handles only v4 split mode
else ()
    emitDebugLoc() -- handles everything else -- v4 v5 and v5 + split
llvm/lib/DebugInfo/DWARF/DWARFDie.cpp
96–98

I haven't got a chance to look into details of llvm-dwp utility.
My Initial plan is to get loclist work in split mode, so that at least using LLDB{GDB is mostly broken for dwarf-5 compiled binaries} we can debug a binary compiled with -gdwarf-5 -gsplit-dwarf.
As of now LLDB is not able to handle loclists.dwo section. May be @labath can add more to this.

llvm/test/DebugInfo/Inputs/dwarfdump-test-loclists-dwo.c
2–19

Adding extra checks to test/DebugInfo/X86/sret.ll is fine.
Do you think, we should both of these files also. C source file and dwo check file ?

As I can see from my initial implementation based on your suggestion. test/DebugInfo/X86/sret.ll this suffice our needs here.

dblaikie added inline comments.Fri, Nov 1, 4:43 PM
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
2457

Eh, yeah, looks like that'd be inconsistent with the other section support.

looking at emitDebugRanges/emitDebugRangesDWO - I've refactored those in llvmorg-10-init-8906-g89b7f16204a - this should give a good basis on which to refactor emitDebugLoc/emitDebugLocDWO.

With the exception that the existing emitDebugLocDWO will need to be a special case (the curretn/old code) that doesn't exist in the debugRanges/RangesDWO code.

llvm/lib/DebugInfo/DWARF/DWARFDie.cpp
96–98

Yeah, maybe leave a FIXIT here about DWP support?

I think the whole thing needs a bit more deep surgery for more complete/general DWP support & such, out of scope for this change.

llvm/test/DebugInfo/Inputs/dwarfdump-test-loclists-dwo.c
2–19

Oh, sorry - a better test that's got source in there and everything, is llvm/test/CodeGen/X86/debug-loclists.ll

Yes, you could add a test that tests that with split DWARF (& them dumping loclists.dwo from the dwo file).

See llvm/test/DebugInfo/X86/cu-ranges.ll for the range list equivalent that tests both split and non-split forms.

SouraVX added a subscriber: alok.Sun, Nov 3, 11:01 PM

Since D69672, D68271 are accepted and ready to land.
To avoid conflicts and save some rework, I'm considering rebasing my changes on top of D69672, once get merged.

Sorry for the trouble, I should be able to land the other patches soon.

llvm/lib/DebugInfo/DWARF/DWARFDie.cpp
96–98

Yep, lldb does not support loclists.dwo (and its support for the non-dwo variant is somewhat dodgy as well). I am planning to address all of that by making lldb use the llvm classes for parsing (hence my other patches).

SouraVX updated this revision to Diff 228394.Fri, Nov 8, 2:50 AM

Revised to use new api's based on @labath and @dblaikie work, to emit loclists.dwo section.

SouraVX marked 2 inline comments as done.Fri, Nov 8, 2:59 AM
SouraVX added inline comments.
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1195

In current implementation, emitDebugLocDWO() only handles -gdwarf-4 -gsplit-dwarf case.
Rest of the cases
-gdwarf-4, -gdwarf-5, -gdwarf-5 -gsplit-dwarf are handled by emitDebugLoc();.

SouraVX marked an inline comment as done.Fri, Nov 8, 6:10 AM
SouraVX added inline comments.
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
1195

I also have another implementation, which keeps above code uniform as in

if (useSplitDwarf())
  emitDebugLocDWO();
else
  emitDebugLoc();

But bloat's up a little emitDebugLocDWO() function. This is a bit cleaner but with one extra check in if.
Please share, your views on this and overall patch, and I shall incorporate changes.

SouraVX planned changes to this revision.Mon, Nov 18, 5:48 AM

Since DW_FORM_loclistx form is supported now.
I'm planning to revise this to accommodate DW_FORM_loclistx form.
Thank you for your patience.

SouraVX updated this revision to Diff 230024.Tue, Nov 19, 3:18 AM

Rebased and revised to adapt DW_FORM_loclistx.

Hi @labath @dblaikie , could you guys please have a look at this revised update. So that we all are on same we page as we progress. Pavel has done lot changes thanks a lot, I just wanted my work to be in sync with his work.
+ This patch doesn't take account of dwp files{Dwarf-5}. dwp prior{Dwarf-5} is intact.
+ Though I haven't look at this D70394 but their might be small conflict with this. But I'm happy to resolve that and address comments on this.
Thanks!

SouraVX marked an inline comment as done.Tue, Nov 19, 3:31 AM
SouraVX added inline comments.
llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
189

I wrote this as conditional, just to avoid breaking of DWP{Prior Dwarf-5} stuff .
Notably failure of loclists-dwp.ll . Since this test case doesn't address DWP{Dwarf-5}. We're good here.
Till we DWP{Dwarf-5} support.

I've only looked at the consumption side, and it seems reasonable to me. The main thing I noticed is that it would be good to abstract the logic for computing the actual final offset in the loclists table somehow (I'm talking about the code that's presently in DWARFDie.cpp:dumpLocation). That's because the this logic will need to be repeated in the code which fetches the final resolved addresses programmatically (DWARFDie::getLocations in D70394). It also probably means that the code in that patch does not handle all of these cases correctly.

However, currently I don't know what's the best way to do that (and I don't even think it's you who needs to make that happen). Though, if you have any thoughts on this, I'd like to hear them.

llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
190–199

I'd probably factor this so that just the DWARFDataExtractor is created in the conditional, and the creation of the loclists object is a separate statement.

dblaikie added inline comments.Tue, Nov 19, 5:36 PM
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
2435–2452

I'm not sure I understand why this change is here - if I'm reading it right, the only difference between the two emitRangeList calls is "ShouldUseBaseAddress" - if that change is needed, perhaps it'd be better to have a single call that uses "!DD.useSplitDwarf()" as the "ShouldUseBaseAddress" parameter. But I don't think that's correct - base addresses should be used in split and non-split DWARF5, I think.

I believe base addresses should not be used in pre-DWARF5 split DWARF, but that codepath isn't using this function right now. (I think maybe it should, though)

2515–2518

What motivated this change? Perhaps this should be in a separate patch if there's a bug to fix here that's separate from the new loclist support being added?

llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
514

Can probably remove this FIXME now.

529

Maybe in the IsDWO case just set it to "HeaderSize"?

Hmm, @aprantl @probinson - I can't find the wording in the DWARF5 spec about the use of loclists_base and rnglists_base in split units. It's unclear if these need to be specified in the split unit, or if they're assumed to be zero (or, technically, zero + sizeof(list header))? Currently LLVM doesn't generate a rnglists_base for split DWARFv5 units (I haven't checked the history on that change - but I might've had a hand in it) & this change is going to propagate that choice & do similarly for loclists_base.

GCC 8.1 at least doesn't seem to give us any hint there - since they don't use rnglists.dwo so far as I can tell (I induced a function-local range list and it was put in the .o rnglists section, there was no rnglists.dwo section)

In some ways it'd be tidier to emit it, of course - consistent between .o and .dwo, but also it'd likely be redundant/always have the same value in every .dwo file.

llvm/test/CodeGen/X86/debug-loclists.ll
5

Does this check need a different prefix, or can it use the same/default prefix? At a glance it looks like all the CHECKs are the same (& I think they should be) so maybe they can be reused.

llvm/test/DebugInfo/X86/dwarfdump-debug-loclists-dwo.test
1

What does this test that's different from the debug-loclists.ll?

This is a separate test for the parser only? We don't /always/ bother with that, but it's nice to have - if we're going to do that, the functionality should probably be committed in two separate patches - one with the parser functionality and its test, and then anotehr with the emission functionality and test.

Is there no existing llm-dwarfdump/parser test for loclists? (existing source example that could be reused, possibly existing CHECKs that could be reused (they may need some adaptation to be flexible enough to handle a .dwo file, where the addresses won't be able to be resolved, etc), even if a new object file would have to be committed to go with it)

SouraVX marked 8 inline comments as done.Tue, Nov 19, 8:46 PM
SouraVX added inline comments.
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
2435–2452

Thanks for noting this. Yeah, I was previously of holding on opposite thought, then

base addresses should be used in split and non-split DWARF5

This seems fine, after referring back to Spec.

But bear with me for a moment, I'm noting one further glitch-- if we do pass
ShouldUseBaseAddress , I'm getting loclist entries like.
` 0x00000018:

DW_LLE_base_addressx   (0x0000000000000000)  -- coming in every pair with 0
DW_LLE_offset_pair     (0x0000000000000000, 0x0000000000000004): DW_OP_reg5 RDI
DW_LLE_offset_pair     (0x0000000000000004, 0x0000000000000018): DW_OP_reg3 RBX
DW_LLE_end_of_list     () `

I'm guessing base is not their for split ?? Any thought on this ??

2515–2518

Sure, will do this separately.

llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
190–199

Thanks for suggestion, refactored code looks much cleaner.

514

Sure, Will do.

llvm/test/CodeGen/X86/debug-loclists.ll
5

I'll try to remove redundancy, but I guess some are needed specially when checking dump of debug_loclists.dwo section. -- that contains / uses different form then non-split case.

SouraVX marked 4 inline comments as done.Tue, Nov 19, 9:05 PM
SouraVX added inline comments.
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
2435–2452

+ Surprisingly this DW_LLE_base_addressx doesn't pops up in non-split case, Not sure if this is fine.
+ For ShouldUseBaseAddress {false}, we're producing / emitting
DW_LLE_stratx_length while for {true} as mentioned above we're producing /emitting.
DW_LLE_offset_pair

SouraVX marked an inline comment as done.Tue, Nov 19, 9:18 PM
SouraVX added inline comments.
llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
529

Anyways it's getLocSectionBase() value is 0 .
Since, we've set it up that way above --

if (IsDWO)
       setLocSection(&Context.getDWARFObj().getLoclistsDWOSection(), 0);

Added this here just for readibility / uniformity sake.

labath added inline comments.Wed, Nov 20, 2:24 AM
llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
529

Does LocSectionBase have to be zero? Could set the LocSectionBase to sizeof(header) instead? (If the consensus is that DW_AT_loclists_base is not needed in dwo files -- which seems reasonable to me.)

That way, you wouldn't need to do the if(DWO) dance here and also in DWARFDie.cpp:dumpLocation. I think that would actually mostly address my earlier comment about having a "central" place to compute the final location section offset.

SouraVX added inline comments.Wed, Nov 20, 2:43 AM
llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
529

Does LocSectionBase have to be zero? Could set the LocSectionBase to sizeof(header) instead? (If the consensus is that DW_AT_loclists_base is not needed in dwo files -- which seems reasonable to me.)

Yes, as per Spec also. I also tried to hack DW_AT_loclists_base to place in .dwo file. that ends up in a backend error

Fatal Backend Error :A dwo section may not contain relocations

later, I re-read Spec to find the same, dwo section contain no relocation. this is also caused trouble, in another piece of my work, where I tried to insert

DW_AT_macinfo

in a dwo file.

labath added inline comments.Wed, Nov 20, 3:07 AM
llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
529

Fatal Backend Error :A dwo section may not contain relocations

If that's the only problem then presumably the relocation could be dispensed with by emitting the attribute value as a constant, or a label difference (distance from the start of the section) instead of a plain label (though I don't think that is necessary).

dblaikie added inline comments.Wed, Nov 20, 11:05 AM
llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp
2435–2452

But bear with me for a moment, I'm noting one further glitch-- if we do pass ShouldUseBaseAddress , I'm getting loclist entries like.
DW_LLE_base_addressx (0x0000000000000000) -- coming in every pair with 0

Yes, it's expected that dumping loclists.dwo would print zero (Or some other small numbers) in a base_addressx or startx_length - those are address pool indexes, not addresses. The addresses themselves are in the debug_addr section in the .o file, so llvm-dwarfdump (dumping the .dwo file) doesn't have access to them - so it prints out the raw entry parameters (address indexes, offsets, etc) rather than the "processed" values you see when dumping non-split DWARF.

It should do this in non-split DWARF too:

$ cat loc.cpp
int f1(int i, int j) {
  int x = 5;
  int y = 3;
  int r = i + j;
  int undef;
  x = undef;
  y = 4;
  return r;
}
void f2() {
}
blaikie@blaikie-linux2:~/dev/scratch$ clang++-tot -ffunction-sections -O3 loc.cpp -gdwarf-5 -c && llvm-dwarfdump-tot loc.o -debug-loclists -v
loc.o:  file format ELF64-x86-64

.debug_loclists contents:
0x00000000: locations list header: length = 0x00000035, version = 0x0005, addr_size = 0x08, seg_size = 0x00, 
offset_entry_count = 0x00000003
offsets: [
0x0000000c => 0x00000018
0x0000001d => 0x00000029
0x00000025 => 0x00000031
]
0x00000018: 
            DW_LLE_base_addressx   (0x0000000000000000)
            DW_LLE_offset_pair     (0x0000000000000000, 0x0000000000000003): DW_OP_consts +3, DW_OP_stack_value
            DW_LLE_offset_pair     (0x0000000000000003, 0x0000000000000004): DW_OP_consts +4, DW_OP_stack_value
            DW_LLE_end_of_list     ()

0x00000029: 
            DW_LLE_startx_length   (0x0000000000000000, 0x0000000000000003): DW_OP_consts +5, DW_OP_stack_value
            DW_LLE_end_of_list     ()

0x00000031: 
            DW_LLE_base_addressx   (0x0000000000000000)
            DW_LLE_offset_pair     (0x0000000000000003, 0x0000000000000004): DW_OP_reg0 RAX
            DW_LLE_end_of_list     ()

Though if you compile without -ffunction-sections, the CU will have a relocated base address, and the loclists will not need to use base_addressx - they will instead use offset_pair and rely on the CU's base address.

In short: I would expect the loclists to be exactly the same in split and non-split cases. So I don't know why you're adding the "ShouldUseBaseAddress = false" part to the split case.

llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp
529

@labath - yeah, I like that idea of changing:

if (IsDWO)
     setLocSection(&Context.getDWARFObj().getLoclistsDWOSection(), 0);

to

if (IsDWO)
     setLocSection(&Context.getDWARFObj().getLoclistsDWOSection(), DWARFListTableHeader::getHeaderSize(Header.getFormat());

And then simplifying this:

uint64_t Offset =
        IsDWO ? HeaderSize + getLocSectionBase() : getLocSectionBase();

down to:

uint64_t Offset = getLocSectionBase();

@SouraVX - please make those changes. We'll continue here without loclists_base in the split unit & if that (& rnglists_base in split units) needs to be addressed, we'll do them in separate commits/discussions, so as to avoid conflating/complicating this review further.

llvm/test/CodeGen/X86/debug-loclists.ll
5

I believe these should be the same in split and non-split case (as discussed in the other thread about the implementation and use of ShouldUseBaseAddress) at which point these tests should be/can be simplified to use the same CHECKs for both split and non-split.