[PATCH] Fix bug in C++ overload resolution

[PATCH] Fix bug in C++ overload resolution

[Tom Tromey]

PR c++/28901 points out a bug in C++ overload resolution.  When
comparing two overloads, one might be better than the other for
certain parameters -- but, if that one also has some invalid
conversion, then it should never be considered the better choice.
Instead, a valid-but-not-apparently-quite-as-good overload should be
preferred.

This patch fixes this problem by changing how overload comparisons are
done.  I don't believe it should affect any currently valid overload
resolution; nor should it affect resolutions where all the choices are
equally invalid.
---
 gdb/gdbtypes.c                    | 43 ++++++++++++++++++++++++-------
 gdb/testsuite/gdb.cp/overload.cc  | 10 +++++++
 gdb/testsuite/gdb.cp/overload.exp |  3 +++
 3 files changed, 47 insertions(+), 9 deletions(-)

diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
index a2dbe7551b5..f41d6bd960e 100644
--- a/gdb/gdbtypes.c
+++ b/gdb/gdbtypes.c
@@ -44,12 +44,15 @@
 #include <algorithm>
 #include "gmp-utils.h"
 
+/* The value of an invalid conversion badness.  */
+#define INVALID_CONVERSION 100
+
 /* Initialize BADNESS constants.  */
 
-const struct rank LENGTH_MISMATCH_BADNESS = {100,0};
+const struct rank LENGTH_MISMATCH_BADNESS = {INVALID_CONVERSION,0};
 
-const struct rank TOO_FEW_PARAMS_BADNESS = {100,0};
-const struct rank INCOMPATIBLE_TYPE_BADNESS = {100,0};
+const struct rank TOO_FEW_PARAMS_BADNESS = {INVALID_CONVERSION,0};
+const struct rank INCOMPATIBLE_TYPE_BADNESS = {INVALID_CONVERSION,0};
 
 const struct rank EXACT_MATCH_BADNESS = {0,0};
 
@@ -3966,8 +3969,14 @@ compare_badness (const badness_vector &a, const badness_vector &b)
 {
   int i;
   int tmp;
-  short found_pos = 0;		/* any positives in c? */
-  short found_neg = 0;		/* any negatives in c? */
+  /* Any positives in comparison? */
+  bool found_pos = false;
+  /* Any negatives in comparison? */
+  bool found_neg = false;
+  /* Did A have any INVALID_CONVERSION entries.  */
+  bool a_invalid = false;
+  /* Did B have any INVALID_CONVERSION entries.  */
+  bool b_invalid = false;
 
   /* differing sizes => incomparable */
   if (a.size () != b.size ())
@@ -3978,12 +3987,27 @@ compare_badness (const badness_vector &a, const badness_vector &b)
     {
       tmp = compare_ranks (b[i], a[i]);
       if (tmp > 0)
-	found_pos = 1;
+	found_pos = true;
       else if (tmp < 0)
-	found_neg = 1;
+	found_neg = true;
+      if (a[i].rank >= INVALID_CONVERSION)
+	a_invalid = true;
+      if (b[i].rank >= INVALID_CONVERSION)
+	b_invalid = true;
     }
 
-  if (found_pos)
+  /* B will only be considered better than or incomparable to A if
+     they both have invalid entries, or if neither does.  That is, if
+     A has only valid entries, and B has an invalid entry, then A will
+     be considered better than B, even if B happens to be better for
+     some parameter.  */
+  if (a_invalid != b_invalid)
+    {
+      if (a_invalid)
+	return 3;		/* A > B */
+      return 2;			/* A < B */
+    }
+  else if (found_pos)
     {
       if (found_neg)
 	return 1;		/* incomparable */
@@ -4742,7 +4766,8 @@ rank_one_type_parm_set (struct type *parm, struct type *arg, struct value *value
  * Return 0 if they are identical types;
  * Otherwise, return an integer which corresponds to how compatible
  * PARM is to ARG.  The higher the return value, the worse the match.
- * Generally the "bad" conversions are all uniformly assigned a 100.  */
+ * Generally the "bad" conversions are all uniformly assigned
+ * INVALID_CONVERSION.  */
 
 struct rank
 rank_one_type (struct type *parm, struct type *arg, struct value *value)
diff --git a/gdb/testsuite/gdb.cp/overload.cc b/gdb/testsuite/gdb.cp/overload.cc
index 5c782a46104..ab015721b2b 100644
--- a/gdb/testsuite/gdb.cp/overload.cc
+++ b/gdb/testsuite/gdb.cp/overload.cc
@@ -93,10 +93,15 @@ class A {};
 class B: public A {};
 class C: public B {};
 class D: C {};
+class E {};
+class F {};
 
 int bar (A) { return 11; }
 int bar (B) { return 22; }
 
+int bar2 (E &, A &) { return 33; }
+int bar2 (F &, B &) { return 44; }
+
 int intintfunc (int x) { return x; }
 
 int main () 
@@ -119,11 +124,16 @@ int main ()
     B b;
     C c;
     D d;
+    E e;
+    F f;
 
     bar (a);
     bar (b);
     bar (c);
 
+    bar2 (e, b);
+    bar2 (f, b);
+
     char *str = (char *) "A";
     foo foo_instance1(111);
     foo foo_instance2(222, str);
diff --git a/gdb/testsuite/gdb.cp/overload.exp b/gdb/testsuite/gdb.cp/overload.exp
index 56cd5ac2106..73ca0d2d55c 100644
--- a/gdb/testsuite/gdb.cp/overload.exp
+++ b/gdb/testsuite/gdb.cp/overload.exp
@@ -259,6 +259,9 @@ gdb_test "print bar(b)" "= 22"
 gdb_test "print bar(c)" "= 22"
 gdb_test "print bar(d)" "= 22"
 
+# PR c++/28901 - gdb thought this was ambiguous.
+gdb_test "print bar2(e, b)" " = 33"
+
 # ---
 
 # List overloaded functions.
-- 
2.31.1

Re: [PATCH] Fix bug in C++ overload resolution

[Bruno Larsen]

On 2/18/22 18:07, Tom Tromey via Gdb-patches wrote:
> PR c++/28901 points out a bug in C++ overload resolution.  When
> comparing two overloads, one might be better than the other for
> certain parameters -- but, if that one also has some invalid
> conversion, then it should never be considered the better choice.
> Instead, a valid-but-not-apparently-quite-as-good overload should be
> preferred.
> 
> This patch fixes this problem by changing how overload comparisons are
> done.  I don't believe it should affect any currently valid overload
> resolution; nor should it affect resolutions where all the choices are
> equally invalid.

LGTM. Doesn't introduce any regressions, does fix the testcase and the overall code looks good

-- 
Cheers!
Bruno Larsen
> ---
>   gdb/gdbtypes.c                    | 43 ++++++++++++++++++++++++-------
>   gdb/testsuite/gdb.cp/overload.cc  | 10 +++++++
>   gdb/testsuite/gdb.cp/overload.exp |  3 +++
>   3 files changed, 47 insertions(+), 9 deletions(-)
> 
> diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
> index a2dbe7551b5..f41d6bd960e 100644
> --- a/gdb/gdbtypes.c
> +++ b/gdb/gdbtypes.c
> @@ -44,12 +44,15 @@
>   #include <algorithm>
>   #include "gmp-utils.h"
>   
> +/* The value of an invalid conversion badness.  */
> +#define INVALID_CONVERSION 100
> +
>   /* Initialize BADNESS constants.  */
>   
> -const struct rank LENGTH_MISMATCH_BADNESS = {100,0};
> +const struct rank LENGTH_MISMATCH_BADNESS = {INVALID_CONVERSION,0};
>   
> -const struct rank TOO_FEW_PARAMS_BADNESS = {100,0};
> -const struct rank INCOMPATIBLE_TYPE_BADNESS = {100,0};
> +const struct rank TOO_FEW_PARAMS_BADNESS = {INVALID_CONVERSION,0};
> +const struct rank INCOMPATIBLE_TYPE_BADNESS = {INVALID_CONVERSION,0};
>   
>   const struct rank EXACT_MATCH_BADNESS = {0,0};
>   
> @@ -3966,8 +3969,14 @@ compare_badness (const badness_vector &a, const badness_vector &b)
>   {
>     int i;
>     int tmp;
> -  short found_pos = 0;		/* any positives in c? */
> -  short found_neg = 0;		/* any negatives in c? */
> +  /* Any positives in comparison? */
> +  bool found_pos = false;
> +  /* Any negatives in comparison? */
> +  bool found_neg = false;
> +  /* Did A have any INVALID_CONVERSION entries.  */
> +  bool a_invalid = false;
> +  /* Did B have any INVALID_CONVERSION entries.  */
> +  bool b_invalid = false;
>   
>     /* differing sizes => incomparable */
>     if (a.size () != b.size ())
> @@ -3978,12 +3987,27 @@ compare_badness (const badness_vector &a, const badness_vector &b)
>       {
>         tmp = compare_ranks (b[i], a[i]);
>         if (tmp > 0)
> -	found_pos = 1;
> +	found_pos = true;
>         else if (tmp < 0)
> -	found_neg = 1;
> +	found_neg = true;
> +      if (a[i].rank >= INVALID_CONVERSION)
> +	a_invalid = true;
> +      if (b[i].rank >= INVALID_CONVERSION)
> +	b_invalid = true;
>       }
>   
> -  if (found_pos)
> +  /* B will only be considered better than or incomparable to A if
> +     they both have invalid entries, or if neither does.  That is, if
> +     A has only valid entries, and B has an invalid entry, then A will
> +     be considered better than B, even if B happens to be better for
> +     some parameter.  */
> +  if (a_invalid != b_invalid)
> +    {
> +      if (a_invalid)
> +	return 3;		/* A > B */
> +      return 2;			/* A < B */
> +    }
> +  else if (found_pos)
>       {
>         if (found_neg)
>   	return 1;		/* incomparable */
> @@ -4742,7 +4766,8 @@ rank_one_type_parm_set (struct type *parm, struct type *arg, struct value *value
>    * Return 0 if they are identical types;
>    * Otherwise, return an integer which corresponds to how compatible
>    * PARM is to ARG.  The higher the return value, the worse the match.
> - * Generally the "bad" conversions are all uniformly assigned a 100.  */
> + * Generally the "bad" conversions are all uniformly assigned
> + * INVALID_CONVERSION.  */
>   
>   struct rank
>   rank_one_type (struct type *parm, struct type *arg, struct value *value)
> diff --git a/gdb/testsuite/gdb.cp/overload.cc b/gdb/testsuite/gdb.cp/overload.cc
> index 5c782a46104..ab015721b2b 100644
> --- a/gdb/testsuite/gdb.cp/overload.cc
> +++ b/gdb/testsuite/gdb.cp/overload.cc
> @@ -93,10 +93,15 @@ class A {};
>   class B: public A {};
>   class C: public B {};
>   class D: C {};
> +class E {};
> +class F {};
>   
>   int bar (A) { return 11; }
>   int bar (B) { return 22; }
>   
> +int bar2 (E &, A &) { return 33; }
> +int bar2 (F &, B &) { return 44; }
> +
>   int intintfunc (int x) { return x; }
>   
>   int main ()
> @@ -119,11 +124,16 @@ int main ()
>       B b;
>       C c;
>       D d;
> +    E e;
> +    F f;
>   
>       bar (a);
>       bar (b);
>       bar (c);
>   
> +    bar2 (e, b);
> +    bar2 (f, b);
> +
>       char *str = (char *) "A";
>       foo foo_instance1(111);
>       foo foo_instance2(222, str);
> diff --git a/gdb/testsuite/gdb.cp/overload.exp b/gdb/testsuite/gdb.cp/overload.exp
> index 56cd5ac2106..73ca0d2d55c 100644
> --- a/gdb/testsuite/gdb.cp/overload.exp
> +++ b/gdb/testsuite/gdb.cp/overload.exp
> @@ -259,6 +259,9 @@ gdb_test "print bar(b)" "= 22"
>   gdb_test "print bar(c)" "= 22"
>   gdb_test "print bar(d)" "= 22"
>   
> +# PR c++/28901 - gdb thought this was ambiguous.
> +gdb_test "print bar2(e, b)" " = 33"
> +
>   # ---
>   
>   # List overloaded functions.

Re: [PATCH] Fix bug in C++ overload resolution

[Andrew Burgess]

Tom Tromey via Gdb-patches <gdb-patches@sourceware.org> writes:

> PR c++/28901 points out a bug in C++ overload resolution.  When
> comparing two overloads, one might be better than the other for
> certain parameters -- but, if that one also has some invalid
> conversion, then it should never be considered the better choice.
> Instead, a valid-but-not-apparently-quite-as-good overload should be
> preferred.
>
> This patch fixes this problem by changing how overload comparisons are
> done.  I don't believe it should affect any currently valid overload
> resolution; nor should it affect resolutions where all the choices are
> equally invalid.

LGTM.

Thanks,
Andrew

> ---
>  gdb/gdbtypes.c                    | 43 ++++++++++++++++++++++++-------
>  gdb/testsuite/gdb.cp/overload.cc  | 10 +++++++
>  gdb/testsuite/gdb.cp/overload.exp |  3 +++
>  3 files changed, 47 insertions(+), 9 deletions(-)
>
> diff --git a/gdb/gdbtypes.c b/gdb/gdbtypes.c
> index a2dbe7551b5..f41d6bd960e 100644
> --- a/gdb/gdbtypes.c
> +++ b/gdb/gdbtypes.c
> @@ -44,12 +44,15 @@
>  #include <algorithm>
>  #include "gmp-utils.h"
>  
> +/* The value of an invalid conversion badness.  */
> +#define INVALID_CONVERSION 100
> +
>  /* Initialize BADNESS constants.  */
>  
> -const struct rank LENGTH_MISMATCH_BADNESS = {100,0};
> +const struct rank LENGTH_MISMATCH_BADNESS = {INVALID_CONVERSION,0};
>  
> -const struct rank TOO_FEW_PARAMS_BADNESS = {100,0};
> -const struct rank INCOMPATIBLE_TYPE_BADNESS = {100,0};
> +const struct rank TOO_FEW_PARAMS_BADNESS = {INVALID_CONVERSION,0};
> +const struct rank INCOMPATIBLE_TYPE_BADNESS = {INVALID_CONVERSION,0};
>  
>  const struct rank EXACT_MATCH_BADNESS = {0,0};
>  
> @@ -3966,8 +3969,14 @@ compare_badness (const badness_vector &a, const badness_vector &b)
>  {
>    int i;
>    int tmp;
> -  short found_pos = 0;		/* any positives in c? */
> -  short found_neg = 0;		/* any negatives in c? */
> +  /* Any positives in comparison? */
> +  bool found_pos = false;
> +  /* Any negatives in comparison? */
> +  bool found_neg = false;
> +  /* Did A have any INVALID_CONVERSION entries.  */
> +  bool a_invalid = false;
> +  /* Did B have any INVALID_CONVERSION entries.  */
> +  bool b_invalid = false;
>  
>    /* differing sizes => incomparable */
>    if (a.size () != b.size ())
> @@ -3978,12 +3987,27 @@ compare_badness (const badness_vector &a, const badness_vector &b)
>      {
>        tmp = compare_ranks (b[i], a[i]);
>        if (tmp > 0)
> -	found_pos = 1;
> +	found_pos = true;
>        else if (tmp < 0)
> -	found_neg = 1;
> +	found_neg = true;
> +      if (a[i].rank >= INVALID_CONVERSION)
> +	a_invalid = true;
> +      if (b[i].rank >= INVALID_CONVERSION)
> +	b_invalid = true;
>      }
>  
> -  if (found_pos)
> +  /* B will only be considered better than or incomparable to A if
> +     they both have invalid entries, or if neither does.  That is, if
> +     A has only valid entries, and B has an invalid entry, then A will
> +     be considered better than B, even if B happens to be better for
> +     some parameter.  */
> +  if (a_invalid != b_invalid)
> +    {
> +      if (a_invalid)
> +	return 3;		/* A > B */
> +      return 2;			/* A < B */
> +    }
> +  else if (found_pos)
>      {
>        if (found_neg)
>  	return 1;		/* incomparable */
> @@ -4742,7 +4766,8 @@ rank_one_type_parm_set (struct type *parm, struct type *arg, struct value *value
>   * Return 0 if they are identical types;
>   * Otherwise, return an integer which corresponds to how compatible
>   * PARM is to ARG.  The higher the return value, the worse the match.
> - * Generally the "bad" conversions are all uniformly assigned a 100.  */
> + * Generally the "bad" conversions are all uniformly assigned
> + * INVALID_CONVERSION.  */
>  
>  struct rank
>  rank_one_type (struct type *parm, struct type *arg, struct value *value)
> diff --git a/gdb/testsuite/gdb.cp/overload.cc b/gdb/testsuite/gdb.cp/overload.cc
> index 5c782a46104..ab015721b2b 100644
> --- a/gdb/testsuite/gdb.cp/overload.cc
> +++ b/gdb/testsuite/gdb.cp/overload.cc
> @@ -93,10 +93,15 @@ class A {};
>  class B: public A {};
>  class C: public B {};
>  class D: C {};
> +class E {};
> +class F {};
>  
>  int bar (A) { return 11; }
>  int bar (B) { return 22; }
>  
> +int bar2 (E &, A &) { return 33; }
> +int bar2 (F &, B &) { return 44; }
> +
>  int intintfunc (int x) { return x; }
>  
>  int main () 
> @@ -119,11 +124,16 @@ int main ()
>      B b;
>      C c;
>      D d;
> +    E e;
> +    F f;
>  
>      bar (a);
>      bar (b);
>      bar (c);
>  
> +    bar2 (e, b);
> +    bar2 (f, b);
> +
>      char *str = (char *) "A";
>      foo foo_instance1(111);
>      foo foo_instance2(222, str);
> diff --git a/gdb/testsuite/gdb.cp/overload.exp b/gdb/testsuite/gdb.cp/overload.exp
> index 56cd5ac2106..73ca0d2d55c 100644
> --- a/gdb/testsuite/gdb.cp/overload.exp
> +++ b/gdb/testsuite/gdb.cp/overload.exp
> @@ -259,6 +259,9 @@ gdb_test "print bar(b)" "= 22"
>  gdb_test "print bar(c)" "= 22"
>  gdb_test "print bar(d)" "= 22"
>  
> +# PR c++/28901 - gdb thought this was ambiguous.
> +gdb_test "print bar2(e, b)" " = 33"
> +
>  # ---
>  
>  # List overloaded functions.
> -- 
> 2.31.1