[ELF] Prevent internalizing used comdat symbol

[ikudrin]

Summary

When a comdat symbol is defined in both bitcode and regular object files, which are contained in the same archive, the linker could lose the flag that the symbol is used in the regular object file and allow LTO to internalize it, which led to "error: undefined symbol".

The issue was introduced in D79300.

[peter.smith]

Looks good to me, just some suggestions on the comments.

lld/ELF/InputFiles.cpp

1150–1153

Would be good to include LTO in the comment to give some context. For example:

// Prevent LTO from internalizing the symbol in case there is a reference to this symbol from this file.

1151

Presumably we would only need to do this if there were a reference (relocation) from outside the group, i.e. if the caller to foo is getting discarded as well then it doesn't matter. However I don't think it is worth the extra complexity to check for this case.

lld/test/ELF/lto/comdat-mixed-archive.test

46

IIUC we have the sequence:

start.o refs baz
bc.o fetched for baz
Group foo selected from bc.o
obj.o fetched for bar
Group foo rejected from obj.o
foo loses usedInRegularObj
LTO internalizes foo
reference to foo from bar is undefined.

If that is the case may be worth adding

;; group foo in obj.o is rejected in favor of bc.o but we need to prevent LTO from
;; internalizing foo as there is still a reference from outside the group in obj.o.

[ikudrin]

• Updated comments. Thanks for the suggestions, @psmith!

lld/ELF/InputFiles.cpp

1151

I thought the same.

The fun fact is that moving the definition of foo in bc.ll before declare void @bar() makes the symbol foo at this point Defined, not Lazy, so sym->resolve() will be called instead of replace(), and the flag isUsedInRegularObj will be set.

lld/test/ELF/lto/comdat-mixed-archive.test

46

Your analysis is accurate. I'll add your suggested comment in the common description of the test because this Note is more situational.

[MaskRay]

LGTM. Previously resolve sets the isUsedInRegularObj bit. replace loses the bit.

lld/test/ELF/lto/comdat-mixed-archive.test

49

Maybe use llvm-nm %bc.o to test the symbol order.

You can also name the bitcode *.bc to be different from relocatable object files.

[MaskRay]

lld/test/ELF/lto/comdat-mixed-archive.test

22

If we want this to be robust or detect stale test, test -y foo -y bar output.

[ikudrin]

• Added checks suggested by @MaskRay. Thanks! Is that what you meant?

lld/ELF/InputFiles.cpp

if (sec == &InputSection::discarded) {
       // !ArchiveFile::parsed or LazyObjFile::extracted means that the file
       // containing this object has not finished processing, i.e. this symbol is
       // a result of a lazy symbol extract. We should demote the lazy symbol to
       // an Undefined so that any relocations outside of the group to it will
       // trigger a discarded section error.
       if ((sym->symbolKind == Symbol::LazyArchiveKind &&
            !cast<ArchiveFile>(sym->file)->parsed) ||
           (sym->symbolKind == Symbol::LazyObjectKind &&
-           cast<LazyObjFile>(sym->file)->extracted))
+           cast<LazyObjFile>(sym->file)->extracted)) {
         sym->replace(und);
-      else
+        // Prevent LTO from internalizing the symbol in case there is a
+        // reference to this symbol from this file.
+        sym->isUsedInRegularObj = true;
+      } else
         sym->resolve(und);
       continue;
     }
 
     // Handle global defined symbols.
     if (binding == STB_GLOBAL || binding == STB_WEAK ||
         binding == STB_GNU_UNIQUE) {
       this->symbols[i]->resolve(

lld/test/ELF/lto/comdat-mixed-archive.test

+REQUIRES: x86
+
+;; This checks a case when an archive contains a bitcode and a regular object
+;; files, and a comdat symbol is defined and used in both of them. Previously,
+;; lld could lose the flag that the symbol is used in a regular object file
+;; which led to the LTO backend internalizing the symbol and the linker
+;; reporting an "undefined symbol" error.
+
+;; In this test, group "foo" in "obj.o" is rejected in favor of "bc.bc" but we
+;; need to prevent LTO from internalizing "foo" as there is still a reference
+;; from outside the group in "obj.o".
+
+RUN: rm -rf %t.dir
+RUN: split-file %s %t.dir
+RUN: cd %t.dir
+
+RUN: llvm-mc -filetype=obj -triple=x86_64 start.s -o start.o
+RUN: llvm-mc -filetype=obj -triple=x86_64 obj.s -o obj.o
+RUN: llvm-as bc.ll -o bc.bc
+RUN: llvm-nm bc.bc --no-sort | FileCheck %s --check-prefix=BCSYM
+RUN: llvm-ar rc lib.a obj.o bc.bc
+RUN: ld.lld start.o lib.a -y foo -y bar -o /dev/null | FileCheck %s --check-prefix=TRACE
+
+;; "bar" should be encountered before "foo" so that it triggers the loading of
+;; "obj.o" while "foo" is still lazy.
+BCSYM:      U bar
+BCSYM-NEXT: W foo
+
+;; Check that the symbols are handled in the expected order.
+TRACE:      lib.a: lazy definition of foo
+TRACE-NEXT: lib.a: lazy definition of bar
+TRACE-NEXT: lib.a(bc.bc): reference to bar
+TRACE-NEXT: lib.a(obj.o): reference to foo
+TRACE-NEXT: lib.a(obj.o): definition of bar
+TRACE-NEXT: lib.a(bc.bc): definition of foo
+TRACE-NEXT: <internal>: reference to foo
+;; The definition of "foo" is visible outside the LTO result.
+TRACE-NEXT: lto.tmp: definition of foo
+TRACE-NEXT: lto.tmp: reference to bar
+
+;--- start.s
+  .global _start, baz
+_start:
+  call baz
+
+;--- obj.s
+  .weak foo
+  .global bar
+
+  .section .text.foo,"axG",@progbits,foo,comdat
+foo:
+  ret
+
+  .section .text.bar,"ax",@progbits
+bar:
+  call foo
+
+;--- bc.ll
+target triple = "x86_64-unknown-linux-gnu"
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+
+$foo = comdat any
+
+declare void @bar()
+
+define linkonce_odr void @foo() comdat {
+  ret void
+}
+
+define void @baz() {
+  call void @foo()
+  call void @bar()
+  ret void
+}