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

Mircea Trofin via llvm-dev
2020-11-10T14:31:51Z

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>