[PATCH] D69462: [DebugInfo]: Support for debug_loclists.dwo section in llvm and llvm-dwarfdump.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:1140-1145
+        useSplitDwarf()
+            ? DebugLocs.setSym(Asm->createTempSymbol("loclists_dwo_table_base"))
+            : DebugLocs.setSym(Asm->createTempSymbol("loclists_table_base")),
+            U.addSectionLabel(U.getUnitDie(), dwarf::DW_AT_loclists_base,
+                              DebugLocs.getSym(),
+                              TLOF.getDwarfLoclistsSection()->getBeginSymbol());
----------------
This is a bit of a surprising/uncommon piece of code (using a conditional operator without using its result, and using the comma operator). Probably best to use more common constructs for this sort of situation.

I don't think there's any need to name the label differently (loclists_table_base/loclists_dwo_table_base) depending on dwo usage. Probably just keep it with a single/the same name always (loclists_table_base).


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2457
 
 void DwarfDebug::emitDebugLocDWO() {
+  if (DebugLocs.getLists().empty())
----------------
This function handles the pre-standard loclists format (which can't use some of the new/more compact encoding options available in DWARFv5 - as the comment inside the loop mentions)

It'd be better to reuse the emitDebugLoc() function (refactored to be able to use the appropriate .dwo section in some way (either add a function parameter, or choose the section based on whether Split DWARF is enabled, etc)) because it now has some advanced handling of choosing base address selection entries, etc.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDie.cpp:128-130
+    if (Loclistsv5)
+      LoclistsSectionData = UseDWO ? Obj.getLoclistsDWOSection().Data
+                                   : Obj.getLoclistsSection().Data;
----------------
I'm /guessing/ this is probably not compatible with DWP files (the DWARFUnit's "LocSectionData" is carefully populated from the DWP's cu_index - see DWARFUnit's ctor & try testing dumping of a DWP file (admittedly, there's no DWP tool that can produce a DWP file for DWARFv5 right now, so you'd have to hand-craft one/locally patch llvm-dwp).


================
Comment at: llvm/test/DebugInfo/Inputs/dwarfdump-test-loclists-dwo.c:1-18
+// clang -c -O2 -gdwarf-5 -gsplit-dwarf dwarfdump-test-loclists-dwo.c
+
+#include <stdio.h>
+int func(int *ptr) {
+
+  printf("%d\n", *ptr);
+  return *ptr + 5;
----------------
Probably easier/better to add extra RUN/check to test/DebugInfo/X86/sret.ll - see my changes in r374600: http://llvm.org/viewvc/llvm-project?view=revision&revision=374600


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69462/new/

https://reviews.llvm.org/D69462