[LTO] Remove temporary files corresponding to stream which LTO skips

[vitalybuka]

Summary

Recently few test failed to detect regression because of files created on the
previews run.

[vitalybuka]

lld/COFF/LTO.cpp

150

No tests as I don't know how to reproduce this for LLD.
In other LTO invocation it can be triggered with -thinlto-distributed-indexes

[vitalybuka]

Not sure if this patch is needed, or we can just ignore the issue

[tejohnson]

llvm/test/ThinLTO/X86/delete-unused-temps.ll

9

I'm not sure I understand what is going on - why are there fewer files after the -thinlto-distributed-indexes invocation than without?

[vitalybuka]

llvm/test/ThinLTO/X86/delete-unused-temps.ll

9

not 100% sure without debugging, but I think it's because LTO::getMaxTasks() in both cases returns 2, so LTO client expects 2 streams. But with -thinlto-distributed-indexes nothing is written into the last stream as module is empty, and nothing deletes corresponding file from a previous runs.

[pcc]

I probably wouldn't do this. It seems a little better to change the tests to rm -f the expected files beforehand rather than have the tests rely on code to work properly.

lld/COFF/LTO.cpp

checkError(LTOObj->run(
       },
       Cache));
 
   if (!Config->LTOCache.empty())
     pruneCache(Config->LTOCache, Config->LTOCachePolicy);
 
   std::vector<StringRef> Ret;
   for (unsigned I = 0; I != MaxTasks; ++I) {
-    if (Buff[I].empty())
+    std::string FileName = Config->OutputFile;
+    if (I)
+      FileName += std::to_string(I);
+    FileName += ".lto.obj";
+    if (Buff[I].empty()) {
+      if (Config->SaveTemps)
+        sys::fs::remove(FileName);
       continue;
-    if (Config->SaveTemps) {
-      if (I == 0)
-        saveBuffer(Buff[I], Config->OutputFile + ".lto.obj");
-      else
-        saveBuffer(Buff[I], Config->OutputFile + Twine(I) + ".lto.obj");
     }
+    if (Config->SaveTemps)
+      saveBuffer(Buff[I], FileName);
     Ret.emplace_back(Buff[I].data(), Buff[I].size());
   }
 
   for (std::unique_ptr<MemoryBuffer> &File : Files)
     if (File)
       Ret.push_back(File->getBuffer());
 
   return Ret;
 }

lld/ELF/LTO.cpp

checkError(LTOObj->run(
             llvm::make_unique<raw_svector_ostream>(Buff[Task]));
       },
       Cache));
 
   if (!Config->ThinLTOCacheDir.empty())
     pruneCache(Config->ThinLTOCacheDir, Config->ThinLTOCachePolicy);
 
   for (unsigned I = 0; I != MaxTasks; ++I) {
-    if (Buff[I].empty())
+    std::string FileName = Config->OutputFile;
+    if (I)
+      FileName += std::to_string(I);
+    FileName += ".lto.o";
+    if (Buff[I].empty()) {
+      if (Config->SaveTemps)
+        sys::fs::remove(FileName);
       continue;
-    if (Config->SaveTemps) {
-      if (I == 0)
-        saveBuffer(Buff[I], Config->OutputFile + ".lto.o");
-      else
-        saveBuffer(Buff[I], Config->OutputFile + Twine(I) + ".lto.o");
     }
+    if (Config->SaveTemps)
+      saveBuffer(Buff[I], FileName);
     InputFile *Obj = createObjectFile(MemoryBufferRef(Buff[I], "lto.tmp"));
     Ret.push_back(Obj);
   }
 
   for (std::unique_ptr<MemoryBuffer> &File : Files)
     if (File)
       Ret.push_back(createObjectFile(*File));
 
   return Ret;
 }

llvm/test/ThinLTO/X86/delete-unused-temps.ll

+; Test that llvm-lto2 deletes unused temps.
+
+; RUN: opt -module-summary -o %t.bc %s
+
+; RUN: llvm-lto2 run  %t.bc -o %t2
+; RUN: ls %t2* | count 2
+
+; RUN: llvm-lto2 run  %t.bc -o %t2 -thinlto-distributed-indexes
+; RUN: ls %t2* | count 1
+
+; CHECK: Format: ELF64-x86-64
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"

llvm/test/tools/gold/X86/thinlto.ll

 ; RUN:    --plugin-opt=thinlto \
 ; RUN:    --plugin-opt=jobs=2 \
 ; RUN:    --plugin-opt=thinlto-index-only \
 ; RUN:    --plugin-opt=obj-path=%t5.o \
 ; RUN:    -shared %t.o %t2.o -o %t4
 ; RUN: llvm-readobj -h %t5.o | FileCheck %s --check-prefix=FORMAT
 ; RUN: llvm-nm %t5.o | count 0
 
