[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:
> 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 ?
Also, may be we should just rename this variable to something more common, like SymbolIndex and leave it as is with current restrictions implemented in this patch ?
Seems it should work fine for linux kernel, it does not link agains DSO and not uses -pie/-shared.


https://reviews.llvm.org/D28612