[llvm-dev] RFC: [FileCheck] CHECK-DAG for multiple occurrences of string

[Joel E. Denny via llvm-dev]

On Wed, May 16, 2018 at 11:40 AM, <paul.robinson at sony.com> wrote:

> I just stumbled across r332416, which was modifying a test with multiple
> identical CHECK-DAG directives, which reminded me I needed to get back
> to you about this... sorry for the slow response.
>

Not a problem.


> > From: Joel E. Denny [mailto:jdenny.ornl at gmail.com]
> >> On Mon, May 7, 2018 at 12:56 PM, <paul.robinson at sony.com> wrote:
> >>> 1. In a CHECK-DAG group, don't let the matches for patterns overlap.
> >>> 2. Add a new CHECK-DAG-N directive, where N is some integer, to express
> >>> that a pattern must have N non-overlapping matches.
> >>
> >> I think #1 is much more intuitive and easy to describe/document than #2.
> >> Changing the meaning of DAG in that way is highly unlikely to affect
> any
> >> existing test, IMO.  And if it does, my first expectation is that the
> test
> >> wasn't doing what the author intended anyway.
> >
> > I implemented #1, and I now have 105 new unexpected tests failures from
> > llvm core (58), clang (45), openmp runtime (1), and compiler-rt (1) test
> > suites.
>
> Okay... obviously more than I expected.  But hey, if we look at it as a
> percentage of all tests, it's really small!  [attempting to salvage a
> degree of self-respect]
>

I didn't exactly expect it either.  :-)

> I've only looked at a few failures so far.  Of those, the problem is
> > the expected one: in a CHECK-DAG group, multiple patterns match the
> > same line, so not permitting overlapping matches causes some patterns
> > never to match successfully.  However, all the overlapping matches so
> > far seem unintentional.  That is, this new implementation seems to be
> > catching test errors.
>
> Awesome.  Catching test errors is a definite plus for the project.
>

Thanks for encouraging me to pursue this implementation.


> > ...
> > Path Forward
> > ------------------
> >
> > These examples seem like strong evidence that permitting overlapping
> > matches is a significant source of test errors.  Moreover, we have my
> > original goal that some test authors were apparently assuming was
> > already supported: to be able to handle sets of strings that are both
> > unordered and non-unique.  Nevertheless, it's possible that there are
> > use cases, such as the one suggested by S1, that we lose.
>
> Hmmm... Either that one intended for the matches to be different, and
> it's a bug that they aren't; or it's an overly flexible test, and
> tightening it up does no harm.  The test change to accommodate the
> updated FileCheck behavior would have to go through the original test
> author or some other domain expert in order to determine which it is.
>
> I don't see the loss of flexibility as necessarily a bad thing; although
> we might find cases where behavior reasonably varies across targets, and
> then we would need to work out how to accommodate those differences.
>

Agreed.

Thanks for the feedback.

Joel

> Here are some potential paths forward:
> >
> > P1. Give up on approach #1 (non-overlapping CHECK-DAG) and go back to
> > #2 (CHECK-DAG-N).  I don't prefer this, but it would be less work.
> >
> > P2. Make CHECK-DAG non-overlapping but fix all test cases before
> > committing that change.  Patch reviews will surely be slow.
> >
> > P3. Make non-overlapping CHECK-DAG a special mode that's off by default
> > until all test suites have been fixed.
>
> I vote for P3.  It's easier to divide up the work if the feature is in
> upstream FileCheck, and it's easy for someone doing the work to enable
> the feature in their local branch.  I would not bother making it a
> command-line switch, unless we really do come up with cases where the
> overlapping matches are absolutely the right solution for some class of
> tests.
>
> I suggest you file a bug with the complete list of test failures that
> non-overlapping CHECK-DAG finds.  That will help keep track of the work,
> and if anyone else feels moved to pick up some part of it, they can make
> notes in the bug so we can try to avoid duplicating effort.  Hopefully
> I can devote some time to this myself, I think it's how CHECK-DAG should
> have been done in the first place and I'm sad we didn't catch it at the
> time.
>
> > By the way, I think it would be generally useful for FileCheck to have
> > a debugging mode that prints every match.  It would be particularly
> > useful while migrating to a non-overlapping CHECK-DAG.
> >
> > Thoughts?
> >
> > Joel
>
> A debugging mode like that sounds like a useful feature for writing a
> test, independent of the migration aid.  Do it.
>
> --paulr
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20180516/fe3cadf2/attachment.html>