[lldb][windows] re-enable unicode tests on Windows

[charles-zablit]

This patch re-enables unicode tests on Windows by improving the Terminal::SupportsUnicode check.

Checking that the stdout handle is a FILE_TYPE_CHAR is a better heuristic than always returning true, which assumed we were always using a terminal and never piping the output.

[github-actions]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

[llvmbot]

@llvm/pr-subscribers-lldb

Author: Charles Zablit (charles-zablit)

Changes

This patch re-enables unicode tests on Windows by improving the Terminal::SupportsUnicode check.

---

Full diff: https://github.com/llvm/llvm-project/pull/190828.diff

2 Files Affected:

• (modified) lldb/packages/Python/lldbsuite/test/decorators.py (+14-4)
• (modified) lldb/source/Host/common/Terminal.cpp (+5-1)

diff --git a/lldb/packages/Python/lldbsuite/test/decorators.py b/lldb/packages/Python/lldbsuite/test/decorators.py
index 8fd38c62f7b16..165833808dd2d 100644
--- a/lldb/packages/Python/lldbsuite/test/decorators.py
+++ b/lldb/packages/Python/lldbsuite/test/decorators.py
@@ -480,15 +480,25 @@ def unicode_test(func):
     """Decorate the item as a test which requires Unicode to be enabled.
 
     lldb checks the value of the `LANG` environment variable for the substring "utf-8"
-    to determine if the terminal supports Unicode (except on Windows, where we assume
-    it's always supported).
+    to determine if the terminal supports Unicode (except on Windows, where stdout
+    being connected to an interactive console is used as the signal instead).
     This decorator sets LANG to `utf-8` before running the test and resets it to its
     previous value afterwards.
     """
 
     if sys.platform == "win32":
-        # Unicode support on Windows is flaky in CI.
-        return expectedFailureWindows
+        import ctypes
+
+        STD_OUTPUT_HANDLE = -11
+        FILE_TYPE_CHAR = 0x0002
+        handle = ctypes.windll.kernel32.GetStdHandle(STD_OUTPUT_HANDLE)
+        file_type = ctypes.windll.kernel32.GetFileType(handle)
+        # Mirror Terminal::SupportsUnicode(): Unicode is supported only when
+        # stdout is connected to a real console.
+        if file_type != FILE_TYPE_CHAR:
+            return unittest.skip(
+                "Unicode test requires an interactive console (stderr is redirected)"
+            )
 
     def unicode_wrapped(*args, **kwargs):
         import os
diff --git a/lldb/source/Host/common/Terminal.cpp b/lldb/source/Host/common/Terminal.cpp
index b6d09425e956e..2ddfb6fc3bebe 100644
--- a/lldb/source/Host/common/Terminal.cpp
+++ b/lldb/source/Host/common/Terminal.cpp
@@ -20,6 +20,10 @@
 #include <termios.h>
 #endif
 
+#ifdef _WIN32
+#include "lldb/Host/windows/windows.h"
+#endif
+
 using namespace lldb_private;
 
 struct Terminal::Data {
@@ -402,7 +406,7 @@ llvm::Error Terminal::SetHardwareFlowControl(bool enabled) {
 
 bool Terminal::SupportsUnicode() {
 #ifdef _WIN32
-  return true;
+  return ::GetFileType(GetStdHandle(STD_OUTPUT_HANDLE)) == FILE_TYPE_CHAR;
 #else
   static std::optional<bool> g_result;
   if (g_result)

[DavidSpickett]

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This is odd, the web UI shows you as "member" so it shouldn't have posted a comment. Hopefully this is a one off problem on GitHub's side.

Checking that the stdout handle is a FILE_TYPE_CHAR is a better heuristic than always returning true,

I have not been following these efforts, so I am curious. Does Unicode support require that a certain terminal is used, or is it purely a feature of Windows itself?

[charles-zablit]

I have not been following these efforts, so I am curious. Does Unicode support require that a certain terminal is used, or is it purely a feature of Windows itself?

This was the original implementation: #168603.

Essentially, since we use WriteConsoleW to write to the console, we can assume that unicode will be supported on modern version of Windows (10 and 11). This was tested in the main Windows terminals (Terminal App, conhost, and Git bash).

The issue with this assumption is that this does not apply to files and pipes, especially on the build bots. The new check should better detect if we are writing to a file or console device. I say "should" because I can't reproduce the exact buildbot setup at desk.

[DavidSpickett]

Essentially, since we use WriteConsoleW to write to the console, we can assume that unicode will be supported on modern version of Windows (10 and 11). This was tested in the main Windows terminals (Terminal App, conhost, and Git bash).

Nice. Pleasantly surprised that conhost works too.

[DavidSpickett]

I understand the logic but I'll let the others take care of reviewing since I'm not up on the details of Windows.

[Nerixyz]

The TestHiddenFrameMarkers doesn't pass for me on Windows when I run it directly - not through lit. Running it through lit will always skip the test (file_type != FILE_TYPE_CHAR).

I ran it with python_d path/to/dotest.py - the command that's invoked from lit. You can find it when you run python build\bin\llvm-lit.py -v -a .\lldb\test\API\terminal\hidden_frame_markers\TestHiddenFrameMarkers.py.

It fails because the hidden frame markers are not visible. I ran the executable manually through LLDB, and they don't show for me there either.

[github-actions]

✅ With the latest revision this PR passed the Python code formatter.

[charles-zablit]

The TestHiddenFrameMarkers doesn't pass for me on Windows when I run it directly - not through lit. Running it through lit will always skip the test (file_type != FILE_TYPE_CHAR).

I ran it with python_d path/to/dotest.py - the command that's invoked from lit. You can find it when you run python build\bin\llvm-lit.py -v -a .\lldb\test\API\terminal\hidden_frame_markers\TestHiddenFrameMarkers.py.

It fails because the hidden frame markers are not visible. I ran the executable manually through LLDB, and they don't show for me there either.

I will retry running the tests again once I fix my test machine.

It turns out that hidden frames are not detected on Windows anyways. I have added an expectedFailure to the tests and opened a tracking issue.

[charles-zablit]

Passing at desk, merging 👍

[github-actions]

@charles-zablit Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!