Tom de Vries via llvm-dev
2016-Dec-15 10:31 UTC
[llvm-dev] [FileCheck] Fix --strict-whitespace --match-full-lines
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>
Robinson, Paul via llvm-dev
2016-Dec-15 16:51 UTC
[llvm-dev] [FileCheck] Fix --strict-whitespace --match-full-lines
> -----Original Message----- > From: Tom de Vries [mailto:Tom_deVries at mentor.com] > Sent: Thursday, December 15, 2016 2:31 AM > To: Robinson, Paul > Cc: Jonathan Roelofs; llvm-dev at lists.llvm.org > Subject: Re: [llvm-dev] [FileCheck] Fix --strict-whitespace --match-full- > lines > > 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.An intriguing trick, unfortunately it didn't work when I tried it on Windows. The Windows shell doesn't strip the single-quotes, so sed doesn't understand the command; but a Posix shell requires the quotes, to avoid globbing the .* part of the expression. We could work around that by using 'REQUIRES: shell' but at that point you're basically never testing FileCheck on Windows, which seems like a pretty serious hole in the test coverage. I think we're stuck with the fake regex hack, if we want single-file tests (for better maintainability). --paulr> > 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
Jonathan Roelofs via llvm-dev
2016-Dec-15 16:58 UTC
[llvm-dev] [FileCheck] Fix --strict-whitespace --match-full-lines
On 12/15/16 9:51 AM, Robinson, Paul wrote:> >> -----Original Message----- >> From: Tom de Vries [mailto:Tom_deVries at mentor.com] >> Sent: Thursday, December 15, 2016 2:31 AM >> To: Robinson, Paul >> Cc: Jonathan Roelofs; llvm-dev at lists.llvm.org >> Subject: Re: [llvm-dev] [FileCheck] Fix --strict-whitespace --match-full- >> lines >> >> 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. > An intriguing trick, unfortunately it didn't work when I tried it on > Windows. The Windows shell doesn't strip the single-quotes, so sed > doesn't understand the command; but a Posix shell requires the quotes, > to avoid globbing the .* part of the expression. We could work around > that by using 'REQUIRES: shell' but at that point you're basically > never testing FileCheck on Windows, which seems like a pretty serious > hole in the test coverage.Indeed. In general the community is moving away from using sed/awk/wc in tests, in favor of pure FileCheck tests, precisely because of these kinds of portability concerns.> > I think we're stuck with the fake regex hack, if we want single-file > tests (for better maintainability).+1 Jon> --paulr > >> 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-- Jon Roelofs jonathan at codesourcery.com CodeSourcery / Mentor Embedded
Tom de Vries via llvm-dev
2016-Dec-15 17:11 UTC
[llvm-dev] [FileCheck] Fix --strict-whitespace --match-full-lines
On 15/12/16 17:51, Robinson, Paul wrote:> > >> -----Original Message----- >> From: Tom de Vries [mailto:Tom_deVries at mentor.com] >> Sent: Thursday, December 15, 2016 2:31 AM >> To: Robinson, Paul >> Cc: Jonathan Roelofs; llvm-dev at lists.llvm.org >> Subject: Re: [llvm-dev] [FileCheck] Fix --strict-whitespace --match-full- >> lines >> >> 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. > > An intriguing trick, unfortunately it didn't work when I tried it on > Windows. The Windows shell doesn't strip the single-quotes, so sed > doesn't understand the command; but a Posix shell requires the quotes, > to avoid globbing the .* part of the expression. We could work around > that by using 'REQUIRES: shell' but at that point you're basically > never testing FileCheck on Windows, which seems like a pretty serious > hole in the test coverage. >What about implicit-check-not.txt? It contains the sed command, but with '#' instead of '/': ... ; RUN: sed 's#^;.*##' %s | FileCheck -check-prefix=CHECK-PASS -implicit-check-not=warning: %s ... Does that also not work on windows? Thanks, - Tom