[ThinLTO] Emit files for distributed builds for all modules

[tejohnson]

Summary

With the new LTO API in r278338, we stopped emitting the individual
index files and imports files for some modules in the distributed backend
case (thinlto-index-only plugin option).

Specifically, this is when the linker decides not to include a module in the
link, because it was in an archive library and did not have a strong
reference to it. Not creating the expected output files makes the
distributed build system implementation more difficult, in terms of
checking for the expected outputs of the thin link, and scheduling the
backend jobs. To address this, the gold-plugin will write dummy empty
.thinlto.bc and .imports files for modules not included in the link
(which LTO never sees).

Augmented a gold v1.12+ test, since that version of gold has the handling
for notifying on modules not being included in the link.

[mehdi_amini]

tools/llvm-lto2/llvm-lto2.cpp

147 ↗

(On Diff #68572)

Looks like we stepped on each other feet (D23681)

[mehdi_amini]

They were no longer being emitted for files that didn't contain a module index (e.g. when they had inline assembly, since we can't currently safely import), because the new API only added them to the ThinLTO backend based on the presence of a module index. They were instead added as regular LTO files, and were all expected to be merged into a single LTO module. This is problematic because the distributed build system will not know how to create backend actions for those files.

I do not necessarily see this as a "bug" right now: we need a coherent story for the intended behavior here. If a file is not meant to go to to the ThinLTO backend, then it shouldn't go there. It seems to me that the real issue you're tackling is that we don't emit any summary in the bitcode for files that have inline asm, and thus they are not detected as "ThinLTO" files and not sent to the right backend.
I believe the "right" fix for this is to change the way we detect if a file was produced with -flto=thin to detect this file (the "easy" way may be to emit an empty summary?).

It does not totally solve the question about how to do with inputs that were not build with -flto=thin but -flto=full. It does not seem right to me to send these to the ThinLTO backend. You can start by consider this as "unsupported" by your current distributed build system integration, and then built the mechanism that would allow the build system to deal with this if needed.

Note also that the class LTO is doing too much right now: if the RegularLTOState was moved to a separate entity in way that it could be injected like the ThinLTOBackend, all of this could be handled/overridden by the client (I mentioned this lack of layering in early reviews). The "ForceThinLTO" flag in the config seems to me to be a workaround for this.

[tejohnson]

In D23680#519874, @mehdi_amini wrote:

They were no longer being emitted for files that didn't contain a module index (e.g. when they had inline assembly, since we can't currently safely import), because the new API only added them to the ThinLTO backend based on the presence of a module index. They were instead added as regular LTO files, and were all expected to be merged into a single LTO module. This is problematic because the distributed build system will not know how to create backend actions for those files.

I do not necessarily see this as a "bug" right now: we need a coherent story for the intended behavior here. If a file is not meant to go to to the ThinLTO backend, then it shouldn't go there. It seems to me that the real issue you're tackling is that we don't emit any summary in the bitcode for files that have inline asm, and thus they are not detected as "ThinLTO" files and not sent to the right backend.
I believe the "right" fix for this is to change the way we detect if a file was produced with -flto=thin to detect this file (the "easy" way may be to emit an empty summary?).

Possible, but:

It does not totally solve the question about how to do with inputs that were not build with -flto=thin but -flto=full. It does not seem right to me to send these to the ThinLTO backend. You can start by consider this as "unsupported" by your current distributed build system integration, and then built the mechanism that would allow the build system to deal with this if needed.

For now we can handle this unsupported case by treating all the IR modules (regardless of how they were produced) through the ThinLTO compilation model. I am happy to put this under a different option rather than keying off of thinlto-index-only if you'd prefer, would that be better?

Note also that the class LTO is doing too much right now: if the RegularLTOState was moved to a separate entity in way that it could be injected like the ThinLTOBackend, all of this could be handled/overridden by the client (I mentioned this lack of layering in early reviews). The "ForceThinLTO" flag in the config seems to me to be a workaround for this.

That may be a better longer term change. In the meantime this is a simple way to reverse the regression (before the new API, the gold-plugin keyed off of the "thinlto" plugin option, not the presence of a module index section). I could put a FIXME there if that would help. Does that sound ok?

[tejohnson]

Address feedback from earlier review:
1) Emit empty summaries for files without any GVs that can be imported
when in ThinLTO mode. This ensures that LTO treats them as ThinLTO
inputs and schedules ThinLTO backends for them. It required a few
fixes to handle the empty summaries.
2) Per offline discussion with Mehdi, for the case where the linker
is not going to include some of the modules in the final link, in
the gold-plugin client itself write the dummy empty files expected by the
distributed build system for all modules.

