[cfe-commits] Patch for review: enhancements/fixes to -verify

[zygoloid]

Hi all,

The -verify option has the potential for uses outside just the clang
test-suite, for example, within a test-suite for a 3rd-party library.
However, it has a number of idiosyncrasies or missing features which the
attached patch seeks to address. While none of the alterations are perhaps
critical to the development of clang itself (that remains to be seen!), they
do make the -verify feature immeasurably more flexible and useful.

In the first instance, the patch enables -verify to distinguish between
blocks of code which have been #if/#ifdef-ed out and those which have not.
The current implementation will not do this, so for example, this trivial
fragment will cause an “expected but not seen” failure:

#if 0
#error some error // expected-error {{some error}}
#endif

I think this is neat, but I have a concern that this makes it too easy to write tests which unexpectedly check nothing. For instance, a test like this becomes a no-op with your patch:

#if __has_feature(foo)
#error should have foo // expected-error {{should have foo}}
#endif

However, this may be a price worth paying for the extra convenience of being able to put multiple similar tests in the same file.

Secondly, in the current implementation, all diagnostics must be contained
on the actual line which generates the diagnostic. The new implementation
allows a line number to be optionally specified as part of the diagnostic
declaration using the @ symbol, for example:

// expected-error@ {{some error}}

where can be some absolute line number, for example: 10, or a
relative line number, for example: -1 or +5, from the current line. Note
that where an absolute line number is given this is intentionally the actual
source line in the file in question, not one modified by the #line
directive. Likewise, relative line numbers are also not affected by the
#line directive.

Another problem with the existing -verify logic is the way it deals with diagnostics which appear outside the main source file. You must currently annotate the line (in the main source file) corresponding to the line in the other file on which the diagnostic appears. It would be useful to extend this extension to handle expected-error@:, where must match (a suffix of?) the relevant file name.

The patch also includes the relevant modifications necessary to the existing
clang test-suite. These modifications were necessary for two reasons: since
#if-block processing is now included, various tests in the PCH folder were
now missing their relevant diagnostics.

The PCH changes are unfortunate. Is there any way we can get this to work properly for those cases? The relevant lines are included in the TU (through an included file or PCH).

Were all the non-PCH test changes actually necessary? I would find it very worrying if we are somehow losing expected-foo checks in some cases. For instance:

+// expected-error@+2 {{pasting formed ‘/', an invalid preprocessing token}}
#define COMM / ## *
-COMM // expected-error {{pasting formed '/’, an invalid preprocessing token}}
+COMM

Why is this change necessary?

— test/Preprocessor/if_warning.c (revision 158299)
+++ test/Preprocessor/if_warning.c (working copy)

@@ -3,7 +3,8 @@

extern int x;

-#if foo // expected-error {{‘foo’ is not defined, evaluates to 0}}
+// expected-error@+1 {{‘foo’ is not defined, evaluates to 0}}
+#if foo
#endif

#ifdef foo
@@ -22,8 +23,9 @@

// rdar://9475098
#if 0
-#else 1 // expected-warning {{extra tokens}}
+#else 1
#endif
+// expected-warning@-2 {{extra tokens}}

I would have expected both of these to work as-is; neither the #if nor the #else line here is skipped.

Also #error/#warning directives
include all text up to the end-of-line in their message so tests containing
these have the diagnostics shifted onto a separate line now.

That looks like a bug. Comments are supposed to be “replaced by one space character” in the phase of translation preceding the expansion of preprocessing directives, so should not appear in the diagnostic.

[Andy_Gibbs]

Hi,

[…snip…]
#if 0
#error some error // expected-error {{some error}}
#endif

I think this is neat, but I have a concern that this makes
it too easy to write tests which unexpectedly check nothing.
For instance, a test like this becomes a no-op with your
patch:

#if __has_feature(foo)
#error should have foo // expected-error {{should have foo}}
#endif

However, this may be a price worth paying for the extra
convenience of being able to put multiple similar tests in
the same file.

The way around would be…

// expected-error@+2 {{should have foo}}
#if !__has_feature(foo)
#error should have foo
#endif

Of course, it does require that the test-case writer will think to do this, in which case the unexpected no-op might not be written in the first place. As mentioned in my response to Jordan Rose, perhaps this is where some test-case-writing guidelines on the clang website might come in handy. (I’m not particularly volunteering here; I expect there will be clang veterans much better suited to this.)

Another problem with the existing -verify logic is the way
it deals with diagnostics which appear outside the main
source file. You must currently annotate the line (in the
main source file) corresponding to the line in the other
file on which the diagnostic appears. It would be useful to
extend this extension to handle expected-error@:<line>,
where must match (a suffix of?) the relevant file
name.

