[ELF] Do not leave undefined symbols (specified by -init and -fini) if they are defined in non-fetched archive members

[ikudrin]

Summary

After D69985, symbols for -init and -fini were unconditionally marked as used even if they were just lazy symbols seen when scanning archives. That resulted in exposing them in the symbol table of an output file, as Undefined, which added unwanted dependencies. The patch fixes the issue by checking the kind of the symbols before marking.

[MaskRay]

I think this is about different expectation of _init and _fini behaviors.

@aaronpuchert Your comment https://bugzilla.opensuse.org/show_bug.cgi?id=1155108#c10 "However, it's actually a bug in all three linkers. It has been fixed in lld just today [1]" made me wonder whether https://bugs.llvm.org/show_bug.cgi?id=43927 was really a linker bug.

I need an LLD reproduce file (obtained via LLD_REPRODUCE=/tmp/rep.tar ... or -Wl,--reproduce=/tmp/rep.tar to under the matter better. An alternative fix to @ikudrin's problem is to revert D69985
openSUSE's OpenMP problem probably should be fixed by adding -u __kmp_internal_end_fini along with -Wl,-fini=__kmp_internal_end_fini instead of relying on the linker retaining the bitcode defined symbols.

[aaronpuchert]

In D83549#2144625, @MaskRay wrote:

@aaronpuchert Your comment https://bugzilla.opensuse.org/show_bug.cgi?id=1155108#c10 "However, it's actually a bug in all three linkers. It has been fixed in lld just today [1]" made me wonder whether https://bugs.llvm.org/show_bug.cgi?id=43927 was really a linker bug.

The linkers were behaving differently though: lld and bfd dropped the FINI entirely whereas gold had FINI=0, which lead to a crash. Which of these behaviors would be expected?

I need an LLD reproduce file (obtained via LLD_REPRODUCE=/tmp/rep.tar ... or -Wl,--reproduce=/tmp/rep.tar to under the matter better.

It should suffice to have a source file with an otherwise unused hidden visibility function that's used in a -init or -fini linker flag with LTO. If that doesn't help, I can also try putting some files together.

openSUSE's OpenMP problem probably should be fixed by adding -u __kmp_internal_end_fini along with -Wl,-fini=__kmp_internal_end_fini instead of relying on the linker retaining the bitcode defined symbols.

I think the problem here is that -Wl,-fini without -u works when you don't use LTO, but when you enable it the function is removed and with it the FINI entry in .dynamic. As a layperson I might expect (as the OpenMP writers did) that an explicit -fini flag on the command line is sufficient, and that the linker makes sure LTO doesn't drop the function since it is arguably used.

I can't really comment on this patch though, because I don't know under which circumstances one might use inits or finis that are undefined, and then expect them to be dropped.

[MaskRay]

OK, I think D69885 (lto/init-fini.ll) still makes sense. A bitcode symbol can behave like a regular symbol or an archive symbol.
For the -init case, a bitcode definition should behave more like a regular definition and -init can cause the definition to be emitted.

This patch makes LLD similar to GNU ld: there is no symbol table entry for -init:

cat > a.s <<e
 .globl _init, foo
 _init:
 foo:
   ret
e
as a.s -o a.o
ar rc a.a a.o
as /dev/null -o b.o
ld.bfd -init=foo b.o a.a  # a.a(a.o) not fetched & foo does not exist
ld.bfd b.o a.a  # a.a(a.o) not fetched & foo does not exist

This makes sense because linking in a dropped archive should not affect the symbol table. This intention is not clearly given in the description.

lld/test/ELF/archive-init-fini.s

3 ↗

(On Diff #277006)

If you delete _start from init-fini.s, you can add this test into init-fini.s

7 ↗

(On Diff #277006)

%t.o

10 ↗

(On Diff #277006)

%t.out -> %t

[ikudrin]

• Moved the test into init-fini.s.

Thanks @MaskRay for updating the title! I'll use it for the commit message.

lld/test/ELF/archive-init-fini.s

7 ↗

(On Diff #277006)

This and the next comment are not done because init-fini.s uses a different naming scheme.

[MaskRay]

LGTM.

lld/ELF/Driver.cpp

template <class ELFT> void LinkerDriver::link(opt::InputArgList &args) {
   if (Symbol *sym = symtab->find(config->entry))
     handleUndefined(sym);
 
   // Handle the `--undefined-glob <pattern>` options.
   for (StringRef pat : args::getStrings(args, OPT_undefined_glob))
     handleUndefinedGlob(pat);
 
   // Mark -init and -fini symbols so that the LTO doesn't eliminate them.
-  if (Symbol *sym = symtab->find(config->init))
+  if (Symbol *sym = dyn_cast_or_null<Defined>(symtab->find(config->init)))
     sym->isUsedInRegularObj = true;
-  if (Symbol *sym = symtab->find(config->fini))
+  if (Symbol *sym = dyn_cast_or_null<Defined>(symtab->find(config->fini)))
     sym->isUsedInRegularObj = true;
 
   // If any of our inputs are bitcode files, the LTO code generator may create
   // references to certain library functions that might not be explicit in the
   // bitcode file's symbol table. If any of those library functions are defined
   // in a bitcode file in an archive member, we need to arrange to use LTO to
   // compile those archive members by adding them to the link beforehand.
   //

lld/test/ELF/init-fini.s

 // NOENTRY: DynamicSection [
 // NOENTRY-NOT: INIT
 // NOENTRY-NOT: FINI
 // NOENTRY: ]
 // NOENTRY: Symbols [
 // NOENTRY-NOT: Name: _unknown
 // NOENTRY: ]
 
+// Should not add entries for "_init" and "_fini" to the symbol table
+// if the symbols are defined in non-fetched achive members.
+// RUN: rm -f %t.a
+// RUN: llvm-ar rcs %t.a %t
+// RUN: ld.lld -shared -m elf_x86_64 -e _unknown %t.a -o %t.so
+// RUN: llvm-nm %t.so | \
+// RUN:   FileCheck %s --implicit-check-not=_init --implicit-check-not=_fini
+
 .global _start,_init,_fini,_foo,_bar,_undef
 _start:
 _init = 0x11010
 _fini = 0x11020
 _foo  = 0x11030
 _bar  = 0x11040