[Sema] Note when we've actually encountered a failure in ExprConstant, and take that into account when looking up objects.

[george.burgess.iv]

Summary

This patch aims to fix a bug encountered by compiling code with C++14.

Specifically, when evaluating p in __builtin_object_size(p, n), we start speculatively evaluating. Currently, clang pretends that side-effects have always occurred when we start speculatively evaluating. This prevents us from reading any locals in C++14. So, the following code compiles fine in C++11 mode, but fails to compile with C++14:

void foo(char *p) __attribute__((enable_if(__builtin_object_size(p, 0) != -1, "")));

void runFoo() {
  char buf[5];
  foo(buf);
}

This patch makes ExprConstant note when we've actually failed (read: may have an unmodeled side-effect), and makes us act as conservatively before if a failure happened. If a failure hasn't happened, we can be more aggressive.

[george.burgess.iv]

test/SemaCXX/constant-expression-cxx1y.cpp

182

Aside: This test was fixed because we no longer give up on trying to look up foo, because we know an unmodeled side-effect hasn't happened.

[rsmith]

include/clang/AST/Expr.h

541–546 ↗

(On Diff #51872)

I don't think this provides enough guarantees to be useful as part of the external EvalStatus results. Can you sink this into EvalInfo? Alternatively, can you merge this with HasSideEffects? I don't think we need to maintain a distinction between the two different ways we can get an evaluation result after skipping a subexpression.

[george.burgess.iv]

Addressed all feedback.

include/clang/AST/Expr.h

541–546 ↗

(On Diff #51872)

I don't think this provides enough guarantees to be useful as part of the external EvalStatus results. Can you sink this into EvalInfo

WFM

Alternatively, can you merge this with HasSideEffects.

The result is a bit more subtle (because some things depend on HasSideEffects being accurate, and not being set whenever we fail), but sure.

[george.burgess.iv]

Ping :)

[rsmith]

lib/AST/ExprConstant.cpp

782

unchange -> unchanged

784–785

I think this is the right behavior regardless of the current expectations of any particular caller. After this patch:

• HasSideEffects == false indicates that the evaluation (if evaluation was successful, then up to the current point, otherwise up to the point where evaluation failed) has definitely not skipped any side-effects
• HasSideEffects == true means nothing is known about side-effects (in particular, it does not guarantee that a side-effect has definitely happened, just that one /might/ have happened)

(If any caller expects these flags to mean something else, they are wrong -- and note that HasSideEffects is meaningless if evaluation fails.) Therefore, if we transition from an "evaluation has failed" to an "evaluation was successful" state, we must set HasSideEffects to true to model the subexpressions we might have skipped. And if we just unwind (because keepEvaluatingAfterFailure() returns false), then we don't need to set HasSideEffects at all.

Perhaps you could say something like this:

"Failure when evaluating some expression often means there is some subexpression whose evaluation was skipped. Therefore, (because we don't track whether we skipped an expression when unwinding after an evaluation failure) every evaluation failure that bubbles up from a subexpression implies that a side-effect has potentially happened. We skip setting the HasSideEffects flag to true until we decide to continue evaluating after that point, which happens here."

853–854

I think this comment is now redundant / incorrect.

2644

IsSpeculativelyEvaluating is always false. Presumably it should be set to true in the SpeculativeEvaluationRAII constructor. (Please also add a testcase to catch this...)

3279

Can we give this function a name that reads more naturally at the call site? It's locally unclear what this condition is testing. Maybe keep the old name?

4075–4077

These RAII objects are cheap; maybe just create two of them regardless?

4104–4105

I think this should be an if rather than an assert. If we hit something that's known to be non-constant while evaluating the condition of a ?:, we should bail out of evaluation in checkingPotentialConstantExpression mode. keepEvaluatingAfterFailure should return false in that case (even though it doesn't do so today).

7397–7398

The side-effect here should be delayed until after we've evaluated the left and right hand sides of the assignment.

[george.burgess.iv]

Addressed feedback

[george.burgess.iv]

lib/AST/ExprConstant.cpp

853–854

My bad :)

3279

The best alternative I can come up with is noteFailureAndGetShouldContinueEvaluating(), which occupies 44 characters.

...I think I'll go back to the old name.

4104–4105

I'm not sure that I fully understand what you mean by this. Are you saying that we (ultimately) only want to check the arms of a conditional when we're evaluating for overflow?

Either way, took a stab at fixing it.

7103

I'm happy to make SpeculativeEvalRAII a type that optionally restores state, if you think that would be better. That way, we don't need to duplicate this logic...

Also, I can't figure out how to test this, because it's only ever run after HasSideEffects = true, and the only thing that (I think) checks for if we're speculatively evaluating *also* fails in exactly the same way if we have side effects.

[george.burgess.iv]

