[all-commits] [llvm/llvm-project] b9bca8: [analyzer][NFC] Don't bind values to ObjCForCollec...

[Kristóf Umann via All-commits]

  Branch: refs/heads/master
  Home:   https://github.com/llvm/llvm-project
  Commit: b9bca883c970d36f408db80df21838c713c326db
      https://github.com/llvm/llvm-project/commit/b9bca883c970d36f408db80df21838c713c326db
  Author: Kristóf Umann <dkszelethus at gmail.com>
  Date:   2020-09-11 (Fri, 11 Sep 2020)

  Changed paths:
    M clang/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h
    M clang/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
    M clang/lib/StaticAnalyzer/Checkers/UndefBranchChecker.cpp
    M clang/lib/StaticAnalyzer/Core/Environment.cpp
    M clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
    M clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp
    M clang/lib/StaticAnalyzer/Core/SymbolManager.cpp
    A clang/test/Analysis/objc-live-crash.mm

  Log Message:
  -----------
  [analyzer][NFC] Don't bind values to ObjCForCollectionStmt, replace it with a GDM trait

Based on the discussion in D82598#2171312. Thanks @NoQ!

D82598 is titled "Get rid of statement liveness, because such a thing doesn't
exist", and indeed, expressions express a value, non-expression statements
don't.

if (a && get() || []{ return true; }())
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ has a value
    ~ has a value
    ~~~~~~~~~~ has a value
                  ~~~~~~~~~~~~~~~~~~~~ has a value
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ doesn't have a value

That is simple enough, so it would only make sense if we only assigned symbolic
values to expressions in the static analyzer. Yet the interface checkers can
access presents, among other strange things, the following two methods:

ProgramState::BindExpr(const Stmt *S, const LocationContext *LCtx, SVal V,
                       bool Invalidate=true)
ProgramState::getSVal(const Stmt *S, const LocationContext *LCtx)

So, what gives? Turns out, we make an exception for ReturnStmt (which we'll
leave for another time) and ObjCForCollectionStmt. For any other loops, in order
to know whether we should analyze another iteration, among other things, we
evaluate it's condition. Which is a problem for ObjCForCollectionStmt, because
it simply doesn't have one (CXXForRangeStmt has an implicit one!). In its
absence, we assigned the actual statement with a concrete 1 or 0 to indicate
whether there are any more iterations left. However, this is wildly incorrect,
its just simply not true that the for statement has a value of 1 or 0, we can't
calculate its liveness because that doesn't make any sense either, so this patch
turns it into a GDM trait.

Fixing this allows us to reinstate the assert removed in
https://reviews.llvm.org/rG032b78a0762bee129f33e4255ada6d374aa70c71.

Differential Revision: https://reviews.llvm.org/D86736