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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFFormValue.h:50
     const uint8_t *data = nullptr;
+    uint64_t SectionIndex;      /// Section index for reference forms.
   };
----------------
Should this go inside the union, with the uval (wrapping the two together in a struct (anonymous  would be OK if that's possible, but I'm not sure if anonymous structs are portable here, etc))? (I mean, it doesn't change the size of this structure, I guess - but seems like it'd make the intent clearer with regard to which elements are 'active' together, etc)


================
Comment at: include/llvm/DebugInfo/DWARF/DWARFFormValue.h:62
   uint64_t getRawUValue() const { return Value.uval; }
+  uint64_t getSectionIndex() const { return Value.SectionIndex; }
   void setForm(dwarf::Form F) { Form = F; }
----------------
Presumably this should assert that the type is an address form? Though I guess the other accessors don't have such assertions, so maybe that's OK.


================
Comment at: include/llvm/Object/ELFObjectFile.h:653
+  if (!SectionsOrErr) {
+    assert(false && "Unable to find section.");
+    return 0;
----------------
Yeah, as Rafael said - 'assert(false)' isn't appropriate in LLVM (llvm_unreachable is used instead - though as you point out, this isn't 100% adhered to/fixed/etc but generally should be the goal)

As for the error handling, llvm::Error has a special function for "ignore any errors here": llvm::consumeError(SectionsOrErr). Alternatively, you could handle the error with an assert:

  handleAllErrors(std::move(SectionsOrErr.getError()), [](const ErrorInfoBase &) { llvm_unreachable("There should always be sections here, since a section was passed into getSectionIndex in the first place"); });

Or the like.


================
Comment at: include/llvm/Object/ELFObjectFile.h:656
+  }
+  const Elf_Shdr *First = (*SectionsOrErr).begin();
+  return getSection(Sec) - First;
----------------
Use -> rather than (*x).y


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:911
+/// taken in account if LoadedObjectInfo interface is provided.
+static Expected<std::pair<uint64_t, uint64_t>>
+getSymbolInfo(const object::ObjectFile &Obj, const RelocationRef &Reloc,
----------------
seems like it might be cheap enough to introduce a struct here with named parameters, rather than a std::pair. I mean it's file local so the reader doesn't have to go far to check what the first/second are, but equally - seems pretty cheap to avoid them needing to (& might make the implementation easier to read too)

Actually regarding the implementation - might be easier to have two named variables, and stuff them into the result (be it a std::pair, or named struct) at the end.


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:1114
       object::RelocVisitor V(Obj);
-      uint64_t Val = V.visit(Reloc.getType(), Reloc, *SymAddrOrErr);
+      uint64_t Val = V.visit(Reloc.getType(), Reloc, (*SymInfoOrErr).first);
       if (V.error()) {
----------------
should be able to use -> here:

  SymInfoOrErr->first


================
Comment at: lib/DebugInfo/DWARF/DWARFContext.cpp:1121
       }
-      Map->insert({Reloc.getOffset(), {Val}});
+      llvm::RelocAddrEntry Rel = {(*SymInfoOrErr).second, Val};
+      Map->insert({Reloc.getOffset(), Rel});
----------------
and here


https://reviews.llvm.org/D33184