[PATCH] D51055: [ELF] -thinlto-object-suffix-replace=: don't error if the path does not end with old suffix

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson accepted this revision.
tejohnson added inline comments.
This revision is now accepted and ready to land.


================
Comment at: ELF/InputFiles.cpp:1294
+  if (Path.consume_back(Suffix))
+    return (Path + Repl).str();
+  return Path;
----------------
tejohnson wrote:
> MaskRay wrote:
> > tejohnson wrote:
> > > This is still different than what gold-plugin currently does, which is to always append the new suffix. It would be good to make these do the same thing. With this version as opposed to gold plugin's, there is no way to simply append a new suffix, if we ever wanted to do that (not sure).
> > Is it possible to change `tools/gold/gold-plugin.cpp` for the incompatibility?
> > Currently for `-thinlto-object-suffix-replace=.indexing.o\;.o`, if a `foo.o` is given, `foo.o.o.thinlto.bc` will be generated.
> > I think `foo.o.thinlto.bc` (what this patch makes lld do) looks better
> It depends on how we expect the option to be used. lld was essentially codifying (with the error being removed here), how we use this option in practice, which is to allow distributed build thin link processes to accept minimized bitcode versions of files being built (minimized to remove the IR that we don't care about during the thin link). In reality, at least how we have used this in the Bazel build system until now, all bitcode files have a minimized version, so we never encountered this situation before.
Based on some off-patch investigation and discussion:
- The renamed result doesn't matter (wasn't used) in this case as the archive objects were regular LTO bitcode and therefore don't go through the ThinLTO distributed build backends (which is what we are needing to do the rename for). The suffix replacement is used to support using minimized ThinLTO bitcode during the distributed thin link - which doesn't apply for these regular LTO objects in the archive either.
- It will be potentially useful to have this change, as it will enable supporting mix and match of minimized ThinLTO bitcode files with normal ThinLTO bitcode files in a single link (where we want to apply the suffix replacement to the minimized files, and just ignore it for the normal ThinLTO files).


================
Comment at: test/ELF/lto/thinlto-object-suffix-replace.ll:32
 
-; Ensure lld generates error if old suffix doesn't exist in file name
-; RUN: rm -f %t1.o
-; RUN: not ld.lld --plugin-opt=thinlto-index-only \
-; RUN: --plugin-opt=thinlto-object-suffix-replace=".abc;.o" -shared %t1.thinlink.bc \
-; RUN: -o %t3 2>&1 | FileCheck %s --check-prefix=ERR2
-; ERR2: error: -thinlto-object-suffix-replace=.abc;.o was given, but {{.*}} does not end with the suffix
+; If filename does not end with old suffix, append ".thinlto.bc" instead of new suffix plus ".thinlto.bc"
+; RUN: rm -f %t1.thinlink.bc.thinlto.bc
----------------
Nit: fix line length
But suggest rewording to something like:
; If filename does not end with old suffix, no suffix change should occur,
; so "thinlto.bc" will simply be appended to the input file name.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D51055