Set Windows console mode to enable support for ansi escape codes

[xbolva00]

Summary

Windows console now supports supports ANSI escape codes, but we need to enable it using SetConsoleMode with ENABLE_VIRTUAL_TERMINAL_PROCESSING flag.

https://docs.microsoft.com/en-us/windows/console/console-virtual-terminal-sequences.

Syntax hightlighting now works fine on Windows:
https://i.imgur.com/P0i04A7.png

[teemperor]

Thanks! Didn't know we can activate this setting for the user.

Some questions though: Should we restore this setting to the original value after LLDB shuts down? And what happens if this code runs on a Windows version before ENABLE_VIRTUAL_TERMINAL_PROCESSING was introduced? I see there some fallback code, but it's not clear to me what it does and what will happen on older platforms.

source/Core/Debugger.cpp

56 ↗

(On Diff #163766)

Can you document why this has to be 0x0004? I assume older Windows versions won't have ENABLE_VIRTUAL_TERMINAL_PROCESSING, so that should also be documented that this code is covering this case.

815 ↗

(On Diff #163766)

A comment here would be nice. E.g. `// Enabling use of ANSI color codes because LLDB is using them to highlight text.

[xbolva00]

In D51615#1222730, @teemperor wrote:

Thanks! Didn't know we can activate this setting for the user.

Some questions though: Should we restore this setting to the original value after LLDB shuts down?

Good question. I tried run lldb, quit it and then I ran echo some ansi code in the same terminal. Ansi code wasn't resolved (same behaviour as before LLDB launch), so LLDB didn't modify something globally in the terminal.
I think we don't need to restore original value, but... suggestions welcome! :)

And what happens if this code runs on a Windows version before ENABLE_VIRTUAL_TERMINAL_PROCESSING was introduced? I see there some fallback code, but it's not clear to me what it does and what will happen on older platforms.

On older systems there is no ENABLE_VIRTUAL_TERMINAL_PROCESSING so we have
SetConsoleMode(0x004). On older systems Windows does not recognize 0x004 and function will return false.

source/Core/Debugger.cpp

56 ↗

(On Diff #163766)

https://docs.microsoft.com/en-us/windows/console/setconsolemode

Should I add a comment with this link?

815 ↗

(On Diff #163766)

Alright, will do.

[xbolva00]

source/Core/Debugger.cpp

819 ↗

(On Diff #163766)

Should I rewrite it to more clear version like

bool ansi_supported = SetConsoleMode(hConsole, consoleMode);
SetUseColor(ansi_supported);

?

[xbolva00]

• Addressed review comments

[xbolva00]

I will reuse the accepted code which arrives with https://reviews.llvm.org/D51611 here also.

[xbolva00]

• Updated

[xbolva00]

@teemperor please take a look again :)

[teemperor]

LGTM from my side, but @zturner should take a look too.

[zturner]

source/Core/Debugger.cpp

53 ↗

(On Diff #163882)

This should be #include "lldb/Host/windows/windows.h". Otherwise you're getting the system windows.h file. If that's what you wanted, then it should be <windows.h> instead of "windows.h", but that's never what you want, we should always be including LLDB's wrapper around windows.h instead.

819 ↗

(On Diff #163766)

Shouldn't we change this to call llvm::sys::Process::UseANSIEscapeCodes(true);?

[xbolva00]

• use llvm::sys::Process::UseANSIEscapeCodes

[zturner]

lgtm after the one suggested change to the pre-processor conditional.

source/Core/Debugger.cpp

809 ↗

(On Diff #164053)

Can you also add defined(_WIN32) to this condition?

815 ↗

(On Diff #163766)

As a future cleanup we should probably add a function to LLVM called AreAnsiEscapeCodesSupported(). Then you could just write:

if (llvm::sys::Process::AreAnsiEscapeCodesSupported())
  llvm::sys::Process::UseAnsiEscapeCodes(true);

and get rid of the preprocessor comment. Don't need to do it in this patch though.

[xbolva00]

• Check for _WIN32

[stella.stamenova]

This change causes one of the LLDB tests to fail on Windows. More specifically:

********************
FAIL: lldb :: Settings/TestDisableColor.test (42646 of 43956)
******************** TEST 'lldb :: Settings/TestDisableColor.test' FAILED ********************
          Script:
          --
          : 'RUN: at line 1';   E:\_work\36\b\LLVMBuild\Release\bin\lldb.EXE -S E:/_work/36/s/llvm/tools/lldb/lit/lit-lldb-init -x -b -s E:\_work\36\s\llvm\tools\lldb\lit\Settings\TestDisableColor.test | E:\_work\36\b\LLVMBuild\Release\bin\FileCheck.EXE E:\_work\36\s\llvm\tools\lldb\lit\Settings\TestDisableColor.test
          --
          Exit Code: 1
          
          Command Output (stdout):
          --
          $ ":" "RUN: at line 1"
          $ "E:\_work\36\b\LLVMBuild\Release\bin\lldb.EXE" "-S" "E:/_work/36/s/llvm/tools/lldb/lit/lit-lldb-init" "-x" "-b" "-s" "E:\_work\36\s\llvm\tools\lldb\lit\Settings\TestDisableColor.test"
          $ "E:\_work\36\b\LLVMBuild\Release\bin\FileCheck.EXE" "E:\_work\36\s\llvm\tools\lldb\lit\Settings\TestDisableColor.test"
          # command stderr:
 ##[error]llvm\tools\lldb\lit\Settings\TestDisableColor.test(7,10): Error GDC2C77F0: CHECK: expected string not found in input
      1>E:\_work\36\s\llvm\tools\lldb\lit\Settings\TestDisableColor.test(7,10): error GDC2C77F0: CHECK: expected string not found in input [e:\_work\36\b\LLVMBuild\check-all.vcxproj]
          
          # CHECK: use-color (boolean) = false
          
                   ^
          
          <stdin>:1:1: note: scanning from here
          
          (lldb) command source -s 0 'E:/_work/36/s/llvm/tools/lldb/lit/lit-lldb-init'
          
          ^
          
          <stdin>:9:1: note: possible intended match here
          
          use-color (boolean) = true
          
          ^

[davide]

You can probably revert it. BTW, is the bot public? I would like to
understand whether I broke something after I commit.

Davide

[xbolva00]

Bot sure how to fix tests since this is somehow hard to problematic.

[stella.stamenova]

In D51615#1226386, @davide wrote:

You can probably revert it. BTW, is the bot public? I would like to
understand whether I broke something after I commit.

Davide

It's not public (yet) because there's one more failure in the LLVM unit tests that needs to be fixed before it can be green and I don't want to make it public and just add noise because of a known failure.

[davide]

Cool. The argument about reverting still stands though (you might
consider following up with logs et simila to make debugging easier).

[stella.stamenova]

In D51615#1226396, @xbolva00 wrote:

Bot sure how to fix tests since this is somehow hard to problematic.

I think the fix for this is simply to not make the call to SetUseColor here. Is there a reason for the call? It looks like the check for "dumb" terminal and "GetIsTerminalWithColors" might have to come after setting the ANSI escape codes if setting the codes impacts those two cases.

[xbolva00]

In D51615#1226445, @stella.stamenova wrote:

In D51615#1226396, @xbolva00 wrote:

Bot sure how to fix tests since this is somehow hard to problematic.

I think the fix for this is simply to not make the call to SetUseColor here. Is there a reason for the call? It looks like the check for "dumb" terminal and "GetIsTerminalWithColors" might have to come after setting the ANSI escape codes if setting the codes impacts those two cases.

We may disable "use color" setting thanks to GetIsTerminalWithColors but then we realize that we can "enable it" via SetConsoleMode for newer Win 10, so we need to call it I think. The reverse order does not help us either.. I think.

[stella.stamenova]

In D51615#1226494, @xbolva00 wrote:

In D51615#1226445, @stella.stamenova wrote:

In D51615#1226396, @xbolva00 wrote:

Bot sure how to fix tests since this is somehow hard to problematic.

I think the fix for this is simply to not make the call to SetUseColor here. Is there a reason for the call? It looks like the check for "dumb" terminal and "GetIsTerminalWithColors" might have to come after setting the ANSI escape codes if setting the codes impacts those two cases.

We may disable "use color" setting thanks to GetIsTerminalWithColors but then we realize that we can "enable it" via SetConsoleMode for newer Win 10, so we need to call it I think. The reverse order does not help us either.. I think.

Sounds like GetIsTerminalWithColors might have to be updated to include the new logic as well. Would you rather commit a fix or revert the change and then commit a new one?

[xbolva00]

In D51615#1226500, @stella.stamenova wrote:

In D51615#1226494, @xbolva00 wrote:

In D51615#1226445, @stella.stamenova wrote:

In D51615#1226396, @xbolva00 wrote:

Bot sure how to fix tests since this is somehow hard to problematic.

I think the fix for this is simply to not make the call to SetUseColor here. Is there a reason for the call? It looks like the check for "dumb" terminal and "GetIsTerminalWithColors" might have to come after setting the ANSI escape codes if setting the codes impacts those two cases.

We may disable "use color" setting thanks to GetIsTerminalWithColors but then we realize that we can "enable it" via SetConsoleMode for newer Win 10, so we need to call it I think. The reverse order does not help us either.. I think.

Sounds like GetIsTerminalWithColors might have to be updated to include the new logic as well. Would you rather commit a fix or revert the change and then commit a new one?

I can try to fix tomorrow with a new patch :)

lldb/trunk/source/Core/Debugger.cpp

 #include "lldb/Utility/Log.h" // for LLDB_LOG_OPTION_...
 #include "lldb/Utility/State.h"
 #include "lldb/Utility/Stream.h" // for Stream
 #include "lldb/Utility/StreamCallback.h"
 #include "lldb/Utility/StreamString.h"
 
 #if defined(_WIN32)
 #include "lldb/Host/windows/PosixApi.h" // for PATH_MAX
+#include "lldb/Host/windows/windows.h"
 #endif
 
 #include "llvm/ADT/None.h"      // for None
 #include "llvm/ADT/STLExtras.h" // for make_unique
 #include "llvm/ADT/StringRef.h"
 #include "llvm/ADT/iterator.h" // for iterator_facade_...
 #include "llvm/Support/DynamicLibrary.h"
 #include "llvm/Support/FileSystem.h"
+#include "llvm/Support/Process.h"
 #include "llvm/Support/Threading.h"
 #include "llvm/Support/raw_ostream.h" // for raw_fd_ostream
 
 #include <list>   // for list
 #include <memory> // for make_shared
 #include <mutex>
 #include <set>          // for set
 #include <stdio.h>      // for size_t, NULL
Debugger::Debugger(lldb::LogOutputCallback log_callback, void *baton)
 
   // Turn off use-color if this is a dumb terminal.
   const char *term = getenv("TERM");
   if (term && !strcmp(term, "dumb"))
     SetUseColor(false);
   // Turn off use-color if we don't write to a terminal with color support.
   if (!m_output_file_sp->GetFile().GetIsTerminalWithColors())
     SetUseColor(false);
+
+#if defined(_WIN32) && defined(ENABLE_VIRTUAL_TERMINAL_PROCESSING)
+  // Enabling use of ANSI color codes because LLDB is using them to highlight
+  // text.
+  llvm::sys::Process::UseANSIEscapeCodes(true);
+  SetUseColor(true);
+#endif
 }
 
 Debugger::~Debugger() { Clear(); }
 
 void Debugger::Clear() {
   //----------------------------------------------------------------------
   // Make sure we call this function only once. With the C++ global destructor
   // chain having a list of debuggers and with code that can be running on