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

[Reid Kleckner via llvm-commits]

rnk added inline comments.

================
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)
----------------
filcab wrote:
> 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.
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.

================
Comment at: test/ubsan/TestCases/TypeCheck/misaligned.cpp:3
@@ +2,3 @@
+// which throws away DWARF debug info.
+// REQUIRES: posix
+//
----------------
filcab wrote:
> 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`).
That works for me.


http://reviews.llvm.org/D11960