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


================
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);
----------------
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?


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


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp:529
+      uint64_t Offset =
+          IsDWO ? HeaderSize + getLocSectionBase() : getLocSectionBase();
       DWARFDataExtractor Data(Context.getDWARFObj(), *LocSection,
----------------
Maybe in the IsDWO case just set it to "HeaderSize"?

Hmm, @aprantl @probinson - I can't find the wording in the DWARF5 spec about the use of loclists_base and rnglists_base in split units. It's unclear if these need to be specified in the split unit, or if they're assumed to be zero (or, technically, zero + sizeof(list header))? Currently LLVM doesn't generate a rnglists_base for split DWARFv5 units (I haven't checked the history on that change - but I might've had a hand in it) & this change is going to propagate that choice & do similarly for loclists_base.

GCC 8.1 at least doesn't seem to give us any hint there - since they don't use rnglists.dwo so far as I can tell (I induced a function-local range list and it was put in the .o rnglists section, there was no rnglists.dwo section)

In some ways it'd be tidier to emit it, of course - consistent between .o and .dwo, but also it'd likely be redundant/always have the same value in every .dwo file.


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


================
Comment at: llvm/test/DebugInfo/X86/dwarfdump-debug-loclists-dwo.test:1
+# Test for loclists.dwo section emission DWARFv5 fission
+# source dwarfdump-test-loclists-dwo.c
----------------
What does this test that's different from the debug-loclists.ll?

This is a separate test for the parser only? We don't /always/ bother with that, but it's nice to have - if we're going to do that, the functionality should probably be committed in two separate patches - one with the parser functionality and its test, and then anotehr with the emission functionality and test.

Is there no existing llm-dwarfdump/parser test for loclists? (existing source example that could be reused, possibly existing CHECKs that could be reused (they may need some adaptation to be flexible enough to handle a .dwo file, where the addresses won't be able to be resolved, etc), even if a new object file would have to be committed to go with it)


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

https://reviews.llvm.org/D69462