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

[Filipe Cabecinhas via llvm-commits]

filcab added a comment.

In http://reviews.llvm.org/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.


================
Comment at: test/lit.common.cfg:15
@@ -17,1 +14,3 @@
+# 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)
----------------
rnk wrote:
> filcab wrote:
> > 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`?
> This is inconsistent with LLVM and Clang, and is exactly what I want to disable.
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.

================
Comment at: test/ubsan/TestCases/TypeCheck/misaligned.cpp:3
@@ +2,3 @@
+// which throws away DWARF debug info.
+// REQUIRES: posix
+//
----------------
rnk wrote:
> filcab wrote:
> > From the comment above, it seems this should be an `XFAIL` when we have *both* `host-windows` and "target Windows", no?
> 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.
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`).


http://reviews.llvm.org/D11960