Frontend: Delete output streams before closing CompilerInstance outputs

[dexonsmith]

Summary

Delete the output streams coming from
CompilerInstance::createOutputFile() and friends once writes are
finished. Concretely, replacing OS->flush() with OS = nullptr in:

• PrecompiledPreambleAction::setEmittedPreamblePCH()
• ExtractAPIAction::EndSourceFileAction()
• cc1_main()'s support for -ftime-trace`

This fixes theoretical bugs related to proxy streams, which may have
cleanups to run in their destructor. E.g., buffer_ostream supports
pwrite() by holding all content in a buffer until destruction time.

This also protects against some logic bugs, triggering a null
dereference on a latter attempt to write to the stream.

No tests, since in practice these particular code paths don't seem to
ever use buffer_ostream; you need to be writing a binary file to a
pipe (such as stdout) to hit it; but I have some other patches in the
works that add some safety, crashing if the stream hasn't been
destructed by the time the CompilerInstance is told to keep the output
file.

[benlangmuir]

LGTM, although I made a suggestion about the spelling.

This fixes theoretical bugs related to proxy streams, which may have
cleanups to run in their destructor. E.g., buffer_ostream supports
pwrite() by holding all content in a buffer until destruction time.

The general point about streams with destructors is well taken, but it seems a bit broken that buffer_ostream cannot flush. I guess the layering it just weird in that we have buffering in the base class, but buffer_ostream wants to do something different. Could we afford to make flush virtual? Not something needed for this patch.

clang/lib/ExtractAPI/ExtractAPIConsumer.cpp

796

Nit: OS.reset(nullptr); makes it more obvious this is a unique_ptr and not a raw pointer. Same for the other cases as well.

[dexonsmith]

In D124635#3481346, @benlangmuir wrote:

LGTM, although I made a suggestion about the spelling.

Thanks for the review!

This fixes theoretical bugs related to proxy streams, which may have
cleanups to run in their destructor. E.g., buffer_ostream supports
pwrite() by holding all content in a buffer until destruction time.

The general point about streams with destructors is well taken, but it seems a bit broken that buffer_ostream cannot flush. I guess the layering it just weird in that we have buffering in the base class, but buffer_ostream wants to do something different. Could we afford to make flush virtual? Not something needed for this patch.

I don't think making flush() virtual would help; flush() means "I want my changes to show up somewhere", but it does not mean "I'm done / EOF". buffer_ostream cannot forward to the buffered stream until it is sure that there will never be a(nother) pwrite() call. Otherwise it can't service the pwrite().

But yeah, it's awkward.

What might be nice is if createOutputFile() took an argument saying "I don't need pwrite"; could implement that by not using buffer_ostream, and instead using a simple forwarding proxy that had report_fatal_error in pwrite_impl().

clang/lib/ExtractAPI/ExtractAPIConsumer.cpp

796

I see your point; I'll update.

clang/lib/ExtractAPI/ExtractAPIConsumer.cpp

void ExtractAPIAction::EndSourceFileAction() {
   if (!OS)
     return;
 
   // Setup a SymbolGraphSerializer to write out collected API information in
   // the Symbol Graph format.
   // FIXME: Make the kind of APISerializer configurable.
   SymbolGraphSerializer SGSerializer(*API, ProductName);
   SGSerializer.serialize(*OS);
-  OS->flush();
+  OS.reset();
 }
 
 std::unique_ptr<raw_pwrite_stream>
 ExtractAPIAction::CreateOutputFile(CompilerInstance &CI, StringRef InFile) {
   std::unique_ptr<raw_pwrite_stream> OS =
       CI.createDefaultOutputFile(/*Binary=*/false, InFile, /*Extension=*/"json",
                                  /*RemoveFileOnSignal=*/false);
   if (!OS)
     return nullptr;
   return OS;
 }

clang/lib/Frontend/PrecompiledPreamble.cpp

std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &CI,
                                                  StringRef InFile) override;
 
   bool hasEmittedPreamblePCH() const { return HasEmittedPreamblePCH; }
 
   void setEmittedPreamblePCH(ASTWriter &Writer) {
     if (FileOS) {
       *FileOS << Buffer->Data;
       // Make sure it hits disk now.
-      FileOS->flush();
+      FileOS.reset();
     }
 
     this->HasEmittedPreamblePCH = true;
     Callbacks.AfterPCHEmitted(Writer);
   }
 
   bool BeginSourceFileAction(CompilerInstance &CI) override {
     assert(CI.getLangOpts().CompilingPCH);

clang/tools/driver/cc1_main.cpp

int cc1_main(ArrayRef<const char *> Argv, const char *Argv0, void *MainAddr) {
 
   if (llvm::timeTraceProfilerEnabled()) {
     SmallString<128> Path(Clang->getFrontendOpts().OutputFile);
     llvm::sys::path::replace_extension(Path, "json");
     if (auto profilerOutput = Clang->createOutputFile(
             Path.str(), /*Binary=*/false, /*RemoveFileOnSignal=*/false,
             /*useTemporary=*/false)) {
       llvm::timeTraceProfilerWrite(*profilerOutput);
-      // FIXME(ibiryukov): make profilerOutput flush in destructor instead.
-      profilerOutput->flush();
+      profilerOutput.reset();
       llvm::timeTraceProfilerCleanup();
       Clang->clearOutputFiles(false);
     }
   }
 
   // Our error handler depends on the Diagnostics object, which we're
   // potentially about to delete. Uninstall the handler now so that any
   // later errors use the default handling behavior instead.