[PATCH] c++: ICE w/ ambig and non-strictly-viable cands [PR115239]

[PATCH] c++: ICE w/ ambig and non-strictly-viable cands [PR115239]

[Patrick Palka]

Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
trunk/14?

-- >8 --

Here during overload resolution we have two strictly viable ambiguous
candidates #1 and #2, and two non-strictly viable candidates #3 and #4
which we hold on to ever since r14-6522.  These latter candidates have
an empty third arg conversion since the second arg conversion was deemed
bad.  This ends up causing an ICE during joust for #3 and #4 due to this
empty arg conversion.

We can fix this by making joust robust to empty arg conversions, but in
this situation we shouldn't need to compare #3 and #4 at all given that
we have a strictly viable candidate.  To that end, this patch makes
tourney shortcut considering non-strictly viable candidates upon
encountering ambiguity between two strictly viable candidates, taking
advantage of the fact that the candidates list is sorted according to
viability via splice_viable.

	PR c++/115239

gcc/cp/ChangeLog:

	* call.cc (tourney): Don't consider a non-strictly viable
	candidate as the champ if there was ambiguity between two
	strictly viable candidates.

gcc/testsuite/ChangeLog:

	* g++.dg/overload/error7.C: New test.
---
 gcc/cp/call.cc                         |  4 +++-
 gcc/testsuite/g++.dg/overload/error7.C | 10 ++++++++++
 2 files changed, 13 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/g++.dg/overload/error7.C

diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc
index ed68eb3c568..82c70f5c39f 100644
--- a/gcc/cp/call.cc
+++ b/gcc/cp/call.cc
@@ -13484,9 +13484,11 @@ tourney (struct z_candidate *candidates, tsubst_flags_t complain)
 	}
       else
 	{
+	  z_candidate *prev_champ = *champ;
 	  previous_worse_champ = nullptr;
 	  champ = &(*challenger)->next;
-	  if (!*champ || !(*champ)->viable)
+	  if (!*champ || !(*champ)->viable
+	      || (prev_champ->viable == 1 && (*champ)->viable == -1))
 	    {
 	      champ = nullptr;
 	      break;
diff --git a/gcc/testsuite/g++.dg/overload/error7.C b/gcc/testsuite/g++.dg/overload/error7.C
new file mode 100644
index 00000000000..68aaa236de4
--- /dev/null
+++ b/gcc/testsuite/g++.dg/overload/error7.C
@@ -0,0 +1,10 @@
+// PR c++/115239
+
+bool foo(const char *, char *, long);     // #1, strictly viable, ambig with #2
+bool foo(const char *, char *, unsigned); // #2, strictly viable, ambig with #1
+bool foo(char, char, long);               // #3, non-strictly viable
+bool foo(char, char, unsigned);           // #4, non-strictly viable
+
+int main() {
+  foo((char *)0, (char *)0, 0); // { dg-error "ambiguous" }
+}
-- 
2.45.2.457.g8d94cfb545

Re: [PATCH] c++: ICE w/ ambig and non-strictly-viable cands [PR115239]

[Patrick Palka]

On Wed, 12 Jun 2024, Patrick Palka wrote:

> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
> trunk/14?
> 
> -- >8 --
> 
> Here during overload resolution we have two strictly viable ambiguous
> candidates #1 and #2, and two non-strictly viable candidates #3 and #4
> which we hold on to ever since r14-6522.  These latter candidates have
> an empty third arg conversion since the second arg conversion was deemed
> bad.  This ends up causing an ICE during joust for #3 and #4 due to this
> empty arg conversion.
> 
> We can fix this by making joust robust to empty arg conversions, but in
> this situation we shouldn't need to compare #3 and #4 at all given that
> we have a strictly viable candidate.  To that end, this patch makes
> tourney shortcut considering non-strictly viable candidates upon
> encountering ambiguity between two strictly viable candidates, taking
> advantage of the fact that the candidates list is sorted according to
> viability via splice_viable.
> 
> 	PR c++/115239
> 
> gcc/cp/ChangeLog:
> 
> 	* call.cc (tourney): Don't consider a non-strictly viable
> 	candidate as the champ if there was ambiguity between two
> 	strictly viable candidates.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/overload/error7.C: New test.
> ---
>  gcc/cp/call.cc                         |  4 +++-
>  gcc/testsuite/g++.dg/overload/error7.C | 10 ++++++++++
>  2 files changed, 13 insertions(+), 1 deletion(-)
>  create mode 100644 gcc/testsuite/g++.dg/overload/error7.C
> 
> diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc
> index ed68eb3c568..82c70f5c39f 100644
> --- a/gcc/cp/call.cc
> +++ b/gcc/cp/call.cc
> @@ -13484,9 +13484,11 @@ tourney (struct z_candidate *candidates, tsubst_flags_t complain)
>  	}
>        else
>  	{
> +	  z_candidate *prev_champ = *champ;
>  	  previous_worse_champ = nullptr;
>  	  champ = &(*challenger)->next;
> -	  if (!*champ || !(*champ)->viable)
> +	  if (!*champ || !(*champ)->viable
> +	      || (prev_champ->viable == 1 && (*champ)->viable == -1))
>  	    {
>  	      champ = nullptr;
>  	      break;
> diff --git a/gcc/testsuite/g++.dg/overload/error7.C b/gcc/testsuite/g++.dg/overload/error7.C
> new file mode 100644
> index 00000000000..68aaa236de4
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/overload/error7.C
> @@ -0,0 +1,10 @@
> +// PR c++/115239
> +
> +bool foo(const char *, char *, long);     // #1, strictly viable, ambig with #2
> +bool foo(const char *, char *, unsigned); // #2, strictly viable, ambig with #1
> +bool foo(char, char, long);               // #3, non-strictly viable
> +bool foo(char, char, unsigned);           // #4, non-strictly viable
> +
> +int main() {
> +  foo((char *)0, (char *)0, 0); // { dg-error "ambiguous" }
> +}

FWIW I just realized this testcase can be simplified to:

  // PR c++/115239

  bool foo(char *, long);     // #1, strictly viable, ambig with #2
  bool foo(char *, unsigned); // #2, strictly viable, ambig with #1
  bool foo(char, long);       // #3, non-strictly viable
  bool foo(char, unsigned);   // #4, non-strictly viable

  int main() {
    foo((char *)0, 0); // { dg-error "ambiguous" }
  }

Re: [PATCH] c++: ICE w/ ambig and non-strictly-viable cands [PR115239]

[Jason Merrill]

On 6/12/24 13:56, Patrick Palka wrote:
> Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
> trunk/14?
> 
> -- >8 --
> 
> Here during overload resolution we have two strictly viable ambiguous
> candidates #1 and #2, and two non-strictly viable candidates #3 and #4
> which we hold on to ever since r14-6522.  These latter candidates have
> an empty third arg conversion since the second arg conversion was deemed
> bad.  This ends up causing an ICE during joust for #3 and #4 due to this
> empty arg conversion.
> 
> We can fix this by making joust robust to empty arg conversions, but in
> this situation we shouldn't need to compare #3 and #4 at all given that
> we have a strictly viable candidate.  To that end, this patch makes
> tourney shortcut considering non-strictly viable candidates upon
> encountering ambiguity between two strictly viable candidates, taking
> advantage of the fact that the candidates list is sorted according to
> viability via splice_viable.
> 
> 	PR c++/115239
> 
> gcc/cp/ChangeLog:
> 
> 	* call.cc (tourney): Don't consider a non-strictly viable
> 	candidate as the champ if there was ambiguity between two
> 	strictly viable candidates.
> 
> gcc/testsuite/ChangeLog:
> 
> 	* g++.dg/overload/error7.C: New test.
> ---
>   gcc/cp/call.cc                         |  4 +++-
>   gcc/testsuite/g++.dg/overload/error7.C | 10 ++++++++++
>   2 files changed, 13 insertions(+), 1 deletion(-)
>   create mode 100644 gcc/testsuite/g++.dg/overload/error7.C
> 
> diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc
> index ed68eb3c568..82c70f5c39f 100644
> --- a/gcc/cp/call.cc
> +++ b/gcc/cp/call.cc
> @@ -13484,9 +13484,11 @@ tourney (struct z_candidate *candidates, tsubst_flags_t complain)
>   	}
>         else
>   	{
> +	  z_candidate *prev_champ = *champ;
>   	  previous_worse_champ = nullptr;
>   	  champ = &(*challenger)->next;
> -	  if (!*champ || !(*champ)->viable)
> +	  if (!*champ || !(*champ)->viable
> +	      || (prev_champ->viable == 1 && (*champ)->viable == -1))

Maybe

(!*champ || (*champ)->viable < prev_champ->viable) ?

OK with that change.

Jason

Re: [PATCH] c++: ICE w/ ambig and non-strictly-viable cands [PR115239]

[Patrick Palka]

On Wed, 12 Jun 2024, Jason Merrill wrote:

> On 6/12/24 13:56, Patrick Palka wrote:
> > Bootstrapped and regtested on x86_64-pc-linux-gnu, does this look OK for
> > trunk/14?
> > 
> > -- >8 --
> > 
> > Here during overload resolution we have two strictly viable ambiguous
> > candidates #1 and #2, and two non-strictly viable candidates #3 and #4
> > which we hold on to ever since r14-6522.  These latter candidates have
> > an empty third arg conversion since the second arg conversion was deemed
> > bad.  This ends up causing an ICE during joust for #3 and #4 due to this
> > empty arg conversion.
> > 
> > We can fix this by making joust robust to empty arg conversions, but in
> > this situation we shouldn't need to compare #3 and #4 at all given that
> > we have a strictly viable candidate.  To that end, this patch makes
> > tourney shortcut considering non-strictly viable candidates upon
> > encountering ambiguity between two strictly viable candidates, taking
> > advantage of the fact that the candidates list is sorted according to
> > viability via splice_viable.
> > 
> > 	PR c++/115239
> > 
> > gcc/cp/ChangeLog:
> > 
> > 	* call.cc (tourney): Don't consider a non-strictly viable
> > 	candidate as the champ if there was ambiguity between two
> > 	strictly viable candidates.
> > 
> > gcc/testsuite/ChangeLog:
> > 
> > 	* g++.dg/overload/error7.C: New test.
> > ---
> >   gcc/cp/call.cc                         |  4 +++-
> >   gcc/testsuite/g++.dg/overload/error7.C | 10 ++++++++++
> >   2 files changed, 13 insertions(+), 1 deletion(-)
> >   create mode 100644 gcc/testsuite/g++.dg/overload/error7.C
> > 
> > diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc
> > index ed68eb3c568..82c70f5c39f 100644
> > --- a/gcc/cp/call.cc
> > +++ b/gcc/cp/call.cc
> > @@ -13484,9 +13484,11 @@ tourney (struct z_candidate *candidates,
> > tsubst_flags_t complain)
> >   	}
> >         else
> >   	{
> > +	  z_candidate *prev_champ = *champ;
> >   	  previous_worse_champ = nullptr;
> >   	  champ = &(*challenger)->next;
> > -	  if (!*champ || !(*champ)->viable)
> > +	  if (!*champ || !(*champ)->viable
> > +	      || (prev_champ->viable == 1 && (*champ)->viable == -1))
> 
> Maybe
> 
> (!*champ || (*champ)->viable < prev_champ->viable) ?
> 
> OK with that change.

Nice, done.  In passing it occurred to me that we can just consider
challenger->viable instead of prev_champ->viable (they must be the
same since there was ambiguity), so we don't need to introduce
prev_champ.

This is what I ended up pushing:

-- >8 --

Subject: [PATCH] c++: ICE w/ ambig and non-strictly-viable cands [PR115239]

	PR c++/115239

gcc/cp/ChangeLog:

	* call.cc (tourney): Don't consider a non-strictly viable
	candidate as the champ if there was ambiguity between two
	strictly viable candidates.

gcc/testsuite/ChangeLog:

	* g++.dg/overload/error7.C: New test.

Reviewed-by: Jason Merrill <jason@redhat.com>
---
 gcc/cp/call.cc                         |  3 ++-
 gcc/testsuite/g++.dg/overload/error7.C | 10 ++++++++++
 2 files changed, 12 insertions(+), 1 deletion(-)
 create mode 100644 gcc/testsuite/g++.dg/overload/error7.C

diff --git a/gcc/cp/call.cc b/gcc/cp/call.cc
index 85536fc25ff..7bbc1fb0c78 100644
--- a/gcc/cp/call.cc
+++ b/gcc/cp/call.cc
@@ -13490,7 +13490,8 @@ tourney (struct z_candidate *candidates, tsubst_flags_t complain)
 	{
 	  previous_worse_champ = nullptr;
 	  champ = &(*challenger)->next;
-	  if (!*champ || !(*champ)->viable)
+	  if (!*champ || !(*champ)->viable
+	      || (*champ)->viable < (*challenger)->viable)
 	    {
 	      champ = nullptr;
 	      break;
diff --git a/gcc/testsuite/g++.dg/overload/error7.C b/gcc/testsuite/g++.dg/overload/error7.C
new file mode 100644
index 00000000000..de50ce5f66e
--- /dev/null
+++ b/gcc/testsuite/g++.dg/overload/error7.C
@@ -0,0 +1,10 @@
+// PR c++/115239
+
+bool foo(char *, long);     // #1, strictly viable, ambig with #2
+bool foo(char *, unsigned); // #2, strictly viable, ambig with #1
+bool foo(char, long);       // #3, non-strictly viable
+bool foo(char, unsigned);   // #4, non-strictly viable
+
+int main() {
+  foo((char *)0, 0); // { dg-error "ambiguous" }
+}
-- 
2.45.2.457.g8d94cfb545