Skip to content

Commit

Permalink
c++: Check for indirect change of active union member in constexpr [P…
Browse files Browse the repository at this point in the history
…R101631,PR102286]

Ping for https://gcc.gnu.org/pipermail/gcc-patches/2023-September/631203.html

Rebased on top of current trunk and bootstrap + regtest on
x86_64-pc-linux-gnu now completed without errors.

-- >8 --

This patch adds checks for attempting to change the active member of a
union by methods other than a member access expression.

To be able to properly distinguish `*(&u.a) = ` from `u.a = `, this
patch redoes the solution for c++/59950 to avoid extranneous *&; it
seems that the only case that needed the workaround was when copying
empty classes.

This patch also ensures that constructors for a union field mark that
field as the active member before entering the call itself; this ensures
that modifications of the field within the constructor's body don't
cause false positives (as these will not appear to be member access
expressions). This means that we no longer need to start the lifetime of
empty union members after the constructor body completes.

As a drive-by fix, this patch also ensures that value-initialised unions
are considered to have activated their initial member for the purpose of
checking stores, which catches some additional mistakes pre-C++20.

	PR c++/101631
	PR c++/102286

gcc/cp/ChangeLog:

	* call.cc (build_over_call): Fold more indirect refs for trivial
	assignment op.
	* class.cc (type_has_non_deleted_trivial_default_ctor): Create.
	* constexpr.cc (cxx_eval_call_expression): Start lifetime of
	union member before entering constructor.
	(cxx_eval_store_expression): Activate member for
	value-initialised union. Check for accessing inactive union
	member indirectly.
	* cp-tree.h (type_has_non_deleted_trivial_default_ctor):
	Forward declare.

gcc/testsuite/ChangeLog:

	* g++.dg/cpp1y/constexpr-89336-3.C: Fix union initialisation.
	* g++.dg/cpp1y/constexpr-union6.C: New test.
	* g++.dg/cpp2a/constexpr-union2.C: New test.
	* g++.dg/cpp2a/constexpr-union3.C: New test.
	* g++.dg/cpp2a/constexpr-union4.C: New test.
	* g++.dg/cpp2a/constexpr-union5.C: New test.
	* g++.dg/cpp2a/constexpr-union6.C: New test.

Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
  • Loading branch information
wreien authored and ouuleilei-bot committed Oct 9, 2023
1 parent c2d62cd commit 965a5ae
Show file tree
Hide file tree
Showing 11 changed files with 345 additions and 43 deletions.
11 changes: 6 additions & 5 deletions gcc/cp/call.cc
Original file line number Diff line number Diff line change
Expand Up @@ -10322,18 +10322,19 @@ build_over_call (struct z_candidate *cand, int flags, tsubst_flags_t complain)
&& DECL_OVERLOADED_OPERATOR_IS (fn, NOP_EXPR)
&& trivial_fn_p (fn))
{
/* Don't use cp_build_fold_indirect_ref, op= returns an lvalue even if
the object argument isn't one. */
tree to = cp_build_indirect_ref (input_location, argarray[0],
RO_ARROW, complain);
tree to = cp_build_fold_indirect_ref (argarray[0]);
tree type = TREE_TYPE (to);
tree as_base = CLASSTYPE_AS_BASE (type);
tree arg = argarray[1];
location_t loc = cp_expr_loc_or_input_loc (arg);

if (is_really_empty_class (type, /*ignore_vptr*/true))
{
/* Avoid copying empty classes. */
/* Avoid copying empty classes, but ensure op= returns an lvalue even
if the object argument isn't one. This isn't needed in other cases
since MODIFY_EXPR is always considered an lvalue. */
to = cp_build_addr_expr (to, tf_none);
to = cp_build_indirect_ref (input_location, to, RO_ARROW, complain);
val = build2 (COMPOUND_EXPR, type, arg, to);
suppress_warning (val, OPT_Wunused);
}
Expand Down
8 changes: 8 additions & 0 deletions gcc/cp/class.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5651,6 +5651,14 @@ type_has_virtual_destructor (tree type)
return (dtor && DECL_VIRTUAL_P (dtor));
}

/* True iff class TYPE has a non-deleted trivial default
constructor. */

