[PATCH] D32410: change the way the expr evaluator handles objcboxedexpr
Richard Smith via Phabricator via cfe-commits
cfe-commits at lists.llvm.org
Thu Apr 27 00:18:49 PDT 2017
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
More information about the cfe-commits
mailing list