This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by JDevlieghere on Jun 3 2020, 12:11 AM.
Tokens
"Like" token, awarded by kwk.

Details

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

Diff Detail

Event Timeline

JDevlieghere created this revision.Jun 3 2020, 12:11 AM
JDevlieghere retitled this revision from [Interpreter] Color "error:" and "warning:" in the CommandReturnObject output. to [lldb/Interpreter] Color "error:" and "warning:" in the CommandReturnObject output..Jun 3 2020, 12:14 AM
teemperor accepted this revision.Jun 3 2020, 12:47 AM

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.

This revision is now accepted and ready to land.Jun 3 2020, 12:47 AM

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 requested changes to this revision.Jun 3 2020, 1:32 AM

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.

This revision now requires changes to proceed.Jun 3 2020, 1:32 AM
teemperor added inline comments.Jun 3 2020, 1:52 AM
lldb/include/lldb/Utility/Stream.h
433

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

kwk awarded a token.Jun 3 2020, 2:02 AM

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).

Rebase on top of parent patches.

labath added inline comments.Jun 8 2020, 4:53 AM
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 marked an inline comment as done.

Address feedback

Add unstaged file.

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...

  • Remove default value for color in CommandReturnObject.
  • Add test
JDevlieghere marked 4 inline comments as done.Jun 9 2020, 9:43 AM
JDevlieghere added inline comments.
lldb/include/lldb/Utility/Stream.h
416–417

Correct!

JDevlieghere marked an inline comment as done.
  • Update ctors in lldb-test and unit test
labath accepted this revision.Jun 9 2020, 10:16 AM
This revision is now accepted and ready to land.Jun 9 2020, 10:16 AM
This revision was automatically updated to reflect the committed changes.