r261243 - [analyzer] Improve modeling of ObjC synthesized property setters.

[Devin Coughlin via cfe-commits]

Author: dcoughlin
Date: Thu Feb 18 13:13:30 2016
New Revision: 261243

URL: http://llvm.org/viewvc/llvm-project?rev=261243&view=rev
Log:
[analyzer] Improve modeling of ObjC synthesized property setters.

When modeling a call to a setter for a property that is synthesized to be
backed by an instance variable, don't invalidate the entire instance
but rather only the storage for the updated instance variable itself.

This still doesn't model the effect of the setter completely. It doesn't
bind the set value to the ivar storage location because doing so would cause
the set value to escape, removing valuable diagnostics about potential
leaks of the value from the retain count checker.

Modified:
    cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
    cfe/trunk/lib/StaticAnalyzer/Core/CallEvent.cpp
    cfe/trunk/test/Analysis/properties.m

Modified: cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h?rev=261243&r1=261242&r2=261243&view=diff
==============================================================================
--- cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h (original)
+++ cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h Thu Feb 18 13:13:30 2016
@@ -957,6 +957,11 @@ public:
     llvm_unreachable("Unknown message kind");
   }
 
+  // Returns the property accessed by this method, either explicitly via
+  // property syntax or implicitly via a getter or setter method. Returns
+  // nullptr if the call is not a prooperty access.
+  const ObjCPropertyDecl *getAccessedProperty() const;
+
   RuntimeDefinition getRuntimeDefinition() const override;
 
   bool argumentsMayEscape() const override;

Modified: cfe/trunk/lib/StaticAnalyzer/Core/CallEvent.cpp
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/CallEvent.cpp?rev=261243&r1=261242&r2=261243&view=diff
==============================================================================
--- cfe/trunk/lib/StaticAnalyzer/Core/CallEvent.cpp (original)
+++ cfe/trunk/lib/StaticAnalyzer/Core/CallEvent.cpp Thu Feb 18 13:13:30 2016
@@ -678,9 +678,26 @@ ArrayRef<ParmVarDecl*> ObjCMethodCall::p
   return D->parameters();
 }
 
-void
-ObjCMethodCall::getExtraInvalidatedValues(ValueList &Values,
-                  RegionAndSymbolInvalidationTraits *ETraits) const {
+void ObjCMethodCall::getExtraInvalidatedValues(
+    ValueList &Values, RegionAndSymbolInvalidationTraits *ETraits) const {
+
+  // If the method call is a setter for property known to be backed by
+  // an instance variable, don't invalidate the entire receiver, just
+  // the storage for that instance variable.
+  if (const ObjCPropertyDecl *PropDecl = getAccessedProperty()) {
+    if (const ObjCIvarDecl *PropIvar = PropDecl->getPropertyIvarDecl()) {
+      SVal IvarLVal = getState()->getLValue(PropIvar, getReceiverSVal());
+      const MemRegion *IvarRegion = IvarLVal.getAsRegion();
+      ETraits->setTrait(
+          IvarRegion,
+          RegionAndSymbolInvalidationTraits::TK_DoNotInvalidateSuperRegion);
+      ETraits->setTrait(IvarRegion,
+                        RegionAndSymbolInvalidationTraits::TK_SuppressEscape);
+      Values.push_back(IvarLVal);
+      return;
+    }
+  }
+
   Values.push_back(getReceiverSVal());
 }
 
@@ -740,6 +757,18 @@ const PseudoObjectExpr *ObjCMethodCall::
   return ObjCMessageDataTy::getFromOpaqueValue(Data).getPointer();
 }
 
