Page MenuHomePhabricator

Teach lit to expand glob expressions
ClosedPublic

Authored by zturner on Feb 25 2017, 1:44 PM.

Details

Summary

This is a proposed solution to D30371. libfuzzer and compiler-rt in general often need to pass lists of files as inputs to the tools, frequently involving wildcard expressions, and this is not easy to do portably given the difference in environment between Windows and non-Windows platforms. Usually it involves some hacky call to xargs, but then when you mix find into the equation you run into trouble with Windows tools.

This patch adds a new lit substitution, %[[expr]] which will be treated as a glob expression and replaced in the input. Note that all glob expressions are expanded before doing any other substitutions, and because of this you can use traditional substitutions as part of a glob expression, which allows you to root your paths %S, %T, etc.

A simple test is added to demonstrate the use of the syntax.

Diff Detail

Repository
rL LLVM

Event Timeline

zturner created this revision.Feb 25 2017, 1:44 PM
mehdi_amini added inline comments.Feb 25 2017, 1:51 PM
llvm/utils/lit/lit/TestRunner.py
727 ↗(On Diff #89799)

Where is tmpDir used?

750 ↗(On Diff #89799)

I'm not totally sure about why a loop is needed here? Can't it be done in a single pass?
Also could we write a pattern that would infinite loop this?

I was expecting first to do ln = doDirectSubstitutions(ln) and then process the "glob" expressions.

zturner added inline comments.Feb 25 2017, 1:56 PM
llvm/utils/lit/lit/TestRunner.py
727 ↗(On Diff #89799)

It's not, I meant to remove this.

750 ↗(On Diff #89799)

If you have multiple glob expressions in a single line, each iteration will only find the first one, but we need to replace all of them.

You're right, I can probably replace all the "direct" substitutions first.

mehdi_amini added inline comments.Feb 25 2017, 2:01 PM
llvm/utils/lit/lit/TestRunner.py
750 ↗(On Diff #89799)

Right but re.findall or re.finditer would allow to iterate over the expression with a single regex query.
I'm not worried about efficiency, just readability, the iterative algorithm you're using makes me think that there is a "feature" to the iterative mode (like one expansion depends on the previous one), which I don't think is intentional here (having a file named %[[.*]] is what I meant with "infinite loop" earlier.

zturner updated this revision to Diff 89801.Feb 25 2017, 2:08 PM

The reason I didn't use findall or finditer is because they only return the portion inside the capture group, so you lose the information about the original replacement string, making the code to actually replace the outer part a little obnoxious.

mehdi_amini accepted this revision.Feb 25 2017, 2:16 PM

I believe you :)

LGTM then.

This revision is now accepted and ready to land.Feb 25 2017, 2:16 PM

I am confused you say glob expansion happens before other substitution. Shouldn't it be in the opposite order if you want %[[%T/*]] to work?

MatzeB added a subscriber: MatzeB.Feb 28 2017, 12:56 PM

In my experience being explicit and mention every thing file instead of globbing is more stable and protects against tests failing because of accidental circumstances like files lying around that are not tracked by version control (anymore?). I haven't looked at the specifics of https://reviews.llvm.org/D30371, but are we really adding a new lit feature to simplify a single test using a bad pattern?

This might look like a single test, but this issue has come up repeatedly in compiler-rt and libfuzzer.

here is the list of all tests that have a run line matching the expression RUN: .*\*.*. Note this doesn't catch all instances of using a wildcard.

D:\src\llvm-mono\llvm\lib\Fuzzer\test\dump_coverage.test(3):RUN: xargs sancov -covered-functions LLVMFuzzer-NullDerefTest* %t_workdir/*.sancov | FileCheck %s --check-prefix=SANCOV
D:\src\llvm-mono\llvm\lib\Fuzzer\test\merge.test(10):RUN: cp %tmp/T0/* %tmp/T1/
D:\src\llvm-mono\llvm\lib\Fuzzer\test\merge.test(33):RUN: rm %tmp/T1/*
D:\src\llvm-mono\llvm\lib\Fuzzer\test\merge.test(34):RUN: cp %tmp/T0/* %tmp/T1/
D:\src\llvm-mono\llvm\lib\Fuzzer\test\merge.test(40):RUN: rm %tmp/T1/*
D:\src\llvm-mono\llvm\lib\Fuzzer\test\merge.test(41):RUN: cp %tmp/T0/* %tmp/T1/
D:\src\llvm-mono\llvm\lib\Fuzzer\test\merge.test(51):RUN: rm -rf  %tmp/T1/* %tmp/T2/*
D:\src\llvm-mono\llvm\lib\Fuzzer\test\minimize_crash.test(6):RUN: rm not_minimal_crash minimized-from-* exact_minimized_path
D:\src\llvm-mono\llvm\lib\Fuzzer\test\minimize_crash.test(8):RUN: echo -n 'abcd*xyz' > not_minimal_crash
D:\src\llvm-mono\llvm\test\tools\llvm-cov\llvm-cov.test(11):RUN: cp %p/Inputs/test* .

I don't think it's necessarily fair to classify these tests as poorly designed. dynamic tools, by design, often need to do things with the file system and the tests need to verify this behavior. LLDB (once it starts moving more towards list tests) will definitely need to be doing the same thing. So I don't believe this to be a one-off addition.

I don't think it's necessarily fair to classify these tests as poorly designed. dynamic tools, by design, often need to do things with the file system and the tests need to verify this behavior. LLDB (once it starts moving more towards list tests) will definitely need to be doing the same thing. So I don't believe this to be a one-off addition.

s/list/lit/

I don't think it's necessarily fair to classify these tests as poorly designed. dynamic tools, by design, often need to do things with the file system and the tests need to verify this behavior. LLDB (once it starts moving more towards list tests) will definitely need to be doing the same thing. So I don't believe this to be a one-off addition.

s/list/lit/

I still consider it bad design if you don't know what your dynamic are doing/are supposed to be doing. Anyway it's a minor thing so go ahead with this if you must, I just hope people will not start to use this too much.

As for LLDB I would expect it to be complicated enough to want/need a custom lit test plugin anyway in which it can handle all the things it requires. I haven't looked at lldb code yet, maybe it is already using one?

I don't think it's necessarily fair to classify these tests as poorly designed. dynamic tools, by design, often need to do things with the file system and the tests need to verify this behavior. LLDB (once it starts moving more towards list tests) will definitely need to be doing the same thing. So I don't believe this to be a one-off addition.

s/list/lit/

I still consider it bad design if you don't know what your dynamic are doing/are supposed to be doing. Anyway it's a minor thing so go ahead with this if you must, I just hope people will not start to use this too much.

As for LLDB I would expect it to be complicated enough to want/need a custom lit test plugin anyway in which it can handle all the things it requires. I haven't looked at lldb code yet, maybe it is already using one?

LLDB doesn't use lit at all, it uses completely different testing infrastructure. It's a longstanding goal to migrate this over to lit.

I don't think it's necessarily fair to classify these tests as poorly designed. dynamic tools, by design, often need to do things with the file system and the tests need to verify this behavior.

That's a reason to look at the filesystem, that's not a reason that explain why a test does not know in advance which file will be generated or need to be looked at. Can you clarify this?

I don't think it's necessarily fair to classify these tests as poorly designed. dynamic tools, by design, often need to do things with the file system and the tests need to verify this behavior.

That's a reason to look at the filesystem, that's not a reason that explain why a test does not know in advance which file will be generated or need to be looked at. Can you clarify this?

One example is when the input to a tool is an executable file name, where on Windows you have to suffix it with .exe. You can see this in my earlier comment dump_coverage.test(3). It writes llvm-FuzzerDerefTest* which catches the filename either with or without a .exe suffix.

Other examples include where a test program can take many inputs, and you put all your input files in a single directory. Sure you could write out each one, but this is higher maintenance and much uglier to look at when reading the test. Every time you add or remove an input, you need to go remember to update the test.

I don't think it's necessarily fair to classify these tests as poorly designed. dynamic tools, by design, often need to do things with the file system and the tests need to verify this behavior.

That's a reason to look at the filesystem, that's not a reason that explain why a test does not know in advance which file will be generated or need to be looked at. Can you clarify this?

One example is when the input to a tool is an executable file name, where on Windows you have to suffix it with .exe. You can see this in my earlier comment dump_coverage.test(3). It writes llvm-FuzzerDerefTest* which catches the filename either with or without a .exe suffix.

This is statically defined, so it could be solved with a lit expansion %exe_suffix

Other examples include where a test program can take many inputs, and you put all your input files in a single directory. Sure you could write out each one, but this is higher maintenance and much uglier to look at when reading the test. Every time you add or remove an input, you need to go remember to update the test.

Can you point at one such test? I'd be interested to see what kind of test does not need to be changed/updated anyway when adding an input.

+aizatsky and +kcc to comment on sanitizer specific stuff.

Other examples include where a test program can take many inputs, and you put all your input files in a single directory. Sure you could write out each one, but this is higher maintenance and much uglier to look at when reading the test. Every time you add or remove an input, you need to go remember to update the test.

The same discussion is going for decades of whether it is fine to use glob constructs in Makefiles (or cmakefiles) to not specify every single source file. I'd say most people think it is a bad idea to use glob there.

Other examples include where a test program can take many inputs, and you put all your input files in a single directory. Sure you could write out each one, but this is higher maintenance and much uglier to look at when reading the test. Every time you add or remove an input, you need to go remember to update the test.

The same discussion is going for decades of whether it is fine to use glob constructs in Makefiles (or cmakefiles) to not specify every single source file. I'd say most people think it is a bad idea to use glob there.

And to give you an example of what can happen if you glob: Today if you run the unittests in llvm lit globs builddir/unittests for executables and runs everything it finds. The result is that when I removed a unittest a while back, the executable would stay on the incremental bots and they happily kept executing an old test and eventually break. This leads to stupid hacks like r261194 (short of fixing it properly and have cmake compile a definite list of unittest executables that need to be run).

I looked at the fuzzer test that triggered this in the first place: the issue is that it spawns multiple processes that dump their coverage file with the PID in the name. The PID can't be predict in advance obviously...

rnk edited edge metadata.Feb 28 2017, 4:10 PM

I don't think we can do this glob expansion in advance. We'd want to do it at the last minute before we do the subprocess.Popen call, to make it behave as consistently as possible with a real shell. If we can't do that, I'd rather keep using find+xargs. It's portable enough for most purposes.

rnk requested changes to this revision.Feb 28 2017, 4:10 PM
This revision now requires changes to proceed.Feb 28 2017, 4:10 PM
aizatsky edited edge metadata.Feb 28 2017, 8:19 PM

PID is one of the reason there's '*' in compiler-rt tests: runtime information is often dumped in <basename>-<pid> files. These files are then fed to analysis tools by using '<basename>-*' shell glob.

If new glob expansion is made before RUN line execution, it won't find any of those files.

zturner updated this revision to Diff 90208.Mar 1 2017, 10:50 AM
zturner edited edge metadata.

Move the globbing closer to the invocation of subprocess.Popen.

rnk added inline comments.Mar 1 2017, 2:57 PM
llvm/utils/lit/lit/TestRunner.py
147 ↗(On Diff #90208)

Why look for double square brackets at all? The lit shell is supposed to be bash compatible, since we use bash on Linux and the internal shell on Windows. Can we just look for '*'?

zturner added inline comments.Mar 1 2017, 3:34 PM
llvm/utils/lit/lit/TestRunner.py
147 ↗(On Diff #90208)

* is not the only character that might indicate the presence of a glob. There's also ?. And character classes in the form of [abc] are also valid glob patterns. We could look for the presence of either of these two characters, but the idea behind the double square brackets was to force the user to acknowledge that they're doing this, as well as to maintain consistency with the existing substitution patterns. So I do believe we need some kind of delimeter that says "Everything inside of here is a glob expression", and from there we just need to decide what is the best choice that allows the full syntax of a glob expression inside, while not interfering with valid filenames.

Now that I've pointed out [abc] though, this raises a concern that %[[Foo[abc]]] would be incorrectly parsed as Foo[abc. I used square braces originally so as not to confuse with %{pathsep}, but since we've already agreed that curly braces are ok and will not interfere with the shell, how about %{{expr}}?

rnk added a comment.Mar 1 2017, 4:06 PM

We had some offline discussion about this. I would really like the lit shell to be as much like bash as possible to match test writers' expectations. As Mehdi mentioned, there are cases where an LLVM tool does not generate output in a predictable location, like clang crash recovery. I'd like to test this with regular shell globbing if possible. It sounds like Zach's going to give that a try.

I think we need to look for unescaped '*' and '?' in the lit shell lexer (ShUtil.py), replace that with an unambiguous escape sequence (%{globstart} and %{globquestion} or something), and then evaluate those immediately before Popen.

zturner updated this revision to Diff 90377.Mar 2 2017, 12:53 PM

Look for glob expressions during tokenization but resolve right before launching the command.

rnk accepted this revision.Mar 3 2017, 10:42 AM

lgtm

llvm/utils/lit/lit/ShCommands.py
38 ↗(On Diff #90377)

This is a nicer design than what I suggested. :)

llvm/utils/lit/lit/ShUtil.py
101 ↗(On Diff #90377)

Seems like a reasonable limitation to me.

This revision is now accepted and ready to land.Mar 3 2017, 10:42 AM

Nice catch @rnk and nice improvements in the end :)

This revision was automatically updated to reflect the committed changes.

Have you tested these changes on a unix system yet? I think we concatenate all the RUN lines together into a single command on unix, so wouldn't that resolve the glob pattern before any of the commands that produce the intermediate files have even run?

I would really like the lit shell to be as much like bash as possible to match test writers' expectations.

+1 (though we should better say posix shell instead of bash)! In any way shell should be a very familiar language and should really be powerful enough for our needs.