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

Mon Oct 3 08:32:53 PDT 2022

tejohnson added a comment.

In D135014#3829154 <https://reviews.llvm.org/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.

================
Comment at: lld/test/ELF/lto/thinlto-index-files-thin-archive.ll:1
+; REQUIRES: x86
+
----------------
Why is this needed?

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D135014/new/

https://reviews.llvm.org/D135014