[PATCH] D27409: [analyzer] RetainCountChecker: Improve support for libdispatch APIs.

2016-12-06 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin added a comment.

The analyzer currently doesn't do any checking for dispatch retain/release APIs 
in C. It similarly doesn't do any checking in Objective-C when 
OS_OBJECT_USE_OBJC is 0 (and thus the dispatch types are defined to their 
C-struct versions). This happens when the user explicitly sets 
-DOS_OBJECT_USE_OBJC=0 (to safely share dispatch between ARC and non-ARC 
projects) and also under certain combinations of deployment targets and 
architectures where the runtime support for the feature is not present (for 
example, earlier than iOS 6.0).

My feeling is that for this patch it is fine to continue the policy of not 
diagnosing dispatch leaks/overreleases when OS_OBJECT_USE_OBJC is 0. Adding 
this support to the retain count checker is a larger project and in my opinion 
it should be done separately. To that end, I would recommend removing the 
summary creation for dispatch_retain/dispatch_release().

That said, people do use these APIs in C (and with -DOS_OBJECT_USE_OBJC=0 in 
ObjC), so it would be great test to make sure that this patch don't introduce 
any new false positives in these situations. I think it important to suck in 
enough of the header typedefs used when -DOS_OBJECT_USE_OBJC=0 to write these 
tests to make sure we don't regress when using the C-based APIs.




Comment at: test/Analysis/dispatch-data-leak.m:4
+// RUN: %clang_cc1 -w -triple x86_64-apple-macosx10.12.0 -fblocks 
-DDISPATCH_MACROS -analyze -analyzer-output=text 
-analyzer-checker=core,osx.cocoa,unix.Malloc -verify %s
+// RUN: %clang_cc1 -w -triple x86_64-apple-macosx10.12.0 -fblocks 
-DDISPATCH_MARCOS -fobjc-arc -analyze -analyzer-output=text 
-analyzer-checker=core,osx.cocoa,unix.Malloc -verify %s
+

Looks like there is a typo here ('MARCOS' vs. 'MACROS')?


https://reviews.llvm.org/D27409



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


[PATCH] D27409: [analyzer] RetainCountChecker: Improve support for libdispatch APIs.

2016-12-06 Thread Devin Coughlin via Phabricator via cfe-commits
dcoughlin added a comment.

In https://reviews.llvm.org/D27409#614601, @NoQ wrote:

> Not sure we need to stay merged with `retain-release-arc.m`, as we're not 
> really reusing many declarations across these files.


