[llvm-dev] [FileCheck] Error for unused check prefixes (was: [RFC] FileCheck: (dis)allowing unused prefixes)

Tue Feb 2 15:50:12 PST 2021

Last October Mircea sent an RFC about (dis)allowing unused check prefixes
in FileCheck:
https://lists.llvm.org/pipermail/llvm-dev/2020-October/146162.html
(In short, if FileCheck --check-prefix or --check-prefixes specifies a
prefix which does not occur in the test, FileCheck will error. Note: a
`-NOT` pattern is also counted as an occurrence.)

Mircea created a worksheet
https://docs.google.com/spreadsheets/d/1o6q3XH1n3DDyyccnYZ_kVfhFbTDzC_S09e973_cwYuw/edit#gid=0
which
may be a bit stale now but llvm-project has reached a state where all
issues have been identified and fixed, or worked around (by opting in
FileCheck --allow-unused-prefixes in some test directories, such as:
clang/test/OpenMP, llvm/test/Transforms/Attributor and llvm/test/FileCheck).

We can make a switch if the community thinks that not allowing unused
prefixes is the better default: https://reviews.llvm.org/D95849
For downstream projects using FileCheck and lit, it should be easy to
restore the previous permissive behavior

from lit.llvm.subst import ToolSubst

fc = ToolSubst('FileCheck', unresolved='fatal')
config.substitutions.insert(0, (fc.regex, 'FileCheck --allow-unused-prefixes'))

# Note: if multiple --allow-unused-prefixes options are specified, the
last wins.

Personally I am strongly in favor of disallowing unused check prefixes by
default. We have identified numerous issues:

(1) typo. A misspelled check prefix does not test what it intends to test.
(2) bitrot tests due to refactoring
(3) unspecified `-NOT` patterns. Sometimes a test uses something like
`--check-prefixes=COMMON,TRUE` and `--check-prefixes=COMMON,FALSE` to test
both behaviors but they forget to include a `FALSE-NO:` pattern to test
that some string does not appear.

(1) and (2) are especially common. There are indeed tests where
--allow-unused-prefixes is more suitable - but they are sufficiently few
that I think the default should be --allow-unused-prefixes=false.

So, what do folks think?

On Mon, Nov 16, 2020 at 8:22 AM Mircea Trofin <mtrofin at google.com> wrote:

> Thanks for doing this (as well as the awesome cleanup in CodeGen/X86)!
>
> On Sun, Nov 15, 2020 at 6:31 PM Wang, Pengfei <pengfei.wang at intel.com>
> wrote:
>
>> Hi,
>>
>> I wrote a script to help removing unused trivial prefixes. It is located
>> in llvm/utils/update_test_prefix.py.
>> It is a rough one and based on problems I met. It also highly relies on
>> the output of llvm-lit and update tools. So carefully verification on its
>> changes is MUST.
>>
>> Here's the usage of it:
>> 1. turn allow-unused-prefixes to false and built it: sed -i
>> '/allow-unused-prefixes/s/true/false/' llvm/utils/FileCheck/FileCheck.cpp;
>> 2. cd llvm-project;
>> 3. make sure llvm-lit and update_llc_test_checks.py or other update tools
>> are available;
>> 4. llvm/utils/update_test_prefix.py test_file_you_want_to_update.xxx
>> 5. you can also put them in a file and cat update_tests.txt | xargs
>> llvm/utils/update_test_prefix.py
>>
>> The script removes unused prefixes and verifies those autogenerated
>> tests. It reports "Failed" if there's conflicting in regeneration and
>> "Changed" if there're changes in the check contents. There's a known issue
>> if the prefix is not connected by "=", e.g. "--check-prefix X86". It will
>> be put into "Aborted". Please pay attention for tests in these lists.
>> Wish the script would help you😊
>>
>> Thanks
>> Pengfei
>>
>> -----Original Message-----
>> From: Fāng-ruì Sòng <maskray at google.com>
>> Sent: Friday, November 6, 2020 3:50 AM
>> To: Matt Arsenault <Matthew.Arsenault at amd.com>; Alexey.Bataev at ibm.com;
>> Tim Northover <t.p.northover at gmail.com>; Wang, Pengfei <
>> pengfei.wang at intel.com>
>> Cc: Mircea Trofin <mtrofin at google.com>; David Blaikie <dblaikie at gmail.com>;
>> LLVM Dev <llvm-dev at lists.llvm.org>
>> Subject: Re: [llvm-dev] [RFC] FileCheck: (dis)allowing unused prefixes
>>
>> 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?
>> >
>> > 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_kVfhFb
>> >>>>>> TDzC_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
>>
>>
>>
>> --
>> 宋方睿
>>
>

-- 
宋方睿
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20210202/85b5f042/attachment.html>