+; Test to ensure that gold deleted unneeded file which existed before.
+; RUN: not ls %t5.o1 2>/dev/null
+
 ; NM: T f
 ; NM2: T {{f|g}}
 ; FORMAT: Format: ELF64-x86-64
 
 ; The backend index for this module contains summaries from itself and
 ; Inputs/thinlto.ll, as it imports from the latter.
 ; BACKEND1: <MODULE_STRTAB_BLOCK
 ; BACKEND1-NEXT: <ENTRY {{.*}} record string = '{{.*}}/test/tools/gold/X86/Output/thinlto.ll.tmp{{.*}}.o'

llvm/tools/gold/gold-plugin.cpp

static void recordFile(const std::string &Filename, bool TempOutFile) {
   if (add_input_file(Filename.c_str()) != LDPS_OK)
     message(LDPL_FATAL,
             "Unable to add .o file to the link. File left behind in: %s",
             Filename.c_str());
   if (TempOutFile)
     Cleanup.push_back(Filename);
 }
 
-/// Return the desired output filename given a base input name, a flag
+/// Returns the desired output filename given a base input name.
+static std::string getOutputFileName(StringRef InFilename, int TaskID) {
+  return (TaskID > 0 ? InFilename + utostr(TaskID) : InFilename).str();
+}
+
+/// Return the desired output file given a base input name, a flag
 /// indicating whether a temp file should be generated, and an optional task id.
 /// The new filename generated is returned in \p NewFilename.
-static int getOutputFileName(StringRef InFilename, bool TempOutFile,
-                             SmallString<128> &NewFilename, int TaskID) {
+static int getOutputFile(StringRef InFilename, bool TempOutFile,
   int FD = -1;
   if (TempOutFile) {
     std::error_code EC =
         sys::fs::createTemporaryFile("lto-llvm", "o", FD, NewFilename);
     if (EC)
       message(LDPL_FATAL, "Could not create temporary file: %s",
               EC.message().c_str());
   } else {
-    NewFilename = InFilename;
-    if (TaskID > 0)
-      NewFilename += utostr(TaskID);
+    NewFilename = getOutputFileName(InFilename, TaskID);
     std::error_code EC =
         sys::fs::openFileForWrite(NewFilename, FD, sys::fs::F_None);
     if (EC)
       message(LDPL_FATAL, "Could not open file %s: %s", NewFilename.c_str(),
               EC.message().c_str());
   }
   return FD;
 }
static ld_plugin_status allSymbolsReadHook() {
 
   size_t MaxTasks = Lto->getMaxTasks();
   std::vector<uintptr_t> IsTemporary(MaxTasks);
   std::vector<SmallString<128>> Filenames(MaxTasks);
 
   auto AddStream =
       [&](size_t Task) -> std::unique_ptr<lto::NativeObjectStream> {
     IsTemporary[Task] = !SaveTemps;
-    int FD = getOutputFileName(Filename, /*TempOutFile=*/!SaveTemps,
-                               Filenames[Task], Task);
+    int FD = getOutputFile(Filename, /*TempOutFile=*/!SaveTemps,
     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.
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]);
+    else if (SaveTemps)
+      Cleanup.push_back(getOutputFileName(Filename, I));
 
   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;
 }
 

llvm/tools/llvm-lto2/llvm-lto2.cpp

if (!CommandLineResolutions.empty()) {
     for (auto UnusedRes : CommandLineResolutions)
       llvm::errs() << argv[0] << ": unused symbol resolution for "
                    << UnusedRes.first.first << ',' << UnusedRes.first.second
                    << '\n';
   }
   if (HasErrors)
     return 1;
 
+  auto MakeStreamFileName = [&](size_t Task) -> std::string {
+    return OutputFilename + "." + utostr(Task);
+  };
+
+  size_t MaxTasks = Lto.getMaxTasks();
+  for (unsigned I = 0; I != MaxTasks; ++I)
+    sys::fs::remove(MakeStreamFileName(I));
+
   auto AddStream =
       [&](size_t Task) -> std::unique_ptr<lto::NativeObjectStream> {
-    std::string Path = OutputFilename + "." + utostr(Task);
-
+    std::string Path = MakeStreamFileName(Task);
     std::error_code EC;
     auto S = llvm::make_unique<raw_fd_ostream>(Path, EC, sys::fs::F_None);
     check(EC, Path);
     return llvm::make_unique<lto::NativeObjectStream>(std::move(S));
   };
 
   auto AddBuffer = [&](size_t Task, std::unique_ptr<MemoryBuffer> MB,
                        StringRef Path) {