[mehdi_amini]

lib/Bitcode/Reader/BitcodeReader.cpp

6111 ↗

(On Diff #71952)

Can you commit this as a separate patch? This is mostly unrelated to the distributed build.

[tejohnson]

lib/Bitcode/Reader/BitcodeReader.cpp

6111 ↗

(On Diff #71952)

It is related to the change to always emit a summary when compiling for ThinLTO. Maybe I should split that portion (along with the corresponding fixes like this one) out into a separate patch. Is that what you meant? It fixes part of the distributed build issue.

[pcc]

tools/gold/gold-plugin.cpp

755

I am not sure if this is needed, but even if it is, can't this be done inside lib/LTO?

[tejohnson]

tools/gold/gold-plugin.cpp

755

This is needed because the distributed build system (in this case Bazel) wants to check that its expected outputs were generated. It doesn't know what the linker decided to include in the final link or not. This was a regression from the earlier behavior before we moved to the new LTO API.

My earlier version of the patch had this fix in lib/LTO, but Mehdi expressed a preference (in an offline discussion) to move this into the client.

[mehdi_amini]

lib/Bitcode/Reader/BitcodeReader.cpp

6111 ↗

(On Diff #71952)

It also change the behavior outside of distributed build right?
It will make files that we didn't include in ThinLTO before, part of the ThinLTO partitions now.

[mehdi_amini]

tools/gold/gold-plugin.cpp

755

Right: it seems "client specific" as it is a constraint that comes from Bazel.

[tejohnson]

lib/Bitcode/Reader/BitcodeReader.cpp

6111 ↗

(On Diff #71952)

Yes. Ok, will split that whole part out into a separate patch. This patch will end up just being the changes to gold-plugin.cc, test/tools/gold/X86/v1.12/thinlto_emit_linked_objects.ll, and the refactoring of getThinLTOOutputFile in LTO.

tools/gold/gold-plugin.cpp

755

Although note that Bazel is independent of gold. It just happens that we build with using the gold linker and the Bazel distributed build system.

[pcc]

tools/gold/gold-plugin.cpp

755

When you say "It doesn't know what the linker decided to include" are you referring to which archive members the linker chose?

So the regression is that the distributed build is missing non-ThinLTO objects from archives, and the fix is to create empty .thinlto.bc files to cause the build system to include the non-ThinLTO objects in the final link?

Or am I misunderstanding?

[tejohnson]

tools/gold/gold-plugin.cpp

755

Correct, it is to deal with the fact that with archives (--start-lib/--end-lib in our case), gold will not include some of the files in the final link. Those are not passed to the backend and no ThinLTO outputs are created for them.

We do emit the list of files to include in the final link (into the thinlto_linked_objects_file, which is an optional file provided to the thinlto-index-only= option), and that ends up informing the final link done by Bazel (essentially passed in via the @ linker option).

However, the issue here is with the way Bazel does sanity checking of expected outputs - all it knows when it kicks off the ThinLink action is that it is passing a set of object (IR) files to the Thin Link action, and each action needs to be told what output files it should bring back. The lower level job handling checks that these expected outputs are created and fetches them back. With the current LTO code, that contract is being broken, because not all expected outputs are being created.

[mehdi_amini]

tools/gold/gold-plugin.cpp

755

Although note that Bazel is independent of gold. It just happens that we build with using the gold linker and the Bazel distributed build system.

Right, I'm not convinced it belongs here entirely :)

My understanding is that it is very targeted: this operates under the assumptions that:

1. The build system has a list of all the input to the linker (including archive members)
2. The build system expects the linker to produces 1-1 output for each input.

So it is about both archive members not chosen by the linker, and input files that are not ThinLTO.
I don't think the latter case is handled though, it seems to be relying on every input being built with -flto=thin (or I missed a piece somewhere, how would an object file be handled?)

[tejohnson]

tools/gold/gold-plugin.cpp

755

So it is about both archive members not chosen by the linker, and input files that are not ThinLTO.
I don't think the latter case is handled though, it seems to be relying on every input being built with -flto=thin
(or I missed a piece somewhere, how would an object file be handled?)

We know which compile actions are ThinLTO actions and set up the expected inputs and outputs that way. In cases where there is e.g. a native object, there isn't a ThinLTO output scheduled to be returned from the ThinLink. So the part of this patch that always generates a summary when compiling -flto=thin fixes the issue where we were not treating these as ThinLTO inputs in the ThinLink.

For the archive members not included in the final link. the issue is that when we set up the ThinLink action, we know which inputs are ThinLTO inputs, and need to specify which outputs should be expected and brought back. That has no knowledge of the linker behavior and which inputs it will ultimately select.

[mehdi_amini]

tools/gold/gold-plugin.cpp

755

So if I have a project that depends on a vendor supplied library, it is up to the person configuring the project to instruct Bazel about the ThinLTO member inside this archive?

[pcc]

tools/gold/gold-plugin.cpp

755

I see. So my next question is, if Bazel knows the names of the expected output files, can it (or some other component of the system such as a wrapper script) not be taught to touch those files by itself before running the initial linker pass?

I can appreciate if the answer is "not easily". I'm just trying to get a sense of the right tradeoff here.

[tejohnson]

tools/gold/gold-plugin.cpp

755

So if I have a project that depends on a vendor supplied library, it is up to the person configuring the project to instruct Bazel about the ThinLTO member inside this archive?

There are a few limitations that would currently prevent this from working well with the build system right now. That use case would require some more work beyond this, at least to get the backend job distributed properly. We currently expect to compile all ThinLTO code from source.

[tejohnson]

tools/gold/gold-plugin.cpp

755

I can appreciate if the answer is "not easily". I'm just trying to get a sense of the right tradeoff here.

I don't think this is easy given the way things are architected. But it also prevents error checking that we did in fact run the thin link and get outputs.

[tejohnson]

Moved the part of this patch that handles always emitting a summary
section for files compiled -flto=thin into a separate patch (D24779).

This part deals with ensuring the distributed build system gets back
expected outputs for the files it compiled in ThinLTO mode.

[tejohnson]

pcc, are you ok with keeping this here rather than in LTO? The advantage is that LTO doesn't need to know about files the linker decided to exclude, just those to be built with LTO.

[mehdi_amini]

I'm fine with the patch as it is now.
Let's just wait for Peter to confirm.

[pcc]

tools/gold/gold-plugin.cpp

755

But it also prevents error checking that we did in fact run the thin link and get outputs.

You could do that by reading the LinkedObjectsFile, no?

But okay, I suppose this is fine since it doesn't preclude a better implementation on the bazel side later.

[tejohnson]

tools/gold/gold-plugin.cpp

755

You could do that by reading the LinkedObjectsFile, no?

That's difficult since where the outputs are checked is low level generic job management functionality, and not cognizant of the fact that this is ThinLTO (I looked at this initially).

But okay, I suppose this is fine since it doesn't preclude a better implementation on the bazel side later.

Ok thanks.

[tejohnson]

r282100

include/llvm/LTO/LTO.h

 /// as external and non-exported values as internal. The ThinLTO backends
 /// must apply the changes to the Module via thinLTOInternalizeModule.
 void thinLTOInternalizeAndPromoteInIndex(
     ModuleSummaryIndex &Index,
     function_ref<bool(StringRef, GlobalValue::GUID)> isExported);
 
 namespace lto {
 
+/// Given the original \p Path to an output file, replace any path
+/// prefix matching \p OldPrefix with \p NewPrefix. Also, create the
+/// resulting directory if it does not yet exist.
+std::string getThinLTOOutputFile(const std::string &Path,
+                                 const std::string &OldPrefix,
+                                 const std::string &NewPrefix);
+
 class LTO;
 struct SymbolResolution;
 class ThinBackendProc;
 
 /// An input file. This is a wrapper for IRObjectFile that exposes only the
 /// information that an LTO client should need in order to do symbol resolution.
 class InputFile {
   // FIXME: Remove LTO class friendship once we have bitcode symbol tables.

lib/LTO/LTO.cpp

return [=](Config &Conf, ModuleSummaryIndex &CombinedIndex,
              const StringMap<GVSummaryMapTy> &ModuleToDefinedGVSummaries,
              AddOutputFn AddOutput) {
     return llvm::make_unique<InProcessThinBackend>(
         Conf, CombinedIndex, ParallelismLevel, ModuleToDefinedGVSummaries,
         AddOutput);
   };
 }
 
+// Given the original \p Path to an output file, replace any path
+// prefix matching \p OldPrefix with \p NewPrefix. Also, create the
+// resulting directory if it does not yet exist.
+std::string lto::getThinLTOOutputFile(const std::string &Path,
+                                      const std::string &OldPrefix,
+                                      const std::string &NewPrefix) {
+  if (OldPrefix.empty() && NewPrefix.empty())
+    return Path;
+  SmallString<128> NewPath(Path);
+  llvm::sys::path::replace_path_prefix(NewPath, OldPrefix, NewPrefix);
+  StringRef ParentPath = llvm::sys::path::parent_path(NewPath.str());
+  if (!ParentPath.empty()) {
+    // Make sure the new directory exists, creating it if necessary.
+    if (std::error_code EC = llvm::sys::fs::create_directories(ParentPath))
+      llvm::errs() << "warning: could not create directory '" << ParentPath
+                   << "': " << EC.message() << '\n';
+  }
+  return NewPath.str();
+}
+
 class WriteIndexesThinBackend : public ThinBackendProc {
   std::string OldPrefix, NewPrefix;
   bool ShouldEmitImportsFiles;
 
   std::string LinkedObjectsFileName;
   std::unique_ptr<llvm::raw_fd_ostream> LinkedObjectsFile;
 
 public:
   WriteIndexesThinBackend(
       Config &Conf, ModuleSummaryIndex &CombinedIndex,
       const StringMap<GVSummaryMapTy> &ModuleToDefinedGVSummaries,
       std::string OldPrefix, std::string NewPrefix, bool ShouldEmitImportsFiles,
       std::string LinkedObjectsFileName)
       : ThinBackendProc(Conf, CombinedIndex, ModuleToDefinedGVSummaries),
         OldPrefix(OldPrefix), NewPrefix(NewPrefix),
         ShouldEmitImportsFiles(ShouldEmitImportsFiles),
         LinkedObjectsFileName(LinkedObjectsFileName) {}
 
-  /// Given the original \p Path to an output file, replace any path
-  /// prefix matching \p OldPrefix with \p NewPrefix. Also, create the
-  /// resulting directory if it does not yet exist.
-  std::string getThinLTOOutputFile(const std::string &Path,
-                                   const std::string &OldPrefix,
-                                   const std::string &NewPrefix) {
-    if (OldPrefix.empty() && NewPrefix.empty())
-      return Path;
-    SmallString<128> NewPath(Path);
-    llvm::sys::path::replace_path_prefix(NewPath, OldPrefix, NewPrefix);
-    StringRef ParentPath = llvm::sys::path::parent_path(NewPath.str());
-    if (!ParentPath.empty()) {
-      // Make sure the new directory exists, creating it if necessary.
-      if (std::error_code EC = llvm::sys::fs::create_directories(ParentPath))
-        llvm::errs() << "warning: could not create directory '" << ParentPath
-                     << "': " << EC.message() << '\n';
-    }
-    return NewPath.str();
-  }
-
   Error start(
       unsigned Task, MemoryBufferRef MBRef,
       const FunctionImporter::ImportMapTy &ImportList,
       const FunctionImporter::ExportSetTy &ExportList,
       const std::map<GlobalValue::GUID, GlobalValue::LinkageTypes> &ResolvedODR,
       MapVector<StringRef, MemoryBufferRef> &ModuleMap) override {
     StringRef ModulePath = MBRef.getBufferIdentifier();
     std::string NewModulePath =

test/tools/gold/X86/v1.12/thinlto_emit_linked_objects.ll

 ; First generate bitcode with a module summary index for each file
 ; RUN: opt -module-summary %s -o %t.o
 ; RUN: opt -module-summary %p/Inputs/thinlto_emit_linked_objects.ll -o %t2.o
 
 ; Next do the ThinLink step, specifying thinlto-index-only so that the gold
 ; plugin exits after generating individual indexes. The objects the linker
 ; decided to include in the link should be emitted into the file specified
 ; after 'thinlto-index-only='. In this version of the test, only %t.o will
 ; be included in the link, and not %t2.o since it is within
 ; a library (--start-lib/--end-lib pair) and not strongly referenced.
 ; Note that the support for detecting this is in gold v1.12.
+; RUN: rm -f %t.o.thinlto.bc
+; RUN: rm -f %t2.o.thinlto.bc
+; RUN: rm -f %t.o.imports
+; RUN: rm -f %t2.o.imports
 ; RUN: %gold -plugin %llvmshlibdir/LLVMgold.so \
 ; RUN:    --plugin-opt=thinlto \
 ; RUN:    --plugin-opt=thinlto-index-only=%t3 \
+; RUN:    --plugin-opt=thinlto-emit-imports-files \
 ; RUN:    -m elf_x86_64 \
 ; RUN:    -o %t4 \
 ; RUN:    %t.o \
 ; RUN:    --start-lib %t2.o --end-lib
 
+; Ensure that the expected output files are created, even for the file
+; the linker decided not to include in the link.
+; RUN: ls %t.o.thinlto.bc
+; RUN: ls %t2.o.thinlto.bc
+; RUN: ls %t.o.imports
+; RUN: ls %t2.o.imports
+
 ; RUN: cat %t3 | FileCheck %s
 ; CHECK: thinlto_emit_linked_objects.ll.tmp.o
 ; CHECK-NOT: thinlto_emit_linked_objects.ll.tmp2.o
 
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-unknown-linux-gnu"
 
 define i32 @main() {
 entry:
   ret i32 0
 }

tools/gold/gold-plugin.cpp

check(Conf.addSaveTemps(output_name + ".",
                             /* UseInputModulePath */ true));
     break;
   }
 
   return llvm::make_unique<LTO>(std::move(Conf), Backend,
                                 ParallelCodeGenParallelismLevel);
 }
 
+// Write empty files that may be expected by a distributed build
+// system when invoked with thinlto_index_only. This is invoked when
+// the linker has decided not to include the given module in the
+// final link. Frequently the distributed build system will want to
+// confirm that all expected outputs are created based on all of the
+// modules provided to the linker.
+static void writeEmptyDistributedBuildOutputs(std::string &ModulePath,
+                                              std::string &OldPrefix,
+                                              std::string &NewPrefix) {
+  std::string NewModulePath =
+      getThinLTOOutputFile(ModulePath, OldPrefix, NewPrefix);
+  std::error_code EC;
+  {
+    raw_fd_ostream OS(NewModulePath + ".thinlto.bc", EC,
+                      sys::fs::OpenFlags::F_None);
+    if (EC)
+      message(LDPL_FATAL, "Failed to write '%s': %s",
+              (NewModulePath + ".thinlto.bc").c_str(), EC.message().c_str());
+  }
+  if (options::thinlto_emit_imports_files) {
+    raw_fd_ostream OS(NewModulePath + ".imports", EC,
+                      sys::fs::OpenFlags::F_None);
+    if (EC)
+      message(LDPL_FATAL, "Failed to write '%s': %s",
+              (NewModulePath + ".imports").c_str(), EC.message().c_str());
+  }
+}
+
 /// gold informs us that all symbols have been read. At this point, we use
 /// get_symbols to see if any of our definitions have been overridden by a
 /// native object file. Then, perform optimization and codegen.
 static ld_plugin_status allSymbolsReadHook() {
   if (Modules.empty())
     return LDPS_OK;
 
   if (unsigned NumOpts = options::extra.size())
     cl::ParseCommandLineOptions(NumOpts, &options::extra[0]);
 
   // Map to own RAII objects that manage the file opening and releasing
   // interfaces with gold. This is needed only for ThinLTO mode, since
   // unlike regular LTO, where addModule will result in the opened file
   // being merged into a new combined module, we need to keep these files open
   // through Lto->run().
   DenseMap<void *, std::unique_ptr<PluginInputFile>> HandleToInputFile;
 
   std::unique_ptr<LTO> Lto = createLTO();
 
+  std::string OldPrefix, NewPrefix;
+  if (options::thinlto_index_only)
+    getThinLTOOldAndNewPrefix(OldPrefix, NewPrefix);
+
   for (claimed_file &F : Modules) {
     if (options::thinlto && !HandleToInputFile.count(F.leader_handle))
       HandleToInputFile.insert(std::make_pair(
           F.leader_handle, llvm::make_unique<PluginInputFile>(F.handle)));
     const void *View = getSymbolsAndView(F);
-    if (!View)
+    if (!View) {
+      if (options::thinlto_index_only)
+        // Write empty output files that may be expected by the distributed
+        // build system.
+        writeEmptyDistributedBuildOutputs(F.name, OldPrefix, NewPrefix);
       continue;
+    }
     addModule(*Lto, F, View);
   }
 
   SmallString<128> Filename;
   // Note that getOutputFileName will append a unique ID for each task
   if (!options::obj_path.empty())
     Filename = options::obj_path;
   else if (options::TheOutputType == options::OT_SAVE_TEMPS)