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

Mircea Trofin via llvm-dev llvm-dev at lists.llvm.org
Mon Nov 9 14:05:56 PST 2020 

On Mon, Nov 9, 2020 at 1:54 PM David Blaikie <dblaikie at gmail.com> wrote:

> On Mon, Nov 9, 2020 at 10:18 AM Mircea Trofin via llvm-dev
> <llvm-dev at lists.llvm.org> 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.
>
> Got a sense of roughly how many are like this? (have you found a lot,
> or so far just a few?) FileCheck has mechanism for referencing the
> current (& relative) line numbers which can help make tests like this
> more flexible - if it's only a few I'd be in favor of improving the
> tests to use that sort of thing.
>

Tens. I'd be happy to mark them in the spreadsheet, if anyone wants to fix
them that way.


>
> > 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?
>
> Not hugely in favor, but plausible - I'd probably use a long/fairly
> explanatory name, but don't have great ideas for what that'd be.
>
> "-allow-unused-prefixes=allowed_by_cleanup_please_check_if_usage_is_intentional"
> or something.
>

Right, or a separate flag dedicated to this.


>
> >
> > 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
> >
> > _______________________________________________
> > 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/551d59db/attachment.html>

More information about the llvm-dev mailing list