Correct aligment computation for shared object symbols

[shenhan]

Summary

The original computation for shared object symbol alignment is wrong when st_value equals 0.

It is very unusual for dso symbols to have st_value equal 0. But when it happens, it causes obscure run time bugs.

[ruiu]

Ahh, that's obviously a bug. Thank you for finding! Could you write a test?

[shenhan]

Thanks Rui. I just updated the patch a little bit, that - st_value is only valid for alignment when symbol shndx is set to SHN_COMMON.

I'll add a test case and fix other failing tests. Thanks.

[ruiu]

ELF/InputFiles.cpp

905

Are you sure if this is correct?

[pcc]

ELF/InputFiles.cpp

905

It doesn't look right to me -- a dynamic symbol table should not contain symbols in SHN_COMMON.

[shenhan]

ELF/InputFiles.cpp

905

From ELF specification:

If a symbol’s value refers to a specific location within a section, its section index member, st_shndx,
holds an index into the section header table. (shenhan: if st_shndx contains a valid section index, then st_value is a specific location)

Some special section index values give other semantics.

SHN_COMMON The symbol labels a common block that has not yet been allocated. The symbol’s value
gives alignment constraints, similar to a section’s sh_addralign member. That is, the
link editor will allocate the storage for the symbol at an address that is a multiple of
st_value. (shenhan: if st_shndx equals SHN_COMMON, then st_value is the alignment requirement).

I put my understandings in the (shenhan: ...)

[ruiu]

Common symbol is for (linkable) object files. Linked object files (i.e. execuables or DSOs) don't contain common symbols.

[pcc]

ELF/InputFiles.cpp

906

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.

[ruiu]

ELF/InputFiles.cpp

906

Hmm, you are probably right. Han, what are you trying to fix by this change?

[shenhan]

ELF/InputFiles.cpp

906

I cc'ed you and @pcc in the internal bug.

[shenhan]

@pcc

ELF/InputFiles.cpp

906

@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.

[ruiu]

ELF/InputFiles.cpp

906

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...

[pcc]

ELF/InputFiles.cpp

906

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?

[shenhan]

Thanks @ruiu .

@pcc do you agree with initializing Ret to uint64-1?

[ruiu]

Can you first add a test to demonstrate the issue you are fixing?

[pcc]

In D47602#1118200, @shenhan wrote:

Thanks @ruiu .

@pcc do you agree with initializing Ret to uint64-1?

Sounds good. Even in the case where the virtual address is intentionally 0, we would still want to use the sh_addralign alignment.

[shenhan]

Thanks @ruiu Updated the patch, also attached a test case.

[pcc]

ELF/InputFiles.cpp

899

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?

test/ELF/relocation-copy-alignment.s

4

What are the st_values of the symbols in %t.so? I don't think this would result in any of them being 0. You might want to consider using yaml2obj to create the .so file.

[shenhan]

ELF/InputFiles.cpp

899

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

[pcc]

ELF/InputFiles.cpp

899

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.

[ruiu]

test/ELF/Inputs/relocation-copy-alignment.s

7

Do not include a trailing blank line. I'd recommend you do git diff before sending a patch and visit reviews.llvm.org to review your patch yourself on the web to catch this kind of errors.

test/ELF/relocation-copy-alignment.s

4

I want to see a test case with llvm-mc because it would be more realistic test case and demonstrate a real problem. With yaml2obj, it is perhaps too easy to create a nonsensical object file.

[pcc]

test/ELF/relocation-copy-alignment.s

4

I'm not sure if it's actually possible to do that using llvm-mc, though. It may be possible using a linker script, but that seems more fragile than testing for the property more directly.

[ruiu]

test/ELF/relocation-copy-alignment.s

4

That makes me wonder how this issue caused a trouble to our internal software. Looks like I need to take a look at that program...

[shenhan]

ELF/InputFiles.cpp

899

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?

[pcc]

ELF/InputFiles.cpp

899

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.

[shenhan]

@pcc thanks for the review. Updated the patch -

1. if getAlignment sees a ABS symbols, returns 0
2. initialize Ret to UINT64_MAX
3. in "addCopyRelSymbol", errors out if alignment is 0.
4. unit test case that tests for proper handling of ABS symbols.

[ruiu]

ELF/Relocations.cpp

532–534 ↗

