[clang-tools-extra] [llvm] [Support] Remove raw_ostream::tie (PR #97396)
Alexis Engelke via cfe-commits
cfe-commits at lists.llvm.org
Tue Jul 2 04:34:05 PDT 2024
https://github.com/aengelke updated https://github.com/llvm/llvm-project/pull/97396
>From 04b794a8a02f06803f96c40de042b13fd4b12c5f Mon Sep 17 00:00:00 2001
From: Alexis Engelke <engelke at in.tum.de>
Date: Mon, 1 Jul 2024 16:50:44 +0200
Subject: [PATCH 1/2] [Support] Remove raw_ostream::tie
Originally, tie was introduced by D81156 to flush stdout before writing
to stderr. 030897523 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 sees almost no use, remove it.
Reverts commit 1ce831912c797df1cb6d313d8e576a3f86175b6d.
---
.../clangd/index/remote/server/Server.cpp | 2 -
clang-tools-extra/clangd/tool/ClangdMain.cpp | 2 -
llvm/include/llvm/Support/raw_ostream.h | 11 ---
llvm/lib/Support/raw_ostream.cpp | 14 +---
llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp | 4 -
llvm/unittests/Support/raw_ostream_test.cpp | 82 -------------------
6 files changed, 4 insertions(+), 111 deletions(-)
diff --git a/clang-tools-extra/clangd/index/remote/server/Server.cpp b/clang-tools-extra/clangd/index/remote/server/Server.cpp
index 4ef3ab6f9af9c..52fca53260a16 100644
--- a/clang-tools-extra/clangd/index/remote/server/Server.cpp
+++ b/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);
diff --git a/clang-tools-extra/clangd/tool/ClangdMain.cpp b/clang-tools-extra/clangd/tool/ClangdMain.cpp
index c3ba655ee2dc6..73000d96c6ca8 100644
--- a/clang-tools-extra/clangd/tool/ClangdMain.cpp
+++ b/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.
diff --git a/llvm/include/llvm/Support/raw_ostream.h b/llvm/include/llvm/Support/raw_ostream.h
index 0951ffb19ffa1..012915d341634 100644
--- a/llvm/include/llvm/Support/raw_ostream.h
+++ b/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();
};
diff --git a/llvm/lib/Support/raw_ostream.cpp b/llvm/lib/Support/raw_ostream.cpp
index 0acb54f76c0bf..b202d0fe5a687 100644
--- a/llvm/lib/Support/raw_ostream.cpp
+++ b/llvm/lib/Support/raw_ostream.cpp
@@ -221,7 +221,7 @@ 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) {
@@ -229,7 +229,7 @@ raw_ostream &raw_ostream::write(unsigned char C) {
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
diff --git a/llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp b/llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp
index d00cf52075712..d5f5e6fe37b78 100644
--- a/llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp
+++ b/llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp
@@ -821,10 +821,6 @@ static bool handleFile(StringRef Filename, HandlerFn HandleObj,
int main(int argc, char **argv) {
InitLLVM X(argc, argv);
- // Flush outs() when printing to errs(). This avoids interleaving output
- // between the two.
- errs().tie(&outs());
-
llvm::InitializeAllTargetInfos();
llvm::InitializeAllTargetMCs();
diff --git a/llvm/unittests/Support/raw_ostream_test.cpp b/llvm/unittests/Support/raw_ostream_test.cpp
index 451eda8af51b6..cfd00f212ef11 100644
--- a/llvm/unittests/Support/raw_ostream_test.cpp
+++ b/llvm/unittests/Support/raw_ostream_test.cpp
@@ -382,88 +382,6 @@ TEST(raw_fd_ostreamTest, multiple_raw_fd_ostream_to_stdout) {
{ raw_fd_ostream("-", EC, sys::fs::OpenFlags::OF_None); }
}
-TEST(raw_ostreamTest, flush_tied_to_stream_on_write) {
- std::string TiedToBuffer;
- raw_string_ostream TiedTo(TiedToBuffer);
- TiedTo.SetBuffered();
- TiedTo << "a";
-
- std::string Buffer;
- raw_string_ostream TiedStream(Buffer);
- TiedStream.tie(&TiedTo);
- // Sanity check that the stream hasn't already been flushed.
- EXPECT_EQ("", TiedToBuffer);
-
- // Empty string doesn't cause a flush of TiedTo.
- TiedStream << "";
- EXPECT_EQ("", TiedToBuffer);
-
- // Non-empty strings trigger flush of TiedTo.
- TiedStream << "abc";
- EXPECT_EQ("a", TiedToBuffer);
-
- // Single char write flushes TiedTo.
- TiedTo << "c";
- TiedStream << 'd';
- EXPECT_EQ("ac", TiedToBuffer);
-
- // Write to buffered stream without flush does not flush TiedTo.
- TiedStream.SetBuffered();
- TiedStream.SetBufferSize(2);
- TiedTo << "e";
- TiedStream << "f";
- EXPECT_EQ("ac", TiedToBuffer);
-
- // Explicit flush of buffered stream flushes TiedTo.
- TiedStream.flush();
- EXPECT_EQ("ace", TiedToBuffer);
-
- // Explicit flush of buffered stream with empty buffer does not flush TiedTo.
- TiedTo << "g";
- TiedStream.flush();
- EXPECT_EQ("ace", TiedToBuffer);
-
- // Write of data to empty buffer that is greater than buffer size flushes
- // TiedTo.
- TiedStream << "hijklm";
- EXPECT_EQ("aceg", TiedToBuffer);
-
- // Write of data that overflows buffer size also flushes TiedTo.
- TiedStream.flush();
- TiedStream << "n";
- TiedTo << "o";
- TiedStream << "pq";
- EXPECT_EQ("acego", TiedToBuffer);
-
- // Streams can be tied to each other safely.
- TiedStream.flush();
- Buffer = "";
- TiedTo.tie(&TiedStream);
- TiedTo.SetBufferSize(2);
- TiedStream << "r";
- TiedTo << "s";
- EXPECT_EQ("", Buffer);
- EXPECT_EQ("acego", TiedToBuffer);
- TiedTo << "tuv";
- EXPECT_EQ("r", Buffer);
- TiedStream << "wxy";
- EXPECT_EQ("acegostuv", TiedToBuffer);
- // The x 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);
-
- TiedTo.flush();
- TiedStream.flush();
-}
-
TEST(raw_ostreamTest, reserve_stream) {
std::string Str;
raw_string_ostream OS(Str);
>From b67053aaea5474f05266fb52c6f64d654c3f3eee Mon Sep 17 00:00:00 2001
From: Alexis Engelke <engelke at in.tum.de>
Date: Tue, 2 Jul 2024 13:26:41 +0200
Subject: [PATCH 2/2] Retain tie(), move to raw_fd_ostream, expand comment
---
llvm/include/llvm/Support/raw_ostream.h | 11 ++
llvm/lib/Support/raw_ostream.cpp | 3 +
llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp | 4 +
llvm/unittests/Support/raw_ostream_test.cpp | 128 +++++++++++++++++--
4 files changed, 135 insertions(+), 11 deletions(-)
diff --git a/llvm/include/llvm/Support/raw_ostream.h b/llvm/include/llvm/Support/raw_ostream.h
index 012915d341634..df9ee2e5a7858 100644
--- a/llvm/include/llvm/Support/raw_ostream.h
+++ b/llvm/include/llvm/Support/raw_ostream.h
@@ -464,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.
@@ -542,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
diff --git a/llvm/lib/Support/raw_ostream.cpp b/llvm/lib/Support/raw_ostream.cpp
index b202d0fe5a687..2ce54faa9857e 100644
--- a/llvm/lib/Support/raw_ostream.cpp
+++ b/llvm/lib/Support/raw_ostream.cpp
@@ -736,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;
diff --git a/llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp b/llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp
index d5f5e6fe37b78..d00cf52075712 100644
--- a/llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp
+++ b/llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp
@@ -821,6 +821,10 @@ static bool handleFile(StringRef Filename, HandlerFn HandleObj,
int main(int argc, char **argv) {
InitLLVM X(argc, argv);
+ // Flush outs() when printing to errs(). This avoids interleaving output
+ // between the two.
+ errs().tie(&outs());
+
llvm::InitializeAllTargetInfos();
llvm::InitializeAllTargetMCs();
diff --git a/llvm/unittests/Support/raw_ostream_test.cpp b/llvm/unittests/Support/raw_ostream_test.cpp
index cfd00f212ef11..4c4b6cc317be5 100644
--- a/llvm/unittests/Support/raw_ostream_test.cpp
+++ b/llvm/unittests/Support/raw_ostream_test.cpp
@@ -382,6 +382,123 @@ TEST(raw_fd_ostreamTest, multiple_raw_fd_ostream_to_stdout) {
{ raw_fd_ostream("-", EC, sys::fs::OpenFlags::OF_None); }
}
+TEST(raw_ostreamTest, flush_tied_to_stream_on_write) {
+ std::string TiedToBuffer;
+ raw_string_ostream TiedTo(TiedToBuffer);
+ TiedTo.SetBuffered();
+ TiedTo << "a";
+
+ 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);
+
+ // Empty string doesn't cause a flush of TiedTo.
+ TiedStream << "";
+ EXPECT_EQ("", TiedToBuffer);
+
+ // Non-empty strings trigger flush of TiedTo.
+ TiedStream << "abc";
+ EXPECT_EQ("a", TiedToBuffer);
+
+ // Single char write flushes TiedTo.
+ TiedTo << "c";
+ TiedStream << 'd';
+ EXPECT_EQ("ac", TiedToBuffer);
+
+ // Write to buffered stream without flush does not flush TiedTo.
+ TiedStream.SetBuffered();
+ TiedStream.SetBufferSize(2);
+ TiedTo << "e";
+ TiedStream << "f";
+ EXPECT_EQ("ac", TiedToBuffer);
+
+ // Explicit flush of buffered stream flushes TiedTo.
+ TiedStream.flush();
+ EXPECT_EQ("ace", TiedToBuffer);
+
+ // Explicit flush of buffered stream with empty buffer does not flush TiedTo.
+ TiedTo << "g";
+ TiedStream.flush();
+ EXPECT_EQ("ace", TiedToBuffer);
+
+ // Write of data to empty buffer that is greater than buffer size flushes
+ // TiedTo.
+ TiedStream << "hijklm";
+ EXPECT_EQ("aceg", TiedToBuffer);
+
+ // Write of data that overflows buffer size also flushes TiedTo.
+ TiedStream.flush();
+ TiedStream << "n";
+ TiedTo << "o";
+ TiedStream << "pq";
+ EXPECT_EQ("acego", TiedToBuffer);
+
+ // Calling tie with nullptr unties stream.
+ TiedStream.SetUnbuffered();
+ TiedStream.tie(nullptr);
+ TiedTo << "y";
+ TiedStream << "0";
+ EXPECT_EQ("acego", TiedToBuffer);
+
+ TiedTo.flush();
+ TiedStream.flush();
+}
+
+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";
+ checkFileData(PathTiedStream.str(), "");
+ checkFileData(PathTiedTo.str(), "");
+ TiedTo << "tuv";
+ checkFileData(PathTiedStream.str(), "r");
+ TiedStream << "wxy";
+ checkFileData(PathTiedTo.str(), "stuv");
+ // The y remains in the buffer, since it was written after the flush of
+ // TiedTo.
+ checkFileData(PathTiedStream.str(), "rwx");
+
+ TiedTo.flush();
+ TiedStream.flush();
+}
+
TEST(raw_ostreamTest, reserve_stream) {
std::string Str;
raw_string_ostream OS(Str);
@@ -396,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;
More information about the cfe-commits
mailing list