Thanks for letting me know! I've merged your revert to the branch in r292947. Please let me know if there is any more follow-up.
On Mon, Jan 23, 2017 at 6:30 PM, Devin Coughlin <dcough...@apple.com> wrote: > FYI, I reverted r292800 from trunk in r292874. It was causing our internal > validation bots to have false positives whenever a static local was > dereferenced/passed to a nonnull function in a block evaluated at the top > level. > > Devin > > > > On Jan 23, 2017, at 4:19 PM, Hans Wennborg <h...@chromium.org> wrote: > > Merged in r292858. > > Thanks, > Hans > > On Mon, Jan 23, 2017 at 4:15 PM, Anna Zaks <ga...@apple.com> wrote: > > Yes, ok to merge! > Thank you. > > Sent from my iPhone > > On Jan 23, 2017, at 1:50 PM, Hans Wennborg <h...@chromium.org> wrote: > > Sounds good to me. > > Anna, you're the code owner here. Ok to merge this? > > Thanks, > Hans > > On Mon, Jan 23, 2017 at 10:37 AM, Artem Dergachev <noqnoq...@gmail.com> > wrote: > Hans, > > Could we merge this one into the 4.0.0 release branch? It's a recent bugfix > for the analyzer. > > Thanks, > Artem. > > > > On 1/23/17 7:57 PM, Artem Dergachev via cfe-commits wrote: > > Author: dergachev > Date: Mon Jan 23 10:57:11 2017 > New Revision: 292800 > > URL: http://llvm.org/viewvc/llvm-project?rev=292800&view=rev > Log: > [analyzer] Fix memory space of static locals seen from nested blocks. > > When a block within a function accesses a function's static local > variable, > this local is captured by reference rather than copied to the heap. > > Therefore this variable's memory space is known: StaticGlobalSpaceRegion. > Used to be UnknownSpaceRegion, same as for stack locals. > > Fixes a false positive in MacOSXAPIChecker. > > rdar://problem/30105546 > Differential revision: https://reviews.llvm.org/D28946 > > Modified: > cfe/trunk/lib/StaticAnalyzer/Core/MemRegion.cpp > cfe/trunk/test/Analysis/dispatch-once.m > > Modified: cfe/trunk/lib/StaticAnalyzer/Core/MemRegion.cpp > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/StaticAnalyzer/Core/MemRegion.cpp?rev=292800&r1=292799&r2=292800&view=diff > > ============================================================================== > --- cfe/trunk/lib/StaticAnalyzer/Core/MemRegion.cpp (original) > +++ cfe/trunk/lib/StaticAnalyzer/Core/MemRegion.cpp Mon Jan 23 10:57:11 > 2017 > @@ -776,6 +776,22 @@ getStackOrCaptureRegionForDeclContext(co > return (const StackFrameContext *)nullptr; > } > +static CanQualType getBlockPointerType(const BlockDecl *BD, ASTContext > &C) { > + // FIXME: The fallback type here is totally bogus -- though it should > + // never be queried, it will prevent uniquing with the real > + // BlockCodeRegion. Ideally we'd fix the AST so that we always had a > + // signature. > + QualType T; > + if (const TypeSourceInfo *TSI = BD->getSignatureAsWritten()) > + T = TSI->getType(); > + if (T.isNull()) > + T = C.VoidTy; > + if (!T->getAs<FunctionType>()) > + T = C.getFunctionNoProtoType(T); > + T = C.getBlockPointerType(T); > + return C.getCanonicalType(T); > +} > + > const VarRegion* MemRegionManager::getVarRegion(const VarDecl *D, > const LocationContext > *LC) { > const MemRegion *sReg = nullptr; > @@ -803,7 +819,7 @@ const VarRegion* MemRegionManager::getVa > sReg = getGlobalsRegion(); > } > - // Finally handle static locals. > + // Finally handle locals. > } else { > // FIXME: Once we implement scope handling, we will need to properly > lookup > // 'D' to the proper LocationContext. > @@ -816,9 +832,22 @@ const VarRegion* MemRegionManager::getVa > const StackFrameContext *STC = V.get<const StackFrameContext*>(); > - if (!STC) > - sReg = getUnknownRegion(); > - else { > + if (!STC) { > + if (D->isStaticLocal()) { > + const CodeTextRegion *fReg = nullptr; > + if (const auto *ND = dyn_cast<NamedDecl>(DC)) > + fReg = getFunctionCodeRegion(ND); > + else if (const auto *BD = dyn_cast<BlockDecl>(DC)) > + fReg = getBlockCodeRegion(BD, getBlockPointerType(BD, > getContext()), > + LC->getAnalysisDeclContext()); > + assert(fReg && "Unable to determine code region for a static > local!"); > + sReg = getGlobalsRegion(MemRegion::StaticGlobalSpaceRegionKind, > fReg); > + } else { > + // We're looking at a block-captured local variable, which may be > either > + // still local, or already moved to the heap. So we're not sure. > + sReg = getUnknownRegion(); > + } > + } else { > if (D->hasLocalStorage()) { > sReg = isa<ParmVarDecl>(D) || isa<ImplicitParamDecl>(D) > ? static_cast<const > MemRegion*>(getStackArgumentsRegion(STC)) > @@ -831,22 +860,9 @@ const VarRegion* MemRegionManager::getVa > sReg = > getGlobalsRegion(MemRegion::StaticGlobalSpaceRegionKind, > > getFunctionCodeRegion(cast<NamedDecl>(STCD))); > else if (const BlockDecl *BD = dyn_cast<BlockDecl>(STCD)) { > - // FIXME: The fallback type here is totally bogus -- though it > should > - // never be queried, it will prevent uniquing with the real > - // BlockCodeRegion. Ideally we'd fix the AST so that we always > had a > - // signature. > - QualType T; > - if (const TypeSourceInfo *TSI = BD->getSignatureAsWritten()) > - T = TSI->getType(); > - if (T.isNull()) > - T = getContext().VoidTy; > - if (!T->getAs<FunctionType>()) > - T = getContext().getFunctionNoProtoType(T); > - T = getContext().getBlockPointerType(T); > - > const BlockCodeRegion *BTR = > - getBlockCodeRegion(BD, C.getCanonicalType(T), > - STC->getAnalysisDeclContext()); > + getBlockCodeRegion(BD, getBlockPointerType(BD, > getContext()), > + STC->getAnalysisDeclContext()); > sReg = > getGlobalsRegion(MemRegion::StaticGlobalSpaceRegionKind, > BTR); > } > > Modified: cfe/trunk/test/Analysis/dispatch-once.m > URL: > http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Analysis/dispatch-once.m?rev=292800&r1=292799&r2=292800&view=diff > > ============================================================================== > --- cfe/trunk/test/Analysis/dispatch-once.m (original) > +++ cfe/trunk/test/Analysis/dispatch-once.m Mon Jan 23 10:57:11 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 > + }; > +} > > > _______________________________________________ > 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
