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

git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@261243 91177308-0d34-0410-b5e6-96231b3b80d8

diff --git a/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h b/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
index 55fd4b8880b54017ebdbb08f8d85a35bf5428f0a..7e6a3b9faafb77e02de7986a72831fe512e423a4 100644 (file)
--- a/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
+++ b/include/clang/StaticAnalyzer/Core/PathSensitive/CallEvent.h
@@ -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;

diff --git a/lib/StaticAnalyzer/Core/CallEvent.cpp b/lib/StaticAnalyzer/Core/CallEvent.cpp
index 59b90b5ce9876f9e7f629edf895ae518e8c85cb7..2c7b5302dd6f729ede82f8d26b9efb6bdbcbfa60 100644 (file)
--- a/lib/StaticAnalyzer/Core/CallEvent.cpp
+++ b/lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -678,9 +678,26 @@ ArrayRef<ParmVarDecl*> ObjCMethodCall::parameters() const {
   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::getContainingPseudoObjectExpr() const {
   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::getMessageKind() const {
 
     // 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::getMessageKind() const {
   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 {

diff --git a/test/Analysis/properties.m b/test/Analysis/properties.m
index 4fdbb69d87a19c1f6e53505cc2bd509a3b432817..d92f1a1a7e8d6ff4c3b0b56ec4352fae3c0b6e58 100644 (file)
--- a/test/Analysis/properties.m
+++ b/test/Analysis/properties.m
@@ -238,6 +238,75 @@ void testConsistencyAssign(Person *p) {
 }
 @end
 
+//------
+// Setter ivar invalidation.
+//------
+
+@interface ClassWithSetters
+// Note: These properties have implicit @synthesize implementations to be
+// backed with ivars.
+@property (assign) int propWithIvar1;
+@property (assign) int propWithIvar2;
+
+@property (retain) NSNumber *retainedProperty;
+
+@end
+
+@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.
+@property (assign) int propInOther;
+@end
+
+@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
+@end
+
 #if !__has_feature(objc_arc)
 void testOverrelease(Person *p, int coin) {
   switch (coin) {