[clang] 37c74df - Revert "[c++17] Implement P0145R3 during constant evaluation."
Richard Smith via cfe-commits
cfe-commits at lists.llvm.org
Tue Oct 6 12:34:39 PDT 2020
Thanks, fixed and re-committed as 00d3e6c1b4d0b7879afc6002b721111b49ecf755.
On Tue, 6 Oct 2020 at 06:50, Dmitri Gribenko via cfe-commits <
cfe-commits at 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 at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20201006/dc19a84b/attachment-0001.html>
More information about the cfe-commits
mailing list