Ping :)

[george.burgess.iv]

Ping :)

[rsmith]

LGTM with or without the SpeculativeEvaluationRAII refactor (which is ultimately pre-existing duplication).

Please commit the rename of keepEvaluatingAfterFailure -> noteFailure separately.

lib/AST/ExprConstant.cpp

4104–4105

The relevant case is when we're checking the body of a constexpr function to see whether it could possibly be a constant expression. In that case, what we do next should depend on whether we've already found something that is known to never be constant. If so (which is indicated by us having a diagnostic), we should bail out. If not, we should check both arms of the conditional operator and report a problem if neither of them can be constant.

I think the upshot of all of that is that keepEvaluatingAfterFailure should return false for EM_PotentialConstantExpression* if we have already produced a diagnostic. But that's not relevant for this particular patch, so let's leave it as you have it for now. We can revisit this.

7103

Yes, if we can reuse SpeculativeEvaluationRAII here, or otherwise factor out the duplication, that would be better. Also I think you're probably right that this is not observable right now.

test/SemaCXX/constant-expression-cxx1y.cpp

942

Unused?

lib/AST/ExprConstant.cpp

struct EvalInfo {
     /// HasActiveDiagnostic - Was the previous diagnostic stored? If so, further
     /// notes attached to it will also be stored, otherwise they will not be.
     bool HasActiveDiagnostic;
 
     /// \brief Have we emitted a diagnostic explaining why we couldn't constant
     /// fold (not just why it's not strictly a constant expression)?
     bool HasFoldFailureDiagnostic;
 
+    /// \brief Whether or not we're currently speculatively evaluating.
+    bool IsSpeculativelyEvaluating;
+
     enum EvaluationMode {
       /// Evaluate as a constant expression. Stop if we find that the expression
       /// is not a constant expression.
       EM_ConstantExpression,
 
       /// Evaluate as a potential constant expression. Keep going if we hit a
       /// construct that we can't evaluate yet (because we don't yet know the
       /// value of something) but stop if we hit something that could never be
struct EvalInfo {
 
     EvalInfo(const ASTContext &C, Expr::EvalStatus &S, EvaluationMode Mode)
       : Ctx(const_cast<ASTContext &>(C)), EvalStatus(S), CurrentCall(nullptr),
         CallStackDepth(0), NextCallIndex(1),
         StepsLeft(getLangOpts().ConstexprStepLimit),
         BottomFrame(*this, SourceLocation(), nullptr, nullptr, nullptr),
         EvaluatingDecl((const ValueDecl *)nullptr),
         EvaluatingDeclValue(nullptr), HasActiveDiagnostic(false),
-        HasFoldFailureDiagnostic(false), EvalMode(Mode) {}
+        HasFoldFailureDiagnostic(false), IsSpeculativelyEvaluating(false),
+        EvalMode(Mode) {}
 
     void setEvaluatingDecl(APValue::LValueBase Base, APValue &Value) {
       EvaluatingDecl = Base;
       EvaluatingDeclValue = &Value;
     }
 
     const LangOptions &getLangOpts() const { return Ctx.getLangOpts(); }
 
bool keepEvaluatingAfterFailure() {
       case EM_ConstantFold:
       case EM_IgnoreSideEffects:
       case EM_DesignatorFold:
         return false;
       }
       llvm_unreachable("Missed EvalMode case");
     }
 
+    /// Notes that we failed to evaluate an expression that other expressions
+    /// directly depend on, and determine if we should keep evaluating. This
+    /// should only be called if we actually intend to keep evaluating.
+    ///
+    /// Call noteSideEffect() instead if we may be able to ignore the value that
+    /// we failed to evaluate, e.g. if we failed to evaluate Foo() in:
+    ///
+    /// (Foo(), 1)      // use noteSideEffect
+    /// (Foo() || true) // use noteSideEffect
+    /// Foo() + 1       // use noteFailure
+    LLVM_ATTRIBUTE_UNUSED_RESULT bool noteFailure() {
+      // Failure when evaluating some expression often means there is some
+      // subexpression whose evaluation was skipped. Therefore, (because we
+      // don't track whether we skipped an expression when unwinding after an
+      // evaluation failure) every evaluation failure that bubbles up from a
+      // subexpression implies that a side-effect has potentially happened. We
+      // skip setting the HasSideEffects flag to true until we decide to
+      // continue evaluating after that point, which happens here.
+      bool KeepGoing = keepEvaluatingAfterFailure();
+      EvalStatus.HasSideEffects |= KeepGoing;
+      return KeepGoing;
+    }
+
     bool allowInvalidBaseExpr() const {
       return EvalMode == EM_DesignatorFold;
     }
   };
 
