diff mbox series

[RFA,(fold)] c++: constexpr bit_cast with empty field

Message ID 20230717211905.946580-1-jason@redhat.com
State New
Headers show
Series [RFA,(fold)] c++: constexpr bit_cast with empty field | expand

Commit Message

Jason Merrill July 17, 2023, 9:19 p.m. UTC 
Tested x86_64-pc-linux-gnu, OK for trunk?

-- 8< --

The change to only cache constexpr calls that are
reduced_constant_expression_p tripped on bit-cast3.C, which failed that
predicate due to the presence of an empty field in the result of
native_interpret_aggregate, which reduced_constant_expression_p rejects to
avoid confusing output_constructor.

This patch proposes to skip such fields in native_interpret_aggregate, since
they aren't actually involved in the value representation.

gcc/ChangeLog:

	* fold-const.cc (native_interpret_aggregate): Skip empty fields.

gcc/cp/ChangeLog:

	* constexpr.cc (cxx_eval_bit_cast): Check that the result of
	native_interpret_aggregate doesn't need more evaluation.
---
 gcc/cp/constexpr.cc | 9 +++++++++
 gcc/fold-const.cc   | 3 ++-
 2 files changed, 11 insertions(+), 1 deletion(-)


base-commit: caabf0973a4e9a26421c94d540e3e20051e93e77

Comments

