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

[via llvm-dev]

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.

> 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'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.

> ...
> 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.

> 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