I find it more helpful to organize tests around functionality ('retain count 
checker') than around bugs ('dispatch data leak', 'PRxyzab.c').  Organizing 
around functionality helps future contributors and maintainers know where to 
add a new test and where to look for existing tests. In my view this critical 
for developers who are new to the project or who are learning a new subsystem. 
It also typically reduces the number of 'clang' RUN lines, which have a cost in 
maintenance and running time.


https://reviews.llvm.org/D27409



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


[PATCH] D27409: [analyzer] RetainCountChecker: Improve support for libdispatch APIs.

2016-12-06 Thread Artem Dergachev via Phabricator via cfe-commits
NoQ retitled this revision from "[analyzer] RetainCountChecker: The callback in 
dispatch_data_create() doesn't free the return symbol." to "[analyzer] 
RetainCountChecker: Improve support for libdispatch APIs.".
NoQ updated the summary for this revision.
NoQ updated this revision to Diff 80432.
NoQ added a comment.

Add more tests.

Not sure we need to stay merged with `retain-release-arc.m`, as we're not 
really reusing many declarations across these files.

As well, handle the case when `dispatch_retain` and `dispatch_release` are 
regular functions.


https://reviews.llvm.org/D27409

Files:
  lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
  test/Analysis/dispatch-data-leak.m

Index: test/Analysis/dispatch-data-leak.m
===
--- /dev/null
+++ test/Analysis/dispatch-data-leak.m
@@ -0,0 +1,95 @@
+// RUN: %clang_cc1 -w -triple x86_64-apple-macosx10.12.0 -fblocks -analyze -analyzer-output=text -analyzer-checker=core,osx.cocoa,unix.Malloc -verify %s
+// RUN: %clang_cc1 -w -triple x86_64-apple-macosx10.12.0 -fblocks -fobjc-arc -analyze -analyzer-output=text -analyzer-checker=core,osx.cocoa,unix.Malloc -verify %s
+// RUN: %clang_cc1 -w -triple x86_64-apple-macosx10.12.0 -fblocks -DDISPATCH_MACROS -analyze -analyzer-output=text -analyzer-checker=core,osx.cocoa,unix.Malloc -verify %s
+// RUN: %clang_cc1 -w -triple x86_64-apple-macosx10.12.0 -fblocks -DDISPATCH_MARCOS -fobjc-arc -analyze -analyzer-output=text -analyzer-checker=core,osx.cocoa,unix.Malloc -verify %s
+
+#include "Inputs/system-header-simulator.h"
+#include "Inputs/system-header-simulator-objc.h"
+
+#define NON_ARC !__has_feature(objc_arc)
+
+#if !NON_ARC
+  // expected-no-diagnostics
+#endif
+
+#define OS_OBJECT_RETURNS_RETAINED __attribute__((__ns_returns_retained__))
+#define DISPATCH_RETURNS_RETAINED OS_OBJECT_RETURNS_RETAINED
+
+@protocol OS_dispatch_object
+@end
+@protocol OS_dispatch_data 
+@end
+@protocol OS_dispatch_queue 
+@end
+
+typedef NSObject *dispatch_object_t;
+typedef NSObject *dispatch_data_t;
+typedef NSObject *dispatch_queue_t;
+
+typedef void (^dispatch_block_t)(void);
+
+dispatch_queue_t dispatch_get_main_queue(void);
+
+DISPATCH_RETURNS_RETAINED dispatch_data_t
+dispatch_data_create(const void *buffer, size_t size,
+ dispatch_queue_t _Nullable queue,
+ dispatch_block_t _Nullable destructor);
+
+#ifdef DISPATCH_MACROS
+void _dispatch_object_validate(dispatch_object_t object);
+#define dispatch_retain(object) \
+  __extension__({ dispatch_object_t _o = (object); \
+  _dispatch_object_validate(_o); \
+  (void)[_o retain]; })
+#define dispatch_release(object) \
+  __extension__({ dispatch_object_t _o = (object); \
+  _dispatch_object_validate(_o); \
+  [_o release]; })
+#else
+void dispatch_retain(dispatch_object_t object);
+void dispatch_release(dispatch_object_t object);
+#endif
+
+int buf[1024];
+
+void leaked_data() {
+  dispatch_data_t data = dispatch_data_create(buf, 1024,
+  dispatch_get_main_queue(), ^{});
+}
+#if NON_ARC
+  // expected-warning@-2{{Potential leak of an object stored into 'data'}}
+  // expected-note@-5{{Call to function 'dispatch_data_create' returns an Objective-C object with a +1 retain count}}
+  // expected-note@-4{{Object leaked: object allocated and stored into 'data' is not referenced later in this execution path and has a retain count of +1}}
+#endif
+
+void dispatch_released_data() {
+  dispatch_data_t data = dispatch_data_create(buf, 1024,
+  dispatch_get_main_queue(), ^{});
+#if NON_ARC
+  dispatch_release(data); // no-warning
+#endif
+}
+
+void objc_released_data() {
+  dispatch_data_t data = dispatch_data_create(buf, 1024,
+  dispatch_get_main_queue(), ^{});
+#if NON_ARC
+  [data release]; // no-warning
+#endif
+}
+
+void leaked_retained_data() {
+  dispatch_data_t data = dispatch_data_create(buf, 1024,
+  dispatch_get_main_queue(), ^{});
+#if NON_ARC
+  dispatch_retain(data);
+  [data release];
+#endif
+}
+#if NON_ARC
+// expected-warning@-2{{Potential leak of an object stored into 'data'}}
+// expected-note@-9{{Call to function 'dispatch_data_create' returns an Objective-C object with a +1 retain count}}
+// expected-note@-7{{Reference count incremented. The object now has a +2 retain count}}
+// expected-note@-7{{Reference count decremented. The object now has a +1 retain count}}
+// expected-note@-6{{Object leaked: object allocated and stored into 'data' is not referenced later in this execution path and has a retain count of +1}}
+#endif
Index: lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
===
--- lib/StaticAnalyzer/Checkers/RetainCountChecker.cpp
+++