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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

Can you please investigate the Linux kernel why they are using this option before proceeding with this patch? The fact that they are using the option is not strong enough to justify that we need to implement it, because the use might be easily replaced by other features. And if that's the case, we don't need this change. We need to understand use cases before checking in any new feature.



================
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:
> > 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.


https://reviews.llvm.org/D28612