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


================
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
----------------
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)


================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1968
 
-  // Start the dwarf loc section.
-  Asm->OutStreamer->SwitchSection(
-      Asm->getObjFileLowering().getDwarfLocSection());
+  bool IsRngLists = getDwarfVersion() >= 5;
+
----------------
Perhaps renamed to "IsLocLists"?


================
Comment at: lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2011-2013
+        // TODO: Calculate the base address manually basing on the first entry
+        // and use DW_LLE_base_address + DW_LLE_offset_pair, that should reduce
+        // the size of the debug data emited.
----------------
Probably use base_addressx+offset_pair (note, addressx rather than address - using the address pool is handy) and startx_length if there's only a single range (which does happen - when the range is smaller than the range of the scope of the variable)


https://reviews.llvm.org/D53365