c++: Fix ICE with invalid defaulted operator <=> [PR118387]

  Hi!

In the following testcase there are 2 issues, one is that B doesn't
have operator<=> and the other is that A's operator<=> has int return
type, i.e. not the standard comparison category.
Because of the int return type, retcat is cc_last; when we first
try to synthetize it, it is therefore with tentative false and complain
tf_none, we find that B doesn't have operator<=> and because retcat isn't
tc_last, don't try to search for other operators in genericize_spaceship.
And then mark the operator deleted.
When trying to explain the use of the deleted operator, tentative is still
false, but complain is tf_error_or_warning.
do_one_comp will first do:
  tree comp = build_new_op (loc, code, flags, lhs, rhs,
                            NULL_TREE, NULL_TREE, &overload,
                            tentative ? tf_none : complain);
and because complain isn't tf_none, it will actually diagnose the bug
already, but then (tentative || complain) is true and we call
genericize_spaceship, which has
  if (tag == cc_last && is_auto (type))
    {
...
    }

  gcc_checking_assert (tag < cc_last);
and because tag is cc_last and type isn't auto, we just ICE on that
assertion.

The following patch fixes it by calling genericize_spaceship only if
tentative or complain with auto return type in which case
genericize_spaceship can deal with that.

Other possibility would be

	Jakub
  

On 1/10/25 5:02 AM, Jakub Jelinek wrote:
> Hi!
> 
> In the following testcase there are 2 issues, one is that B doesn't
> have operator<=> and the other is that A's operator<=> has int return
> type, i.e. not the standard comparison category.
> Because of the int return type, retcat is cc_last; when we first
> try to synthetize it, it is therefore with tentative false and complain
> tf_none, we find that B doesn't have operator<=> and because retcat isn't
> tc_last, don't try to search for other operators in genericize_spaceship.
> And then mark the operator deleted.
> When trying to explain the use of the deleted operator, tentative is still
> false, but complain is tf_error_or_warning.
> do_one_comp will first do:
>    tree comp = build_new_op (loc, code, flags, lhs, rhs,
>                              NULL_TREE, NULL_TREE, &overload,
>                              tentative ? tf_none : complain);
> and because complain isn't tf_none, it will actually diagnose the bug
> already, but then (tentative || complain) is true and we call
> genericize_spaceship, which has
>    if (tag == cc_last && is_auto (type))
>      {
> ...
>      }
> 
>    gcc_checking_assert (tag < cc_last);
> and because tag is cc_last and type isn't auto, we just ICE on that
> assertion.
> 
> The following patch fixes it by calling genericize_spaceship only if
> tentative or complain with auto return type in which case
> genericize_spaceship can deal with that.
> 
> Other possibility would be
> --- gcc/cp/method.cc.jj	2025-01-08 23:11:23.375456869 +0100
> +++ gcc/cp/method.cc	2025-01-09 19:06:05.529933600 +0100
> @@ -1097,8 +1097,8 @@ genericize_spaceship (location_t loc, tr
>         if (type == error_mark_node)
>   	return error_mark_node;
>       }
> -
> -  gcc_checking_assert (tag < cc_last);
> +  else if (tag == cc_last)
> +    return error_mark_node;

I think I prefer this approach; such a patch is OK.

>     tree r;
>     bool scalar = SCALAR_TYPE_P (TREE_TYPE (op0));
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> 
> Note, the PR raises another problem.
> If on the same testcase the B b; line is removed, we silently synthetize
> operator<=> which will crash at runtime due to returning without a return
> statement.  That is because the standard says that in that case
> it should return static_cast<int>(std::strong_ordering::equal);
> but I can't find anywhere wording which would say that if that isn't
> valid, the function is deleted.
> https://eel.is/c++draft/class.compare#class.spaceship-2.2
> seems to talk just about cases where there are some members and their
> comparison is invalid it is deleted, but here there are none and it
> follows
> https://eel.is/c++draft/class.compare#class.spaceship-3.sentence-2
> So, we synthetize with tf_none, see the static_cast is invalid, don't
> add error_mark_node statement silently, but as the function isn't deleted,
> we just silently emit it.
> Should the standard be amended to say that the operator should be deleted
> even if it has no elements and the static cast from
> https://eel.is/c++draft/class.compare#class.spaceship-3.sentence-2
> ?

That seems pretty obviously what we want, and is what the other 
compilers implement.

> 2025-01-10  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c++/118387
> 	* method.cc (do_one_comp): Don't call genericize_spaceship if
> 	!tentative and rettype is not auto.
> 
> 	* g++.dg/cpp2a/spaceship-synth17.C: New test.
> 
> --- gcc/cp/method.cc.jj	2025-01-08 23:11:23.375456869 +0100
> +++ gcc/cp/method.cc	2025-01-09 18:56:03.246302240 +0100
> @@ -1399,7 +1399,8 @@ do_one_comp (location_t loc, const comp_
>   
>     if (comp == error_mark_node)
>       {
> -      if (overload == NULL_TREE && (tentative || complain))
> +      if (overload == NULL_TREE
> +	  && (tentative || (complain && is_auto (rettype))))
>   	{
>   	  /* No viable <=>, try using op< and op==.  */
>   	  tree lteq = genericize_spaceship (loc, rettype, lhs, rhs);
> --- gcc/testsuite/g++.dg/cpp2a/spaceship-synth17.C.jj	2025-01-09 19:00:39.416464901 +0100
> +++ gcc/testsuite/g++.dg/cpp2a/spaceship-synth17.C	2025-01-09 19:03:22.803194662 +0100
> @@ -0,0 +1,19 @@
> +// PR c++/118387
> +// { dg-do compile { target c++20 } }
> +
> +#include <compare>
> +
> +struct B {};
> +
> +struct A
> +{
> +  B b;			// { dg-error "no match for 'operator<=>' in '\[^\n\r]*' \\\(operand types are 'B' and 'B'\\\)" }
> +  int operator<=> (const A &) const = default;
> +};
> +
> +int
> +main ()
> +{
> +  A a;
> +  return a <=> a;	// { dg-error "use of deleted function 'constexpr int A::operator<=>\\\(const A&\\\) const'" }
> +}
> 
> 	Jakub
>