[Lldb-commits] [PATCH] D88975: [LLDB] On Windows, fix tests

[Alexandre Ganea via Phabricator via lldb-commits]

aganea added a comment.

I've commited one more fix here: https://reviews.llvm.org/rG97e7fbb343e2 - This 
didn't occur locally on my machine, but only on another machine. But in 
essence, the issue was the same, an accentuated Windows-1252 codepage character 
that was being decoded as UTF-8 by the Python runtime.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88975/new/

https://reviews.llvm.org/D88975

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D88975: [LLDB] On Windows, fix tests

[Alexandre Ganea via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG79809f58b024: [LLDB] On Windows, fix tests (authored by 
aganea).

Changed prior to commit:
  https://reviews.llvm.org/D88975?vs=296809=296982#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88975/new/

https://reviews.llvm.org/D88975

Files:
  lldb/packages/Python/lldbsuite/test/decorators.py
  lldb/packages/Python/lldbsuite/test/dotest.py
  lldb/packages/Python/lldbsuite/test_event/build_exception.py
  lldb/test/API/commands/target/basic/TestTargetCommand.py
  lldb/unittests/Utility/StatusTest.cpp

Index: lldb/unittests/Utility/StatusTest.cpp
===
--- lldb/unittests/Utility/StatusTest.cpp
+++ lldb/unittests/Utility/StatusTest.cpp
@@ -10,7 +10,7 @@
 #include "gtest/gtest.h"
 
 #ifdef _WIN32
-#include 
+#include 
 #endif
 
 using namespace lldb_private;
@@ -71,14 +71,26 @@
   EXPECT_FALSE(success.ToError());
   EXPECT_TRUE(success.Success());
 
+  WCHAR name[128]{};
+  ULONG nameLen = llvm::array_lengthof(name);
+  ULONG langs = 0;
+  GetUserPreferredUILanguages(MUI_LANGUAGE_NAME, ,
+  reinterpret_cast(), );
+  // Skip the following tests on non-English, non-US, locales because the
+  // formatted messages will be different.
+  bool skip = wcscmp(L"en-US", name) != 0;
+
   auto s = Status(ERROR_ACCESS_DENIED, ErrorType::eErrorTypeWin32);
   EXPECT_TRUE(s.Fail());
-  EXPECT_STREQ("Access is denied. ", s.AsCString());
+  if (!skip)
+EXPECT_STREQ("Access is denied. ", s.AsCString());
 
   s.SetError(ERROR_IPSEC_IKE_TIMED_OUT, ErrorType::eErrorTypeWin32);
-  EXPECT_STREQ("Negotiation timed out ", s.AsCString());
+  if (!skip)
+EXPECT_STREQ("Negotiation timed out ", s.AsCString());
 
   s.SetError(16000, ErrorType::eErrorTypeWin32);
-  EXPECT_STREQ("unknown error", s.AsCString());
+  if (!skip)
+EXPECT_STREQ("unknown error", s.AsCString());
 }
 #endif
Index: lldb/test/API/commands/target/basic/TestTargetCommand.py
===
--- lldb/test/API/commands/target/basic/TestTargetCommand.py
+++ lldb/test/API/commands/target/basic/TestTargetCommand.py
@@ -350,6 +350,7 @@
 self.expect("target create a b", error=True,
 substrs=["'target create' takes exactly one executable path"])
 
