r293043 - [analyzer] Fix MacOSXAPIChecker fp with static locals seen from nested blocks.

[Hans Wennborg via cfe-commits]

Ping?

On Wed, Feb 1, 2017 at 11:00 AM, Hans Wennborg <hans at 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 <noqnoqneo at 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 at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>
>>