[cfe-commits] [PATCH] Add analyzer_assert for regression tests

[Anna Zaks]

A couple of comments:

1) Maybe we could make the name of the checker more generic as it can easily be extended to provide info other that constraints.
2) I think C.addTransition() is a no-op in this case, so the call can be removed:
+  // These checks should have no effect on the surrounding environment
+  // (globals should not be evaluated, etc).
+  C.addTransition();

3) Why not support these. They will just result in several diagnostics.
+  // A specific instantiation of an inlined function may have more constrained
+  // values than can generally be assumed.
+  if (LC->getParent() != 0)
+    return true;

4) Should this be an assert?
+        else
+          Msg = "INVALID";

5) This one is the most important. I'd only change the test cases where you are absolutely sure that the constraint solver is the only thing that's being tested there and leave the rest. It is OK to have both types of tests and we don't want to decrease our coverage of testing for actual warnings. I see this enhancement as simplifying addition of this type of tests in the future; we don't need to convert all our tests to using it.

Otherwise looks great!

Thanks,
Anna.

On May 15, 2012, at 9:15 AM, Jordan Rose wrote:

> All right, version 2, with your suggestions taken into account. What do you think?
> 
> I went with clang_analyzer_eval because it's a little shorter, but it may be a bit less clear (does it eval integers as well?). The output is in all caps to try to avoid conflicts with legitimate warnings.
> 
> Merging the constraint and taint testers would be possible, but it's something we can do later.
> 
> In my working branch I've identified these files as systematically using null dereferencing and malloc leaks purely for error reporting:
> 
> additive-folding-range-constraints.c
> additive-folding.c
> bstring.c
> constant-folding.c
> ptr-arith.c
> string-fail.c
> string.c (in this patch)
> 
> (Many of those files were actually originally created or reformatted by me, so that's not surprising.)
> 
> There are several more files where the policy is not as clear, but which should probably still be using this in at least one part of the test:
> 
> array-struct-region.c (in this patch)
> base-init.cpp
> dynamic-cast.cpp (some null derefs remain, since it's about pointers anyway)
> global-region-invalidation.c
> initializer.cpp
> inline.c
> method-call-intra-p.cpp
> method-call.cpp
> objc-arc.m
> reference.cpp
> 
> Finally, some of the "ps" miscellaneous files also use null dereferencing and deadcode warnings, but since a lot of those are taken verbatim from PRs and Radars it's possible the particular form is what triggered the past bug. I left those alone, but I'm planning to go ahead and change the other two sets above as well.
> 
> I'm a little concerned about blame churn for the less-systematic tests, but it still seems like the right thing to do moving forward.
> 
> Jordy
> 
> <ConstraintTest.patch>
> 
> // Any signature with an integral type will do.
> // 'bool' would be canonical in C++.
> void clang_analyzer_eval(int);
> 
> void testUnsigned (unsigned a) {
>  if (a > 0)
>    return;
> 
>  clang_analyzer_eval(a == 0); // expected-warning{{TRUE}}
> }
> 
> void testSigned (int a) {
>  if (a > 0)
>    return;
> 
>  clang_analyzer_eval(a == 0); // expected-warning{{UNKNOWN}}
> }
> 
> 
> On May 14, 2012, at 19:40, Anna Zaks wrote:
> 
>> Jordy,
>> 
>> I think having something like this is a great idea.
>> 
>>> A downside I am realizing is that silent success means the output looks the same whether debug.Asserts is on or off...not sure yet what to do about that.
>> 
>> How about having something like "clang_analyzer_check_expr()" that always produces a warning which tells us about the expression's state, like "true", "false", "unknown"?
>> 
>> I've added a taint debug checker, which serves similar purpose. We could also use this to check taint or other info by appending another string to the warning message "true (tainted:true)", but possibly with better markup.
>> 
>> A few minor comments:
>> - I'd prefix these asserts with "clang_analyzer".
>> - We should check for undefined here. This checker is not guaranteed to be run along with other checkers.
>> 
>> Cheers,
>> Anna.
>