This revision was automatically updated to reflect the committed changes.
Closed by commit rL318705: [analyzer] Diagnose stack leaks via block captures
(authored by alexshap).
Changed prior to commit:
https://reviews.llvm.org/D39438?vs=123392=123664#toc
Repository:
rL LLVM
NoQ accepted this revision.
NoQ added a comment.
This revision is now accepted and ready to land.
Thanks, looks great, please commit!~
Repository:
rL LLVM
https://reviews.llvm.org/D39438
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
alexshap updated this revision to Diff 123392.
alexshap added a comment.
Herald added a subscriber: a.sidorin.
adjust the messages, more uses of auto
Repository:
rL LLVM
https://reviews.llvm.org/D39438
Files:
lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
alexshap added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp:192
+ this, "Address of stack-allocated memory is captured");
+SmallString<512> Buf;
+llvm::raw_svector_ostream Out(Buf);
xazax.hun wrote:
> How
xazax.hun added a comment.
I found some nits, but overall I think this is getting close.
Comment at: lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp:76
range = CL->getSourceRange();
- }
- else if (const AllocaRegion* AR = dyn_cast(R)) {
+ } else if (const
alexshap added a comment.
ping
Repository:
rL LLVM
https://reviews.llvm.org/D39438
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
alexshap updated this revision to Diff 122507.
alexshap added a comment.
Address the comments
Repository:
rL LLVM
https://reviews.llvm.org/D39438
Files:
lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
test/Analysis/stack-capture-leak-arc.mm
NoQ added a comment.
Looks great, thanks! Minor inline stuff.
Comment at: lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp:116
+ const StackSpaceRegion *S = cast(R->getMemorySpace());
+ return S->getStackFrame() != C.getLocationContext()->getCurrentStackFrame();
+}
alexshap updated this revision to Diff 122194.
alexshap added a comment.
Fix
Repository:
rL LLVM
https://reviews.llvm.org/D39438
Files:
lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
test/Analysis/stack-capture-leak-arc.mm
test/Analysis/stack-capture-leak-no-arc.mm
Index:
alexshap added a comment.
ping @NoQ
Repository:
rL LLVM
https://reviews.llvm.org/D39438
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
alexshap updated this revision to Diff 121431.
alexshap added a comment.
1. Add no-warning comments
2. Switch to using a cached IdentifierInfo (similar to how it's done in
CheckObjCDealloc.cpp)
3. Rerun the all the tests - they are fine.
@dcoughlin - many thanks for the code review,
yeah, i
dcoughlin added a comment.
Thanks!
Another round of comments inline. With those addressed it looks good to me --
but you should wait on Artem's go-ahead before committing.
Comment at: lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp:121
+QualType Q =
alexshap added inline comments.
Comment at: test/Analysis/stack-capture-leak-arc.mm:68
+ int x = 123;
+ int = x;
+ void (^b)(void) = ^void(void) {
c++ reference
Repository:
rL LLVM
https://reviews.llvm.org/D39438
alexshap updated this revision to Diff 121230.
alexshap added a comment.
Add more tests for blocks being passed around
Repository:
rL LLVM
https://reviews.llvm.org/D39438
Files:
lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
test/Analysis/stack-capture-leak-arc.mm
alexshap added inline comments.
Comment at: test/Analysis/stack-async-leak.m:1
+// RUN: %clang_analyze_cc1 -triple x86_64-apple-darwin10
-analyzer-checker=core -fblocks -fobjc-arc -verify %s
+
dcoughlin wrote:
> Can you add an additional run line testing the
alexshap updated this revision to Diff 121229.
alexshap added a comment.
Refactor the checker, add more tests.
Repository:
rL LLVM
https://reviews.llvm.org/D39438
Files:
lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
test/Analysis/stack-capture-leak-arc.mm
dcoughlin added a reviewer: george.karpenkov.
dcoughlin added a comment.
I think this is a great idea, and I expect it to find some nasty bugs!
However, I'm worried about false positives in the following not-too-uncommon
idiom:
dispatch_semaphore_t semaphore = dispatch_semaphore_create(0);
alexshap updated this revision to Diff 121100.
alexshap added a comment.
cleanup
Repository:
rL LLVM
https://reviews.llvm.org/D39438
Files:
lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
test/Analysis/stack-async-leak.m
Index: test/Analysis/stack-async-leak.m
alexshap updated this revision to Diff 121099.
alexshap added a comment.
more tests
Repository:
rL LLVM
https://reviews.llvm.org/D39438
Files:
lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
test/Analysis/stack-async-leak.m
Index: test/Analysis/stack-async-leak.m
alexshap added inline comments.
Comment at: test/Analysis/stack-async-leak.m:60
+ void (^b)(void) = ^void(void) {
+*p = 1;
+ };
NoQ wrote:
> You may enjoy adding an extra note piece (as in D24278) to the offending
> statement within the block. It might
alexshap updated this revision to Diff 121055.
alexshap added a comment.
Address the comments, add more tests
https://reviews.llvm.org/D39438
Files:
lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
test/Analysis/stack-async-leak.m
Index: test/Analysis/stack-async-leak.m
alexshap added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp:144
+return;
+ if (dyn_cast_or_null(Region->getMemorySpace())) {
+ExplodedNode *N = C.generateErrorNode();
NoQ wrote:
> `getMemorySpace()` is
NoQ added a comment.
What i was trying to say is - Hey this check looks useful!~ Great idea.
Repository:
rL LLVM
https://reviews.llvm.org/D39438
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
NoQ added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp:128
+ !Call.isGlobalCFunction("dispatch_async") &&
+ !Call.isGlobalCFunction("dispatch_once"))
+return;
`dispatch_once` isn't really asynchronous; i think
xazax.hun added inline comments.
Comment at: lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp:145
+ if (dyn_cast_or_null(Region->getMemorySpace())) {
+ExplodedNode *N = C.generateErrorNode();
+if (!N)
This will stop the analysis on
alexshap updated this revision to Diff 120908.
alexshap added a comment.
Add positive tests
Repository:
rL LLVM
https://reviews.llvm.org/D39438
Files:
lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp
test/Analysis/stack-async-leak.m
Index: test/Analysis/stack-async-leak.m
alexshap created this revision.
Herald added subscribers: szepet, xazax.hun.
This diff extends StackAddrEscapeChecker to catch stack addr leaks via block
captures if the block is executed
asynchronously.
Test plan: make check-all
Repository:
rL LLVM
https://reviews.llvm.org/D39438
Files:
27 matches
Mail list logo
