Thanks, fixed and re-committed as 00d3e6c1b4d0b7879afc6002b721111b49ecf755.
On Tue, 6 Oct 2020 at 06:50, Dmitri Gribenko via cfe-commits < cfe-commits@lists.llvm.org> wrote: > > Author: Dmitri Gribenko > Date: 2020-10-06T15:49:44+02:00 > New Revision: 37c74dfe72ecf4e7def22702c5a944682a7865df > > URL: > https://github.com/llvm/llvm-project/commit/37c74dfe72ecf4e7def22702c5a944682a7865df > DIFF: > https://github.com/llvm/llvm-project/commit/37c74dfe72ecf4e7def22702c5a944682a7865df.diff > > LOG: 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). > > Added: > > > Modified: > clang/lib/AST/ExprConstant.cpp > clang/test/SemaCXX/constant-expression-cxx1z.cpp > clang/www/cxx_status.html > > Removed: > > > > > ################################################################################ > diff --git a/clang/lib/AST/ExprConstant.cpp > b/clang/lib/AST/ExprConstant.cpp > index 49ad01f27545..4460e3a17e6d 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<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 > diff erent 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,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<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); > > diff --git a/clang/test/SemaCXX/constant-expression-cxx1z.cpp > b/clang/test/SemaCXX/constant-expression-cxx1z.cpp > index 7770e92c6331..2b366adf2e91 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<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 > -} > > diff --git a/clang/www/cxx_status.html b/clang/www/cxx_status.html > index 9c39e396edd4..3c546eb409de 100755 > --- a/clang/www/cxx_status.html > +++ b/clang/www/cxx_status.html > @@ -807,7 +807,6 @@ <h2 id="cxx17">C++17 implementation status</h2> > <tt>operator&&</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 > > > > _______________________________________________ > cfe-commits mailing list > cfe-commits@lists.llvm.org > https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits >
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
