[Support] Move raw_ostream::tie to raw_fd_ostream (#97396)

Originally, tie was introduced by D81156 to flush stdout before writing
to stderr. 0308975 reverted this due to race conditions. Nonetheless,
it does cost performance, causing an extra check in the "cold" path,
which is actually the hot path for raw_svector_ostream. Given that this
feature is only used for errs(), move it to raw_fd_ostream so that it no
longer affects performance of other stream classes.

Author: aengelke

clang-tools-extra/clangd/index/remote/server/Server.cpp

@@ -499,8 +499,6 @@ int main(int argc, char *argv[]) {
   }
 
   llvm::errs().SetBuffered();
-  // Don't flush stdout when logging for thread safety.
-  llvm::errs().tie(nullptr);
   auto Logger = makeLogger(LogPrefix.getValue(), llvm::errs());
   clang::clangd::LoggingSession LoggingSession(*Logger);
 

clang-tools-extra/clangd/tool/ClangdMain.cpp

@@ -840,8 +840,6 @@ clangd accepts flags on the commandline, and in the CLANGD_FLAGS environment var
   // Use buffered stream to stderr (we still flush each log message). Unbuffered
   // stream can cause significant (non-deterministic) latency for the logger.
   llvm::errs().SetBuffered();
-  // Don't flush stdout when logging, this would be both slow and racy!
-  llvm::errs().tie(nullptr);
   StreamLogger Logger(llvm::errs(), LogLevel);
   LoggingSession LoggingSession(Logger);
   // Write some initial logs before we start doing any real work.

llvm/include/llvm/Support/raw_ostream.h

@@ -82,10 +82,6 @@ class raw_ostream {
   char *OutBufStart, *OutBufEnd, *OutBufCur;
   bool ColorEnabled = false;
 
-  /// Optional stream this stream is tied to. If this stream is written to, the
-  /// tied-to stream will be flushed first.
-  raw_ostream *TiedStream = nullptr;
-
   enum class BufferKind {
     Unbuffered = 0,
     InternalBuffer,
@@ -360,10 +356,6 @@ class raw_ostream {
 
   bool colors_enabled() const { return ColorEnabled; }
 
-  /// Tie this stream to the specified stream. Replaces any existing tied-to
-  /// stream. Specifying a nullptr unties the stream.
-  void tie(raw_ostream *TieTo) { TiedStream = TieTo; }
-
   //===--------------------------------------------------------------------===//
   // Subclass Interface
   //===--------------------------------------------------------------------===//
@@ -422,9 +414,6 @@ class raw_ostream {
   /// flushing. The result is affected by calls to enable_color().
   bool prepare_colors();
 
-  /// Flush the tied-to stream (if present) and then write the required data.
-  void flush_tied_then_write(const char *Ptr, size_t Size);
-
   virtual void anchor();
 };
 
@@ -475,6 +464,10 @@ class raw_fd_ostream : public raw_pwrite_stream {
   bool IsRegularFile = false;
   mutable std::optional<bool> HasColors;
 
+  /// Optional stream this stream is tied to. If this stream is written to, the
+  /// tied-to stream will be flushed first.
+  raw_ostream *TiedStream = nullptr;
+
 #ifdef _WIN32
   /// True if this fd refers to a Windows console device. Mintty and other
   /// terminal emulators are TTYs, but they are not consoles.
@@ -553,6 +546,13 @@ class raw_fd_ostream : public raw_pwrite_stream {
 
   bool has_colors() const override;
 
+  /// Tie this stream to the specified stream. Replaces any existing tied-to
+  /// stream. Specifying a nullptr unties the stream. This is intended for to
+  /// tie errs() to outs(), so that outs() is flushed whenever something is
+  /// written to errs(), preventing weird and hard-to-test output when stderr
+  /// is redirected to stdout.
+  void tie(raw_ostream *TieTo) { TiedStream = TieTo; }
+
   std::error_code error() const { return EC; }
 
   /// Return the value of the flag in this raw_fd_ostream indicating whether an

llvm/lib/Support/raw_ostream.cpp

@@ -221,15 +221,15 @@ void raw_ostream::flush_nonempty() {
   assert(OutBufCur > OutBufStart && "Invalid call to flush_nonempty.");
   size_t Length = OutBufCur - OutBufStart;
   OutBufCur = OutBufStart;
-  flush_tied_then_write(OutBufStart, Length);
+  write_impl(OutBufStart, Length);
 }
 
 raw_ostream &raw_ostream::write(unsigned char C) {
   // Group exceptional cases into a single branch.
   if (LLVM_UNLIKELY(OutBufCur >= OutBufEnd)) {
     if (LLVM_UNLIKELY(!OutBufStart)) {
       if (BufferMode == BufferKind::Unbuffered) {
-        flush_tied_then_write(reinterpret_cast<char *>(&C), 1);
+        write_impl(reinterpret_cast<char *>(&C), 1);
         return *this;
       }
       // Set up a buffer and start over.
@@ -249,7 +249,7 @@ raw_ostream &raw_ostream::write(const char *Ptr, size_t Size) {
   if (LLVM_UNLIKELY(size_t(OutBufEnd - OutBufCur) < Size)) {
     if (LLVM_UNLIKELY(!OutBufStart)) {
       if (BufferMode == BufferKind::Unbuffered) {
-        flush_tied_then_write(Ptr, Size);
+        write_impl(Ptr, Size);
         return *this;
       }
       // Set up a buffer and start over.
@@ -265,7 +265,7 @@ raw_ostream &raw_ostream::write(const char *Ptr, size_t Size) {
     if (LLVM_UNLIKELY(OutBufCur == OutBufStart)) {
       assert(NumBytes != 0 && "undefined behavior");
       size_t BytesToWrite = Size - (Size % NumBytes);
-      flush_tied_then_write(Ptr, BytesToWrite);
+      write_impl(Ptr, BytesToWrite);
       size_t BytesRemaining = Size - BytesToWrite;
       if (BytesRemaining > size_t(OutBufEnd - OutBufCur)) {
         // Too much left over to copy into our buffer.
@@ -306,12 +306,6 @@ void raw_ostream::copy_to_buffer(const char *Ptr, size_t Size) {
   OutBufCur += Size;
 }
 
-void raw_ostream::flush_tied_then_write(const char *Ptr, size_t Size) {
-  if (TiedStream)
-    TiedStream->flush();
-  write_impl(Ptr, Size);
-}
-
 // Formatted output.
 raw_ostream &raw_ostream::operator<<(const format_object_base &Fmt) {
   // If we have more than a few bytes left in our output buffer, try
@@ -742,6 +736,9 @@ static bool write_console_impl(int FD, StringRef Data) {
 #endif
 
 void raw_fd_ostream::write_impl(const char *Ptr, size_t Size) {
+  if (TiedStream)
+    TiedStream->flush();
+
   assert(FD >= 0 && "File already closed.");
   pos += Size;
 

llvm/unittests/Support/raw_ostream_test.cpp

@@ -388,9 +388,14 @@ TEST(raw_ostreamTest, flush_tied_to_stream_on_write) {
   TiedTo.SetBuffered();
   TiedTo << "a";
 
-  std::string Buffer;
-  raw_string_ostream TiedStream(Buffer);
+  SmallString<64> Path;
+  int FD;
+  ASSERT_FALSE(sys::fs::createTemporaryFile("tietest", "", FD, Path));
+  FileRemover Cleanup(Path);
+  raw_fd_ostream TiedStream(FD, /*ShouldClose=*/false);
+  TiedStream.SetUnbuffered();
   TiedStream.tie(&TiedTo);
+
   // Sanity check that the stream hasn't already been flushed.
   EXPECT_EQ("", TiedToBuffer);
 
@@ -435,30 +440,60 @@ TEST(raw_ostreamTest, flush_tied_to_stream_on_write) {
   TiedStream << "pq";
   EXPECT_EQ("acego", TiedToBuffer);
 
-  // Streams can be tied to each other safely.
+  // Calling tie with nullptr unties stream.
+  TiedStream.SetUnbuffered();
+  TiedStream.tie(nullptr);
+  TiedTo << "y";
+  TiedStream << "0";
+  EXPECT_EQ("acego", TiedToBuffer);
+
+  TiedTo.flush();
   TiedStream.flush();
-  Buffer = "";
+}
+
+static void checkFileData(StringRef FileName, StringRef GoldenData) {
+  ErrorOr<std::unique_ptr<MemoryBuffer>> BufOrErr =
+      MemoryBuffer::getFileOrSTDIN(FileName);
+  EXPECT_FALSE(BufOrErr.getError());
+
+  EXPECT_EQ((*BufOrErr)->getBufferSize(), GoldenData.size());
+  EXPECT_EQ(memcmp((*BufOrErr)->getBufferStart(), GoldenData.data(),
+                   GoldenData.size()),
+            0);
+}
+
+TEST(raw_ostreamTest, raw_fd_ostream_mutual_ties) {
+  SmallString<64> PathTiedTo;
+  int FDTiedTo;
+  ASSERT_FALSE(
+      sys::fs::createTemporaryFile("tietest1", "", FDTiedTo, PathTiedTo));
+  FileRemover CleanupTiedTo(PathTiedTo);
+  raw_fd_ostream TiedTo(FDTiedTo, /*ShouldClose=*/false);
+
+  SmallString<64> PathTiedStream;
+  int FDTiedStream;
+  ASSERT_FALSE(sys::fs::createTemporaryFile("tietest2", "", FDTiedStream,
+                                            PathTiedStream));
+  FileRemover CleanupTiedStream(PathTiedStream);
+  raw_fd_ostream TiedStream(FDTiedStream, /*ShouldClose=*/false);
+
+  // Streams can be tied to each other safely.
+  TiedStream.tie(&TiedTo);
+  TiedStream.SetBuffered();
+  TiedStream.SetBufferSize(2);
   TiedTo.tie(&TiedStream);
   TiedTo.SetBufferSize(2);
   TiedStream << "r";
   TiedTo << "s";
-  EXPECT_EQ("", Buffer);
-  EXPECT_EQ("acego", TiedToBuffer);
+  checkFileData(PathTiedStream.str(), "");
+  checkFileData(PathTiedTo.str(), "");
   TiedTo << "tuv";
-  EXPECT_EQ("r", Buffer);
+  checkFileData(PathTiedStream.str(), "r");
   TiedStream << "wxy";
-  EXPECT_EQ("acegostuv", TiedToBuffer);
-  // The x remains in the buffer, since it was written after the flush of
+  checkFileData(PathTiedTo.str(), "stuv");
+  // The y remains in the buffer, since it was written after the flush of
   // TiedTo.
-  EXPECT_EQ("rwx", Buffer);
-  TiedTo.tie(nullptr);
-
-  // Calling tie with nullptr unties stream.
-  TiedStream.SetUnbuffered();
-  TiedStream.tie(nullptr);
-  TiedTo << "y";
-  TiedStream << "0";
-  EXPECT_EQ("acegostuv", TiedToBuffer);
+  checkFileData(PathTiedStream.str(), "rwx");
 
   TiedTo.flush();
   TiedStream.flush();
@@ -478,17 +513,6 @@ TEST(raw_ostreamTest, reserve_stream) {
   EXPECT_EQ("11111111111111111111hello1world", Str);
 }
 
-static void checkFileData(StringRef FileName, StringRef GoldenData) {
-  ErrorOr<std::unique_ptr<MemoryBuffer>> BufOrErr =
-      MemoryBuffer::getFileOrSTDIN(FileName);
-  EXPECT_FALSE(BufOrErr.getError());
-
-  EXPECT_EQ((*BufOrErr)->getBufferSize(), GoldenData.size());
-  EXPECT_EQ(memcmp((*BufOrErr)->getBufferStart(), GoldenData.data(),
-                   GoldenData.size()),
-            0);
-}
-
 TEST(raw_ostreamTest, writeToOutputFile) {
   SmallString<64> Path;
   int FD;