This is an archive of the discontinued LLVM Phabricator instance.

[Support] Move color handling from raw_fd_ostream to raw_ostream
ClosedPublic

Authored by JDevlieghere on Jun 3 2020, 10:28 AM.

Details

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.

Diff Detail

Event Timeline

JDevlieghere created this revision.Jun 3 2020, 10:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 3 2020, 10:28 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
MaskRay added inline comments.Jun 3 2020, 3:28 PM
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.

JDevlieghere marked 4 inline comments as done.
MaskRay accepted this revision.Jun 3 2020, 10:49 PM

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

This revision is now accepted and ready to land.Jun 3 2020, 10:49 PM
labath requested changes to this revision.Jun 4 2020, 8:56 AM

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

This revision now requires changes to proceed.Jun 4 2020, 8:56 AM

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.

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.

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

  • Improve comments.
  • Use has_colors() to compute the default value for ColorsEnabled.
jhenderson added inline comments.Jun 5 2020, 12:20 AM
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 added a comment.Jun 5 2020, 2:54 AM

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 marked 9 inline comments as done.
  • Fix virtual call from ctor.
  • Limit the unit test to unix.
  • Rename helper to prepare_colors.
jhenderson accepted this revision.Jun 8 2020, 12:34 AM

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 accepted this revision.Jun 8 2020, 4:43 AM

Yep, this looks good. Thanks for your patience.

llvm/include/llvm/Support/raw_ostream.h
292

superfluous semicolon

This revision is now accepted and ready to land.Jun 8 2020, 4:43 AM
JDevlieghere marked 3 inline comments as done.Jun 8 2020, 9:16 AM

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.

This revision was automatically updated to reflect the committed changes.