c++: Handle array members in build_comparison_op [PR93480]

Hi!

http://eel.is/c++draft/class.compare.default#6 says for the
expanded list of subobjects:
"In that list, any subobject of array type is recursively expanded
to the sequence of its elements, in the order of increasing subscript."
but build_comparison_op just tried to compare the whole arrays, which
failed and therefore the defaulted comparison was deleted.

The following patch instead compares the array elements, and
if info.defining, adds runtime loops around it so that it iterates
over increasing subscripts.

For flexible array members it punts, we don't know how large those will be,
for zero sized arrays it doesn't even try to compare the elements,
because if there are no elements, there is nothing to compare, and
for [1] arrays it will not emit a loop because it is enough to use
[0] array ref to cover everything.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2020-12-21  Jakub Jelinek  <jakub@redhat.com>

	PR c++/93480
	* method.c (common_comparison_type): If comps[i] is a TREE_LIST,
	use its TREE_VALUE instead.
	(build_comparison_op): Handle array members.

	* g++.dg/cpp2a/spaceship-synth10.C: New test.
	* g++.dg/cpp2a/spaceship-synth-neg5.C: New test.


	Jakub

On 12/21/20 3:12 AM, Jakub Jelinek wrote:
> Hi!
> 
> http://eel.is/c++draft/class.compare.default#6 says for the
> expanded list of subobjects:
> "In that list, any subobject of array type is recursively expanded
> to the sequence of its elements, in the order of increasing subscript."
> but build_comparison_op just tried to compare the whole arrays, which
> failed and therefore the defaulted comparison was deleted.
> 
> The following patch instead compares the array elements, and
> if info.defining, adds runtime loops around it so that it iterates
> over increasing subscripts.
> 
> For flexible array members it punts, we don't know how large those will be,
> for zero sized arrays it doesn't even try to compare the elements,
> because if there are no elements, there is nothing to compare, and
> for [1] arrays it will not emit a loop because it is enough to use
> [0] array ref to cover everything.
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Please add a comment somewhere describing the meaning of the different 
fields of this TREE_LIST.  OK with that change.

