[PATCH] D33184: [DWARF] - Make collectAddressRanges() return section index in addition to Low/High PC

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugRangeList.h:25-27
+struct DWARFAddress {
+  uint64_t LowPC;
+  uint64_t HighPC;
----------------
dblaikie wrote:
> This should probably be called DWARFAddressRange since it represents more than a single address (or possibly no addresses, if the range is empty) - and I think that's the right terminology in DWARF (that a contiguous range is a 'range' and a series of ranges is a 'range list').
Done in rL303163.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugRangeList.h:28
+  uint64_t HighPC;
+  uint64_t SecNdx;
+};
----------------
aprantl wrote:
> I'm assuming that this is the "official" name used in the specification? Then this name is fine, otherwise I would prefer to call this field SectionIndex.
No, "SecNdx" was something from my head. Does not seem that specification mentions section index for address range in any way. Renamed to SectionIndex here and below.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFDebugRangeList.h:48
     uint64_t EndAddress;
+    // A section index this range belongs to.
+    uint64_t SecNdx;
----------------
aprantl wrote:
> If you find the time, it would be nice to convert all these to /// in a separate commit.
Done in rL303241.


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:949
       Ret += SectionLoadAddress - RSec->getAddress();
-  return Ret;
+  return std::make_pair(Ret, RSec->getIndex());
 }
----------------
aprantl wrote:
> `return {Ret, RSec->getIndex()};`
I changed code around, made Ret to be pair like it probably should be to match return value and this issue was gone.


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:1124
+        llvm::RelocAddrEntry Rel = {(*SymInfoOrErr).second, R.Value};
+        Map->insert(std::make_pair(Address, Rel));
       }
----------------
aprantl wrote:
> {}
Done.



================
Comment at: lib/DebugInfo/DWARF/DWARFUnit.cpp:355
+      auto B = AddrDieMap.upper_bound(R.LowPC);
+      if (B != AddrDieMap.begin() && R.LowPC < (--B)->second.first) {
         // The range is a sub-range of existing ranges, we need to split the
----------------
aprantl wrote:
> (--B)->second.first:
> 
> Would it make sense to replace some std::pairs with structs for better readability?
I think yes, I'll try to take a look while I am here changing DWARF code around.


================
Comment at: lib/Object/COFFObjectFile.cpp:298
+  // TODO: COFFObjectFile::getSectionIndex() not implemented
+  return -1LL;
+}
----------------
aprantl wrote:
> Why does this return -1 and the others 0 for not found? Should it return an optional?
Others ? I am returning -1 in all getSectionIndex() implementations because 0 can be an index of valid section in theory. Since numeration starts from zero.

I would not return Optional. Because that would be an overcomplication of code I think. If we pass
some existent DataRefImpl Sec, then we should be able to find it's index.

I probably can't imagine code that would have real benefits from Optional. We return uint64_t in
getSectionAddress() and getSectionSize() below, so it is also a consistent solution.

As an althernative solution I was thinking about just placing llvm_unreachable() here and then store section address only for ELF objects in getSymbolInfo(). Would it be better ?


https://reviews.llvm.org/D33184