llvm dev - Dec 2016 - [FileCheck] Fix --strict-whitespace --match-full-lines

Hi,

this patch fixes a problem with leading/trailing whitespace matching for 
FileCheck --strict-whitespace --match-full-lines.

Consider a text file:
...
$ cat DUMP
bla1
bla2
  bla3
bla4
  bla5
...

with some leading and trailing spaces, made more visible like this:
...
$ sed 's/ /_/g' DUMP
bla1
bla2
_bla3
bla4_
_bla5_
...

and a FileCheck file CHECK to match DUMP:
...
$ cat CHECK
// CHECK-LABEL:bla1
// CHECK-NEXT:bla2
// CHECK-NEXT: bla3
// CHECK-NEXT:bla4
// CHECK-NEXT: bla5
...

with whitespace made more visible like this:
...
$ sed 's/ /_/g' CHECK
//_CHECK-LABEL:bla1
//_CHECK-NEXT:bla2
//_CHECK-NEXT:_bla3
//_CHECK-NEXT:bla4_
//_CHECK-NEXT:_bla5_
...

When trying the match, it fails:
...
$ cat DUMP | FileCheck CHECK --strict-whitespace --match-full-lines
CHECK:3:16: error: expected string not found in input
// CHECK-NEXT: bla3
                ^
<stdin>:3:2: note: scanning from here
  bla3
  ^
...

I expect the match to succeed, because I expect leading and trailing 
whitespace _not_ to be ignored, because the documentation states:
...
  --match-full-lines

     By default, FileCheck allows matches of anywhere on a line. This 
option will require all positive matches to cover an entire line. 
Leading and trailing whitespace is ignored, unless --strict-whitespace 
is also specified.
...

After adding some debug code to FileCheck (which I proposed here on 
llvm-dev ml as '[FileCheck] Add --verbose'), we can see where things go 
wrong:
...
$ cat DUMP | /home/vries/gt/build/./bin/FileCheck CHECK 
--strict-whitespace --match-full-lines --verbose
CHECK:3:16: note: RegEx string match: '^bla3$'
// CHECK-NEXT: bla3
...

The resulting regexp string is '^bla3$' instead of '^ bla3$'.

The patch fixes this, and makes the behavior match the documentation.

I ran the llvm/test/FileCheck tests, no regressions.

Any comments? OK for trunk?

Thanks,
- Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-FileCheck-Fix-strict-whitespace-match-full-lines.patch
Type: text/x-patch
Size: 2106 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20161214/de6c8180/attachment.bin>

+jyknight, who added --match-full-lines


On 12/14/16 5:37 AM, Tom de Vries wrote:
> Hi,
>
> this patch fixes a problem with leading/trailing whitespace matching 
> for FileCheck --strict-whitespace --match-full-lines.
>
> The resulting regexp string is '^bla3$' instead of '^
bla3$'.
>
> The patch fixes this, and makes the behavior match the documentation.
>
> I ran the llvm/test/FileCheck tests, no regressions.
Should probably run the entire llvm testsuite, if not clang's too. I 
doubt FileCheck's own tests have good coverage of the tool itself.


Jon
>
> Any comments? OK for trunk?
>
> Thanks,
> - Tom
-- 
Jon Roelofs
jonathan at codesourcery.com
CodeSourcery / Mentor Embedded

Please send patches to llvm-commits not llvm-dev.

Writing FileCheck tests has pitfalls. A test along these lines:

bla0
CHECK:bla1

will actually pass, because the CHECK pattern is also part of the input
so it will readily match itself.  You want the CHECK lines not to match
themselves, which you can easily do by introducing {{}} into the (middle
of the) pattern.  That is:

bla0
CHECK:{{bla1}}

will still pass (incorrectly), while

bla0
CHECK:bla{{1}}

will fail (correctly).  A correct FileCheck test would thus be

bla1
CHECK:bla{{1}}


(I didn't include this in my recent FileCheck Follies lightning talk,
because testing FileCheck itself is kind of an obscure corner of the
LLVM world and I was already bumping up against the 5-minute time limit.)
--paulr

