[PATCH v14 3/4] Add support for lookup, overload resolution and invocation of C++ debug methods

[Doug Evans]

On Tue, Apr 22, 2014 at 1:46 PM, Siva Chandra <sivachandra@google.com> wrote:
> On Mon, Apr 21, 2014 at 4:56 PM, Doug Evans <dje@google.com> wrote:
>> Another issue I have is this bit of code in, e.g., value_x_binop:
>>
>>   value_user_defined_op (&arg1, argvec + 1, tstr, &static_memfuncp, 2,
>>                          &argvec[0], &dm_worker);
>>
>>   if (argvec[0])
>>     {
>>       if (static_memfuncp)
>>         {
>>           argvec[1] = argvec[0];
>>           argvec++;
>>         }
>>       if (noside == EVAL_AVOID_SIDE_EFFECTS)
>>         {
>>           struct type *return_type;
>>
>>           return_type
>>             = TYPE_TARGET_TYPE (check_typedef (value_type (argvec[0])));
>>           return value_zero (return_type, VALUE_LVAL (arg1));
>>         }
>>       return call_function_by_hand (argvec[0], 2 - static_memfuncp,
>>                                     argvec + 1);
>>     }
>>   if (dm_worker != NULL)
>>     {
>>       struct cleanup *dm_worker_cleanup = make_cleanup (xfree, dm_worker);
>>       struct value *ret_val = invoke_debug_method (dm_worker, arg1, &arg2, 1);
>>
>>       do_cleanups (dm_worker_cleanup);
>>       return ret_val;
>>     }
>>
>> Does this mean we call the debug method (assuming it wins) for the
>> case of noside == EVAL_AVOID_SIDE_EFFECTS?
>
> To be frank, I never understood why this option exists at all. If side
> effects (like even reading target memory) are not desired, why
> evaluate expressions at all? And, fwiw, there is a piece of code in
> find_oload_match which looks up the most derived type of an object
> which involves reading the vtable much before all this.

A good question we need to answer is: What happens if a user does
"ptype" on an expression involving a debug method?
[ptype uses evaluate_type which is an example of using EVAL_AVOID_SIDE_EFFECTS]
A user should be able to do that and have confidence that inferior
state will not be modified.
Reading the vtable is ok, but if the debug method is implementing,
say, some kind of iterator then modifying inferior state would not be
ok.

I tried "ptype a1 + a2" from your py-debugmethods.exp testcase, and
the debug method does get invoked.
Is that ok?  In many cases probably, but it doesn't seem likely to be
ok in the general case.

How to achieve that is another good question.
[Ideally we wouldn't impose EVAL_AVOID_SIDE_EFFECTS on debug method writers.]
I have been assuming that debug methods could fit in with the existing
logic and the solution would just fall out.
That needs to be verified of course. :-)

>> Here and elsewhere there's logic that the debug method code is not
>> being included in, and it makes me want to do things differently.
>
> Can you point me to an example different from EVAL_AVOID_SIDE_EFFECTS?

I was thinking of the invoke_debug_method call in evaluate_subexp_standard.
There's a lot of code for case OP_FUNCALL, and the call to
call_function_by_hand appears at the very bottom of the case.
IWBN if the actual call to the debug method was done there too.

>> Fortunately, I think (though I haven't implemented it) it won't be hard.
>> Just like we have TYPE_CODE_INTERNAL_FUNCTION we could also have
>> TYPE_CODE_EXTERNAL_FUNCTION, and use that to make debug methods less
>> special case.
>> clone_debug_method_worker could return a struct value that is a
>> TYPE_CODE_EXTERNAL_FUNCTION, and then just before the call to
>> call_function_by_hand that would happen for normal methods,
>> we'd have a check for TYPE_CODE_EXTERNAL_FUNCTION and call
>> call_debug_method instead.
>
> I am putting down my notes and questions.
>
> 1. I think TYPE_CODE_INTERNAL_FUNCTIONs can also be implemented in
> extension languages. So, what makes them internal and different from
> TYPE_CODE_EXTERNAL_FUNCTION? I have tried to convince myself that the
> name "INTERNAL" exists because of historical reasons and does not mean
> anything today. Hence, is TYPE_CODE_EXTERNAL_FUNCTION a good name? Am
> I missing something?

The names are a bit confusing alright.
I left TYPE_CODE_INTERNAL_FUNCTION alone because it is used for $foo(bar).
Convenience functions have their own syntax.
Maybe we *could* morph TYPE_CODE_INTERNAL_FUNCTION into something more
general purpose, I'm not sure.

> 2.If debug methods are to be represented by a value (say, of type
> TYPE_CODE_DEBUG_METHOD), then a new value should be created for every
> debug method invocation. I do not know if this is good or bad, and I
> do not know if this happens for internal functions as well. A
> fundamental difference (atleast in my mind) is that internal functions
> have a name by definition (at least currently) and are invoked by that
> name. Debug methods do not have a name by definition that they are
> invoked with. In fact, debug methods have different invocation names
> based on the context (ie. template arguments).

Yeah.  Though once we have support for static methods will they still
be "debug methods"?
I'd like to avoid pinning down the term "debug methods" to a specific
use-case (c++ non-static methods).
[assuming that is the name we're sticking with for this feature]

> 3. Extending debug methods to functions (if that is what you meant by
> making debug methods less special case); I think this would lead to a
> lot of confusion between convenience functions, debug methods and
> "debug functions". Best would be to support only two mutually
> exclusive features "Debug Methods" and "Convenience Functions" with
> latter extended to have the ability to replace existing source
> functions. This is my personal opinion which I am ready to change if
> you prescribe a specific different way.

It's mostly just a matter of naming, but the term "Convenience
Functions" already has a specific connotation that implies a leading $
in the name.
Extending convenience functions to foo() in addition to $foo() could
also inflict confusion.
It would be odd, I think, to the user to have non-static methods be
"debug methods" and static methods be "convenience functions".
I don't have a strong opinion though.
IWBN to hear from others in the community but we may be on our own on this one.

btw, "making debug methods less special case" was more aimed at the
implementation (IOW treating them internally no different than other
functions: TYPE_CODE_FUNC and TYPE_CODE_INTERNAL_FUNCTION).

> 4. What benefit, apart from lesser number of arguments to
> find_oload_match, do you see? I guess this is related to the above
> question on what else is the debug methods logic currently missing.

To the person reading gdb code, debug methods are conceptually no
different than TYPE_CODE_INTERNAL_FUNCTION when it comes to actually
invoking them.  Yeah, they're looked up differently, but once they're
looked up they're just some "external" code that gdb will invoke to
evaluate an expression the user entered.  Thus my mental model of how
I expect to see debug methods implemented in the code should match to
the extent possible and reasonable how I see
TYPE_CODE_INTERNAL_FUNCTION implemented.  Where it doesn't it means
more mental effort has to go in to understand and maintain them, and
more state to have to keep in cache while working with the code.