[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:2457
 
 void DwarfDebug::emitDebugLocDWO() {
+  if (DebugLocs.getLists().empty())
----------------
SouraVX wrote:
> 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
Eh, yeah, looks like that'd be inconsistent with the other section support.

looking at emitDebugRanges/emitDebugRangesDWO - I've refactored those in llvmorg-10-init-8906-g89b7f16204a - this should give a good basis on which to refactor emitDebugLoc/emitDebugLocDWO. 

With the exception that the existing emitDebugLocDWO will need to be a special case (the curretn/old code) that doesn't exist in the debugRanges/RangesDWO code.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDie.cpp:128-130
+    if (Loclistsv5)
+      LoclistsSectionData = UseDWO ? Obj.getLoclistsDWOSection().Data
+                                   : Obj.getLoclistsSection().Data;
----------------
SouraVX wrote:
> 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.
Yeah, maybe leave a FIXIT here about DWP support? 

I think the whole thing needs a bit more deep surgery for more complete/general DWP support & such, out of scope for this change.


================
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;
----------------
SouraVX wrote:
> 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.
Oh, sorry - a better test that's got source in there and everything, is llvm/test/CodeGen/X86/debug-loclists.ll

Yes, you could add a test that tests that with split DWARF (& them dumping loclists.dwo from the dwo file).

See llvm/test/DebugInfo/X86/cu-ranges.ll for the range list equivalent that tests both split and non-split forms.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69462