On Jun 5, 2014, at 5:16 , Anders Rönnholm <[email protected]> wrote:
> Hi Jordan,
>
> New patch and some comments.
>
> +inline void inline_func3(int x) {
> + var = 77 / x;
> +}
> +void ok_inline(int x) {
> + var = 77 / x;
> + inline_func3(x);
> + if (x==0){}
> +}
>
>> This is an example where we should still be warning. Why aren't we?
>>
> It doesn't warn here because blockid is in the value and not the key as it
> should. I have made a set of symbol-block-frame as you suggested and that
> fixes this problem.
>
>
>>> +// RUN: %clang_cc1 -analyze -analyzer-checker=core.DivideZero2
>>> -analyzer-output=text -verify %s
>>>
>> We should never be running path-sensitive checks without the rest of the
>> core checkers.
>>
> There is a small problem here. It looks like the DivideZero checker makes the
> assumtion that because it can't see that a variable is zero then it's not
> zero so all variables in our tests have the range reg_$0<x> : { [-2147483648,
> -1], [1, 2147483647] }. I have removed the range check in my checker to make
> it work otherwise i would have to change the DivideZero checker.
Hm. The important thing is that a variable is zero before the division; in the
absence of this checker, it's a reasonable assumption that it's zero if we made
it past the division. Would it make sense to step back through ExplodedNodes
instead until we found a ProgramPoint before the division statement itself, and
then look at the value of the variable in that state?
>>> + const ZeroState *ZS = C.getState()->get<DivZeroMap>(SR);
>>> + return (ZS && *ZS == ZeroState(C.getBlockID(), C.getStackFrame()));
>>>
>> ...now I'm wondering about the wisdom of using a map for this. What if the
>> same symbol is constrained in two stack frames? Would it make more sense to
>> keep around a set of symbol-block-frame triples, rather than a map from
>> symbols to block/frame pairs?
>>
>> Or can this not happen because a value can only be used once along a certain
>> path before it is constrained to be non-zero?
>>
> Changed to a set of symbol-block-frame triples to make the above example work.
>
>
>>> +def DivZeroChecker2 : Checker<"DivideZero2">,
>>> + HelpText<"Check for division by variable that is later compared against
>>> 0. Either the comparison is useless or there is division by zero.">,
>>> + DescFile<"DivZeroChecker2.cpp">;
>>> +
>>>
>> I'm not sure this is really a core checker, since it's safe to run the
>> analyzer without it. But we don't have a category for "useful C checks that
>> should be on by default" yet. Anton was looking at reorganizing the
>> categories, but the problem is that everything outside of "alpha" is
>> something people expect to be able to rely on when using scan-build.
>>
>> Any ideas for this particular checker, or for the general issue?
>
> I ok with having it as an alpha, or something else. I just put it in core
> since the other DivideZero checker is there.
Let's put it in alpha for now.
Code comments:
+class DivZeroChecker2
+ : public Checker<check::PreStmt<BinaryOperator>, check::BranchCondition,
+ check::EndFunction> {
Since we're not doing the normal divide-by-zero check in this checker, let's
name it something else. "TestAfterDivideChecker", maybe?
+ DivZeroMapTy DivZeroes = C.getState()->get<DivZeroMap>();
+ if (DivZeroes.isEmpty())
+ return false;
+
+ for (llvm::ImmutableSet<ZeroState>::iterator I = DivZeroes.begin(),
+ E = DivZeroes.end();
+ I != E; ++I) {
+ if (*I == ZeroState(SR, C.getBlockID(), C.getStackFrame()))
+ return true;
+ }
+ return false;
This whole thing can be simplified quite a bit:
ZeroState ZS(SR, C.getBlockID(), C.getStackFrame());
return C.getState()->contains<DivZeroMap>(ZS);
...and other than that it's looking pretty good! Has this checker found any
real bugs that you know of? (Does PVS-Studio have a similar checker?)
Jordan_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
