[PATCH] D26159: [analyzer] MacOSXAPIChecker: Improve warning messages for __block vars in dispatch_once().

[Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL285637: [analyzer] MacOSXAPIChecker: Improve warnings for 
__block vars in dispatch_once. (authored by dergachev).

Changed prior to commit:
  https://reviews.llvm.org/D26159?vs=76453&id=76478#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D26159

Files:
  cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp
  cfe/trunk/test/Analysis/dispatch-once.m


Index: cfe/trunk/test/Analysis/dispatch-once.m
===
--- cfe/trunk/test/Analysis/dispatch-once.m
+++ cfe/trunk/test/Analysis/dispatch-once.m
@@ -90,3 +90,20 @@
 void test_ivar_array_from_external_obj(Object *o) {
   dispatch_once(&o->once_array[1], ^{}); // expected-warning{{Call to 
'dispatch_once' uses memory within the instance variable 'once_array' for the 
predicate value.}}
 }
+
+void test_block_var_from_block() {
+  __block dispatch_once_t once;
+  ^{
+dispatch_once(&once, ^{}); // expected-warning{{Call to 'dispatch_once' 
uses the block variable 'once' for the predicate value.}}
+  };
+}
+
+void use_block_var(dispatch_once_t *once);
+
+void test_block_var_from_outside_block() {
+  __block dispatch_once_t once;
+  ^{
+use_block_var(&once);
+  };
+  dispatch_once(&once, ^{}); // expected-warning{{Call to 'dispatch_once' uses 
the block variable 'once' for the predicate value.}}
+}
Index: cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp
@@ -94,10 +94,15 @@
   bool SuggestStatic = false;
   os << "Call to '" << FName << "' uses";
   if (const VarRegion *VR = dyn_cast(RB)) {
-// We filtered out globals earlier, so it must be a local variable.
+// We filtered out globals earlier, so it must be a local variable
+// or a block variable which is under UnknownSpaceRegion.
 if (VR != R)
   os << " memory within";
-os << " the local variable '" << VR->getDecl()->getName() << '\'';
+if (VR->getDecl()->hasAttr())
+  os << " the block variable '";
+else
+  os << " the local variable '";
+os << VR->getDecl()->getName() << '\'';
 SuggestStatic = true;
   } else if (const ObjCIvarRegion *IVR = getParentIvarRegion(R)) {
 if (IVR != R)
@@ -108,6 +113,9 @@
   } else if (isa(RS)) {
 // Presence of an IVar superregion has priority over this branch, because
 // ObjC objects are on the heap even if the core doesn't realize this.
+// Presence of a block variable base region has priority over this branch,
+// because block variables are known to be either on stack or on heap
+// (might actually move between the two, hence UnknownSpace).
 return;
   } else {
 os << " stack allocated memory";


Index: cfe/trunk/test/Analysis/dispatch-once.m
===
--- cfe/trunk/test/Analysis/dispatch-once.m
+++ cfe/trunk/test/Analysis/dispatch-once.m
@@ -90,3 +90,20 @@
 void test_ivar_array_from_external_obj(Object *o) {
   dispatch_once(&o->once_array[1], ^{}); // expected-warning{{Call to 'dispatch_once' uses memory within the instance variable 'once_array' for the predicate value.}}
 }
+
+void test_block_var_from_block() {
+  __block dispatch_once_t once;
+  ^{
+dispatch_once(&once, ^{}); // expected-warning{{Call to 'dispatch_once' uses the block variable 'once' for the predicate value.}}
+  };
+}
+
+void use_block_var(dispatch_once_t *once);
+
+void test_block_var_from_outside_block() {
+  __block dispatch_once_t once;
+  ^{
+use_block_var(&once);
+  };
+  dispatch_once(&once, ^{}); // expected-warning{{Call to 'dispatch_once' uses the block variable 'once' for the predicate value.}}
+}
Index: cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp
===
--- cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp
+++ cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp
@@ -94,10 +94,15 @@
   bool SuggestStatic = false;
   os << "Call to '" << FName << "' uses";
   if (const VarRegion *VR = dyn_cast(RB)) {
-// We filtered out globals earlier, so it must be a local variable.
+// We filtered out globals earlier, so it must be a local variable
+// or a block variable which is under UnknownSpaceRegion.
 if (VR != R)
   os << " memory within";
-os << " the local variable '" << VR->getDecl()->getName() << '\'';
+if (VR->getDecl()->hasAttr())
+  os << " the block variable '";
+else
+  os << " the local variable '";
+os << VR->getDecl()->getName() << '\'';
 SuggestStatic = true;
   } else if (const ObjCIvarRegion *IVR = getParentIvarRegion(R)) {
 if (IVR != R)
@@ -108,6 +113,9 @@
   } else if (isa(RS)) {
 // Presence of an IVar superregion has priority o

[PATCH] D26159: [analyzer] MacOSXAPIChecker: Improve warning messages for __block vars in dispatch_once().

[Artem Dergachev via cfe-commits]

NoQ added a comment.

> FIXME: The analyzer sets stack memory space for __block variables when they 
> are referenced outside the block (eg. test_block_var_from_outside_block() 
> line 108). Will try to fix in a separate patch; i'm not relying on the memory 
> space in this patch.

That's actually correct, never mind.


https://reviews.llvm.org/D26159



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

[PATCH] D26159: [analyzer] MacOSXAPIChecker: Improve warning messages for __block vars in dispatch_once().

[Devin Coughlin via cfe-commits]

dcoughlin accepted this revision.
dcoughlin added a comment.
This revision is now accepted and ready to land.

LGTM.

We should probably be warning any time the address of a block variable is taken 
since the address is not stable -- but that's a job for a different checker or 
possibly even Sema.


https://reviews.llvm.org/D26159



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

[PATCH] D26159: [analyzer] MacOSXAPIChecker: Improve warning messages for __block vars in dispatch_once().

[Artem Dergachev via cfe-commits]

NoQ created this revision.
NoQ added reviewers: zaks.anna, dcoughlin.
NoQ added a subscriber: cfe-commits.

The checker already warns for `__block`-storage variables being used as a 
`dispatch_once()` predicate, however it refers to them as local which is not 
quite accurate, so we fix that.

Also add tests to make sure it works.

FIXME: The analyzer sets stack memory space for `__block` variables when they 
are referenced outside the block (eg. `test_block_var_from_outside_block()` 
line 108). Will try to fix in a separate patch; i'm not relying on the memory 
space in this patch.


https://reviews.llvm.org/D26159

Files:
  lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp
  test/Analysis/dispatch-once.m


Index: test/Analysis/dispatch-once.m
===
--- test/Analysis/dispatch-once.m
+++ test/Analysis/dispatch-once.m
@@ -90,3 +90,20 @@
 void test_ivar_array_from_external_obj(Object *o) {
   dispatch_once(&o->once_array[1], ^{}); // expected-warning{{Call to 
'dispatch_once' uses memory within the instance variable 'once_array' for the 
predicate value.}}
 }
