[lldb/Interpreter] Color "error:" and "warning:" in the CommandReturnObject output.

[JDevlieghere]

Summary

Color the error: and warning: part of the CommandReturnObject output, similar to how an error is printed from the driver when colors are enabled.

Here's a screenshot of the output: https://jonasdevlieghere.com/static/colors.png

[teemperor]

That was also on my list but it seems you beat me to it. I couldn't resist some smaller nit-picks but otherwise this LGTM beside that.

lldb/include/lldb/Interpreter/CommandReturnObject.h

27

I think a documentation line like "use_colors Use colors to highlight parts of the command output" would make this more obvious (also use_colors would bring this in line to what we call this setting in other parts of LLDB).

lldb/include/lldb/Utility/Stream.h

433

Nit: This can all be PutCString(colorcode) (which handles the nullptr check and the strlen). Same as below.

[labath]

I'm wondering if it would be possible/acceptable to move the color-related code from llvm::raw_fd_ostream (where the RawOstreamForward code is copied from) into the raw_ostream base class (except that it would default to false, of course). Outputting the color escape codes to a non-terminal, is not a completely unreasonable thing to do. For example, one gather the codes into a string stream, and later output the string to a real terminal -- which, incidentally, is exactly what we are doing here. Then all we would need is to plumb the "color" flag into the appropriate stream object.

One possible objection to that idea might be that this trick will work on windows only if we are using ansi emulation, which isn't always possible (I believe it's not available before windows 10). However, that objection applies regardless of where the feature is implemented -- the only difference is that we may be more willing to throw windows under the bus.

Another thing that would be good to check is whether these color codes make their way through the SB API (SBCommandReturnObject). I don't think that's a bad thing -- the embedding application might actually want to print lldb command results in color. And also, it provides a nice way to test this feature.

[labath]

Let's talk about this more. I don't think we should cargo-cult code from llvm, especially when the cargo-culting won't actually work for the given use case. On windows, the llvm::sys::Process::*** functions will be messing with the terminal which does not sound like a good thing to do from a generic place like this.

[teemperor]

lldb/include/lldb/Utility/Stream.h

433

Actually nvm, I thought this is in the Stream class (not our own raw_ostream subclass).

[amccarth]

I'm just responding to the questions @labath raised without really looking at the details of the code.

I agree that there's nothing wrong with having the ability to output color codes to a file, but it's a surprising default and experience tells me it would break a lot of tests that are rightfully looking just at the content rather than the styling. But having the ability to turn on color for file output would make it possible to also test styling, an ability we don't currently have.

Plumbing the option for color and other niceties down to a low level sounds like a great idea, as long as we get the defaults right. For Windows (for now) and for output redirected to a file, the default should be off. That said, I could see lowering of the color option being a separate effort outside the scope of this patch. For one thing, we'd have to get buy-in from the more general LLVM folks. And several of the llvm utilities currently use the WithColor approach, so there could be a lot of work updating those (or they would continue to use the old approach indefinitely).

[JDevlieghere]

Rebase on top of parent patches.

[labath]

lldb/source/Utility/Stream.cpp

178

It looks like the only use for this function and the member variable is to initialize the ColorEnabled field of the raw_ostream forwarder. Can we delete this and pass the value via a constructor argument instead?

