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

[MaskRay]

Summary

For -thinlto-object-suffix-replace=old\;new, in
tools/gold/gold-plugin.cpp, the thinlto object filename is Path minus
optional "old" suffix.

static std::string getThinLTOObjectFileName(StringRef Path, StringRef OldSuffix,
                                            StringRef NewSuffix) {
  if (OldSuffix.empty() && NewSuffix.empty())
    return Path;
  StringRef NewPath = Path;
  NewPath.consume_back(OldSuffix);
  std::string NewNewPath = NewPath;
  NewNewPath += NewSuffix;
  return NewNewPath;
}

Currently lld will error that the path does not end with old suffix.

This patch makes lld accept such paths but only add new suffix if Path
ends with old suffix. This fixes a link error where bitcode members in
some archives do not end with old suffix.

[tejohnson]

ELF/InputFiles.cpp

1294 ↗

(On Diff #161789)

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).

[MaskRay]

ELF/InputFiles.cpp

1294 ↗

(On Diff #161789)

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

[tejohnson]

ELF/InputFiles.cpp

1294 ↗

(On Diff #161789)

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.

[tejohnson]

ELF/InputFiles.cpp

1294 ↗

(On Diff #161789)

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).

test/ELF/lto/thinlto-object-suffix-replace.ll

32 ↗

(On Diff #161789)

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.

[tejohnson]

LGTM with comment nit

[pcc]

ELF/InputFiles.cpp

1294 ↗

(On Diff #161789)

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?

[tejohnson]

ELF/InputFiles.cpp

1294 ↗

(On Diff #161789)

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.

[MaskRay]

ELF/InputFiles.cpp

1294 ↗

(On Diff #161789)

llvm-ar r t2.a t2.o
ld.lld --plugin-opt=thinlto-index-only -shared --plugin-opt=thinlto-object-suffix-replace=".abc;.o" -shared t1.o t2.a -o /dev/null

t2.o in t2.a emits t2.at2.o78.thinlto.bc, which is mangled and unlikely to conflict with normal files.

[MaskRay]

Update comment

[pcc]

ELF/InputFiles.cpp

1294 ↗

(On Diff #161789)

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.

[tejohnson]

ELF/InputFiles.cpp

1294 ↗

(On Diff #161789)

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

Yeah for archive members if we could simply write to /dev/null that would be even better. But that would be done elsewhere in the code anyway. Also, the change here will enable handling of the case where we passed some files to the thin link that aren't minimized and therefore don't need the suffix replacement (not sure why that would be done in practice, but might as well handle it).

lld/trunk/ELF/InputFiles.cpp

for (const typename ELFT::Shdr &Sec : Sections) {
     return;
   }
 }
 
 std::string elf::replaceThinLTOSuffix(StringRef Path) {
   StringRef Suffix = Config->ThinLTOObjectSuffixReplace.first;
   StringRef Repl = Config->ThinLTOObjectSuffixReplace.second;
 
-  if (!Path.endswith(Suffix)) {
-    error("-thinlto-object-suffix-replace=" + Suffix + ";" + Repl +
-          " was given, but " + Path + " does not end with the suffix");
-    return "";
-  }
-  return (Path.drop_back(Suffix.size()) + Repl).str();
+  if (Path.consume_back(Suffix))
+    return (Path + Repl).str();
+  return Path;
 }
 
 template void ArchiveFile::parse<ELF32LE>();
 template void ArchiveFile::parse<ELF32BE>();
 template void ArchiveFile::parse<ELF64LE>();
 template void ArchiveFile::parse<ELF64BE>();
 
 template void BitcodeFile::parse<ELF32LE>(DenseSet<CachedHashStringRef> &);

lld/trunk/test/ELF/lto/thinlto-object-suffix-replace.ll

 
 ; Ensure lld generates error if object suffix replace option does not have 'old;new' format
 ; RUN: rm -f %t1.o.thinlto.bc
 ; RUN: not ld.lld --plugin-opt=thinlto-index-only \
 ; RUN: --plugin-opt=thinlto-object-suffix-replace="abc:def" -shared %t1.thinlink.bc \
 ; RUN: -o %t3 2>&1 | FileCheck %s --check-prefix=ERR1
 ; ERR1: --plugin-opt=thinlto-object-suffix-replace= expects 'old;new' format, but got abc:def
 
-; 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, no suffix change should occur,
+; so ".thinlto.bc" will simply be appended to the input file name.
+; RUN: rm -f %t1.thinlink.bc.thinlto.bc
+; RUN: ld.lld --plugin-opt=thinlto-index-only \
+; RUN: --plugin-opt=thinlto-object-suffix-replace=".abc;.o" -shared %t1.thinlink.bc -o /dev/null
+; RUN: ls %t1.thinlink.bc.thinlto.bc
 
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
 define void @f() {
 entry:
   ret void
 }
 
 !llvm.dbg.cu = !{}
 
 !1 = !{i32 2, !"Debug Info Version", i32 3}
 !llvm.module.flags = !{!1}