> -----Original Message-----
> From: llvm-dev [mailto:llvm-dev-bounces at lists.llvm.org] On Behalf Of Tom
> de Vries via llvm-dev
> Sent: Wednesday, December 14, 2016 4:38 AM
> To: llvm-dev at lists.llvm.org
> Cc: Jonathan Roelofs
> Subject: [llvm-dev] [FileCheck] Fix --strict-whitespace --match-full-lines
> 
> Hi,
> 
> this patch fixes a problem with leading/trailing whitespace matching for
> FileCheck --strict-whitespace --match-full-lines.
> 
> Consider a text file:
> ...
> $ cat DUMP
> bla1
> bla2
>   bla3
> bla4
>   bla5
> ...
> 
> with some leading and trailing spaces, made more visible like this:
> ...
> $ sed 's/ /_/g' DUMP
> bla1
> bla2
> _bla3
> bla4_
> _bla5_
> ...
> 
> and a FileCheck file CHECK to match DUMP:
> ...
> $ cat CHECK
> // CHECK-LABEL:bla1
> // CHECK-NEXT:bla2
> // CHECK-NEXT: bla3
> // CHECK-NEXT:bla4
> // CHECK-NEXT: bla5
> ...
> 
> with whitespace made more visible like this:
> ...
> $ sed 's/ /_/g' CHECK
> //_CHECK-LABEL:bla1
> //_CHECK-NEXT:bla2
> //_CHECK-NEXT:_bla3
> //_CHECK-NEXT:bla4_
> //_CHECK-NEXT:_bla5_
> ...
> 
> When trying the match, it fails:
> ...
> $ cat DUMP | FileCheck CHECK --strict-whitespace --match-full-lines
> CHECK:3:16: error: expected string not found in input
> // CHECK-NEXT: bla3
>                 ^
> <stdin>:3:2: note: scanning from here
>   bla3
>   ^
> ...
> 
> I expect the match to succeed, because I expect leading and trailing
> whitespace _not_ to be ignored, because the documentation states:
> ...
>   --match-full-lines
> 
>      By default, FileCheck allows matches of anywhere on a line. This
> option will require all positive matches to cover an entire line.
> Leading and trailing whitespace is ignored, unless --strict-whitespace
> is also specified.
> ...
> 
> After adding some debug code to FileCheck (which I proposed here on
> llvm-dev ml as '[FileCheck] Add --verbose'), we can see where
things go
> wrong:
> ...
> $ cat DUMP | /home/vries/gt/build/./bin/FileCheck CHECK
> --strict-whitespace --match-full-lines --verbose
> CHECK:3:16: note: RegEx string match: '^bla3$'
> // CHECK-NEXT: bla3
> ...
> 
> The resulting regexp string is '^bla3$' instead of '^
bla3$'.
> 
> The patch fixes this, and makes the behavior match the documentation.
> 
> I ran the llvm/test/FileCheck tests, no regressions.
> 
> Any comments? OK for trunk?
> 
> Thanks,
> - Tom

> -----Original Message-----
> From: llvm-dev [mailto:llvm-dev-bounces at lists.llvm.org] On Behalf Of
> Jonathan Roelofs via llvm-dev
> Sent: Wednesday, December 14, 2016 8:32 AM
> To: Tom de Vries; llvm-dev at lists.llvm.org
> Subject: Re: [llvm-dev] [FileCheck] Fix --strict-whitespace --match-full-
> lines
> 
> +jyknight, who added --match-full-lines
> 
> 
> On 12/14/16 5:37 AM, Tom de Vries wrote:
> > Hi,
> >
> > this patch fixes a problem with leading/trailing whitespace matching
> > for FileCheck --strict-whitespace --match-full-lines.
> >
> > The resulting regexp string is '^bla3$' instead of '^
bla3$'.
> >
> > The patch fixes this, and makes the behavior match the documentation.
> >
> > I ran the llvm/test/FileCheck tests, no regressions.
> 
> Should probably run the entire llvm testsuite, if not clang's too. I
> doubt FileCheck's own tests have good coverage of the tool itself.
They are smoke tests, like most of clang/llvm lit tests.
Running the full clang/llvm lit suites with the modified FileCheck
seems like a pretty reasonable exercise.
--paulr
> 
> 
> Jon
> 
> >
> > Any comments? OK for trunk?
> >
> > Thanks,
> > - Tom
> 
> --
> Jon Roelofs
> jonathan at codesourcery.com
> CodeSourcery / Mentor Embedded
> 
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

On 14/12/16 18:48, Robinson, Paul wrote:
> Please send patches to llvm-commits not llvm-dev.
>
> Writing FileCheck tests has pitfalls. A test along these lines:
>
> bla0
> CHECK:bla1
>
> will actually pass, because the CHECK pattern is also part of the input
> so it will readily match itself.  You want the CHECK lines not to match
> themselves, which you can easily do by introducing {{}} into the (middle
> of the) pattern.  That is:
>
> bla0
> CHECK:{{bla1}}
>
> will still pass (incorrectly), while
>
> bla0
> CHECK:bla{{1}}
>
> will fail (correctly).  A correct FileCheck test would thus be
>
> bla1
> CHECK:bla{{1}}
>
>
I see. ISTM though that the FileCheck tests are written in a style that 
is not the typical usage of FileCheck.

As far as I've seen sofar, a typical FileCheck usage looks like:
...
<comment-token> RUN: ignore-comments-and-translate %s \
<comment-token> RUN: | FileCheck %s

