This is an archive of the discontinued LLVM Phabricator instance.

Fix an issue that common symbols are not internalized under some condition.
ClosedPublic

Authored by ruiu on Jun 24 2019, 10:15 PM.

Details

Summary

r360841 introduced CommonSymbol class. An unintended behavioral change
introduced by that change was that common symbols are not internalized
by LTO under some condition, as reported in
https://bugs.llvm.org/show_bug.cgi?id=41978.

This patch fixes that issue.

The issue occurred under the following condition:

  1. There exists a common symbol
  2. At least one DSO is given to lld

If the above conditions are met, Symbol::includeInDynsym() returned a
wrong value for a common symbol.

Event Timeline

ruiu created this revision.Jun 24 2019, 10:15 PM

compileBitcodeFiles<ELFT>(); is executed before replaceCommonSymbols().
When includeInDynsym() is called to compute VisibileToRegularObj, it unnecessarily returned true for a common symbol.

The description can be generalized a bit:

The issue occurred under the following condition:

  • There exists a common symbol.
  • At least one DSO is given to lld, or -pie (i.e. ld.lld t.o t1.so or ld.lld t.o -pie)
lld/test/ELF/lto/common4.ll
2

llvm-mc -filetype=obj -triple=x86_64-unknown-linux /dev/null -o %t.so.o

/dev/null has been used in many other tests.

6

Nit: put input files together, %t.o %t.so

8

You may add another RUN+CHECK lines: ld.lld -pie %t.o -o %t.exe -save-temps

The purpose of the input DSO is to make HasDynSymTab true.

MaskRay added inline comments.Jun 24 2019, 11:22 PM
lld/test/ELF/lto/common4.ll
13

Probably add a comment saying @a does not interpose a symbol in a DSO so it does not have to be exported.

ruiu updated this revision to Diff 206364.Jun 24 2019, 11:24 PM
ruiu marked 3 inline comments as done.
  • address review coments
ruiu updated this revision to Diff 206366.Jun 24 2019, 11:25 PM
  • update the test
ruiu updated this revision to Diff 206367.Jun 24 2019, 11:28 PM
  • add a comment to the test file
MaskRay accepted this revision.Jun 24 2019, 11:51 PM
MaskRay added inline comments.
lld/test/ELF/lto/common4.ll
5

I am not sure if module-local is a proper term here.

@a does not have to exported (ExportDynamic) because there is no DSO has an undefined symbol named a.

This revision is now accepted and ready to land.Jun 24 2019, 11:51 PM
This revision was automatically updated to reflect the committed changes.