change the way the expr evaluator handles objcboxedexpr

[nlewycky]

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.

[rsmith]

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]

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]

LGTM, thanks!

lib/AST/ExprConstant.cpp

   bool VisitUnaryAddrOf(const UnaryOperator *E);
   bool VisitObjCStringLiteral(const ObjCStringLiteral *E)
       { return Success(E); }
-  bool VisitObjCBoxedExpr(const ObjCBoxedExpr *E)
-      { return Success(E); }
+  bool VisitObjCBoxedExpr(const ObjCBoxedExpr *E) {
+    if (Info.noteFailure())
+      EvaluateIgnoredValue(Info, E->getSubExpr());
+    return Error(E);
+  }
   bool VisitAddrLabelExpr(const AddrLabelExpr *E)
       { return Success(E); }
   bool VisitCallExpr(const CallExpr *E);

lib/Sema/SemaChecking.cpp

 
     if (auto InitList = dyn_cast<InitListExpr>(E))
       Exprs.append(InitList->inits().begin(), InitList->inits().end());
+
+    if (isa<ObjCBoxedExpr>(E))
+      E->IgnoreParenCasts()->EvaluateForOverflow(Context);
   } while (!Exprs.empty());
 }
 

lib/Sema/SemaExprObjC.cpp

         break;
       }
     }
-    CheckForIntOverflow(ValueExpr);
     // FIXME:  Do I need to do anything special with BoolTy expressions?
     
     // Look for the appropriate method within NSNumber.

test/CodeGenObjCXX/boxing.mm

+// RUN: %clang_cc1 -triple x86_64-apple-darwin10 -emit-llvm -o - %s | FileCheck %s
+
+@interface NSNumber
++ (id)numberWithInt:(int)n;
+@end
+
+int n = 1;
+int m = (@(n++), 0);
+
+// CHECK: define {{.*}} @__cxx_global_var_init()
+// CHECK: load i32, i32* @n
+// CHECK: store i32 %{{.*}}, i32* @n