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

[Sourabh Singh Tomar via Phabricator via llvm-commits]

SouraVX marked an inline comment as done.
SouraVX 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());
----------------
dblaikie wrote:
> 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).
Yeah, I'll clean this up in next revision. basically I was only interested in creating symbol based on whether we are in split mode or not. So didn't bothered to check the result of ternary operator. 



================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2457
 
 void DwarfDebug::emitDebugLocDWO() {
+  if (DebugLocs.getLists().empty())
----------------
dblaikie wrote:
> 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.
Hi @dblaikie, Thanks for reviewing this. 
Primary reason why I added {loclists.dwo} code to this function is because -- 
top level bifurcation of emission is based on "split or not"--

if (useSplitDwarf())
    emitDebugLocDWO();   -- this handles both v4 and v5 loc and loclists format
  else
    // Emit info into a debug loc section.
    emitDebugLoc(); -- as you, mentioned this only handles pre-standard loclist format.. So I choose to add it here.

Now your suggestion is to add the v5 loclist.dwo format to emitDebugLoc() function -- as it can handle v4 and v5 both ?? 
I think that also implies to make emission decision based on version of dwarf ??
sort of -- 

    if (getDwarfVersion() == 4 && UseSplitDwarf())
        emitDebugLocDWO()   -- handles only v4 split mode
    else ()
        emitDebugLoc() -- handles everything else -- v4 v5 and v5 + split


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDie.cpp:128-130
+    if (Loclistsv5)
+      LoclistsSectionData = UseDWO ? Obj.getLoclistsDWOSection().Data
+                                   : Obj.getLoclistsSection().Data;
----------------
dblaikie wrote:
> 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).
I haven't got a chance to look into details of llvm-dwp utility. 
My Initial plan is to get loclist work in split mode, so that at least using LLDB{GDB is mostly broken for dwarf-5 compiled binaries} we can debug a binary compiled with -gdwarf-5 -gsplit-dwarf.
 As of now LLDB is not able to handle loclists.dwo section. May be @labath can add more to this.


================
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;
----------------
dblaikie wrote:
> 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
Adding extra checks to test/DebugInfo/X86/sret.ll  is fine.
Do you think, we should both of these files also. C source file and dwo check file ?

As I can see from my initial implementation based on your suggestion. test/DebugInfo/X86/sret.ll this suffice our needs here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69462