+
+void test_block_var_from_block() {
+  __block dispatch_once_t once;
+  ^{
+dispatch_once(&once, ^{}); // expected-warning{{Call to 'dispatch_once' 
uses the block variable 'once' for the predicate value.}}
+  };
+}
+
+void use_block_var(dispatch_once_t *once);
+
+void test_block_var_from_outside_block() {
+  __block dispatch_once_t once;
+  ^{
+use_block_var(&once);
+  };
+  dispatch_once(&once, ^{}); // expected-warning{{Call to 'dispatch_once' uses 
the block variable 'once' for the predicate value.}}
+}
Index: lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp
@@ -94,10 +94,15 @@
   bool SuggestStatic = false;
   os << "Call to '" << FName << "' uses";
   if (const VarRegion *VR = dyn_cast(RB)) {
-// We filtered out globals earlier, so it must be a local variable.
+// We filtered out globals earlier, so it must be a local variable
+// or a block variable which is under UnknownSpaceRegion.
 if (VR != R)
   os << " memory within";
-os << " the local variable '" << VR->getDecl()->getName() << '\'';
+if (VR->getDecl()->hasAttr())
+  os << " the block variable '";
+else
+  os << " the local variable '";
+os << VR->getDecl()->getName() << '\'';
 SuggestStatic = true;
   } else if (const ObjCIvarRegion *IVR = getParentIvarRegion(R)) {
 if (IVR != R)
@@ -108,6 +113,9 @@
   } else if (isa(RS)) {
 // Presence of an IVar superregion has priority over this branch, because
 // ObjC objects are on the heap even if the core doesn't realize this.
+// Presence of a block variable base region has priority over this branch,
+// because block variables are known to be either on stack or on heap
+// (might actually move between the two, hence UnknownSpace).
 return;
   } else {
 os << " stack allocated memory";


Index: test/Analysis/dispatch-once.m
===
--- test/Analysis/dispatch-once.m
+++ test/Analysis/dispatch-once.m
@@ -90,3 +90,20 @@
 void test_ivar_array_from_external_obj(Object *o) {
   dispatch_once(&o->once_array[1], ^{}); // expected-warning{{Call to 'dispatch_once' uses memory within the instance variable 'once_array' for the predicate value.}}
 }
+
+void test_block_var_from_block() {
+  __block dispatch_once_t once;
+  ^{
+dispatch_once(&once, ^{}); // expected-warning{{Call to 'dispatch_once' uses the block variable 'once' for the predicate value.}}
+  };
+}
+
+void use_block_var(dispatch_once_t *once);
+
+void test_block_var_from_outside_block() {
+  __block dispatch_once_t once;
+  ^{
+use_block_var(&once);
+  };
+  dispatch_once(&once, ^{}); // expected-warning{{Call to 'dispatch_once' uses the block variable 'once' for the predicate value.}}
+}
Index: lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp
+++ lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp
@@ -94,10 +94,15 @@
   bool SuggestStatic = false;
   os << "Call to '" << FName << "' uses";
   if (const VarRegion *VR = dyn_cast(RB)) {
-// We filtered out globals earlier, so it must be a local variable.
+// We filtered out globals earlier, so it must be a local variable
+// or a block variable which is under UnknownSpaceRegion.
 if (VR != R)
   os << " memory within";
-os << " the local variable '" << VR->getDecl()->getName() << '\'';
+if (VR->getDecl()->hasAttr())
+  os << " the block variable '";
+else
+  os << " the local variable '";
+os << VR->getDecl()->getName() << '\'';
 SuggestStatic = true;
   } else if (const ObjCIvarRegion *IVR = getPare