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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: test/CodeGen/X86/debug-loclists.ll:7
+; CHECK-NEXT: 0x00000000:
+; CHECK-NEXT:             [0x0000000000000000, 0x000000000000001e): DW_OP_breg3 RBX+0
+
----------------
grimar wrote:
> dblaikie wrote:
> > 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)
> 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.
Ah, right, no worries - sorry I didn't look more closely.


================
Comment at: test/CodeGen/X86/debug-loclists.ll:26-45
+; ASM-NEXT:  .short 5                                                     # Version
+; ASM-NEXT:  .byte 8                                                      # Address size
+; ASM-NEXT:  .byte 0                                                      # Segment selector size
+; ASM-NEXT:  .long 0                                                      # Offset entry count
+; ASM-NEXT: .Lloclists_table_base0:
+; ASM-NEXT: .Ldebug_loc0:
+; ASM-NEXT:  .byte 4                                                      # DW_LLE_offset_pair
----------------
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)


https://reviews.llvm.org/D53365