[PATCH] D58194: [DebugInfo] add SectionedAddress to DebugInfo interfaces.

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/include/llvm/Object/ObjectFile.h:139
+struct SectionedAddress {
+  SectionedAddress() : SectionIndex(UndefSection), Address(0) {}
+  SectionedAddress(uint64_t SI, uint64_t VMA)
----------------
alexey.lapshin wrote:
> dblaikie wrote:
> > Maybe use non-static data member initializers rather than constructors? 
> > (& then drop the other two ctors too - let this be a plain struct)
> I changed it to non-static data member initializers. But it looks like I could not drop following constructors. Since if I add non-static data member initializers then it stopped to compile inplace initializers = {Addr, Index}. To compile inplace initializer - it require constructor SectionedAddress (Addr, Index). It also require default constructor for default instantiation.
ah, seems that rule doesn't exist in C++14 anymore - so maybe a FIXME to remove the ctors once we've adopted C++14?


================
Comment at: llvm/include/llvm/Object/ObjectFile.h:149
+
+static inline bool operator<(const SectionedAddress &LHS,
+                             const SectionedAddress &RHS) {
----------------
These shouldn't be static (means they'd have internal linkage, and they'd be duplicated (& not deduplicated by the linker) and lead to ODR violations (any non-static inline function that calls these and is used in multiple translation units would be in violation of the ODR because that inline function would differ (it'd be calling different functions) between fileS))


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp:938-949
+  bool AddressFound = lookupAddressRangeImpl(Address, Size, Result);
+
+  if (!AddressFound) {
+    if (Address.SectionIndex != object::SectionedAddress::UndefSection) {
+      // Search for absolute addresses
+      Address.SectionIndex = object::SectionedAddress::UndefSection;
+
----------------
Probably flip this around to reduce indentation:

  if (lookupAddressRangeImpl(...))
    return true;

  if (Address.SectionIndex == Undef)
    return false;

  Address.SectionIndex = Undef;
  return lookupAddressRangeImpl(...);


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp:988-989
     // Figure out the last row in the range.
-    uint32_t LastRowIndex = findRowInSeq(CurSeq, EndAddr - 1);
+    object::SectionedAddress LastAddress = {EndAddr - 1, Address.SectionIndex};
+    uint32_t LastRowIndex = findRowInSeq(CurSeq, LastAddress);
     if (LastRowIndex == UnknownRowIndex)
----------------
In some places you've rolled the SectionedAddress creation into the call site, and in others you have a named variable like this. I think it probably makes sense to not name these variables in general (unless they're needed more than once)? But not a big deal.


================
Comment at: llvm/lib/DebugInfo/DWARF/DWARFDebugLine.cpp:1070-1072
+  if (RowIndex == -1U) {
     return false;
+  }
----------------
Still looks like some unrelated refactoring here?


================
Comment at: llvm/tools/llvm-rtdyld/llvm-rtdyld.cpp:391-392
             Addr += SectionLoadAddress - Sec->getAddress();
+        } else {
+          if (auto SecOrErr = Sym.getSection())
+            Address.SectionIndex = SecOrErr.get()->getIndex();
----------------
Could you use "else if" here?


================
Comment at: llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:206
+
+  if (error(BinaryOrErr))
+    return object::SectionedAddress::UndefSection;
----------------
can't say I've seen this written this way - I think it's probably more common to write it as "if (!BinaryOrErr)"?


================
Comment at: llvm/tools/llvm-symbolizer/llvm-symbolizer.cpp:209
+
+  Binary &Binary = *BinaryOrErr.get().getBinary();
+
----------------
Rather than x.get().y you could write this as "x->y"?


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

https://reviews.llvm.org/D58194