bool type_has_non_deleted_trivial_default_ctor (tree type)
{
return TYPE_HAS_TRIVIAL_DFLT (type) && locate_ctor (type);
}

/* Returns true iff T, a class, has a move-assignment or
move-constructor. Does not lazily declare either.
If USER_P is false, any move function will do. If it is true, the
Expand Down
135 changes: 98 additions & 37 deletions gcc/cp/constexpr.cc
Original file line number Diff line number Diff line change
Expand Up @@ -3095,40 +3095,34 @@ cxx_eval_call_expression (const constexpr_ctx *ctx, tree t,
cxx_set_object_constness (ctx, new_obj, /*readonly_p=*/false,
non_constant_p, overflow_p);

/* If this is a constructor, we are beginning the lifetime of the
object we are initializing. */
if (new_obj
&& DECL_CONSTRUCTOR_P (fun)
&& TREE_CODE (new_obj) == COMPONENT_REF
&& TREE_CODE (TREE_TYPE (TREE_OPERAND (new_obj, 0))) == UNION_TYPE)
{
tree activate = build2 (INIT_EXPR, TREE_TYPE (new_obj),
new_obj,
build_constructor (TREE_TYPE (new_obj),
NULL));
cxx_eval_constant_expression (ctx, activate,
lval, non_constant_p, overflow_p);
ggc_free (activate);
}

tree jump_target = NULL_TREE;
cxx_eval_constant_expression (&ctx_with_save_exprs, body,
vc_discard, non_constant_p, overflow_p,
&jump_target);

if (DECL_CONSTRUCTOR_P (fun))
{
/* This can be null for a subobject constructor call, in
which case what we care about is the initialization
side-effects rather than the value. We could get at the
value by evaluating *this, but we don't bother; there's
no need to put such a call in the hash table. */
result = lval ? ctx->object : ctx->ctor;

/* If we've just evaluated a subobject constructor call for an
empty union member, it might not have produced a side effect
that actually activated the union member. So produce such a
side effect now to ensure the union appears initialized. */
if (!result && new_obj
&& TREE_CODE (new_obj) == COMPONENT_REF
&& TREE_CODE (TREE_TYPE
(TREE_OPERAND (new_obj, 0))) == UNION_TYPE
&& is_really_empty_class (TREE_TYPE (new_obj),
/*ignore_vptr*/false))
{
tree activate = build2 (MODIFY_EXPR, TREE_TYPE (new_obj),
new_obj,
build_constructor (TREE_TYPE (new_obj),
NULL));
cxx_eval_constant_expression (ctx, activate, lval,
non_constant_p, overflow_p);
ggc_free (activate);
}
}
/* This can be null for a subobject constructor call, in
which case what we care about is the initialization
side-effects rather than the value. We could get at the
value by evaluating *this, but we don't bother; there's
no need to put such a call in the hash table. */
result = lval ? ctx->object : ctx->ctor;
else if (VOID_TYPE_P (TREE_TYPE (res)))
result = void_node;
else
Expand Down Expand Up @@ -5966,6 +5960,14 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t,
mutable_p)
&& const_object_being_modified == NULL_TREE)
const_object_being_modified = probe;

/* Track named member accesses for unions to validate modifications
that change active member. */
if (!evaluated && TREE_CODE (probe) == COMPONENT_REF)
vec_safe_push (refs, probe);
else
vec_safe_push (refs, NULL_TREE);

vec_safe_push (refs, elt);
vec_safe_push (refs, TREE_TYPE (probe));
probe = ob;
Expand All @@ -5974,13 +5976,15 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t,

case REALPART_EXPR:
gcc_assert (probe == target);
vec_safe_push (refs, NULL_TREE);
vec_safe_push (refs, probe);
vec_safe_push (refs, TREE_TYPE (probe));
probe = TREE_OPERAND (probe, 0);
break;

case IMAGPART_EXPR:
gcc_assert (probe == target);
vec_safe_push (refs, NULL_TREE);
vec_safe_push (refs, probe);
vec_safe_push (refs, TREE_TYPE (probe));
probe = TREE_OPERAND (probe, 0);
Expand Down Expand Up @@ -6069,6 +6073,7 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t,
enum tree_code code = TREE_CODE (type);
tree reftype = refs->pop();
tree index = refs->pop();
bool is_access_expr = refs->pop() != NULL_TREE;