   /// Object used to treat all foldable expressions as constant expressions.
   struct FoldConstant {
     EvalInfo &Info;
explicit FoldOffsetRAII(EvalInfo &Info, bool Subobject)
       if (!Info.checkingPotentialConstantExpression())
         Info.EvalMode = Subobject ? EvalInfo::EM_DesignatorFold
                                   : EvalInfo::EM_ConstantFold;
     }
 
     ~FoldOffsetRAII() { Info.EvalMode = OldMode; }
   };
 
-  /// RAII object used to suppress diagnostics and side-effects from a
-  /// speculative evaluation.
+  /// RAII object used to potentially suppress diagnostics and side-effects from
+  /// a speculative evaluation.
   class SpeculativeEvaluationRAII {
     EvalInfo &Info;
     Expr::EvalStatus Old;
-
+    bool OldSpecEval;
   public:
-    SpeculativeEvaluationRAII(EvalInfo &Info,
-                        SmallVectorImpl<PartialDiagnosticAt> *NewDiag = nullptr)
-      : Info(Info), Old(Info.EvalStatus) {
+    SpeculativeEvaluationRAII(
+        EvalInfo &Info, SmallVectorImpl<PartialDiagnosticAt> *NewDiag = nullptr)
+        : Info(Info), Old(Info.EvalStatus),
+          OldSpecEval(Info.IsSpeculativelyEvaluating) {
       Info.EvalStatus.Diag = NewDiag;
-      // If we're speculatively evaluating, we may have skipped over some
-      // evaluations and missed out a side effect.
-      Info.EvalStatus.HasSideEffects = true;
+      Info.IsSpeculativelyEvaluating = true;
     }
+
     ~SpeculativeEvaluationRAII() {
       Info.EvalStatus = Old;
+      Info.IsSpeculativelyEvaluating = OldSpecEval;
     }
   };
 
   /// RAII object wrapping a full-expression or block scope, and handling
   /// the ending of the lifetime of temporaries created within it.
   template<bool IsFullExpression>
   class ScopeRAII {
     EvalInfo &Info;
static CompleteObject findCompleteObject(EvalInfo &Info, const Expr *E,
                                          AccessKinds AK, const LValue &LVal,
                                          QualType LValType) {
   if (!LVal.Base) {
     Info.Diag(E, diag::note_constexpr_access_null) << AK;
     return CompleteObject();
   }
 
   CallStackFrame *Frame = nullptr;
   if (LVal.CallIndex) {
     Frame = Info.getCallFrame(LVal.CallIndex);
     if (!Frame) {
       Info.Diag(E, diag::note_constexpr_lifetime_ended, 1)
         << AK << LVal.Base.is<const ValueDecl*>();
       NoteLValueLocation(Info, LVal.Base);
       return CompleteObject();
     }
   }
static CompleteObject findCompleteObject(EvalInfo &Info, const Expr *E,
   // and this doesn't do quite the right thing for const subobjects of the
   // object under construction.
   if (LVal.getLValueBase() == Info.EvaluatingDecl) {
     BaseType = Info.Ctx.getCanonicalType(BaseType);
     BaseType.removeLocalConst();
   }
 
   // In C++1y, we can't safely access any mutable state when we might be
-  // evaluating after an unmodeled side effect or an evaluation failure.
+  // evaluating after an unmodeled side effect.
   //
   // FIXME: Not all local state is mutable. Allow local constant subobjects
   // to be read here (but take care with 'mutable' fields).
-  if (Frame && Info.getLangOpts().CPlusPlus14 &&
-      (Info.EvalStatus.HasSideEffects || Info.keepEvaluatingAfterFailure()))
+  if ((Frame && Info.getLangOpts().CPlusPlus14 &&
+       Info.EvalStatus.HasSideEffects) ||
+      (AK != AK_Read && Info.IsSpeculativelyEvaluating))
     return CompleteObject();
 
   return CompleteObject(BaseVal, BaseType);
 }
 
 /// \brief Perform an lvalue-to-rvalue conversion on the given glvalue. This
 /// can also be used for 'lvalue-to-lvalue' conversions for looking up the
 /// glvalue referred to by an entity of reference type.

 
 static const ValueDecl *HandleMemberPointerAccess(EvalInfo &Info,
                                                   const BinaryOperator *BO,
                                                   LValue &LV,
                                                   bool IncludeMember = true) {
   assert(BO->getOpcode() == BO_PtrMemD || BO->getOpcode() == BO_PtrMemI);
 
   if (!EvaluateObjectArgument(Info, BO->getLHS(), LV)) {
-    if (Info.keepEvaluatingAfterFailure()) {
+    if (Info.noteFailure()) {
       MemberPtr MemPtr;
       EvaluateMemberPointer(BO->getRHS(), MemPtr, Info);
     }
     return nullptr;
   }
 
   return HandleMemberPointerAccess(Info, BO->getLHS()->getType(), LV,
                                    BO->getRHS(), IncludeMember);
static EvalStmtResult EvaluateStmt(StmtResult &Result, EvalInfo &Info,
 
   case Stmt::DeclStmtClass: {
     const DeclStmt *DS = cast<DeclStmt>(S);
     for (const auto *DclIt : DS->decls()) {
       // Each declaration initialization is its own full-expression.
       // FIXME: This isn't quite right; if we're performing aggregate
       // initialization, each braced subexpression is its own full-expression.
       FullExpressionRAII Scope(Info);
-      if (!EvaluateDecl(Info, DclIt) && !Info.keepEvaluatingAfterFailure())
+      if (!EvaluateDecl(Info, DclIt) && !Info.noteFailure())
         return ESR_Failed;
     }
     return ESR_Succeeded;
   }
 
   case Stmt::ReturnStmtClass: {
     const Expr *RetExpr = cast<ReturnStmt>(S)->getRetValue();
     FullExpressionRAII Scope(Info);

 static bool EvaluateArgs(ArrayRef<const Expr*> Args, ArgVector &ArgValues,
                          EvalInfo &Info) {
   bool Success = true;
   for (ArrayRef<const Expr*>::iterator I = Args.begin(), E = Args.end();
        I != E; ++I) {
     if (!Evaluate(ArgValues[I - Args.begin()], Info, *I)) {
       // If we're checking for a potential constant expression, evaluate all
       // initializers even if some of them fail.
-      if (!Info.keepEvaluatingAfterFailure())
+      if (!Info.noteFailure())
         return false;
       Success = false;
     }
   }
   return Success;
 }
 
 /// Evaluate a function call.
#endif
     }
 
     FullExpressionRAII InitScope(Info);
     if (!EvaluateInPlace(*Value, Info, Subobject, I->getInit()) ||
         (FD && FD->isBitField() && !truncateBitfieldValue(Info, I->getInit(),
                                                           *Value, FD))) {
       // If we're checking for a potential constant expression, evaluate all
       // initializers even if some of them fail.
-      if (!Info.keepEvaluatingAfterFailure())
+      if (!Info.noteFailure())
         return false;
       Success = false;
     }
   }
 
   return Success &&
          EvaluateStmt(Ret, Info, Definition->getBody()) != ESR_Failed;
 }
private:
   }
 
   // Check whether a conditional operator with a non-constant condition is a
   // potential constant expression. If neither arm is a potential constant
   // expression, then the conditional operator is not either.
   template<typename ConditionalOperator>
   void CheckPotentialConstantConditional(const ConditionalOperator *E) {
     assert(Info.checkingPotentialConstantExpression());
 
     // Speculatively evaluate both arms.
-    {
-      SmallVector<PartialDiagnosticAt, 8> Diag;
+    {
       SpeculativeEvaluationRAII Speculate(Info, &Diag);
-
       StmtVisitorTy::Visit(E->getFalseExpr());
       if (Diag.empty())
         return;
+    }
 
+    {
+      SpeculativeEvaluationRAII Speculate(Info, &Diag);
       Diag.clear();
       StmtVisitorTy::Visit(E->getTrueExpr());
       if (Diag.empty())
         return;
     }
 
     Error(E, diag::note_constexpr_conditional_never_const);
   }
 
 
   template<typename ConditionalOperator>
   bool HandleConditionalOperator(const ConditionalOperator *E) {
     bool BoolResult;
     if (!EvaluateAsBooleanCondition(E->getCond(), BoolResult, Info)) {
-      if (Info.checkingPotentialConstantExpression())
+      if (Info.checkingPotentialConstantExpression() && Info.noteFailure())
         CheckPotentialConstantConditional(E);
       return false;
     }
 
     Expr *EvalExpr = BoolResult ? E->getTrueExpr() : E->getFalseExpr();
     return StmtVisitorTy::Visit(EvalExpr);
   }
 
 protected:
   EvalInfo &Info;
   typedef ConstStmtVisitor<Derived, bool> StmtVisitorTy;
   typedef ExprEvaluatorBase ExprEvaluatorBaseTy;
bool LValueExprEvaluator::VisitCompoundAssignOperator(
     const CompoundAssignOperator *CAO) {
   if (!Info.getLangOpts().CPlusPlus14 && !Info.keepEvaluatingAfterFailure())
     return Error(CAO);
 
   APValue RHS;
 
   // The overall lvalue result is the result of evaluating the LHS.
   if (!this->Visit(CAO->getLHS())) {
-    if (Info.keepEvaluatingAfterFailure())
+    if (Info.noteFailure())
       Evaluate(RHS, this->Info, CAO->getRHS());
     return false;
   }
 
   if (!Evaluate(RHS, this->Info, CAO->getRHS()))
     return false;
 
   return handleCompoundAssignment(
       this->Info, CAO,
       Result, CAO->getLHS()->getType(), CAO->getComputationLHSType(),
       CAO->getOpForCompoundAssignment(CAO->getOpcode()), RHS);
 }
 
 bool LValueExprEvaluator::VisitBinAssign(const BinaryOperator *E) {
   if (!Info.getLangOpts().CPlusPlus14 && !Info.keepEvaluatingAfterFailure())
     return Error(E);
 
   APValue NewVal;
 
   if (!this->Visit(E->getLHS())) {
-    if (Info.keepEvaluatingAfterFailure())
+    if (Info.noteFailure())
       Evaluate(NewVal, this->Info, E->getRHS());
     return false;
   }
 
   if (!Evaluate(NewVal, this->Info, E->getRHS()))
     return false;
 
   return handleAssignment(this->Info, E, Result, E->getLHS()->getType(),
if (E->getOpcode() != BO_Add &&
     return ExprEvaluatorBaseTy::VisitBinaryOperator(E);
 
   const Expr *PExp = E->getLHS();
   const Expr *IExp = E->getRHS();
   if (IExp->getType()->isPointerType())
     std::swap(PExp, IExp);
 
   bool EvalPtrOK = EvaluatePointer(PExp, Result, Info);
-  if (!EvalPtrOK && !Info.keepEvaluatingAfterFailure())
+  if (!EvalPtrOK && !Info.noteFailure())
     return false;
 
   llvm::APSInt Offset;
   if (!EvaluateInteger(IExp, Offset, Info) || !EvalPtrOK)
     return false;
 
   int64_t AdditionalOffset = getExtValue(Offset);
   if (E->getOpcode() == BO_Sub)
for (const auto &Base : CXXRD->bases()) {
       const Expr *Init = E->getInit(ElementNo);
 
       LValue Subobject = This;
       if (!HandleLValueBase(Info, Init, Subobject, CXXRD, &Base))
         return false;
 
       APValue &FieldVal = Result.getStructBase(ElementNo);
       if (!EvaluateInPlace(FieldVal, Info, Subobject, Init)) {
-        if (!Info.keepEvaluatingAfterFailure())
+        if (!Info.noteFailure())
           return false;
         Success = false;
       }
       ++ElementNo;
     }
   }
 
   // Initialize members.
for (const auto *Field : RD->fields()) {
     // Temporarily override This, in case there's a CXXDefaultInitExpr in here.
     ThisOverrideRAII ThisOverride(*Info.CurrentCall, &This,
                                   isa<CXXDefaultInitExpr>(Init));
 
     APValue &FieldVal = Result.getStructField(Field->getFieldIndex());
     if (!EvaluateInPlace(FieldVal, Info, Subobject, Init) ||
         (Field->isBitField() && !truncateBitfieldValue(Info, Init,
                                                        FieldVal, Field))) {
-      if (!Info.keepEvaluatingAfterFailure())
+      if (!Info.noteFailure())
         return false;
       Success = false;
     }
   }
 
   return Success;
 }
 
bool ArrayExprEvaluator::VisitInitListExpr(const InitListExpr *E) {
   Subobject.addArray(Info, E, CAT);
   for (unsigned Index = 0; Index != NumEltsToInit; ++Index) {
     const Expr *Init =
         Index < E->getNumInits() ? E->getInit(Index) : FillerExpr;
     if (!EvaluateInPlace(Result.getArrayInitializedElt(Index),
                          Info, Subobject, Init) ||
         !HandleLValueArrayAdjustment(Info, Init, Subobject,
                                      CAT->getElementType(), 1)) {
-      if (!Info.keepEvaluatingAfterFailure())
+      if (!Info.noteFailure())
         return false;
       Success = false;
     }
   }
 
   if (!Result.hasArrayFiller())
     return Success;
 
class DataRecursiveIntBinOpEvaluator {
   struct Job {
     const Expr *E;
     EvalResult LHSResult; // meaningful only for binary operator expression.
     enum { AnyExprKind, BinOpKind, BinOpVisitedLHSKind } Kind;
 
     Job() = default;
     Job(Job &&J)
         : E(J.E), LHSResult(J.LHSResult), Kind(J.Kind),
-          StoredInfo(J.StoredInfo), OldEvalStatus(J.OldEvalStatus) {
+          StoredInfo(J.StoredInfo), OldEvalStatus(J.OldEvalStatus),
+          WasSpecEval(J.WasSpecEval) {
       J.StoredInfo = nullptr;
     }
 
     void startSpeculativeEval(EvalInfo &Info) {
       OldEvalStatus = Info.EvalStatus;
+      WasSpecEval = Info.IsSpeculativelyEvaluating;
       Info.EvalStatus.Diag = nullptr;
+      Info.IsSpeculativelyEvaluating = true;
       StoredInfo = &Info;
     }
     ~Job() {
       if (StoredInfo) {
         StoredInfo->EvalStatus = OldEvalStatus;
+        StoredInfo->IsSpeculativelyEvaluating = WasSpecEval;
       }
     }
   private:
     EvalInfo *StoredInfo = nullptr; // non-null if status changed.
     Expr::EvalStatus OldEvalStatus;
+    bool WasSpecEval;
   };
 
   SmallVector<Job, 16> Queue;
 
   IntExprEvaluator &IntEval;
   EvalInfo &Info;
   APValue &FinalResult;
 
if (!LHSResult.Failed && HandleConversionToBool(LHSResult.Val, LHSAsBool)) {
       if (LHSAsBool == (E->getOpcode() == BO_LOr)) {
         Success(LHSAsBool, E, LHSResult.Val);
         return false; // Ignore RHS
       }
     } else {
       LHSResult.Failed = true;
 
       // Since we weren't able to evaluate the left hand side, it
-      // must have had side effects.
+      // might have had side effects.
       if (!Info.noteSideEffect())
         return false;
 
       // We can't evaluate the LHS; however, sometimes the result
       // is determined by the RHS: X && 0 -> 0, X || 1 -> 1.
       // Don't ignore RHS and suppress diagnostics from this arm.
       SuppressRHSDiags = true;
     }
 
     return true;
   }
 
   assert(E->getLHS()->getType()->isIntegralOrEnumerationType() &&
          E->getRHS()->getType()->isIntegralOrEnumerationType());
 
-  if (LHSResult.Failed && !Info.keepEvaluatingAfterFailure())
+  if (LHSResult.Failed && !Info.noteFailure())
     return false; // Ignore RHS;
 
   return true;
 }
 
 bool DataRecursiveIntBinOpEvaluator::
        VisitBinOp(const EvalResult &LHSResult, const EvalResult &RHSResult,
                   const BinaryOperator *E, APValue &Result) {
case Job::BinOpVisitedLHSKind: {
       Queue.pop_back();
       return;
     }
   }
   
   llvm_unreachable("Invalid Job::Kind!");
 }
 
+namespace {
+/// Used when we determine that we should fail, but can keep evaluating prior to
+/// noting that we had a failure.
+class DelayedNoteFailureRAII {
+  EvalInfo &Info;
+  bool NoteFailure;
+
+public:
+  DelayedNoteFailureRAII(EvalInfo &Info, bool NoteFailure = true)
+      : Info(Info), NoteFailure(NoteFailure) {}
+  ~DelayedNoteFailureRAII() {
+    if (NoteFailure) {
+      bool ContinueAfterFailure = Info.noteFailure();
+      (void)ContinueAfterFailure;
+      assert(ContinueAfterFailure &&
+             "Shouldn't have kept evaluating on failure.");
+    }
+  }
+};
+}
+
 bool IntExprEvaluator::VisitBinaryOperator(const BinaryOperator *E) {
+  // We don't call noteFailure immediately because the assignment happens after
+  // we evaluate LHS and RHS.
   if (!Info.keepEvaluatingAfterFailure() && E->isAssignmentOp())
     return Error(E);
 
+  DelayedNoteFailureRAII MaybeNoteFailureLater(Info, E->isAssignmentOp());
   if (DataRecursiveIntBinOpEvaluator::shouldEnqueue(E))
     return DataRecursiveIntBinOpEvaluator(*this, Result).Traverse(E);
 
   QualType LHSTy = E->getLHS()->getType();
   QualType RHSTy = E->getRHS()->getType();
 
   if (LHSTy->isAnyComplexType() || RHSTy->isAnyComplexType()) {
     ComplexValue LHS, RHS;
     bool LHSOK;
     if (E->isAssignmentOp()) {
       LValue LV;
       EvaluateLValue(E->getLHS(), LV, Info);
       LHSOK = false;
     } else if (LHSTy->isRealFloatingType()) {
       LHSOK = EvaluateFloat(E->getLHS(), LHS.FloatReal, Info);
       if (LHSOK) {
         LHS.makeComplexFloat();
         LHS.FloatImag = APFloat(LHS.FloatReal.getSemantics());
       }
     } else {
       LHSOK = EvaluateComplex(E->getLHS(), LHS, Info);
     }
-    if (!LHSOK && !Info.keepEvaluatingAfterFailure())
+    if (!LHSOK && !Info.noteFailure())
       return false;
 
     if (E->getRHS()->getType()->isRealFloatingType()) {
       if (!EvaluateFloat(E->getRHS(), RHS.FloatReal, Info) || !LHSOK)
         return false;
       RHS.makeComplexFloat();
       RHS.FloatImag = APFloat(RHS.FloatReal.getSemantics());
     } else if (!EvaluateComplex(E->getRHS(), RHS, Info) || !LHSOK)
if (LHSTy->isAnyComplexType() || RHSTy->isAnyComplexType()) {
     }
   }
 
   if (LHSTy->isRealFloatingType() &&
       RHSTy->isRealFloatingType()) {
     APFloat RHS(0.0), LHS(0.0);
 
     bool LHSOK = EvaluateFloat(E->getRHS(), RHS, Info);
-    if (!LHSOK && !Info.keepEvaluatingAfterFailure())
+    if (!LHSOK && !Info.noteFailure())
       return false;
 
     if (!EvaluateFloat(E->getLHS(), LHS, Info) || !LHSOK)
       return false;
 
     APFloat::cmpResult CR = LHS.compare(RHS);
 
     switch (E->getOpcode()) {
if (LHSTy->isRealFloatingType() &&
     }
   }
 
   if (LHSTy->isPointerType() && RHSTy->isPointerType()) {
     if (E->getOpcode() == BO_Sub || E->isComparisonOp()) {
       LValue LHSValue, RHSValue;
 
       bool LHSOK = EvaluatePointer(E->getLHS(), LHSValue, Info);
-      if (!LHSOK && !Info.keepEvaluatingAfterFailure())
+      if (!LHSOK && !Info.noteFailure())
         return false;
 
       if (!EvaluatePointer(E->getRHS(), RHSValue, Info) || !LHSOK)
         return false;
 
       // Reject differing bases from the normal codepath; we special-case
       // comparisons to null.
       if (!HasSameBase(LHSValue, RHSValue)) {
bool IntExprEvaluator::VisitBinaryOperator(const BinaryOperator *E) {
 
   if (LHSTy->isMemberPointerType()) {
     assert(E->isEqualityOp() && "unexpected member pointer operation");
     assert(RHSTy->isMemberPointerType() && "invalid comparison");
 
     MemberPtr LHSValue, RHSValue;
 
     bool LHSOK = EvaluateMemberPointer(E->getLHS(), LHSValue, Info);
-    if (!LHSOK && Info.keepEvaluatingAfterFailure())
+    if (!LHSOK && !Info.noteFailure())
       return false;
 
     if (!EvaluateMemberPointer(E->getRHS(), RHSValue, Info) || !LHSOK)
       return false;
 
     // C++11 [expr.eq]p2:
     //   If both operands are null, they compare equal. Otherwise if only one is
     //   null, they compare unequal.

 }
 
 bool FloatExprEvaluator::VisitBinaryOperator(const BinaryOperator *E) {
   if (E->isPtrMemOp() || E->isAssignmentOp() || E->getOpcode() == BO_Comma)
     return ExprEvaluatorBaseTy::VisitBinaryOperator(E);
 
   APFloat RHS(0.0);
   bool LHSOK = EvaluateFloat(E->getLHS(), Result, Info);
-  if (!LHSOK && !Info.keepEvaluatingAfterFailure())
+  if (!LHSOK && !Info.noteFailure())
     return false;
   return EvaluateFloat(E->getRHS(), RHS, Info) && LHSOK &&
          handleFloatFloatBinOp(Info, E, Result, E->getOpcode(), RHS);
 }
 
 bool FloatExprEvaluator::VisitFloatingLiteral(const FloatingLiteral *E) {
   Result = E->getValue();
   return true;
if (E->getLHS()->getType()->isRealFloatingType()) {
     LHSOK = EvaluateFloat(E->getLHS(), Real, Info);
     if (LHSOK) {
       Result.makeComplexFloat();
       Result.FloatImag = APFloat(Real.getSemantics());
     }
   } else {
     LHSOK = Visit(E->getLHS());
   }
-  if (!LHSOK && !Info.keepEvaluatingAfterFailure())
+  if (!LHSOK && !Info.noteFailure())
     return false;
 
   ComplexValue RHS;
   if (E->getRHS()->getType()->isRealFloatingType()) {
     RHSReal = true;
     APFloat &Real = RHS.FloatReal;
     if (!EvaluateFloat(E->getRHS(), Real, Info) || !LHSOK)
       return false;

test/SemaCXX/builtin-object-size-cxx14.cpp

+// RUN: %clang_cc1 -fsyntax-only -verify -std=c++14 %s
+
+namespace basic {
+// Ensuring that __bos can be used in constexpr functions without anything
+// sketchy going on...
+constexpr int bos0() {
+  int k = 5;
+  char cs[10] = {};
+  return __builtin_object_size(&cs[k], 0);
+}
+
+constexpr int bos1() {
+  int k = 5;
+  char cs[10] = {};
+  return __builtin_object_size(&cs[k], 1);
+}
+
+constexpr int bos2() {
+  int k = 5;
+  char cs[10] = {};
+  return __builtin_object_size(&cs[k], 2);
+}
+
+constexpr int bos3() {
+  int k = 5;
+  char cs[10] = {};
+  return __builtin_object_size(&cs[k], 3);
+}
+
+static_assert(bos0() == sizeof(char) * 5, "");
+static_assert(bos1() == sizeof(char) * 5, "");
+static_assert(bos2() == sizeof(char) * 5, "");
+static_assert(bos3() == sizeof(char) * 5, "");
+}
+
+namespace in_enable_if {
+// The code that prompted these changes was __bos in enable_if
+
+void copy5CharsInto(char *buf) // expected-note{{candidate}}
+    __attribute__((enable_if(__builtin_object_size(buf, 0) != -1 &&
+                                 __builtin_object_size(buf, 0) > 5,
+                             "")));
+
+// We use different EvalModes for __bos with type 0 versus 1. Ensure 1 works,
+// too...
+void copy5CharsIntoStrict(char *buf) // expected-note{{candidate}}
+    __attribute__((enable_if(__builtin_object_size(buf, 1) != -1 &&
+                                 __builtin_object_size(buf, 1) > 5,
+                             "")));
+
+struct LargeStruct {
+  int pad;
+  char buf[6];
+  int pad2;
+};
+
+struct SmallStruct {
+  int pad;
+  char buf[5];
+  int pad2;
+};
+
+void noWriteToBuf() {
+  char buf[6];
+  copy5CharsInto(buf);
+
+  LargeStruct large;
+  copy5CharsIntoStrict(large.buf);
+}
+
+void initTheBuf() {
+  char buf[6] = {};
+  copy5CharsInto(buf);
+
+  LargeStruct large = {0, {}, 0};
+  copy5CharsIntoStrict(large.buf);
+}
+
+int getI();
+void initTheBufWithALoop() {
+  char buf[6] = {};
+  for (unsigned I = getI(); I != sizeof(buf); ++I)
+    buf[I] = I;
+  copy5CharsInto(buf);
+
+  LargeStruct large;
+  for (unsigned I = getI(); I != sizeof(buf); ++I)
+    large.buf[I] = I;
+  copy5CharsIntoStrict(large.buf);
+}
+
+void tooSmallBuf() {
+  char buf[5];
+  copy5CharsInto(buf); // expected-error{{no matching function for call}}
+
+  SmallStruct small;
+  copy5CharsIntoStrict(small.buf); // expected-error{{no matching function for call}}
+}
+}

test/SemaCXX/constant-expression-cxx1y.cpp

namespace string_assign {
   }
   static_assert(!test1(1), "");
   static_assert(test1(3), "");
   static_assert(!test1(6), "");
   static_assert(test1(9), "");
   static_assert(!test1(100), "");
   static_assert(!test1(101), ""); // expected-error {{constant expression}} expected-note {{in call to 'test1(101)'}}
 
-  // FIXME: We should be able to reject this before it's called
-  constexpr void f() {
+  constexpr void f() { // expected-error{{constexpr function never produces a constant expression}} expected-note@+2{{assignment to dereferenced one-past-the-end pointer is not allowed in a constant expression}}
     char foo[10] = { "z" }; // expected-note {{here}}
-    foo[10] = 'x'; // expected-warning {{past the end}} expected-note {{assignment to dereferenced one-past-the-end pointer}}
+    foo[10] = 'x'; // expected-warning {{past the end}}
   }
-  constexpr int k = (f(), 0); // expected-error {{constant expression}} expected-note {{in call}}
 }
 
 namespace array_resize {
   constexpr int do_stuff(int k1, int k2) {
     int arr[1234] = { 1, 2, 3, 4 };
     arr[k1] = 5; // expected-note {{past-the-end}} expected-note {{cannot refer to element 1235}} expected-note {{cannot refer to element -1}}
     return arr[k2];
   }
namespace EmptyClass {
   constexpr int test6 = f(e3, 1);
   constexpr int test7 = f(e1, 2);
   constexpr int test8 = f(e2, 2); // expected-error {{constant expression}} expected-note {{in call}}
   constexpr int test9 = f(e3, 2);
   constexpr int testa = f(e1, 3);
   constexpr int testb = f(e2, 3); // expected-error {{constant expression}} expected-note {{in call}}
   constexpr int testc = f(e3, 3);
 }
+
+namespace SpeculativeEvalWrites {
+  // Ensure that we don't try to speculatively evaluate writes.
+  constexpr int add(int a, int b) { return a + b; }
+
+  constexpr int f() {
+    int i = 0;
+    int a = 0;
+    // __builtin_object_size speculatively evaluates its first argument.
+    __builtin_object_size((i = 1, &a), 0);
+    return i;
+  }
+
+  static_assert(!f(), "");
+}