Re: [PATCH] Allow calling of user-defined function call operators

[Guinevere Larsen <blarsen@redhat.com>]

On 4/21/24 09:49, Hannes Domani wrote:
> Currently it's not possible to call user-defined function call
> operators, at least not without specifying operator() directly:
> ```
> (gdb) l 1
> 1       struct S {
> 2         int operator() (int x) { return x + 5; }
> 3       };
> 4
> 5       int main () {
> 6         S s;
> 7
> 8         return s(23);
> 9       }
> (gdb) p s(10)
> Invalid data type for function to be called.
> (gdb) p s.operator()(10)
> $1 = 15
> ```
>
> This now looks if an user-defined call operator is available when
> trying to 'call' a struct value, and calls it instead, making this
> possible:
> ```
> (gdb) p s(10)
> $1 = 15
> ```
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=12213

Thanks for this fix, this has been a long time coming.

I have one, possibly big, question.

> ---
>   gdb/eval.c                       | 46 +++++++++++++++++++++++++++-----
>   gdb/testsuite/gdb.cp/userdef.cc  | 18 +++++++++++++
>   gdb/testsuite/gdb.cp/userdef.exp |  4 +++
>   3 files changed, 62 insertions(+), 6 deletions(-)
>
> diff --git a/gdb/eval.c b/gdb/eval.c
> index 6b752e70635..e737774ca28 100644
> --- a/gdb/eval.c
> +++ b/gdb/eval.c
> @@ -664,14 +664,34 @@ operation::evaluate_funcall (struct type *expect_type,
>   
>     value *callee = evaluate_with_coercion (exp, noside);
>     struct type *type = callee->type ();
> -  if (type->code () == TYPE_CODE_PTR)
> -    type = type->target_type ();
> -  for (int i = 0; i < args.size (); ++i)
> +
> +  /* If the callee is a struct, there might be a user-defined function call
> +     operator that should be used instead.  */
> +  if (overload_resolution
> +      && exp->language_defn->la_language == language_cplus
> +      && check_typedef (type)->code () == TYPE_CODE_STRUCT)
>       {
> -      if (i < type->num_fields ())
> -	vals[i] = args[i]->evaluate (type->field (i).type (), exp, noside);
> -      else
> +      for (int i = 0; i < args.size (); ++i)
>   	vals[i] = args[i]->evaluate_with_coercion (exp, noside);
> +
> +      vals.insert (vals.begin(), value_addr (callee));
> +      int static_memfuncp;
> +      find_overload_match (vals, "operator()", METHOD, &vals[0], nullptr,
> +			   &callee, nullptr, &static_memfuncp, 0, noside);
> +      if (static_memfuncp)
> +	vals.erase (vals.begin ());
> +    }
> +  else
> +    {
> +      if (type->code () == TYPE_CODE_PTR)
> +	type = type->target_type ();
> +      for (int i = 0; i < args.size (); ++i)
> +	{
> +	  if (i < type->num_fields ())
> +	    vals[i] = args[i]->evaluate (type->field (i).type (), exp, noside);
> +	  else
> +	    vals[i] = args[i]->evaluate_with_coercion (exp, noside);
> +	}

This change had me confused for quite a bit. I couldn't figure out why 
the base operation::evaluate_funcall. The problem seems to be that if 
command used is something less straightforward, like "print (foo+bar) 
(args)", we will use the evaluate_funcall of the operation (in this 
case, add_operation) instead of var_value_operation's. Did I understand 
it correctly?

Assuming I did, I don't think this code duplication is the best way to 
go. Especially seeing as there are some differences in those functions 
already, and if all that it takes to defeat out expression parser is use 
(*&foo) or (a + b) (), we probably already have latent bugs in this 
area. I don't know how to verify it, though, because I really don't 
understand the code differences.

Ideally, I think the best way would be to put all the code in 
var_value_operation::evaluate_funcall, but to do this, you'd need to be 
able to find a way to call the resulting var_value_operation's function 
from all relevant operations, which is probably a lot.

Another option is to just park all the logic in 
operation::evaluate_funcall, so we're always using the correct function. 
This comes with the cost of being very confusing in a couple of months 
to a year.

Does this make sense?

-- 
Cheers,
Guinevere Larsen
She/Her/Hers

>       }
>   
>     return evaluate_subexp_do_call (exp, noside, callee, vals,
> @@ -702,6 +722,20 @@ var_value_operation::evaluate_funcall (struct type *expect_type,
>     value *callee = evaluate_var_value (noside, std::get<0> (m_storage).block,
>   				      symp);
>   
> +  /* If the callee is a struct, there might be a user-defined function call
> +     operator that should be used instead.  */
> +  if (overload_resolution
> +      && exp->language_defn->la_language == language_cplus
> +      && check_typedef (callee->type ())->code () == TYPE_CODE_STRUCT)
> +    {
> +      argvec.insert (argvec.begin(), value_addr (callee));
> +      int static_memfuncp;
> +      find_overload_match (argvec, "operator()", METHOD, &argvec[0], nullptr,
> +			   &callee, nullptr, &static_memfuncp, 0, noside);
> +      if (static_memfuncp)
> +	argvec.erase (argvec.begin ());
> +    }
> +
>     return evaluate_subexp_do_call (exp, noside, callee, argvec,
>   				  nullptr, expect_type);
>   }
> diff --git a/gdb/testsuite/gdb.cp/userdef.cc b/gdb/testsuite/gdb.cp/userdef.cc
> index 774191726f3..48507551079 100644
> --- a/gdb/testsuite/gdb.cp/userdef.cc
> +++ b/gdb/testsuite/gdb.cp/userdef.cc
> @@ -68,6 +68,9 @@ A1 operator++(int);
>   A1 operator--();
>   A1 operator--(int);
>   
> +int operator()();
> +int operator()(int);
> +
>   };
>   
>   
> @@ -293,6 +296,16 @@ ostream& operator<<(ostream& outs, A1 one)
>    return (outs << endl << "x = " << one.x << endl << "y = " << one.y << endl << "-------" << endl);
>   }
>   
> +int A1::operator()()
> +{
> +  return x + y;
> +}
> +
> +int A1::operator()(int value)
> +{
> +  return value * (x + y);
> +}
> +
>   class A2 {
>     public:
>   A2 operator+();
> @@ -404,6 +417,11 @@ int main (void)
>    ++three;
>    cout << "preinc " << three;
>   
> + val = two();
> + cout << "funcall " << val << endl;
> + val = two(10);
> + cout << "funcall 2 " << val << endl;
> +
>    (*c).z = 1;
>   
>    return 0;
> diff --git a/gdb/testsuite/gdb.cp/userdef.exp b/gdb/testsuite/gdb.cp/userdef.exp
> index e96636bef0c..667bded6b83 100644
> --- a/gdb/testsuite/gdb.cp/userdef.exp
> +++ b/gdb/testsuite/gdb.cp/userdef.exp
> @@ -119,6 +119,10 @@ gdb_test "print one += 7" "\\\$\[0-9\]* = {x = 9, y = 10}"
>   
>   gdb_test "print two = one" "\\\$\[0-9\]* = {x = 9, y = 10}"
>   
> +gdb_test "print two()" " = 19"
> +gdb_test "print two(10)" " = 190"
> +gdb_test "print (*&two)(2)" " = 38"
> +
>   # Check that GDB tolerates whitespace in operator names.
>   gdb_test "break A2::operator+" ".*Breakpoint $decimal at.*"
>   gdb_test "break A2::operator +" ".*Breakpoint $decimal at.*"