[PATCH] D47602: Correct aligment computation for shared object symbols

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: ELF/InputFiles.cpp:899
+  // If no available alignment information from either section or
+  // symbol, return 1.
+  if (!(Sym.st_value || (0 < Sym.st_shndx && Sym.st_shndx < Sections.size())))
----------------
shenhan wrote:
> pcc wrote:
> > shenhan wrote:
> > > pcc wrote:
> > > > Is that correct? If there is a symbol at virtual address 0 in an unknown section, doesn't that mean that we should error out via the `Ret > UINT32_MAX` code path below since we don't know what alignment to use?
> > > We have quite a few tests that link with libm.so, which has quite a few symbols with st_value as 0 and shndx as "ABS".  Shall we handle symbols with "ABS" shndex separately (if symbols has absolute address, linker does not need to change its address or its address is not relevant, right?) 
> > > 
> > >    221: 0000000000000000     0 OBJECT  GLOBAL DEFAULT  ABS GLIBC_2.4
> > >    238: 0000000000000000     0 OBJECT  GLOBAL DEFAULT  ABS GLIBC_2.2.5
> > >    394: 0000000000000000     0 OBJECT  GLOBAL DEFAULT  ABS GLIBC_2.15
> > >    399: 0000000000000000     0 OBJECT  GLOBAL DEFAULT  ABS GLIBC_2.18
> > >   1736: 0000000000000000     0 OBJECT  GLOBAL DEFAULT  ABS GLIBC_2.15
> > >   1922: 0000000000000000     0 OBJECT  GLOBAL DEFAULT  ABS GLIBC_2.4
> > >   1953: 0000000000000000     0 OBJECT  GLOBAL DEFAULT  ABS GLIBC_2.18
> > >   2136: 0000000000000000     0 OBJECT  GLOBAL DEFAULT  ABS GLIBC_2.2.5
> > We should only end up in this function if we need to create a copy relocation for a symbol. Those symbols appear to be associated with symbol versions so we wouldn't expect to be linking against them directly. Furthermore I don't think there is anything sensible that we can do if we're asked to create a copy relocation for an absolute symbol, so I think the correct behaviour would be to error out.
> By reading/debugging the code - this "getAlignment" is called by "parseRest" and parseRest iterates all GlobalELFSyms() and calls getAlignment for each symbol (except when sym is undefined or binding is STB_LOCAL or its a mips symbol).
> 
> For the above example, GLIBC_2.4 is a global defined symbol, so lld tries to create COPY_RELOC for the symbol, the logic is correct here. 
> 
> So shall we exclude such symbols (global defined symbol but with abs address 0)  at the first place, eg. in parseRest?
You're right, we do end up querying the alignment for every symbol here:
http://llvm-cs.pcc.me.uk/tools/lld/ELF/InputFiles.cpp#967

The alignment is stored in a field here:
http://llvm-cs.pcc.me.uk/tools/lld/ELF/Symbols.h#264

But the only user of the alignment is in addCopyRelSymbol here:
http://llvm-cs.pcc.me.uk/tools/lld/ELF/Relocations.cpp#537

I think you'll find that we only call addCopyRelSymbol on symbols that we create a copy relocation for, and GLIBC_2.4 shouldn't be one of them.

We need to add absolute symbols to the symbol table so that they can be linked against, but we don't need to allow creating copy relocations for them. It would probably be fine to store 0 in the alignment field for such symbols and error out in addCopyRelSymbol if we see a 0.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D47602