[PATCH] D32410: change the way the expr evaluator handles objcboxedexpr

[Richard Smith via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: lib/AST/ExprConstant.cpp:4469-4470
     { return StmtVisitorTy::Visit(E->getSubExpr()); }
+  bool VisitObjCBoxedExpr(const ObjCBoxedExpr *E)
+    { return StmtVisitorTy::Visit(E->getSubExpr()); }
   bool VisitChooseExpr(const ChooseExpr *E)
----------------
I believe this is unreachable: an `ObjCBoxedExpr` will always have pointer (or dependent) type; the former will be handled below and the latter should never be evaluated at all. (We might want a mode to recurse into non-dependent subexpressions of value-dependent expressions, but that should probably be a separate visitor.)


================
Comment at: lib/AST/ExprConstant.cpp:5487
+          }
+
+        case EvalInfo::EM_ConstantExpression:
----------------
Add `LLVM_FALLTHROUGH;` here, please.


================
Comment at: lib/AST/ExprConstant.cpp:5492
+        case EvalInfo::EM_OffsetFold:
+          return Success(E);
+        }
----------------
As far as I can see, this (from the pre-existing code) is very wrong: the evaluation semantics of `ObjCBoxedExpr` are to evaluate the subexpression and then pass that to a certain Objective-C method to box the result. We can't just skip evaluating the subexpression! We miscompile this, for instance:

```
@interface NSNumber
+ (id)numberWithInt:(int)n;
@end

int n;
int m = (@(n++), 0);
```

... completely losing the increment of `n` because we incorrectly return `Success` here without evaluating the subexpression.

Plus returning `Success` here seems like it's never likely to be useful, since CGExprConstant can't actually emit an `APValue` of this form.

As a minimal fix that would still let us perform this weird evaluation, we could unconditionally call `EvaluateIgnoredValue` here prior to returning `Success(E)`.


https://reviews.llvm.org/D32410