Page MenuHomePhabricator

[libFuzzer] cmd prompt doesn't expand wildcard.
ClosedPublic

Authored by mpividori on Jan 31 2017, 10:06 PM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

mpividori created this revision.Jan 31 2017, 10:06 PM
kcc edited edge metadata.Feb 1 2017, 8:42 AM

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

@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 added a comment.Feb 1 2017, 9:18 AM

@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.

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 added a comment.Feb 1 2017, 9:31 AM

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

zturner edited edge metadata.Feb 1 2017, 10:24 AM

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 added a comment.Feb 1 2017, 10:33 AM

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

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.

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

kcc added a comment.Feb 1 2017, 10:45 AM

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.

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 added a comment.Feb 1 2017, 10:54 AM

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

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 added a comment.Feb 1 2017, 11:01 AM

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

It looks like this is the only one.

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).

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

kcc accepted this revision.Feb 1 2017, 11:47 AM

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.

This revision is now accepted and ready to land.Feb 1 2017, 11:47 AM
This revision was automatically updated to reflect the committed changes.