+static const Expr *
+getSyntacticFromForPseudoObjectExpr(const PseudoObjectExpr *POE) {
+  const Expr *Syntactic = POE->getSyntacticForm();
+
+  // This handles the funny case of assigning to the result of a getter.
+  // This can happen if the getter returns a non-const reference.
+  if (const BinaryOperator *BO = dyn_cast<BinaryOperator>(Syntactic))
+    Syntactic = BO->getLHS();
+
+  return Syntactic;
+}
+
 ObjCMessageKind ObjCMethodCall::getMessageKind() const {
   if (!Data) {
 
@@ -749,12 +778,7 @@ ObjCMessageKind ObjCMethodCall::getMessa
 
     // Check if parent is a PseudoObjectExpr.
     if (const PseudoObjectExpr *POE = dyn_cast_or_null<PseudoObjectExpr>(S)) {
-      const Expr *Syntactic = POE->getSyntacticForm();
-
-      // This handles the funny case of assigning to the result of a getter.
-      // This can happen if the getter returns a non-const reference.
-      if (const BinaryOperator *BO = dyn_cast<BinaryOperator>(Syntactic))
-        Syntactic = BO->getLHS();
+      const Expr *Syntactic = getSyntacticFromForPseudoObjectExpr(POE);
 
       ObjCMessageKind K;
       switch (Syntactic->getStmtClass()) {
@@ -790,6 +814,27 @@ ObjCMessageKind ObjCMethodCall::getMessa
   return static_cast<ObjCMessageKind>(Info.getInt());
 }
 
+const ObjCPropertyDecl *ObjCMethodCall::getAccessedProperty() const {
+  // Look for properties accessed with property syntax (foo.bar = ...)
+  if ( getMessageKind() == OCM_PropertyAccess) {
+    const PseudoObjectExpr *POE = getContainingPseudoObjectExpr();
+    assert(POE && "Property access without PseudoObjectExpr?");
+
+    const Expr *Syntactic = getSyntacticFromForPseudoObjectExpr(POE);
+    auto *RefExpr = cast<ObjCPropertyRefExpr>(Syntactic);
+
+    if (RefExpr->isExplicitProperty())
+      return RefExpr->getExplicitProperty();
+  }
+
+  // Look for properties accessed with method syntax ([foo setBar:...]).
+  const ObjCMethodDecl *MD = getDecl();
+  if (!MD || !MD->isPropertyAccessor())
+    return nullptr;
+
+  // Note: This is potentially quite slow.
+  return MD->findPropertyDecl();
+}
 
 bool ObjCMethodCall::canBeOverridenInSubclass(ObjCInterfaceDecl *IDecl,
                                              Selector Sel) const {

Modified: cfe/trunk/test/Analysis/properties.m
URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/properties.m?rev=261243&r1=261242&r2=261243&view=diff
==============================================================================
--- cfe/trunk/test/Analysis/properties.m (original)
+++ cfe/trunk/test/Analysis/properties.m Thu Feb 18 13:13:30 2016
@@ -238,6 +238,75 @@ void testConsistencyAssign(Person *p) {
 }
 @end
 
+//------
+// Setter ivar invalidation.
+//------
+
+ at interface ClassWithSetters
+// Note: These properties have implicit @synthesize implementations to be
+// backed with ivars.
+ at property (assign) int propWithIvar1;
+ at property (assign) int propWithIvar2;
+
+ at property (retain) NSNumber *retainedProperty;
+
+ at end
+
+ at interface ClassWithSetters (InOtherTranslationUnit)
+// The implementation of this property is in another translation unit.
+// We don't know whether it is backed by an ivar or not.
+ at property (assign) int propInOther;
+ at end
+
+ at implementation ClassWithSetters
+- (void) testSettingPropWithIvarInvalidatesExactlyThatIvar; {
+  _propWithIvar1 = 1;
+  _propWithIvar2 = 2;
+  self.propWithIvar1 = 66;
+
+  // Calling the setter of a property backed by the instance variable
+  // should invalidate the storage for the instance variable but not
+  // the rest of the receiver. Ideally we would model the setter completely
+  // but doing so would cause the new value to escape when it is bound
+  // to the ivar. This would cause bad false negatives in the retain count
+  // checker. (There is a test for this scenario in
+  // testWriteRetainedValueToRetainedProperty below).
+  clang_analyzer_eval(_propWithIvar1 == 66); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(_propWithIvar2 == 2);  // expected-warning{{TRUE}}
+
+  _propWithIvar1 = 1;
+  [self setPropWithIvar1:66];
+
+  clang_analyzer_eval(_propWithIvar1 == 66); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(_propWithIvar2 == 2);  // expected-warning{{TRUE}}
+}
+
+- (void) testSettingPropWithoutIvarInvalidatesEntireInstance; {
+  _propWithIvar1 = 1;
+  _propWithIvar2 = 2;
+  self.propInOther = 66;
+
+  clang_analyzer_eval(_propWithIvar1 == 66); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(_propWithIvar2 == 2);  // expected-warning{{UNKNOWN}}
+
+  _propWithIvar1 = 1;
+  _propWithIvar2 = 2;
+  [self setPropInOther:66];
+
+  clang_analyzer_eval(_propWithIvar1 == 66); // expected-warning{{UNKNOWN}}
+  clang_analyzer_eval(_propWithIvar2 == 2);  // expected-warning{{UNKNOWN}}
+}
+
+#if !__has_feature(objc_arc)
+- (void) testWriteRetainedValueToRetainedProperty; {
+  NSNumber *number = [[NSNumber alloc] initWithInteger:5]; // expected-warning {{Potential leak of an object stored into 'number'}}
+
+  // Make sure we catch this leak.
+  self.retainedProperty = number;
+}
+#endif
+ at end
+
 #if !__has_feature(objc_arc)
 void testOverrelease(Person *p, int coin) {
   switch (coin) {