I guess this should be possible… assuming an iterator of FileID objects can accessed through SourceManager, or similar, then the correct FileID can be linked to the diagnostic location. Then the only further change would be to also check FileIDs when running through the diagnostics list during post-processing. This latter change is probably worth doing anyway (assuming it doesn’t break any existing test cases). I don’t have the code in front of me right now – I’ll have a look tomorrow, and see if I have time to do this.

The PCH changes are unfortunate. Is there any way we can get
this to work properly for those cases? The relevant lines are
included in the TU (through an included file or PCH).

But my understanding is that the generated PCH doesn’t include the comments, so they can’t be extracted from there. Someone will be able to correct me here, or may have some other ideas how to get around it.

Were all the non-PCH test changes actually necessary? I would
find it very worrying if we are somehow losing expected-foo
checks in some cases.

I don’t believe we are losing any expected-foo checks. They are all still there, but may have been moved to a different location in the file. The main reason is, if we have test-case of the style…

#ifndef PASS1
#define PASS1
/// this part ends up in the PCH
#else
/// this part tests the PCH
#endif

by necessity the expected-foo checks must be moved out of the first block into the second, unless somehow they can be embedded in the PCH itself. The extension is, of course, the situation where the PCH is not generated out of the test-case itself, but is pre-generated in some other fashion. In which case, it is clear that the expected-foo checks would need to be part of the test-case (failing that they aren’t embedded in the PCH, which at present they aren’t, to my understanding).

For instance:

+// expected-error@+2 {{pasting formed ‘/', an invalid preprocessing token}}
#define COMM / ## *
-COMM // expected-error {{pasting formed '/’, an invalid preprocessing token}}
+COMM

Why is this change necessary?

Because the preprocessor was unable to “see” the comment in this situation. This may be a bug in the preprocessor, but that is outside my expertise (see further explanation below).

— test/Preprocessor/if_warning.c (revision 158299)
+++ test/Preprocessor/if_warning.c (working copy)
@@ -3,7 +3,8 @@

extern int x;

-#if foo // expected-error {{‘foo’ is not defined, evaluates to 0}}
+// expected-error@+1 {{‘foo’ is not defined, evaluates to 0}}
+#if foo
#endif

#ifdef foo
@@ -22,8 +23,9 @@

// rdar://9475098
#if 0
-#else 1 // expected-warning {{extra tokens}}
+#else 1
#endif
+// expected-warning@-2 {{extra tokens}}

I would have expected both of these to work as-is; neither
the #if nor the #else line here is skipped.

Again, this came down to the preprocessor not “seeing” the comments. I stepped through with a debugger while writing the patch, but I wasn’t able to observe an exact reason as to why the preprocessor/lexer didn’t call CommentHandler::HandleComment.

The main change that I have made in my patch is to hook into the preprocessor using the CommentHandler mechanism rather than re-parsing the source file during post-processing of the diagnostics list, which was the previous behaviour (which didn’t use the full preprocessor, just the lexer). Since this is the case, it now relies on the preprocessor to correctly expose the comments in the source file(s), which therefore may now expose bugs there (possibly – again, I don’t want to point fingers of blame, especially since the workings of the preprocessor are not something I have particular experience of at present).

The changes made to the test-suite were necessary to ensure no regressions due to my patch. I checked each change thoroughly to be satisfied that it didn’t change the nature of the test and that no diagnostic checks were lost, so I hope I was successful to this end.

Alternatively, we could mark these tests as XFAIL and then, as a separate thing, look at fixing the preprocessor, if that is what needs doing.

What’s the best approach, from your point of view?

Cheers
Andy

[zygoloid]

Hi,

[…snip…]

#if 0
#error some error // expected-error {{some error}}
#endif

I think this is neat, but I have a concern that this makes
it too easy to write tests which unexpectedly check nothing.
For instance, a test like this becomes a no-op with your
patch:

#if __has_feature(foo)
#error should have foo // expected-error {{should have foo}}
#endif

However, this may be a price worth paying for the extra
convenience of being able to put multiple similar tests in
the same file.

The way around would be…

// expected-error@+2 {{should have foo}}
#if !__has_feature(foo)
#error should have foo
#endif

Of course, it does require that the test-case writer will think to do this, in which case the unexpected no-op might not be written in the first place. As mentioned in my response to Jordan Rose, perhaps this is where some test-case-writing guidelines on the clang website might come in handy. (I’m not particularly volunteering here; I expect there will be clang veterans much better suited to this.)

Right, my point was not that it’s hard to test, but that it’s dangerous: it adds to the list of things we need to be careful about. I’m also concerned that we may have existing tests which do something like that, which this change would neuter. If not, then I’m happy to accept that this doesn’t happen in practice.

The PCH changes are unfortunate. Is there any way we can get

this to work properly for those cases? The relevant lines are
included in the TU (through an included file or PCH).