(I don't mind the method itself, just the fact that information on whether colors are "enabled" is stored in two places (m_color_enabled, ColorEnabled). The simplest way to fix that is to delete these things.)

[JDevlieghere]

Address feedback

[JDevlieghere]

Add unstaged file.

[labath]

I think this looks good now. All that's now left is a test case. I think it should be possible to test this in a number of ways:

• CommandReturnObject unittest
• a shell test which forces use-color to true
• a python test which forces use-color to true
• a pexpect test

lldb/include/lldb/Utility/Stream.h

416–417

This is not consistent with the intend of this method (This function determines if this stream is displayed and supports colors.). One could argue whether this could be "true" for CommandObjectResult streams which are destined to be later printed to a terminal, but it's definitely not a good default.

However, all of that is irrelevant, as this should not be needed anymore. Is that correct?

lldb/source/Interpreter/CommandInterpreter.cpp

212

I'm wondering in how many places do we construct CommandReturnObjects. If it's not too many, it may make sense to remove the =false from the colors argument, to force the user to think about the right value...

[JDevlieghere]

• Remove default value for color in CommandReturnObject.
• Add test

[JDevlieghere]

lldb/include/lldb/Utility/Stream.h

416–417

Correct!

[JDevlieghere]

• Update ctors in lldb-test and unit test

lldb/include/lldb/Interpreter/CommandReturnObject.h

 
 #include "lldb/Core/StreamFile.h"
 #include "lldb/Utility/StreamString.h"
 #include "lldb/Utility/StreamTee.h"
 #include "lldb/lldb-private.h"
 
 #include "llvm/ADT/StringRef.h"
 #include "llvm/Support/FormatVariadic.h"
+#include "llvm/Support/WithColor.h"
 
 #include <memory>
 
 namespace lldb_private {
 
 class CommandReturnObject {
 public:
-  CommandReturnObject();
+  CommandReturnObject(bool colors = false);
 
   ~CommandReturnObject();
 
   llvm::StringRef GetOutputData() {
     lldb::StreamSP stream_sp(m_out_stream.GetStreamAtIndex(eStreamStringIndex));
     if (stream_sp)
       return std::static_pointer_cast<StreamString>(stream_sp)->GetString();
     return llvm::StringRef();

lldb/include/lldb/Utility/Stream.h

public:
     /// ByteDelta object was created.
     size_t operator*() const { return m_stream->GetWrittenBytes() - m_start; }
   };
 
   /// Construct with flags and address size and byte order.
   ///
   /// Construct with dump flags \a flags and the default address size. \a
   /// flags can be any of the above enumeration logical OR'ed together.
-  Stream(uint32_t flags, uint32_t addr_size, lldb::ByteOrder byte_order);
+  Stream(uint32_t flags, uint32_t addr_size, lldb::ByteOrder byte_order,
+         bool color = false);
 
   /// Construct a default Stream, not binary, host byte order and host addr
   /// size.
   ///
   Stream();
 
   // FIXME: Streams should not be copyable.
   Stream(const Stream &other) : m_forwarder(*this) { (*this) = other; }
 
   Stream &operator=(const Stream &rhs) {
     m_flags = rhs.m_flags;
     m_addr_size = rhs.m_addr_size;
     m_byte_order = rhs.m_byte_order;
     m_indent_level = rhs.m_indent_level;
+    m_color_enabled = rhs.m_color_enabled;
     return *this;
   }
 
   /// Destructor
   virtual ~Stream();
 
   // Subclasses must override these methods
 
public:
   size_t Indent(llvm::StringRef s = "");
 
   /// Decrement the current indentation level.
   void IndentLess(unsigned amount = 2);
 
   /// Increment the current indentation level.
   void IndentMore(unsigned amount = 2);
 
+  /// The color accessor.
+  ///
+  /// \return
+  ///     True if colors are enabled.
+  bool HasColors() const;
+
+  /// The color accessor.
+  ///
+  /// \param[in] color
+  ///     The new color value.
+  void SetColors(bool colors);
+
   /// Output an offset value.
   ///
   /// Put an offset \a uval out to the stream using the printf format in \a
   /// format.
   ///
   /// \param[in] offset
   ///     The offset value.
   ///

 protected:
   // Member variables
   Flags m_flags;        ///< Dump flags.
   uint32_t m_addr_size; ///< Size of an address in bytes.
   lldb::ByteOrder
       m_byte_order;   ///< Byte order to use when encoding scalar types.
   unsigned m_indent_level;         ///< Indention level.
   std::size_t m_bytes_written = 0; ///< Number of bytes written so far.
+  bool m_color_enabled;
 
   void _PutHex8(uint8_t uvalue, bool add_prefix);
 
   /// Output character bytes to the stream.
   ///
   /// Appends \a src_len characters from the buffer \a src to the stream.
   ///
   /// \param[in] src
class RawOstreamForward : public llvm::raw_ostream {
     // differ from the actually written bytes by the size of the internal
     // raw_ostream buffer.
 
     Stream &m_target;
     void write_impl(const char *Ptr, size_t Size) override {
       m_target.Write(Ptr, Size);
     }
 
+    bool has_colors() const override { return true; }
+
     uint64_t current_pos() const override {
       return m_target.GetWrittenBytes();
     }
 
   public:
     RawOstreamForward(Stream &target)
-        : llvm::raw_ostream(/*unbuffered*/ true), m_target(target) {}
+        : llvm::raw_ostream(/*unbuffered*/ true), m_target(target) {
+      enable_colors(target.HasColors());
+    }
   };
   RawOstreamForward m_forwarder;
 };
 
 /// Output an address value to this stream.
 ///
 /// Put an address \a addr out to the stream with optional \a prefix and \a
 /// suffix strings.
 ///
 /// \param[in] s
 ///     The output stream.
 ///
 /// \param[in] addr
 ///     An address value.
 ///

lldb/source/Interpreter/CommandInterpreter.cpp

bool CommandInterpreter::GetSpaceReplPrompts() const {
   return m_collection_sp->GetPropertyAtIndexAsBoolean(
       nullptr, idx, g_interpreter_properties[idx].default_uint_value != 0);
 }
 
 void CommandInterpreter::Initialize() {
   static Timer::Category func_cat(LLVM_PRETTY_FUNCTION);
   Timer scoped_timer(func_cat, LLVM_PRETTY_FUNCTION);
 
-  CommandReturnObject result;
+  CommandReturnObject result(m_debugger.GetUseColor());
 
   LoadCommandDictionary();
 
   // An alias arguments vector to reuse - reset it before use...
   OptionArgVectorSP alias_arguments_vector_sp(new OptionArgVector);
 
   // Set up some initial aliases.
   CommandObjectSP cmd_obj_sp = GetCommandSPExact("quit", false);
if (!is_interactive) {
     // command output and no command...
     if (EchoCommandNonInteractive(line, io_handler.GetFlags()))
       io_handler.GetOutputStreamFileSP()->Printf(
           "%s%s\n", io_handler.GetPrompt(), line.c_str());
   }
 
   StartHandlingCommand();
 
-  lldb_private::CommandReturnObject result;
+  lldb_private::CommandReturnObject result(m_debugger.GetUseColor());
   HandleCommand(line.c_str(), eLazyBoolCalculate, result);
 
   // Now emit the command output text from the command we just executed
   if ((result.Succeeded() &&
        io_handler.GetFlags().Test(eHandleCommandFlagPrintResult)) ||
       io_handler.GetFlags().Test(eHandleCommandFlagPrintErrors)) {
     // Display any STDOUT/STDERR _prior_ to emitting the command result text
     GetProcessOutput();

lldb/source/Interpreter/CommandReturnObject.cpp

if (!s.empty()) {
 
     const char last_char = *s.rbegin();
     add_newline = last_char != '\n' && last_char != '\r';
   }
   if (add_newline)
     strm.EOL();
 }
 
-CommandReturnObject::CommandReturnObject()
+CommandReturnObject::CommandReturnObject(bool colors)
     : m_out_stream(), m_err_stream(), m_status(eReturnStatusStarted),
-      m_did_change_process_state(false), m_interactive(true) {}
+      m_did_change_process_state(false), m_interactive(true) {
+  m_out_stream.SetColors(colors);
+  m_err_stream.SetColors(colors);
+}
 
 CommandReturnObject::~CommandReturnObject() {}
 
 void CommandReturnObject::AppendErrorWithFormat(const char *format, ...) {
   if (!format)
     return;
   va_list args;
   va_start(args, format);
   StreamString sstrm;
   sstrm.PrintfVarArg(format, args);
   va_end(args);
 
   const std::string &s = std::string(sstrm.GetString());
   if (!s.empty()) {
-    Stream &error_strm = GetErrorStream();
-    error_strm.PutCString("error: ");
-    DumpStringToStreamWithNewline(error_strm, s);
+    llvm::WithColor::error(GetErrorStream().AsRawOstream());
+    DumpStringToStreamWithNewline(GetErrorStream(), s);
   }
 }
 
 void CommandReturnObject::AppendMessageWithFormat(const char *format, ...) {
   if (!format)
     return;
   va_list args;
   va_start(args, format);
   StreamString sstrm;
   sstrm.PrintfVarArg(format, args);
   va_end(args);
 
   GetOutputStream() << sstrm.GetString();
 }
 
 void CommandReturnObject::AppendWarningWithFormat(const char *format, ...) {
   if (!format)
     return;
   va_list args;
   va_start(args, format);
   StreamString sstrm;
   sstrm.PrintfVarArg(format, args);
   va_end(args);
 
-  GetErrorStream() << "warning: " << sstrm.GetString();
+  llvm::WithColor::warning(GetErrorStream().AsRawOstream())
+      << sstrm.GetString();
 }
 
 void CommandReturnObject::AppendMessage(llvm::StringRef in_string) {
   if (in_string.empty())
     return;
   GetOutputStream() << in_string << "\n";
 }
 
 void CommandReturnObject::AppendWarning(llvm::StringRef in_string) {
   if (in_string.empty())
     return;
-  GetErrorStream() << "warning: " << in_string << "\n";
+  llvm::WithColor::warning(GetErrorStream().AsRawOstream())
+      << in_string << '\n';
 }
 
 // Similar to AppendWarning, but do not prepend 'warning: ' to message, and
 // don't append "\n" to the end of it.
 
 void CommandReturnObject::AppendRawWarning(llvm::StringRef in_string) {
   if (in_string.empty())
     return;
   GetErrorStream() << in_string;
 }
 
 void CommandReturnObject::AppendError(llvm::StringRef in_string) {
   if (in_string.empty())
     return;
-  GetErrorStream() << "error: " << in_string << "\n";
+  llvm::WithColor::error(GetErrorStream().AsRawOstream()) << in_string << '\n';
 }
 
 void CommandReturnObject::SetError(const Status &error,
                                    const char *fallback_error_cstr) {
   const char *error_cstr = error.AsCString();
   if (error_cstr == nullptr)
     error_cstr = fallback_error_cstr;
   SetError(error_cstr);

lldb/source/Utility/Stream.cpp

 #include <string>
 
 #include <inttypes.h>
 #include <stddef.h>
 
 using namespace lldb;
 using namespace lldb_private;
 
-Stream::Stream(uint32_t flags, uint32_t addr_size, ByteOrder byte_order)
+Stream::Stream(uint32_t flags, uint32_t addr_size, ByteOrder byte_order,
+               bool color)
     : m_flags(flags), m_addr_size(addr_size), m_byte_order(byte_order),
-      m_indent_level(0), m_forwarder(*this) {}
+      m_indent_level(0), m_color_enabled(color), m_forwarder(*this) {}
 
 Stream::Stream()
     : m_flags(0), m_addr_size(4), m_byte_order(endian::InlHostByteOrder()),
-      m_indent_level(0), m_forwarder(*this) {}
+      m_indent_level(0), m_color_enabled(false), m_forwarder(*this) {}
 
 // Destructor
 Stream::~Stream() {}
 
 ByteOrder Stream::SetByteOrder(ByteOrder byte_order) {
   ByteOrder old_byte_order = m_byte_order;
   m_byte_order = byte_order;
   return old_byte_order;

 // Decrement the current indentation level
 void Stream::IndentLess(unsigned amount) {
   if (m_indent_level >= amount)
     m_indent_level -= amount;
   else
     m_indent_level = 0;
 }
 
+bool Stream::HasColors() const { return m_color_enabled; }
+
+void Stream::SetColors(bool colors) {
+  m_color_enabled = colors;
+  m_forwarder.enable_colors(colors);
+}
+
 // Get the address size in bytes
 uint32_t Stream::GetAddressByteSize() const { return m_addr_size; }
 
 // Set the address size in bytes
 void Stream::SetAddressByteSize(uint32_t addr_size) { m_addr_size = addr_size; }
 
 // The flags get accessor
 Flags &Stream::GetFlags() { return m_flags; }