-
Notifications
You must be signed in to change notification settings - Fork 10.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
release/18.x: [Support] Fix color handling in formatted_raw_ostream (#86700) #86940
Conversation
@JDevlieghere What do you think about merging this PR to the release branch? |
@llvm/pr-subscribers-llvm-binary-utilities @llvm/pr-subscribers-llvm-support Author: None (llvmbot) ChangesBackport c9db031 Requested by: @nga888 Full diff: https://github.com/llvm/llvm-project/pull/86940.diff 4 Files Affected:
diff --git a/llvm/include/llvm/Support/FormattedStream.h b/llvm/include/llvm/Support/FormattedStream.h
index 5f937cfa798408..850a18dbb94121 100644
--- a/llvm/include/llvm/Support/FormattedStream.h
+++ b/llvm/include/llvm/Support/FormattedStream.h
@@ -52,6 +52,10 @@ class formatted_raw_ostream : public raw_ostream {
/// have the rest of it.
SmallString<4> PartialUTF8Char;
+ /// DisableScan - Temporarily disable scanning of output. Used to ignore color
+ /// codes.
+ bool DisableScan;
+
void write_impl(const char *Ptr, size_t Size) override;
/// current_pos - Return the current position within the stream,
@@ -89,9 +93,33 @@ class formatted_raw_ostream : public raw_ostream {
SetUnbuffered();
TheStream->SetUnbuffered();
+ enable_colors(TheStream->colors_enabled());
+
Scanned = nullptr;
}
+ void PreDisableScan() {
+ assert(!DisableScan);
+ ComputePosition(getBufferStart(), GetNumBytesInBuffer());
+ assert(PartialUTF8Char.empty());
+ DisableScan = true;
+ }
+
+ void PostDisableScan() {
+ assert(DisableScan);
+ DisableScan = false;
+ Scanned = getBufferStart() + GetNumBytesInBuffer();
+ }
+
+ struct DisableScanScope {
+ formatted_raw_ostream *S;
+
+ DisableScanScope(formatted_raw_ostream *FRO) : S(FRO) {
+ S->PreDisableScan();
+ }
+ ~DisableScanScope() { S->PostDisableScan(); }
+ };
+
public:
/// formatted_raw_ostream - Open the specified file for
/// writing. If an error occurs, information about the error is
@@ -104,12 +132,12 @@ class formatted_raw_ostream : public raw_ostream {
/// underneath it.
///
formatted_raw_ostream(raw_ostream &Stream)
- : TheStream(nullptr), Position(0, 0) {
+ : TheStream(nullptr), Position(0, 0), DisableScan(false) {
setStream(Stream);
}
- explicit formatted_raw_ostream() : TheStream(nullptr), Position(0, 0) {
- Scanned = nullptr;
- }
+ explicit formatted_raw_ostream()
+ : TheStream(nullptr), Position(0, 0), Scanned(nullptr),
+ DisableScan(false) {}
~formatted_raw_ostream() override {
flush();
@@ -136,17 +164,26 @@ class formatted_raw_ostream : public raw_ostream {
}
raw_ostream &resetColor() override {
- TheStream->resetColor();
+ if (colors_enabled()) {
+ DisableScanScope S(this);
+ raw_ostream::resetColor();
+ }
return *this;
}
raw_ostream &reverseColor() override {
- TheStream->reverseColor();
+ if (colors_enabled()) {
+ DisableScanScope S(this);
+ raw_ostream::reverseColor();
+ }
return *this;
}
raw_ostream &changeColor(enum Colors Color, bool Bold, bool BG) override {
- TheStream->changeColor(Color, Bold, BG);
+ if (colors_enabled()) {
+ DisableScanScope S(this);
+ raw_ostream::changeColor(Color, Bold, BG);
+ }
return *this;
}
diff --git a/llvm/lib/Support/FormattedStream.cpp b/llvm/lib/Support/FormattedStream.cpp
index c0d28435099570..c50530e76efc0a 100644
--- a/llvm/lib/Support/FormattedStream.cpp
+++ b/llvm/lib/Support/FormattedStream.cpp
@@ -94,6 +94,9 @@ void formatted_raw_ostream::UpdatePosition(const char *Ptr, size_t Size) {
/// ComputePosition - Examine the current output and update line and column
/// counts.
void formatted_raw_ostream::ComputePosition(const char *Ptr, size_t Size) {
+ if (DisableScan)
+ return;
+
// If our previous scan pointer is inside the buffer, assume we already
// scanned those bytes. This depends on raw_ostream to not change our buffer
// in unexpected ways.
diff --git a/llvm/tools/llvm-mc/llvm-mc.cpp b/llvm/tools/llvm-mc/llvm-mc.cpp
index cf6aaa1f06981f..2754d49645595d 100644
--- a/llvm/tools/llvm-mc/llvm-mc.cpp
+++ b/llvm/tools/llvm-mc/llvm-mc.cpp
@@ -547,11 +547,6 @@ int main(int argc, char **argv) {
std::unique_ptr<MCAsmBackend> MAB(
TheTarget->createMCAsmBackend(*STI, *MRI, MCOptions));
auto FOut = std::make_unique<formatted_raw_ostream>(*OS);
- // FIXME: Workaround for bug in formatted_raw_ostream. Color escape codes
- // are (incorrectly) written directly to the unbuffered raw_ostream wrapped
- // by the formatted_raw_ostream.
- if (Action == AC_CDisassemble)
- FOut->SetUnbuffered();
Str.reset(
TheTarget->createAsmStreamer(Ctx, std::move(FOut), /*asmverbose*/ true,
/*useDwarfDirectory*/ true, IP,
diff --git a/llvm/tools/llvm-objdump/llvm-objdump.cpp b/llvm/tools/llvm-objdump/llvm-objdump.cpp
index 22b427f57658e1..7ecdd60313d065 100644
--- a/llvm/tools/llvm-objdump/llvm-objdump.cpp
+++ b/llvm/tools/llvm-objdump/llvm-objdump.cpp
@@ -2032,13 +2032,6 @@ disassembleObject(ObjectFile &Obj, const ObjectFile &DbgObj,
formatted_raw_ostream FOS(outs());
- // FIXME: Workaround for bug in formatted_raw_ostream. Color escape codes
- // are (incorrectly) written directly to the unbuffered raw_ostream
- // wrapped by the formatted_raw_ostream.
- if (DisassemblyColor == ColorOutput::Enable ||
- DisassemblyColor == ColorOutput::Auto)
- FOS.SetUnbuffered();
-
std::unordered_map<uint64_t, std::string> AllLabels;
std::unordered_map<uint64_t, std::vector<BBAddrMapLabel>> BBAddrMapLabels;
if (SymbolizeOperands) {
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Hi @nga888 (or anyone else). If you would like to add a note about this fix in the release notes (completely optional). Please reply to this comment with a one or two sentence description of the fix. |
The color methods in formatted_raw_ostream were forwarding directly to the underlying stream without considering existing buffered output. This would cause incorrect colored output for buffered uses of formatted_raw_ostream. Fix this issue by applying the color to the formatted_raw_ostream itself and temporarily disabling scanning of any color related output so as not to affect the position tracking. This fix means that workarounds that forced formatted_raw_ostream buffering to be disabled can be removed. In the case of llvm-objdump, this can improve disassembly performance when redirecting to a file by more than an order of magnitude on both Windows and Linux. This improvement restores the disassembly performance when redirecting to a file to a level similar to before color support was added. (cherry picked from commit c9db031)
Fix color handling for buffered formatted_raw_ostream which allows the unbuffered workaround in llvm-objdump to be removed, restoring the performance of llvm-objdump disassembly when the output is redirected. |
Backport c9db031
Requested by: @nga888