Add support for ThinLTO plugin option thinlto-object-suffix-replace

[rdhindsa]

Summary

Add support for ThinLTO plugin option thinlto-object-suffix-replace

thinlto-object-suffix-replace is an option to control the name of modules encoded in the individual index files for a distributed backend. This enables the use of minimized bitcode files for the thin link, assuming the name of the full bitcode file used in the backend differs just in some part of the file suffix.

In this patch, we are updating obj path names by consuming old suffix from the back and appending new suffix.

[ruiu]

Could you explain what this option is? I cannot get what is expected for that option from this patch.

lld/ELF/Driver.cpp

817 ↗

(On Diff #145801)

Please also add "but got" part.

lld/ELF/InputFiles.cpp

1028 ↗

(On Diff #145801)

Remove llvm::

lld/ELF/LTO.cpp

269 ↗

(On Diff #145801)

Remove llvm::

(You probably should other code in the same file and write code in the same way as others.)

lld/test/ELF/lto/thinlto_object_suffix_replace.ll

1 ↗

(On Diff #145801)

We use - instead of - in test file name.

18 ↗

(On Diff #145801)

Please split a long line into shorter lines and concatenate them with \.

[rdhindsa]

Updated the summary for option description.

[ruiu]

lld/ELF/InputFiles.cpp

1028 ↗

(On Diff #145986)

I don't think we should add new tricky code to this common place. Instead, we should modify LTO.cpp or somewhere else. Let me try to do that locally.

[ruiu]

lld/ELF/InputFiles.cpp

1030–1033 ↗

(On Diff #145986)

All tests including the new one you are adding in this patch seem to pass without these four lines of code. Is this needed?

[rdhindsa]

lld/ELF/InputFiles.cpp

1030–1033 ↗

(On Diff #145986)

Yeah it is needed as this handles all the bitcode files except lazyobjfiles. I verified that the test thinlto-object-suffix-replace fails without these changes.

[ruiu]

lld/ELF/InputFiles.cpp

1030–1033 ↗

(On Diff #145986)

Yeah, I verified that your test would fail without this piece of code. But it doesn't feel right to add a new piece of this common path just for a minor ThinLTO feature. That seems too intrusive to me. It might be unavoidable, but we probably shouldn't do this. Let me actually modify this code.

[ruiu]

I tried a few different ideas but I couldn't come up with a better idea than this patch, so this seems like a best way of doing this so far. Could you please update the patch with my comments?

lld/ELF/InputFiles.cpp

1028 ↗

(On Diff #145986)

StringRef -> std::string and then remove UpdatedPath. You can mutate Path if you change its type to std::string.

1031–1032 ↗

(On Diff #145986)

If Path doesn't end with first, then it doesn't remove anything from its end, and then second is added to Path. I doubt it is a correct behavior.

[ruiu]

lld/ELF/Driver.cpp

822–823 ↗

(On Diff #146411)

nit: it is perhaps better to split the long literal string into two like this

error("thinlto-object-suffix-replace expects 'old;new' format, "
      "but got " + S.substr(30));

so that the code looks better.

lld/ELF/InputFiles.cpp

1037 ↗

(On Diff #146411)

I don't think this is correct. Don't you need to use ends_with?

lld/ELF/LTO.cpp

270–278 ↗

(On Diff #146411)

The amount of code duplication is growing, and you probably should do something to against it.

[rdhindsa]

lld/ELF/Driver.cpp

822–823 ↗

(On Diff #146411)

clang-format updates by splitting (" but got " + S.substr(30));) to two lines .

[ruiu]

There are a few things that can be improved in this patch, but I think that changing everything before submitting is not necessary. Please go ahead and submit. I'll make a follow-up change after that.

lld/ELF/Driver.cpp

822–823 ↗

(On Diff #146411)

This is much uglier than before, but I'd think we should fight against clang-format. I'd just concatenate the two strings into one literal string just like before.

lld/trunk/ELF/Config.h

struct Configuration {
   llvm::StringRef MapFile;
   llvm::StringRef OutputFile;
   llvm::StringRef OptRemarksFilename;
   llvm::StringRef ProgName;
   llvm::StringRef SoName;
   llvm::StringRef Sysroot;
   llvm::StringRef ThinLTOCacheDir;
   llvm::StringRef ThinLTOIndexOnlyArg;
+  std::pair<llvm::StringRef, llvm::StringRef> ThinLTOObjectSuffixReplace;
   std::pair<llvm::StringRef, llvm::StringRef> ThinLTOPrefixReplace;
   std::string Rpath;
   std::vector<VersionDefinition> VersionDefinitions;
   std::vector<llvm::StringRef> AuxiliaryList;
   std::vector<llvm::StringRef> FilterList;
   std::vector<llvm::StringRef> SearchPaths;
   std::vector<llvm::StringRef> SymbolOrderingFile;
   std::vector<llvm::StringRef> Undefined;

lld/trunk/ELF/Driver.cpp

for (auto *Arg : Args.filtered(OPT_plugin_opt)) {
     } else if (S == "thinlto-emit-imports-files") {
       Config->ThinLTOEmitImportsFiles = true;
     } else if (S.startswith("thinlto-prefix-replace=")) {
       std::tie(Config->ThinLTOPrefixReplace.first,
                Config->ThinLTOPrefixReplace.second) = S.substr(23).split(';');
       if (Config->ThinLTOPrefixReplace.second.empty())
         error("thinlto-prefix-replace expects 'old;new' format, but got " +
               S.substr(23));
+    } else if (S.startswith("thinlto-object-suffix-replace=")) {
+      std::tie(Config->ThinLTOObjectSuffixReplace.first,
+               Config->ThinLTOObjectSuffixReplace.second) =
+          S.substr(30).split(';');
+      if (Config->ThinLTOObjectSuffixReplace.second.empty())
+        error(
+            "thinlto-object-suffix-replace expects 'old;new' format, but got " +
+            S.substr(30));
     } else if (!S.startswith("/") && !S.startswith("-fresolution=") &&
                !S.startswith("-pass-through=") && !S.startswith("thinlto")) {
       parseClangOption(S, Arg->getSpelling());
     }
   }
 
   // Parse -mllvm options.
   for (auto *Arg : Args.filtered(OPT_mllvm))

lld/trunk/ELF/InputFiles.h

 InputFile *createObjectFile(MemoryBufferRef MB, StringRef ArchiveName = "",
                             uint64_t OffsetInArchive = 0);
 InputFile *createSharedFile(MemoryBufferRef MB, StringRef DefaultSoName);
 
 inline bool isBitcode(MemoryBufferRef MB) {
   return identify_magic(MB.getBuffer()) == llvm::file_magic::bitcode;
 }
 
+inline std::string updateSuffixInPath(llvm::StringRef Path) {
+  if (Path.endswith(Config->ThinLTOObjectSuffixReplace.first)) {
+    size_t pos = Path.rfind(Config->ThinLTOObjectSuffixReplace.first);
+    std::string SuffixedPath =
+        (Path.str().substr(0, pos) +
+         Config->ThinLTOObjectSuffixReplace.second.str());
+    return SuffixedPath;
+  } else {
+    error("cannot find suffix " +
+          Config->ThinLTOObjectSuffixReplace.first.str());
+    return "";
+  }
+}
+
 extern std::vector<BinaryFile *> BinaryFiles;
 extern std::vector<BitcodeFile *> BitcodeFiles;
 extern std::vector<LazyObjFile *> LazyObjFiles;
 extern std::vector<InputFile *> ObjectFiles;
 extern std::vector<InputFile *> SharedFiles;
 
 } // namespace elf
 } // namespace lld
 
 #endif

lld/trunk/ELF/InputFiles.cpp

BitcodeFile::BitcodeFile(MemoryBufferRef MB, StringRef ArchiveName,
   // Here we pass a new MemoryBufferRef which is identified by ArchiveName
   // (the fully resolved path of the archive) + member name + offset of the
   // member in the archive.
   // ThinLTO uses the MemoryBufferRef identifier to access its internal
   // data structures and if two archives define two members with the same name,
   // this causes a collision which result in only one of the objects being
   // taken into consideration at LTO time (which very likely causes undefined
   // symbols later in the link stage).
+  std::string Path = MB.getBufferIdentifier().str();
+  if (Config->ThinLTOIndexOnly)
+    Path = updateSuffixInPath(MB.getBufferIdentifier());
+
   MemoryBufferRef MBRef(
       MB.getBuffer(),
-      Saver.save(ArchiveName + MB.getBufferIdentifier() +
+      Saver.save(ArchiveName + Path +
                  (ArchiveName.empty() ? "" : utostr(OffsetInArchive))));
   Obj = CHECK(lto::InputFile::create(MBRef), this);
 
   Triple T(Obj->getTargetTriple());
   EKind = getBitcodeELFKind(T);
   EMachine = getBitcodeMachineKind(MB.getBufferIdentifier(), T);
 }
 

lld/trunk/ELF/LTO.cpp

std::vector<InputFile *> BitcodeCompiler::compile() {
   // If LazyObjFile has not been added to link, emit empty index files.
   // This is needed because this is what GNU gold plugin does and we have a
   // distributed build system that depends on that behavior.
   if (Config->ThinLTOIndexOnly) {
     for (LazyObjFile *F : LazyObjFiles) {
       if (F->AddedToLink || !isBitcode(F->MB))
         continue;
 
-      std::string Path = getThinLTOOutputFile(F->getName());
+      std::string Path = updateSuffixInPath(getThinLTOOutputFile(F->getName()));
+
       std::unique_ptr<raw_fd_ostream> OS = openFile(Path + ".thinlto.bc");
       if (!OS)
         continue;
 
       ModuleSummaryIndex M(false);
       M.setSkipModuleByDistributedBackend();
       WriteIndexToFile(M, *OS);
 

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

+; REQUIRES: x86
+
+; Test to make sure the thinlto-object-suffix-replace option is handled
+; correctly.
+
+; Generate bitcode file with summary, as well as a minimized bitcode without
+; the debug metadata for the thin link.
+; RUN: opt -thinlto-bc %s -thin-link-bitcode-file=%t1.thinlink.bc -o %t1.o
+
+; First perform the thin link on the normal bitcode file, and save the
+; resulting index.
+; RUN: ld.lld -m elf_x86_64 --plugin-opt=thinlto-index-only -shared %t1.o -o %t3
+; RUN: cp %t1.o.thinlto.bc %t1.o.thinlto.bc.orig
+
+; Next perform the thin link on the minimized bitcode file, and compare dump
+; of the resulting index to the above dump to ensure they are identical.
+; RUN: rm -f %t1.o.thinlto.bc
+; Make sure it isn't inadvertently using the regular bitcode file.
+; RUN: rm -f %t1.o
+; RUN: ld.lld -m elf_x86_64 --plugin-opt=thinlto-index-only \
+; RUN: --plugin-opt=thinlto-object-suffix-replace=".thinlink.bc;.o" \
+; RUN: -shared %t1.thinlink.bc -o %t3
+; RUN: diff %t1.o.thinlto.bc.orig %t1.o.thinlto.bc
+
+; 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 -m elf_x86_64 --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: 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 -m elf_x86_64 --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: cannot find suffix .abc
+
+
+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}