[v2] Allow calling of user-defined function call operators

  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
```

The change in operation::evaluate_funcall is to make sure the type
fields are only used for function types, only they use them as the
argument types.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=12213
---
v2:
- Move the logic into evaluate_subexp_do_call, to avoid duplication in
  every evaluate_funcall of each operation subclass.
  This makes it now work for some cases it didn't in v1, like if it's
  called on a class member (`print c.m(5)` in the new test).
- Added tests for other (struct member) operations.
---
 gdb/eval.c                       | 29 ++++++++++++++++++++++++++---
 gdb/testsuite/gdb.cp/userdef.cc  | 22 ++++++++++++++++++++++
 gdb/testsuite/gdb.cp/userdef.exp |  7 +++++++
 3 files changed, 55 insertions(+), 3 deletions(-)
  

On 4/27/24 13:36, 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
> ```
>
> The change in operation::evaluate_funcall is to make sure the type
> fields are only used for function types, only they use them as the
> argument types.
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=12213
> ---
> v2:
> - Move the logic into evaluate_subexp_do_call, to avoid duplication in
>    every evaluate_funcall of each operation subclass.
>    This makes it now work for some cases it didn't in v1, like if it's
>    called on a class member (`print c.m(5)` in the new test).
> - Added tests for other (struct member) operations.
> ---
>   gdb/eval.c                       | 29 ++++++++++++++++++++++++++---
>   gdb/testsuite/gdb.cp/userdef.cc  | 22 ++++++++++++++++++++++
>   gdb/testsuite/gdb.cp/userdef.exp |  7 +++++++
>   3 files changed, 55 insertions(+), 3 deletions(-)
>
> diff --git a/gdb/eval.c b/gdb/eval.c
> index 6b752e70635..8d5c354f480 100644
> --- a/gdb/eval.c
> +++ b/gdb/eval.c
> @@ -588,14 +588,35 @@ evaluate_subexp_do_call (expression *exp, enum noside noside,
>   {
>     if (callee == NULL)
>       error (_("Cannot evaluate function -- may be inlined"));
> +
> +  type *ftype = callee->type ();
> +
> +  /* If the callee is a struct, there might be a user-defined function call
> +     operator that should be used instead.  */
> +  std::vector<value *> vals;
> +  if (overload_resolution
> +      && exp->language_defn->la_language == language_cplus
> +      && check_typedef (ftype)->code () == TYPE_CODE_STRUCT)
> +    {
> +      vals.resize (argvec.size () + 1);
> +
> +      vals[0] = value_addr (callee);
> +      for (int i = 0; i < argvec.size (); ++i)
> +	vals[i + 1] = argvec[i];
> +
> +      int static_memfuncp;
> +      find_overload_match (vals, "operator()", METHOD, &vals[0], nullptr,
> +			   &callee, nullptr, &static_memfuncp, 0, noside);
> +      if (!static_memfuncp)
> +	argvec = vals;

I don't really understand this change. From what I can see in this 
patch, you're just shifting all values in argvec one to the right, to 
add the callee address. Wouldn't this necessitate a change in the logic 
for the rest of the function?

> +    }
> +
>     if (noside == EVAL_AVOID_SIDE_EFFECTS)
>       {
>         /* If the return type doesn't look like a function type,
>   	 call an error.  This can happen if somebody tries to turn
>   	 a variable into a function call.  */
>   
> -      type *ftype = callee->type ();
> -
>         if (ftype->code () == TYPE_CODE_INTERNAL_FUNCTION)
>   	{
>   	  /* We don't know anything about what the internal
> @@ -666,9 +687,11 @@ operation::evaluate_funcall (struct type *expect_type,
>     struct type *type = callee->type ();
>     if (type->code () == TYPE_CODE_PTR)
>       type = type->target_type ();
> +  bool type_has_arguments
> +    = type->code () == TYPE_CODE_FUNC || type->code () == TYPE_CODE_METHOD;
>     for (int i = 0; i < args.size (); ++i)
>       {
> -      if (i < type->num_fields ())
> +      if (type_has_arguments && i < type->num_fields ())
This change also looks to be unrelated to this patch?
  

Am Freitag, 3. Mai 2024 um 20:29:47 MESZ hat Guinevere Larsen <blarsen@redhat.com> Folgendes geschrieben:

> On 4/27/24 13:36, 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
> > ```
> >
> > The change in operation::evaluate_funcall is to make sure the type
> > fields are only used for function types, only they use them as the
> > argument types.
> >
> > Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=12213
> > ---
> > v2:
> > - Move the logic into evaluate_subexp_do_call, to avoid duplication in
> >    every evaluate_funcall of each operation subclass.
> >    This makes it now work for some cases it didn't in v1, like if it's
> >    called on a class member (`print c.m(5)` in the new test).
> > - Added tests for other (struct member) operations.
> > ---
> >  gdb/eval.c                      | 29 ++++++++++++++++++++++++++---
> >  gdb/testsuite/gdb.cp/userdef.cc  | 22 ++++++++++++++++++++++
> >  gdb/testsuite/gdb.cp/userdef.exp |  7 +++++++
> >  3 files changed, 55 insertions(+), 3 deletions(-)
> >
> > diff --git a/gdb/eval.c b/gdb/eval.c
> > index 6b752e70635..8d5c354f480 100644
> > --- a/gdb/eval.c
> > +++ b/gdb/eval.c
> > @@ -588,14 +588,35 @@ evaluate_subexp_do_call (expression *exp, enum noside noside,
> >  {
> >    if (callee == NULL)
> >      error (_("Cannot evaluate function -- may be inlined"));
> > +
> > +  type *ftype = callee->type ();
> > +
> > +  /* If the callee is a struct, there might be a user-defined function call
> > +    operator that should be used instead.  */
> > +  std::vector<value *> vals;
> > +  if (overload_resolution
> > +      && exp->language_defn->la_language == language_cplus
> > +      && check_typedef (ftype)->code () == TYPE_CODE_STRUCT)
> > +    {
> > +      vals.resize (argvec.size () + 1);
> > +
> > +      vals[0] = value_addr (callee);
> > +      for (int i = 0; i < argvec.size (); ++i)
> > +    vals[i + 1] = argvec[i];
> > +
> > +      int static_memfuncp;
> > +      find_overload_match (vals, "operator()", METHOD, &vals[0], nullptr,
> > +              &callee, nullptr, &static_memfuncp, 0, noside);
> > +      if (!static_memfuncp)
> > +    argvec = vals;
>
> I don't really understand this change. From what I can see in this
> patch, you're just shifting all values in argvec one to the right, to
> add the callee address. Wouldn't this necessitate a change in the logic
> for the rest of the function?

Yes, this adds the 'this' pointer as the first argument for operator().
I'm not sure why this would change the logic for the rest of the function.


> > +    }
> > +
> >    if (noside == EVAL_AVOID_SIDE_EFFECTS)
> >      {
> >        /* If the return type doesn't look like a function type,
> >      call an error.  This can happen if somebody tries to turn
> >      a variable into a function call.  */
> >
> > -      type *ftype = callee->type ();
> > -
> >        if (ftype->code () == TYPE_CODE_INTERNAL_FUNCTION)
> >      {
> >        /* We don't know anything about what the internal
> > @@ -666,9 +687,11 @@ operation::evaluate_funcall (struct type *expect_type,
> >    struct type *type = callee->type ();
> >    if (type->code () == TYPE_CODE_PTR)
> >      type = type->target_type ();
> > +  bool type_has_arguments
> > +    = type->code () == TYPE_CODE_FUNC || type->code () == TYPE_CODE_METHOD;
> >    for (int i = 0; i < args.size (); ++i)
> >      {
> > -      if (i < type->num_fields ())
> > +      if (type_has_arguments && i < type->num_fields ())
> This change also looks to be unrelated to this patch?

It's what I described here:

> The change in operation::evaluate_funcall is to make sure the type
> fields are only used for function types, only they use them as the
> argument types.
 
Before this patch it didn't matter if it used the field types in the
evaluate calls, but since the caller type could now also be a struct,
these would be the struct fields types, not function arguments.


Hannes
  

>>>>> "Hannes" == Hannes Domani <ssbssa@yahoo.de> writes:

Hannes> The change in operation::evaluate_funcall is to make sure the type
Hannes> fields are only used for function types, only they use them as the
Hannes> argument types.

IIRC the evaluation operations are all kind of complicated and
hairy... but it seems to me that the type of the chosen overload of
operator() would supply the type here?

Hannes> +  type *ftype = callee->type ();
Hannes> +
Hannes> +  /* If the callee is a struct, there might be a user-defined function call
Hannes> +     operator that should be used instead.  */
Hannes> +  std::vector<value *> vals;
Hannes> +  if (overload_resolution
Hannes> +      && exp->language_defn->la_language == language_cplus
Hannes> +      && check_typedef (ftype)->code () == TYPE_CODE_STRUCT)
Hannes> +    {

One question to consider is whether this should be done in the
expression node or elsewhere.  Other operator overloads are handled in
the value API instead.

I'm not sure which is better.  It would probably be cleaner to do it in
the expression nodes, like you've done.  Actually the best would
probably be to make a new operation subclass and avoid the need for a
language check.

However, the value API is convenient to use -- for example, this is what
makes operator overloading work in the Python API.

You can see the distinction with this patch by trying to call a
struct-with-operator() object from Python.

Tom
  

Am Freitag, 3. Mai 2024 um 22:06:46 MESZ hat Tom Tromey <tom@tromey.com> Folgendes geschrieben:

> >>>>> "Hannes" == Hannes Domani <ssbssa@yahoo.de> writes:
>
> Hannes> The change in operation::evaluate_funcall is to make sure the type
> Hannes> fields are only used for function types, only they use them as the
> Hannes> argument types.
>
> IIRC the evaluation operations are all kind of complicated and
> hairy... but it seems to me that the type of the chosen overload of
> operator() would supply the type here?

Here the overload of operator() is chosen based on the argument values,
not the other way round.


> Hannes> +  type *ftype = callee->type ();
> Hannes> +
> Hannes> +  /* If the callee is a struct, there might be a user-defined function call
> Hannes> +    operator that should be used instead.  */
> Hannes> +  std::vector<value *> vals;
> Hannes> +  if (overload_resolution
> Hannes> +      && exp->language_defn->la_language == language_cplus
> Hannes> +      && check_typedef (ftype)->code () == TYPE_CODE_STRUCT)
> Hannes> +    {
>
> One question to consider is whether this should be done in the
> expression node or elsewhere.  Other operator overloads are handled in
> the value API instead.
>
> I'm not sure which is better.  It would probably be cleaner to do it in
> the expression nodes, like you've done.  Actually the best would
> probably be to make a new operation subclass and avoid the need for a
> language check.

I've now moved the logic into evaluate_subexp_do_call so all operation
subclasses would profit from this, so I don't understand how a new
operation subclass would work for this.


> However, the value API is convenient to use -- for example, this is what
> makes operator overloading work in the Python API.
>
> You can see the distinction with this patch by trying to call a
> struct-with-operator() object from Python.

Calling a struct-with-operator() object from Python does not work, because
valpy_call directly calls call_function_by_hand.
It would maybe be possible to also call evaluate_subexp_do_call there.


Hannes
  

On 5/3/24 15:51, Hannes Domani wrote:
>   Am Freitag, 3. Mai 2024 um 20:29:47 MESZ hat Guinevere Larsen <blarsen@redhat.com> Folgendes geschrieben:
>
>> On 4/27/24 13:36, 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
>>> ```
>>>
>>> The change in operation::evaluate_funcall is to make sure the type
>>> fields are only used for function types, only they use them as the
>>> argument types.
>>>
>>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=12213
>>> ---
>>> v2:
>>> - Move the logic into evaluate_subexp_do_call, to avoid duplication in
>>>      every evaluate_funcall of each operation subclass.
>>>      This makes it now work for some cases it didn't in v1, like if it's
>>>      called on a class member (`print c.m(5)` in the new test).
>>> - Added tests for other (struct member) operations.
>>> ---
>>>    gdb/eval.c                      | 29 ++++++++++++++++++++++++++---
>>>    gdb/testsuite/gdb.cp/userdef.cc  | 22 ++++++++++++++++++++++
>>>    gdb/testsuite/gdb.cp/userdef.exp |  7 +++++++
>>>    3 files changed, 55 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/gdb/eval.c b/gdb/eval.c
>>> index 6b752e70635..8d5c354f480 100644
>>> --- a/gdb/eval.c
>>> +++ b/gdb/eval.c
>>> @@ -588,14 +588,35 @@ evaluate_subexp_do_call (expression *exp, enum noside noside,
>>>    {
>>>      if (callee == NULL)
>>>        error (_("Cannot evaluate function -- may be inlined"));
>>> +
>>> +  type *ftype = callee->type ();
>>> +
>>> +  /* If the callee is a struct, there might be a user-defined function call
>>> +    operator that should be used instead.  */
>>> +  std::vector<value *> vals;
>>> +  if (overload_resolution
>>> +      && exp->language_defn->la_language == language_cplus
>>> +      && check_typedef (ftype)->code () == TYPE_CODE_STRUCT)
>>> +    {
>>> +      vals.resize (argvec.size () + 1);
>>> +
>>> +      vals[0] = value_addr (callee);
>>> +      for (int i = 0; i < argvec.size (); ++i)
>>> +    vals[i + 1] = argvec[i];
>>> +
>>> +      int static_memfuncp;
>>> +      find_overload_match (vals, "operator()", METHOD, &vals[0], nullptr,
>>> +              &callee, nullptr, &static_memfuncp, 0, noside);
>>> +      if (!static_memfuncp)
>>> +    argvec = vals;
>> I don't really understand this change. From what I can see in this
>> patch, you're just shifting all values in argvec one to the right, to
>> add the callee address. Wouldn't this necessitate a change in the logic
>> for the rest of the function?
> Yes, this adds the 'this' pointer as the first argument for operator().
Ah, thanks for explaining! I think a comment would be pretty helpful in 
this situation.
> I'm not sure why this would change the logic for the rest of the function.

My thinking is that, on some situations for the rest of the function, 
there may be one too many arguments. To give a concrete example, I 
thought that if you had foo.bar(), where bar is a regular method, this 
could be adding the "this" argument one too many times. But that would 
mean "callee" is 'foo', and re-reading with fresh eyes, callee is 
supposed to be "bar", right?

>
>
>>> +    }
>>> +
>>>      if (noside == EVAL_AVOID_SIDE_EFFECTS)
>>>        {
>>>          /* If the return type doesn't look like a function type,
>>>        call an error.  This can happen if somebody tries to turn
>>>        a variable into a function call.  */
>>>
>>> -      type *ftype = callee->type ();
>>> -
>>>          if (ftype->code () == TYPE_CODE_INTERNAL_FUNCTION)
>>>        {
>>>          /* We don't know anything about what the internal
>>> @@ -666,9 +687,11 @@ operation::evaluate_funcall (struct type *expect_type,
>>>      struct type *type = callee->type ();
>>>      if (type->code () == TYPE_CODE_PTR)
>>>        type = type->target_type ();
>>> +  bool type_has_arguments
>>> +    = type->code () == TYPE_CODE_FUNC || type->code () == TYPE_CODE_METHOD;
>>>      for (int i = 0; i < args.size (); ++i)
>>>        {
>>> -      if (i < type->num_fields ())
>>> +      if (type_has_arguments && i < type->num_fields ())
>> This change also looks to be unrelated to this patch?
> It's what I described here:
>
>> The change in operation::evaluate_funcall is to make sure the type
>> fields are only used for function types, only they use them as the
>> argument types.
>   
> Before this patch it didn't matter if it used the field types in the
> evaluate calls, but since the caller type could now also be a struct,
> these would be the struct fields types, not function arguments.

Ooohh, I see.

Again, I think a code comment would help a lot. Something above the if, 
saying for example:

     "If type is a struct, num_fields would refer to the number of 
members in the type, not the number of arguments"

or similar.
  

>> IIRC the evaluation operations are all kind of complicated and
>> hairy... but it seems to me that the type of the chosen overload of
>> operator() would supply the type here?

Hannes> Here the overload of operator() is chosen based on the argument values,
Hannes> not the other way round.

Ok, I looked a little more and I see other paths pretty much doing the
same thing.  I don't really understand how this works if, say, a call
like this requires a pointer-adjusting cast to be done --
evaluate_with_coercion won't do this properly.  However, if there's a
bug here, it's probably reproducible some other way already.

>> However, the value API is convenient to use -- for example, this is what
>> makes operator overloading work in the Python API.
>> 
>> You can see the distinction with this patch by trying to call a
>> struct-with-operator() object from Python.

Hannes> Calling a struct-with-operator() object from Python does not work, because
Hannes> valpy_call directly calls call_function_by_hand.
Hannes> It would maybe be possible to also call evaluate_subexp_do_call there.

We'll probably just need a new value API if/when the time comes.

Tom