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

[Han Shen via Phabricator via llvm-commits]

shenhan 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())))
----------------
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?


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D47602