[PATCH] D31840: [analyzer] Fix crash on access to property

2017-04-12 Thread Artem Dergachev via Phabricator via cfe-commits 
NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.

Yep, looks good now, thanks!


Repository:
  rL LLVM

https://reviews.llvm.org/D31840



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D31840: [analyzer] Fix crash on access to property

2017-04-10 Thread Alexander Shaposhnikov via Phabricator via cfe-commits 
alexshap updated this revision to Diff 94713.
alexshap added a comment.

1. update the patch following NoQ@ suggestion
2. rerun the tests: make check-all - they are green


Repository:
  rL LLVM

https://reviews.llvm.org/D31840

Files:
  lib/StaticAnalyzer/Core/CallEvent.cpp
  test/Analysis/properties.m


Index: test/Analysis/properties.m
===
--- test/Analysis/properties.m
+++ test/Analysis/properties.m
@@ -987,5 +987,21 @@
 }
 
 @end
+
+@interface Wrapper
+@property(nonatomic, readonly) int value;
+@end
+
+@implementation Wrapper
+@synthesize value;
+@end
+
+void testNoCrashWhenAccessPropertyAndThereAreNoDirectBindingsAtAll() {
+   union {
+Wrapper *wrapper;
+   } u = { 0 };
+   [u.wrapper value];
+}
+
 #endif // non-ARC
 
Index: lib/StaticAnalyzer/Core/CallEvent.cpp
===
--- lib/StaticAnalyzer/Core/CallEvent.cpp
+++ lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -695,13 +695,15 @@
   if (const ObjCPropertyDecl *PropDecl = getAccessedProperty()) {
 if (const ObjCIvarDecl *PropIvar = PropDecl->getPropertyIvarDecl()) {
   SVal IvarLVal = getState()->getLValue(PropIvar, getReceiverSVal());
-  const MemRegion *IvarRegion = IvarLVal.getAsRegion();
-  ETraits->setTrait(
+  if (const MemRegion *IvarRegion = IvarLVal.getAsRegion()) {
+ETraits->setTrait(
   IvarRegion,
   RegionAndSymbolInvalidationTraits::TK_DoNotInvalidateSuperRegion);
-  ETraits->setTrait(IvarRegion,
-RegionAndSymbolInvalidationTraits::TK_SuppressEscape);
-  Values.push_back(IvarLVal);
+ETraits->setTrait(
+  IvarRegion,
+  RegionAndSymbolInvalidationTraits::TK_SuppressEscape);
+Values.push_back(IvarLVal);
+  }
   return;
 }
   }


Index: test/Analysis/properties.m
===
--- test/Analysis/properties.m
+++ test/Analysis/properties.m
@@ -987,5 +987,21 @@
 }
 
 @end
+
+@interface Wrapper
+@property(nonatomic, readonly) int value;
+@end
+
+@implementation Wrapper
+@synthesize value;
+@end
+
+void testNoCrashWhenAccessPropertyAndThereAreNoDirectBindingsAtAll() {
+   union {
+Wrapper *wrapper;
+   } u = { 0 };
+   [u.wrapper value];
+}
+
 #endif // non-ARC
 
Index: lib/StaticAnalyzer/Core/CallEvent.cpp
===
--- lib/StaticAnalyzer/Core/CallEvent.cpp
+++ lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -695,13 +695,15 @@
   if (const ObjCPropertyDecl *PropDecl = getAccessedProperty()) {
 if (const ObjCIvarDecl *PropIvar = PropDecl->getPropertyIvarDecl()) {
   SVal IvarLVal = getState()->getLValue(PropIvar, getReceiverSVal());
-  const MemRegion *IvarRegion = IvarLVal.getAsRegion();
-  ETraits->setTrait(
+  if (const MemRegion *IvarRegion = IvarLVal.getAsRegion()) {
+ETraits->setTrait(
   IvarRegion,
   RegionAndSymbolInvalidationTraits::TK_DoNotInvalidateSuperRegion);
-  ETraits->setTrait(IvarRegion,
-RegionAndSymbolInvalidationTraits::TK_SuppressEscape);
-  Values.push_back(IvarLVal);
+ETraits->setTrait(
+  IvarRegion,
+  RegionAndSymbolInvalidationTraits::TK_SuppressEscape);
+Values.push_back(IvarLVal);
+  }
   return;
 }
   }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D31840: [analyzer] Fix crash on access to property

2017-04-10 Thread Artem Dergachev via Phabricator via cfe-commits 
NoQ added a comment.

This defensive check looks reasonable regardless of how well we model unions 
(we don't). Thanks for the patch!




Comment at: lib/StaticAnalyzer/Core/CallEvent.cpp:706
+  }
   Values.push_back(IvarLVal);
   return;

I believe there's not much to `push_back` here when we don't have the region. 
So this can go under the `if(){}` as well.


Repository:
  rL LLVM

https://reviews.llvm.org/D31840



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D31840: [analyzer] Fix crash on access to property

2017-04-10 Thread Alexander Shaposhnikov via Phabricator via cfe-commits 
alexshap created this revision.
alexshap created this object with visibility "All Users".

Preliminary context about unions:
at the moment for unions the static analyzer creates default bindings (as a 
compoundVal).
If a union has only one field x which is initialized to zero this is handled as 
an unknown val.

In the newly added test case a union contains a pointer to an objc object.
When the static analyzer (during the path sensitive analysis) processes the 
call of the objc method, 
in this particular case it will fall back on defaultEvalCall / 
conservativeEvalCall. The sub calls of invalidateRegions / 
getExtraInvalidatedValues assume that the returned MemRegion for the associated 
ivar is not zero, 
which is not the case here. Similarly to CXXInstanceCall we add the 
corresponding check to ObjCMethodCall.


Repository:
  rL LLVM

https://reviews.llvm.org/D31840

Files:
  lib/StaticAnalyzer/Core/CallEvent.cpp
  test/Analysis/properties.m


Index: test/Analysis/properties.m
===
--- test/Analysis/properties.m
+++ test/Analysis/properties.m
@@ -987,5 +987,21 @@
 }
 
 @end
