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

Fri Nov 6 07:05:30 PST 2020

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>