... input for translation ...

<comment-token> CHECK: ... check translation ...
<comment-token> CHECK: ... check translation ...
...

So if you run the FileCheck tests in a similar way, like this:
...
; RUN: sed 's/^;.*$//' %s \
; RUN: | FileCheck %s
...
you don't have to obfuscate the check patterns to work around this problem.

I've updated the test-case accordingly.

[ If we want to use this more often, perhaps something like this would 
be good:
...
diff --git a/test/lit.cfg b/test/lit.cfg
index d8728b6..80580c8 100644
--- a/test/lit.cfg
+++ b/test/lit.cfg
@@ -202,6 +202,9 @@
    lli += ' -mtriple='+config.host_triple+'-elf'
  config.substitutions.append( ('%lli', lli ) )

+config.substitutions.append( ('%filter-txt-comments',
+                              'sed \'s/^;.*$//\'') )
+
  # Similarly, have a macro to use llc with DWARF even when the host is 
win32.
  llc_dwarf = 'llc'
  if re.search(r'win32', config.target_triple):
...
]
> (I didn't include this in my recent FileCheck Follies lightning talk,
+1 viewcount.
> because testing FileCheck itself is kind of an obscure corner of the
> LLVM world and I was already bumping up against the 5-minute time limit.)
> --paulr
>
>
>> -----Original Message-----
>> From: llvm-dev [mailto:llvm-dev-bounces at lists.llvm.org] On Behalf Of
Tom
>> de Vries via llvm-dev
>> Sent: Wednesday, December 14, 2016 4:38 AM
>> To: llvm-dev at lists.llvm.org
>> Cc: Jonathan Roelofs
>> Subject: [llvm-dev] [FileCheck] Fix --strict-whitespace
--match-full-lines
>>
>> Hi,
>>
>> this patch fixes a problem with leading/trailing whitespace matching
for
>> FileCheck --strict-whitespace --match-full-lines.
>>
>> Consider a text file:
>> ...
>> $ cat DUMP
>> bla1
>> bla2
>>   bla3
>> bla4
>>   bla5
>> ...
>>
>> with some leading and trailing spaces, made more visible like this:
>> ...
>> $ sed 's/ /_/g' DUMP
>> bla1
>> bla2
>> _bla3
>> bla4_
>> _bla5_
>> ...
>>
>> and a FileCheck file CHECK to match DUMP:
>> ...
>> $ cat CHECK
>> // CHECK-LABEL:bla1
>> // CHECK-NEXT:bla2
>> // CHECK-NEXT: bla3
>> // CHECK-NEXT:bla4
>> // CHECK-NEXT: bla5
>> ...
>>
>> with whitespace made more visible like this:
>> ...
>> $ sed 's/ /_/g' CHECK
>> //_CHECK-LABEL:bla1
>> //_CHECK-NEXT:bla2
>> //_CHECK-NEXT:_bla3
>> //_CHECK-NEXT:bla4_
>> //_CHECK-NEXT:_bla5_
>> ...
>>
>> When trying the match, it fails:
>> ...
>> $ cat DUMP | FileCheck CHECK --strict-whitespace --match-full-lines
>> CHECK:3:16: error: expected string not found in input
>> // CHECK-NEXT: bla3
>>                 ^
>> <stdin>:3:2: note: scanning from here
>>   bla3
>>   ^
>> ...
>>
>> I expect the match to succeed, because I expect leading and trailing
>> whitespace _not_ to be ignored, because the documentation states:
>> ...
>>   --match-full-lines
>>
>>      By default, FileCheck allows matches of anywhere on a line. This
>> option will require all positive matches to cover an entire line.
>> Leading and trailing whitespace is ignored, unless --strict-whitespace
>> is also specified.
>> ...
>>
>> After adding some debug code to FileCheck (which I proposed here on
>> llvm-dev ml as '[FileCheck] Add --verbose'), we can see where
things go
>> wrong:
>> ...
>> $ cat DUMP | /home/vries/gt/build/./bin/FileCheck CHECK
>> --strict-whitespace --match-full-lines --verbose
>> CHECK:3:16: note: RegEx string match: '^bla3$'
>> // CHECK-NEXT: bla3
>> ...
>>
>> The resulting regexp string is '^bla3$' instead of '^
bla3$'.
>>
>> The patch fixes this, and makes the behavior match the documentation.
>>
>> I ran the llvm/test/FileCheck tests, no regressions.
>>
Ran check-all this time.

Any further comments? OK for trunk?

Thanks,
- Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-FileCheck-Fix-strict-whitespace-match-full-lines.patch
Type: text/x-patch
Size: 2271 bytes
Desc: not available
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20161215/70ce98b9/attachment.bin>