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

[Sourabh Singh Tomar via Phabricator via llvm-commits]

SouraVX added inline comments.


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


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

https://reviews.llvm.org/D69462