[analyzer] do not crash on subscripts into ObjC properties

[george.karpenkov]

Summary

Subscripts into ObjC properties can be achieved by using extension CUDA types.
In that case, subscripts are not allowed to be lvalues, so we relax the assumption that array subscript expression is always an lvalue in that particular case.

Fixes (or at least prevents crashing) in rdar://34829842

[george.karpenkov]

@dcoughlin @NoQ thoughts? Option B is to relax the assertion when the value is unknown.

[george.karpenkov]

Update: change the logic, do nothing if the index result is an rvalue.

[dcoughlin]

lib/StaticAnalyzer/Core/ExprEngine.cpp

2136 ↗

(On Diff #125022)

This drops the flow when A is an rvalue. No code after the subscript expression will be reached. You need to make sure that Pred (or its successors) make it into Dst. You should add a test for this.

This also doesn't perform the pre- and post- statement checker callbacks, which we still need to do for the rvalue case. One way to do this is to move the check for the lvalue into VisitLvalArraySubscriptExpr() (renaming it, of course) and only construct the lvalue SVal when A is an lvalue.

[NoQ]

include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h

335–337 ↗

(On Diff #125374)

Indent :)

lib/StaticAnalyzer/Core/ExprEngine.cpp

2130 ↗

(On Diff #125374)

Does this deserve a FIXME?

2151 ↗

(On Diff #125374)

So we screw all vectors, not just the ObjC rvalue thingies. Not sure if they worked before in any cases, but i guess we could be more careful here.

test/Analysis/vector.m

41 ↗

(On Diff #125374)

Just curious, did V not work?

49 ↗

(On Diff #125374)

Devin wanted a coverage test, i.e. show how we continue our analysis after encountering i2.v[0]. We can divide by zero after obtaining the value, or just use ExprInspection's clang_analyzer_warnIfReached().

[george.karpenkov]

lib/StaticAnalyzer/Core/ExprEngine.cpp

2130 ↗

(On Diff #125374)

actually, the comment was misplaced, and probably yes.

2151 ↗

(On Diff #125374)

yeah that's a mistake. It is meant to be non-glvalue vectors.

[dcoughlin]

Minor nits inline. Other than that, looks great to me.

lib/StaticAnalyzer/Core/ExprEngine.cpp

2142 ↗

(On Diff #125402)

I think it would great to add a comment here describing when the "like" kicks in (for example, taking the address of a subscript expression on a void * base).

test/Analysis/vector.m

54 ↗

(On Diff #125402)

I don't think there is anything special about Objective-C properties here -- the problem manifests on all function calls. Do you think we need to move the test into a '.m' and test both the property and function return case? Or is the function return case sufficient?

58 ↗

(On Diff #125402)

clang_analyzer_warnIfReached() is more idiomatic here.

[george.karpenkov]

test/Analysis/vector.m

54 ↗

(On Diff #125402)

I would prefer to leave it here.
Obj-C properties are indeed covered by the same if-branch as the return case, but I think it does not hurt to have an extra test case, as such situations are relatively rare.

cfe/trunk/include/clang/StaticAnalyzer/Core/PathSensitive/ExprEngine.h

public:
   const CoreEngine &getCoreEngine() const { return Engine; }
 
 public:
   /// Visit - Transfer function logic for all statements.  Dispatches to
   ///  other functions that handle specific kinds of statements.
   void Visit(const Stmt *S, ExplodedNode *Pred, ExplodedNodeSet &Dst);
 
   /// VisitArraySubscriptExpr - Transfer function for array accesses.
-  void VisitLvalArraySubscriptExpr(const ArraySubscriptExpr *Ex,
-                                   ExplodedNode *Pred,
-                                   ExplodedNodeSet &Dst);
+  void VisitArraySubscriptExpr(const ArraySubscriptExpr *Ex,
 
   /// VisitGCCAsmStmt - Transfer function logic for inline asm.
   void VisitGCCAsmStmt(const GCCAsmStmt *A, ExplodedNode *Pred,
                        ExplodedNodeSet &Dst);
 
   /// VisitMSAsmStmt - Transfer function logic for MS inline asm.
   void VisitMSAsmStmt(const MSAsmStmt *A, ExplodedNode *Pred,
                       ExplodedNodeSet &Dst);

cfe/trunk/lib/StaticAnalyzer/Core/ExprEngine.cpp

case Expr::ObjCBoxedExprClass: {
 
       getCheckerManager().runCheckersForPostStmt(Dst, Tmp, S, *this);
       Bldr.addNodes(Dst);
       break;
     }
 
     case Stmt::ArraySubscriptExprClass:
       Bldr.takeNodes(Pred);
-      VisitLvalArraySubscriptExpr(cast<ArraySubscriptExpr>(S), Pred, Dst);
+      VisitArraySubscriptExpr(cast<ArraySubscriptExpr>(S), Pred, Dst);
       Bldr.addNodes(Dst);
       break;
 
     case Stmt::GCCAsmStmtClass:
       Bldr.takeNodes(Pred);
       VisitGCCAsmStmt(cast<GCCAsmStmt>(S), Pred, Dst);
       Bldr.addNodes(Dst);
       break;
Bldr.generateNode(Ex, Pred, state->BindExpr(Ex, LCtx, V), nullptr,
           ProgramPoint::PostLValueKind);
     return;
   }
 
   llvm_unreachable("Support for this Decl not implemented.");
 }
 
 /// VisitArraySubscriptExpr - Transfer function for array accesses
-void ExprEngine::VisitLvalArraySubscriptExpr(const ArraySubscriptExpr *A,
+void ExprEngine::VisitArraySubscriptExpr(const ArraySubscriptExpr *A,
                                              ExplodedNode *Pred,
                                              ExplodedNodeSet &Dst){
-
   const Expr *Base = A->getBase()->IgnoreParens();
   const Expr *Idx  = A->getIdx()->IgnoreParens();
 
   ExplodedNodeSet CheckerPreStmt;
   getCheckerManager().runCheckersForPreStmt(CheckerPreStmt, Pred, A, *this);
 
   ExplodedNodeSet EvalSet;
   StmtNodeBuilder Bldr(CheckerPreStmt, EvalSet, *currBldrCtx);
-  assert(A->isGLValue() ||
-          (!AMgr.getLangOpts().CPlusPlus &&
-           A->getType().isCForbiddenLValueType()));
+
+  bool IsVectorType = A->getBase()->getType()->isVectorType();
+
+  // The "like" case is for situations where C standard prohibits the type to
+  // be an lvalue, e.g. taking the address of a subscript of an expression of
+  // type "void *".
+  bool IsGLValueLike = A->isGLValue() ||
+    (A->getType().isCForbiddenLValueType() && !AMgr.getLangOpts().CPlusPlus);
 
   for (auto *Node : CheckerPreStmt) {
     const LocationContext *LCtx = Node->getLocationContext();
     ProgramStateRef state = Node->getState();
+
+    if (IsGLValueLike) {
       SVal V = state->getLValue(A->getType(),
-                              state->getSVal(Idx, LCtx),
-                              state->getSVal(Base, LCtx));
       Bldr.generateNode(A, Node, state->BindExpr(A, LCtx, V), nullptr,
-                      ProgramPoint::PostLValueKind);
+    } else if (IsVectorType) {
+      // FIXME: non-glvalue vector reads are not modelled.
+      Bldr.generateNode(A, Node, state, nullptr);
+    } else {
+      llvm_unreachable("Array subscript should be an lValue when not \
+a vector and not a forbidden lvalue type");
+    }
   }
 
   getCheckerManager().runCheckersForPostStmt(Dst, EvalSet, A, *this);
 }
 
 /// VisitMemberExpr - Transfer function for member expressions.
 void ExprEngine::VisitMemberExpr(const MemberExpr *M, ExplodedNode *Pred,
                                  ExplodedNodeSet &Dst) {

cfe/trunk/test/Analysis/vector.c

-// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify %s
-
-typedef int __attribute__((ext_vector_type(2))) V;
-
-void clang_analyzer_numTimesReached();
-void clang_analyzer_eval(int);
-
-int flag;
-
-V pass_through_and_set_flag(V v) {
-  flag = 1;
-  return v;
-}
-
-V dont_crash_and_dont_split_state(V x, V y) {
-  flag = 0;
-  V z = x && pass_through_and_set_flag(y);
-  clang_analyzer_eval(flag); // expected-warning{{TRUE}}
-  // FIXME: For now we treat vector operator && as short-circuit,
-  // but in fact it is not. It should always evaluate
-  // pass_through_and_set_flag(). It should not split state.
-  // Now we also get FALSE on the other path.
-  // expected-warning@-5{{FALSE}}
-
-  // FIXME: Should be 1 since we should not split state.
-  clang_analyzer_numTimesReached(); // expected-warning{{2}}
-  return z;
-}

cfe/trunk/test/Analysis/vector.m

+// RUN: %clang_analyze_cc1 -analyzer-checker=core,debug.ExprInspection -verify %s
+
+typedef int __attribute__((ext_vector_type(2))) V;
+
+void clang_analyzer_warnIfReached();
+void clang_analyzer_numTimesReached();
+void clang_analyzer_eval(int);
+
+int flag;
+
+V pass_through_and_set_flag(V v) {
+  flag = 1;
+  return v;
+}
+
+V dont_crash_and_dont_split_state(V x, V y) {
+  flag = 0;
+  V z = x && pass_through_and_set_flag(y);
+  clang_analyzer_eval(flag); // expected-warning{{TRUE}}
+  // FIXME: For now we treat vector operator && as short-circuit,
+  // but in fact it is not. It should always evaluate
+  // pass_through_and_set_flag(). It should not split state.
+  // Now we also get FALSE on the other path.
+  // expected-warning@-5{{FALSE}}
+
+  // FIXME: Should be 1 since we should not split state.
+  clang_analyzer_numTimesReached(); // expected-warning{{2}}
+  return z;
+}
+
+void test_read() {
+  V x;
+  x[0] = 0;
+  x[1] = 1;
+
+  clang_analyzer_eval(x[0] == 0); // expected-warning{{TRUE}}
+}
+
+V return_vector() {
+  V z;
+  z[0] = 0;
+  z[1] = 0;
+  return z;
+}
+
+int test_vector_access() {
+  return return_vector()[0]; // no-crash no-warning
+}
+
+@interface I
+@property V v;
+@end
+
+// Do not crash on subscript operations into ObjC properties.
+int myfunc(I *i2) {
+  int out = i2.v[0]; // no-crash no-warning
+
+  // Check that the analysis continues.
+  clang_analyzer_warnIfReached(); // expected-warning{{REACHABLE}}
+  return out;
+}