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

Tue Nov 10 08:02:11 PST 2020

On Tue, Nov 10, 2020 at 7:54 AM James Henderson <
jh7370.2008 at my.bristol.ac.uk> wrote:

> I think David Blaikie already touched on it - it leaves noise in the tests
> that isn't immediately clear why it is present. Individuals are more likely
> to therefore ignore it. Test noise should be kept to a minimum, as it a)
> obfuscates what's important and b) might get copied over when people want
> to create a new test based on an existing one.
>

I see - indeed, probably a big "FIXME" would give folks more pause when
copying than a flag would. I missed this consideration, thanks!

>
> What's the negative with fixing the tests? Somebody's got to do it! :-)
> Alternatively, proactively go after the people who have recently modified
> the test and/or wrote it in the first place to get them to address the
> issue. I don't think there's a massive rush here, so if it takes a couple
> of weeks or a month for somebody to get it sorted, so be it (I had to get
> changes made a couple of years ago to an internal code base sprawled across
> dozens of different teams, and it took months for all the changes to get
> made!)
>

maskray sent last week a "chase-up" email, didn't seem to get much traction
though, hence my looking for alternatives that would address our problems,
assuming their trade-offs are seen as reasonable.

OK, I'll go piecemeal over the 30 or so tests, and stick to FIXMEs; if
we're OK with them sometimes being on the same line, that would give me
some leeway in some corner cases, I think.

>
> On Tue, 10 Nov 2020 at 14:32, Mircea Trofin <mtrofin at google.com> wrote:
>
>>
>>
>> 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/7855a49a/attachment.html>