[PATCH] D28612: [ELF] - Added support for --emit-relocs.

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: ELF/Driver.cpp:166-169
+    if (Config->EmitRelocs) {
+      error("--emit-reloc does not allow link against dynamic object " + Path);
+      return;
+    }
----------------
grimar wrote:
> ruiu wrote:
> > grimar wrote:
> > > ruiu wrote:
> > > > Why do you need this restriction?
> > > Like I mentioned in patch description, currently we use Symbol->DynsymIndex to store symbol index in .symtab for -relocatable output.
> > > That works because -r means static linking, so .dynsym not exist.
> > > We need to know symtab symbol index when perform fixup of relocations in InputSection<ELFT>::copyRelocations(),
> > > because .rela.text is linked with .symtab.
> > > 
> > > This patch reuses Symbol->DynsymIndex for the same purpose for --emit-reloc.
> > > That should work for linux kerner I think. So I had to add the same restrictions for now.
> > > 
> > > That restrictions probably can be fixed with adding one more variable to Symbol, like SymtabIndex instead of reusing
> > > DynsymIndex. But I am not sure if it is worst to do ?
> > That wasn't good in the first place. We shouldn't have reused a field for different purposes in such a obscure way. That needs to be fixed first.
> Yep. I think we did it to reduce Symbol size, but I never liked reusing this variable, I am happy to prepare the patch for it :)
I prepared demo patch to show one of the ways to avoid reusing it. It is D28773.
Actually it seems that introducing something like Symbol::SymtabIndex would be simpler.
Though that makes Symbol class larger and -r/--emit-relocs are not often used probably to do that.
What do you think we should prefer ?


https://reviews.llvm.org/D28612