[Support] Move color handling from raw_fd_ostream to raw_ostream

[JDevlieghere]

Summary

This moves the color handling code from raw_fd_ostream to raw_ostream. This was requested in D81058 and will make it possible to unit test WithColor as requested in D81056.

With the exception that colors are now counted towards the output characters, this change is NFC.

[MaskRay]

llvm/lib/Support/raw_ostream.cpp

514

len can be inlined

528

len can be inlined

542

len can be inlined

llvm/unittests/Support/raw_ostream_test.cpp

357

Can we print a character and check the byte sequence? We can do the tests on non-Windows platforms.

[MaskRay]

LGTM.

llvm/unittests/Support/raw_ostream_test.cpp

354

It seems that you can call sys::Process:UseAnsiEscapeCodes(true) to use ANSI escape code on Windows. You can then drop the #ifdef

[labath]

I think this (seemingly simple) patch is complex enough to be split into llvm&lldb parts.

For the llvm part, I think we also need to somehow address the fact that color handling on windows is.... unusual. More on that in my inline comment.

PS: The last upload appears to be broken -- a partial diff only.

lldb/include/lldb/Utility/Stream.h

408 ↗

(On Diff #268365)

I don't think that enabling colors for all lldb streams is a good default. I think a better solution would be to make this a constructor argument and set it to true when constructing the Streams that are a part of the CommandReturnObject (if colors are enabled). Then we wouldn't need the m_colors variable. Would that work?

llvm/lib/Support/raw_ostream.cpp

507–508

This part worries me. The reason why this code is so convoluted and calls out to sys::Process is because on windows, one cannot always just write an ansi code in order to change the color (that functionality only arrived in windows ~10, and it is off by default). The way this code gets around that is by having the sys::Process:: functions call some windows-specific APIs to achieve the color change.

This means that in the cases we don't have ansi emulation enabled, changing the "color" of a raw_string_ostream will result in twiddling of a color of the real terminal that the current process is attached to. I'm not entirely sure what are the effects of that (at the very least, it can cause text which is concurrently displayed to the console be randomly colored), but it definitely does not seem like an ideal state to be in.

This was sort of already a problem with raw_fd_ostream, where if you call enable_colors on a random stream, you'd get strange effects. However, if you only did that on streams where has_colors returned true, everything was ok.

But now, we're going to be routinely calling enable_colors on streams with has_colors() == false, so I think we should come up with a story of how can one be sure that writing a color to a string stream won't mess with global state.

One solution would be to say that one should call a function like maybe_static bool raw_ostream::colors_use_ansi_codes() (whose implementation could simply be return !sys::Process::ColorNeedsFlush()) before he tries to enable colors on these streams. That would be easy to implement but also easy to use incorrectly.

On the other end of the spectrum, we could refactor the raw_ostream<->sys::Process<->ANSI interface. For example, instead of having the Process class vend ansi codes, they could be embedded directly into the raw_ostream class. Most raw_ostream classes would use these directly. However, raw_fd_ostream::changeColor would do something like:

if (is_displayed() && sys::Process::colorsNeedFancyHandling()) {
  flush();
  if (colors == SAVEDCOLOR)
    sys::Process::OutputBold(bg); //this now returns void
  else
    sys::Process::OutputColor(static_cast<char>(colors), bold, bg);
  return *this;
}
return raw_ostream::changeColor(colors, bold, bg);

That would require some amount of refactoring (though hopefully not too much), but it would enable one to always get ansi escape codes out.

OTOH, it's not clear how useful it is to be able to get ansi escape codes if your system can't print them -- it would definitely be useful for testing but in reality they are most likely bound to end up on a terminal sooner or later). So it may be best to just disable printing ansi escape codes to *all* streams, just do it in a less shoot-footy way. One way to do that would be to make enable_colors a no-op on non-displayed streams if fancy handling is required. Maybe

raw_ostream::enable_colors(bool Enable) {
  if (sys::Process::colorsNeedFancyHandling() && !is_displayed() /* or maybe has_colors()?*/)
    return;
  ColorEnabled = Enable;
}

?

Other options are possible...

[JDevlieghere]

In D81110#2073917, @labath wrote:

I think this (seemingly simple) patch is complex enough to be split into llvm&lldb parts.
PS: The last upload appears to be broken -- a partial diff only.

