If Anna is Ok with it, I'm fine with merging. Thanks, Hans
On Wed, Feb 1, 2017 at 10:29 AM, Artem Dergachev <noqnoq...@gmail.com> wrote: > 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@lists.llvm.org >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits > > _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
