[PATCH] D34704: [DWARF] NFC: Combine relocs with DataExtractor

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDataExtractor.h:18-26
+class DWARFDataExtractor : public DataExtractor {
+  const RelocAddrMap *RelocMap;
+public:
+  DWARFDataExtractor(StringRef Data, const RelocAddrMap *RelocMap,
+                     bool IsLittleEndian, uint8_t AddressSize)
+    : DataExtractor(Data, IsLittleEndian, AddressSize), RelocMap(RelocMap) {}
+
----------------
probinson wrote:
> probinson wrote:
> > dblaikie wrote:
> > > dblaikie wrote:
> > > > What about composition rather than inheritance? (basically a pair of DataExtractor and RelocMap - if I recall correctly, that's how it's stored in the DWARFContext? (ah, not quite - it's the DWARFSection which is only StringRef + RelocAddrMap))
> > > > 
> > > > If the plan is to add more utility functions to encapsulate the use of the RelocAddrMap with the DataExtractor (especially if that could be done to the point of hiding the RelocAddrMap entirely (removing getRelocMap) from users of the DWARFDataExtractor) I could see this being a good direction.
> > > Thanks for pulling these little classes out into their own headers - it does really help readability, I think/agree!
> > The is-a versus has-a relationship is certainly arguable here.  IMO is-a has a stronger case, as I do want to replace most (ideally all) uses of getRelocatedValue with a member function here, allowing us to hide the RelocMap.  In that sense, DWARFDataExtractor is extending DataExtractor (to handle relocated values) rather than using DataExtractor.
> > I could do the getRelocatedValue member function in this patch if you like but I was thinking to do it separately.
> You're welcome!  I didn't do that with DWARFFormParams because it seemed too tightly connected to DWARFFormValue, but in general I see the DebugInfo headers split up that way and it makes sense.
Separately or together is fine - just wanted to check on the direction.


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:595
   // We have to parse it first.
-  DataExtractor lineData(U->getLineSection(), isLittleEndian(),
-                         U->getAddressByteSize());
+  DWARFDataExtractor lineData(U->getLineSection(), Line->getRelocMap(),
+                              isLittleEndian(), U->getAddressByteSize());
----------------
probinson wrote:
> dblaikie wrote:
> > If the RelocMap is passed through the DWARFDataExtractor, does DWARDebugLine even need to have a RelocMap passed into the ctor/as a member?
> > 
> > It's a bit weird that this DWARFDataExtractor uses the relocmap from the Line section from the DWARFContext but has to access a line section that's specific to this DWARFUnit. Is that right? I guess the DWARFUnit's LineSection is the sub-range of the full section that is the line table for the unit - wouldn't this make the offsets incorrect for looking up things in the reloc map maybe? (since they're relative to the whole line section, I would think)
> Several questions there.
> I hadn't noticed but yes the relocmap passed to the DWARFDebugLine ctor is now redundant.  Previously it was used only to resolve address references in DW_LNE_set_address.
> Line->getRelocMap() is retrieving that relocmap, which was set just a moment ago by construction of DWARFDebugLine, passing in getLineSection.Relocs(), so it's all the same relocmap.  That can be more obvious (especially if the separate data member in DWARFDebugLine is removed).
> The base offset for this unit's line table is passed separately to get[OrParse]LineTable, so the extractor correctly has a reference to the entire section, and lookups in the relocmap DTRT.
> 
Are you planning changes to address these in this patch/should I wait to review them?


https://reviews.llvm.org/D34704