[llvm] [LTO] Fix a crash with thin LTO caching and asm output (PR #138203)

[Teresa Johnson via llvm-commits]

================
@@ -439,27 +439,33 @@ static void codegen(const Config &Conf, TargetMachine *TM,
   std::unique_ptr<CachedFileStream> &Stream = *StreamOrErr;
   TM->Options.ObjectFilenameForDebug = Stream->ObjectPathName;
 
-  legacy::PassManager CodeGenPasses;
-  TargetLibraryInfoImpl TLII(Mod.getTargetTriple());
-  CodeGenPasses.add(new TargetLibraryInfoWrapperPass(TLII));
-  // No need to make index available if the module is empty.
-  // In theory these passes should not use the index for an empty
-  // module, however, this guards against doing any unnecessary summary-based
-  // analysis in the case of a ThinLTO build where this might be an empty
-  // regular LTO combined module, with a large combined index from ThinLTO.
-  if (!isEmptyModule(Mod))
-    CodeGenPasses.add(
-        createImmutableModuleSummaryIndexWrapperPass(&CombinedIndex));
-  if (Conf.PreCodeGenPassesHook)
-    Conf.PreCodeGenPassesHook(CodeGenPasses);
-  if (TM->addPassesToEmitFile(CodeGenPasses, *Stream->OS,
-                              DwoOut ? &DwoOut->os() : nullptr,
-                              Conf.CGFileType))
-    report_fatal_error("Failed to setup codegen");
-  CodeGenPasses.run(Mod);
-
-  if (DwoOut)
-    DwoOut->keep();
+  // Create the LTO pipeline in its own scope so it gets deleted before
+  // Stream->commit() is called. The commit function of CacheFile deletes
----------------
teresajohnson wrote:

Ok I see the issue, which you described earlier but I didn't quite follow until poking around more. As you note there are 2 totally separate raw_ostream objects. The first which is owned by CacheFileStream is of derived type raw_fd_stream, and constructed when invoking the CacheStream (and base class CacheFileStream) constructor. The second is formatted_raw_ostream, which is also derived from raw_ostream but a different one, and it holds a pointer to the other raw_ostream object (via data member TheStream). The formatted_raw_ostream::write_impl calls write() on TheStream.

Presumably if CacheStream::commit was changed to invoke flush() instead, because formatted_raw_ostream has not yet been flushed (and therefore TheStream->write() hadn't been called yet on CacheStream's raw_ostream, we don't actually get the file written as expected by commit()?

This is unfortunately gnarly, and I agree that the only quick fixes are the one in this PR, reverting PR136121, or changing some APIs so that e.g. CacheStream owns the formatted_raw_ostream. My preference is to revert PR136121, as improved errors are nice but IMO not worth the introduced complexity. Most of the callers of the new commit() function still call report_fatal_error on the result, so I don't know that it has improved the situation significantly anyway?

https://github.com/llvm/llvm-project/pull/138203