Mircea Trofin via llvm-dev
2020-Nov-09 18:18 UTC
[llvm-dev] [RFC] FileCheck: (dis)allowing unused prefixes
There's a wrinkle in this: some tests (clang ones, for instance) have output checks depending on the line position of the input. For example, they check debug info. Adding // FIXME: comments shift that. If the goal is easy identification of auto-inserted -allow-unused-prefixes directives, how about: - we make the flag an enum: true, false, and auto_inserted - we use -allow-unused-prefixes=auto_inserted for the scripts This allows easy identification, and should be easier to script without requiring more significant test by test surgery. WDYT? On Fri, Nov 6, 2020 at 7:13 AM Mircea Trofin <mtrofin at google.com> wrote:> Oh! Perfect - thanks! > > (plus, if it becomes unsupported, there's another nudge to fix :) ) > > On Fri, Nov 6, 2020 at 7:05 AM James Henderson < > jh7370.2008 at my.bristol.ac.uk> wrote: > >> 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/20201109/3f085738/attachment.html>
Mircea Trofin via llvm-dev
2020-Nov-09 18:24 UTC
[llvm-dev] [RFC] FileCheck: (dis)allowing unused prefixes
or - a variation - adding a --script-inserted-allow-unused-prefixes - to still allow stuff like --allow-unused-prefixes (meaning "true"). On Mon, Nov 9, 2020 at 10:18 AM Mircea Trofin <mtrofin at google.com> wrote:> There's a wrinkle in this: some tests (clang ones, for instance) have > output checks depending on the line position of the input. For example, > they check debug info. Adding // FIXME: comments shift that. > > If the goal is easy identification of auto-inserted -allow-unused-prefixes > directives, how about: > - we make the flag an enum: true, false, and auto_inserted > - we use -allow-unused-prefixes=auto_inserted for the scripts > > This allows easy identification, and should be easier to script without > requiring more significant test by test surgery. > > WDYT? > > On Fri, Nov 6, 2020 at 7:13 AM Mircea Trofin <mtrofin at google.com> wrote: > >> Oh! Perfect - thanks! >> >> (plus, if it becomes unsupported, there's another nudge to fix :) ) >> >> On Fri, Nov 6, 2020 at 7:05 AM James Henderson < >> jh7370.2008 at my.bristol.ac.uk> wrote: >> >>> 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/20201109/d5216862/attachment.html>
David Blaikie via llvm-dev
2020-Nov-09 21:53 UTC
[llvm-dev] [RFC] FileCheck: (dis)allowing unused prefixes
On Mon, Nov 9, 2020 at 10:18 AM Mircea Trofin via llvm-dev <llvm-dev at lists.llvm.org> wrote:> > There's a wrinkle in this: some tests (clang ones, for instance) have output checks depending on the line position of the input. For example, they check debug info. Adding // FIXME: comments shift that.Got a sense of roughly how many are like this? (have you found a lot, or so far just a few?) FileCheck has mechanism for referencing the current (& relative) line numbers which can help make tests like this more flexible - if it's only a few I'd be in favor of improving the tests to use that sort of thing.> If the goal is easy identification of auto-inserted -allow-unused-prefixes directives, how about: > - we make the flag an enum: true, false, and auto_inserted > - we use -allow-unused-prefixes=auto_inserted for the scripts > > This allows easy identification, and should be easier to script without requiring more significant test by test surgery. > > WDYT?Not hugely in favor, but plausible - I'd probably use a long/fairly explanatory name, but don't have great ideas for what that'd be. "-allow-unused-prefixes=allowed_by_cleanup_please_check_if_usage_is_intentional" or something.> > On Fri, Nov 6, 2020 at 7:13 AM Mircea Trofin <mtrofin at google.com> wrote: >> >> Oh! Perfect - thanks! >> >> (plus, if it becomes unsupported, there's another nudge to fix :) ) >> >> On Fri, Nov 6, 2020 at 7:05 AM James Henderson <jh7370.2008 at my.bristol.ac.uk> wrote: >>> >>> 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 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 > > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Mircea Trofin via llvm-dev
2020-Nov-09 22:05 UTC
[llvm-dev] [RFC] FileCheck: (dis)allowing unused prefixes
On Mon, Nov 9, 2020 at 1:54 PM David Blaikie <dblaikie at gmail.com> wrote:> On Mon, Nov 9, 2020 at 10:18 AM Mircea Trofin via llvm-dev > <llvm-dev at lists.llvm.org> wrote: > > > > There's a wrinkle in this: some tests (clang ones, for instance) have > output checks depending on the line position of the input. For example, > they check debug info. Adding // FIXME: comments shift that. > > Got a sense of roughly how many are like this? (have you found a lot, > or so far just a few?) FileCheck has mechanism for referencing the > current (& relative) line numbers which can help make tests like this > more flexible - if it's only a few I'd be in favor of improving the > tests to use that sort of thing. >Tens. I'd be happy to mark them in the spreadsheet, if anyone wants to fix them that way.> > > If the goal is easy identification of auto-inserted > -allow-unused-prefixes directives, how about: > > - we make the flag an enum: true, false, and auto_inserted > > - we use -allow-unused-prefixes=auto_inserted for the scripts > > > > This allows easy identification, and should be easier to script without > requiring more significant test by test surgery. > > > > WDYT? > > Not hugely in favor, but plausible - I'd probably use a long/fairly > explanatory name, but don't have great ideas for what that'd be. > > "-allow-unused-prefixes=allowed_by_cleanup_please_check_if_usage_is_intentional" > or something. >Right, or a separate flag dedicated to this.> > > > > On Fri, Nov 6, 2020 at 7:13 AM Mircea Trofin <mtrofin at google.com> wrote: > >> > >> Oh! Perfect - thanks! > >> > >> (plus, if it becomes unsupported, there's another nudge to fix :) ) > >> > >> On Fri, Nov 6, 2020 at 7:05 AM James Henderson < > jh7370.2008 at my.bristol.ac.uk> wrote: > >>> > >>> 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 > > > > _______________________________________________ > > 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/20201109/551d59db/attachment.html>
James Henderson via llvm-dev
2020-Nov-10 09:03 UTC
[llvm-dev] [RFC] FileCheck: (dis)allowing unused prefixes
I don't know if lit's parser is up to this (I suspect it isn't), but could you add a comment to the end/in the middle of a RUN? Something like `# RUN: do some thing | FileCheck %s --check-prefix=UNUSED --allow-unused-prefixes ## FIXME? That would avoid changing the line count. Alternatively, I'd just fixup the tests as you go (i.e. change the expected line number, if they're not huge in number). It may be obvious from reading whether or not the prefix is intentionally unused, so you might even be able to skip the FIXME annotation and just do the right thing. On Mon, 9 Nov 2020 at 18:18, Mircea Trofin <mtrofin at google.com> wrote:> There's a wrinkle in this: some tests (clang ones, for instance) have > output checks depending on the line position of the input. For example, > they check debug info. Adding // FIXME: comments shift that. > > If the goal is easy identification of auto-inserted -allow-unused-prefixes > directives, how about: > - we make the flag an enum: true, false, and auto_inserted > - we use -allow-unused-prefixes=auto_inserted for the scripts > > This allows easy identification, and should be easier to script without > requiring more significant test by test surgery. > > WDYT? > > On Fri, Nov 6, 2020 at 7:13 AM Mircea Trofin <mtrofin at google.com> wrote: > >> Oh! Perfect - thanks! >> >> (plus, if it becomes unsupported, there's another nudge to fix :) ) >> >> On Fri, Nov 6, 2020 at 7:05 AM James Henderson < >> jh7370.2008 at my.bristol.ac.uk> wrote: >> >>> 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/20201110/81fd0b7b/attachment.html>
Mircea Trofin via llvm-dev
2020-Nov-10 14:31 UTC
[llvm-dev] [RFC] FileCheck: (dis)allowing unused prefixes
On Tue, Nov 10, 2020, 01:03 James Henderson <jh7370.2008 at my.bristol.ac.uk> wrote:> I don't know if lit's parser is up to this (I suspect it isn't), but could > you add a comment to the end/in the middle of a RUN? Something like `# RUN: > do some thing | FileCheck %s --check-prefix=UNUSED --allow-unused-prefixes > ## FIXME? That would avoid changing the line count. Alternatively, I'd just > fixup the tests as you go (i.e. change the expected line number, if they're > not huge in number). It may be obvious from reading whether or not the > prefix is intentionally unused, so you might even be able to skip the FIXME > annotation and just do the right thing. >That's what I'm trying to avoid - there are about 30 or so such cases. What's the negative to the flag-based ideas?> On Mon, 9 Nov 2020 at 18:18, Mircea Trofin <mtrofin at google.com> wrote: > >> There's a wrinkle in this: some tests (clang ones, for instance) have >> output checks depending on the line position of the input. For example, >> they check debug info. Adding // FIXME: comments shift that. >> >> If the goal is easy identification of auto-inserted >> -allow-unused-prefixes directives, how about: >> - we make the flag an enum: true, false, and auto_inserted >> - we use -allow-unused-prefixes=auto_inserted for the scripts >> >> This allows easy identification, and should be easier to script without >> requiring more significant test by test surgery. >> >> WDYT? >> >> On Fri, Nov 6, 2020 at 7:13 AM Mircea Trofin <mtrofin at google.com> wrote: >> >>> Oh! Perfect - thanks! >>> >>> (plus, if it becomes unsupported, there's another nudge to fix :) ) >>> >>> On Fri, Nov 6, 2020 at 7:05 AM James Henderson < >>> jh7370.2008 at my.bristol.ac.uk> wrote: >>> >>>> 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/20201110/c13e4605/attachment.html>