Page MenuHomePhabricator

change the way the expr evaluator handles objcboxedexpr
AcceptedPublic

Authored by nlewycky on Apr 23 2017, 3:53 PM.

Details

Reviewers
rsmith
Summary

Make ObjCBoxedExpr less of a special case, teach the expression evaluator to handle it in general, sometimes descending through to its subexpr. Remove the code that called CheckForIntOverflow from outside BuildObjCBoxedExpr, leaving its only caller CheckCompletedExpr.

To make the existing tests continue to work, we also need to whitelist ObjCBoxedExpr as a top-level expression in CheckForIntOverflow. Ultimately that we shouldn't need to whitelist, but one piece at a time.

Diff Detail

Event Timeline

nlewycky created this revision.Apr 23 2017, 3:53 PM
nlewycky retitled this revision from change the way objcboxedexpr is handled to change the way the expr evaluator handles objcboxedexpr.Apr 23 2017, 3:55 PM
rsmith added inline comments.Apr 27 2017, 12:18 AM
lib/AST/ExprConstant.cpp
4469–4470

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.)

5487

Add LLVM_FALLTHROUGH; here, please.

5492

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).

nlewycky updated this revision to Diff 97173.Apr 28 2017, 4:35 PM

If the boxing method can't be constexpr then we can never evaluate it for a constant value, which means that we should unconditionally return Error, and use noteFailure to decide whether to visit the subexpr.

rsmith accepted this revision.Apr 28 2017, 4:38 PM

LGTM, thanks!

This revision is now accepted and ready to land.Apr 28 2017, 4:38 PM