This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] For now model that OSDynamicCast never returns 0
ClosedPublic

Authored by george.karpenkov on Oct 23 2018, 5:24 PM.

Details

Summary

Previously, OSDynamicCast was modeled as an identity.

This is not correct: the output of OSDynamicCast may be zero even if the input was not zero (if the class is not of desired type), and thus the modeling led to false positives.
This patch adds another overapproximation: assume that OSDynamicCast never returns zero.
What we probably really want is an implication x != 0 /\ a = OSDynamicCast(x, C) => a != 0,
but that is hard to assume with our interval solver (though could be done with a table lookup hack).
Modeling checking the class hierarchy is probably not practical.

This patch required a substantial refactoring of canEval infrastructure, as now it can return different function summaries, and not just true/false.

rdar://45497400

Diff Detail

Event Timeline

NoQ accepted this revision.Oct 24 2018, 12:09 PM

Makes sense!

This revision is now accepted and ready to land.Oct 24 2018, 12:09 PM

Or maybe it does not: we should still be able to catch cases where OSDynamicCast fails.

NoQ accepted this revision.Oct 25 2018, 2:18 PM

On second thought, i actually think this should have been done as part of the dynamic type checker. This functionality doesn't have much to do with retain counts, but for more precise modeling some sort of reasoning about dynamic types behind pointers would be absolutely necessary.

I guess let's land this improvement, unless eager splits result in too many false alarms, and move to the right place later.

This revision was automatically updated to reflect the committed changes.
NoQ added inline comments.Oct 30 2018, 5:23 PM
cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
813–814 ↗(On Diff #171223)

If assume fails and yields a null state, you shouldn't transition into that state; it won't generate a sink but will instead transition into the original state (with the original program point but with checker tag).

cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
813–814 ↗(On Diff #171223)

That does not seem to be true: addTransition sets Tag to nullptr by default, and then addTransitionImpl has the following lines:

if (!State || (State == Pred->getState() && !Tag && !MarkAsSink))
      return Pred;

So if we create a transition to an empty state, that simply returns the original state, which caches out.

NoQ added inline comments.Oct 30 2018, 7:34 PM
cfe/trunk/lib/StaticAnalyzer/Checkers/RetainCountChecker/RetainCountChecker.cpp
813–814 ↗(On Diff #171223)

Aha, aha, that's right, i was wrong and confused it with generateNonFatalErrorNode().

*runs away and blames API*