Page MenuHomePhabricator

[analyzer] Skip Pre/Post handlers for ObjC calls when receiver is nil.
ClosedPublic

Authored by dcoughlin on Aug 18 2015, 3:39 PM.

Details

Summary

[analyzer] Skip Pre/Post handlers for ObjC calls when receiver is nil.

In Objective-C, method calls with nil receivers are essentially no-ops. They
do not fault (although the returned value may be garbage depending on the
declared return type and architecture). Programmers are aware of this
behavior and will complain about a false alarm when the analyzer
diagnoses API violations for method calls when the receiver is known to
be nil.

Rather than require each individual checker to be aware of this behavior
and suppress a warning when the receiver is nil, this patch
changes ExprEngineObjC so that VisitObjCMessage skips calling checker
pre/post handlers when the receiver is definitely nil. Instead, it adds a
new event, ObjCMessageNil, that is only called in that case.

The CallAndMessageChecker explicitly cares about this case, so I've changed it
to add a callback for ObjCMessageNil and moved the logic in PreObjCMessage
that handles nil receivers to the new callback.

rdar://problem/18092611

Diff Detail

Repository
rL LLVM

Event Timeline

dcoughlin updated this revision to Diff 32462.Aug 18 2015, 3:39 PM
dcoughlin retitled this revision from to [analyzer] Skip Pre/Post handlers for ObjC calls when receiver is nil..
dcoughlin updated this object.
dcoughlin added a subscriber: cfe-commits.

I think this is a great refinement overall, with a few minor nits. It also isn't clear what the test does.

include/clang/StaticAnalyzer/Core/CheckerManager.h
577 ↗(On Diff #32462)

Can we break with tradition here and add a documentation comment?

lib/StaticAnalyzer/Checkers/CallAndMessageChecker.cpp
64 ↗(On Diff #32462)

Can we break with tradition here and actually add a small documentation comment?

test/Analysis/NSContainers.m
297 ↗(On Diff #32462)

Can we add a comment above this test that makes it clear what this test is doing? It is not obvious at all from basic inspection.

There's also no matching 'no-warning' or 'expected-warning', so it is not clear at all what it is testing. Having the comment clearly say what the test is testing will make it more resilient to somebody accidentally deleting it.

xazax.hun edited edge metadata.Aug 18 2015, 4:40 PM

Looks good to me. There are some minor nits inline.

include/clang/StaticAnalyzer/Core/CheckerManager.h
96 ↗(On Diff #32462)

I do not really like the name ObjCCheckerKind, because it is not kind of an Obj-C related checker. It is the kind of the visit of the message expression. Maybe ObjCMessageVisitKind? Or just MessageVisitKind to be a bit shorter?

lib/StaticAnalyzer/Core/ExprEngineObjC.cpp
153 ↗(On Diff #32462)

The old code had a comment about merging two cases and a reference to a rdar. Is that rdar already fixed? Maybe it would be good to preserve the at least the first part of the commend?

dcoughlin updated this revision to Diff 32638.Aug 19 2015, 4:25 PM
dcoughlin updated this object.
dcoughlin edited edge metadata.

Update patch to address review comments. I've also updated the CheckerDocumentation checker to document the new ObjCMessageNil callback.

This version of the patch also restores the pre-patch behavior of assuming that the receiver is non-nil after a method call when the receiver is not definitely nil. I've added a test for this behavior (objc-message.m) and a comment based on a discussion with Jordan that explains why that behavior is important.

dcoughlin marked 5 inline comments as done.Aug 19 2015, 4:30 PM
dcoughlin added inline comments.
include/clang/StaticAnalyzer/Core/CheckerManager.h
96 ↗(On Diff #32462)

I've changed this to ObjCMessageVisitKind, as you suggested.

lib/StaticAnalyzer/Core/ExprEngineObjC.cpp
153 ↗(On Diff #32462)

Thanks for pointing this out! This revealed a deeper issue where the previous patch did not drop the non-nil flow after a method call when it was unknown whether the receiver was nil. I've fixed the behavior, added a comment explaining why it is needed, and added a test for it.

xazax.hun added inline comments.Aug 20 2015, 11:26 AM
lib/StaticAnalyzer/Core/CheckerManager.cpp
237 ↗(On Diff #32638)

nit: remove the break after the return.

lib/StaticAnalyzer/Core/ExprEngineObjC.cpp
197 ↗(On Diff #32638)

Aren't we dropping the nil flow here instead of the non-nil? If that's the case, the comment should reflect that.

dcoughlin marked 2 inline comments as done.Aug 20 2015, 11:32 AM
dcoughlin added inline comments.
lib/StaticAnalyzer/Core/ExprEngineObjC.cpp
197 ↗(On Diff #32638)

Yes, you're right. I'll update the comment.

test/Analysis/objc-message.m
25 ↗(On Diff #32638)

This comment is wrong too (we drop the *nil* flow). I'll update it.

dcoughlin updated this revision to Diff 32864.Aug 21 2015, 1:51 PM

Update comments to correct nil/non-nil mistakes.

dcoughlin marked an inline comment as done.Aug 21 2015, 1:56 PM
xazax.hun accepted this revision.Aug 25 2015, 9:51 AM
xazax.hun edited edge metadata.

Looks good to me.

This revision is now accepted and ready to land.Aug 25 2015, 9:51 AM
dcoughlin updated this revision to Diff 33836.Sep 2 2015, 11:51 AM
dcoughlin edited edge metadata.

I've updated ExprEngine::VisitObjCMessage to create a pre-statement program point node for the ObjCMessageExpr when the receiver is definitely nil and changed the MessageNil checker handlers to run on a post-statement program point node. This makes sure we always have both a pre- and a post-statement program point for the message expression -- an invariant the path diagnostic machinery relies upon.

This revision was automatically updated to reflect the committed changes.