Revert "[c++17] Implement P0145R3 during constant evaluation."

This reverts commit ded79be. It causes
a crash (I sent the crash reproducer directly to the author).

Author: gribozavr

clang/lib/AST/ExprConstant.cpp

@@ -1856,12 +1856,8 @@ void CallStackFrame::describe(raw_ostream &Out) {
       Out << ", ";
 
     const ParmVarDecl *Param = *I;
-    if (Arguments) {
-      const APValue &Arg = Arguments[ArgIndex];
-      Arg.printPretty(Out, Info.Ctx, Param->getType());
-    } else {
-      Out << "<...>";
-    }
+    const APValue &Arg = Arguments[ArgIndex];
+    Arg.printPretty(Out, Info.Ctx, Param->getType());
 
     if (ArgIndex == 0 && IsMemberCall)
       Out << "->" << *Callee << '(';
@@ -5796,8 +5792,6 @@ typedef SmallVector<APValue, 8> ArgVector;
 /// EvaluateArgs - Evaluate the arguments to a function call.
 static bool EvaluateArgs(ArrayRef<const Expr *> Args, ArgVector &ArgValues,
                          EvalInfo &Info, const FunctionDecl *Callee) {
-  ArgValues.resize(Args.size());
-
   bool Success = true;
   llvm::SmallBitVector ForbiddenNullArgs;
   if (Callee->hasAttr<NonNullAttr>()) {
@@ -5815,6 +5809,8 @@ static bool EvaluateArgs(ArrayRef<const Expr *> Args, ArgVector &ArgValues,
         }
     }
   }
+  // FIXME: This is the wrong evaluation order for an assignment operator
+  // called via operator syntax.
   for (unsigned Idx = 0; Idx < Args.size(); Idx++) {
     if (!Evaluate(ArgValues[Idx], Info, Args[Idx])) {
       // If we're checking for a potential constant expression, evaluate all
@@ -5838,13 +5834,17 @@ static bool EvaluateArgs(ArrayRef<const Expr *> Args, ArgVector &ArgValues,
 /// Evaluate a function call.
 static bool HandleFunctionCall(SourceLocation CallLoc,
                                const FunctionDecl *Callee, const LValue *This,
-                               ArrayRef<const Expr *> Args, APValue *ArgValues,
-                               const Stmt *Body, EvalInfo &Info,
-                               APValue &Result, const LValue *ResultSlot) {
+                               ArrayRef<const Expr*> Args, const Stmt *Body,
+                               EvalInfo &Info, APValue &Result,
+                               const LValue *ResultSlot) {
+  ArgVector ArgValues(Args.size());
+  if (!EvaluateArgs(Args, ArgValues, Info, Callee))
+    return false;
+
   if (!Info.CheckCallLimit(CallLoc))
     return false;
 
-  CallStackFrame Frame(Info, CallLoc, Callee, This, ArgValues);
+  CallStackFrame Frame(Info, CallLoc, Callee, This, ArgValues.data());
 
   // For a trivial copy or move assignment, perform an APValue copy. This is
   // essential for unions, where the operations performed by the assignment
@@ -7293,8 +7293,6 @@ class ExprEvaluatorBase
     auto Args = llvm::makeArrayRef(E->getArgs(), E->getNumArgs());
     bool HasQualifier = false;
 
-    ArgVector ArgValues;
-
     // Extract function decl and 'this' pointer from the callee.
     if (CalleeType->isSpecificBuiltinType(BuiltinType::BoundMember)) {
       const CXXMethodDecl *Member = nullptr;
@@ -7343,22 +7341,6 @@ class ExprEvaluatorBase
         return Error(E);
       }
 
-      // For an (overloaded) assignment expression, evaluate the RHS before the
-      // LHS.
-      auto *OCE = dyn_cast<CXXOperatorCallExpr>(E);
-      if (OCE && OCE->isAssignmentOp()) {
-        assert(Args.size() == 2 && "wrong number of arguments in assignment");
-        if (isa<CXXMethodDecl>(FD)) {
-          // Args[0] is the object argument.
-          if (!EvaluateArgs({Args[1]}, ArgValues, Info, FD))
-            return false;
-        } else {
-          if (!EvaluateArgs({Args[1], Args[0]}, ArgValues, Info, FD))
-            return false;
-          std::swap(ArgValues[0], ArgValues[1]);
-        }
-      }
-
       // Overloaded operator calls to member functions are represented as normal
       // calls with '*this' as the first argument.
       const CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(FD);
@@ -7421,11 +7403,6 @@ class ExprEvaluatorBase
     } else
       return Error(E);
 
-    // Evaluate the arguments now if we've not already done so.
-    if (ArgValues.empty() && !Args.empty() &&
-        !EvaluateArgs(Args, ArgValues, Info, FD))
-      return false;
-
     SmallVector<QualType, 4> CovariantAdjustmentPath;
     if (This) {
       auto *NamedMember = dyn_cast<CXXMethodDecl>(FD);
@@ -7447,7 +7424,6 @@ class ExprEvaluatorBase
     // Destructor calls are different enough that they have their own codepath.
     if (auto *DD = dyn_cast<CXXDestructorDecl>(FD)) {
       assert(This && "no 'this' pointer for destructor call");
-      assert(ArgValues.empty() && "unexpected destructor arguments");
       return HandleDestruction(Info, E, *This,
                                Info.Ctx.getRecordType(DD->getParent()));
     }
@@ -7456,8 +7432,8 @@ class ExprEvaluatorBase
     Stmt *Body = FD->getBody(Definition);
 
     if (!CheckConstexprFunction(Info, E->getExprLoc(), FD, Definition, Body) ||
-        !HandleFunctionCall(E->getExprLoc(), Definition, This, Args,
-                            ArgValues.data(), Body, Info, Result, ResultSlot))
+        !HandleFunctionCall(E->getExprLoc(), Definition, This, Args, Body, Info,
+                            Result, ResultSlot))
       return false;
 
     if (!CovariantAdjustmentPath.empty() &&
@@ -8095,20 +8071,17 @@ bool LValueExprEvaluator::VisitArraySubscriptExpr(const ArraySubscriptExpr *E) {
   if (E->getBase()->getType()->isVectorType())
     return Error(E);
 
-  APSInt Index;
   bool Success = true;
-
-  // C++17's rules require us to evaluate the LHS first, regardless of which
-  // side is the base.
-  for (const Expr *SubExpr : {E->getLHS(), E->getRHS()}) {
-    if (SubExpr == E->getBase() ? !evaluatePointer(SubExpr, Result)
-                                : !EvaluateInteger(SubExpr, Index, Info)) {
-      if (!Info.noteFailure())
-        return false;
-      Success = false;
-    }
+  if (!evaluatePointer(E->getBase(), Result)) {
+    if (!Info.noteFailure())
+      return false;
+    Success = false;
   }
 
+  APSInt Index;
+  if (!EvaluateInteger(E->getIdx(), Index, Info))
+    return false;
+
   return Success &&
          HandleLValueArrayAdjustment(Info, E, Result, E->getType(), Index);
 }
@@ -8152,10 +8125,7 @@ bool LValueExprEvaluator::VisitCompoundAssignOperator(
   if (!Info.getLangOpts().CPlusPlus14 && !Info.keepEvaluatingAfterFailure())
     return Error(CAO);
 
-  // C++17 onwards require that we evaluate the RHS first.
   APValue RHS;
-  if (!Evaluate(RHS, this->Info, CAO->getRHS()))
-    return false;
 
   // The overall lvalue result is the result of evaluating the LHS.
   if (!this->Visit(CAO->getLHS())) {
@@ -8164,6 +8134,9 @@ bool LValueExprEvaluator::VisitCompoundAssignOperator(
     return false;
   }
 
+  if (!Evaluate(RHS, this->Info, CAO->getRHS()))
+    return false;
+
   return handleCompoundAssignment(
       this->Info, CAO,
       Result, CAO->getLHS()->getType(), CAO->getComputationLHSType(),
@@ -8174,17 +8147,17 @@ bool LValueExprEvaluator::VisitBinAssign(const BinaryOperator *E) {
   if (!Info.getLangOpts().CPlusPlus14 && !Info.keepEvaluatingAfterFailure())
     return Error(E);
 
-  // C++17 onwards require that we evaluate the RHS first.
   APValue NewVal;
-  if (!Evaluate(NewVal, this->Info, E->getRHS()))
-    return false;
 
   if (!this->Visit(E->getLHS())) {
     if (Info.noteFailure())
       Evaluate(NewVal, this->Info, E->getRHS());
     return false;
   }
 
+  if (!Evaluate(NewVal, this->Info, E->getRHS()))
+    return false;
+
   if (Info.getLangOpts().CPlusPlus20 &&
       !HandleUnionActiveMemberChange(Info, E->getLHS(), Result))
     return false;
@@ -15297,8 +15270,7 @@ bool Expr::isPotentialConstantExpr(const FunctionDecl *FD,
   } else {
     SourceLocation Loc = FD->getLocation();
     HandleFunctionCall(Loc, FD, (MD && MD->isInstance()) ? &This : nullptr,
-                       Args, /*ArgValues*/ nullptr, FD->getBody(), Info,
-                       Scratch, nullptr);
+                       Args, FD->getBody(), Info, Scratch, nullptr);
   }
 
   return Diags.empty();
@@ -15320,8 +15292,13 @@ bool Expr::isPotentialConstantExprUnevaluated(Expr *E,
   Info.CheckingPotentialConstantExpression = true;
 
   // Fabricate a call stack frame to give the arguments a plausible cover story.
-  CallStackFrame Frame(Info, SourceLocation(), FD, /*This*/ nullptr,
-                       /*ArgValues*/ nullptr);
+  ArrayRef<const Expr*> Args;
+  ArgVector ArgValues(0);
+  bool Success = EvaluateArgs(Args, ArgValues, Info, FD);
+  (void)Success;
+  assert(Success &&
+         "Failed to set up arguments for potential constant evaluation");
+  CallStackFrame Frame(Info, SourceLocation(), FD, nullptr, ArgValues.data());
 
   APValue ResultScratch;
   Evaluate(ResultScratch, Info, E);

clang/test/SemaCXX/constant-expression-cxx1z.cpp

@@ -59,112 +59,3 @@ void test() {
   else if constexpr (v<int>) {}
 }
 }
-
-// Check that assignment operators evaluate their operands right-to-left.
-namespace EvalOrder {
-  template<typename T> struct lvalue {
-    T t;
-    constexpr T &get() { return t; }
-  };
-
-  struct UserDefined {
-    int n = 0;
-    constexpr UserDefined &operator=(const UserDefined&) { return *this; }
-    constexpr UserDefined &operator+=(const UserDefined&) { return *this; }
-    constexpr void operator<<(const UserDefined&) const {}
-    constexpr void operator>>(const UserDefined&) const {}
-    constexpr void operator+(const UserDefined&) const {}
-    constexpr void operator[](int) const {}
-  };
-  constexpr UserDefined ud;
-
-  struct NonMember {};
-  constexpr void operator+=(NonMember, NonMember) {}
-  constexpr void operator<<(NonMember, NonMember) {}
-  constexpr void operator>>(NonMember, NonMember) {}
-  constexpr void operator+(NonMember, NonMember) {}
-  constexpr NonMember nm;
-
-  constexpr void f(...) {}
-
-  // Helper to ensure that 'a' is evaluated before 'b'.
-  struct seq_checker {
-    bool done_a = false;
-    bool done_b = false;
-
-    template <typename T> constexpr T &&a(T &&v) {
-      done_a = true;
-      return (T &&)v;
-    }
-    template <typename T> constexpr T &&b(T &&v) {
-      if (!done_a)
-        throw "wrong";
-      done_b = true;
-      return (T &&)v;
-    }
-
-    constexpr bool ok() { return done_a && done_b; }
-  };
-
-  // SEQ(expr), where part of the expression is tagged A(...) and part is
-  // tagged B(...), checks that A is evaluated before B.
-  #define A sc.a
-  #define B sc.b
-  #define SEQ(...) static_assert([](seq_checker sc) { void(__VA_ARGS__); return sc.ok(); }({}))
-
-  // Longstanding sequencing rules.
-  SEQ((A(1), B(2)));
-  SEQ((A(true) ? B(2) : throw "huh?"));
-  SEQ((A(false) ? throw "huh?" : B(2)));
-  SEQ(A(true) && B(true));
-  SEQ(A(false) || B(true));
-
-  // From P0145R3:
-
-  // Rules 1 and 2 have no effect ('b' is not an expression).
-
-  // Rule 3: a->*b
-  SEQ(A(ud).*B(&UserDefined::n));
-  SEQ(A(&ud)->*B(&UserDefined::n));
-
-  // Rule 4: a(b1, b2, b3)
-  SEQ(A(f)(B(1), B(2), B(3)));
-
-  // Rule 5: b = a, b @= a
-  SEQ(B(lvalue<int>().get()) = A(0));
-  SEQ(B(lvalue<UserDefined>().get()) = A(ud));
-  SEQ(B(lvalue<int>().get()) += A(0));
-  SEQ(B(lvalue<UserDefined>().get()) += A(ud));
-  SEQ(B(lvalue<NonMember>().get()) += A(nm));
-
-  // Rule 6: a[b]
-  constexpr int arr[3] = {};
-  SEQ(A(arr)[B(0)]);
-  SEQ(A(+arr)[B(0)]);
-  SEQ(A(0)[B(arr)]);
-  SEQ(A(0)[B(+arr)]);
-  SEQ(A(ud)[B(0)]);
-
-  // Rule 7: a << b
-  SEQ(A(1) << B(2));
-  SEQ(A(ud) << B(ud));
-  SEQ(A(nm) << B(nm));
-
-  // Rule 8: a >> b
-  SEQ(A(1) >> B(2));
-  SEQ(A(ud) >> B(ud));
-  SEQ(A(nm) >> B(nm));
-
-  // No particular order of evaluation is specified in other cases, but we in
-  // practice evaluate left-to-right.
-  // FIXME: Technically we're expected to check for undefined behavior due to
-  // unsequenced read and modification and treat it as non-constant due to UB.
-  SEQ(A(1) + B(2));
-  SEQ(A(ud) + B(ud));
-  SEQ(A(nm) + B(nm));
-  SEQ(f(A(1), B(2)));
-
-  #undef SEQ
-  #undef A
-  #undef B
-}

clang/www/cxx_status.html

@@ -807,7 +807,6 @@ <h2 id="cxx17">C++17 implementation status</h2>
 <tt>operator&amp;&amp;</tt>, <tt>operator||</tt>, and <tt>operator,</tt>
 functions using expression syntax are no longer guaranteed to be destroyed in
 reverse construction order in that ABI.
-This is not fully supported during constant expression evaluation until Clang 12.
 </span><br>
 <span id="p0522">(10): Despite being the resolution to a Defect Report, this
 feature is disabled by default in all language versions, and can be enabled