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

[Sourabh Singh Tomar via Phabricator via llvm-commits]

SouraVX marked 8 inline comments as done.
SouraVX added inline comments.


================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2435-2452
+  if (!DD.useSplitDwarf())
+    emitRangeList(DD, Asm, List.Label, DD.getDebugLocs().getEntries(List),
+                  *List.CU, dwarf::DW_LLE_base_addressx,
+                  dwarf::DW_LLE_offset_pair, dwarf::DW_LLE_startx_length,
+                  dwarf::DW_LLE_end_of_list, llvm::dwarf::LocListEncodingString,
+                  /* ShouldUseBaseAddress */ true,
+                  [&](const DebugLocStream::Entry &E) {
----------------
dblaikie wrote:
> I'm not sure I understand why this change is here - if I'm reading it right, the only difference between the two emitRangeList calls is "ShouldUseBaseAddress" - if that change is needed, perhaps it'd be better to have a single call that uses "!DD.useSplitDwarf()" as the "ShouldUseBaseAddress" parameter. But I don't think that's correct - base addresses should be used in split and non-split DWARF5, I think.
> 
> I believe base addresses should not be used in pre-DWARF5 split DWARF, but that codepath isn't using this function right now. (I think maybe it should, though)
Thanks for noting this. Yeah, I was previously of holding on opposite thought, then

> base addresses should be used in split and non-split DWARF5

This seems fine, after referring back to Spec. 

But bear with me for a moment, I'm noting one further glitch-- if we do pass 
` ShouldUseBaseAddress ` , I'm getting loclist entries like.
` 0x00000018:
            DW_LLE_base_addressx   (0x0000000000000000)  -- coming in every pair with 0
            DW_LLE_offset_pair     (0x0000000000000000, 0x0000000000000004): DW_OP_reg5 RDI
            DW_LLE_offset_pair     (0x0000000000000004, 0x0000000000000018): DW_OP_reg3 RBX
            DW_LLE_end_of_list     () `

I'm guessing base is not their for split ?? Any thought on this ??











================
Comment at: llvm/lib/CodeGen/AsmPrinter/DwarfDebug.cpp:2515-2518
+    // Pre-standardized fission uses two 0 to mark
+    // end of the location list
+    Asm->OutStreamer->EmitIntValue(0, Size);
+    Asm->OutStreamer->EmitIntValue(0, Size);
----------------
dblaikie wrote:
> What motivated this change? Perhaps this should be in a separate patch if there's a bug to fix here that's separate from the new loclist support being added?
Sure, will do this separately.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp:190-199
+                   ? std::make_unique<DWARFDebugLoclists>(
+                         DWARFDataExtractor(
+                             Context.getDWARFObj(),
+                             Context.getDWARFObj().getLoclistsDWOSection(),
+                             isLittleEndian, getAddressByteSize()),
+                         Header.getVersion())
+                   : std::make_unique<DWARFDebugLoclists>(
----------------
labath wrote:
> I'd probably factor this so that just the DWARFDataExtractor is created in the conditional, and the creation of the loclists object is a separate statement.
Thanks for suggestion, refactored code looks much cleaner.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp:514
 
     // FIXME: add loclists.dwo support
+    if (IsDWO)
----------------
dblaikie wrote:
> Can probably remove this FIXME now.
Sure, Will do.


================
Comment at: llvm/test/CodeGen/X86/debug-loclists.ll:5
+; RUN: llc -dwarf-version=5 -split-dwarf-file=foo.dwo -mtriple=x86_64-pc-linux -filetype=obj -function-sections -o %t < %s
+; RUN: llvm-dwarfdump -v -debug-info -debug-loclists %t | FileCheck %s --check-prefix=DWO
+
----------------
dblaikie wrote:
> Does this check need a different prefix, or can it use the same/default prefix? At a glance it looks like all the CHECKs are the same (& I think they should be) so maybe they can be reused.
I'll try to remove redundancy, but I guess some are needed specially when checking dump of debug_loclists.dwo section. -- that contains / uses different form then non-split case.


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

https://reviews.llvm.org/D69462