[libFuzzer] cmd prompt doesn't expand wildcard.

[mpividori]

Summary

The commands should expand the wildcards in Windows, the cmd prompt doesn't.
Because of that sancov was not finding the needed file. To deal with this, I use ls and xargs from gnu win utils.

[kcc]

this sucks. can we require that the tests run in a proper shell instead?

[mpividori]

@kcc as far as I understand, cmd prompt is the only shell in all Windows systems by default. So we can't use a different one.
Instead, I think sancov implementation should be updated to expand wildcards in its parameters.

[kcc]

In D29374#663301, @mpividori wrote:

@kcc as far as I understand, cmd prompt is the only shell in all Windows systems by default. So we can't use a different one.

Then we probably shouldn't use any.
Some of the tests in compiler-rt have 'REQUIRES: shell'
Will the same thing help here (either to require a shell on windows, or just to disable the test)?

Using xargs complicates the test.
The next time someone on linux will add more shell-ism to the tests and we will be constantly chasing failures on windows.

Instead, I think sancov implementation should be updated to expand wildcards in its parameters.

I hope you are kidding.

[mpividori]

First of all. I am working. Unless I make it explicit, I am never kidding.
Second, all the gnu tool programs, expand wildcard, so the only problem is the sancov program.
Disabling the test is the easy solution for me.
Let me know what you want and I will do that.

[kcc]

I think we need to add
REQUIRES: shell
(and make sure it helps)

[zturner]

The meaning of REQUIRES: shell is not intuitive, but it does not mean "I have the ability to run shell commands". See this thread for more details: http://lists.llvm.org/pipermail/llvm-dev/2016-August/103713.html

TL;DR is that even if REQUIRES: shell would be false (as it would on Windows), this test is still guaranteed to work on Windows. Using ls and xargs does not mean we need to add REQUIRES: shell. GnuWin32 (which provides windows implementations of commands such as ls and xargs) is guaranteed to be present on Windows because it is a required dependency.

Another option I can think of is to build wildcard expansion functionality into lit, but this is probably beyond the scope of this patch and would likely require lots of agreement and buy-in from various people in the community.

[kcc]

btw, it would be totally fine to mark any test that fails on windows now as such (e.g. REQUIRES: not-windows).
This would allow to set up a bot to watch for regressions

[zturner]

Does that work? I know you can say REQUIRES: windows but I didn't know you can say REQUIRES: not-windows. Can you confirm that this works? In any case, for the purposes of this patch, are you still against the solution Marcos proposed here? It would be a shame to disable the test even though it actually works.

[mpividori]

REQUIRES: not-windows seems to work.
UNSUPPORTED: windows doesn't.

[kcc]

I've just invented 'REQUIRES: not-windows', I don't know if it works out of the box, but it's easy to implement.
Yes, I still don't like the change and even more I dislike the prospect to have more such changes.
Also, there is no way to prevent us from introducing more windows-incompatible tests.

[zturner]

That's true, but it's also true that more windows incompatible tests could be introduced for many other reasons. But that's what the bot is for (assuming we can get it turned on in time). It should be easy to see that the bot is failing on Windows and then choose the most appropriate fix. If the only hammer you have is a big one, then that's the one you're always going to use. It would be a shame if the default behavior of everyone who doesn't work on Windows when they see a bot fail is to just go and add REQUIRES: not-windows without at least seeing if there's an easy fix (such as this one). I'm worried that if we have REQUIRES: not-windows then it will just become commonplace to add it to every test going forward instead of taking a few extra minutes to find a way to write the test so that it works everywhere.

[kcc]

but isn't there a way to run this test in a proper bash on windows?
This will solve all problems like this one.

[zturner]

In D29374#663547, @kcc wrote:

but isn't there a way to run this test in a proper bash on windows?
This will solve all problems like this one.

Sadly Windows doesn't have bash :(

There are ways to get bash on Windows, but it's only in very new versions of Windows (which means not the bots), it's off by default and hard to turn on, and anyway LLVM's system requirements don't require it.

[kcc]

do you have an estimate on how more tests will need to be fixed this way?

[zturner]

It looks like this is the only one.

[mpividori]

Hi,
Which option should I use to disable a test on Windows?

• XFAIL: windows doesn't work.
• UNSUPPORTED: windows doesn't work.
• REQUIRES: not-windows is not implemented (this disables the tests on all systems, because no one matches not-windows).

[zturner]

You may need to modify lib/Fuzzer/lit.cfg and add the appropriate thing to the available_features array.

[kcc]

LGTM
this is all a very sad story, and I still don't like this change,
but thanks to your explanations I agree we have no better short term solution.

llvm/trunk/lib/Fuzzer/test/dump_coverage.test

 RUN: DIR=%t_workdir
 RUN: BUILD_DIR=$(pwd)
 RUN: rm -rf $DIR && mkdir -p $DIR && cd $DIR
 RUN: not $BUILD_DIR/LLVMFuzzer-NullDerefTest -dump_coverage=1 2>&1 | FileCheck %s
-RUN: sancov -covered-functions *.sancov $BUILD_DIR/LLVMFuzzer-NullDerefTest | FileCheck %s --check-prefix=SANCOV
+RUN: ls *.sancov | xargs sancov -covered-functions $BUILD_DIR/LLVMFuzzer-NullDerefTest* | FileCheck %s --check-prefix=SANCOV
 RUN: $BUILD_DIR/LLVMFuzzer-DSOTest -dump_coverage=1 -runs=0 2>&1 | FileCheck %s --check-prefix=DSO
 RUN: not $BUILD_DIR/LLVMFuzzer-NullDerefTest -dump_coverage=0 2>&1 | FileCheck %s --check-prefix=NOCOV
 RUN: rm -rf $DIR
 
 
 CHECK: SanitizerCoverage: ./LLVMFuzzer-NullDerefTest.{{.*}}.sancov {{.*}} PCs written
 SANCOV: LLVMFuzzerTestOneInput
 
 DSO: SanitizerCoverage: ./LLVMFuzzer-DSOTest.{{.*}}.sancov {{.*}} PCs written
 DSO-DAG: SanitizerCoverage: {{.*}}LLVMFuzzer-DSO1.{{.*}}.sancov {{.*}} PCs written
 DSO-DAG: SanitizerCoverage: {{.*}}LLVMFuzzer-DSO2.{{.*}}.sancov {{.*}} PCs written
 
 NOCOV-NOT: SanitizerCoverage: {{.*}} PCs written