Richard Biener July 18, 2023, 11:31 a.m. UTC | #1 
On Mon, Jul 17, 2023 at 11:20 PM Jason Merrill via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> Tested x86_64-pc-linux-gnu, OK for trunk?
>
> -- 8< --
>
> The change to only cache constexpr calls that are
> reduced_constant_expression_p tripped on bit-cast3.C, which failed that
> predicate due to the presence of an empty field in the result of
> native_interpret_aggregate, which reduced_constant_expression_p rejects to
> avoid confusing output_constructor.
>
> This patch proposes to skip such fields in native_interpret_aggregate, since
> they aren't actually involved in the value representation.
>
> gcc/ChangeLog:
>
>         * fold-const.cc (native_interpret_aggregate): Skip empty fields.
>
> gcc/cp/ChangeLog:
>
>         * constexpr.cc (cxx_eval_bit_cast): Check that the result of
>         native_interpret_aggregate doesn't need more evaluation.
> ---
>  gcc/cp/constexpr.cc | 9 +++++++++
>  gcc/fold-const.cc   | 3 ++-
>  2 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc
> index 9d85c3be5cc..6e8f1c2b61e 100644
> --- a/gcc/cp/constexpr.cc
> +++ b/gcc/cp/constexpr.cc
> @@ -1440,6 +1440,8 @@ enum value_cat {
>
>  static tree cxx_eval_constant_expression (const constexpr_ctx *, tree,
>                                           value_cat, bool *, bool *, tree * = NULL);
> +static tree cxx_eval_bare_aggregate (const constexpr_ctx *, tree,
> +                                    value_cat, bool *, bool *);
>  static tree cxx_fold_indirect_ref (const constexpr_ctx *, location_t, tree, tree,
>                                    bool * = NULL);
>  static tree find_heap_var_refs (tree *, int *, void *);
> @@ -4803,6 +4805,13 @@ cxx_eval_bit_cast (const constexpr_ctx *ctx, tree t, bool *non_constant_p,
>         {
>           clear_type_padding_in_mask (TREE_TYPE (t), mask);
>           clear_uchar_or_std_byte_in_mask (loc, r, mask);
> +         if (CHECKING_P)
> +           {
> +             tree e = cxx_eval_bare_aggregate (ctx, r, vc_prvalue,
> +                                               non_constant_p, overflow_p);
> +             gcc_checking_assert (e == r);
> +             r = e;
> +           }
>         }
>      }
>
> diff --git a/gcc/fold-const.cc b/gcc/fold-const.cc
> index a02ede79fed..db8f7de5680 100644
> --- a/gcc/fold-const.cc
> +++ b/gcc/fold-const.cc
> @@ -8935,7 +8935,8 @@ native_interpret_aggregate (tree type, const unsigned char *ptr, int off,
>      return NULL_TREE;
>    for (tree field = TYPE_FIELDS (type); field; field = DECL_CHAIN (field))
>      {
> -      if (TREE_CODE (field) != FIELD_DECL || DECL_PADDING_P (field))
> +      if (TREE_CODE (field) != FIELD_DECL || DECL_PADDING_P (field)
> +         || integer_zerop (DECL_SIZE (field)))

The gimplifier uses is_empty_type (TREE_TYPE  (field)), wouldn't that be better
for consistency reasons at least?

OK with that change or with integer_zerop if there is a reason to not
use is_empty_type
here.

Thanks,
Richard.

>         continue;
>        tree fld = field;
>        HOST_WIDE_INT bitoff = 0, pos = 0, sz = 0;
>
> base-commit: caabf0973a4e9a26421c94d540e3e20051e93e77
> --
> 2.39.3
>
Jason Merrill July 18, 2023, 4:26 p.m. UTC | #2 
On 7/18/23 07:31, Richard Biener wrote:
> On Mon, Jul 17, 2023 at 11:20 PM Jason Merrill via Gcc-patches
> <gcc-patches@gcc.gnu.org> wrote:
>>
>> Tested x86_64-pc-linux-gnu, OK for trunk?
>>
>> -- 8< --
>>
>> The change to only cache constexpr calls that are
>> reduced_constant_expression_p tripped on bit-cast3.C, which failed that
>> predicate due to the presence of an empty field in the result of
>> native_interpret_aggregate, which reduced_constant_expression_p rejects to
>> avoid confusing output_constructor.
>>
>> This patch proposes to skip such fields in native_interpret_aggregate, since
>> they aren't actually involved in the value representation.
>>
>> gcc/ChangeLog:
>>
>>          * fold-const.cc (native_interpret_aggregate): Skip empty fields.
>>
>> gcc/cp/ChangeLog:
>>
>>          * constexpr.cc (cxx_eval_bit_cast): Check that the result of
>>          native_interpret_aggregate doesn't need more evaluation.
>> ---
>>   gcc/cp/constexpr.cc | 9 +++++++++
>>   gcc/fold-const.cc   | 3 ++-
>>   2 files changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc
>> index 9d85c3be5cc..6e8f1c2b61e 100644
>> --- a/gcc/cp/constexpr.cc
>> +++ b/gcc/cp/constexpr.cc
>> @@ -1440,6 +1440,8 @@ enum value_cat {
>>
>>   static tree cxx_eval_constant_expression (const constexpr_ctx *, tree,
>>                                            value_cat, bool *, bool *, tree * = NULL);
>> +static tree cxx_eval_bare_aggregate (const constexpr_ctx *, tree,
>> +                                    value_cat, bool *, bool *);
>>   static tree cxx_fold_indirect_ref (const constexpr_ctx *, location_t, tree, tree,
>>                                     bool * = NULL);
>>   static tree find_heap_var_refs (tree *, int *, void *);
>> @@ -4803,6 +4805,13 @@ cxx_eval_bit_cast (const constexpr_ctx *ctx, tree t, bool *non_constant_p,
>>          {
>>            clear_type_padding_in_mask (TREE_TYPE (t), mask);
>>            clear_uchar_or_std_byte_in_mask (loc, r, mask);
>> +         if (CHECKING_P)
>> +           {
>> +             tree e = cxx_eval_bare_aggregate (ctx, r, vc_prvalue,
>> +                                               non_constant_p, overflow_p);
>> +             gcc_checking_assert (e == r);
>> +             r = e;
>> +           }
>>          }
>>       }
>>
>> diff --git a/gcc/fold-const.cc b/gcc/fold-const.cc
>> index a02ede79fed..db8f7de5680 100644
>> --- a/gcc/fold-const.cc
>> +++ b/gcc/fold-const.cc
>> @@ -8935,7 +8935,8 @@ native_interpret_aggregate (tree type, const unsigned char *ptr, int off,
>>       return NULL_TREE;
>>     for (tree field = TYPE_FIELDS (type); field; field = DECL_CHAIN (field))
>>       {
>> -      if (TREE_CODE (field) != FIELD_DECL || DECL_PADDING_P (field))
>> +      if (TREE_CODE (field) != FIELD_DECL || DECL_PADDING_P (field)
>> +         || integer_zerop (DECL_SIZE (field)))
> 
> The gimplifier uses is_empty_type (TREE_TYPE  (field)), wouldn't that be better
> for consistency reasons at least?

Good point, thanks.  Pushed with that change.

> OK with that change or with integer_zerop if there is a reason to not
> use is_empty_type
> here.
> 
> Thanks,
> Richard.
> 
>>          continue;
>>         tree fld = field;
>>         HOST_WIDE_INT bitoff = 0, pos = 0, sz = 0;
>>
>> base-commit: caabf0973a4e9a26421c94d540e3e20051e93e77
>> --
>> 2.39.3
>>
>
diff mbox series

Patch

diff --git a/gcc/cp/constexpr.cc b/gcc/cp/constexpr.cc
index 9d85c3be5cc..6e8f1c2b61e 100644
--- a/gcc/cp/constexpr.cc
+++ b/gcc/cp/constexpr.cc
@@ -1440,6 +1440,8 @@  enum value_cat {
 
 static tree cxx_eval_constant_expression (const constexpr_ctx *, tree,
 					  value_cat, bool *, bool *, tree * = NULL);
+static tree cxx_eval_bare_aggregate (const constexpr_ctx *, tree,
+				     value_cat, bool *, bool *);
 static tree cxx_fold_indirect_ref (const constexpr_ctx *, location_t, tree, tree,
 				   bool * = NULL);
 static tree find_heap_var_refs (tree *, int *, void *);
@@ -4803,6 +4805,13 @@  cxx_eval_bit_cast (const constexpr_ctx *ctx, tree t, bool *non_constant_p,
 	{
 	  clear_type_padding_in_mask (TREE_TYPE (t), mask);
 	  clear_uchar_or_std_byte_in_mask (loc, r, mask);
+	  if (CHECKING_P)
+	    {
+	      tree e = cxx_eval_bare_aggregate (ctx, r, vc_prvalue,
+						non_constant_p, overflow_p);
+	      gcc_checking_assert (e == r);
+	      r = e;
+	    }
 	}
     }
 
diff --git a/gcc/fold-const.cc b/gcc/fold-const.cc
index a02ede79fed..db8f7de5680 100644
--- a/gcc/fold-const.cc
+++ b/gcc/fold-const.cc
@@ -8935,7 +8935,8 @@  native_interpret_aggregate (tree type, const unsigned char *ptr, int off,
     return NULL_TREE;
   for (tree field = TYPE_FIELDS (type); field; field = DECL_CHAIN (field))
     {
-      if (TREE_CODE (field) != FIELD_DECL || DECL_PADDING_P (field))
+      if (TREE_CODE (field) != FIELD_DECL || DECL_PADDING_P (field)
+	  || integer_zerop (DECL_SIZE (field)))
 	continue;
       tree fld = field;
       HOST_WIDE_INT bitoff = 0, pos = 0, sz = 0;