Re: [PATCH v2] Add a way to preserve overridden GDB commands for later invocation

[Marco Barisione <mbarisione@undo.io>]

On 5 Nov 2019, at 10:17, Andrew Burgess <andrew.burgess@embecosm.com> wrote:
> * Marco Barisione <mbarisione@undo.io> [2019-11-01 21:01:18 +0000]:
>> Let's say you override delete. Let's call the first class overriding it
>> DeleteCommand1 and the second DeleteCommand2.
>> 
>> If DeleteCommand1 didn't use preserve_when_overridden then
>> DeleteCommand2.invoke_overridden will just call the original delete.
>> 
>> If DeleteCommand1 was preserved, then DeleteCommand2.invoke_overridden
>> will call DeleteCommand1.invoke.  At this point DeleteCommand1 may just do
>> whatever it needs on its own, or call its own invoke_overridden method
>> which will call the original delete command.
>> DeleteCommand2 has not way to invoke the original delete command directly.
>> This is partly because I'm not sure how I would expose this in a nice way,
>> but mainly because I don't want commands to accidentally skip some
>> previous implementation by accident.  For instance, if we had an
>> invoke_original method, then DeleteCommand2 could call that and skip
>> DeleteCommand1.
> 
> I too have sometimes wondered about providing override type
> behaviour.
> 
> I wonder, instead of having 'invoke_overridden' and/or
> 'invoke_original', if we could/should look to TCL for inspiration,
> specifically it's uplevel command[1].

Hm, I think this is potentially a very good idea.
I’m slightly worried about people accidentally (or on purpose but without
understanding the consequences) skipping a new command implementation,
but it’s probably not a big deal.

I would suggest the method to be uplevel_command(args, from_tty[, level])
so, if you don't specify the level, you just invoke the directly
overridden command.

> If we consider your 'delete' example, the builtin delete would be
> considered to be at level 0, then we define DeleteCommand1, this would
> be level 1, and then comes DeleteCommand2 which would be at level 2.
> 
> Inside DeleteCommand1 we can do:
> 
>  DeleteCommand1.invoke_uplevel (-1, args)
> 
> this would invoke the command one level down the stack, so in this
> case the builtin delete.  In DeleteCommand2 we can do:

I would say this is level 1, not level -1.  This would match TCL, make
more sense with the name (IMHO) of the method as it says "up”, and make
the common case use positive numbers, not negative ones.

Does it make sense to have a level of 0?  Why not just calling the invoke
method directly?

How about negative numbers?  Why would you need it?  It seems it's risky
as you could easily create cycles.
It would also mean that an overridden command would need to know that it
was overridden and that the overriding command if not going to blindly
call back the overridden command.

Because of this, I *think* it would make more sense (at least in the first
version of this patch until we find a use case) to just have level > 0 (or
>= 0).

> Where I would find this idea really interesting is that we could
> easily provide access to this functionality from the command line.
> This might be useful in two ways I think, first, if a Python script
> has overridden a command and a user wants to quickly bypass that for
> some reason they can just do:
> 
>  (gdb) uplevel 0 delete args…

I love this!
Why 0?  Shouldn’t this be -1 following your example above?

Like I said above I think this would be simpler if level were > 0 and
optional:
    uplevel delete 42
Or:
    uplevel 1 delete 42

> in order to get the base version of the command.  Second, with some
> extra work we could make the existing hook mechanism redundant, and
> possibly provide something more flexible (maybe?), consider:
> 
>  define delete
>    echo Basically a pre-hook here\n
>    uplevel -1 delete $argv
>    echo Basically a post-hook here\n
>  end

Being able to avoid hooks is one of the reasons for this patch.
Currently, in my code, I have a lot of hooks which just call into Python
but they are ugly (GDB Script code to just call into Python) and they
cannot prevent the command from being executed without a Python stacktrace
being printed (ont very nice for users).

I'd be happy to provide uplevel as part of this patch.

> Notice I just invented '$argv' on the fly there - this doesn't exist
> currently, but I'm imagining that this would expand to all of the
> arguments passed to the newly defined 'delete' command.
> 
> Anyway, I don't think all of the ideas mentioned above would need to
> be implemented in one go, but I thought it might be worth mentioning
> some of the thoughts I'd had in this area.

I’d skip the $argv thing for this patch but I think it makes sense.

How would the from_tty argument for C or Python command be defined?

-- 
Marco Barisione