[gold] Extract runLTO to avoid exit(0) from function with non-trivial objects on the stack

[vitalybuka]

Summary

exit(0) skips destructors, but we have LTO and dependencies on the stack which may have non trivial ones. E.g. closing files.

[tejohnson]

Can you clarify the motivation for this change - I don't see what is significantly different. The exit(0) for the index-only case is still in the same location and we just end up with a different data structure (Files) on the stack. AFAICT this just trades 2 vectors for a vector of tuples.

[vitalybuka]

In D43537#1014745, @tejohnson wrote:

Can you clarify the motivation for this change - I don't see what is significantly different. The exit(0) for the index-only case is still in the same location and we just end up with a different data structure (Files) on the stack. AFAICT this just trades 2 vectors for a vector of tuples.

exit(0) skips destructors, but we had LTO and dependencies objects on the stack. Some of them may have or add in the future non trivial destructors. E.g. closing files.
In D43415 I've added local std::unique_ptr<raw_fd_ostream> and it produced empty outputs, because stream was not closed correctly by destructor.

[tejohnson]

LGTM

Thanks for the clarification - I focused on the wrong thing given the change to Filenames/IsTemporary.

llvm/tools/gold/gold-plugin.cpp

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]);
-
+/// Runs LTO and return a list of pairs <FileName, IsTemporary>.
+static std::vector<std::pair<SmallString<128>, bool>> runLTO() {
   // 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;
 
   // Owns string objects and tells if index file was already created.
static std::vector<std::pair<SmallString<128>, bool>> runLTO() {
   // 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)
     Filename = output_name + ".o";
   bool SaveTemps = !Filename.empty();
 
   size_t MaxTasks = Lto->getMaxTasks();
-  std::vector<uintptr_t> IsTemporary(MaxTasks);
-  std::vector<SmallString<128>> Filenames(MaxTasks);
+  std::vector<std::pair<SmallString<128>, bool>> Files(MaxTasks);
 
   auto AddStream =
       [&](size_t Task) -> std::unique_ptr<lto::NativeObjectStream> {
-    IsTemporary[Task] = !SaveTemps;
+    Files[Task].second = !SaveTemps;
     int FD = getOutputFileName(Filename, /* TempOutFile */ !SaveTemps,
-                               Filenames[Task], Task);
+                               Files[Task].first, Task);
     return llvm::make_unique<lto::NativeObjectStream>(
         llvm::make_unique<llvm::raw_fd_ostream>(FD, true));
   };
 
   auto AddBuffer = [&](size_t Task, std::unique_ptr<MemoryBuffer> MB,
                        StringRef Path) {
     // Note that this requires that the memory buffers provided to AddBuffer are
     // backed by a file.
-    Filenames[Task] = Path;
+    Files[Task].first = Path;
   };
 
   NativeObjectCache Cache;
   if (!options::cache_dir.empty())
     Cache = check(localCache(options::cache_dir, AddBuffer));
 
   check(Lto->run(AddStream, Cache));
 
   // Write empty output files that may be expected by the distributed build
   // system.
   if (options::thinlto_index_only)
     for (auto &Identifier : ObjectToIndexFileState)
       if (!Identifier.getValue())
         writeEmptyDistributedBuildOutputs(Identifier.getKey(), OldPrefix,
                                           NewPrefix, /* SkipModule */ false);
 
+  return Files;
+}
+
+/// 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]);
+
+  std::vector<std::pair<SmallString<128>, bool>> Files = runLTO();
+
   if (options::TheOutputType == options::OT_DISABLE ||
       options::TheOutputType == options::OT_BC_ONLY)
     return LDPS_OK;
 
   if (options::thinlto_index_only) {
     if (llvm::AreStatisticsEnabled())
       llvm::PrintStatistics();
     cleanup_hook();
     exit(0);
   }
 
-  for (unsigned I = 0; I != MaxTasks; ++I)
-    if (!Filenames[I].empty())
-      recordFile(Filenames[I].str(), IsTemporary[I]);
+  for (const auto &F : Files)
+    if (!F.first.empty())
+      recordFile(F.first.str(), F.second);
 
   if (!options::extra_library_path.empty() &&
       set_extra_library_path(options::extra_library_path.c_str()) != LDPS_OK)
     message(LDPL_FATAL, "Unable to set the extra library path.");
 
   return LDPS_OK;
 }