But my understanding is that the generated PCH doesn’t include the comments, so they can’t be extracted from there. Someone will be able to correct me here, or may have some other ideas how to get around it.

Yes, I suspect this may not be possible. One alternative would be to introduce another -verify mode which looks at comments within #if 0’d out blocks.

Were all the non-PCH test changes actually necessary? I would
find it very worrying if we are somehow losing expected-foo
checks in some cases.

I don’t believe we are losing any expected-foo checks. They are all still there, but may have been moved to a different location in the file.

[…]

For instance:

+// expected-error@+2 {{pasting formed ‘/', an invalid preprocessing token}}
#define COMM / ## *
-COMM // expected-error {{pasting formed '/’, an invalid preprocessing token}}
+COMM

Why is this change necessary?

Because the preprocessor was unable to “see” the comment in this situation.

This is exactly what I meant by “losing” expected-foo checks – any new markers added in such places will have no effect. For what it’s worth, I think this is at least partially a feature of these changes: they’re allowing us to more directly test our CommentHandler logic, which appears to be broken in various ways. Nonetheless, we should fix the preprocessor rather than working around it in the tests.

Alternatively, we could mark these tests as XFAIL and then, as a separate thing, look at fixing the preprocessor, if that is what needs doing.

What’s the best approach, from your point of view?

The @line part seems to be uncontroversial – if you can provide a patch for just that part, we can work to get that checked in first. I’m not particularly comfortable basing -verify on CommentHandler while it’s known to have dropped-comment bugs, so I’d prefer to get it fixed before taking the CommentHandler change. Assuming that we’re happy to change all the PCH tests per your patch, how many tests would we have to XFAIL?

[Andy_Gibbs]

The way around would be...

// expected-error@+2 {{should have foo}}
#if !__has_feature(foo)
#error should have foo
#endif

Of course, it does require that the test-case writer will think
to do this, in which case the unexpected no-op might not be
written in the first place. As mentioned in my response to
Jordan Rose, perhaps this is where some test-case-writing
guidelines on the clang website might come in handy. (I'm not
particularly volunteering here; I expect there will be clang
veterans much better suited to this.)

Right, my point was not that it's hard to test, but that it's
dangerous: it adds to the list of things we need to be careful
about. I'm also concerned that we may have existing tests which
do something like that, which this change would neuter. If not,
then I'm happy to accept that this doesn't happen in practice.

Ok, I understand your point and it is impossible to cast-iron guarantee that
no-one in future will write a test in this way that won't false-pass: you
can't add a feature providing conditional checking without hitting this
problem. However, I have gone through the current test suite and of the
1734 tests that invoke clang with "-verify" and have some sort of expected-*
test, only 1 has an expected-* test inside a #if-style block (and this was
one that I had already picked up), so I think the idiom is not too common.

The PCH changes are unfortunate. Is there any way we can get
this to work properly for those cases? The relevant lines *are*
included in the TU (through an included file or PCH).

But my understanding is that the generated PCH doesn't include
the comments, so they can't be extracted from there. Someone
will be able to correct me here, or may have some other ideas
how to get around it.

Yes, I suspect this may not be possible. One alternative would be
to introduce another -verify mode which looks at comments within
#if 0'd out blocks.

My first version of the verify patch did do exactly this: I had
a -verify-all mode, but reached the conclusion that especially with the
ability to attach a line number to a diagnostic, it didn't really make much
real sense to do this. Even in the PCH test-cases, the diagnostics are
still properly exposed and handled. The disadvantage in my opinion of an
alternative mode, is you must remember when (or if) to use it and I'm not
sure that this is any better?

Were all the non-PCH test changes actually necessary? I would
find it very worrying if we are somehow losing expected-foo
checks in some cases.

For instance:

+// expected-error@+2 {{pasting formed '/*', an invalid preprocessing
token}}
#define COMM / ## *
-COMM // expected-error {{pasting formed '/*', an invalid preprocessing
token}}
+COMM

Why is this change necessary?

Because the preprocessor was unable to "see" the comment in this
situation.

This is exactly what I meant by "losing" expected-foo checks --
any new markers added in such places will have no effect. For what
it's worth, I think this is at least partially a feature of these
changes: they're allowing us to more directly test our
CommentHandler logic, which appears to be broken in various ways.
Nonetheless, we should fix the preprocessor rather than working
around it in the tests.

I do agree.

However, lets just quickly consider the changes that have been made to the
existing test-cases. They split into three main groups:

1. PCH tests

This group forms the majority: and in my opinion the changes do not affect
the validity or the purpose of these tests, nor do they weaken the tests
(for the reasons given below).

2. Tests involving #warning/#error