(On Diff #149538)

Can you really trigger this? Please always add a test when you add a new feature.

[pcc]

ELF/InputFiles.cpp

909

I was thinking that instead of erroring here you would return 0. Then you wouldn't need the special case for SHN_ABS.

[shenhan]

ELF/Relocations.cpp

532–534 ↗

(On Diff #149538)

No, from the discussion, this could never be triggered, otherwise this is a linker error.
Replaced with assert.

[ruiu]

ELF/Relocations.cpp

532–534 ↗

(On Diff #149538)

We don't add use assert() that much in lld. Please just remove.

[shenhan]

ELF/InputFiles.cpp

909

Yup, done.

[pcc]

test/ELF/copy-relocation-zero-abs-addr.s

15 ↗

(On Diff #149548)

I think this is still missing a test showing that a non-SHN_ABS symbol with st_value=0 will be aligned using the section's sh_addralign.

[shenhan]

test/ELF/copy-relocation-zero-abs-addr.s

15 ↗

(On Diff #149548)

Thanks. Added test case new copy-relocation-zero-nonabs-addr.s.

[shenhan]

Mind take another look?

[ruiu]

ELF/InputFiles.cpp

911

I'm not too excited about change. This error message was to catch a nearly-broken object file that has insanely large alignment requirement. Now you are making a change to ignore such object file.

[pcc]

ELF/Relocations.cpp

532–534 ↗

(On Diff #149538)

It's possible to get here with something like (the somewhat contrived)

.globl abs
abs = 0
.size abs, 1

in a .so, I think. I was imagining that we'd change the if (SymSize == 0) above to be if (SymSize == 0 || SS.Alignment == 0). That would cover this case as well as the huge alignment case.

[shenhan]

Re-write the getAlignment function, keep the original logic but with the 3 additional tests:

1. return 0 for symbols with absolute address
2. for symbols with zero st_value, return section alignment.
3. for symbols with zero st_value and whose section alignment does not exist, return 0

ELF/Relocations.cpp

532–534 ↗

(On Diff #149538)

Done. Also, rewrite the getAlignment function, so huge alignment is erred out in getAlignment. And absolute symbols will error out here.

[pcc]

ELF/InputFiles.cpp

911

It should be possible to link against symbols with huge alignments, we just can't create copy relocations for them. So I think you can make InputFiles.cpp look more like your previous revision (i.e. return 0 on huge alignments). The check that you've added to Relocations.cpp should be enough to make sure that we error out in that case. We don't have to produce a good error message in that case because we expect it to be rare.

[shenhan]

ELF/InputFiles.cpp

911

Thanks, done.

[pcc]

This LGTM if @ruiu is happy.

[shenhan]

@ruiu whatś your opinion on this?

[ruiu]

LGTM

ELF/Relocations.cpp

532 ↗

(On Diff #150050)

If this code is reachable by giving a (even contrived) object file, it should've been error() instead of fatal(), but you don't need to fix that since that's not new code. I'll address that in a follow-up patch.

ELF/InputFiles.cpp

 
 // We do not usually care about alignments of data in shared object
 // files because the loader takes care of it. However, if we promote a
 // DSO symbol to point to .bss due to copy relocation, we need to keep
 // the original alignment requirements. We infer it in this function.
 template <class ELFT>
 uint32_t SharedFile<ELFT>::getAlignment(ArrayRef<Elf_Shdr> Sections,
                                         const Elf_Sym &Sym) {
-  uint64_t Ret = 1;
+  // 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())))
+    return 1;
+  uint64_t Ret = UINT64_MAX;
   if (Sym.st_value)
     Ret = 1ULL << countTrailingZeros((uint64_t)Sym.st_value);
   if (0 < Sym.st_shndx && Sym.st_shndx < Sections.size())
     Ret = std::min<uint64_t>(Ret, Sections[Sym.st_shndx].sh_addralign);
 
   if (Ret > UINT32_MAX)
     error(toString(this) + ": alignment too large: " +
           CHECK(Sym.getName(this->StringTable), this));
   return Ret;
 }
 
 // Fully parse the shared object file. This must be called after parseSoName().
 //
 // This function parses symbol versions. If a DSO has version information,
 // the file has a ".gnu.version_d" section which contains symbol version
 // definitions. Each symbol is associated to one version through a table in
 // ".gnu.version" section. That table is a parallel array for the symbol

test/ELF/Inputs/relocation-copy-alignment.s

+.data
+.type dso_symbol,@object
+.comm dso_symbol,4,4
+
+.type dso_symbol1,@object
+.comm dso_symbol1,16,16
+

test/ELF/relocation-copy-alignment.s

+// REQUIRES: x86
+// RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %s -o %t.o
+// RUN: llvm-mc -filetype=obj -triple=x86_64-pc-linux %p/Inputs/relocation-copy-alignment.s -o %t2.o
+// RUN: ld.lld -shared %t2.o -o %t.so
+// RUN: ld.lld %t.o %t.so -o %t3
+// RUN: llvm-readobj -s -r --expand-relocs %t3 | FileCheck %s
+
+.text
+.global _start
+_start:
+movl $5, dso_symbol
+movl $6, dso_symbol1
+
+// Symbol alignment information must be kept.
+// CHECK:       Section (5) .rela.dyn {
+// CHECK-NEXT:    Relocation {
+// CHECK-NEXT:      Offset: 0x203000
+// CHECK-NEXT:      Type: R_X86_64_COPY
+// CHECK-NEXT:      Symbol: dso_symbol
+// CHECK-NEXT:      Addend: 0
+// CHECK-NEXT:    }
+// CHECK-NEXT:    Relocation {
+// CHECK-NEXT:      Offset: 0x203010
+// CHECK-NEXT:      Type: R_X86_64_COPY
+// CHECK-NEXT:      Symbol: dso_symbol1
+// CHECK-NEXT:      Addend: 0
+// CHECK-NEXT:    }
+// CHECK-NEXT:  }