[Support] Use outs() in ToolOutputFile

[labath]

Summary

If the output filename was specified as "-", the ToolOutputFile class
would create a brand new raw_ostream object referring to the stdout.
This patch changes it to reuse the llvm::outs() singleton.

At the moment, this change should be "NFC", but it does enable other
enhancements, like the automatic stdout/stderr synchronization as
discussed on D80803.

I've checked the history, and I did not find any indication that this
class *has* to use a brand new stream object instead of outs() --
indeed, it is special-casing "-" in a number of places already, so this
change fits the pattern pretty well. I suspect the main reason for the
current state of affairs is that the class was originally introduced
(r111595, in 2010) as a raw_fd_ostream subclass, which made any other
solution impossible.

Another potential benefit of this patch is that it makes it possible to
move the raw_ostream class out of the business of special-casing "-" for
stdout handling. That state of affairs does not seem appropriate because
"-" is a valid filename (albeit hard to access with a lot of command
line tools) on most systems. Handling "-" in ToolOutputFile seems more
appropriate.

[jhenderson]

Thanks for looking at this. Having given it more thought, any side-effects this fix will have must be in the realm of bug fixes - if people were mixing printing to stdout from two different sources, they wouldn't expect potential stream interleaving after all.

Perhaps it's worth a simple unit test that shows that the result of ToolOutputFile.os() is the same as outs(), by comparing addresses? Not sure though.

llvm/include/llvm/Support/raw_ostream.h

501

I'm not sure I'm following the reason for changing from the base class here? I'm guessing it's to fit in well with ToolOutputFile's usage, but coudl that be changed to store a pointer to and return a raw_ostream easily?

llvm/lib/Support/ToolOutputFile.cpp

23

This logic seems to have been inverted?

[labath]

• fix inverted condition
• add test
• replace std::unique_ptr with llvm::Optional

[MaskRay]

LGTM.

llvm/include/llvm/Support/raw_ostream.h

501

&outs() is stored to raw_fd_ostream *ToolOutputFile::OS.

Some ToolOutputFile::os() call sites (lib/LTO/LTOCodeGenerator.cpp) use clear_error(), error(), etc, which requires a raw_fd_ostream

I think changing the return type to raw_fd_ostream & is fine.

[JDevlieghere]

Nice!

[jhenderson]

LGTM, thanks!

llvm/include/llvm/Support/ToolOutputFile.h

 //===- ToolOutputFile.h - Output files for compiler-like tools -----------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 //  This file defines the ToolOutputFile class.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_SUPPORT_TOOLOUTPUTFILE_H
 #define LLVM_SUPPORT_TOOLOUTPUTFILE_H
 
+#include "llvm/ADT/Optional.h"
 #include "llvm/Support/raw_ostream.h"
 
 namespace llvm {
 
 /// This class contains a raw_fd_ostream and adds a few extra features commonly
 /// needed for compiler-like tool output files:
 ///   - The file is automatically deleted if the process is killed.
 ///   - The file is automatically deleted when the ToolOutputFile
class ToolOutputFile {
   public:
     /// The flag which indicates whether we should not delete the file.
     bool Keep;
 
     explicit CleanupInstaller(StringRef Filename);
     ~CleanupInstaller();
   } Installer;
 
-  /// The contained stream. This is intentionally declared after Installer.
-  raw_fd_ostream OS;
+  /// Storage for the stream, if we're owning our own stream. This is
+  /// intentionally declared after Installer.
+  Optional<raw_fd_ostream> OSHolder;
+
+  /// The actual stream to use.
+  raw_fd_ostream *OS;
 
 public:
   /// This constructor's arguments are passed to raw_fd_ostream's
   /// constructor.
   ToolOutputFile(StringRef Filename, std::error_code &EC,
                  sys::fs::OpenFlags Flags);
 
   ToolOutputFile(StringRef Filename, int FD);
 
   /// Return the contained raw_fd_ostream.
-  raw_fd_ostream &os() { return OS; }
+  raw_fd_ostream &os() { return *OS; }
 
   /// Indicate that the tool's job wrt this output file has been successful and
   /// the file should not be deleted.
   void keep() { Installer.Keep = true; }
 };
 
 } // end llvm namespace
 
 #endif

llvm/include/llvm/Support/raw_ostream.h

public:
   ///
   ///   "Errors should never pass silently.
   ///    Unless explicitly silenced."
   ///      - from The Zen of Python, by Tim Peters
   ///
   void clear_error() { EC = std::error_code(); }
 };
 
-/// This returns a reference to a raw_ostream for standard output. Use it like:
-/// outs() << "foo" << "bar";
-raw_ostream &outs();
-
-/// This returns a reference to a raw_ostream for standard error. Use it like:
-/// errs() << "foo" << "bar";
-raw_ostream &errs();
+/// This returns a reference to a raw_fd_ostream for standard output. Use it
+/// like: outs() << "foo" << "bar";
+raw_fd_ostream &outs();
+
+/// This returns a reference to a raw_fd_ostream for standard error. Use it
+/// like: errs() << "foo" << "bar";
+raw_fd_ostream &errs();
 
 /// This returns a reference to a raw_ostream which simply discards output.
 raw_ostream &nulls();
 
 //===----------------------------------------------------------------------===//
 // Output Stream Adaptors
 //===----------------------------------------------------------------------===//
 

