This is an archive of the discontinued LLVM Phabricator instance.

Disable use-color if the output stream is not a terminal with color support.
ClosedPublic

Authored by teemperor on Aug 24 2018, 5:11 PM.

Details

Summary

LLDB currently only checks the output terminal for color support by looking
at the TERM environment variable and comparing it to "dumb". This causes that
when running LLDB on a CI node, the syntax highlighter will not be deactivated by
LLDB and the output log is filled with color codes (unless the terminal emulator
actually exposes itself as dumb).

This patch now relies on the LLVM code for detecting color support which is more
reliable. We now also correctly actually initialize the m_supports_colors variable in File.
m_supports_colors was so far uninitialized, but the code path that uses m_supports_colors
was also dead so the sanitizers didn't sound an alarm.

The old check that compares TERM is not removed by this patch as the new LLVM code
doesn't seem to handle this case (and it's a good thing to check for "dumb" terminals).

Diff Detail

Repository
rL LLVM

Event Timeline

teemperor created this revision.Aug 24 2018, 5:11 PM
aprantl accepted this revision.Aug 24 2018, 5:12 PM
This revision is now accepted and ready to land.Aug 24 2018, 5:12 PM
aprantl added inline comments.Aug 24 2018, 5:13 PM
source/Core/Debugger.cpp
805 ↗(On Diff #162510)

Shouldn't this check be obsolete now?

805 ↗(On Diff #162510)

You can probably test that by running env TERM=dumb lldb

teemperor added inline comments.Aug 24 2018, 5:15 PM
source/Core/Debugger.cpp
805 ↗(On Diff #162510)

LLVM doesn't seem to check for this environment variable from what I see, so the new code doesn't include this functionality. And it's technically a good thing to check for this, so I think we should let it stay until LLVM also adds this feature.

teemperor edited the summary of this revision. (Show Details)Aug 24 2018, 5:16 PM
teemperor added inline comments.Aug 24 2018, 6:50 PM
source/Core/Debugger.cpp
805 ↗(On Diff #162510)

Just tested it, and the LLVM code indeed ignored TERM=dumb. I'll fix this in LLVM and then we see what people think about this (and if it goes in, we can remove it here).

This revision was automatically updated to reflect the committed changes.