[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/DWARFFormValue.h:50
     const uint8_t *data = nullptr;
+    uint64_t SectionIndex;      /// Section index for reference forms.
   };
----------------
dblaikie wrote:
> 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)
Your idea looks reasonable to me, but unfortunately use of unnamed struct looks to be not portable solution, at least I am using MSVS 2015 and it reports:
warning C4201: nonstandard extension used: nameless struct/union
when I am trying to do that :(

And use of named struct sure works here, but requires much more changes in code (particullary in DWARFFormValue.cpp) and I probably would not do that. 


================
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; }
----------------
dblaikie wrote:
> 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.
Yes, here I would also be consistent and go without assert. 

I actually was thinking about removing setters/getters here. Not sure I understand why do we need getRawUValue()/setUValue() pair for example. But that would be different patch.


================
Comment at: include/llvm/Object/ELFObjectFile.h:653
+  if (!SectionsOrErr) {
+    assert(false && "Unable to find section.");
+    return 0;
----------------
dblaikie wrote:
> 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.
Done. I used handleAllErrors and a shorter message because I really do not expect anyone see it..


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


================
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,
----------------
dblaikie wrote:
> 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.
Done. I agree that sometimes std::pairs looks weak in compare with struct for readability. Since here value is used for caching too, it is looks better probably to have named values.


================
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()) {
----------------
dblaikie wrote:
> should be able to use -> here:
> 
>   SymInfoOrErr->first
Done.


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


https://reviews.llvm.org/D33184