[PATCH] D83748: Adding support for PCRel32GOTLoad in ELF x86 for the jitlinker

[Lang Hames via Phabricator via llvm-commits]

lhames added inline comments.


================
Comment at: llvm/lib/ExecutionEngine/JITLink/ELF_x86_64.cpp:409
         });
-
+        auto SymbolIndex = Rela.getSymbol(false);
         auto Symbol = Obj.getRelocationSymbol(&Rela, &SymTab);
----------------
jaredwy wrote:
> lhames wrote:
> > MaskRay wrote:
> > > https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable
> > > 
> > > Please spell out the exact type if it is non-obvious...
> > I'm comfortable with auto being used where the variable name clearly implies valid usage, in this case as an index.
> > 
> > I understand there's room for differences in opinion there, but I don't want to delay this landing: We want to get it into LLVM 11 so that people can start testing/living on it.
> I share langs view, to me that the name is index the type almost doesn't matter, but understand there could be a difference of opinion there. 
> 
> There is also the consideration that this just supports x86 at the moment, Given that the we want to follow up with 32 bit support and this class is going to become very templated to match the ElfFile<T> so keeping it auto will help with reducing code churn.
> 
> Are you ok with landing it as is? 
> Are you ok with landing it as is?

Yep. Please go ahead and commit.

We can address style issues in follow up patches. The focus for now should be on getting as much ELF functionality as we reasonably can into the LLVM 11 release branch so that potential clients can start testing and providing feedback.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83748