public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Jason Merrill <jason@redhat.com>
To: Nathaniel Shead <nathanieloshead@gmail.com>
Cc: gcc-patches@gcc.gnu.org
Subject: Re: [PATCH v3] c++: Catch indirect change of active union member in constexpr [PR101631]
Date: Fri, 22 Sep 2023 14:21:15 +0100	[thread overview]
Message-ID: <bfe816fe-5ce5-8dc8-3d00-455ef37d67df@redhat.com> (raw)
In-Reply-To: <ZQxIGK4oIxNMun2i@Thaum.localdomain>

On 9/21/23 09:41, Nathaniel Shead wrote:
> I've updated the error messages, and also fixed another bug I found
> while retesting (value-initialised unions weren't considered to have any
> active member yet).
> 
> 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 `*(&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
> 
> 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.
> 
> Signed-off-by: Nathaniel Shead <nathanieloshead@gmail.com>
> ---
>   gcc/cp/call.cc                                |  11 +-
>   gcc/cp/class.cc                               |   8 ++
>   gcc/cp/constexpr.cc                           | 129 +++++++++++++-----
>   gcc/cp/cp-tree.h                              |   1 +
>   .../g++.dg/cpp1y/constexpr-89336-3.C          |   2 +-
>   gcc/testsuite/g++.dg/cpp1y/constexpr-union6.C |  13 ++
>   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 |  71 ++++++++++
>   10 files changed, 296 insertions(+), 43 deletions(-)
>   create mode 100644 gcc/testsuite/g++.dg/cpp1y/constexpr-union6.C
>   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 e8dafbd8ba6..c1fb8807d3f 100644
> --- a/gcc/cp/call.cc
> +++ b/gcc/cp/call.cc
> @@ -10330,10 +10330,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];
> @@ -10341,7 +10338,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 b71333af1f8..e31aeb8e68b 100644
> --- a/gcc/cp/class.cc
> +++ b/gcc/cp/class.cc
> @@ -5688,6 +5688,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 a673a6022f1..5e50fc2c792 100644
> --- a/gcc/cp/constexpr.cc
> +++ b/gcc/cp/constexpr.cc
> @@ -3146,40 +3146,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 (&call_ctx, 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
> @@ -6127,6 +6121,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;
> @@ -6135,6 +6137,7 @@ 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);
> @@ -6142,6 +6145,7 @@ cxx_eval_store_expression (const constexpr_ctx *ctx, tree t,
>   
>   	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);
> @@ -6230,6 +6234,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)
>   	{
> @@ -6270,21 +6275,72 @@ 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)
> +	  /* Probably a change of active member for a union. Ensure that
> +	     this is valid.  */
> +	  no_zero_init = true;
> +
> +	  tree init = *valp;
> +	  if (!CONSTRUCTOR_NO_CLEARING (*valp) && CONSTRUCTOR_NELTS (*valp) == 0)
> +	    /* This is 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
> +		   || (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",
> +			    index, inner);

It now occurs to me that this should suggest using placement new 
instead.  And that we should test using placement new to begin the 
lifetime of such members.

Jason


  reply	other threads:[~2023-09-22 13:21 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-08-29 13:35 [PATCH] c++: Check for " Nathaniel Shead
2023-08-30 20:28 ` Jason Merrill
2023-09-01 12:22   ` [PATCH v2] c++: Catch " Nathaniel Shead
2023-09-17 12:46     ` Nathaniel Shead
2023-09-19 21:25     ` Jason Merrill
2023-09-20  0:55       ` Nathaniel Shead
2023-09-20 19:23         ` Jason Merrill
2023-09-21 13:41           ` [PATCH v3] " Nathaniel Shead
2023-09-22 13:21             ` Jason Merrill [this message]
2023-09-22 15:01               ` [PATCH v4] c++: Check for indirect change of active union member in constexpr [PR101631,PR102286] Nathaniel Shead
2023-09-23  0:38                 ` Nathaniel Shead
2023-09-23  6:40                   ` Jonathan Wakely
2023-09-23  7:30                     ` [PATCH] libstdc++: Ensure active union member is correctly set Nathaniel Shead
2023-09-23 10:52                       ` Jonathan Wakely
2023-09-27 14:13                       ` Jonathan Wakely
2023-09-28 23:25                         ` Nathaniel Shead
2023-09-29  9:32                           ` Jonathan Wakely
2023-09-29 15:06                             ` Jonathan Wakely
2023-09-29 16:29                               ` Nathaniel Shead
2023-09-29 16:46                                 ` Jonathan Wakely
2023-10-21 14:45                                   ` Jonathan Wakely
2023-10-09  1:03                 ` [PATCH v4] c++: Check for indirect change of active union member in constexpr [PR101631,PR102286] Nathaniel Shead
2023-10-09 20:46                   ` Jason Merrill
2023-10-10 13:48                     ` [PATCH v5] " Nathaniel Shead
2023-10-12  8:53                       ` [PATCH v6] " Nathaniel Shead
2023-10-12 20:24                         ` Jason Merrill
2023-10-12 22:05                           ` Nathaniel Shead
2023-10-20  3:23                             ` Jason Merrill

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=bfe816fe-5ce5-8dc8-3d00-455ef37d67df@redhat.com \
    --to=jason@redhat.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=nathanieloshead@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).