[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

1160

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.

1161

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

45

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

1161

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

45

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 possibly internalizing the symbol.
+        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
+;; file 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.
+
+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.o
+RUN: llvm-ar rc lib.a obj.o bc.o
+RUN: ld.lld start.o lib.a -o /dev/null
+
+;--- 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()
+
+;; Note. "foo" is defined after declaring "bar" so that loading the regular
+;; object file "obj.o" is triggered before processing the definition of "foo".
+define linkonce_odr void @foo() comdat {
+  ret void
+}
+
+define void @baz() {
+  call void @foo()
+  call void @bar()
+  ret void
+}