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

[David Blaikie via Phabricator via llvm-commits]

dblaikie 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();
----------------
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.


================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1928-1962
+// Emit the header of a DWARF 5 range list table/locations list table. Returns
+// the symbol that designates the end of the table for the caller to emit when
+// the table is complete.
+static MCSymbol *emitRngOrLoclistsTableHeader(AsmPrinter *Asm,
+                                              DwarfFile &Holder,
+                                              bool IsRngList) {
+  // The length is described by a starting label right after the length field
----------------
grimar wrote:
> dblaikie wrote:
> > 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)
> 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.
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.


================
Comment at: test/CodeGen/X86/debug-loclists.ll:7
+; CHECK-NEXT: 0x00000000:
+; CHECK-NEXT:             [0x0000000000000000, 0x000000000000001e): DW_OP_breg3 RBX+0
+
----------------
Probably use a -v dump to test the specific LLE opcodes, etc?


https://reviews.llvm.org/D53365