r294050. Thanks, Hans
On Fri, Feb 3, 2017 at 1:50 PM, Anna Zaks <ga...@apple.com> wrote: > Fine with merging. Thank you! > Anna. >> On Feb 1, 2017, at 11:00 AM, Hans Wennborg <h...@chromium.org> wrote: >> >> 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
