r292800 - [analyzer] Fix memory space of static locals seen from nested blocks.

[Anna Zaks via cfe-commits]

Yes, ok to merge!
Thank you.

Sent from my iPhone

> On Jan 23, 2017, at 1:50 PM, Hans Wennborg <hans at 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 <noqnoqneo at 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 at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>> 
>>