[clang-tools-extra] d8c0734 - [Support] Move raw_ostream::tie to raw_fd_ostream (#97396)
via cfe-commits
cfe-commits at lists.llvm.org
Wed Jul 3 02:15:06 PDT 2024
Author: Alexis Engelke
Date: 2024-07-03T11:15:02+02:00
New Revision: d8c07342c021f31f08dee7d15e17f7eca0d1aae0
URL: https://github.com/llvm/llvm-project/commit/d8c07342c021f31f08dee7d15e17f7eca0d1aae0
DIFF: https://github.com/llvm/llvm-project/commit/d8c07342c021f31f08dee7d15e17f7eca0d1aae0.diff
LOG: [Support] Move raw_ostream::tie to raw_fd_ostream (#97396)
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 is only used for errs(), move it to raw_fd_ostream so that it no
longer affects performance of other stream classes.
Added:
Modified:
clang-tools-extra/clangd/index/remote/server/Server.cpp
clang-tools-extra/clangd/tool/ClangdMain.cpp
llvm/include/llvm/Support/raw_ostream.h
llvm/lib/Support/raw_ostream.cpp
llvm/unittests/Support/raw_ostream_test.cpp
Removed:
################################################################################
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..df9ee2e5a7858 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();
};
@@ -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
diff --git a/llvm/lib/Support/raw_ostream.cpp b/llvm/lib/Support/raw_ostream.cpp
index 0acb54f76c0bf..2ce54faa9857e 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
@@ -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;
diff --git a/llvm/unittests/Support/raw_ostream_test.cpp b/llvm/unittests/Support/raw_ostream_test.cpp
index 451eda8af51b6..4c4b6cc317be5 100644
--- a/llvm/unittests/Support/raw_ostream_test.cpp
+++ b/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;
More information about the cfe-commits
mailing list