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

Mircea Trofin via llvm-dev llvm-dev at lists.llvm.org
Mon Nov 9 10:18:19 PST 2020 

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>

More information about the llvm-dev mailing list