[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

335–337 ↗

(On Diff #125374)

Indent :)

lib/StaticAnalyzer/Core/ExprEngine.cpp

2130

Does this deserve a FIXME?

2151

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

actually, the comment was misplaced, and probably yes.

2151

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

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.

lib/StaticAnalyzer/Core/ExprEngine.cpp

 
   llvm_unreachable("Support for this Decl not implemented.");
 }
 
 /// VisitArraySubscriptExpr - Transfer function for 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()));
+      (!AMgr.getLangOpts().CPlusPlus && A->getType().isCForbiddenLValueType())
+
+      // Subscripts into ObjCProperties are always rvalues.
+      || (isa<PseudoObjectExpr>(Base) && isa<ObjCPropertyRefExpr>(
+          cast<PseudoObjectExpr>(Base)->getSyntacticForm())));
 
   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.

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
+}