r293043 - [analyzer] Fix MacOSXAPIChecker fp with static locals seen from nested blocks.
Artem Dergachev via cfe-commits
cfe-commits at lists.llvm.org
Wed Feb 1 10:29:00 PST 2017
Hans,
This is a fixed and tested version of the previously-merged-and-reverted
r292800, do we still have time to land this into 4.0?
Thanks,
Artem.
On 1/25/17 1:21 PM, Artem Dergachev via cfe-commits wrote:
> Author: dergachev
> Date: Wed Jan 25 04:21:45 2017
> New Revision: 293043
>
> URL: http://llvm.org/viewvc/llvm-project?rev=293043&view=rev
> Log:
> [analyzer] Fix MacOSXAPIChecker fp with static locals seen from nested blocks.
>
> This is an attempt to avoid new false positives caused by the reverted r292800,
> however the scope of the fix is significantly reduced - some variables are still
> in incorrect memory spaces.
>
> Relevant test cases added.
>
> rdar://problem/30105546
> rdar://problem/30156693
> Differential revision: https://reviews.llvm.org/D28946
>
> Added:
> cfe/trunk/test/Analysis/null-deref-static.m
> Modified:
> cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp
> cfe/trunk/lib/StaticAnalyzer/Core/MemRegion.cpp
> cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp
> cfe/trunk/test/Analysis/dispatch-once.m
>
> Modified: cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp?rev=293043&r1=293042&r2=293043&view=diff
> ==============================================================================
> --- cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp (original)
> +++ cfe/trunk/lib/StaticAnalyzer/Checkers/MacOSXAPIChecker.cpp Wed Jan 25 04:21:45 2017
> @@ -94,11 +94,18 @@ void MacOSXAPIChecker::CheckDispatchOnce
> bool SuggestStatic = false;
> os << "Call to '" << FName << "' uses";
> if (const VarRegion *VR = dyn_cast<VarRegion>(RB)) {
> + const VarDecl *VD = VR->getDecl();
> + // FIXME: These should have correct memory space and thus should be filtered
> + // out earlier. This branch only fires when we're looking from a block,
> + // which we analyze as a top-level declaration, onto a static local
> + // in a function that contains the block.
> + if (VD->isStaticLocal())
> + return;
> // 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";
> - if (VR->getDecl()->hasAttr<BlocksAttr>())
> + if (VD->hasAttr<BlocksAttr>())
> os << " the block variable '";
> else
> os << " the local variable '";
>
> Modified: cfe/trunk/lib/StaticAnalyzer/Core/MemRegion.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/MemRegion.cpp?rev=293043&r1=293042&r2=293043&view=diff
> ==============================================================================
> --- cfe/trunk/lib/StaticAnalyzer/Core/MemRegion.cpp (original)
> +++ cfe/trunk/lib/StaticAnalyzer/Core/MemRegion.cpp Wed Jan 25 04:21:45 2017
> @@ -816,9 +816,11 @@ const VarRegion* MemRegionManager::getVa
>
> const StackFrameContext *STC = V.get<const StackFrameContext*>();
>
> - if (!STC)
> + if (!STC) {
> + // FIXME: Assign a more sensible memory space to static locals
> + // we see from within blocks that we analyze as top-level declarations.
> sReg = getUnknownRegion();
> - else {
> + } else {
> if (D->hasLocalStorage()) {
> sReg = isa<ParmVarDecl>(D) || isa<ImplicitParamDecl>(D)
> ? static_cast<const MemRegion*>(getStackArgumentsRegion(STC))
>
> Modified: cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp?rev=293043&r1=293042&r2=293043&view=diff
> ==============================================================================
> --- cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp (original)
> +++ cfe/trunk/lib/StaticAnalyzer/Core/RegionStore.cpp Wed Jan 25 04:21:45 2017
> @@ -1849,6 +1849,8 @@ SVal RegionStoreManager::getBindingForVa
>
> // Function-scoped static variables are default-initialized to 0; if they
> // have an initializer, it would have been processed by now.
> + // FIXME: This is only true when we're starting analysis from main().
> + // We're losing a lot of coverage here.
> if (isa<StaticGlobalSpaceRegion>(MS))
> return svalBuilder.makeZeroVal(T);
>
>
> Modified: cfe/trunk/test/Analysis/dispatch-once.m
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/dispatch-once.m?rev=293043&r1=293042&r2=293043&view=diff
> ==============================================================================
> --- cfe/trunk/test/Analysis/dispatch-once.m (original)
> +++ cfe/trunk/test/Analysis/dispatch-once.m Wed Jan 25 04:21:45 2017
> @@ -107,3 +107,10 @@ void test_block_var_from_outside_block()
> };
> dispatch_once(&once, ^{}); // expected-warning{{Call to 'dispatch_once' uses the block variable 'once' for the predicate value.}}
> }
> +
> +void test_static_var_from_outside_block() {
> + static dispatch_once_t once;
> + ^{
> + dispatch_once(&once, ^{}); // no-warning
> + };
> +}
>
> Added: cfe/trunk/test/Analysis/null-deref-static.m
> URL: http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/null-deref-static.m?rev=293043&view=auto
> ==============================================================================
> --- cfe/trunk/test/Analysis/null-deref-static.m (added)
> +++ cfe/trunk/test/Analysis/null-deref-static.m Wed Jan 25 04:21:45 2017
> @@ -0,0 +1,35 @@
> +// RUN: %clang_cc1 -w -fblocks -analyze -analyzer-checker=core,deadcode,alpha.core,debug.ExprInspection -verify %s
> +
> +void *malloc(unsigned long);
> +void clang_analyzer_warnIfReached();
> +
> +void test_static_from_block() {
> + static int *x;
> + ^{
> + *x; // no-warning
> + };
> +}
> +
> +void test_static_within_block() {
> + ^{
> + static int *x;
> + *x; // expected-warning{{Dereference of null pointer}}
> + };
> +}
> +
> +void test_static_control_flow(int y) {
> + static int *x;
> + if (x) {
> + // FIXME: Should be reachable.
> + clang_analyzer_warnIfReached(); // no-warning
> + }
> + if (y) {
> + // We are not sure if this branch is possible, because the developer
> + // may argue that function is always called with y == 1 for the first time.
> + // In this case, we can only advise the developer to add assertions
> + // for suppressing such path.
> + *x; // expected-warning{{Dereference of null pointer}}
> + } else {
> + x = malloc(1);
> + }
> +}
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
More information about the cfe-commits
mailing list