[PATCH] D52830: [ELF] - Set sh_info and sh_link for .rela.plt sections.

[Ed Maste via Phabricator via llvm-commits]

emaste added a comment.

This change LGTM.



================
Comment at: ELF/SyntheticSections.cpp:1496-1497
   // dynamic symbol and we don't need a dynamic symbol table. If that
-  // is the case, just use 0 as the link.
-  getParent()->Link =
-      In.DynSymTab ? In.DynSymTab->getParent()->SectionIndex : 0;
+  // is the case, just use the index of the regular symbol table section
+  // as the link to suppress the warnings reported by GNU tools like readelf.
+  getParent()->Link = In.DynSymTab ? In.DynSymTab->getParent()->SectionIndex
----------------
grimar wrote:
> ruiu wrote:
> > You do that not because GNU tools complain but because that's the right thing to do according to ELF spec, no?
> I would not say it is clear to me that it is required.
> Spec says about `sh_link` that it contains "The section header index of the associated symbol table. ".
> In the case of `.rela.iplt` we can live without the symbol table. In that case, this association does not make sense.
> 
> Spec does not say what should we do when there is no symbol table associated. With the same success, it probably
> could be an empty dynamic section we could create for consistency.
> 
> Or, another example, with `-strip-all` we do not create the `.symtab`. And tools would complain again I guess,
> though I see nothing obviously wrong with that scenario.
> 
> Perhaps I am overthinking. I'll shorten the comment.
The issue I encountered was with `sh_info` where according to the ELF spec I believe this change is a correctness fix (and did not trigger warnings from GNU tools anyhow).

For `sh_link` in an ifunc-using statically-linked binary I think it is arguable that this change is only to appease GNU readelf.


https://reviews.llvm.org/D52830