[clang-tools-extra] [llvm] [Support] Remove raw_ostream::tie (PR #97396)
via cfe-commits
cfe-commits at lists.llvm.org
Tue Jul 2 01:50:23 PDT 2024
llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT-->
@llvm/pr-subscribers-debuginfo
@llvm/pr-subscribers-llvm-support
@llvm/pr-subscribers-clang-tools-extra
Author: Alexis Engelke (aengelke)
<details>
<summary>Changes</summary>
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.
---
What was the motivation to keep this in llvm-dwarfdump? Is there any other way to achieve the same goal?
---
Full diff: https://github.com/llvm/llvm-project/pull/97396.diff
6 Files Affected:
- (modified) clang-tools-extra/clangd/index/remote/server/Server.cpp (-2)
- (modified) clang-tools-extra/clangd/tool/ClangdMain.cpp (-2)
- (modified) llvm/include/llvm/Support/raw_ostream.h (-11)
- (modified) llvm/lib/Support/raw_ostream.cpp (+4-10)
- (modified) llvm/tools/llvm-dwarfdump/llvm-dwarfdump.cpp (-4)
- (modified) llvm/unittests/Support/raw_ostream_test.cpp (-82)
``````````diff
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);
``````````
</details>
https://github.com/llvm/llvm-project/pull/97396
More information about the cfe-commits
mailing list