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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu 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);
 
----------------
pcc wrote:
> 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?
I guess that Ret should just be initialized to (uint64_t)-1?

By the way, all that confusion could have been prevented if you have added a test case to this patch from beginning, so please always include a test case (from the beginning) to demonstrate what you are fixing. I guess that without a test, even you are not sure that this fix is correct...


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D47602