+@skipIfWindowsAndNonEnglish
 @no_debug_info_test
 def test_target_create_nonexistent_core_file(self):
 self.expect("target create -c doesntexist", error=True,
@@ -365,6 +366,7 @@
 self.expect("target create -c '" + tf.name + "'", error=True,
 substrs=["Cannot open '", "': Permission denied"])
 
+@skipIfWindowsAndNonEnglish
 @no_debug_info_test
 def test_target_create_nonexistent_sym_file(self):
 self.expect("target create -s doesntexist doesntexisteither", error=True,
Index: lldb/packages/Python/lldbsuite/test_event/build_exception.py
===
--- lldb/packages/Python/lldbsuite/test_event/build_exception.py
+++ lldb/packages/Python/lldbsuite/test_event/build_exception.py
@@ -12,4 +12,4 @@
 @staticmethod
 def format_build_error(command, command_output):
 return "Error when building test subject.\n\nBuild Command:\n{}\n\nBuild Command Output:\n{}".format(
-command, command_output.decode("utf-8"))
+command, command_output.decode("utf-8", errors='ignore'))
Index: lldb/packages/Python/lldbsuite/test/dotest.py
===
--- lldb/packages/Python/lldbsuite/test/dotest.py
+++ lldb/packages/Python/lldbsuite/test/dotest.py
@@ -52,6 +52,9 @@
 """Returns true if fpath is an executable."""
 if fpath == None:
 return False
+if sys.platform == 'win32':
+if not fpath.endswith(".exe"):
+fpath += ".exe"
 return os.path.isfile(fpath) and os.access(fpath, os.X_OK)
 
 
Index: lldb/packages/Python/lldbsuite/test/decorators.py
===
--- lldb/packages/Python/lldbsuite/test/decorators.py
+++ lldb/packages/Python/lldbsuite/test/decorators.py
@@ -3,6 +3,8 @@
 # System modules
 from distutils.version import LooseVersion
 from functools import wraps
+import ctypes
+import locale
 import os
 import platform
 import re
@@ -592,6 +594,17 @@
 """Decorate the item to skip tests that should be skipped on Windows."""
 return skipIfPlatform(["windows"])(func)
 
+def skipIfWindowsAndNonEnglish(func):
+"""Decorate the item to skip tests that should be skipped on non-English locales on Windows."""
+def is_Windows_NonEnglish(self):
+if lldbplatformutil.getPlatform() != "windows":
+return None
+kernel = ctypes.windll.kernel32
+

[Lldb-commits] [PATCH] D88975: [LLDB] On Windows, fix tests

[Alexandre Ganea via Phabricator via lldb-commits]

aganea marked 2 inline comments as done.
aganea added a comment.

Thank you all for your time!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88975/new/

https://reviews.llvm.org/D88975

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D88975: [LLDB] On Windows, fix tests

[Alexandre Ganea via Phabricator via lldb-commits]

aganea added a comment.

In D88975#2317494 , @amccarth wrote:

> It's interesting that I haven't encountered some of these errors.  There are 
> five _other_ lldb tests that do fail for me.  I have a fix in the works for 
> some of those.

Please see my failing tests (with VS2019 16.7.5):
F13201512: failing_lldb_vs2019.txt 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88975/new/

https://reviews.llvm.org/D88975

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D88975: [LLDB] On Windows, fix tests

[Alexandre Ganea via Phabricator via lldb-commits]

aganea updated this revision to Diff 296809.
aganea marked an inline comment as done.
aganea added a comment.

As discussed above:

- Remove locale-specific messages.
- Skip relevant locale-sensitive TestTargetCommand tests.
- Only skip EXPECT calls in Utility/StatusTest.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88975/new/

https://reviews.llvm.org/D88975

Files:
  lldb/packages/Python/lldbsuite/test/decorators.py
  lldb/packages/Python/lldbsuite/test/dotest.py
  lldb/packages/Python/lldbsuite/test_event/build_exception.py
  lldb/test/API/commands/target/basic/TestTargetCommand.py
  lldb/unittests/Utility/StatusTest.cpp

Index: lldb/unittests/Utility/StatusTest.cpp
===
--- lldb/unittests/Utility/StatusTest.cpp
+++ lldb/unittests/Utility/StatusTest.cpp
@@ -10,7 +10,7 @@
 #include "gtest/gtest.h"
 
 #ifdef _WIN32
-#include 
+#include 
 #endif
 
 using namespace lldb_private;
@@ -71,14 +71,26 @@
   EXPECT_FALSE(success.ToError());
   EXPECT_TRUE(success.Success());
 
+  WCHAR name[128]{};
+  ULONG langs = 0;
+  ULONG nameLen = sizeof(name) / sizeof(WCHAR);
+  GetUserPreferredUILanguages(MUI_LANGUAGE_NAME, , (PZZWSTR),
+  );
+  // Skip the following tests on non-English, non-US, locales because the
+  // formatted messages will be different.
+  bool skip = wcscmp(L"en-US", name) != 0;
+
   auto s = Status(ERROR_ACCESS_DENIED, ErrorType::eErrorTypeWin32);
   EXPECT_TRUE(s.Fail());
-  EXPECT_STREQ("Access is denied. ", s.AsCString());
+  if (!skip)
+EXPECT_STREQ("Access is denied. ", s.AsCString());
 
   s.SetError(ERROR_IPSEC_IKE_TIMED_OUT, ErrorType::eErrorTypeWin32);
-  EXPECT_STREQ("Negotiation timed out ", s.AsCString());
+  if (!skip)
+EXPECT_STREQ("Negotiation timed out ", s.AsCString());
 
   s.SetError(16000, ErrorType::eErrorTypeWin32);
-  EXPECT_STREQ("unknown error", s.AsCString());
+  if (!skip)
+EXPECT_STREQ("unknown error", s.AsCString());
 }
 #endif
