Patch for review: enhancements/fixes to -verify (resubmission)

Hi,

Attached is a resubmission of my -verify enhancement patch.

There are two main differences between this version and the previous one at
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20120611/058799.html:

1. Following the patches for bugs in the preprocessor posted at:
     http://lists.cs.uiuc.edu/pipermail/cfe-dev/2012-June/022037.html
     http://lists.cs.uiuc.edu/pipermail/cfe-dev/2012-June/022044.html
   the needed changes to pre-existing test-cases has been brought to
   a minimum. ***NOTE*** this patch won't apply without these first
   being applied!!!

2. I have added an extensive test-case to (hopefully) demonstrate that
   there should be no erroneous swallowing of expected-* checks, as
   was Richard Smith's concern.

Since this is a resubmission, and there has been some discussion on
what the functionality should be, I will quickly outline what the
main changes for the new -verify implementation are:

1. Diagnostic checks can now be filtered out along with the lines
   that generate the diagnostics, when they are encapsulated inside
   a #if block that is skipped. This enables conditional checks.

2. It is also now possible to place expected-* checks on separate
   lines to that which creates the diagnostic. A line number can
   be specified in the check, and can be either absolute (i.e. to
   the actual line in the source file) or relative to the current
   line.

3. There is additional flexibility in the "number of occurrences"
   feature. In addition to being able to specify a fixed number
   or "one or more", it is now possible to specify "n or more"
   and "between n and m". As mentioned in one of the earlier
   posts, this is not necessarily a feature that may find much
   use for writing clang test-cases, but is a useful feature for
   anyone wishing to use -verify to test their own code.

The following are features discussed but not implemented:

1. Richard Smith asked for a @file:line feature, but this will
   cause multiple issues in pre-existing test-cases, since if I
   were to implement this properly, the verifier should check
   that every diagnostic is from the same file that the check
   specifies (and the default would be the current file). This
   means many test-cases will fail since currently they make
   use (misuse?!) of the fact that there isn't this check.

2. Jonathan Sauer suggested a way of specifying a way of
   overriding the conditional checks, but as Jordan Rose noted
   there are better ways of handling the corner cases that may
   need such a feature, notably FileCheck could be used.

3. There was also talk about different "modes" for -verify, but
   this has been avoided intentionally.

The following changes were necessary in pre-existing test-cases:

1. Where PCHs are involved, the expected-* checks needed to be
   moved into the actual test-case rather than the "header".

2. Where expected-* checks followed #error/#warning directives,
   the checks have been moved to a separate line. This enables
   better checking of these directives (given the way they are
   implemented themselves currently -- see below).

The following are bugs in the current implementation of "-verify"
that are fixed in my implementation:

1. It is possible to have a test-case with the following:

     #error XYZ // expected-error{{ABC}}

   In the current implementation, this will always pass. In the
   new implementation the expected-error check *must* go on a
   separate line, otherwise clang will say that there is a missing
   diagnostic check. This is because in the current implementation
   of #error/#warning, the whole line is considered as the text for
   the diagnostic (comments are included!). With the expected-*
   check on a separate line, a correct matching with the diagnostic
   is possible.

2. If a fatal error occurs in a test-case, then in the current
   implementation there will be no diagnostics from the -verify
   function either. This is fixed in the new implementation.

3. It could happen that the current implementation would not gather
   all the expected-* checks from included header files since it
   only considered the main file and one other FileID record. The
   new implementation considers all FileID records, marking them
   "unseen" if a diagnostic occurs with them, and "seen" if a
   expected-* check is found within them. At the end, if there
   exists any files that are "unseen" and not "seen", then these
   are parsed again for expected-* checks. This should limit the
   likelihood of missed expected-* checks, and did reveal an error
   in test/Modules/on-demand-build.m test-case.

I hope that has covered in summary everything important! Please
still quiz me if there are outstanding concerns not properly
addressed...

Thanks to all who have hung on in there with me on this ;o)

Cheers
Andy

verify-enhancement.diff (44.8 KB)

Hi Andy,

Attached is a resubmission of my -verify enhancement patch.

There are two main differences between this version and the previous one at
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20120611/058799.html:

1. Following the patches for bugs in the preprocessor posted at:
http://lists.cs.uiuc.edu/pipermail/cfe-dev/2012-June/022037.html
http://lists.cs.uiuc.edu/pipermail/cfe-dev/2012-June/022044.html
the needed changes to pre-existing test-cases has been brought to
a minimum. ***NOTE*** this patch won't apply without these first
being applied!!!

