[PATCH] D53365: [Codegen] - Implement basic .debug_loclists section emission (DWARF5).

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

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.



================
Comment at: test/CodeGen/X86/debug-loclists.ll:7
+; CHECK-NEXT: 0x00000000:
+; CHECK-NEXT:             [0x0000000000000000, 0x000000000000001e): DW_OP_breg3 RBX+0
+
----------------
grimar wrote:
> grimar wrote:
> > dblaikie wrote:
> > > grimar wrote:
> > > > dblaikie wrote:
> > > > > Probably use a -v dump to test the specific LLE opcodes, etc?
> > > > 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 :)
> > > 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.
> > 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!
> 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?
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)


https://reviews.llvm.org/D53365