Re: [PATCH v2] c++: Catch indirect change of active union member in constexpr [PR101631]
On 9/19/23 20:55, Nathaniel Shead wrote:
On Tue, Sep 19, 2023 at 05:25:20PM -0400, Jason Merrill wrote:
On 9/1/23 08:22, Nathaniel Shead wrote:
On Wed, Aug 30, 2023 at 04:28:18PM -0400, Jason Merrill wrote:
On 8/29/23 09:35, Nathaniel Shead wrote:
This is an attempt to improve the constexpr machinery's handling of
union lifetime by catching more cases that cause UB. Is this approach
OK?
I'd also like some feedback on a couple of pain points with this
implementation; in particular, is there a good way to detect if a type
has a non-deleted trivial constructor? I've used 'is_trivially_xible' in
this patch, but that also checks for a trivial destructor which by my
reading of [class.union.general]p5 is possibly incorrect. Checking for a
trivial default constructor doesn't seem too hard but I couldn't find a
good way of checking if that constructor is deleted.
I guess the simplest would be
(TYPE_HAS_TRIVIAL_DFLT (t) && locate_ctor (t))
because locate_ctor returns null for a deleted default ctor. It would be
good to make this a separate predicate.
I'm also generally unsatisfied with the additional complexity with the
third 'refs' argument in 'cxx_eval_store_expression' being pushed and
popped; would it be better to replace this with a vector of some
specific structure type for the data that needs to be passed on?
Perhaps, but what you have here is fine. Another possibility would be to
just have a vec of the refs and extract the index from the ref later as
needed.
Jason
Thanks for the feedback. I've kept the refs as-is for now. I've also
cleaned up a couple of other typos I'd had with comments and diagnostics.
Bootstrapped and regtested on x86_64-pc-linux-gnu.
@@ -6192,10 +6197,16 @@ 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)
+ if (code == UNION_TYPE
+ && TREE_CODE (t) == MODIFY_EXPR
+ && (CONSTRUCTOR_NELTS (*valp) == 0
+ || CONSTRUCTOR_ELT (*valp, 0)->index != index))
{
- if (cxx_dialect < cxx20)
+ /* We changed the active member of a union. Ensure that this is
+valid. */
+ bool has_active_member = CONSTRUCTOR_NELTS (*valp) != 0;
+ tree inner = strip_array_types (reftype);
+ if (has_active_member && cxx_dialect < cxx20)
{
if (!ctx->quiet)
error_at (cp_expr_loc_or_input_loc (t),
While we're looking at this area, this error message should really mention
that it's allowed in C++20.
@@ -6205,8 +6216,36 @@ cxx_eval_store_expression (const constexpr_ctx *ctx,
tree t,
index);
*non_constant_p = true;
}
- else if (TREE_CODE (t) == MODIFY_EXPR
- && CONSTRUCTOR_NO_CLEARING (*valp))
+ else if (!is_access_expr
+ || (CLASS_TYPE_P (inner)
+ && !type_has_non_deleted_trivial_default_ctor (inner)))
+ {
+ /* Diagnose changing active union member after initialisation
+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 (*valp, 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",
+ index, inner);
+ }
The !is_access_expr case could also use an explanatory message.
Thanks for the review, I've updated these messages and will send through
an updated patch once bootstrap/regtest is complete.
Also, I notice that this testcase crashes with the patch:
union U { int i; float f; };
constexpr auto g (U u) { return (u.i = 42); }
static_assert (g({.f = 3.14}) == 42);
This appears to segfault even without the patch since GCC 13.1.
https://godbolt.org/z/45sPh8WaK
I haven't done a bisect yet to work out what commit exactly caused this.
Should I aim to fix this first before coming back with this patch?
Ah, I was just assuming it was related, never mind. I'll fix it.
Jason
Re: [PATCH v2] c++: Catch indirect change of active union member in constexpr [PR101631]
On Tue, Sep 19, 2023 at 05:25:20PM -0400, Jason Merrill wrote:
> On 9/1/23 08:22, Nathaniel Shead wrote:
> > On Wed, Aug 30, 2023 at 04:28:18PM -0400, Jason Merrill wrote:
> > > On 8/29/23 09:35, Nathaniel Shead wrote:
> > > > This is an attempt to improve the constexpr machinery's handling of
> > > > union lifetime by catching more cases that cause UB. Is this approach
> > > > OK?
> > > >
> > > > I'd also like some feedback on a couple of pain points with this
> > > > implementation; in particular, is there a good way to detect if a type
> > > > has a non-deleted trivial constructor? I've used 'is_trivially_xible' in
> > > > this patch, but that also checks for a trivial destructor which by my
> > > > reading of [class.union.general]p5 is possibly incorrect. Checking for a
> > > > trivial default constructor doesn't seem too hard but I couldn't find a
> > > > good way of checking if that constructor is deleted.
> > >
> > > I guess the simplest would be
> > >
> > > (TYPE_HAS_TRIVIAL_DFLT (t) && locate_ctor (t))
> > >
> > > because locate_ctor returns null for a deleted default ctor. It would be
> > > good to make this a separate predicate.
> > >
> > > > I'm also generally unsatisfied with the additional complexity with the
> > > > third 'refs' argument in 'cxx_eval_store_expression' being pushed and
> > > > popped; would it be better to replace this with a vector of some
> > > > specific structure type for the data that needs to be passed on?
> > >
> > > Perhaps, but what you have here is fine. Another possibility would be to
> > > just have a vec of the refs and extract the index from the ref later as
> > > needed.
> > >
> > > Jason
> > >
> >
> > Thanks for the feedback. I've kept the refs as-is for now. I've also
> > cleaned up a couple of other typos I'd had with comments and diagnostics.
> >
> > Bootstrapped and regtested on x86_64-pc-linux-gnu.
> >
> > @@ -6192,10 +6197,16 @@ 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)
> > + if (code == UNION_TYPE
> > + && TREE_CODE (t) == MODIFY_EXPR
> > + && (CONSTRUCTOR_NELTS (*valp) == 0
> > + || CONSTRUCTOR_ELT (*valp, 0)->index != index))
> > {
> > - if (cxx_dialect < cxx20)
> > + /* We changed the active member of a union. Ensure that this is
> > +valid. */
> > + bool has_active_member = CONSTRUCTOR_NELTS (*valp) != 0;
> > + tree inner = strip_array_types (reftype);
> > + if (has_active_member && cxx_dialect < cxx20)
> > {
> > if (!ctx->quiet)
> > error_at (cp_expr_loc_or_input_loc (t),
>
> While we're looking at this area, this error message should really mention
> that it's allowed in C++20.
>
> > @@ -6205,8 +6216,36 @@ cxx_eval_store_expression (const constexpr_ctx *ctx,
> > tree t,
> > index);
> > *non_constant_p = true;
> > }
> > - else if (TREE_CODE (t) == MODIFY_EXPR
> > - && CONSTRUCTOR_NO_CLEARING (*valp))
> > + else if (!is_access_expr
> > + || (CLASS_TYPE_P (inner)
> > + && !type_has_non_deleted_trivial_default_ctor (inner)))
> > + {
> > + /* Diagnose changing active union member after initialisation
> > +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 (*valp, 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",
> > + index, inner);
> > + }
>
> The !is_access_expr case could also use an explanatory message.
Thanks for the review, I've updated these messages and will send through
an updated patch once bootstrap/regtest is complete.
> Also, I notice that this testcase crashes with the patch:
>
> union U { int i; float f; };
> constexpr auto g (U u) { return (u.i = 42); }
> static_assert (g({.f = 3.14}) == 42);
This appears to segfault even without the patch since GCC 13.1.
https://godbolt.org/z/45sPh8WaK
I haven't done a bisect yet to work out what commit exactly
Re: [PATCH v2] c++: Catch indirect change of active union member in constexpr [PR101631]
On 9/1/23 08:22, Nathaniel Shead wrote:
On Wed, Aug 30, 2023 at 04:28:18PM -0400, Jason Merrill wrote:
On 8/29/23 09:35, Nathaniel Shead wrote:
This is an attempt to improve the constexpr machinery's handling of
union lifetime by catching more cases that cause UB. Is this approach
OK?
I'd also like some feedback on a couple of pain points with this
implementation; in particular, is there a good way to detect if a type
has a non-deleted trivial constructor? I've used 'is_trivially_xible' in
this patch, but that also checks for a trivial destructor which by my
reading of [class.union.general]p5 is possibly incorrect. Checking for a
trivial default constructor doesn't seem too hard but I couldn't find a
good way of checking if that constructor is deleted.
I guess the simplest would be
(TYPE_HAS_TRIVIAL_DFLT (t) && locate_ctor (t))
because locate_ctor returns null for a deleted default ctor. It would be
good to make this a separate predicate.
I'm also generally unsatisfied with the additional complexity with the
third 'refs' argument in 'cxx_eval_store_expression' being pushed and
popped; would it be better to replace this with a vector of some
specific structure type for the data that needs to be passed on?
Perhaps, but what you have here is fine. Another possibility would be to
just have a vec of the refs and extract the index from the ref later as
needed.
Jason
Thanks for the feedback. I've kept the refs as-is for now. I've also
cleaned up a couple of other typos I'd had with comments and diagnostics.
Bootstrapped and regtested on x86_64-pc-linux-gnu.
@@ -6192,10 +6197,16 @@ 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)
+ if (code == UNION_TYPE
+ && TREE_CODE (t) == MODIFY_EXPR
+ && (CONSTRUCTOR_NELTS (*valp) == 0
+ || CONSTRUCTOR_ELT (*valp, 0)->index != index))
{
- if (cxx_dialect < cxx20)
+ /* We changed the active member of a union. Ensure that this is
+valid. */
+ bool has_active_member = CONSTRUCTOR_NELTS (*valp) != 0;
+ tree inner = strip_array_types (reftype);
+ if (has_active_member && cxx_dialect < cxx20)
{
if (!ctx->quiet)
error_at (cp_expr_loc_or_input_loc (t),
While we're looking at this area, this error message should really
mention that it's allowed in C++20.
@@ -6205,8 +6216,36 @@ cxx_eval_store_expression (const constexpr_ctx *ctx,
tree t,
index);
*non_constant_p = true;
}
- else if (TREE_CODE (t) == MODIFY_EXPR
- && CONSTRUCTOR_NO_CLEARING (*valp))
+ else if (!is_access_expr
+ || (CLASS_TYPE_P (inner)
+ && !type_has_non_deleted_trivial_default_ctor (inner)))
+ {
+ /* Diagnose changing active union member after initialisation
+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 (*valp, 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",
+ index, inner);
+ }
The !is_access_expr case could also use an explanatory message.
Also, I notice that this testcase crashes with the patch:
union U { int i; float f; };
constexpr auto g (U u) { return (u.i = 42); }
static_assert (g({.f = 3.14}) == 42);
Jason
[PATCH v2] c++: Catch indirect change of active union member in constexpr [PR101631]
Ping for https://gcc.gnu.org/pipermail/gcc-patches/2023-September/629084.html
-- >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 `*() = ` 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.
Additionally, this patch 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.
PR c++/101631
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): Check for accessing inactive union
member indirectly.
* cp-tree.h (type_has_non_deleted_trivial_default_ctor):
Forward declare.
gcc/testsuite/ChangeLog:
* 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.
Signed-off-by: Nathaniel Shead
---
gcc/cp/call.cc| 11 +-
gcc/cp/class.cc | 8 ++
gcc/cp/constexpr.cc | 105 --
gcc/cp/cp-tree.h | 1 +
gcc/testsuite/g++.dg/cpp2a/constexpr-union2.C | 30 +
gcc/testsuite/g++.dg/cpp2a/constexpr-union3.C | 45
gcc/testsuite/g++.dg/cpp2a/constexpr-union4.C | 29 +
gcc/testsuite/g++.dg/cpp2a/constexpr-union5.C | 55 +
8 files changed, 246 insertions(+), 38 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/cpp2a/constexpr-union2.C
create mode 100644 gcc/testsuite/g++.dg/cpp2a/constexpr-union3.C
create mode 100644 gcc/testsuite/g++.dg/cpp2a/constexpr-union4.C
create mode 100644 gcc/testsuite/g++.dg/cpp2a/constexpr-union5.C
diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc
index 23e458d3252..3372c88f182 100644
--- a/gcc/cp/call.cc
+++ b/gcc/cp/call.cc
@@ -10358,10 +10358,7 @@ 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];
@@ -10369,7 +10366,11 @@ build_over_call (struct z_candidate *cand, int flags,
tsubst_flags_t complain)
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);
}
diff --git a/gcc/cp/class.cc b/gcc/cp/class.cc
index 778759237dc..43898dabbe7 100644
--- a/gcc/cp/class.cc
+++ b/gcc/cp/class.cc
@@ -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
diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc
index 8bd5c4a47f8..b82e87be974 100644
--- a/gcc/cp/constexpr.cc
+++ b/gcc/cp/constexpr.cc
@@ -3141,40 +3141,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
[PATCH v2] c++: Catch indirect change of active union member in constexpr [PR101631]
On Wed, Aug 30, 2023 at 04:28:18PM -0400, Jason Merrill wrote:
> On 8/29/23 09:35, Nathaniel Shead wrote:
> > This is an attempt to improve the constexpr machinery's handling of
> > union lifetime by catching more cases that cause UB. Is this approach
> > OK?
> >
> > I'd also like some feedback on a couple of pain points with this
> > implementation; in particular, is there a good way to detect if a type
> > has a non-deleted trivial constructor? I've used 'is_trivially_xible' in
> > this patch, but that also checks for a trivial destructor which by my
> > reading of [class.union.general]p5 is possibly incorrect. Checking for a
> > trivial default constructor doesn't seem too hard but I couldn't find a
> > good way of checking if that constructor is deleted.
>
> I guess the simplest would be
>
> (TYPE_HAS_TRIVIAL_DFLT (t) && locate_ctor (t))
>
> because locate_ctor returns null for a deleted default ctor. It would be
> good to make this a separate predicate.
>
> > I'm also generally unsatisfied with the additional complexity with the
> > third 'refs' argument in 'cxx_eval_store_expression' being pushed and
> > popped; would it be better to replace this with a vector of some
> > specific structure type for the data that needs to be passed on?
>
> Perhaps, but what you have here is fine. Another possibility would be to
> just have a vec of the refs and extract the index from the ref later as
> needed.
>
> Jason
>
Thanks for the feedback. I've kept the refs as-is for now. I've also
cleaned up a couple of other typos I'd had with comments and diagnostics.
Bootstrapped and regtested on x86_64-pc-linux-gnu.
-- 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 `*() = ` 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.
Additionally, this patch 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.
PR c++/101631
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): Check for accessing inactive union
member indirectly.
* cp-tree.h (type_has_non_deleted_trivial_default_ctor):
Forward declare.
gcc/testsuite/ChangeLog:
* 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.
Signed-off-by: Nathaniel Shead
---
gcc/cp/call.cc| 11 +-
gcc/cp/class.cc | 8 ++
gcc/cp/constexpr.cc | 105 --
gcc/cp/cp-tree.h | 1 +
gcc/testsuite/g++.dg/cpp2a/constexpr-union2.C | 30 +
gcc/testsuite/g++.dg/cpp2a/constexpr-union3.C | 45
gcc/testsuite/g++.dg/cpp2a/constexpr-union4.C | 29 +
gcc/testsuite/g++.dg/cpp2a/constexpr-union5.C | 55 +
8 files changed, 246 insertions(+), 38 deletions(-)
create mode 100644 gcc/testsuite/g++.dg/cpp2a/constexpr-union2.C
create mode 100644 gcc/testsuite/g++.dg/cpp2a/constexpr-union3.C
create mode 100644 gcc/testsuite/g++.dg/cpp2a/constexpr-union4.C
create mode 100644 gcc/testsuite/g++.dg/cpp2a/constexpr-union5.C
diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc
index 23e458d3252..3372c88f182 100644
--- a/gcc/cp/call.cc
+++ b/gcc/cp/call.cc
@@ -10358,10 +10358,7 @@ 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];
@@ -10369,7 +10366,11 @@ build_over_call (struct z_candidate *cand, int flags,
tsubst_flags_t complain)
if (is_really_empty_class (type, /*ignore_vptr*/true))
{
- /* Avoid copying empty classes. */
+
