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

[Hannes Domani <ssbssa@yahoo.de>]

 Am Donnerstag, 25. April 2024 um 20:34:19 MESZ hat Guinevere Larsen <blarsen@redhat.com> Folgendes geschrieben:

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

Yes, it does.
And it turns out my patch already didn't work if you e.g. try to use the
operator() on struct members.

Not sure why I didn't see this before, but all evaluate_funcall variations
call evaluate_subexp_do_call at the end, so I just moved the logic there
in this v2: https://sourceware.org/pipermail/gdb-patches/2024-April/208663.html


Hannes