I edited the wrong commit when rebasing to address @MaskRay's comments. I'll address your Stream comment in D81058.

[JDevlieghere]

Implement a variation of Pavel's suggestion. I've added a method called use_color() which computer whether we should use colors are not. I hope the inline comments are sufficiently self-explanatory.

[labath]

In D81110#2074372, @JDevlieghere wrote:

Implement a variation of Pavel's suggestion. I've added a method called use_color() which computer whether we should use colors are not. I hope the inline comments are sufficiently self-explanatory.

Those comments are fine, but can you elaborate on the intended relationship between use_color, has_colors and enable_colors? I'm not sure those are entirely consistent. In particular, the fact that raw_string_ostream::has_colors returns true seems to be in contradiction with the comment on that method in the base class: "This function determines if this stream is displayed and supports colors."

[JDevlieghere]

• Improve comments.
• Use has_colors() to compute the default value for ColorsEnabled.

[jhenderson]

lldb/source/Interpreter/CommandReturnObject.cpp

49–51 ↗

(On Diff #268365)

I assume you can't just use llvm::WithColor::error() here?

Same below.

79 ↗

(On Diff #268365)

Same as above, but with WithColor::warning()

llvm/lib/Support/raw_ostream.cpp

507–508

This may be completely unrelated, but something I wanted to throw out there - sometimes, at least on Windows, if you Ctrl-C in the middle of a process run, or sometimes when you get a crash, the console's colour is permanently changed to something. I have no idea why, but @labath's comments about the terminal's colour itself being changed made me wonder if this was somehow related.

515–516

Is this properly clang-formatted? It looks slightly weird to me.

[labath]

I think this is starting to look ok. There's still the constructor-calling-virtual issue that needs to be resolved though...

llvm/include/llvm/Support/raw_ostream.h

107

This is equivalent to enable_colors(false) because it will not be calling the overriden methods of derived classes (as they don't exist yet) -- the "calling virtual functions from a constructor is weird" problem.

I think it makes sense that the default color state of a generic stream is "disabled". That is basically the status quo, and if someone really wants to enable colors on a string_stream, I don't think it's unreasonable to ask him to explicitly enable that.

raw_fd_ostream can override this behavior. The status quo is that raw_fd_ostream hardcodes ColorEnabled = true, but I don't think it's unreasonable to change that to ColorEnabled = has_colors() like you did (or wanted to do).

515

Per the above comment, I guess this is not really needed?

Also, when you're making changes to raw_string_ostream, don't forget about raw_svector_ostream -- they really shouldn't diverge on color handling.

llvm/lib/Support/raw_ostream.cpp

507–508

Yes, that could be related, though the relationship is probably not so straightforward. You can definitely get the same effect also on unix with ansi escape codes. I'm guessing that the reason why you haven't seen this before is because unix shells typically have coloured prompts, which will override whatever state has the crashed process left the terminal in. You can observe this by killing an interactive application which disables terminal echo or other fancy tty flags -- shells don't typically reset these, and the result can be quite amusing (protip: typing "stty sane" blindly usually fixes things).

In fact, windows may be in a better position to do something about this, since it's the os itself that interprets the color changes (and not a random application sitting on the other end of the pty reading multiplexed output without knowing where it came from) and could reset the state when an application exits. Whether it does that -- I don't know. Based on your description, the answer is probably no.

535–536

(just thinking out loud) It might be nice to move this bit into the use_color function. Then maybe the function should be renamed to something that suggests it can have side effects... prepare_colors ?

llvm/unittests/Support/raw_ostream_test.cpp

367

I'm guessing this check will fail on windows in the "non-ansi" mode, as changing colors there does not produce any "output" whatsoever. Right now, I don't think we're able to assert anything useful on windows. (well.. maybe we could say that the string is either empty or contains the appropriate ansi code).

@MaskRays suggestion for calling sys::Process:UseAnsiEscapeCodes(true) will make this always output ansi codes (regardless of whether the os supports them). The main problem with that is that the call changes global state which can effect the execution of other tests. We could save the original value of that flag and restore it, except that it is currently not possible to get the value of that flag (well... it is kinda exposed through ColorNeedsFlush, but that's a bit weird...)

[JDevlieghere]

• Fix virtual call from ctor.
• Limit the unit test to unix.
• Rename helper to prepare_colors.

[jhenderson]

LGTM, from my point of view. Best get @labath to confirm he's happy though.

llvm/include/llvm/Support/raw_ostream.h

351

to the -> do the (?)

[labath]

Yep, this looks good. Thanks for your patience.

llvm/include/llvm/Support/raw_ostream.h

292

superfluous semicolon

[JDevlieghere]

In D81110#2079321, @labath wrote:

Yep, this looks good. Thanks for your patience.

No worries, I really appreciate a thorough review and the end result is always better for it.

llvm/include/llvm/Support/raw_ostream.h

private:
   /// If buffered, then the raw_ostream owns the buffer if (BufferMode ==
   /// InternalBuffer); otherwise the buffer has been set via SetBuffer and is
   /// managed by the subclass.
   ///
   /// If a subclass installs an external buffer using SetBuffer then it can wait
   /// for a \see write_impl() call to handle the data which has been put into
   /// this buffer.
   char *OutBufStart, *OutBufEnd, *OutBufCur;
+  bool ColorEnabled = false;
 
   enum class BufferKind {
     Unbuffered = 0,
     InternalBuffer,
     ExternalBuffer
   } BufferMode;
 
 public:
public:
   static constexpr Colors RESET = Colors::RESET;
 
   explicit raw_ostream(bool unbuffered = false)
       : BufferMode(unbuffered ? BufferKind::Unbuffered
                               : BufferKind::InternalBuffer) {
     // Start out ready to flush.
     OutBufStart = OutBufEnd = OutBufCur = nullptr;
   }
 
   raw_ostream(const raw_ostream &) = delete;
   void operator=(const raw_ostream &) = delete;
 
   virtual ~raw_ostream();
 
   /// tell - Return the current offset with the file.
   uint64_t tell() const { return current_pos() + GetNumBytesInBuffer(); }
 
public:
   /// forward.
   /// @param Color ANSI color to use, the special SAVEDCOLOR can be used to
   /// change only the bold attribute, and keep colors untouched
   /// @param Bold bold/brighter text, default false
   /// @param BG if true change the background, default: change foreground
   /// @returns itself so it can be used within << invocations
   virtual raw_ostream &changeColor(enum Colors Color,
                                    bool Bold = false,
-                                   bool BG = false) {
-    (void)Color;
-    (void)Bold;
-    (void)BG;
-    return *this;
-  }
+                                   bool BG = false);
 
   /// Resets the colors to terminal defaults. Call this when you are done
   /// outputting colored text, or before program exit.
-  virtual raw_ostream &resetColor() { return *this; }
+  virtual raw_ostream &resetColor();
 
   /// Reverses the foreground and background colors.
-  virtual raw_ostream &reverseColor() { return *this; }
+  virtual raw_ostream &reverseColor();
 
   /// This function determines if this stream is connected to a "tty" or
   /// "console" window. That is, the output would be displayed to the user
   /// rather than being put on a pipe or stored in a file.
   virtual bool is_displayed() const { return false; }
 
   /// This function determines if this stream is displayed and supports colors.
   virtual bool has_colors() const { return is_displayed(); }
 
   // Enable or disable colors. Once disable_colors() is called,
   // changeColor() has no effect until enable_colors() is called.
-  virtual void enable_colors(bool /*enable*/) {}
+  virtual void enable_colors(bool enable) { ColorEnabled = enable; }
 
   //===--------------------------------------------------------------------===//
   // Subclass Interface
   //===--------------------------------------------------------------------===//
 
 private:
   /// The is the piece of the class that is implemented by subclasses.  This
   /// writes the \p Size bytes starting at
private:
   /// Flush the current buffer, which is known to be non-empty. This outputs the
   /// currently buffered data and resets the buffer to empty.
   void flush_nonempty();
 
   /// Copy data into the buffer. Size must not be greater than the number of
   /// unused bytes in the buffer.
   void copy_to_buffer(const char *Ptr, size_t Size);
 
   virtual void anchor();
 };
 
 /// Call the appropriate insertion operator, given an rvalue reference to a
 /// raw_ostream object and return a stream of the same type as the argument.
 template <typename OStream, typename T>
 std::enable_if_t<!std::is_reference<OStream>::value &&
                      std::is_base_of<raw_ostream, OStream>::value,
                  OStream &&>

 //===----------------------------------------------------------------------===//
 
 /// A raw_ostream that writes to a file descriptor.
 ///
 class raw_fd_ostream : public raw_pwrite_stream {
   int FD;
   bool ShouldClose;
   bool SupportsSeeking = false;
-  bool ColorEnabled = true;
 
 #ifdef _WIN32
   /// True if this fd refers to a Windows console device. Mintty and other
   /// terminal emulators are TTYs, but they are not consoles.
   bool IsWindowsConsole = false;
 #endif
 
   std::error_code EC;
public:
   void close();
 
   bool supportsSeeking() { return SupportsSeeking; }
 
   /// Flushes the stream and repositions the underlying file descriptor position
   /// to the offset specified from the beginning of the file.
   uint64_t seek(uint64_t off);
 
-  raw_ostream &changeColor(enum Colors colors, bool bold=false,
-                           bool bg=false) override;
-  raw_ostream &resetColor() override;
-
-  raw_ostream &reverseColor() override;
-
   bool is_displayed() const override;
 
   bool has_colors() const override;
 
-  void enable_colors(bool enable) override { ColorEnabled = enable; }
-
   std::error_code error() const { return EC; }
 
   /// Return the value of the flag in this raw_fd_ostream indicating whether an
   /// output error has been encountered.
   /// This doesn't implicitly flush any pending output.  Also, it doesn't
   /// guarantee to detect all errors unless the stream has been closed.
   bool has_error() const { return bool(EC); }
 
class raw_string_ostream : public raw_ostream {
   void write_impl(const char *Ptr, size_t Size) override;
 
   /// Return the current position within the stream, not counting the bytes
   /// currently in the buffer.
   uint64_t current_pos() const override { return OS.size(); }
 
 public:
   explicit raw_string_ostream(std::string &O) : OS(O) {
     SetUnbuffered();
   }
   ~raw_string_ostream() override;
 
   /// Flushes the stream contents to the target string and returns  the string's
   /// reference.
   std::string& str() {
     flush();
     return OS;

llvm/lib/Support/raw_ostream.cpp

raw_ostream &raw_ostream::indent(unsigned NumSpaces) {
   return write_padding<' '>(*this, NumSpaces);
 }
 
 /// write_zeros - Insert 'NumZeros' nulls.
 raw_ostream &raw_ostream::write_zeros(unsigned NumZeros) {
   return write_padding<'\0'>(*this, NumZeros);
 }
 
+raw_ostream &raw_ostream::changeColor(enum Colors colors, bool bold,
+                                         bool bg) {
+  if (!ColorEnabled)
+    return *this;
+
+  if (sys::Process::ColorNeedsFlush())
+    flush();
+  const char *colorcode =
+      (colors == SAVEDCOLOR)
+          ? sys::Process::OutputBold(bg)
+          : sys::Process::OutputColor(static_cast<char>(colors), bold, bg);
+  if (colorcode) {
+    size_t len = strlen(colorcode);
+    write(colorcode, len);
+  }
+  return *this;
+}
+
+raw_ostream &raw_ostream::resetColor() {
+  if (!ColorEnabled)
+    return *this;
+
+  if (sys::Process::ColorNeedsFlush())
+    flush();
+  const char *colorcode = sys::Process::ResetColor();
+  if (colorcode) {
+    size_t len = strlen(colorcode);
+    write(colorcode, len);
+  }
+  return *this;
+}
+
+raw_ostream &raw_ostream::reverseColor() {
+  if (!ColorEnabled)
+    return *this;
+
+  if (sys::Process::ColorNeedsFlush())
+    flush();
+  const char *colorcode = sys::Process::OutputReverse();
+  if (colorcode) {
+    size_t len = strlen(colorcode);
+    write(colorcode, len);
+  }
+  return *this;
+}
+
+
 void raw_ostream::anchor() {}
 
 //===----------------------------------------------------------------------===//
 //  Formatted Output
 //===----------------------------------------------------------------------===//
 
 // Out of line virtual method.
 void format_object_base::home() {
if (FD <= STDERR_FILENO)
     ShouldClose = false;
 
 #ifdef _WIN32
   // Check if this is a console device. This is not equivalent to isatty.
   IsWindowsConsole =
       ::GetFileType((HANDLE)::_get_osfhandle(fd)) == FILE_TYPE_CHAR;
 #endif
 
+  // Enable color support.
+  enable_colors(true);
+
   // Get the starting position.
   off_t loc = ::lseek(FD, 0, SEEK_CUR);
 #ifdef _WIN32
   // MSVCRT's _lseek(SEEK_CUR) doesn't return -1 for pipes.
   sys::fs::file_status Status;
   std::error_code EC = status(FD, Status);
   SupportsSeeking = !EC && Status.type() == sys::fs::file_type::regular_file;
 #else
if (S_ISCHR(statbuf.st_mode) && is_displayed())
     return 0;
   // Return the preferred block size.
   return statbuf.st_blksize;
 #else
   return raw_ostream::preferred_buffer_size();
 #endif
 }
 
-raw_ostream &raw_fd_ostream::changeColor(enum Colors colors, bool bold,
-                                         bool bg) {
-  if (!ColorEnabled)
-    return *this;
-
-  if (sys::Process::ColorNeedsFlush())
-    flush();
-  const char *colorcode =
-      (colors == SAVEDCOLOR)
-          ? sys::Process::OutputBold(bg)
-          : sys::Process::OutputColor(static_cast<char>(colors), bold, bg);
-  if (colorcode) {
-    size_t len = strlen(colorcode);
-    write(colorcode, len);
-    // don't account colors towards output characters
-    pos -= len;
-  }
-  return *this;
-}
-
-raw_ostream &raw_fd_ostream::resetColor() {
-  if (!ColorEnabled)
-    return *this;
-
-  if (sys::Process::ColorNeedsFlush())
-    flush();
-  const char *colorcode = sys::Process::ResetColor();
-  if (colorcode) {
-    size_t len = strlen(colorcode);
-    write(colorcode, len);
-    // don't account colors towards output characters
-    pos -= len;
-  }
-  return *this;
-}
-
-raw_ostream &raw_fd_ostream::reverseColor() {
-  if (!ColorEnabled)
-    return *this;
-
-  if (sys::Process::ColorNeedsFlush())
-    flush();
-  const char *colorcode = sys::Process::OutputReverse();
-  if (colorcode) {
-    size_t len = strlen(colorcode);
-    write(colorcode, len);
-    // don't account colors towards output characters
-    pos -= len;
-  }
-  return *this;
-}
-
 bool raw_fd_ostream::is_displayed() const {
   return sys::Process::FileDescriptorIsDisplayed(FD);
 }
 
 bool raw_fd_ostream::has_colors() const {
   return sys::Process::FileDescriptorHasColors(FD);
 }
 

llvm/unittests/Support/raw_ostream_test.cpp

TEST(raw_ostreamTest, FormattedHexBytes) {
   EXPECT_EQ("0000: 61 62 63 64 65 66 67 68  |abcdefgh|\n"
             "0008: 69 6a 6b 6c 6d 6e 6f 70  |ijklmnop|",
             format_bytes_with_ascii_str(B.take_front(16), 0, 8, 1));
   EXPECT_EQ("0000: 61 62 63 64 65 66 67  |abcdefg|\n"
             "0007: 68 69 6a 6b 6c        |hijkl|",
             format_bytes_with_ascii_str(B.take_front(12), 0, 7, 1));
 }
 
+TEST(raw_ostreamTest, colors) {
+  {
+    std::string S;
+    raw_string_ostream Sos(S);
+    Sos.changeColor(raw_ostream::YELLOW);
+    EXPECT_TRUE(Sos.str().empty());
+  }
+  {
+    std::string S;
+    raw_string_ostream Sos(S);
+    Sos.enable_colors(true);
+    Sos.changeColor(raw_ostream::YELLOW);
+    EXPECT_FALSE(Sos.str().empty());
+  }
+}
+
 TEST(raw_fd_ostreamTest, multiple_raw_fd_ostream_to_stdout) {
   std::error_code EC;
 
   { raw_fd_ostream("-", EC, sys::fs::OpenFlags::OF_None); }
   { raw_fd_ostream("-", EC, sys::fs::OpenFlags::OF_None); }
 }
 }