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

Mircea Trofin via llvm-dev
2020-11-09T18:24:38Z

or - a variation - adding a --script-inserted-allow-unused-prefixes - to
still allow stuff like --allow-unused-prefixes (meaning "true").



On Mon, Nov 9, 2020 at 10:18 AM 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/20201109/d5216862/attachment.html>