2. I have added an extensive test-case to (hopefully) demonstrate that
there should be no erroneous swallowing of expected-* checks, as
was Richard Smith's concern.

Thanks, the test cases look great.

Since this is a resubmission, and there has been some discussion on
what the functionality should be, I will quickly outline what the
main changes for the new -verify implementation are:

1. Diagnostic checks can now be filtered out along with the lines
that generate the diagnostics, when they are encapsulated inside
a #if block that is skipped. This enables conditional checks.

2. It is also now possible to place expected-* checks on separate
lines to that which creates the diagnostic. A line number can
be specified in the check, and can be either absolute (i.e. to
the actual line in the source file) or relative to the current
line.

3. There is additional flexibility in the "number of occurrences"
feature. In addition to being able to specify a fixed number
or "one or more", it is now possible to specify "n or more"
and "between n and m". As mentioned in one of the earlier
posts, this is not necessarily a feature that may find much
use for writing clang test-cases, but is a useful feature for
anyone wishing to use -verify to test their own code.

OK, I think you've made a good case for all these features. However,
you are much more likely to get them reviewed if you can provide
separate patches for each feature.

The following changes were necessary in pre-existing test-cases:

1. Where PCHs are involved, the expected-* checks needed to be
moved into the actual test-case rather than the "header".

OK.

2. If a fatal error occurs in a test-case, then in the current
implementation there will be no diagnostics from the -verify
function either. This is fixed in the new implementation.

Nice catch.

3. It could happen that the current implementation would not gather
all the expected-* checks from included header files since it
only considered the main file and one other FileID record. The
new implementation considers all FileID records, marking them
"unseen" if a diagnostic occurs with them, and "seen" if a
expected-* check is found within them. At the end, if there
exists any files that are "unseen" and not "seen", then these
are parsed again for expected-* checks. This should limit the
likelihood of missed expected-* checks, and did reveal an error
in test/Modules/on-demand-build.m test-case.

I didn't follow this. It seems to me that we should either (a) only
consider expected-* comments we see when preprocessing the main source
file, or (b) consider all expected-* comments we see in any file (and
the latter makes more sense to me). When does the need for re-parsing
arise?

Thanks!
Richard

OK, I think you've made a good case for all these features. However,
you are much more likely to get them reviewed if you can provide
separate patches for each feature.

No problem. I have split it up into 7 parts and posted them
to cfe-commits@cs.uiuc.edu:

http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20120625/059586.html
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20120625/059587.html
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20120625/059588.html
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20120625/059589.html
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20120625/059590.html
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20120625/059591.html
http://lists.cs.uiuc.edu/pipermail/cfe-commits/Week-of-Mon-20120625/059592.html

3. It could happen that the current implementation would not gather
all the expected-* checks from included header files since it
only considered the main file and one other FileID record. The
new implementation considers all FileID records, marking them
"unseen" if a diagnostic occurs with them, and "seen" if a
expected-* check is found within them. At the end, if there
exists any files that are "unseen" and not "seen", then these
are parsed again for expected-* checks. This should limit the
likelihood of missed expected-* checks, and did reveal an error
in test/Modules/on-demand-build.m test-case.

I didn't follow this. It seems to me that we should either (a) only
consider expected-* comments we see when preprocessing the main source
file, or (b) consider all expected-* comments we see in any file (and
the latter makes more sense to me). When does the need for re-parsing
arise?

Alright, not so well explained! In the old implementation, where
synthesised include files are used (for example, in modules or pre-
compiled headers) the implementation picked up "one other" FileID and
additionally processed this. The logic was not perfect and it could
(and did in one test-case) lead to a mismatch of expected-* checks and
actual diagnostics. The new implementation betters this slightly by
having a "seen" and "unseen" list: "seen" meaning it has extracted
expected-* checks from this file, "unseen" meaning that diagnostics
have been generated by this file. In an ideal world there should be
no mismatch at the end of processing between the "seen" and "unseen"
lists. However, this is still not the case (my implementation while
arguably better is still not perfect) since *some* synthesised
include files are still not "seen". In the case of ARCMT.cpp, I have
fixed this but other cases still exist. Therefore, I have a backup
which scans the unseen list and ensures that these files are processed
at the end even though they were missed first time around. In the
end, we can still guarantee that all source files have been seen.

A perfect solution, therefore, shouldn't need this backup check. When
I have some more time, I can devote it to pinning down these last
remaining cases and ensure that comments are bubbled up correctly to
VerifyDiagnosticConsumer.

In the meantime, I hope you will agree that the new implementation
is otherwise correct and worthy of being committed?

Many thanks again
Andy