From 37c74dfe72ecf4e7def22702c5a944682a7865df Mon Sep 17 00:00:00 2001 From: Dmitri Gribenko Date: Tue, 6 Oct 2020 15:28:19 +0200 Subject: [PATCH] Revert "[c++17] Implement P0145R3 during constant evaluation." This reverts commit ded79be63555f4e5bfdb0db27ef22b71fe568474. It causes a crash (I sent the crash reproducer directly to the author). --- clang/lib/AST/ExprConstant.cpp | 95 ++++++++------------ clang/test/SemaCXX/constant-expression-cxx1z.cpp | 109 ----------------------- clang/www/cxx_status.html | 1 - 3 files changed, 36 insertions(+), 169 deletions(-) diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp index 49ad01f..4460e3a 100644 --- a/clang/lib/AST/ExprConstant.cpp +++ b/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 ArgVector; /// EvaluateArgs - Evaluate the arguments to a function call. static bool EvaluateArgs(ArrayRef Args, ArgVector &ArgValues, EvalInfo &Info, const FunctionDecl *Callee) { - ArgValues.resize(Args.size()); - bool Success = true; llvm::SmallBitVector ForbiddenNullArgs; if (Callee->hasAttr()) { @@ -5815,6 +5809,8 @@ static bool EvaluateArgs(ArrayRef 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 Args, ArgVector &ArgValues, /// Evaluate a function call. static bool HandleFunctionCall(SourceLocation CallLoc, const FunctionDecl *Callee, const LValue *This, - ArrayRef Args, APValue *ArgValues, - const Stmt *Body, EvalInfo &Info, - APValue &Result, const LValue *ResultSlot) { + ArrayRef 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 @@ public: 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 @@ public: return Error(E); } - // For an (overloaded) assignment expression, evaluate the RHS before the - // LHS. - auto *OCE = dyn_cast(E); - if (OCE && OCE->isAssignmentOp()) { - assert(Args.size() == 2 && "wrong number of arguments in assignment"); - if (isa(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(FD); @@ -7421,11 +7403,6 @@ public: } 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 CovariantAdjustmentPath; if (This) { auto *NamedMember = dyn_cast(FD); @@ -7447,7 +7424,6 @@ public: // Destructor calls are different enough that they have their own codepath. if (auto *DD = dyn_cast(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 @@ public: 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,10 +8147,7 @@ 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()) @@ -8185,6 +8155,9 @@ bool LValueExprEvaluator::VisitBinAssign(const BinaryOperator *E) { 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 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); diff --git a/clang/test/SemaCXX/constant-expression-cxx1z.cpp b/clang/test/SemaCXX/constant-expression-cxx1z.cpp index 7770e92..2b366ad 100644 --- a/clang/test/SemaCXX/constant-expression-cxx1z.cpp +++ b/clang/test/SemaCXX/constant-expression-cxx1z.cpp @@ -59,112 +59,3 @@ void test() { else if constexpr (v) {} } } - -// Check that assignment operators evaluate their operands right-to-left. -namespace EvalOrder { - template 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 constexpr T &&a(T &&v) { - done_a = true; - return (T &&)v; - } - template 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().get()) = A(0)); - SEQ(B(lvalue().get()) = A(ud)); - SEQ(B(lvalue().get()) += A(0)); - SEQ(B(lvalue().get()) += A(ud)); - SEQ(B(lvalue().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 -} diff --git a/clang/www/cxx_status.html b/clang/www/cxx_status.html index 9c39e39..3c546eb 100755 --- a/clang/www/cxx_status.html +++ b/clang/www/cxx_status.html @@ -807,7 +807,6 @@ left to right in the callee. As a result, function parameters in calls to operator&&, operator||, and operator, 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.
(10): Despite being the resolution to a Defect Report, this feature is disabled by default in all language versions, and can be enabled -- 2.7.4