[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
+
+class DWARFDataExtractor : public DataExtractor {
+  const RelocAddrMap *RelocMap;
----------------
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!


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


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


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


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


https://reviews.llvm.org/D34704