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

[Ted Kremenek]

On May 15, 2012, at 1:19 PM, Jordan Rose <jediknil at belkadan.com> wrote:

>> 
>> 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;
> 
> I actually came across this when converting existing tests. Here's a trivial example where a constraint is generally unknowable but is known when inlined.
> 
> void check(void *x) {
>  clang_analyzer_check(x != 0); // expected-warning{{UNKNOWN}}
> }
> 
> void test() {
>  check(0);
> }
> 
> For the case of TRUE or FALSE there would be no change, but UNKNOWN is a problem. (In the previous iteration, these were separated into analyzer_assert and analyzer_assert_unknown, but I don't think it's necessary to check that something is true generally /and/ in a specific inlined case.)

We can strike a compromise.  For the general case, I agree with Jordan here that we shouldn't including inlined functions here.  We're just trying to test the base functionality of the solver.  If we care about inlining, we can pretty much just write  tests as we did before (and look for actual real warnings, not just cases of testing the constraint solver).  The compromise I am thinking of is that we could also possibly have another version of the checker (similar to how CheckSecuritySyntaxOnly.cpp is factored) that enables this checking for inlined code as well.  We would then only use that judiciously in test cases that specifically wanted to test inlining, but we would need to be ready to update those tests frequently whenever we change the inlining heuristics.

> 
> 
>> 4) Should this be an assert?
>> +        else
>> +          Msg = "INVALID";
> 
> I'm not sure. The analyzer will probably grind to a halt long before it gets here, but if someone ever /does/ make a logical mistake on the infrastructure that results in this situation (testing a constraint results in two infeasible states, even with a feasible base state), it might be nicer /not/ to stop analysis here. Since these are intended to go in regression tests, the fact that the test was known to work before might be useful knowledge, in which case it should go on being tested.
> 
> I don't have a strong opinion here, but it doesn't seem to cost much. All we're doing is emitting a warning, not changing the state at all.

I'm fine with printing out a message.  This is for testing, and an error message is usually easier to diagnose than a crash.  At least with an error message we know *where* in the code the condition was not satisfied.  With a crash, we need to go into the debugger to figure that out.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20120522/84067632/attachment.html>