[LLVMdev] lit: deprecating trailing \ in RUN lines

[Alp Toker]

On 09/12/2013 16:59, Daniel Dunbar wrote:
> Ok, that makes sense. I don't see this as a good enough argument to 
> remove backslash support though.
>
> For problems like the clang_cc1 substitution mistakes, it would be 
> much better to just improve the substitution support so that those 
> cause immediate test failures. I would definitely support a move to 
> make lit's substitution machinery more strict.

Totally -- I have a patch for this. Unfortunately the work to add 
BusyBox support for native Windows testing, ninja test driver and lit 
enhanced diagnostics patches all got squashed into a single commit due 
to a screwup and need to be split up before posting :-/


> For problems like your first search, I can appreciate how wrapped 
> lines make this less trivial, but this isn't enough of a use case to 
> motivate removing the feature in my opinion.
>
> My opinion can be summarized as:
>
> 1. If you are worried about bogus or broken tests, but you have an 
> automatic way to detect the brokenness, then the right way to attack 
> this is to build the check into the test runner so no one ever can 
> write the test to pass again.

Already doing this where possible, like r194919 which forbids direct use 
of the frontend in the driver tests.

This is great because it educates patch contributors rather than fixing 
up after the fact but we're obviously limited in the checks we can 
automate without preventing legitimate testing.

>
> 2. If you are trying to do more fuzzy searches that are going to no be 
> fully automatic, the individual developer can deal with the existence 
> of wrapped lines. Or we could add a way to have lit run a regexp over 
> all of the (collapsed) test script lines if this is a really important 
> use case for you. Either way, its not a good enough argument to remove 
> a useful feature.

This hits on the problem precisely.

For any given common mistake 'hypothesis', there are a large number of 
false positives which legitimately test incorrect usage. So the kind of 
validations I'm running are inherently fuzzy.

You're right that it's trivial to /detect/ these by putting a regex just 
after the directives are combined.

The crux of the problem is that detection isn't sufficient to achieve 
real fixes on this scale.

There are 26,631 test runs between LLVM and clang. The only practical 
workflow I've found to fix these is to do in-place bulk edit using an 
assortment of command-line tools, then running and review the changes in 
context using word diff. Before this, we had no workflow so it's 
something of a step forward :-)

Some background: I've been working my way down a list of potential 
problems and committed ~150 fixes or cleanups to clang tests in the last 
few weeks, several of which were hiding crasher or invalid codegen 
regressions.

To put that in context, I only reached the letter 'a' in that list of 
potential issues a few days ago -- it's clear that there are more tests 
in a state of disrepair than anyone imagined and a lot of tests we 
though are fine just aren't working.

I like my 80 columns as much as the next guy, but in this case they're 
blocking important work that's already detected hundreds of issues in 
limited trial runs.

So in this light I think we need to take a close look at the 
cost/benefit of continuing to support split RUN lines.

Alp.


>
>  - Daniel
>
>
>
> On Sun, Dec 8, 2013 at 11:47 AM, Alp Toker <alp at nuanti.com 
> <mailto:alp at nuanti.com>> wrote:
>
>
>     On 08/12/2013 16:43, Daniel Dunbar wrote:
>>
>>         With one-to-one mapping, it becomes possible to use simple
>>         tools like grep to validate common mistakes like %clang /
>>         %clang_cc1 mixups, a missing -o flag and so on.
>>
>>         Right now there's no obvious way to do those checks and we've
>>         ended up without an easy way to lint for broken tests as a
>>         result. Each broken test has a high cost so we need to
>>         continually look at ways to improve the situation.
>>
>>
>>     Can you elaborate on exactly what kind of checks you want to do?
>
>     Basically, it's for asking questions about what's currently tested..
>
>         e.g. /Find me all tests for c++11 exceptions/:
>
>         |git grep 'RUN:.*c++11' -- test/ | grep exceptions|
>
>
>     .. experimenting with new features or refactoring existing tests..
>
>         e.g. /How many tests could we add -fsyntax-only to?/
>
>         |giteach perl -pi -e 's/((RUN:.*%clang_cc1)(.*))$/$2
>         -fsyntax-only$3/g unless
>         m/(-fsyntax-only|-analyze|-arcmt-check|-arcmt-migrate|-o|-emit-llvm|-E|-ast-print|-ast-dump|[|>]|\\$)/'|
>
>         Answer, 405. Then git diff and page through to see if any are
>         actionable.
>
>
>     and fixing broken tests..
>
>         eg. /Let's find and fix %clang_cc1 substitution mistakes../
>
>         giteach perl -pi -e 's/(RUN:.*%clang_cc1)[^\s]+/$1/g'
>
>
>     I committed the results of this run just a few minutes ago in
>     r196729 and r196730.
>
>     One was a test that had been disabled since 2007.
>
>     Losing 80 columns is a small price to pay if it'll help make tests
>     easier to write, understand and validate in my opinion.
>
>     (The two editors I use, vim and XCode, both do a good job of
>     virtual line wrapping so it turns out long RUN lines aren't that
>     much of a big deal in practice. But I can see why it may be a
>     bigger problem with editors that don't support wrapping. Is this
>     still a problem in 2013?)
>
>
>     Alp.
>
>     -- 
>     http://www.nuanti.com
>     the browser experts
>
>

-- 
http://www.nuanti.com
the browser experts