[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:
> 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).


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


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


================
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.
----------------
dblaikie wrote:
> 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)
I updated the comment. (I assume you meant that and not that you want to use these entry kinds in this patch?).


https://reviews.llvm.org/D53365