James Henderson via llvm-dev
2020-Nov-06 10:22 UTC
[llvm-dev] [RFC] FileCheck: (dis)allowing unused prefixes
I think it would make more sense to add it at each individual call site. This ensures that all cases are fixed, rather than just one in a file. It also ensures that in the (hopefully unlikely) event that there are both intentional and unintentional use-cases within a file, each one gets checked. On Thu, 5 Nov 2020 at 20:29, Mircea Trofin via llvm-dev < llvm-dev at lists.llvm.org> wrote:> Thanks! > > On Thu, Nov 5, 2020 at 11:50 AM Fāng-ruì Sòng <maskray at google.com> wrote: > >> On Thu, Nov 5, 2020 at 11:36 AM David Blaikie via llvm-dev >> <llvm-dev at lists.llvm.org> wrote: >> > >> > >> > >> > On Thu, Nov 5, 2020 at 10:46 AM Mircea Trofin <mtrofin at google.com> >> wrote: >> >> >> >> >> >> >> >> On Thu, Nov 5, 2020 at 10:40 AM David Blaikie <dblaikie at gmail.com> >> wrote: >> >>> >> >>> >> >>> >> >>> On Thu, Nov 5, 2020 at 7:30 AM Mircea Trofin via llvm-dev < >> llvm-dev at lists.llvm.org> wrote: >> >>>> >> >>>> There are currently 1350 owner-less failures in the spreadsheet. >> These seem to be the larger areas there. >> >>>> >> >>>> If you see an area you have ownership or expertise in, please sign >> up for fixing the tests by Monday, Nov. 9. >> >>>> >> >>>> Otherwise, I will "blanket-add" --allow-unused-prefixes=true to the >> remaining failing tests. >> >>> >> >>> >> >>> If/when you do that, probably worth adding a comment at each site to >> clarify that this was added automatically, not vetted/intentionally added >> by a human. Something like "// FIXME: Verify that unused prefixes are used >> intentionally" or the like. >> >> >> >> Ack. or, we can grep for -allow-unused-prefixes=true, wdyt? >> > >> > >> > Not sure I understand who/when <https://teams.googleplex.com/u/when> >> they would grep for that? >> > > AAh! Nevermind me :) forgot that there are reasonable cases using it. Yes, > it makes sense to add a FIXME, perhaps at the start of each file > > >> > >> > I was suggesting adding an explicit "this use of >> --allow-unused-prefixes=true hasn't been confirmed as intentional" so that >> the backwards compatibility cases can be distinguished from the intentional >> cases when someone is reading the test case, rather than puzzling over why >> this flag was added (which looks intentional) though the unused prefix may >> not make sense in that particular test. It'll make it easier in the future >> when someone does look at the test for them to not feel like they're being >> implicitly told "this use of unused prefixes is intentional" (by the >> presence of an explicit flag requesting such support) while staring at the >> test and not being able to see why someone would've done that intentionally. >> >> Directly CCing some folks who can fix or find people to fix some >> directories (*/AMDGPU, */X86, */OpenMP, */AArch64) on >> >> https://docs.google.com/spreadsheets/d/1o6q3XH1n3DDyyccnYZ_kVfhFbTDzC_S09e973_cwYuw/edit#gid=0 >> After these big directories are cleaned up, the remaining tests should >> be manageable in amount. >> >> How to reproduce: >> >> sed -i '/allow-unused-prefixes/s/true/false/' >> llvm/utils/FileCheck/FileCheck.cpp >> git update-index --assume-unchanged llvm/utils/FileCheck/FileCheck.cpp >> ninja check-llvm # or check-clang ... >> >> >> >> >> >>> >> >>> >> >>>> >> >>>> >> >>>> >> >>>> On Fri, Oct 30, 2020 at 12:48 PM Mircea Trofin <mtrofin at google.com> >> wrote: >> >>>>> >> >>>>> An update: as of 871d658c9ceb, the flag is now available, if folks >> need to use it. >> >>>>> >> >>>>> On Thu, Oct 29, 2020 at 10:28 AM Mircea Trofin <mtrofin at google.com> >> wrote: >> >>>>>> >> >>>>>> Hello all, >> >>>>>> >> >>>>>> TL;DR; if you used FileCheck --check-prefixes and you missed >> (misspelled, for instance) one of the prefixes in your test, FileCheck >> silently ignores that and the test passes. >> >>>>>> >> >>>>>> 1579 tests have this property. >> >>>>>> >> >>>>>> >> >>>>>> The details >> >>>>>> ========>> >>>>>> Please refer to https://reviews.llvm.org/D90281 and the >> discussion there for more details (make sure you open "older changes" for >> full context) >> >>>>>> >> >>>>>> The problem is covered by the TL;DR;. >> >>>>>> >> >>>>>> The proposal is to add an explicit flag to FileCheck, >> --allow-unused-prefixes, to indicate whether the current behavior is >> intended (for instance, jdoerfert contributed a scenario where that is the >> case). >> >>>>>> >> >>>>>> We want the default behavior to be 'strict', i.e. >> --allow-unused-prefixes=false. Doing that right now would lead to 1500 test >> failures. >> >>>>>> >> >>>>>> To get there (thanks, maskray, for suggestion), we propose we: >> >>>>>> * land D90281 where the flag is introduced, but is flipped to >> match today's behavior >> >>>>>> * employ a 'busy beavers' approach, where test maintainers patch >> their tests: >> >>>>>> - either leveraging the flag, to explicitly indicate that unused >> prefixes is intended (i.e. add --allow-unused-patches=true); or >> >>>>>> - fix the test (e.g. maybe there was a misspelling >> issue/omission/etc). >> >>>>>> >> >>>>>> A spreadsheet with the failing tests is available here [1]. >> >>>>>> >> >>>>>> The request to the community members is to please sign up for >> their respective area in the spreadsheet, and then mark it completed when >> that's the case (yes/no in the respective column). >> >>>>>> >> >>>>>> When all the tests are fixed, we will then flip >> --allow-unused-prefixes to false by default. >> >>>>>> >> >>>>>> Meanwhile, please consider leveraging the flag explicitly when you >> author new tests that use --check-prefixes. That can be then cleaned up >> easily after we switch to the 'strict' behavior. >> >>>>>> >> >>>>>> Thanks! >> >>>>>> >> >>>>>> [1] >> https://docs.google.com/spreadsheets/d/1o6q3XH1n3DDyyccnYZ_kVfhFbTDzC_S09e973_cwYuw/edit?usp=sharing >> >>>> >> >>>> _______________________________________________ >> >>>> LLVM Developers mailing list >> >>>> llvm-dev at lists.llvm.org >> >>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >> > >> > _______________________________________________ >> > LLVM Developers mailing list >> > llvm-dev at lists.llvm.org >> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >> >> >> >> -- >> 宋方睿 >> > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20201106/8ea22f8c/attachment.html>
Mircea Trofin via llvm-dev
2020-Nov-06 15:00 UTC
[llvm-dev] [RFC] FileCheck: (dis)allowing unused prefixes
On Fri, Nov 6, 2020 at 2:22 AM James Henderson <jh7370.2008 at my.bristol.ac.uk> wrote:> I think it would make more sense to add it at each individual call site. > This ensures that all cases are fixed, rather than just one in a file. It > also ensures that in the (hopefully unlikely) event that there are both > intentional and unintentional use-cases within a file, each one gets > checked. > > That would be better indeed, but may be trickier to automate - thechallenge, in my mind, comes from multi-line RUN statements, but maybe I'm missing something / maybe there aren't that many. If folks have suggestions, please don't hesitate to throw them my way! Thanks!> On Thu, 5 Nov 2020 at 20:29, Mircea Trofin via llvm-dev < > llvm-dev at lists.llvm.org> wrote: > >> Thanks! >> >> On Thu, Nov 5, 2020 at 11:50 AM Fāng-ruì Sòng <maskray at google.com> wrote: >> >>> On Thu, Nov 5, 2020 at 11:36 AM David Blaikie via llvm-dev >>> <llvm-dev at lists.llvm.org> wrote: >>> > >>> > >>> > >>> > On Thu, Nov 5, 2020 at 10:46 AM Mircea Trofin <mtrofin at google.com> >>> wrote: >>> >> >>> >> >>> >> >>> >> On Thu, Nov 5, 2020 at 10:40 AM David Blaikie <dblaikie at gmail.com> >>> wrote: >>> >>> >>> >>> >>> >>> >>> >>> On Thu, Nov 5, 2020 at 7:30 AM Mircea Trofin via llvm-dev < >>> llvm-dev at lists.llvm.org> wrote: >>> >>>> >>> >>>> There are currently 1350 owner-less failures in the spreadsheet. >>> These seem to be the larger areas there. >>> >>>> >>> >>>> If you see an area you have ownership or expertise in, please sign >>> up for fixing the tests by Monday, Nov. 9. >>> >>>> >>> >>>> Otherwise, I will "blanket-add" --allow-unused-prefixes=true to the >>> remaining failing tests. >>> >>> >>> >>> >>> >>> If/when you do that, probably worth adding a comment at each site to >>> clarify that this was added automatically, not vetted/intentionally added >>> by a human. Something like "// FIXME: Verify that unused prefixes are used >>> intentionally" or the like. >>> >> >>> >> Ack. or, we can grep for -allow-unused-prefixes=true, wdyt? >>> > >>> > >>> > Not sure I understand who/when <https://teams.googleplex.com/u/when> >>> they would grep for that? >>> >> >> AAh! Nevermind me :) forgot that there are reasonable cases using it. >> Yes, it makes sense to add a FIXME, perhaps at the start of each file >> >> >>> > >>> > I was suggesting adding an explicit "this use of >>> --allow-unused-prefixes=true hasn't been confirmed as intentional" so that >>> the backwards compatibility cases can be distinguished from the intentional >>> cases when someone is reading the test case, rather than puzzling over why >>> this flag was added (which looks intentional) though the unused prefix may >>> not make sense in that particular test. It'll make it easier in the future >>> when someone does look at the test for them to not feel like they're being >>> implicitly told "this use of unused prefixes is intentional" (by the >>> presence of an explicit flag requesting such support) while staring at the >>> test and not being able to see why someone would've done that intentionally. >>> >>> Directly CCing some folks who can fix or find people to fix some >>> directories (*/AMDGPU, */X86, */OpenMP, */AArch64) on >>> >>> https://docs.google.com/spreadsheets/d/1o6q3XH1n3DDyyccnYZ_kVfhFbTDzC_S09e973_cwYuw/edit#gid=0 >>> After these big directories are cleaned up, the remaining tests should >>> be manageable in amount. >>> >>> How to reproduce: >>> >>> sed -i '/allow-unused-prefixes/s/true/false/' >>> llvm/utils/FileCheck/FileCheck.cpp >>> git update-index --assume-unchanged llvm/utils/FileCheck/FileCheck.cpp >>> ninja check-llvm # or check-clang ... >>> >>> >>> >>> >>> >>> >>> >>> >>> >>>> >>> >>>> >>> >>>> >>> >>>> On Fri, Oct 30, 2020 at 12:48 PM Mircea Trofin <mtrofin at google.com> >>> wrote: >>> >>>>> >>> >>>>> An update: as of 871d658c9ceb, the flag is now available, if folks >>> need to use it. >>> >>>>> >>> >>>>> On Thu, Oct 29, 2020 at 10:28 AM Mircea Trofin <mtrofin at google.com> >>> wrote: >>> >>>>>> >>> >>>>>> Hello all, >>> >>>>>> >>> >>>>>> TL;DR; if you used FileCheck --check-prefixes and you missed >>> (misspelled, for instance) one of the prefixes in your test, FileCheck >>> silently ignores that and the test passes. >>> >>>>>> >>> >>>>>> 1579 tests have this property. >>> >>>>>> >>> >>>>>> >>> >>>>>> The details >>> >>>>>> ========>>> >>>>>> Please refer to https://reviews.llvm.org/D90281 and the >>> discussion there for more details (make sure you open "older changes" for >>> full context) >>> >>>>>> >>> >>>>>> The problem is covered by the TL;DR;. >>> >>>>>> >>> >>>>>> The proposal is to add an explicit flag to FileCheck, >>> --allow-unused-prefixes, to indicate whether the current behavior is >>> intended (for instance, jdoerfert contributed a scenario where that is the >>> case). >>> >>>>>> >>> >>>>>> We want the default behavior to be 'strict', i.e. >>> --allow-unused-prefixes=false. Doing that right now would lead to 1500 test >>> failures. >>> >>>>>> >>> >>>>>> To get there (thanks, maskray, for suggestion), we propose we: >>> >>>>>> * land D90281 where the flag is introduced, but is flipped to >>> match today's behavior >>> >>>>>> * employ a 'busy beavers' approach, where test maintainers patch >>> their tests: >>> >>>>>> - either leveraging the flag, to explicitly indicate that >>> unused prefixes is intended (i.e. add --allow-unused-patches=true); or >>> >>>>>> - fix the test (e.g. maybe there was a misspelling >>> issue/omission/etc). >>> >>>>>> >>> >>>>>> A spreadsheet with the failing tests is available here [1]. >>> >>>>>> >>> >>>>>> The request to the community members is to please sign up for >>> their respective area in the spreadsheet, and then mark it completed when >>> that's the case (yes/no in the respective column). >>> >>>>>> >>> >>>>>> When all the tests are fixed, we will then flip >>> --allow-unused-prefixes to false by default. >>> >>>>>> >>> >>>>>> Meanwhile, please consider leveraging the flag explicitly when >>> you author new tests that use --check-prefixes. That can be then cleaned up >>> easily after we switch to the 'strict' behavior. >>> >>>>>> >>> >>>>>> Thanks! >>> >>>>>> >>> >>>>>> [1] >>> https://docs.google.com/spreadsheets/d/1o6q3XH1n3DDyyccnYZ_kVfhFbTDzC_S09e973_cwYuw/edit?usp=sharing >>> >>>> >>> >>>> _______________________________________________ >>> >>>> LLVM Developers mailing list >>> >>>> llvm-dev at lists.llvm.org >>> >>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >>> > >>> > _______________________________________________ >>> > LLVM Developers mailing list >>> > llvm-dev at lists.llvm.org >>> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >>> >>> >>> >>> -- >>> 宋方睿 >>> >> _______________________________________________ >> LLVM Developers mailing list >> llvm-dev at lists.llvm.org >> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >> >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20201106/faeeae88/attachment.html>
James Henderson via llvm-dev
2020-Nov-06 15:05 UTC
[llvm-dev] [RFC] FileCheck: (dis)allowing unused prefixes
I recently discovered that multi-line RUN statements can actually be interrupted with non-RUN lines, without changing the behaviour. In other words, you can do something like: # RUN: some command --option1 \ ## Comment # CHECK: check something # RUN: --option2 And you'd end up with "some command --option1 --option2" being run. It's rather surprising behaviour, and not one I'd generally recommend exercising, but maybe in this context it would be okay? On Fri, 6 Nov 2020 at 15:00, Mircea Trofin <mtrofin at google.com> wrote:> > > On Fri, Nov 6, 2020 at 2:22 AM James Henderson < > jh7370.2008 at my.bristol.ac.uk> wrote: > >> I think it would make more sense to add it at each individual call site. >> This ensures that all cases are fixed, rather than just one in a file. It >> also ensures that in the (hopefully unlikely) event that there are both >> intentional and unintentional use-cases within a file, each one gets >> checked. >> >> That would be better indeed, but may be trickier to automate - the > challenge, in my mind, comes from multi-line RUN statements, but maybe I'm > missing something / maybe there aren't that many. If folks have > suggestions, please don't hesitate to throw them my way! > > Thanks! > > >> On Thu, 5 Nov 2020 at 20:29, Mircea Trofin via llvm-dev < >> llvm-dev at lists.llvm.org> wrote: >> >>> Thanks! >>> >>> On Thu, Nov 5, 2020 at 11:50 AM Fāng-ruì Sòng <maskray at google.com> >>> wrote: >>> >>>> On Thu, Nov 5, 2020 at 11:36 AM David Blaikie via llvm-dev >>>> <llvm-dev at lists.llvm.org> wrote: >>>> > >>>> > >>>> > >>>> > On Thu, Nov 5, 2020 at 10:46 AM Mircea Trofin <mtrofin at google.com> >>>> wrote: >>>> >> >>>> >> >>>> >> >>>> >> On Thu, Nov 5, 2020 at 10:40 AM David Blaikie <dblaikie at gmail.com> >>>> wrote: >>>> >>> >>>> >>> >>>> >>> >>>> >>> On Thu, Nov 5, 2020 at 7:30 AM Mircea Trofin via llvm-dev < >>>> llvm-dev at lists.llvm.org> wrote: >>>> >>>> >>>> >>>> There are currently 1350 owner-less failures in the spreadsheet. >>>> These seem to be the larger areas there. >>>> >>>> >>>> >>>> If you see an area you have ownership or expertise in, please sign >>>> up for fixing the tests by Monday, Nov. 9. >>>> >>>> >>>> >>>> Otherwise, I will "blanket-add" --allow-unused-prefixes=true to >>>> the remaining failing tests. >>>> >>> >>>> >>> >>>> >>> If/when you do that, probably worth adding a comment at each site >>>> to clarify that this was added automatically, not vetted/intentionally >>>> added by a human. Something like "// FIXME: Verify that unused prefixes are >>>> used intentionally" or the like. >>>> >> >>>> >> Ack. or, we can grep for -allow-unused-prefixes=true, wdyt? >>>> > >>>> > >>>> > Not sure I understand who/when <https://teams.googleplex.com/u/when> >>>> they would grep for that? >>>> >>> >>> AAh! Nevermind me :) forgot that there are reasonable cases using it. >>> Yes, it makes sense to add a FIXME, perhaps at the start of each file >>> >>> >>>> > >>>> > I was suggesting adding an explicit "this use of >>>> --allow-unused-prefixes=true hasn't been confirmed as intentional" so that >>>> the backwards compatibility cases can be distinguished from the intentional >>>> cases when someone is reading the test case, rather than puzzling over why >>>> this flag was added (which looks intentional) though the unused prefix may >>>> not make sense in that particular test. It'll make it easier in the future >>>> when someone does look at the test for them to not feel like they're being >>>> implicitly told "this use of unused prefixes is intentional" (by the >>>> presence of an explicit flag requesting such support) while staring at the >>>> test and not being able to see why someone would've done that intentionally. >>>> >>>> Directly CCing some folks who can fix or find people to fix some >>>> directories (*/AMDGPU, */X86, */OpenMP, */AArch64) on >>>> >>>> https://docs.google.com/spreadsheets/d/1o6q3XH1n3DDyyccnYZ_kVfhFbTDzC_S09e973_cwYuw/edit#gid=0 >>>> After these big directories are cleaned up, the remaining tests should >>>> be manageable in amount. >>>> >>>> How to reproduce: >>>> >>>> sed -i '/allow-unused-prefixes/s/true/false/' >>>> llvm/utils/FileCheck/FileCheck.cpp >>>> git update-index --assume-unchanged llvm/utils/FileCheck/FileCheck.cpp >>>> ninja check-llvm # or check-clang ... >>>> >>>> >>>> >>>> >>>> >>> >>>> >>> >>>> >>>> >>>> >>>> >>>> >>>> >>>> >>>> On Fri, Oct 30, 2020 at 12:48 PM Mircea Trofin <mtrofin at google.com> >>>> wrote: >>>> >>>>> >>>> >>>>> An update: as of 871d658c9ceb, the flag is now available, if >>>> folks need to use it. >>>> >>>>> >>>> >>>>> On Thu, Oct 29, 2020 at 10:28 AM Mircea Trofin < >>>> mtrofin at google.com> wrote: >>>> >>>>>> >>>> >>>>>> Hello all, >>>> >>>>>> >>>> >>>>>> TL;DR; if you used FileCheck --check-prefixes and you missed >>>> (misspelled, for instance) one of the prefixes in your test, FileCheck >>>> silently ignores that and the test passes. >>>> >>>>>> >>>> >>>>>> 1579 tests have this property. >>>> >>>>>> >>>> >>>>>> >>>> >>>>>> The details >>>> >>>>>> ========>>>> >>>>>> Please refer to https://reviews.llvm.org/D90281 and the >>>> discussion there for more details (make sure you open "older changes" for >>>> full context) >>>> >>>>>> >>>> >>>>>> The problem is covered by the TL;DR;. >>>> >>>>>> >>>> >>>>>> The proposal is to add an explicit flag to FileCheck, >>>> --allow-unused-prefixes, to indicate whether the current behavior is >>>> intended (for instance, jdoerfert contributed a scenario where that is the >>>> case). >>>> >>>>>> >>>> >>>>>> We want the default behavior to be 'strict', i.e. >>>> --allow-unused-prefixes=false. Doing that right now would lead to 1500 test >>>> failures. >>>> >>>>>> >>>> >>>>>> To get there (thanks, maskray, for suggestion), we propose we: >>>> >>>>>> * land D90281 where the flag is introduced, but is flipped to >>>> match today's behavior >>>> >>>>>> * employ a 'busy beavers' approach, where test maintainers patch >>>> their tests: >>>> >>>>>> - either leveraging the flag, to explicitly indicate that >>>> unused prefixes is intended (i.e. add --allow-unused-patches=true); or >>>> >>>>>> - fix the test (e.g. maybe there was a misspelling >>>> issue/omission/etc). >>>> >>>>>> >>>> >>>>>> A spreadsheet with the failing tests is available here [1]. >>>> >>>>>> >>>> >>>>>> The request to the community members is to please sign up for >>>> their respective area in the spreadsheet, and then mark it completed when >>>> that's the case (yes/no in the respective column). >>>> >>>>>> >>>> >>>>>> When all the tests are fixed, we will then flip >>>> --allow-unused-prefixes to false by default. >>>> >>>>>> >>>> >>>>>> Meanwhile, please consider leveraging the flag explicitly when >>>> you author new tests that use --check-prefixes. That can be then cleaned up >>>> easily after we switch to the 'strict' behavior. >>>> >>>>>> >>>> >>>>>> Thanks! >>>> >>>>>> >>>> >>>>>> [1] >>>> https://docs.google.com/spreadsheets/d/1o6q3XH1n3DDyyccnYZ_kVfhFbTDzC_S09e973_cwYuw/edit?usp=sharing >>>> >>>> >>>> >>>> _______________________________________________ >>>> >>>> LLVM Developers mailing list >>>> >>>> llvm-dev at lists.llvm.org >>>> >>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >>>> > >>>> > _______________________________________________ >>>> > LLVM Developers mailing list >>>> > llvm-dev at lists.llvm.org >>>> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >>>> >>>> >>>> >>>> -- >>>> 宋方睿 >>>> >>> _______________________________________________ >>> LLVM Developers mailing list >>> llvm-dev at lists.llvm.org >>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >>> >>-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20201106/fa41c79c/attachment.html>