+
+@interface Wrapper
+@property(nonatomic, readonly) int value;
+@end
+
+@implementation Wrapper
+@synthesize value;
+@end
+
+void testNoCrashWhenAccessPropertyAndThereAreNoDirectBindingsAtAll() {
+   union {
+Wrapper *wrapper;
+   } u = { 0 };
+   [u.wrapper value];
+}
+
 #endif // non-ARC
 
Index: lib/StaticAnalyzer/Core/CallEvent.cpp
===
--- lib/StaticAnalyzer/Core/CallEvent.cpp
+++ lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -695,12 +695,14 @@
   if (const ObjCPropertyDecl *PropDecl = getAccessedProperty()) {
 if (const ObjCIvarDecl *PropIvar = PropDecl->getPropertyIvarDecl()) {
   SVal IvarLVal = getState()->getLValue(PropIvar, getReceiverSVal());
-  const MemRegion *IvarRegion = IvarLVal.getAsRegion();
-  ETraits->setTrait(
+  if (const MemRegion *IvarRegion = IvarLVal.getAsRegion()) {
+ETraits->setTrait(
   IvarRegion,
   RegionAndSymbolInvalidationTraits::TK_DoNotInvalidateSuperRegion);
-  ETraits->setTrait(IvarRegion,
-RegionAndSymbolInvalidationTraits::TK_SuppressEscape);
+ETraits->setTrait(
+  IvarRegion,
+  RegionAndSymbolInvalidationTraits::TK_SuppressEscape);
+  }
   Values.push_back(IvarLVal);
   return;
 }


Index: test/Analysis/properties.m
===
--- test/Analysis/properties.m
+++ test/Analysis/properties.m
@@ -987,5 +987,21 @@
 }
 
 @end
+
+@interface Wrapper
+@property(nonatomic, readonly) int value;
+@end
+
+@implementation Wrapper
+@synthesize value;
+@end
+
+void testNoCrashWhenAccessPropertyAndThereAreNoDirectBindingsAtAll() {
+   union {
+Wrapper *wrapper;
+   } u = { 0 };
+   [u.wrapper value];
+}
+
 #endif // non-ARC
 
Index: lib/StaticAnalyzer/Core/CallEvent.cpp
===
--- lib/StaticAnalyzer/Core/CallEvent.cpp
+++ lib/StaticAnalyzer/Core/CallEvent.cpp
@@ -695,12 +695,14 @@
   if (const ObjCPropertyDecl *PropDecl = getAccessedProperty()) {
 if (const ObjCIvarDecl *PropIvar = PropDecl->getPropertyIvarDecl()) {
   SVal IvarLVal = getState()->getLValue(PropIvar, getReceiverSVal());
-  const MemRegion *IvarRegion = IvarLVal.getAsRegion();
-  ETraits->setTrait(
+  if (const MemRegion *IvarRegion = IvarLVal.getAsRegion()) {
+ETraits->setTrait(
   IvarRegion,
   RegionAndSymbolInvalidationTraits::TK_DoNotInvalidateSuperRegion);
-  ETraits->setTrait(IvarRegion,
-RegionAndSymbolInvalidationTraits::TK_SuppressEscape);
+ETraits->setTrait(
+  IvarRegion,
+  RegionAndSymbolInvalidationTraits::TK_SuppressEscape);
+  }
   Values.push_back(IvarLVal);
   return;
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits