[llvm-dev] [RFC] FileCheck: (dis)allowing unused prefixes

Fri Nov 6 07:13:24 PST 2020

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/20201106/c370158c/attachment.html>