This covers a couple of tests, and actually, if I may be bold enough to say
so, are better tested now than before, since (as mentioned in my first post
on this), the following is a pass in the current implementation of -verify:

#error XXX // expected-error{{garbage!}}

The one case where this is not true, is that mentioned above, where
#error/#warning appears within an #if-style block: but the test-case in
question is revised to compensate for this.

3. Tests involving some sort of preprocessor error

There are only three of these, and as you say these are probably exposing
bugs then in the preprocessor / CommentHandler interface. To answer your
question below about how many tests might become XFAIL, it would be these
three (if any -- see my comments below in relation to that question).

I think the important thing to remember, and it applies to all three groups
above, is that clang still emits the diagnostics. If, for whatever reason,
the diagnostic check comments are not properly extracted, then the compiler
will emit an error due to unexpected diagnostics. The only case, as far as
I know, where this is not the case is your cruch-case above, i.e. where the
expected-* check is inside an #if-block and the diagnostic is also only
generated inside the #if-block.

However, I would argue that this is precisely what one would want and
expect, since code inside the #if-block will also only produce diagnostics
*if* the code is included. If we consider your cruch-case:

#if !__has_feature(X)
#error doesn't have X
#endif

What is this trying to achieve? My guess is that, in most cases, one simply
wants to stop the test-case if feature X is not available, since it would
probably mean the test cannot run in any case. In this situation, the
test-case doesn't need or want an expected-error to be included.

The alternative is to simply put the expected-error *outside* the #if block
to cause a test-case error if feature X *does* exist (in which case, why not
just invert the test?) or *inside* the #if block to effectively ignore the
#error (in which case why have it?!).

I'm afraid I honestly can't think of many real-world uses for such a
construction in a test-case. :o)

Alternatively, we could mark these tests as XFAIL and then, as a
separate thing, look at fixing the preprocessor, if that is what
needs doing.

What's the best approach, from your point of view?

The @line part seems to be uncontroversial -- if you can provide a
patch for just that part, we can work to get that checked in first.
I'm not particularly comfortable basing -verify on CommentHandler
while it's known to have dropped-comment bugs, so I'd prefer to get
it fixed before taking the CommentHandler change. Assuming that we're
happy to change all the PCH tests per your patch, how many tests would
we have to XFAIL?

If I'm honest, I'd actually rather fix the preprocessor and then seek to get
the whole patch committed as one. After all, the main purpose of the patch
was to enable the conditional testing aspect. The @line part came out as an
extension of that.

I'd like to suggest an alternative: I will add some extra test-cases that
explicitly test the new verify features, specifically to test in what way
expected-* checks are and are not exposed (using FileCheck). I would
suggest that rather than XFAILing the existing tests, since the
modifications to them do not invalidate the tests themselves, I will add
additional tests which do XFAIL at present and then can be switched to PASS
once the preprocessor is fixed (I would be happy to look into this).

Is this an acceptable proposal?

I'm also, later this evening, going to look into extending @line to
@file:line as per your suggestion.

Cheers
Andy

[Andy_Gibbs]

Hi,

I have posted two patches to the cfe-dev list which I hope should meet the concerns raised by Richard:

http://lists.cs.uiuc.edu/pipermail/cfe-dev/2012-June/022037.html
http://lists.cs.uiuc.edu/pipermail/cfe-dev/2012-June/022044.html

If these two patches can be approved and committed, then it is possible to leave all but the small selection of PCH test-cases and test-cases with #error/#warning alone.

I think there is no alternative but to move the position of some of the expected-* checks in the PCH test-cases. As I have suggested in other posts, I don't believe this adversely affects the function of these tests, so I hope that this should not pose a problem.

With regard to the #error/#warning test cases, this revolves around the fact that in clang, the line

#error some error message // with comment

will emit the diagnostic "some error message // with comment", i.e. the diagnostic includes the comment.

I've looked into the source to where the #error/#warning is handled, and there is the following comment:

// Read the rest of the line raw. We do this because we don't want macros
// to be expanded and we don't require that the tokens be valid preprocessing
// tokens. For example, this is allowed: "#warning ` 'foo". GCC does
// collapse multiple consequtive white space between tokens, but this isn't
// specified by the standard.

Therefore, it seems that this behaviour is intentional, not a bug. I would hesitate therefore to change the behaviour, unless it is generally felt it should be changed.

I also feel, that changed or not, the diagnostic check should still be on a separate line so that there can be no possibility that in checking the diagnostic, it checks against its own check text (since this is the problem in the current behaviour).

Therefore, would it be possible now to submit a new patch for consideration that provides the functionality as given in my original patch, but (due to the fixes in my separate patches mentioned at the top of this post) requires minimal changes to the pre-existing test-cases? If so, please let me know and I will put one together first thing tomorrow.

Many, many thanks!!

Andy