> 2020-12-21  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c++/93480
> 	* method.c (common_comparison_type): If comps[i] is a TREE_LIST,
> 	use its TREE_VALUE instead.
> 	(build_comparison_op): Handle array members.
> 
> 	* g++.dg/cpp2a/spaceship-synth10.C: New test.
> 	* g++.dg/cpp2a/spaceship-synth-neg5.C: New test.
> 
> --- gcc/cp/method.c.jj	2020-12-09 00:00:27.047344706 +0100
> +++ gcc/cp/method.c	2020-12-20 15:14:03.048393226 +0100
> @@ -1230,6 +1230,8 @@ common_comparison_type (vec<tree> &comps
>     for (unsigned i = 0; i < comps.length(); ++i)
>       {
>         tree comp = comps[i];
> +      if (TREE_CODE (comp) == TREE_LIST)
> +	comp = TREE_VALUE (comp);
>         tree ctype = TREE_TYPE (comp);
>         comp_cat_tag tag = cat_tag_for (ctype);
>         /* build_comparison_op already checked this.  */
> @@ -1419,10 +1421,47 @@ build_comparison_op (tree fndecl, tsubst
>   	      continue;
>   	    }
>   
> -	  tree lhs_mem = build3 (COMPONENT_REF, expr_type, lhs, field,
> -				 NULL_TREE);
> -	  tree rhs_mem = build3 (COMPONENT_REF, expr_type, rhs, field,
> -				 NULL_TREE);
> +	  tree lhs_mem = build3_loc (field_loc, COMPONENT_REF, expr_type, lhs,
> +				     field, NULL_TREE);
> +	  tree rhs_mem = build3_loc (field_loc, COMPONENT_REF, expr_type, rhs,
> +				     field, NULL_TREE);
> +	  tree loop_indexes = NULL_TREE;
> +	  while (TREE_CODE (expr_type) == ARRAY_TYPE)
> +	    {
> +	      /* Flexible array member.  */
> +	      if (TYPE_DOMAIN (expr_type) == NULL_TREE
> +		  || TYPE_MAX_VALUE (TYPE_DOMAIN (expr_type)) == NULL_TREE)
> +		{
> +		  if (complain & tf_error)
> +		    inform (field_loc, "cannot default compare "
> +				       "flexible array member");
> +		  bad = true;
> +		  break;
> +		}
> +	      tree maxval = TYPE_MAX_VALUE (TYPE_DOMAIN (expr_type));
> +	      /* [0] array.  No subobjects to compare, just skip it.  */
> +	      if (integer_all_onesp (maxval))
> +		break;
> +	      tree idx;
> +	      /* [1] array, no loop needed, just add [0] ARRAY_REF.
> +		 Similarly if !info.defining.  */
> +	      if (integer_zerop (maxval) || !info.defining)
> +		idx = size_zero_node;
> +	      /* Some other array, will need runtime loop.  */
> +	      else
> +		{
> +		  idx = force_target_expr (sizetype, maxval, complain);
> +		  loop_indexes = tree_cons (idx, NULL_TREE, loop_indexes);
> +		}
> +	      expr_type = TREE_TYPE (expr_type);
> +	      lhs_mem = build4_loc (field_loc, ARRAY_REF, expr_type, lhs_mem,
> +				    idx, NULL_TREE, NULL_TREE);
> +	      rhs_mem = build4_loc (field_loc, ARRAY_REF, expr_type, rhs_mem,
> +				    idx, NULL_TREE, NULL_TREE);
> +	    }
> +	  if (TREE_CODE (expr_type) == ARRAY_TYPE)
> +	    continue;
> +
>   	  tree overload = NULL_TREE;
>   	  tree comp = build_new_op (field_loc, code, flags, lhs_mem, rhs_mem,
>   				    NULL_TREE, &overload,
> @@ -1486,6 +1525,11 @@ build_comparison_op (tree fndecl, tsubst
>   	      bad = true;
>   	      continue;
>   	    }
> +	  if (loop_indexes)
> +	    {
> +	      TREE_VALUE (loop_indexes) = comp;
> +	      comp = loop_indexes;
> +	    }
>   	  comps.safe_push (comp);
>   	}
>         if (code == SPACESHIP_EXPR && is_auto (rettype))
> @@ -1502,8 +1546,35 @@ build_comparison_op (tree fndecl, tsubst
>   	{
>   	  tree comp = comps[i];
>   	  tree eq, retval = NULL_TREE, if_ = NULL_TREE;
> +	  tree loop_indexes = NULL_TREE;
>   	  if (info.defining)
> -	    if_ = begin_if_stmt ();
> +	    {
> +	      if (TREE_CODE (comp) == TREE_LIST)
> +		{
> +		  loop_indexes = comp;
> +		  comp = TREE_VALUE (comp);
> +		  loop_indexes = nreverse (loop_indexes);
> +		  for (tree loop_index = loop_indexes; loop_index;
> +		       loop_index = TREE_CHAIN (loop_index))
> +		    {
> +		      tree for_stmt = begin_for_stmt (NULL_TREE, NULL_TREE);
> +		      tree idx = TREE_PURPOSE (loop_index);
> +		      tree maxval = TARGET_EXPR_INITIAL (idx);
> +		      TARGET_EXPR_INITIAL (idx) = size_zero_node;
> +		      add_stmt (idx);
> +		      finish_init_stmt (for_stmt);
> +		      finish_for_cond (build2 (LE_EXPR, boolean_type_node, idx,
> +					       maxval), for_stmt, false, 0);
> +		      finish_for_expr (cp_build_unary_op (PREINCREMENT_EXPR,
> +							  TARGET_EXPR_SLOT (idx),
> +							  false, complain),
> +							  for_stmt);
> +		      TREE_VALUE (loop_index) = for_stmt;
> +		    }
> +		  loop_indexes = nreverse (loop_indexes);
> +		}
> +	      if_ = begin_if_stmt ();
> +	    }
>   	  /* Spaceship is specified to use !=, but for the comparison category
>   	     types, != is equivalent to !(==), so let's use == directly.  */
>   	  if (code == EQ_EXPR)
> @@ -1542,6 +1613,9 @@ build_comparison_op (tree fndecl, tsubst
>   	      finish_return_stmt (retval);
>   	      finish_else_clause (if_);
>   	      finish_if_stmt (if_);
> +	      for (tree loop_index = loop_indexes; loop_index;
> +		   loop_index = TREE_CHAIN (loop_index))
> +		finish_for_stmt (TREE_VALUE (loop_index));
>   	    }
>   	}
>         if (info.defining)
> --- gcc/testsuite/g++.dg/cpp2a/spaceship-synth10.C.jj	2020-12-20 15:03:01.770871487 +0100
> +++ gcc/testsuite/g++.dg/cpp2a/spaceship-synth10.C	2020-12-20 15:15:26.311452033 +0100
> @@ -0,0 +1,57 @@
> +// { dg-do run { target c++20 } }
> +// { dg-options "" }
> +
> +#include <compare>
> +
> +struct C {
> +  int y;
> +  int x[4];
> +  auto operator<=>(C const&) const = default;
> +};
> +
> +struct D {
> +  int y;
> +  int x[1];
> +  auto operator<=>(D const&) const = default;
> +};
> +
> +struct E {
> +  int y;
> +  int x[0];
> +  auto operator<=>(E const&) const = default;
> +};
> +
> +int
> +main ()
> +{
> +  constexpr C c1 = { 1, { 2, 3, 4, 5 } };
> +  constexpr C c2 = { 1, { 2, 3, 5, 4 } };
> +  constexpr C c3 = { 1, { 2, 2, 6, 7 } };
> +  static_assert (c1 < c2);
> +  static_assert (c3 < c1);
> +  constexpr D d1 = { 1, { 2 } };
> +  constexpr D d2 = { 1, { 3 } };
> +  constexpr D d3 = { 1, { 1 } };
> +  static_assert (d1 < d2);
> +  static_assert (d3 < d1);
> +  constexpr E e1 = { 1, {} };
> +  constexpr E e2 = { 2, {} };
> +  constexpr E e3 = { 1, {} };
> +  static_assert (e1 < e2);
> +  static_assert (e1 == e3);
> +  C c4 = { 1, { 2, 3, 4, 5 } };
> +  C c5 = { 1, { 2, 3, 5, 4 } };
> +  C c6 = { 1, { 2, 2, 6, 7 } };
> +  if (c4 >= c5 || c6 >= c4)
> +    __builtin_abort ();
> +  D d4 = { 1, { 2 } };
> +  D d5 = { 1, { 3 } };
> +  D d6 = { 1, { 1 } };
> +  if (d4 >= d5 || d6 >= d4)
> +    __builtin_abort ();
> +  E e4 = { 1, {} };
> +  E e5 = { 2, {} };
> +  E e6 = { 1, {} };
> +  if (e4 >= e5 || e4 != e6)
> +    __builtin_abort ();
> +}
> --- gcc/testsuite/g++.dg/cpp2a/spaceship-synth-neg5.C.jj	2020-12-20 15:05:36.713119245 +0100
> +++ gcc/testsuite/g++.dg/cpp2a/spaceship-synth-neg5.C	2020-12-20 15:17:00.203390689 +0100
> @@ -0,0 +1,16 @@
> +// { dg-do compile { target c++20 } }
> +// { dg-options "" }
> +
> +#include <compare>
> +
> +struct C {
> +  int y;
> +  int x[];					// { dg-message "cannot default compare flexible array member" }
> +  auto operator<=>(C const&) const = default;	// { dg-message "is implicitly deleted because the default definition would be ill-formed" }
> +};
> +
> +bool
> +foo (C &c1, C &c2)
> +{
> +  return c1 > c2;	// { dg-error "use of deleted function" }
> +}
> 
> 	Jakub
>