[windows] Always use the lit shell on Windows, even if bash is present

[rnk]

Summary

This is consistent with LLVM and Clang. The lit shell isn't a complete
bash implementation, but its behavior is more easily reproducible. This
fixes some ubsan test failures.

One test requires a shell currently, so I added "REQUIRES: shell", and the other doesn't work on Windows because it prints a stack trace and uses a linker that doesn't support DWARF. We can fix it eventually through other means.

[filcab]

I really like the idea, but I think we could make a better distinction between hosting on Windows and targetting Windows.
Why don't we use bash.exe if we find it? Are there any problems with using a bash implementation on Windows?
How about letting the user provide a bash.exe path, if they want to have an external shell to execute these tests?

test/lit.common.cfg

15 ↗

(On Diff #31862)

I would prefer if we could test the *target*, instead of the *host*.
I can always work around it by adding to the condition, but still. This looks fragile to me.
If we have a bash-like shell in the path, should we try it, and set execute_external?

118 ↗

(On Diff #31862)

Please name them host-windows and host-posix, as these features are about the host.
If you're running Android tests on a Windows host, for example, you won't run tests which REQUIRES: host-posix. But you should be able to run tests requiring posix functions, since they will run on a posix target.

test/ubsan/TestCases/TypeCheck/misaligned.cpp

2 ↗

(On Diff #31862)

Shouldn't the SourceLocs always be there? Or are there cases when they aren't? If so, how easy/hard is it to fix clang instead of "hiding" the problem?

3 ↗

(On Diff #31862)

From the comment above, it seems this should be an XFAIL when we have *both* host-windows and "target Windows", no?

[filcab]

misaligned.cpp:3: "From the comment above, it seems this should be an XFAIL when we have *both* host-windows and "target Windows", no?"

What I meant is: you have both ON. Otherwise, you're hosting on Windows, targeting another platform, which I guess we can assume (for now?) supports DWARF. Or you're running on Linux (for example) and targetting Windows, in which case you're not using MSVC linker, and we can (can we?) assume the linker supports DWARF.

[yaron.keren]

The bash shell on Windows from MSYS expands anyhting that looks like a relative path, such as /m. It has workaround for switches but not for environment vas so tests using 'env' were failing without any way to fix. In addition, it did not pass environment variable TZ since it does not know how ot map between Unix and Windows TZ. This also broke tests.

Finally, process creation time on Windows is much higher than on Linux so any command that's implemented internally in the lit shell makes the tests go faster. Note that not all bells and whistles of the commands are really used in the tests.

[filcab]

In D11960#222184, @yaron.keren wrote:

The bash shell on Windows from MSYS expands anyhting that looks like a relative path, such as /m. It has workaround for switches but not for environment vas so tests using 'env' were failing without any way to fix.

Can you provide an example of that issue, please? I'm running all my tests (only ASan and UBSan, for now) on a Windows host (not a Windows target), and I don't seem to see this. Can the issue be solved in another way? Quoting parameters that start with /X, for example?

I'm using the bash.exe that comes with git:

$ bash --version
GNU bash, version 3.1.20(4)-release (i686-pc-msys)
Copyright (C) 2005 Free Software Foundation, Inc.

In addition, it did not pass environment variable TZ since it does not know how ot map between Unix and Windows TZ. This also broke tests.

Which tests? Can this be solved with a script for %run? A substitution in lit? It seems like a bug somewhere else. Why is TZ not being passed while other env vars are?

Finally, process creation time on Windows is much higher than on Linux so any command that's implemented internally in the lit shell makes the tests go faster. Note that not all bells and whistles of the commands are really used in the tests.

This tells me that maybe a toggle would be good, to force an internal lit shell. But I don't see the benefit in not running all the tests we can...

[rnk]

In D11960#222206, @filcab wrote:

In D11960#222184, @yaron.keren wrote:

The bash shell on Windows from MSYS expands anyhting that looks like a relative path, such as /m. It has workaround for switches but not for environment vas so tests using 'env' were failing without any way to fix.

Can you provide an example of that issue, please? I'm running all my tests (only ASan and UBSan, for now) on a Windows host (not a Windows target), and I don't seem to see this. Can the issue be solved in another way? Quoting parameters that start with /X, for example?

I'm using the bash.exe that comes with git:

$ bash --version
GNU bash, version 3.1.20(4)-release (i686-pc-msys)
Copyright (C) 2005 Free Software Foundation, Inc.

I have no idea what git's msys bash does. The normal msys bash will turn all command line arguments starting with / into Windows paths when invoking Windows executables that don't link the mingw crt. Cygwin's bash is also buggy and is generally really slow at process creation due to fork emulation. Personally, I think we should move towards using the internal lit shell everywhere when possible. It makes behavior a lot more predictable.

Anyway, whether we use an external bash is definitely a choice of the host. It saves us from debugging issues around incompatible bash implementations.

In addition, it did not pass environment variable TZ since it does not know how ot map between Unix and Windows TZ. This also broke tests.

Which tests? Can this be solved with a script for %run? A substitution in lit? It seems like a bug somewhere else. Why is TZ not being passed while other env vars are?

Some MSys bug. There is no workaround.

Finally, process creation time on Windows is much higher than on Linux so any command that's implemented internally in the lit shell makes the tests go faster. Note that not all bells and whistles of the commands are really used in the tests.

This tells me that maybe a toggle would be good, to force an internal lit shell. But I don't see the benefit in not running all the tests we can...

In LLVM and clang we have this:

use_lit_shell = os.environ.get("LIT_USE_INTERNAL_SHELL")

It overrides the normal decision. It didn't seem worth duplicating here. If we want it, we should roll this decision up into some helper in lit.util that we can share, like lit.util.usePlatformSdkOnDarwin.

[rnk]

test/lit.common.cfg

15 ↗

(On Diff #31862)

This is inconsistent with LLVM and Clang, and is exactly what I want to disable.

118 ↗

(On Diff #31862)

Sure, that seems reasonable.

test/ubsan/TestCases/TypeCheck/misaligned.cpp

3 ↗

(On Diff #31862)

What this test really requires is symbols. We could make a "symbols" feature generally available, and then remove it when the MSVC linker is in use. Everything else generally works.

[filcab]

In D11960#222222, @rnk wrote:

Some MSys bug. There is no workaround.

:-(
Fair enough. But what tests fail on Windows, with this problem? I don't see any test setting TZ to anything.
As for the arguments: That is a problem if we have lots of arguments like "/something". I hate this MSYS behavior :(

In LLVM and clang we have this:

use_lit_shell = os.environ.get("LIT_USE_INTERNAL_SHELL")

It overrides the normal decision. It didn't seem worth duplicating here. If we want it, we should roll this decision up into some helper in lit.util that we can share, like lit.util.usePlatformSdkOnDarwin.

That would probably be good, yes.

test/lit.common.cfg

15 ↗

(On Diff #31862)

I'm assuming the "this is inconsistent" is referring to "target != host".

I'm perfectly ok with having the compiler-rt test suite be different. It *is* very different. No clang or llvm test runs on anything other than the host. All compiler-rt tests run on the target. We can't have the same rules here.

For my case, the characteristics between host and target are very different, but this kind of thing can bite anyone who works on having the sanitizers in external devices (like the PS4, Android, iOS, ...), since tests based on the host might be totally wrong.

For my case, I have to compile and run tests on Windows, right now, and forcing execute_external to false will make me not be able to run some tests which I should be able to run on my bash. This will also happen on the Windows->Windows case where bash is available and the tests don't need any /somehting command line option.

As I said, changing it to test for "are we targeting platform X" is easy enough, but I would really like to have a good separation between host and target on the compiler-rt tests.

test/ubsan/TestCases/TypeCheck/misaligned.cpp

3 ↗

(On Diff #31862)

I'm ok with that. But every UBSan check emits a SourceLocation, since my commit to add them to float_cast_overflow. It seems the "misaligned" check might emit invalid SourceLocs, forcing us to symbolize, though. If it's easy enough to change clang to emit those, then we should probably do it, and not mark the test as REQUIRES, but as XFAIL, since it should be fixed eventually.

The symbols feature seems like a good approach (for either the REQUIRES or the XFAIL).

[rnk]

test/lit.common.cfg

15 ↗

(On Diff #31862)

I think we should just try not to depend on too many complex bash-isms in our test suites. At some point we'll need to implement environment variables in the lit shell, which will solve a large class of issues.

Anyway, I don't see what the issue is here. Whether we have a shell has always been a property of the host that is running the tests. We support remote test execution on targets through the %run lit expansion. I don't imagine we will ever need to test if the target does or does not have bash. Therefore, I don't see the need for host-shell and target-shell features. The "shell" feature always implies that the system running the tests (host, right?) has a shell.

test/ubsan/TestCases/TypeCheck/misaligned.cpp

3 ↗

(On Diff #31862)

That works for me.

[filcab]

test/lit.common.cfg

15 ↗

(On Diff #31862)

Yes, not depending on bash-isms is always a good thing. And yes, we'll eventually have to implement env var support in lit. Sorry for making a mess of my comment :-(
And yes, "shell" is always a feature of hosts (at least for now ;-) ).

[rnk]

• Switch to XFAIL: win32 instead of REQUIRES: posix

[yaron.keren]

Which tests? Can this be solved with a script for %run? A substitution in lit? It seems like a bug somewhere else. Why is TZ not being passed while other env vars are?

https://github.com/Alexpux/MSYS2-packages/issues/294

[samsonov]

LGTM

compiler-rt/trunk/test/asan/lit.cfg

 config.name = 'AddressSanitizer' + config.name_suffix
 
 # Platform-specific default ASAN_OPTIONS for lit tests.
 if config.host_os == 'Darwin':
   # On Darwin, we default to `abort_on_error=1`, which would make tests run
   # much slower. Let's override this and run lit tests with 'abort_on_error=0'.
   config.environment['ASAN_OPTIONS'] = 'abort_on_error=0'
 
-# testFormat: The test format to use to interpret tests.
-external_bash = (not sys.platform in ['win32'])
-config.test_format = lit.formats.ShTest(external_bash)
-
 # Setup source root.
 config.test_source_root = os.path.dirname(__file__)
 
 # There is no libdl on FreeBSD.
 if config.host_os != 'FreeBSD':
   libdl_flag = "-ldl"
 else:
   libdl_flag = ""

compiler-rt/trunk/test/lit.common.cfg

 # -*- Python -*-
 
 # Configuration file for 'lit' test runner.
 # This file contains common rules for various compiler-rt testsuites.
 # It is mostly copied from lit.cfg used by Clang.
 import os
 import platform
 import re
 import subprocess
 
 import lit.formats
 import lit.util
 
-# Setup test format
-execute_external = (platform.system() != 'Windows'
-                    or lit_config.getBashPath() not in [None, ""])
+# Setup test format. Use bash on Unix and the lit shell on Windows.
+execute_external = (not sys.platform in ['win32'])
 config.test_format = lit.formats.ShTest(execute_external)
+if execute_external:
+  config.available_features.add('shell')
 
 # Setup clang binary.
 compiler_path = getattr(config, 'clang', None)
 if (not compiler_path) or (not os.path.exists(compiler_path)):
   lit_config.fatal("Can't find compiler on path %r" % compiler_path)
 
 compiler_id = getattr(config, 'compiler_id', None)
 if compiler_id == "Clang":

compiler-rt/trunk/test/ubsan/TestCases/Misc/coverage-levels.cc

 // Test various levels of coverage
 //
+// FIXME: Port the environment variable logic below for the lit shell.
+// REQUIRES: shell
+//
 // RUN: mkdir -p %T/coverage-levels
 // RUN: OPT=coverage=1:verbosity=1:coverage_dir=%T/coverage-levels
 // RUN: %clangxx -fsanitize=shift                        -DGOOD_SHIFT=1 -O1 -fsanitize-coverage=func  %s -o %t
 // RUN: UBSAN_OPTIONS=$OPT %run %t 2>&1 | FileCheck %s --check-prefix=CHECK1 --check-prefix=CHECK_NOWARN
 // RUN: %clangxx -fsanitize=undefined                    -DGOOD_SHIFT=1 -O1 -fsanitize-coverage=func  %s -o %t
 // RUN: UBSAN_OPTIONS=$OPT %run %t 2>&1 | FileCheck %s --check-prefix=CHECK1 --check-prefix=CHECK_NOWARN
 
 // RUN: %clangxx -fsanitize=shift -O1 -fsanitize-coverage=func  %s -o %t

compiler-rt/trunk/test/ubsan/TestCases/TypeCheck/misaligned.cpp

+// FIXME: This test currently fails on Windows because we use the MSVC linker,
+// which throws away DWARF debug info.
+// XFAIL: win32
+//
 // RUN: %clangxx -fsanitize=alignment -g %s -O3 -o %t
 // RUN: %run %t l0 && %run %t s0 && %run %t r0 && %run %t m0 && %run %t f0 && %run %t n0 && %run %t u0
 // RUN: %run %t l1 2>&1 | FileCheck %s --check-prefix=CHECK-LOAD --strict-whitespace
 // RUN: %run %t s1 2>&1 | FileCheck %s --check-prefix=CHECK-STORE
 // RUN: %run %t r1 2>&1 | FileCheck %s --check-prefix=CHECK-REFERENCE
 // RUN: %run %t m1 2>&1 | FileCheck %s --check-prefix=CHECK-MEMBER
 // RUN: %run %t f1 2>&1 | FileCheck %s --check-prefix=CHECK-MEMFUN
 // RUN: %run %t n1 2>&1 | FileCheck %s --check-prefix=CHECK-NEW