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

[Mircea Trofin via llvm-dev]

On Mon, Nov 9, 2020 at 4:02 PM Fāng-ruì Sòng <maskray at google.com> wrote:

> I don't have a strong preference on the name, but I feel that we
> should fix some big test directories before adding
> --allow-unused-prefixes=something to clutter up git log/blame
> and I feel that they haven't been made aware of the FileCheck change.
>
> (I CCed some folks for these directories
> https://lists.llvm.org/pipermail/llvm-dev/2020-November/146419.html )
>
That's the part that kind of discouraged me hoping folks would fix their
area: having had seen your notifying folks, and no follow up reaction to
that.


>
> On Mon, Nov 9, 2020 at 3:58 PM Mircea Trofin via llvm-dev
> <llvm-dev at lists.llvm.org> wrote:
> >
> > how about -allow-unused-prefixes=needs_review
> >
> > On Mon, Nov 9, 2020 at 3:54 PM David Blaikie <dblaikie at gmail.com> wrote:
> >>
> >> I don't feel too strongly about the "=true" bit for legitimate cases -
> >> I'd prefer not to have it, but it's not the end of the world.
> >> I do feel more strongly that if you're going to automatically add it
> >> to cases that might not've intended to use that feature, it be pretty
> >> clear - I don't think "script_allowed" quite captures/highlights/makes
> >> it clear to the casual reader (why might not've seen this discussion
> >> or seen the flag before) that this was added automatically and there's
> >> a good chance the use of this feature is probably more likely than not
> >> unintentional/in need of cleanup.
> >>
> >> On Mon, Nov 9, 2020 at 3:48 PM Mircea Trofin <mtrofin at google.com>
> wrote:
> >> >
> >> > My preference would be to go with the tri-value option - I think the
> downside of folks needing to write a value after "-allow-unused-prefixes"
> is not that terrible; if folks feel that using true/false/auto is weird,
> how about "allowed/disallowed/script_allowed" or something like that. I'd
> argue that the value here is getting to the place where the default is
> right (so we stop the bleeding), and where the scripted cleanup can be
> easily audited subsequently.
> >> >
> >> > On Mon, Nov 9, 2020 at 2:22 PM David Blaikie <dblaikie at gmail.com>
> wrote:
> >> >>
> >> >> On Mon, Nov 9, 2020 at 2:06 PM Mircea Trofin <mtrofin at google.com>
> wrote:
> >> >> >
> >> >> >
> >> >> >
> >> >> > 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.
> >> >>
> >> >> Yeah, sound worth some annotation on the spreadsheet.
> >> >>
> >> >> >> > 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.
> >> >>
> >> >> *nod* Yeah, if adding this name would mean everyone else has to write
> >> >> "=true" instead of just the bare "-allow-unused-prefixes", another
> >> >> flag would be good. (otherwise maybe another flag would help make it
> >> >> more readable so it doesn't end up with confusing semantics due to
> the
> >> >> '=', etc).
> >> >>
> >> >> >
> >> >> >>
> >> >> >>
> >> >> >> >
> >> >> >> > 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
> >
> > _______________________________________________
> > 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/7aa48ec7/attachment-0001.html>