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

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: ELF/InputFiles.cpp:1294
+  if (Path.consume_back(Suffix))
+    return (Path + Repl).str();
+  return Path;
----------------
tejohnson wrote:
> pcc wrote:
> > tejohnson wrote:
> > > 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).
> > I'm not sure that this always makes sense. If the path suffix does not match we will end up overwriting the input file with the ThinLTO index. Maybe the right solution is to tell ThinLTO to write indexes for archive members to `/dev/null` or something?
> > If the path suffix does not match we will end up overwriting the input file with the ThinLTO index.
> 
> That's not exactly how this is used - the ThinLTO index file name will also have "thinlto.bc" appended to whatever name is generated by this replacement function. This renaming is also used to ensure that the module paths inside the ThinLTO index for the distributed backends are correct (to facilitate importing from the IR file). Otherwise we would try to import from the minimized bitcode file.
Yes, I forgot that we were appending `.thinlto.bc` separately. Never mind me then.

It still seems like we should try to handle archive members differently, but I'm not sure how.


Repository:
  rLLD LLVM Linker

https://reviews.llvm.org/D51055