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

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:889-895
     if (getDwarfVersion() >= 5 && !useSplitDwarf() &&
         !U.getRangeLists().empty())
       U.addRnglistsBase();
 
+    if (getDwarfVersion() >= 5 && !useSplitDwarf() &&
+        !DebugLocs.getLists().empty())
+      U.addLoclistsBase();
----------------
dblaikie wrote:
> grimar wrote:
> > dblaikie wrote:
> > > 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.
> > 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).
> 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.
I see. Thanks for the explanation!


================
Comment at: test/CodeGen/X86/debug-loclists.ll:7
+; CHECK-NEXT: 0x00000000:
+; CHECK-NEXT:             [0x0000000000000000, 0x000000000000001e): DW_OP_breg3 RBX+0
+
----------------
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 :)


https://reviews.llvm.org/D53365