[PATCH] D91533: [lib/Object] - Generalize the RelocationResolver API.

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/lib/Object/RelocationResolver.cpp:757
+  // contains the addend, provided by the caller.
+  return Resolver(/*Type=*/0, /*Offset=*/0, S, LocData,
+                  R.getRawDataRefImpl().p);
----------------
grimar wrote:
> jhenderson wrote:
> > grimar wrote:
> > > jhenderson wrote:
> > > > Is it simply not possible to provide Type and Offset information here? Otherwise, this code seems a little fragile to LLD deciding to try to use those fields for something.
> > > Yes, it is not possible without an owning object :(
> > > 
> > > ```
> > > inline uint64_t RelocationRef::getOffset() const {
> > >   return OwningObject->getRelocationOffset(RelocationPimpl);
> > > }
> > > 
> > > inline uint64_t RelocationRef::getType() const {
> > >   return OwningObject->getRelocationType(RelocationPimpl);
> > > }
> > > ```
> > I think we need some sort of note added to the LLD resolver then, indicating that the values will always be 0 then (possibly could be an assertion?), because the information isn't available. That would at least hint to future developers to not try to use them.
> The `type` and `offset` fields are commented out currently in `lld/ELF/DWARF.cpp`:
> 
> ```
>   static uint64_t resolve(uint64_t /*type*/, uint64_t /*offset*/, uint64_t s,
>                           uint64_t /*locData*/, int64_t addend) {
>     return s + addend;
>   }
> ```
> 
> Do you want me to add an explicit comment about them or uncomment and add an assertion?
Either an explicit comment or assertion would be fine by me. Probably comment, because the assertion will need explaining anyway.


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

https://reviews.llvm.org/D91533