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);
dispatch_async(some_queue, ^{
// Do some work
dispatch_semaphore_signal(semaphore);
});
// Do some other work concurrently with the asynchronous work
// Wait for the asynchronous work to finish
dispatch_semaphore_wait(semaphore, DISPATCH_TIME_FOREVER);
What do you think about suppressing the diagnostic when the block captures a
variable with type 'dispatch_semaphore_t'? This isn't a perfect suppression,
but it will catch most of the cases that I have seen.
Also, does this checker diagnose when a block captures a C++ reference? If so,
it would be great to add a test for that!
Some additional comments inline.
================
Comment at: lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp:124
+void StackAddrEscapeChecker::checkBlockCaptures(const BlockDataRegion &B,
+ CheckerContext &C) const {
----------------
Could we break with tradition and add a oxygen comment describing what this
method does?
================
Comment at: lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp:133
+ continue;
+ if (C.getASTContext().getLangOpts().ObjCAutoRefCount &&
+ isa<BlockDataRegion>(Region))
----------------
Can you factor this logic out with the logic it duplicates in checkPreStmt()?
================
Comment at: lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp:136
+ continue;
+ if (dyn_cast<StackSpaceRegion>(Region->getMemorySpace())) {
+ ExplodedNode *N = C.generateNonFatalErrorNode();
----------------
Stylistically we use isa<> for cases like these where the result of dyn_cast<>
is not used.
================
Comment at: lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp:146
+ SourceRange Range = genName(Out, Region, C.getASTContext());
+ Out << " might leak via the block capture";
+ auto Report =
----------------
Instead of "might leak ... " I would suggest " is captured by an
asynchronously-executed block". In the context of the analyzer we use "leak" to
mean "resource that is not freed" rather than in the sense of exposing
information.
You'll need to make a different variant of the diagnostic text for when the
block is returned rather than asynchronously executed.
================
Comment at: lib/StaticAnalyzer/Checkers/StackAddrEscapeChecker.cpp:185
+ if (const BlockDataRegion *B = dyn_cast<BlockDataRegion>(R))
+ checkBlockCaptures(*B, C);
+
----------------
Will this have a false positive if I create a block in a caller, pass it to a
callee, and then the callee returns it? You may want to factor out and reuse
the logic from below that compares the StackFrameContext for the current frame
to the frame of the captured addresses in checkBlockCaptures().
================
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
+
----------------
Can you add an additional run line testing the expected behavior when ARC is
not enabled? You can pass a -DARC_ENABLED=1 or similar to distinguish between
the two (for example, to test to make sure that a diagnostic is emitted under
MRR but not ARC).
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
