[lld][ELF] Fix lazy ThinLTO index writing in thin archives

[aidengrossman]

Summary

Currently when the --thinlto-emit-index-files is used with LLD and a
thin archive is passed containing references to object files to link
against where the object files are in a different folder than the thin
archive and some of the archives aren't linked against (ie stay lazy),
the empty index file writer ends up trying to write to a path that
doesn't exist. This patch changes the behavior of that function to use
the path of the obj member of the BitcodeFile object rather than just
the path of the BitcodeFile object itself, which matches the behavior of
the default (non-lazy) case.

Fixes #57963

Regression test added.

[aidengrossman]

This fixes the issue that we were seeing in #57963 and shouldn't change any of the behavior in the base case (outside of thin archives) and should just match the behavior that happens in the non-lazy case. I'm not too familiar with this area of the code base, so if I've taken the wrong approach in any specific area, let me know and I can adjust.

[tejohnson]

I read through Issue #57963 just now but still a little unsure of a couple things:

What is the path that is written for the test case without this patch?

This is just for MLGO, right? It isn't going to make this case work for distributed ThinLTO all the way through the ThinLTO backends I believe - is that correct?

It does seem fine to me and consistent with the path used when the bitcode file is actually added to the link, however.

[aidengrossman]

The path that is written to without this patch is the path to the object file relative from the path of the thin archive, but it expands this path over the current working directory rather than the path of the thin archive. For example if the thin archive is in ./test-folder-2/lib.a, pointing to archive files in ./test-folder-1, the thinLTOCreateEmptyIndexFiles() function will try and write the *.thinlto.bc files to ../test-folder-1/* relative to the current working directory (set up in the examples as containing test-folder-1 and test-folder-2, ie one level up). Even just fixing the path issue, the current implementation also names the files differently (ie c.o.thinlto.bc instead of in the non-lazy case lib.a(c.o at xxx).thinlto.bc.

I have just tested this on a ThinLTO Chromium compile for preparing a corpus for MLGO. I don't believe this patch will make this case work for distributed ThinLTO all the way through the backends, but I'm not familiar enough with how distributed ThinLTO picks up all the necessary files in order to say anything conclusive.

[tejohnson]

In D135014#3829154, @aidengrossman wrote:

The path that is written to without this patch is the path to the object file relative from the path of the thin archive, but it expands this path over the current working directory rather than the path of the thin archive. For example if the thin archive is in ./test-folder-2/lib.a, pointing to archive files in ./test-folder-1, the thinLTOCreateEmptyIndexFiles() function will try and write the *.thinlto.bc files to ../test-folder-1/* relative to the current working directory (set up in the examples as containing test-folder-1 and test-folder-2, ie one level up). Even just fixing the path issue, the current implementation also names the files differently (ie c.o.thinlto.bc instead of in the non-lazy case lib.a(c.o at xxx).thinlto.bc.

Got it, thanks.

I have just tested this on a ThinLTO Chromium compile for preparing a corpus for MLGO. I don't believe this patch will make this case work for distributed ThinLTO all the way through the backends, but I'm not familiar enough with how distributed ThinLTO picks up all the necessary files in order to say anything conclusive.

Ok, right. For a full distributed ThinLTO the archives currently need to be expanded and passed as individual object files on the command line surrounded by --start-lib/--end-lib pairs to identify the libraries.

Change lgtm, other than question about test but please wait for @MaskRay to review, especially to see if he wants any comments in the code or test about this being for MLGO but not really enough for supporting thin archives with distributed ThinLTO.

lld/test/ELF/lto/thinlto-index-files-thin-archive.ll

1 ↗

(On Diff #464481)

Why is this needed?

[aidengrossman]

Remove extraneous x86 requirement on regression test.

[aidengrossman]

lld/test/ELF/lto/thinlto-index-files-thin-archive.ll

1 ↗

(On Diff #464481)

It was just an artifact that I copied ever when replicating some of the setup from one of the other regression tests regarding thinlto index files that I mistakenly didn't remove. This has since been fixed in the latest revision of this patch. Thanks for pointing this out.

[MaskRay]

thinlto-emit-index-thin-archive.ll may be better since we already have thinlto-emit-index.ll and generally it's useful to make similar tests share a common prefix.

thinlto-thin-archive-collision.ll may be merged into the test as well.

lld/test/ELF/lto/thinlto-index-files-thin-archive.ll

1 ↗

(On Diff #464748)

The comment may be too long. I'll suggest

For a thin archive referencing object files in a different directory, emit index files (lib.a($member at $offset).thinlto.bc) in the directory containing the archive.

10 ↗

(On Diff #464748)

%t is about tests. test- in test-folder-2 isn't useful. Just use folder-1 folder-2, but actually dir1 dir2` is more common.

13 ↗

(On Diff #464748)

Add another test using --whole-archive lib.a. The patch makes the behavior similar to --whole-archive.

[MaskRay]

lld/test/ELF/lto/thinlto-index-files-thin-archive.ll

1 ↗

(On Diff #464748)

For a thin archive referencing object files in a different directory, emit index files (lib.a($member at $offset).thinlto.bc) in the directory containing the archive. The information about the referenced member's directory is lost.

[aidengrossman]

Combine tests from thinlto-thin-archive-collision.ll into the same test file
and addressed inline comments.

[aidengrossman]

Renamed test to better match other similar tests.

[MaskRay]

lld/test/ELF/lto/thinlto-emit-index-thin-archive.ll

4

Delete ;; Setup. It's not useful

6

mkdir dir1 dir2

41

Rename CHECK-UNUSED and just reuse its check since the pattern is exactly the same.

[aidengrossman]

Addressed inline comments.

lld/ELF/LTO.cpp

static void thinLTOCreateEmptyIndexFiles() {
   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::string path =
+        replaceThinLTOSuffix(getThinLTOOutputFile(f->obj->getName()));
     std::unique_ptr<raw_fd_ostream> os = openFile(path + ".thinlto.bc");
     if (!os)
       continue;
 
     ModuleSummaryIndex m(/*HaveGVs*/ false);
     m.setSkipModuleByDistributedBackend();
     writeIndexToFile(m, *os);
     if (config->thinLTOEmitImportsFiles)

