[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

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

341–343

Indent :)

lib/StaticAnalyzer/Core/ExprEngine.cpp

2141

Does this deserve a FIXME?

2161

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

2141

actually, the comment was misplaced, and probably yes.

2161

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

2153

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.

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 VisitArraySubscriptExpr(const ArraySubscriptExpr *Ex,
+                                   ExplodedNode *Pred,
+                                   ExplodedNodeSet &Dst);
+
+  /// VisitLvalArraySubscriptExpr - Transfer function for lvalue access into
+  /// array.
   void VisitLvalArraySubscriptExpr(const ArraySubscriptExpr *Ex,
                                    ExplodedNode *Pred,
                                    ExplodedNodeSet &Dst);
 
   /// 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);

lib/StaticAnalyzer/Core/ExprEngine.cpp

 
       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;

           ProgramPoint::PostLValueKind);
     return;
   }
 
   llvm_unreachable("Support for this Decl not implemented.");
 }
 
 /// VisitArraySubscriptExpr - Transfer function for array accesses
+///
+/// Ignores array subscripts which return rvalues.
+void ExprEngine::VisitArraySubscriptExpr(const ArraySubscriptExpr *A,
+                                             ExplodedNode *Pred,
+                                             ExplodedNodeSet &Dst){
+  if (A->isGLValue())
+    return VisitLvalArraySubscriptExpr(A, Pred, Dst);
+}
+
+
+/// VisitArraySubscriptExpr - Transfer function for lvalue array accesses
 void ExprEngine::VisitLvalArraySubscriptExpr(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()));
 
   for (auto *Node : CheckerPreStmt) {
     const LocationContext *LCtx = Node->getLocationContext();
     ProgramStateRef state = Node->getState();
     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);
   }
 
   getCheckerManager().runCheckersForPostStmt(Dst, EvalSet, A, *this);
 }
 
 /// VisitMemberExpr - Transfer function for member expressions.
 void ExprEngine::VisitMemberExpr(const MemberExpr *M, ExplodedNode *Pred,
                                  ExplodedNodeSet &Dst) {
 

test/Analysis/ObjCPropertiesSyntaxChecks.m

 @end
 
 @protocol SomeProtocol
 // Don't warn on protocol properties because it is possible to
 // conform to them correctly; it is only synthesized setters that
 // that are definitely incorrect.
 @property (copy) NSMutableString *myProp; // no-crash // no-warning
 @end
+
+typedef __attribute__((__ext_vector_type__(3))) int Vector;
+
+@interface I2
+@property Vector v;
+@end
+
+// Do not crash on subscript operations into ObjC properties.
+int myfunc(I2 *i2) {
+  return i2.v[0]; // no-crash no-warning
+}