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

[Jordan Rose]

On May 15, 2012, at 15:21, Anna Zaks wrote:

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

Hm. debug.StateInspection? debug.Reflection? debug.Introspection?


> 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();

Hm. Wasn't sure how that interacted with evalCall, but you're right.


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


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


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

Okay, I'll stick to the systematically obvious ones, then (those that have a WARN macro or similar).

Thanks!
Jordy