lld/test/ELF/lto/thinlto-emit-index-thin-archive.ll

+; REQUIRES: x86
+
+; RUN: rm -rf %t.dir && mkdir %t.dir && cd %t.dir
+; RUN: mkdir dir1 dir2
+; RUN: llvm-as %s -o ./dir1/main.o
+; RUN: llvm-as %p/Inputs/thinlto.ll -o ./dir1/unused.o
+; RUN: llvm-as %p/Inputs/thin1.ll -o ./dir1/thin.o
+; RUN: llvm-as %p/Inputs/thin2.ll -o ./dir2/thin.o
+; RUN: llvm-ar crT ./dir2/lib.a dir1/unused.o dir1/thin.o dir2/thin.o
+
+;; For a thin archive referencing object files in a different directory,
+;; emit index files (lib.a($member at $offset).thinlto.bc) in the directory
+;; containing the archive, even in the lazy case. The information about the
+;; referenced member's directory is lost.
+; RUN: ld.lld --thinlto-emit-index-files ./dir2/lib.a ./dir1/main.o -o c --save-temps
+; RUN: ls ./dir2 | FileCheck %s --check-prefix CHECK-UNUSED
+
+; CHECK-UNUSED: lib.a(unused.o at {{[1-9][0-9]+}})
+
+;; Index files emitted from object files in a thin archive should have the
+;; offset in the archive specified to avoid collisions
+; RUN: FileCheck %s < c.resolution.txt --check-prefix CHECK-COLLISION
+
+; CHECK-COLLISION: dir1/main.o
+; CHECK-COLLISION: dir2/lib.a(thin.o at {{[1-9][0-9]+}})
+; CHECK-COLLISION-NEXT: -r=./dir2/lib.a(thin.o at {{[1-9][0-9]+}}),blah,pl
+; CHECK-COLLISION: dir2/lib.a(thin.o at {{[1-9][0-9]+}})
+; CHECK-COLLISION-NEXT: -r=./dir2/lib.a(thin.o at {{[1-9][0-9]+}}),foo,pl
+
+;; Clean up
+; RUN: rm -rf ./dir1/*.thinlto.bc
+; RUN: rm -rf ./dir2/*.thinlto.bc
+;; Empty index files for unused files in thin archives should still be emitted
+;; in the same format when using --whole-archive
+; RUN: ld.lld --thinlto-emit-index-files --whole-archive ./dir2/lib.a ./dir1/main.o -o d
+; RUN: ls ./dir2 | FileCheck %s --check-prefix CHECK-UNUSED
+
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+declare i32 @blah(i32 %meh)
+declare i32 @foo(i32 %goo)
+
+define void @_start() {
+entry:
+  call i32 @foo(i32 0)
+  call i32 @blah(i32 0)
+  ret void
+}

lld/test/ELF/lto/thinlto-thin-archive-collision.ll

-; REQUIRES: x86
-; RUN: rm -fr %t && mkdir %t && cd %t
-; RUN: mkdir thinlto-archives thinlto-archives/a thinlto-archives/b
-; RUN: opt -thinlto-bc -o thinlto-archives/main.o %s
-; RUN: opt -thinlto-bc -o thinlto-archives/a/thin.o %S/Inputs/thin1.ll
-; RUN: opt -thinlto-bc -o thinlto-archives/b/thin.o %S/Inputs/thin2.ll
-; RUN: llvm-ar qcT thinlto-archives/thin.a thinlto-archives/a/thin.o thinlto-archives/b/thin.o
-; RUN: ld.lld thinlto-archives/main.o thinlto-archives/thin.a -o thinlto-archives/main.exe --save-temps
-; RUN: FileCheck %s < thinlto-archives/main.exe.resolution.txt
-
-; CHECK: thinlto-archives/main.o
-; CHECK: thinlto-archives/thin.a(thin.o at {{[1-9][0-9]+}})
-; CHECK-NEXT: -r=thinlto-archives/thin.a(thin.o at {{[1-9][0-9]+}}),foo,pl
-; CHECK: thinlto-archives/thin.a(thin.o at {{[1-9][0-9]+}})
-; CHECK-NEXT: -r=thinlto-archives/thin.a(thin.o at {{[1-9][0-9]+}}),blah,pl
-
-target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
-target triple = "x86_64-scei-ps4"
-
-declare i32 @blah(i32 %meh)
-declare i32 @foo(i32 %goo)
-
-define i32 @_start() {
-  call i32 @foo(i32 0)
-  call i32 @blah(i32 0)
-  ret i32 0
-}