[PATCH] D86736: [analyzer][NFC] Don't bind values to ObjCForCollectionStmt, replace it with a GDM trait

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus created this revision.
Szelethus added reviewers: NoQ, vsavchenko, dcoughlin, baloghadamsoftware, martong, balazske, xazax.hun, steakhal.
Szelethus added a project: clang.
Herald added subscribers: cfe-commits, ASDenysPetrov, Charusso, gamesh411, dkrupp, donat.nagy, mikhail.ramalho, a.sidorin, rnkovacs, szepet, whisperity.
Szelethus requested review of this revision.

Based on the discussion in D82598#2171312 <https://reviews.llvm.org/D82598#2171312>. Thanks @NoQ!

Let's talk about how we got here. D82598 <https://reviews.llvm.org/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) <https://clang.llvm.org/doxygen/classclang_1_1ento_1_1ProgramState.html#a1c9e0f3d56ed5206fea62b1fa68c8e94>
ProgramState::getSVal(const Stmt *S, const LocationContext *LCtx) <https://clang.llvm.org/doxygen/classclang_1_1ento_1_1ProgramState.html#aaa0ff169cc4a9f18f4dd626cf7201053>

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 <https://clang.llvm.org/doxygen/classclang_1_1CXXForRangeStmt.html#a06e5bf0adad48dddee1d99a9cd324cf4> 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.

This patch isn't quite done (its nowhere near up to my standards in terms of documentation), but I wanted to publish it to avoid sinking too much work into a doomed patch.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D86736

Files:
  clang/include/clang/StaticAnalyzer/Core/PathSensitive/ProgramState.h
  clang/lib/StaticAnalyzer/Checkers/BasicObjCFoundationChecks.cpp
  clang/lib/StaticAnalyzer/Checkers/UndefBranchChecker.cpp
  clang/lib/StaticAnalyzer/Core/Environment.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngine.cpp
  clang/lib/StaticAnalyzer/Core/ExprEngineObjC.cpp
  clang/lib/StaticAnalyzer/Core/ProgramState.cpp
  clang/lib/StaticAnalyzer/Core/SymbolManager.cpp

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D86736.288434.patch
Type: text/x-patch
Size: 14540 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200827/17ebf0b8/attachment-0001.bin>