Skip to content

Commit ded79be

Browse files
zygoloidzygoloid
committed
[c++17] Implement P0145R3 during constant evaluation.
Ensure that we evaluate assignment and compound-assignment right-to-left, and array subscripting left-to-right. Fixes PR47724.
1 parent ebf6fd6 commit ded79be

File tree

3 files changed

+169
-36
lines changed

3 files changed

+169
-36
lines changed

‎clang/lib/AST/ExprConstant.cpp

Lines changed: 59 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -1856,8 +1856,12 @@ void CallStackFrame::describe(raw_ostream &Out) {
18561856
Out << ", ";
18571857

18581858
const ParmVarDecl *Param = *I;
1859-
const APValue &Arg = Arguments[ArgIndex];
1860-
Arg.printPretty(Out, Info.Ctx, Param->getType());
1859+
if (Arguments) {
1860+
const APValue &Arg = Arguments[ArgIndex];
1861+
Arg.printPretty(Out, Info.Ctx, Param->getType());
1862+
} else {
1863+
Out << "<...>";
1864+
}
18611865

18621866
if (ArgIndex == 0 && IsMemberCall)
18631867
Out << "->" << *Callee << '(';
@@ -5792,6 +5796,8 @@ typedef SmallVector<APValue, 8> ArgVector;
57925796
/// EvaluateArgs - Evaluate the arguments to a function call.
57935797
static bool EvaluateArgs(ArrayRef<const Expr *> Args, ArgVector &ArgValues,
57945798
EvalInfo &Info, const FunctionDecl *Callee) {
5799+
ArgValues.resize(Args.size());
5800+
57955801
bool Success = true;
57965802
llvm::SmallBitVector ForbiddenNullArgs;
57975803
if (Callee->hasAttr<NonNullAttr>()) {
@@ -5809,8 +5815,6 @@ static bool EvaluateArgs(ArrayRef<const Expr *> Args, ArgVector &ArgValues,
58095815
}
58105816
}
58115817
}
5812-
// FIXME: This is the wrong evaluation order for an assignment operator
5813-
// called via operator syntax.
58145818
for (unsigned Idx = 0; Idx < Args.size(); Idx++) {
58155819
if (!Evaluate(ArgValues[Idx], Info, Args[Idx])) {
58165820
// If we're checking for a potential constant expression, evaluate all
@@ -5834,17 +5838,13 @@ static bool EvaluateArgs(ArrayRef<const Expr *> Args, ArgVector &ArgValues,
58345838
/// Evaluate a function call.
58355839
static bool HandleFunctionCall(SourceLocation CallLoc,
58365840
const FunctionDecl *Callee, const LValue *This,
5837-
ArrayRef<const Expr*> Args, const Stmt *Body,
5838-
EvalInfo &Info, APValue &Result,
5839-
const LValue *ResultSlot) {
5840-
ArgVector ArgValues(Args.size());
5841-
if (!EvaluateArgs(Args, ArgValues, Info, Callee))
5842-
return false;
5843-
5841+
ArrayRef<const Expr *> Args, APValue *ArgValues,
5842+
const Stmt *Body, EvalInfo &Info,
5843+
APValue &Result, const LValue *ResultSlot) {
58445844
if (!Info.CheckCallLimit(CallLoc))
58455845
return false;
58465846

5847-
CallStackFrame Frame(Info, CallLoc, Callee, This, ArgValues.data());
5847+
CallStackFrame Frame(Info, CallLoc, Callee, This, ArgValues);
58485848

58495849
// For a trivial copy or move assignment, perform an APValue copy. This is
58505850
// essential for unions, where the operations performed by the assignment
@@ -7293,6 +7293,8 @@ class ExprEvaluatorBase
72937293
auto Args = llvm::makeArrayRef(E->getArgs(), E->getNumArgs());
72947294
bool HasQualifier = false;
72957295

7296+
ArgVector ArgValues;
7297+
72967298
// Extract function decl and 'this' pointer from the callee.
72977299
if (CalleeType->isSpecificBuiltinType(BuiltinType::BoundMember)) {
72987300
const CXXMethodDecl *Member = nullptr;
@@ -7341,6 +7343,22 @@ class ExprEvaluatorBase
73417343
return Error(E);
73427344
}
73437345

7346+
// For an (overloaded) assignment expression, evaluate the RHS before the
7347+
// LHS.
7348+
auto *OCE = dyn_cast<CXXOperatorCallExpr>(E);
7349+
if (OCE && OCE->isAssignmentOp()) {
7350+
assert(Args.size() == 2 && "wrong number of arguments in assignment");
7351+
if (isa<CXXMethodDecl>(FD)) {
7352+
// Args[0] is the object argument.
7353+
if (!EvaluateArgs({Args[1]}, ArgValues, Info, FD))
7354+
return false;
7355+
} else {
7356+
if (!EvaluateArgs({Args[1], Args[0]}, ArgValues, Info, FD))
7357+
return false;
7358+
std::swap(ArgValues[0], ArgValues[1]);
7359+
}
7360+
}
7361+
73447362
// Overloaded operator calls to member functions are represented as normal
73457363
// calls with '*this' as the first argument.
73467364
const CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(FD);
@@ -7403,6 +7421,11 @@ class ExprEvaluatorBase
74037421
} else
74047422
return Error(E);
74057423

7424+
// Evaluate the arguments now if we've not already done so.
7425+
if (ArgValues.empty() && !Args.empty() &&
7426+
!EvaluateArgs(Args, ArgValues, Info, FD))
7427+
return false;
7428+
74067429
SmallVector<QualType, 4> CovariantAdjustmentPath;
74077430
if (This) {
74087431
auto *NamedMember = dyn_cast<CXXMethodDecl>(FD);
@@ -7424,6 +7447,7 @@ class ExprEvaluatorBase
74247447
// Destructor calls are different enough that they have their own codepath.
74257448
if (auto *DD = dyn_cast<CXXDestructorDecl>(FD)) {
74267449
assert(This && "no 'this' pointer for destructor call");
7450+
assert(ArgValues.empty() && "unexpected destructor arguments");
74277451
return HandleDestruction(Info, E, *This,
74287452
Info.Ctx.getRecordType(DD->getParent()));
74297453
}
@@ -7432,8 +7456,8 @@ class ExprEvaluatorBase
74327456
Stmt *Body = FD->getBody(Definition);
74337457

74347458
if (!CheckConstexprFunction(Info, E->getExprLoc(), FD, Definition, Body) ||
7435-
!HandleFunctionCall(E->getExprLoc(), Definition, This, Args, Body, Info,
7436-
Result, ResultSlot))
7459+
!HandleFunctionCall(E->getExprLoc(), Definition, This, Args,
7460+
ArgValues.data(), Body, Info, Result, ResultSlot))
74377461
return false;
74387462

74397463
if (!CovariantAdjustmentPath.empty() &&
@@ -8071,16 +8095,19 @@ bool LValueExprEvaluator::VisitArraySubscriptExpr(const ArraySubscriptExpr *E) {
80718095
if (E->getBase()->getType()->isVectorType())
80728096
return Error(E);
80738097

8098+
APSInt Index;
80748099
bool Success = true;
8075-
if (!evaluatePointer(E->getBase(), Result)) {
8076-
if (!Info.noteFailure())
8077-
return false;
8078-
Success = false;
8079-
}
80808100

8081-
APSInt Index;
8082-
if (!EvaluateInteger(E->getIdx(), Index, Info))
8083-
return false;
8101+
// C++17's rules require us to evaluate the LHS first, regardless of which
8102+
// side is the base.
8103+
for (const Expr *SubExpr : {E->getLHS(), E->getRHS()}) {
8104+
if (SubExpr == E->getBase() ? !evaluatePointer(SubExpr, Result)
8105+
: !EvaluateInteger(SubExpr, Index, Info)) {
8106+
if (!Info.noteFailure())
8107+
return false;
8108+
Success = false;
8109+
}
8110+
}
80848111

80858112
return Success &&
80868113
HandleLValueArrayAdjustment(Info, E, Result, E->getType(), Index);
@@ -8125,7 +8152,10 @@ bool LValueExprEvaluator::VisitCompoundAssignOperator(
81258152
if (!Info.getLangOpts().CPlusPlus14 && !Info.keepEvaluatingAfterFailure())
81268153
return Error(CAO);
81278154

8155+
// C++17 onwards require that we evaluate the RHS first.
81288156
APValue RHS;
8157+
if (!Evaluate(RHS, this->Info, CAO->getRHS()))
8158+
return false;
81298159

81308160
// The overall lvalue result is the result of evaluating the LHS.
81318161
if (!this->Visit(CAO->getLHS())) {
@@ -8134,9 +8164,6 @@ bool LValueExprEvaluator::VisitCompoundAssignOperator(
81348164
return false;
81358165
}
81368166

8137-
if (!Evaluate(RHS, this->Info, CAO->getRHS()))
8138-
return false;
8139-
81408167
return handleCompoundAssignment(
81418168
this->Info, CAO,
81428169
Result, CAO->getLHS()->getType(), CAO->getComputationLHSType(),
@@ -8147,17 +8174,17 @@ bool LValueExprEvaluator::VisitBinAssign(const BinaryOperator *E) {
81478174
if (!Info.getLangOpts().CPlusPlus14 && !Info.keepEvaluatingAfterFailure())
81488175
return Error(E);
81498176

8177+
// C++17 onwards require that we evaluate the RHS first.
81508178
APValue NewVal;
8179+
if (!Evaluate(NewVal, this->Info, E->getRHS()))
8180+
return false;
81518181

81528182
if (!this->Visit(E->getLHS())) {
81538183
if (Info.noteFailure())
81548184
Evaluate(NewVal, this->Info, E->getRHS());
81558185
return false;
81568186
}
81578187

8158-
if (!Evaluate(NewVal, this->Info, E->getRHS()))
8159-
return false;
8160-
81618188
if (Info.getLangOpts().CPlusPlus20 &&
81628189
!HandleUnionActiveMemberChange(Info, E->getLHS(), Result))
81638190
return false;
@@ -15270,7 +15297,8 @@ bool Expr::isPotentialConstantExpr(const FunctionDecl *FD,
1527015297
} else {
1527115298
SourceLocation Loc = FD->getLocation();
1527215299
HandleFunctionCall(Loc, FD, (MD && MD->isInstance()) ? &This : nullptr,
15273-
Args, FD->getBody(), Info, Scratch, nullptr);
15300+
Args, /*ArgValues*/ nullptr, FD->getBody(), Info,
15301+
Scratch, nullptr);
1527415302
}
1527515303

1527615304
return Diags.empty();
@@ -15292,13 +15320,8 @@ bool Expr::isPotentialConstantExprUnevaluated(Expr *E,
1529215320
Info.CheckingPotentialConstantExpression = true;
1529315321

1529415322
// Fabricate a call stack frame to give the arguments a plausible cover story.
15295-
ArrayRef<const Expr*> Args;
15296-
ArgVector ArgValues(0);
15297-
bool Success = EvaluateArgs(Args, ArgValues, Info, FD);
15298-
(void)Success;
15299-
assert(Success &&
15300-
"Failed to set up arguments for potential constant evaluation");
15301-
CallStackFrame Frame(Info, SourceLocation(), FD, nullptr, ArgValues.data());
15323+
CallStackFrame Frame(Info, SourceLocation(), FD, /*This*/ nullptr,
15324+
/*ArgValues*/ nullptr);
1530215325

1530315326
APValue ResultScratch;
1530415327
Evaluate(ResultScratch, Info, E);

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

Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,3 +59,112 @@ void test() {
5959
else if constexpr (v<int>) {}
6060
}
6161
}
62+
63+
// Check that assignment operators evaluate their operands right-to-left.
64+
namespace EvalOrder {
65+
template<typename T> struct lvalue {
66+
T t;
67+
constexpr T &get() { return t; }
68+
};
69+
70+
struct UserDefined {
71+
int n = 0;
72+
constexpr UserDefined &operator=(const UserDefined&) { return *this; }
73+
constexpr UserDefined &operator+=(const UserDefined&) { return *this; }
74+
constexpr void operator<<(const UserDefined&) const {}
75+
constexpr void operator>>(const UserDefined&) const {}
76+
constexpr void operator+(const UserDefined&) const {}
77+
constexpr void operator[](int) const {}
78+
};
79+
constexpr UserDefined ud;
80+
81+
struct NonMember {};
82+
constexpr void operator+=(NonMember, NonMember) {}
83+
constexpr void operator<<(NonMember, NonMember) {}
84+
constexpr void operator>>(NonMember, NonMember) {}
85+
constexpr void operator+(NonMember, NonMember) {}
86+
constexpr NonMember nm;
87+
88+
constexpr void f(...) {}
89+
90+
// Helper to ensure that 'a' is evaluated before 'b'.
91+
struct seq_checker {
92+
bool done_a = false;
93+
bool done_b = false;
94+
95+
template <typename T> constexpr T &&a(T &&v) {
96+
done_a = true;
97+
return (T &&)v;
98+
}
99+
template <typename T> constexpr T &&b(T &&v) {
100+
if (!done_a)
101+
throw "wrong";
102+
done_b = true;
103+
return (T &&)v;
104+
}
105+
106+
constexpr bool ok() { return done_a && done_b; }
107+
};
108+
109+
// SEQ(expr), where part of the expression is tagged A(...) and part is
110+
// tagged B(...), checks that A is evaluated before B.
111+
#define A sc.a
112+
#define B sc.b
113+
#define SEQ(...) static_assert([](seq_checker sc) { void(__VA_ARGS__); return sc.ok(); }({}))
114+
115+
// Longstanding sequencing rules.
116+
SEQ((A(1), B(2)));
117+
SEQ((A(true) ? B(2) : throw "huh?"));
118+
SEQ((A(false) ? throw "huh?" : B(2)));
119+
SEQ(A(true) && B(true));
120+
SEQ(A(false) || B(true));
121+
122+
// From P0145R3:
123+
124+
// Rules 1 and 2 have no effect ('b' is not an expression).
125+
126+
// Rule 3: a->*b
127+
SEQ(A(ud).*B(&UserDefined::n));
128+
SEQ(A(&ud)->*B(&UserDefined::n));
129+
130+
// Rule 4: a(b1, b2, b3)
131+
SEQ(A(f)(B(1), B(2), B(3)));
132+
133+
// Rule 5: b = a, b @= a
134+
SEQ(B(lvalue<int>().get()) = A(0));
135+
SEQ(B(lvalue<UserDefined>().get()) = A(ud));
136+
SEQ(B(lvalue<int>().get()) += A(0));
137+
SEQ(B(lvalue<UserDefined>().get()) += A(ud));
138+
SEQ(B(lvalue<NonMember>().get()) += A(nm));
139+
140+
// Rule 6: a[b]
141+
constexpr int arr[3] = {};
142+
SEQ(A(arr)[B(0)]);
143+
SEQ(A(+arr)[B(0)]);
144+
SEQ(A(0)[B(arr)]);
145+
SEQ(A(0)[B(+arr)]);
146+
SEQ(A(ud)[B(0)]);
147+
148+
// Rule 7: a << b
149+
SEQ(A(1) << B(2));
150+
SEQ(A(ud) << B(ud));
151+
SEQ(A(nm) << B(nm));
152+
153+
// Rule 8: a >> b
154+
SEQ(A(1) >> B(2));
155+
SEQ(A(ud) >> B(ud));
156+
SEQ(A(nm) >> B(nm));
157+
158+
// No particular order of evaluation is specified in other cases, but we in
159+
// practice evaluate left-to-right.
160+
// FIXME: Technically we're expected to check for undefined behavior due to
161+
// unsequenced read and modification and treat it as non-constant due to UB.
162+
SEQ(A(1) + B(2));
163+
SEQ(A(ud) + B(ud));
164+
SEQ(A(nm) + B(nm));
165+
SEQ(f(A(1), B(2)));
166+
167+
#undef SEQ
168+
#undef A
169+
#undef B
170+
}

‎clang/www/cxx_status.html

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

0 commit comments

Comments
 (0)