[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:904
   if (0 < Sym.st_shndx && Sym.st_shndx < Sections.size())
-    Ret = std::min<uint64_t>(Ret, Sections[Sym.st_shndx].sh_addralign);
+    Ret = std::max<uint64_t>(Ret, Sections[Sym.st_shndx].sh_addralign);
 
----------------
shenhan wrote:
> shenhan wrote:
> > ruiu wrote:
> > > pcc wrote:
> > > > Actually, now that I think about it, I think this was correct before. The `sh_addralign` field says what the maximum alignment of anything in the section is going to be, and the code on lines 899-900 (in the old code) uses the symbol's `st_value` to infer a smaller alignment wherever possible.
> > > Hmm, you are probably right. Han, what are you trying to fix by this change?
> > I cc'ed you and @pcc in the internal bug.
> @pcc I debugged this function, the first 2 symbols this function processes are:
> 
> symbol1: shndx=18 st_value=0
> symbol2: shndx=18 st_value=112
> 
> It appears to me st_value is more like a offset than a alignment information. (But it could be interpreted as alignment as well if we count the tailing zeros)
> 
> However, st_value=0 for symbol1 results in the final RET being 1 in the original code, that is apparently not correct.
> 
> So definitely the alignment calculation here is not correct. That's why if I change the variable name (so it does not appear as the first symbol) the program runs correctly.
> 
> 
> 
In a linked executable or shared library the `st_value` is supposed to be a virtual address, not the offset from the start of a section. So it's surprising to see st_value=0 here, unless maybe this is a special symbol like `__ehdr_start`. Maybe the problem is with the producer of the .so?


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D47602