[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) {
----------------
SouraVX wrote:
> SouraVX wrote:
> > 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 ??
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> + Surprisingly this ` DW_LLE_base_addressx ` doesn't pops up in non-split case, Not sure if this is fine.
> + For ` ShouldUseBaseAddress ` {false}, we're producing / emitting
> ` DW_LLE_stratx_length ` while for {true} as mentioned above we're producing /emitting.
> ` DW_LLE_offset_pair `
> But bear with me for a moment, I'm noting one further glitch-- if we do pass ShouldUseBaseAddress  , I'm getting loclist entries like.
> DW_LLE_base_addressx   (0x0000000000000000)  -- coming in every pair with 0

Yes, it's expected that dumping loclists.dwo would print zero (Or some other small numbers) in a base_addressx or startx_length - those are address pool indexes, not addresses. The addresses themselves are in the debug_addr section in the .o file, so llvm-dwarfdump (dumping the .dwo file) doesn't have access to them - so it prints out the raw entry parameters (address indexes, offsets, etc) rather than the "processed" values you see when dumping non-split DWARF.

It should do this in non-split DWARF too:

  $ cat loc.cpp
  int f1(int i, int j) {
    int x = 5;
    int y = 3;
    int r = i + j;
    int undef;
    x = undef;
    y = 4;
    return r;
  }
  void f2() {
  }
  blaikie at blaikie-linux2:~/dev/scratch$ clang++-tot -ffunction-sections -O3 loc.cpp -gdwarf-5 -c && llvm-dwarfdump-tot loc.o -debug-loclists -v
  loc.o:  file format ELF64-x86-64

  .debug_loclists contents:
  0x00000000: locations list header: length = 0x00000035, version = 0x0005, addr_size = 0x08, seg_size = 0x00, 
  offset_entry_count = 0x00000003
  offsets: [
  0x0000000c => 0x00000018
  0x0000001d => 0x00000029
  0x00000025 => 0x00000031
  ]
  0x00000018: 
              DW_LLE_base_addressx   (0x0000000000000000)
              DW_LLE_offset_pair     (0x0000000000000000, 0x0000000000000003): DW_OP_consts +3, DW_OP_stack_value
              DW_LLE_offset_pair     (0x0000000000000003, 0x0000000000000004): DW_OP_consts +4, DW_OP_stack_value
              DW_LLE_end_of_list     ()
  
  0x00000029: 
              DW_LLE_startx_length   (0x0000000000000000, 0x0000000000000003): DW_OP_consts +5, DW_OP_stack_value
              DW_LLE_end_of_list     ()
  
  0x00000031: 
              DW_LLE_base_addressx   (0x0000000000000000)
              DW_LLE_offset_pair     (0x0000000000000003, 0x0000000000000004): DW_OP_reg0 RAX
              DW_LLE_end_of_list     ()

Though if you compile without -ffunction-sections, the CU will have a relocated base address, and the loclists will not need to use base_addressx - they will instead use offset_pair and rely on the CU's base address.

In short: I would expect the loclists to be exactly the same in split and non-split cases. So I don't know why you're adding the "ShouldUseBaseAddress = false" part to the split case.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFUnit.cpp:529
+      uint64_t Offset =
+          IsDWO ? HeaderSize + getLocSectionBase() : getLocSectionBase();
       DWARFDataExtractor Data(Context.getDWARFObj(), *LocSection,
----------------
labath wrote:
> SouraVX wrote:
> > labath wrote:
> > > SouraVX wrote:
> > > > dblaikie wrote:
> > > > > 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.
> > > > Anyways it's ` getLocSectionBase() ` value is ` 0 ` . 
> > > > Since, we've set it up that way above --
> > > > 
> > > > ```
> > > > if (IsDWO)
> > > >        setLocSection(&Context.getDWARFObj().getLoclistsDWOSection(), 0);
> > > > ```
> > > > 
> > > > Added this here just for readibility / uniformity sake.
> > > Does `LocSectionBase` have to be zero? Could set the `LocSectionBase` to sizeof(header) instead? (If the consensus is that `DW_AT_loclists_base` is not needed in dwo files -- which seems reasonable to me.)
> > > 
> > > That way, you wouldn't need to do the `if(DWO)` dance here and also in DWARFDie.cpp:dumpLocation. I think that would actually mostly address my earlier comment about having a "central" place to compute the final location section offset.
> > > Does LocSectionBase have to be zero? Could set the LocSectionBase to sizeof(header) instead? (If the consensus is that DW_AT_loclists_base is not needed in dwo files -- which seems reasonable to me.)
> > 
> > Yes, as per Spec also. I also tried to hack DW_AT_loclists_base to place in .dwo file. that ends up in a backend error 
> > > Fatal Backend Error :A dwo section may not contain relocations
> > 
> > later, I re-read Spec to find the same, dwo section contain no relocation. this is also caused trouble, in another piece of my work, where I tried to insert
> > > DW_AT_macinfo 
> > in a dwo file.
> > Fatal Backend Error :A dwo section may not contain relocations
> 
> If that's the only problem then presumably the relocation could be dispensed with by emitting the attribute value as a constant, or a label difference (distance from the start of the section) instead of a plain label (though I don't think that is necessary).
@labath - yeah, I like that idea of changing:

  if (IsDWO)
       setLocSection(&Context.getDWARFObj().getLoclistsDWOSection(), 0);

to

  if (IsDWO)
       setLocSection(&Context.getDWARFObj().getLoclistsDWOSection(), DWARFListTableHeader::getHeaderSize(Header.getFormat());

And then simplifying this:

  uint64_t Offset =
          IsDWO ? HeaderSize + getLocSectionBase() : getLocSectionBase();

down to:

  uint64_t Offset = getLocSectionBase();

@SouraVX - please make those changes. We'll continue here without loclists_base in the split unit & if that (& rnglists_base in split units) needs to be addressed, we'll do them in separate commits/discussions, so as to avoid conflating/complicating this review further.


================
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
+
----------------
SouraVX wrote:
> 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.
I believe these should be the same in split and non-split case (as discussed in the other thread about the implementation and use of ShouldUseBaseAddress) at which point these tests should be/can be simplified to use the same CHECKs for both split and non-split.


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

https://reviews.llvm.org/D69462