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

[Paul Robinson via Phabricator via llvm-commits]

probinson added inline comments.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDataExtractor.h:18
+
+class DWARFDataExtractor : public DataExtractor {
+  const RelocAddrMap *RelocMap;
----------------
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.


================
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) {}
+
----------------
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.


================
Comment at: lib/DebugInfo/DWARF/DWARFAcceleratorTable.cpp:124-125
       while (AccelSection.isValidOffsetForDataOfSize(DataOffset, 4)) {
-        unsigned StringOffset =
-            getRelocatedValue(AccelSection, 4, &DataOffset, &Relocs);
+        unsigned StringOffset = getRelocatedValue(AccelSection, 4, &DataOffset,
+                                                  AccelSection.getRelocMap());
         if (!StringOffset)
----------------
dblaikie wrote:
> Maybe a wrapper for getRelocatedValue that takes a DWARFDataExtractor? Or maybe that could be a member function of DWARFDataExtractor? (would that allow DWARFDataExtractor to do without getRelocMap entirely?)
That's the plan.


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:78-79
                              bool LittleEndian) {
-  DataExtractor AccelSection(Section.Data, LittleEndian, 0);
+  DWARFDataExtractor AccelSection(Section.Data, &Section.Relocs, LittleEndian,
+                                  0);
   DataExtractor StrData(StringSection, LittleEndian, 0);
----------------
dblaikie wrote:
> Looks like DWARFDataExtractor could use a ctor that takes a DWARFSection? Does it need other ctors? (worst case callers could pass {Data, Relocs} even if they didn't already have a DWARFSection on hand)
I'll look at that.  Sometimes the reloc map is nullptr I think, e.g. in .dwo situations.



================
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());
----------------
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.



https://reviews.llvm.org/D34704