Skip to content

Instantly share code, notes, and snippets.

@Matthewacon

Matthewacon/ExprConstant.cpp.patch

Created July 29, 2021 00:15
Show Gist options
  • Save Matthewacon/d2eacd1667d1665293b67b3f61c5f881 to your computer and use it in GitHub Desktop.
Save Matthewacon/d2eacd1667d1665293b67b3f61c5f881 to your computer and use it in GitHub Desktop.
Download ZIP
Raw
constexpr_union_active_member_change.txt
#Component
C++
#Version
Trunk
#Title
Incomplete implementation of the P1330R0 proposal / [expr.const]p5 and [class.union]p6 (N4892)
#Body
All of the references and quotes in this report are concerned with the changes in P1330R0 and the current working standard document, N4892:
- P1330R0: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p1330r0.pdf
- N4892: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2021/n4892.pdf
Affected versions:
- trunk
- 12.0.1
- 12.0.0
- 11.0.1
- 11.0.0
- 10.0.1
- 10.0.0
- 9.0.1
- 9.0.0
Driver cmdline:
Note: For versions not supporting -std=c++20, -std=c++2a was used.
clang++ -std=c++20
Consider the following examples:
[Example A:
struct A {};
constexpr auto test() {
union U {
int i;
A a;
} a{.i = 0};
u.a = {};
return 0;
}
int main() {
constexpr auto r = test();
(void)r;
}
[Example B:
struct A {
int i;
constexpr A() noexcept : i(0) {}
constexpr A(int i) noexcept : i(i) {}
constexpr A& operator=(A const& other) noexcept {
i = other.i;
return *this;
}
};
constexpr auto test() {
union U {
int i;
A a;
} u{.i = 0};
u.a = {123};
return u.a.i;
}
int main() {
constexpr auto r = test();
(void)r;
}
According to [expr.constant]p5
An expression E is a core constant expression unless the evaluation of E, following the rules of the abstract machine (6.9.1), would evaluate one of the following:
... an invocation of an implicitly-defined copy/move constructor or copy/move assignment operator for a union whose active member (if any) is mutable, unless the lifetime of the union object began within the evaluation of E ...
, and [class.union]p6
When the left operand of an assignment operator involves a member access expression (7.6.1.5) that nominates a union member, it may begin the lifetime of that union member ...
, the lifetime of `u.a` ("Example A":8) should start at the invocation of the `A`'s implicitly defined copy-assignment operator, however, under all listed clang versions this is not the case as compilation yields the following error:
"Example A":13:17: error: constexpr variable 'r' must be initialized by a constant expression
constexpr auto r = test();
^ ~~~~~~
"Example A":8:6: note: member call on member 'a' of union with active member 'i' is not allowed in a constant expression
u.a = {};
^
"Example A":13:21: note: in call to 'test()'
constexpr auto r = test();
^
1 error generated.
Likewise, for Example B the lifetime of `u.a` ("Example B":18) should begin at the invocation of `A`'s user-defined constexpr copy-assignment operator, however compilation yields the same error.
Addressing the issue:
The attached patch addresses this issue and is based on the `release/12.x` tag:
commit fed41342a82f5a3a9201819a82bf7a48313e296b (grafted, HEAD -> release/12.x, tag: llvmorg-12.0.1-rc4, tag: llvmorg-12.0.1, origin/release/12.x)
Author: Tom Stellard <tstellar@redhat.com>
Date: Mon Jun 28 09:23:38 2021 -0700
Revert "Revert "[Coverage] Fix branch coverage merging in FunctionCoverageSummary::get() for instantiation""
This reverts commit 33d312b2d731507327252fd597bac1b738870330.
The original patch was correct, so we need to restore it in the
release branch.
Notes:
- In testing both examples against other major compilers, such as MSVC and GCC, it would seem that they compile and behave as expected.
Raw
ExprConstant.cpp.patch
diff --git a/clang/lib/AST/ExprConstant.cpp b/clang/lib/AST/ExprConstant.cpp
index 1bdad771a..c3ad37e12 100644
--- a/clang/lib/AST/ExprConstant.cpp
+++ b/clang/lib/AST/ExprConstant.cpp
@@ -3602,6 +3602,11 @@ findSubobject(EvalInfo &Info, const Expr *E, const CompleteObject &Obj,
return handler.failed();
}
+ bool IsAssignment = false;
+ if (auto *ASE = dyn_cast<CXXOperatorCallExpr>(E)) {
+ IsAssignment = ASE->isAssignmentOp();
+ }
+
APValue *O = Obj.Value;
QualType ObjType = Obj.Type;
const FieldDecl *LastField = nullptr;
@@ -3610,9 +3615,9 @@ findSubobject(EvalInfo &Info, const Expr *E, const CompleteObject &Obj,
// Walk the designator's path to find the subobject.
for (unsigned I = 0, N = Sub.Entries.size(); /**/; ++I) {
// Reading an indeterminate value is undefined, but assigning over one is OK.
- if ((O->isAbsent() && !(handler.AccessKind == AK_Construct && I == N)) ||
- (O->isIndeterminate() &&
- !isValidIndeterminateAccess(handler.AccessKind))) {
+ if ((O->isAbsent() && !((handler.AccessKind == AK_Construct && I == N) ||
+ (handler.AccessKind == AK_MemberCall && I == N && IsAssignment))) ||
+ (O->isIndeterminate() && !isValidIndeterminateAccess(handler.AccessKind))) {
if (!Info.checkingPotentialConstantExpression())
Info.FFDiag(E, diag::note_constexpr_access_uninit)
<< handler.AccessKind << O->isIndeterminate();
@@ -3751,8 +3756,10 @@ findSubobject(EvalInfo &Info, const Expr *E, const CompleteObject &Obj,
const FieldDecl *UnionField = O->getUnionField();
if (!UnionField ||
UnionField->getCanonicalDecl() != Field->getCanonicalDecl()) {
- if (I == N - 1 && handler.AccessKind == AK_Construct) {
- // Placement new onto an inactive union member makes it active.
+ if (I == N - 1 && (handler.AccessKind == AK_Construct ||
+ (handler.AccessKind == AK_MemberCall && IsAssignment))) {
+ // Placement new onto, or assignment to, an inactive union member
+ // makes it active.
O->setUnion(Field, APValue());
} else {
// FIXME: If O->getUnionValue() is absent, report that there's no
@@ -5878,10 +5885,10 @@ struct StartLifetimeOfUnionMemberHandler {
const AccessKinds StartLifetimeOfUnionMemberHandler::AccessKind;
-/// Handle a builtin simple-assignment or a call to a trivial assignment
-/// operator whose left-hand side might involve a union member access. If it
-/// does, implicitly start the lifetime of any accessed union elements per
-/// C++20 [class.union]5.
+/// Handle a builtin simple-assignment or a call to an assignment operator
+/// whose left-hand side might involve a union member access. If it does,
+/// implicitly start the lifetime of any accessed union elements per C++20
+/// [class.union]5.
static bool HandleUnionActiveMemberChange(EvalInfo &Info, const Expr *LHSExpr,
const LValue &LHS) {
if (LHS.InvalidBase || LHS.Designator.Invalid)
@@ -5893,12 +5900,12 @@ static bool HandleUnionActiveMemberChange(EvalInfo &Info, const Expr *LHSExpr,
unsigned PathLength = LHS.Designator.Entries.size();
for (const Expr *E = LHSExpr; E != nullptr;) {
// -- If E is of the form A.B, S(E) contains the elements of S(A)...
- if (auto *ME = dyn_cast<MemberExpr>(E)) {
+ const auto handleMemberExpr = [&](const MemberExpr *ME) -> bool {
auto *FD = dyn_cast<FieldDecl>(ME->getMemberDecl());
// Note that we can't implicitly start the lifetime of a reference,
// so we don't need to proceed any further if we reach one.
if (!FD || FD->getType()->isReferenceType())
- break;
+ return false;
// ... and also contains A.B if B names a union member ...
if (FD->getParent()->isUnion()) {
@@ -5907,7 +5914,12 @@ static bool HandleUnionActiveMemberChange(EvalInfo &Info, const Expr *LHSExpr,
// such types.
auto *RD =
FD->getType()->getBaseElementTypeUnsafe()->getAsCXXRecordDecl();
- if (!RD || RD->hasTrivialDefaultConstructor())
+
+ // Note: Weaken the requirement of a trivial default constructor to a
+ // default constructor in order to allow for changing the active union
+ // member for a non-trivially constructible type, with a user-defined
+ // constexpr default constructor.
+ if (!RD || RD->hasDefaultConstructor())
UnionPathLengths.push_back({PathLength - 1, FD});
}
@@ -5916,6 +5928,12 @@ static bool HandleUnionActiveMemberChange(EvalInfo &Info, const Expr *LHSExpr,
assert(declaresSameEntity(FD,
LHS.Designator.Entries[PathLength]
.getAsBaseOrMember().getPointer()));
+ return true;
+ };
+
+ if (auto *ME = dyn_cast<MemberExpr>(E)) {
+ if (!handleMemberExpr(ME))
+ break;
// -- If E is of the form A[B] and is interpreted as a built-in array
// subscripting operator, S(E) is [S(the array operand, if any)].
@@ -5945,6 +5963,20 @@ static bool HandleUnionActiveMemberChange(EvalInfo &Info, const Expr *LHSExpr,
.getAsBaseOrMember().getPointer()));
}
+ // Handle non-trivial assignment operator invocation.
+ } else if (auto *ASE = dyn_cast<CXXOperatorCallExpr>(E)) {
+ if (ASE->isAssignmentOp()) {
+ assert(ASE->getNumArgs() > 0);
+ // Start a lifetime for the union member being assigned through a
+ // non-trivial assignment oeprator invocation
+ if (auto *ME = dyn_cast<MemberExpr>(ASE->getArg(0))) {
+ if (!handleMemberExpr(ME))
+ break;
+ }
+ } else {
+ break;
+ }
+
// -- Otherwise, S(E) is empty.
} else {
break;
@@ -6089,7 +6121,7 @@ static bool HandleFunctionCall(SourceLocation CallLoc,
if (!handleTrivialCopy(Info, MD->getParamDecl(0), Args[0], RHSValue,
MD->getParent()->isUnion()))
return false;
- if (Info.getLangOpts().CPlusPlus20 && MD->isTrivial() &&
+ if (Info.getLangOpts().CPlusPlus20 && /*MD->isTrivial() &&*/
!HandleUnionActiveMemberChange(Info, Args[0], *This))
return false;
if (!handleAssignment(Info, Args[0], *This, MD->getThisType(),
@@ -7666,11 +7698,14 @@ public:
if (!FD)
return false;
} else {
+ // Handle union active member change before evaluating check
+ if (Info.getLangOpts().CPlusPlus20) {
+ HandleUnionActiveMemberChange(Info, E, *This);
+ }
+
// Check that the 'this' pointer points to an object of the right type.
- // FIXME: If this is an assignment operator call, we may need to change
- // the active union member before we check this.
if (!checkNonVirtualMemberCallThisPointer(Info, E, *This, NamedMember))
- return false;
+ return false;
}
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment