[C++] Fix ICE in build_new_op_1 (PR c++/92705)

Message ID 20191129233309.GO10088@tucnak
State New
Headers show
Series
  • [C++] Fix ICE in build_new_op_1 (PR c++/92705)
Related show

Commit Message

Jakub Jelinek Nov. 29, 2019, 11:33 p.m.
Hi!

The changed code in build_new_op_1 ICEs on the following testcase,
because conv is user_conv_p with kind == ck_ambig, for which next_conversion
returns NULL.  It seems in other spots where for user_conv_p we are walking
the conversion chain we also don't assume there must be ck_user, so this
patch just uses the first conv if ck_user is not found (so that the previous
diagnostics about ambiguous conversion is emitted).

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

2019-11-29  Jakub Jelinek  <jakub@redhat.com>

	PR c++/92705
	* call.c (build_new_op_1): For user_conv_p, if there is no
	ck_user conversion, use the first one.

	* g++.dg/conversion/ambig4.C: New test.


	Jakub

Comments

Jason Merrill Dec. 2, 2019, 6:48 p.m. | #1
On 11/29/19 6:33 PM, Jakub Jelinek wrote:
> Hi!

> 

> The changed code in build_new_op_1 ICEs on the following testcase,

> because conv is user_conv_p with kind == ck_ambig, for which next_conversion

> returns NULL.  It seems in other spots where for user_conv_p we are walking

> the conversion chain we also don't assume there must be ck_user, so this

> patch just uses the first conv if ck_user is not found (so that the previous

> diagnostics about ambiguous conversion is emitted).


It seems like various places could use a function to strip down to the 
first ck_user, ck_ambig, ck_list, ck_aggr, or ck_identity conversion 
encountered: source_type, the user-defined conversion comparision in 
compare_ics, and now here.  Mind doing that refactoring?  Maybe call it 
strip_standard_conversion.

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

> 

> 2019-11-29  Jakub Jelinek  <jakub@redhat.com>

> 

> 	PR c++/92705

> 	* call.c (build_new_op_1): For user_conv_p, if there is no

> 	ck_user conversion, use the first one.

> 

> 	* g++.dg/conversion/ambig4.C: New test.

> 

> --- gcc/cp/call.c.jj	2019-11-29 12:50:09.664608879 +0100

> +++ gcc/cp/call.c	2019-11-29 14:09:54.311718859 +0100

> @@ -6370,8 +6370,12 @@ build_new_op_1 (const op_location_t &loc

>   	  conv = cand->convs[0];

>   	  if (conv->user_conv_p)

>   	    {

> -	      while (conv->kind != ck_user)

> -		conv = next_conversion (conv);

> +	      for (conversion *t = conv; t; t = next_conversion (t))

> +		if (t->kind == ck_user)

> +		  {

> +		    conv = t;

> +		    break;

> +		  }

>   	      arg1 = convert_like (conv, arg1, complain);

>   	    }

>   

> @@ -6380,8 +6384,12 @@ build_new_op_1 (const op_location_t &loc

>   	      conv = cand->convs[1];

>   	      if (conv->user_conv_p)

>   		{

> -		  while (conv->kind != ck_user)

> -		    conv = next_conversion (conv);

> +		  for (conversion *t = conv; t; t = next_conversion (t))

> +		    if (t->kind == ck_user)

> +		      {

> +			conv = t;

> +			break;

> +		      }

>   		  arg2 = convert_like (conv, arg2, complain);

>   		}

>   	    }

> @@ -6391,8 +6399,12 @@ build_new_op_1 (const op_location_t &loc

>   	      conv = cand->convs[2];

>   	      if (conv->user_conv_p)

>   		{

> -		  while (conv->kind != ck_user)

> -		    conv = next_conversion (conv);

> +		  for (conversion *t = conv; t; t = next_conversion (t))

> +		    if (t->kind == ck_user)

> +		      {

> +			conv = t;

> +			break;

> +		      }

>   		  arg3 = convert_like (conv, arg3, complain);

>   		}

>   	    }

> --- gcc/testsuite/g++.dg/conversion/ambig4.C.jj	2019-11-29 14:11:35.239183848 +0100

> +++ gcc/testsuite/g++.dg/conversion/ambig4.C	2019-11-29 14:11:07.006613238 +0100

> @@ -0,0 +1,14 @@

> +// PR c++/92705

> +// { dg-do compile }

> +

> +struct A {};

> +struct B {};

> +struct C { operator B * (); };	// { dg-message "candidate" }

> +struct D { operator B * (); };	// { dg-message "candidate" }

> +struct E : C, D { operator A * (); };

> +

> +void

> +foo (E e, int B::* pmf)

> +{

> +  int i = e->*pmf;	// { dg-error "is ambiguous" }

> +}

> 

> 	Jakub

>
Jakub Jelinek Dec. 2, 2019, 10:17 p.m. | #2
On Mon, Dec 02, 2019 at 01:48:46PM -0500, Jason Merrill wrote:
> On 11/29/19 6:33 PM, Jakub Jelinek wrote:

> > Hi!

> > 

> > The changed code in build_new_op_1 ICEs on the following testcase,

> > because conv is user_conv_p with kind == ck_ambig, for which next_conversion

> > returns NULL.  It seems in other spots where for user_conv_p we are walking

> > the conversion chain we also don't assume there must be ck_user, so this

> > patch just uses the first conv if ck_user is not found (so that the previous

> > diagnostics about ambiguous conversion is emitted).

> 

> It seems like various places could use a function to strip down to the first

> ck_user, ck_ambig, ck_list, ck_aggr, or ck_identity conversion encountered:

> source_type, the user-defined conversion comparision in compare_ics, and now

> here.  Mind doing that refactoring?  Maybe call it

> strip_standard_conversion.


In neither of the spots it is exactly equivalent to what the code was doing
before (or what the patched build_new_op_1 did), but this is an area I know
next to nothing about, so I've just tried to type into patch what you wrote
above.  Do you mean something like below (didn't see regressions in make
check-c++-all, but haven't tried full bootstrap/regtest yet)?

2019-12-02  Jason Merrill  <jason@redhat.com>
	    Jakub Jelinek  <jakub@redhat.com>

	PR c++/92705
	* call.c (strip_standard_conversion): New function.
	(build_new_op_1): Use it for user_conv_p.
	(compare_ics): Likewise.
	(source_type): Likewise.

	* g++.dg/conversion/ambig4.C: New test.

--- gcc/cp/call.c.jj	2019-11-30 00:15:46.298953538 +0100
+++ gcc/cp/call.c	2019-12-02 23:02:48.494379961 +0100
@@ -865,6 +865,22 @@ next_conversion (conversion *conv)
   return conv->u.next;
 }
 
+/* Strip to the first ck_user, ck_ambig, ck_list, ck_aggr or ck_identity
+   encountered.  */
+
+static conversion *
+strip_standard_conversion (conversion *conv)
+{
+  while (conv
+	 && conv->kind != ck_user
+	 && conv->kind != ck_ambig
+	 && conv->kind != ck_list
+	 && conv->kind != ck_aggr
+	 && conv->kind != ck_identity)
+    conv = next_conversion (conv);
+  return conv;
+}
+
 /* Subroutine of build_aggr_conv: check whether CTOR, a braced-init-list,
    is a valid aggregate initializer for array type ATYPE.  */
 
@@ -6370,8 +6386,7 @@ build_new_op_1 (const op_location_t &loc
 	  conv = cand->convs[0];
 	  if (conv->user_conv_p)
 	    {
-	      while (conv->kind != ck_user)
-		conv = next_conversion (conv);
+	      conv = strip_standard_conversion (conv);
 	      arg1 = convert_like (conv, arg1, complain);
 	    }
 
@@ -6380,8 +6395,7 @@ build_new_op_1 (const op_location_t &loc
 	      conv = cand->convs[1];
 	      if (conv->user_conv_p)
 		{
-		  while (conv->kind != ck_user)
-		    conv = next_conversion (conv);
+		  conv = strip_standard_conversion (conv);
 		  arg2 = convert_like (conv, arg2, complain);
 		}
 	    }
@@ -6391,8 +6405,7 @@ build_new_op_1 (const op_location_t &loc
 	      conv = cand->convs[2];
 	      if (conv->user_conv_p)
 		{
-		  while (conv->kind != ck_user)
-		    conv = next_conversion (conv);
+		  conv = strip_standard_conversion (conv);
 		  arg3 = convert_like (conv, arg3, complain);
 		}
 	    }
@@ -10538,17 +10551,8 @@ compare_ics (conversion *ics1, conversio
   if (ics1->user_conv_p || ics1->kind == ck_list
       || ics1->kind == ck_aggr || ics2->kind == ck_aggr)
     {
-      conversion *t1;
-      conversion *t2;
-
-      for (t1 = ics1; t1 && t1->kind != ck_user; t1 = next_conversion (t1))
-	if (t1->kind == ck_ambig || t1->kind == ck_aggr
-	    || t1->kind == ck_list)
-	  break;
-      for (t2 = ics2; t2 && t2->kind != ck_user; t2 = next_conversion (t2))
-	if (t2->kind == ck_ambig || t2->kind == ck_aggr
-	    || t2->kind == ck_list)
-	  break;
+      conversion *t1 = strip_standard_conversion (ics1);
+      conversion *t2 = strip_standard_conversion (ics2);
 
       if (!t1 || !t2 || t1->kind != t2->kind)
 	return 0;
@@ -10956,14 +10960,7 @@ compare_ics (conversion *ics1, conversio
 static tree
 source_type (conversion *t)
 {
-  for (;; t = next_conversion (t))
-    {
-      if (t->kind == ck_user
-	  || t->kind == ck_ambig
-	  || t->kind == ck_identity)
-	return t->type;
-    }
-  gcc_unreachable ();
+  return strip_standard_conversion (t)->type;
 }
 
 /* Note a warning about preferring WINNER to LOSER.  We do this by storing
--- gcc/testsuite/g++.dg/conversion/ambig4.C.jj	2019-12-02 22:52:24.407017475 +0100
+++ gcc/testsuite/g++.dg/conversion/ambig4.C	2019-12-02 22:52:24.407017475 +0100
@@ -0,0 +1,14 @@
+// PR c++/92705
+// { dg-do compile }
+
+struct A {};
+struct B {};
+struct C { operator B * (); };	// { dg-message "candidate" }
+struct D { operator B * (); };	// { dg-message "candidate" }
+struct E : C, D { operator A * (); };
+
+void
+foo (E e, int B::* pmf)
+{
+  int i = e->*pmf;	// { dg-error "is ambiguous" }
+}


	Jakub
Jason Merrill Dec. 3, 2019, 7:08 a.m. | #3
On 12/2/19 5:17 PM, Jakub Jelinek wrote:
> On Mon, Dec 02, 2019 at 01:48:46PM -0500, Jason Merrill wrote:

>> On 11/29/19 6:33 PM, Jakub Jelinek wrote:

>>> Hi!

>>>

>>> The changed code in build_new_op_1 ICEs on the following testcase,

>>> because conv is user_conv_p with kind == ck_ambig, for which next_conversion

>>> returns NULL.  It seems in other spots where for user_conv_p we are walking

>>> the conversion chain we also don't assume there must be ck_user, so this

>>> patch just uses the first conv if ck_user is not found (so that the previous

>>> diagnostics about ambiguous conversion is emitted).

>>

>> It seems like various places could use a function to strip down to the first

>> ck_user, ck_ambig, ck_list, ck_aggr, or ck_identity conversion encountered:

>> source_type, the user-defined conversion comparision in compare_ics, and now

>> here.  Mind doing that refactoring?  Maybe call it

>> strip_standard_conversion.

> 

> In neither of the spots it is exactly equivalent to what the code was doing

> before (or what the patched build_new_op_1 did), but this is an area I know

> next to nothing about, so I've just tried to type into patch what you wrote

> above.  Do you mean something like below (didn't see regressions in make

> check-c++-all, but haven't tried full bootstrap/regtest yet)?


Yes, thanks.  OK if testing passes.

Jason

> 2019-12-02  Jason Merrill  <jason@redhat.com>

> 	    Jakub Jelinek  <jakub@redhat.com>

> 

> 	PR c++/92705

> 	* call.c (strip_standard_conversion): New function.

> 	(build_new_op_1): Use it for user_conv_p.

> 	(compare_ics): Likewise.

> 	(source_type): Likewise.

> 

> 	* g++.dg/conversion/ambig4.C: New test.

> 

> --- gcc/cp/call.c.jj	2019-11-30 00:15:46.298953538 +0100

> +++ gcc/cp/call.c	2019-12-02 23:02:48.494379961 +0100

> @@ -865,6 +865,22 @@ next_conversion (conversion *conv)

>     return conv->u.next;

>   }

>   

> +/* Strip to the first ck_user, ck_ambig, ck_list, ck_aggr or ck_identity

> +   encountered.  */

> +

> +static conversion *

> +strip_standard_conversion (conversion *conv)

> +{

> +  while (conv

> +	 && conv->kind != ck_user

> +	 && conv->kind != ck_ambig

> +	 && conv->kind != ck_list

> +	 && conv->kind != ck_aggr

> +	 && conv->kind != ck_identity)

> +    conv = next_conversion (conv);

> +  return conv;

> +}

> +

>   /* Subroutine of build_aggr_conv: check whether CTOR, a braced-init-list,

>      is a valid aggregate initializer for array type ATYPE.  */

>   

> @@ -6370,8 +6386,7 @@ build_new_op_1 (const op_location_t &loc

>   	  conv = cand->convs[0];

>   	  if (conv->user_conv_p)

>   	    {

> -	      while (conv->kind != ck_user)

> -		conv = next_conversion (conv);

> +	      conv = strip_standard_conversion (conv);

>   	      arg1 = convert_like (conv, arg1, complain);

>   	    }

>   

> @@ -6380,8 +6395,7 @@ build_new_op_1 (const op_location_t &loc

>   	      conv = cand->convs[1];

>   	      if (conv->user_conv_p)

>   		{

> -		  while (conv->kind != ck_user)

> -		    conv = next_conversion (conv);

> +		  conv = strip_standard_conversion (conv);

>   		  arg2 = convert_like (conv, arg2, complain);

>   		}

>   	    }

> @@ -6391,8 +6405,7 @@ build_new_op_1 (const op_location_t &loc

>   	      conv = cand->convs[2];

>   	      if (conv->user_conv_p)

>   		{

> -		  while (conv->kind != ck_user)

> -		    conv = next_conversion (conv);

> +		  conv = strip_standard_conversion (conv);

>   		  arg3 = convert_like (conv, arg3, complain);

>   		}

>   	    }

> @@ -10538,17 +10551,8 @@ compare_ics (conversion *ics1, conversio

>     if (ics1->user_conv_p || ics1->kind == ck_list

>         || ics1->kind == ck_aggr || ics2->kind == ck_aggr)

>       {

> -      conversion *t1;

> -      conversion *t2;

> -

> -      for (t1 = ics1; t1 && t1->kind != ck_user; t1 = next_conversion (t1))

> -	if (t1->kind == ck_ambig || t1->kind == ck_aggr

> -	    || t1->kind == ck_list)

> -	  break;

> -      for (t2 = ics2; t2 && t2->kind != ck_user; t2 = next_conversion (t2))

> -	if (t2->kind == ck_ambig || t2->kind == ck_aggr

> -	    || t2->kind == ck_list)

> -	  break;

> +      conversion *t1 = strip_standard_conversion (ics1);

> +      conversion *t2 = strip_standard_conversion (ics2);

>   

>         if (!t1 || !t2 || t1->kind != t2->kind)

>   	return 0;

> @@ -10956,14 +10960,7 @@ compare_ics (conversion *ics1, conversio

>   static tree

>   source_type (conversion *t)

>   {

> -  for (;; t = next_conversion (t))

> -    {

> -      if (t->kind == ck_user

> -	  || t->kind == ck_ambig

> -	  || t->kind == ck_identity)

> -	return t->type;

> -    }

> -  gcc_unreachable ();

> +  return strip_standard_conversion (t)->type;

>   }

>   

>   /* Note a warning about preferring WINNER to LOSER.  We do this by storing

> --- gcc/testsuite/g++.dg/conversion/ambig4.C.jj	2019-12-02 22:52:24.407017475 +0100

> +++ gcc/testsuite/g++.dg/conversion/ambig4.C	2019-12-02 22:52:24.407017475 +0100

> @@ -0,0 +1,14 @@

> +// PR c++/92705

> +// { dg-do compile }

> +

> +struct A {};

> +struct B {};

> +struct C { operator B * (); };	// { dg-message "candidate" }

> +struct D { operator B * (); };	// { dg-message "candidate" }

> +struct E : C, D { operator A * (); };

> +

> +void

> +foo (E e, int B::* pmf)

> +{

> +  int i = e->*pmf;	// { dg-error "is ambiguous" }

> +}

> 

> 

> 	Jakub

>

Patch

--- gcc/cp/call.c.jj	2019-11-29 12:50:09.664608879 +0100
+++ gcc/cp/call.c	2019-11-29 14:09:54.311718859 +0100
@@ -6370,8 +6370,12 @@  build_new_op_1 (const op_location_t &loc
 	  conv = cand->convs[0];
 	  if (conv->user_conv_p)
 	    {
-	      while (conv->kind != ck_user)
-		conv = next_conversion (conv);
+	      for (conversion *t = conv; t; t = next_conversion (t))
+		if (t->kind == ck_user)
+		  {
+		    conv = t;
+		    break;
+		  }
 	      arg1 = convert_like (conv, arg1, complain);
 	    }
 
@@ -6380,8 +6384,12 @@  build_new_op_1 (const op_location_t &loc
 	      conv = cand->convs[1];
 	      if (conv->user_conv_p)
 		{
-		  while (conv->kind != ck_user)
-		    conv = next_conversion (conv);
+		  for (conversion *t = conv; t; t = next_conversion (t))
+		    if (t->kind == ck_user)
+		      {
+			conv = t;
+			break;
+		      }
 		  arg2 = convert_like (conv, arg2, complain);
 		}
 	    }
@@ -6391,8 +6399,12 @@  build_new_op_1 (const op_location_t &loc
 	      conv = cand->convs[2];
 	      if (conv->user_conv_p)
 		{
-		  while (conv->kind != ck_user)
-		    conv = next_conversion (conv);
+		  for (conversion *t = conv; t; t = next_conversion (t))
+		    if (t->kind == ck_user)
+		      {
+			conv = t;
+			break;
+		      }
 		  arg3 = convert_like (conv, arg3, complain);
 		}
 	    }
--- gcc/testsuite/g++.dg/conversion/ambig4.C.jj	2019-11-29 14:11:35.239183848 +0100
+++ gcc/testsuite/g++.dg/conversion/ambig4.C	2019-11-29 14:11:07.006613238 +0100
@@ -0,0 +1,14 @@ 
+// PR c++/92705
+// { dg-do compile }
+
+struct A {};
+struct B {};
+struct C { operator B * (); };	// { dg-message "candidate" }
+struct D { operator B * (); };	// { dg-message "candidate" }
+struct E : C, D { operator A * (); };
+
+void
+foo (E e, int B::* pmf)
+{
+  int i = e->*pmf;	// { dg-error "is ambiguous" }
+}