[ELF][Distributed ThinLTO] Do not generate empty index when bitcode object is linked

[weiwang]

Summary

When the same bitcode object file is given multiple times from the Command-line as lazy object file, empty index is generated which overwrites the one from thinlink. This could cause undefined symbols during final link.

[MaskRay]

lld/ELF/LTO.cpp

293

DenseMap<StringRef>

lld/test/ELF/lto/thinlto-index-only.ll

51

Don't repeat the comments. You can place the RUN lines immediately after the last block.

[weiwang]

address comments

[MaskRay]

For Bazel like build systems, it seems weird to have one object file in two modes, lazy and non-lazy. Do you know why it happens for your build system?
I think the change will improve robustness of lld's distributed ThinLTO usage so we probably need to make the change.

[weiwang]

In D133740#3785549, @MaskRay wrote:

For Bazel like build systems, it seems weird to have one object file in two modes, lazy and non-lazy. Do you know why it happens for your build system?
I think the change will improve robustness of lld's distributed ThinLTO usage so we probably need to make the change.

We've seen the case of 2 same lazy objects in buck, and the cause for that is still a mystery to us. @christylee may know more.

I listed the case of lazy and non-lazy just for the sake of test coverage.

[MaskRay]

LGTM.

[MaskRay]

In D133740#3786811, @weiwang wrote:

In D133740#3785549, @MaskRay wrote:

For Bazel like build systems, it seems weird to have one object file in two modes, lazy and non-lazy. Do you know why it happens for your build system?
I think the change will improve robustness of lld's distributed ThinLTO usage so we probably need to make the change.

We've seen the case of 2 same lazy objects in buck, and the cause for that is still a mystery to us. @christylee may know more.

Such a build system usually deduplicates linker input. I have seen duplicate archives but they are -lc, -lc++, etc, not from cc_library like targets.
Technically with linker scripts INPUT and GROUP an archive can be referenced by two cc_library, but that's a very rare case usually implying a user mistake but I feel the linker can guard against it, which will also help traditional build systems which don't do a great job deduplicating input.

I listed the case of lazy and non-lazy just for the sake of test coverage.

[christylee]

In D133740#3787303, @MaskRay wrote:

In D133740#3786811, @weiwang wrote:

In D133740#3785549, @MaskRay wrote:

For Bazel like build systems, it seems weird to have one object file in two modes, lazy and non-lazy. Do you know why it happens for your build system?
I think the change will improve robustness of lld's distributed ThinLTO usage so we probably need to make the change.

We've seen the case of 2 same lazy objects in buck, and the cause for that is still a mystery to us. @christylee may know more.

Such a build system usually deduplicates linker input. I have seen duplicate archives but they are -lc, -lc++, etc, not from cc_library like targets.
Technically with linker scripts INPUT and GROUP an archive can be referenced by two cc_library, but that's a very rare case usually implying a user mistake but I feel the linker can guard against it, which will also help traditional build systems which don't do a great job deduplicating input.

We've seen two cases:

1. Older rust compilers will duplicate transitive dependencies, including C++ dependencies.
2. Third-party prebuilt libraries that are wrapped with --start-group/--end-group and can be referenced by multiple cc_library. This is technically a user error but it's very hard to debug.

I listed the case of lazy and non-lazy just for the sake of test coverage.

lld/ELF/LTO.cpp

void BitcodeCompiler::add(BitcodeFile &f) {
   }
   checkError(ltoObj->add(std::move(f.obj), resols));
 }
 
 // If LazyObjFile has not been added to link, emit empty index files.
 // This is needed because this is what GNU gold plugin does and we have a
 // distributed build system that depends on that behavior.
 static void thinLTOCreateEmptyIndexFiles() {
+  DenseSet<StringRef> linkedBitCodeFiles;
+  for (BitcodeFile *f : ctx->bitcodeFiles)
+    linkedBitCodeFiles.insert(f->getName());
+
   for (BitcodeFile *f : ctx->lazyBitcodeFiles) {
     if (!f->lazy)
       continue;
+    if (linkedBitCodeFiles.contains(f->getName()))
+      continue;
     std::string path = replaceThinLTOSuffix(getThinLTOOutputFile(f->getName()));
     std::unique_ptr<raw_fd_ostream> os = openFile(path + ".thinlto.bc");
     if (!os)
       continue;
 
     ModuleSummaryIndex m(/*HaveGVs*/ false);
     m.setSkipModuleByDistributedBackend();
     writeIndexToFile(m, *os);

lld/test/ELF/lto/thinlto-index-only.ll

 ; RUN: ls 1.o.imports
 
 ;; Ensure LLD generates an empty index for each bitcode file even if all bitcode files are lazy.
 ; RUN: rm -f 1.o.thinlto.bc
 ; RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux-gnu /dev/null -o dummy.o
 ; RUN: ld.lld --plugin-opt=thinlto-index-only -shared dummy.o --start-lib 1.o --end-lib -o /dev/null
 ; RUN: ls 1.o.thinlto.bc
 
+;; Ensure when the same bitcode object is given as both lazy and non-lazy,
+;; LLD does not generate an empty index for the lazy object.
+; RUN: rm -f 2.o.thinlto.bc
+; RUN: ld.lld --plugin-opt=thinlto-index-only -shared 1.o 2.o --start-lib 2.o --end-lib -o /dev/null
+; RUN: llvm-dis < 2.o.thinlto.bc | grep -q '\^0 = module:'
+; RUN: rm -f 2.o.thinlto.bc
+; RUN: ld.lld --plugin-opt=thinlto-index-only -shared --start-lib 2.o --end-lib 2.o 1.o -o /dev/null
+; RUN: llvm-dis < 2.o.thinlto.bc | grep -q '\^0 = module:'
+
+;; Ensure when the same lazy bitcode object is given multiple times,
+;; no empty index file is generated if one of the copies is linked.
+; RUN: rm -f 2.o.thinlto.bc
+; RUN: ld.lld --plugin-opt=thinlto-index-only -shared 1.o --start-lib 2.o --end-lib --start-lib 2.o --end-lib -o /dev/null
+; RUN: llvm-dis < 2.o.thinlto.bc | grep -q '\^0 = module:'
+
 ; NM: T f
 
 ;; The backend index for this module contains summaries from itself and
 ;; Inputs/thinlto.ll, as it imports from the latter.
 ; BACKEND1: <MODULE_STRTAB_BLOCK
 ; BACKEND1-NEXT: <ENTRY {{.*}} record string = '1.o'
 ; BACKEND1-NEXT: <ENTRY {{.*}} record string = '2.o'
 ; BACKEND1-NEXT: </MODULE_STRTAB_BLOCK