llvm/lib/Support/ToolOutputFile.cpp

 //
 //===----------------------------------------------------------------------===//
 
 #include "llvm/Support/ToolOutputFile.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Signals.h"
 using namespace llvm;
 
+static bool isStdout(StringRef Filename) { return Filename == "-"; }
+
 ToolOutputFile::CleanupInstaller::CleanupInstaller(StringRef Filename)
     : Filename(std::string(Filename)), Keep(false) {
   // Arrange for the file to be deleted if the process is killed.
-  if (Filename != "-")
+  if (!isStdout(Filename))
     sys::RemoveFileOnSignal(Filename);
 }
 
 ToolOutputFile::CleanupInstaller::~CleanupInstaller() {
+  if (isStdout(Filename))
+    return;
+
   // Delete the file if the client hasn't told us not to.
-  if (!Keep && Filename != "-")
+  if (!Keep)
     sys::fs::remove(Filename);
 
   // Ok, the file is successfully written and closed, or deleted. There's no
   // further need to clean it up on signals.
-  if (Filename != "-")
-    sys::DontRemoveFileOnSignal(Filename);
 }
 
 ToolOutputFile::ToolOutputFile(StringRef Filename, std::error_code &EC,
                                sys::fs::OpenFlags Flags)
-    : Installer(Filename), OS(Filename, EC, Flags) {
+    : Installer(Filename) {
+  if (isStdout(Filename)) {
+    OS = &outs();
+    EC = std::error_code();
+    return;
+  }
+  OSHolder.emplace(Filename, EC, Flags);
+  OS = OSHolder.getPointer();
   // If open fails, no cleanup is needed.
   if (EC)
     Installer.Keep = true;
 }
 
 ToolOutputFile::ToolOutputFile(StringRef Filename, int FD)
-    : Installer(Filename), OS(FD, true) {}
+    : Installer(Filename) {
+  OSHolder.emplace(FD, true);
+  OS = OSHolder.getPointer();
+}

llvm/lib/Support/raw_ostream.cpp

 }
 
 void raw_fd_ostream::anchor() {}
 
 //===----------------------------------------------------------------------===//
 //  outs(), errs(), nulls()
 //===----------------------------------------------------------------------===//
 
-/// outs() - This returns a reference to a raw_ostream for standard output.
-/// Use it like: outs() << "foo" << "bar";
-raw_ostream &llvm::outs() {
+raw_fd_ostream &llvm::outs() {
   // Set buffer settings to model stdout behavior.
   std::error_code EC;
   static raw_fd_ostream S("-", EC, sys::fs::OF_None);
   assert(!EC);
   return S;
 }
 
-/// errs() - This returns a reference to a raw_ostream for standard error.
-/// Use it like: errs() << "foo" << "bar";
-raw_ostream &llvm::errs() {
+raw_fd_ostream &llvm::errs() {
   // Set standard error to be unbuffered by default.
   static raw_fd_ostream S(STDERR_FILENO, false, true);
   return S;
 }
 
 /// nulls() - This returns a reference to a raw_ostream which discards output.
 raw_ostream &llvm::nulls() {
   static raw_null_ostream S;

llvm/unittests/Support/CMakeLists.txt

add_llvm_unittest(SupportTests
   SymbolRemappingReaderTest.cpp
   TarWriterTest.cpp
   TargetParserTest.cpp
   TaskQueueTest.cpp
   ThreadLocalTest.cpp
   ThreadPool.cpp
   Threading.cpp
   TimerTest.cpp
+  ToolOutputFileTest.cpp
   TypeNameTest.cpp
   TypeTraitsTest.cpp
   TrailingObjectsTest.cpp
   TrigramIndexTest.cpp
   UnicodeTest.cpp
   VersionTupleTest.cpp
   VirtualFileSystemTest.cpp
   YAMLIOTest.cpp

llvm/unittests/Support/ToolOutputFileTest.cpp

+//===- ToolOutputFileTest.cpp - ToolOutputFile tests ----------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/Support/ToolOutputFile.h"
+#include "llvm/Support/FileSystem.h"
+#include "gtest/gtest.h"
+
+using namespace llvm;
+
+namespace {
+
+TEST(ToolOutputFileTest, DashOpensOuts) {
+  std::error_code EC;
+  EXPECT_EQ(&ToolOutputFile("-", EC, sys::fs::OF_None).os(), &outs());
+}
+
+} // namespace

mlir/include/mlir/Pass/PassManager.h

 #include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/iterator.h"
 
 #include <functional>
 #include <vector>
 
 namespace llvm {
 class Any;
-raw_ostream &errs();
+class raw_fd_ostream;
+raw_fd_ostream &errs();
 } // end namespace llvm
 
 namespace mlir {
 class AnalysisManager;
 class MLIRContext;
 class ModuleOp;
 class OperationName;
 class Operation;