Fix backend crash on multiple close of stdout.

[ABataev]

Summary

If stdout is used as an output file for several outputs (like dep file, output file, etc.) , it causes a crash in llvm::raw_fd_ostream::~raw_fd_ostream(), when destructor tries to close file descriptor, which already is closed in another stream.
Here is the part of the code where it happens:

raw_fd_ostream::~raw_fd_ostream() {
  if (FD >= 0) {
    flush();
    if (ShouldClose && sys::Process::SafelyCloseFileDescriptor(FD))
      error_detected();
  }
...
  if (has_error())
    report_fatal_error("IO failure on output stream.", /*GenCrashDiag=*/false);
}

if stdout is already closed sys::Process::SafelyCloseFileDescriptor() cannot close stdout for the second time and an error flag is set for the stream. It makes the destructor to emit IO failure on output stream. fatal error message.

[yaron.keren]

When stdout goes elsewhere the console, the shell creates the the output file (pipe) and will close it when clang terminates so so why clang should close it at all ? it did not open it.

Practically, we have been running locally

    Error(false), UseAtomicWrites(false) {
if (FD < 0 ) {
  ShouldClose = false;
  return;
}
if (FD <= STDERR_FILENO)
  ShouldClose = false;

and passing regression tests on Windows 7 and Linux, maybe this is required on other usage scenarios or OS.

[ABataev]

In D13128#252630, @yaron.keren wrote:

When stdout goes elsewhere the console, the shell creates the the output file (pipe) and will close it when clang terminates so so why clang should close it at all ? it did not open it.

Practically, we have been running locally

    Error(false), UseAtomicWrites(false) {
if (FD < 0 ) {
  ShouldClose = false;
  return;
}
if (FD <= STDERR_FILENO)
  ShouldClose = false;

and passing regression tests on Windows 7 and Linux, maybe this is required on other usage scenarios or OS.

I thought about it. The problem is that few lines above there is a comment (in getFD()):

// Handle "-" as stdout. Note that when we do this, we consider ourself
// the owner of stdout. This means that we can do things like close the
// file descriptor when we're done and set the "binary" flag globally.

and all such stream are created with ShouldClose flag explicitly set to true. Should we remove all this stuff?

[yaron.keren]

The original commit http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20100816/106268.html by Dan Ghoman says:

"Make raw_fd_ostream consider itself the owner of STDOUT_FILENO when
constructed with an output filename of "-". In particular, allow the
file descriptor to be closed, and close the file descriptor in the
destructor if it hasn't been explicitly closed already, to ensure
that any write errors are detected."

Closing stdout causes not only this problem in the 2nd close (which may be worked around) but potentially losing text output depending on who gets to close the stream first. Is it normal behaviour for console programs to close their stdout?

[ABataev]

Reworked patch after some discussion

[sunfish]

This code has been questioned in this way before, and every time it comes up, it turns out that there's a bug somewhere else that really ought to be fixed anyway.

In previous iterations of this discussion, when clang is found attempting to print something like a dep file to the same stream as the normal compiler output file, this is not actually desirable behavior because it mixes two different kinds of output. Besides the fact that this typically isn't what a user actually wants, it can cause serious problems if one of the stream needs to use "binary" mode and the other doesn't, for example.

The resolution has been to have clang issue an error if the dep file and the output file are both using stdout, which is nice to do, because if a user is asking for this it's likely not intentional. With this done, there is no longer a way to crash the compiler, and there's no need to change how raw_fd_ostream works.

[yaron.keren]

Hi Dan, it makes sense that output streams should not usually be mixed together, especially if one is binary as you write.
This may or may not be a problem depending on what the user really wants. He may want to mix the outputs for whatever purposes or it may usually be a user error.
In any case, that's not how clang deal with usage or even internal unexpected errors. It asserts, print error messages but does not crash on purpose. Having clang crash here does not seem like a good solution and will result in this issue continue to surface again.

[ABataev]

The bad thing is that the same problem can be reproduced not only in frontend, but also in opt tool. The test I wrote (test/Other/empty.ll) also failed with the same message.
And Yaron is right, I need to mix several outputs in stdout. What should I do in this case?

[sunfish]

In any case, that's not how clang deal with usage or even internal unexpected errors. It asserts, print error messages but does not crash on purpose. Having clang crash here does not seem like a good solution and will result in this issue continue to surface again.

When clang has bugs, it is common for it to crash unceremoniously. That said, if you know of a reasonable way to assert here before the crash, that'd be great.

The bad thing is that the same problem can be reproduced not only in frontend, but also in opt tool. The test I wrote (test/Other/empty.ll) also failed with the same message.

This could also be interpreted as opt failing to check for incompatible command-line options. One of the command-line options in this case is a hidden option, so this isn't especially surprising.

And Yaron is right, I need to mix several outputs in stdout. What should I do in this case?

What's the context? It may be worth investigating whether there are other ways to address your need.

If you want to use -rewrite-map-file and -info-output-file in a testcase at the same time, see the documentation for '%t' in the TestingGuide for creating multiple temporary output files.

include/clang/Frontend/CompilerInstance.h

class CompilerInstance : public ModuleLoader {
   ///
   /// If TempFilename is not empty we must rename it to Filename at the end.
   /// TempFilename may be empty and Filename non-empty if creating the temporary
   /// failed.
   struct OutputFile {
     std::string Filename;
     std::string TempFilename;
     std::unique_ptr<raw_ostream> OS;
+    /// \brief true if the file stream should be closed before to be destroyed.
+    bool ShouldClose;
 
     OutputFile(std::string filename, std::string tempFilename,
-               std::unique_ptr<raw_ostream> OS)
+               std::unique_ptr<raw_ostream> OS, bool ShouldClose = false)
         : Filename(std::move(filename)), TempFilename(std::move(tempFilename)),
-          OS(std::move(OS)) {}
+          OS(std::move(OS)), ShouldClose(ShouldClose) {}
     OutputFile(OutputFile &&O)
         : Filename(std::move(O.Filename)),
-          TempFilename(std::move(O.TempFilename)), OS(std::move(O.OS)) {}
+          TempFilename(std::move(O.TempFilename)), OS(std::move(O.OS)),
+          ShouldClose(O.ShouldClose) {}
   };
 
   /// If the output doesn't support seeking (terminal, pipe). we switch
   /// the stream to a buffer_ostream. These are the buffer and the original
   /// stream.
   std::unique_ptr<llvm::raw_fd_ostream> NonSeekStream;
 
   /// The list of active output files.

lib/Frontend/CompilerInstance.cpp

 void CompilerInstance::addOutputFile(OutputFile &&OutFile) {
   assert(OutFile.OS && "Attempt to add empty stream to output list!");
   OutputFiles.push_back(std::move(OutFile));
 }
 
 void CompilerInstance::clearOutputFiles(bool EraseFiles) {
   for (OutputFile &OF : OutputFiles) {
     // Manually close the stream before we rename it.
+    if (OF.OS && OF.ShouldClose) {
+      // Close the stream and clear error state for stdout.
+      auto *Stream = static_cast<llvm::raw_fd_ostream *>(OF.OS.get());
+      Stream->close();
+      Stream->clear_error();
+    }
     OF.OS.reset();
 
     if (!OF.TempFilename.empty()) {
       if (EraseFiles) {
         llvm::sys::fs::remove(OF.TempFilename);
       } else {
         SmallString<128> NewOutFile(OF.Filename);
 
         // If '-working-directory' was passed, the output filename should be
         // relative to that.
         FileMgr->FixupRelativePath(NewOutFile);
         if (std::error_code ec =
                 llvm::sys::fs::rename(OF.TempFilename, NewOutFile)) {
           getDiagnostics().Report(diag::err_unable_to_rename_temp)
             << OF.TempFilename << OF.Filename << ec.message();
 
           llvm::sys::fs::remove(OF.TempFilename);
         }
       }
     } else if (!OF.Filename.empty() && EraseFiles)
       llvm::sys::fs::remove(OF.Filename);
 
   }
   OutputFiles.clear();
+  if (NonSeekStream) {
+    // Close the stream and clear error state for stdout.
+    NonSeekStream->close();
+    NonSeekStream->clear_error();
+  }
   NonSeekStream.reset();
 }
 
 raw_pwrite_stream *
 CompilerInstance::createDefaultOutputFile(bool Binary, StringRef InFile,
                                           StringRef Extension) {
   return createOutputFile(getFrontendOpts().OutputFile, Binary,
                           /*RemoveFileOnSignal=*/true, InFile, Extension,
getDiagnostics().Report(diag::err_fe_unable_to_open_output) << OutputPath
                                                                 << EC.message();
     return nullptr;
   }
 
   raw_pwrite_stream *Ret = OS.get();
   // Add the output file -- but don't try to remove "-", since this means we are
   // using stdin.
   addOutputFile(OutputFile((OutputPathName != "-") ? OutputPathName : "",
-                           TempPathName, std::move(OS)));
+                           TempPathName, std::move(OS),
+                           OutputPathName == "-" && !Binary));
 
   return Ret;
 }
 
 std::unique_ptr<llvm::raw_pwrite_stream> CompilerInstance::createOutputFile(
     StringRef OutputPath, std::error_code &Error, bool Binary,
     bool RemoveFileOnSignal, StringRef InFile, StringRef Extension,
     bool UseTemporary, bool CreateMissingDirectories,

test/Frontend/empty.cpp

+// RUN: %clang_cc1 -verify %s -x c++ -triple x86_64-unknown-linux-gnu -dependency-file - -MT - -emit-llvm -o - | FileCheck %s
+// Check that no additional error messages are generated.
+int main() {
+  return; // expected-error {{non-void function 'main' should return a value}}
+}
+// CHECK: -: