[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.