Index: lldb/test/API/commands/target/basic/TestTargetCommand.py
===
--- lldb/test/API/commands/target/basic/TestTargetCommand.py
+++ lldb/test/API/commands/target/basic/TestTargetCommand.py
@@ -350,6 +350,7 @@
 self.expect("target create a b", error=True,
 substrs=["'target create' takes exactly one executable path"])
 
+@skipIfWindowsAndNonEnglish
 @no_debug_info_test
 def test_target_create_nonexistent_core_file(self):
 self.expect("target create -c doesntexist", error=True,
@@ -365,6 +366,7 @@
 self.expect("target create -c '" + tf.name + "'", error=True,
 substrs=["Cannot open '", "': Permission denied"])
 
+@skipIfWindowsAndNonEnglish
 @no_debug_info_test
 def test_target_create_nonexistent_sym_file(self):
 self.expect("target create -s doesntexist doesntexisteither", error=True,
Index: lldb/packages/Python/lldbsuite/test_event/build_exception.py
===
--- lldb/packages/Python/lldbsuite/test_event/build_exception.py
+++ lldb/packages/Python/lldbsuite/test_event/build_exception.py
@@ -12,4 +12,4 @@
 @staticmethod
 def format_build_error(command, command_output):
 return "Error when building test subject.\n\nBuild Command:\n{}\n\nBuild Command Output:\n{}".format(
-command, command_output.decode("utf-8"))
+command, command_output.decode("utf-8", errors='ignore'))
Index: lldb/packages/Python/lldbsuite/test/dotest.py
===
--- lldb/packages/Python/lldbsuite/test/dotest.py
+++ lldb/packages/Python/lldbsuite/test/dotest.py
@@ -52,6 +52,9 @@
 """Returns true if fpath is an executable."""
 if fpath == None:
 return False
+if sys.platform == 'win32':
+if not fpath.endswith(".exe"):
+fpath += ".exe"
 return os.path.isfile(fpath) and os.access(fpath, os.X_OK)
 
 
Index: lldb/packages/Python/lldbsuite/test/decorators.py
===
--- lldb/packages/Python/lldbsuite/test/decorators.py
+++ lldb/packages/Python/lldbsuite/test/decorators.py
@@ -3,6 +3,8 @@
 # System modules
 from distutils.version import LooseVersion
 from functools import wraps
+import ctypes
+import locale
 import os
 import platform
 import re
@@ -592,6 +594,17 @@
 """Decorate the item to skip tests that should be skipped on Windows."""
 return skipIfPlatform(["windows"])(func)
 
+def skipIfWindowsAndNonEnglish(func):
+"""Decorate the item to skip tests that should be skipped on non-English locales on Windows."""
+def is_Windows_NonEnglish(self):
+if lldbplatformutil.getPlatform() != "windows":
+return None
+kernel = ctypes.windll.kernel32
+if locale.windows_locale[ 

[Lldb-commits] [PATCH] D88975: [LLDB] On Windows, fix tests

[Alexandre Ganea via Phabricator via lldb-commits]

aganea added a comment.

In D88975#2317566 , @labath wrote:

> I really want to avoid having language specific strings in the tests.

We all agree on that. I'll update the patch once it passes all tests on my 
machines.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88975/new/

https://reviews.llvm.org/D88975

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D88975: [LLDB] On Windows, fix tests

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

I really want to avoid having language specific strings in the tests. Skipping 
tests which depend on these is a reasonable approach (as you've discovered, 
there shouldn't be that many of them -- it's really just the tests which check 
that we actually can retrieve the error from the OS), and it could be later 
improved by switching to an english locale on a best-effort basis (i.e., if the 
locale is available).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88975/new/

https://reviews.llvm.org/D88975

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D88975: [LLDB] On Windows, fix tests

[Alexandre Ganea via Phabricator via lldb-commits]

aganea added a comment.

In D88975#2317494 , @amccarth wrote:

> Maybe dotest.py could change its own Windows locale setting and the processes 
> it spawns would inherit that.  I don't know if that would work, but I don't 
> see a good alternative.

One problem is that some users might //not// have the English language pack 
installed. I only have `fr-CA` and `en-CA` installed but not `en-US`. There's 
absolutely no guarantee that there will be a `en-XX` package available. I'll do 
some more testing and upload a new diff, but I took a defensive approach, like 
for `StatusTest.cpp`, where the two tests below in `TestTargetCommand.py` would 
only run if `en-US` is the current locale. Maybe afterwards, in a subsequent 
patch, we could add more code to temporarily switch to `en-US` locale if it's 
available?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88975/new/

https://reviews.llvm.org/D88975

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D88975: [LLDB] On Windows, fix tests

[Adrian McCarthy via Phabricator via lldb-commits]

amccarth added a comment.

It's interesting that I haven't encountered some of these errors.  There are 
five _other_ lldb tests that do fail for me.  I have a fix in the works for 
some of those.

I agree with @labath that including error message patterns in various languages 
isn't scalable. Since dotest,py is starting up the processes under test, 
perhaps there's a way to force it to use a particular locale.  Besides the 
language of system error messages, locales can change the format of numbers, 
dates, etc.

Unfortunately, I don't think it's as simple as an environment variable.  I 
expect this is driven by the user's (or system default) locales settings as 
tracked by Windows.  That's distinct from the C runtime concept of locales.

Maybe dotest.py could change its own Windows locale setting and the processes 
it spawns would inherit that.  I don't know if that would work, but I don't see 
a good alternative.




Comment at: lldb/unittests/Utility/StatusTest.cpp:80
+  if (wcscmp(L"en-US", name) != 0)
+return;
+

Rather than an early return, perhaps the code should still be exercised, but 
the language-specific EXPECTs could be skipped.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88975/new/

https://reviews.llvm.org/D88975

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D88975: [LLDB] On Windows, fix tests

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

Is there a way to force a program to use a specific locale, ignoring the 
system-wide setting (something like `export LC_ALL=C` on unix)? Hardcoding the 
error message for all locales is not a viable long term strategy... :/


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D88975/new/

https://reviews.llvm.org/D88975

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D88975: [LLDB] On Windows, fix tests

[Alexandre Ganea via Phabricator via lldb-commits]

aganea created this revision.
aganea added reviewers: amccarth, labath.
Herald added a reviewer: JDevlieghere.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.
aganea requested review of this revision.

This patch fixes a few issues seen when running `ninja check-lldb` in a Release 
build:

- Some binaries couldn't be found (such as lldb-vscode.exe), because `.exe` 
wasn't appended to the file name.
- Many tests used to fail since the OS error messages are not emitted in 
English, because of our installed French locale.
- Also, because of our codepage being Windows-1252, python failed to decode the 
messages.

After this patch, all tests pass with VS2017.
When building with VS2019 or Clang 10, the following tests still //do not 
pass//:

  lldb-api :: python_api/lldbutil/iter/TestLLDBIterator.py
  lldb-shell :: SymbolFile/PDB/enums-layout.test
  lldb-shell :: SymbolFile/PDB/typedefs.test
  lldb-unit :: 
tools/lldb-server/tests/./LLDBServerTests.exe/StandardStartupTest.TestStopReplyContainsThreadPcs

For the first three tests above, this has to do with the debug info not being 
emitted as expected. I'm not sure about the last one.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D88975

Files:
  lldb/packages/Python/lldbsuite/test/dotest.py
  lldb/packages/Python/lldbsuite/test_event/build_exception.py
  lldb/test/API/commands/target/basic/TestTargetCommand.py
  lldb/unittests/Utility/StatusTest.cpp


Index: lldb/unittests/Utility/StatusTest.cpp
===
--- lldb/unittests/Utility/StatusTest.cpp
+++ lldb/unittests/Utility/StatusTest.cpp
@@ -10,7 +10,7 @@
 #include "gtest/gtest.h"
 
 #ifdef _WIN32
-#include 
+#include 
 #endif
 
 using namespace lldb_private;
@@ -71,6 +71,14 @@
   EXPECT_FALSE(success.ToError());
   EXPECT_TRUE(success.Success());
 
+  WCHAR name[128]{};
+  GetLocaleInfoEx(LOCALE_NAME_USER_DEFAULT, LOCALE_SNAME, (LPWSTR),
+  sizeof(name) / sizeof(WCHAR));
+  // Skip the following tests on non-English, non-US, locales because the
+  // formatted messages will be different.
+  if (wcscmp(L"en-US", name) != 0)
+return;
+
   auto s = Status(ERROR_ACCESS_DENIED, ErrorType::eErrorTypeWin32);
   EXPECT_TRUE(s.Fail());
   EXPECT_STREQ("Access is denied. ", s.AsCString());
Index: lldb/test/API/commands/target/basic/TestTargetCommand.py
===
--- lldb/test/API/commands/target/basic/TestTargetCommand.py
+++ lldb/test/API/commands/target/basic/TestTargetCommand.py
@@ -353,7 +353,7 @@
 @no_debug_info_test
 def test_target_create_nonexistent_core_file(self):
 self.expect("target create -c doesntexist", error=True,
-patterns=["Cannot open 'doesntexist'", ": (No such file or 
directory|The system cannot find the file specified)"])
+patterns=["Cannot open 'doesntexist'", ": (No such file or 
directory|The system cannot find the file specified|Le fichier 
sp\udce9cifi\udce9 est introuvable)"])
 
 # Write only files don't seem to be supported on Windows.
 @skipIfWindows
@@ -368,7 +368,7 @@
 @no_debug_info_test
 def test_target_create_nonexistent_sym_file(self):
 self.expect("target create -s doesntexist doesntexisteither", 
error=True,
-patterns=["Cannot open '", ": (No such file or 
directory|The system cannot find the file specified)"])
+patterns=["Cannot open '", ": (No such file or 
directory|The system cannot find the file specified|Le fichier 
sp\udce9cifi\udce9 est introuvable)"])
 
 @skipIfWindows
 @no_debug_info_test
Index: lldb/packages/Python/lldbsuite/test_event/build_exception.py
===
--- lldb/packages/Python/lldbsuite/test_event/build_exception.py
+++ lldb/packages/Python/lldbsuite/test_event/build_exception.py
@@ -12,4 +12,4 @@
 @staticmethod
 def format_build_error(command, command_output):
 return "Error when building test subject.\n\nBuild 
Command:\n{}\n\nBuild Command Output:\n{}".format(
-command, command_output.decode("utf-8"))
+command, command_output.decode("utf-8", errors='ignore'))
Index: lldb/packages/Python/lldbsuite/test/dotest.py
===
--- lldb/packages/Python/lldbsuite/test/dotest.py
+++ lldb/packages/Python/lldbsuite/test/dotest.py
@@ -52,6 +52,9 @@
 """Returns true if fpath is an executable."""
 if fpath == None:
 return False
+if sys.platform == 'win32':
+if not fpath.endswith(".exe"):
+fpath += ".exe"
 return os.path.isfile(fpath) and os.access(fpath, os.X_OK)
 
 


Index: lldb/unittests/Utility/StatusTest.cpp
===
--- lldb/unittests/Utility/StatusTest.cpp
+++ lldb/unittests/Utility/StatusTest.cpp
@@ -10,7