if (code == COMPLEX_TYPE)
{
Expand Down Expand Up @@ -6109,33 +6114,89 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t,

type = reftype;

if (code == UNION_TYPE && CONSTRUCTOR_NELTS (*valp)
&& CONSTRUCTOR_ELT (*valp, 0)->index != index)
/* Check for implicit change of active member for a union. */
if (code == UNION_TYPE
&& (TREE_CODE (t) == MODIFY_EXPR
/* Also check if initializations have implicit change of active
member earlier up the access chain. */
|| !refs->is_empty())
&& (CONSTRUCTOR_NELTS (*valp) == 0
|| CONSTRUCTOR_ELT (*valp, 0)->index != index))
{
if (cxx_dialect < cxx20)
no_zero_init = true;

tree init = *valp;
if (TREE_CODE (t) == MODIFY_EXPR
&& !CONSTRUCTOR_NO_CLEARING (*valp)
&& CONSTRUCTOR_NELTS (*valp) == 0)
/* Modifying a value-initialized union, process the initialization
to ensure that the active member is properly set. */
init = build_value_init (TREE_TYPE (*valp), tf_warning_or_error);

bool has_active_member = CONSTRUCTOR_NELTS (init) != 0;
tree inner = strip_array_types (reftype);

if (has_active_member && CONSTRUCTOR_ELT (init, 0)->index == index)
/* After handling value-initialization, this wasn't actually a
change of active member like we initially thought. */
no_zero_init = false;
else if (has_active_member && cxx_dialect < cxx20)
{
if (!ctx->quiet)
error_at (cp_expr_loc_or_input_loc (t),
"change of the active member of a union "
"from %qD to %qD",
CONSTRUCTOR_ELT (*valp, 0)->index,
"from %qD to %qD is not a constant expression "
"before C++20",
CONSTRUCTOR_ELT (init, 0)->index,
index);
*non_constant_p = true;
}
else if (TREE_CODE (t) == MODIFY_EXPR
&& CONSTRUCTOR_NO_CLEARING (*valp))
else if (!is_access_expr
|| (TREE_CODE (t) == MODIFY_EXPR
&& CLASS_TYPE_P (inner)
&& !type_has_non_deleted_trivial_default_ctor (inner)))
{
/* Diagnose changing active union member after initialization
without a valid member access expression, as described in
[class.union.general] p5. */
if (!ctx->quiet)
{
if (has_active_member)
error_at (cp_expr_loc_or_input_loc (t),
"accessing %qD member instead of initialized "
"%qD member in constant expression",
index, CONSTRUCTOR_ELT (init, 0)->index);
else
error_at (cp_expr_loc_or_input_loc (t),
"accessing uninitialized member %qD",
index);
if (is_access_expr)
inform (DECL_SOURCE_LOCATION (index),
"%qD does not implicitly begin its lifetime "
"because %qT does not have a non-deleted "
"trivial default constructor, use "
"%<std::construct_at%> instead",
index, inner);
else
inform (DECL_SOURCE_LOCATION (index),
"initializing %qD requires a member access "
"expression as the left operand of the assignment",
index);
}
*non_constant_p = true;
}
else if (has_active_member && CONSTRUCTOR_NO_CLEARING (init))
{
/* Diagnose changing the active union member while the union
is in the process of being initialized. */
if (!ctx->quiet)
error_at (cp_expr_loc_or_input_loc (t),
"change of the active member of a union "
"from %qD to %qD during initialization",
CONSTRUCTOR_ELT (*valp, 0)->index,
CONSTRUCTOR_ELT (init, 0)->index,
index);
*non_constant_p = true;
}
no_zero_init = true;
}

ctors.safe_push (valp);
Expand Down
1 change: 1 addition & 0 deletions gcc/cp/cp-tree.h
Original file line number Diff line number Diff line change
Expand Up @@ -6759,6 +6759,7 @@ extern bool trivial_default_constructor_is_constexpr (tree);
extern bool type_has_constexpr_default_constructor (tree);
extern bool type_has_constexpr_destructor (tree);
extern bool type_has_virtual_destructor (tree);
extern bool type_has_non_deleted_trivial_default_ctor (tree);
extern bool classtype_has_move_assign_or_move_ctor_p (tree, bool user_declared);
extern bool classtype_has_non_deleted_move_ctor (tree);
extern tree classtype_has_depr_implicit_copy (tree);
Expand Down
2 changes: 1 addition & 1 deletion gcc/testsuite/g++.dg/cpp1y/constexpr-89336-3.C
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ constexpr int
bar ()
{
union U { int a[5]; long b; };
union V { union U u; short v; };
union V { short v; union U u; };
V w {};
w.v = 5;
w.u.a[3] = w.u.a[1] = w.v; // { dg-error "change of the active member of a union from" "" { target c++17_down } }
Expand Down
13 changes: 13 additions & 0 deletions gcc/testsuite/g++.dg/cpp1y/constexpr-union6.C
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// { dg-do compile { target c++14 } }

union U {
int a;
float b;
};

constexpr bool foo() {
U u {};
u.b = 1.0f; // { dg-error "change of the active member" "" { target c++17_down } }
return u.b == 1.0f;
}
constexpr bool x = foo();
30 changes: 30 additions & 0 deletions gcc/testsuite/g++.dg/cpp2a/constexpr-union2.C
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
// PR c++/101631
// { dg-do compile { target c++20 } }

struct sso {
union {
int buf[10];
int* alloc;
};
};

constexpr bool direct() {
sso val;
val.alloc = nullptr;
val.buf[5] = 42;
return true;
}
constexpr bool ok = direct();


constexpr void perform_assignment(int& left, int right) noexcept {
left = right; // { dg-error "accessing .+ member instead of initialized" }
}

constexpr bool indirect() {
sso val;
val.alloc = nullptr;
perform_assignment(val.buf[5], 42); // { dg-message "in .constexpr. expansion" }
return true;
}
constexpr bool err = indirect(); // { dg-message "in .constexpr. expansion" }
45 changes: 45 additions & 0 deletions gcc/testsuite/g++.dg/cpp2a/constexpr-union3.C
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
// { dg-do compile { target c++20 } }

struct S
{
union {
char buf[8];
char* ptr;
};
unsigned len;

constexpr S(const char* s, unsigned n)
{
char* p;
if (n > 7)
p = ptr = new char[n+1];
else
p = buf;
for (len = 0; len < n; ++len)
p[len] = s[len]; // { dg-error "accessing uninitialized member" }
p[len] = '\0';
}

constexpr ~S()
{
if (len > 7)
delete[] ptr;
}
};

constexpr bool test1()
{
S s("test", 4); // { dg-message "in .constexpr. expansion" }
return true;
}

constexpr bool a = test1(); // { dg-message "in .constexpr. expansion" }


constexpr bool test2()
{
S s("hello world", 11);
return true;
}

constexpr bool b = test2();
29 changes: 29 additions & 0 deletions gcc/testsuite/g++.dg/cpp2a/constexpr-union4.C
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
// { dg-do compile { target c++20 } }

// from [class.union.general] p5

union A { int x; int y[4]; };
struct B { A a; };
union C { B b; int k; };
constexpr int f() {
C c; // does not start lifetime of any union member
c.b.a.y[3] = 4; // OK, S(c.b.a.y[3]) contains c.b and c.b.a.y;
// creates objects to hold union members c.b and c.b.a.y
return c.b.a.y[3]; // OK, c.b.a.y refers to newly created object (see [basic.life])
}
constexpr int a = f();

struct X { const int a; int b; };
union Y { X x; int k; };// { dg-message "does not implicitly begin its lifetime" }
constexpr int g() {
Y y = { { 1, 2 } }; // OK, y.x is active union member ([class.mem])
int n = y.x.a;
y.k = 4; // OK, ends lifetime of y.x, y.k is active member of union

y.x.b = n; // { dg-error "accessing .* member instead of initialized .* member" }
// undefined behavior: y.x.b modified outside its lifetime,
// S(y.x.b) is empty because X's default constructor is deleted,
// so union member y.x's lifetime does not implicitly start
return 0;
}
constexpr int b = g(); // { dg-message "in .constexpr. expansion" }
Loading

0 comments on commit 965a5ae

Please sign in to comment.