Re: [PATCH 2/2] Add a way to preserve redefined GDB commands for later invocation

[Marco Barisione <mbarisione@undo.io>]

On 5 Oct 2020, at 11:24, Andrew Burgess <andrew.burgess@embecosm.com> wrote:
>> +/* Implementation of the "uplevel" command.  */
>> +
>> +static void
>> +uplevel_command (const char *arg, int from_tty)
>> +{
>> +  /* Extract the level argument and update arg to point to the remaining
>> +     string.  */
>> +  size_t level_end_offset;
>> +  int level;
>> +  try
>> +    {
>> +      level = std::stoi (arg, &level_end_offset);
>> +    }
>> +  catch (const std::exception &)
>> +    {
>> +      level = -1;
>> +    }
>> +  if (level < 0)
>> +    error (_("uplevel command requires a non-negative number for the "
>> +	     "level argument."));
> 
> I don't understand why the Python API allows for negative values while
> the CLI command does not.  In my mind, negative values are the only
> really useful way to use this feature as it should be considered bad
> practice to assume that there are not some unknown number of overrides
> above you in the command stack.

This is briefly explained in the first email
(https://sourceware.org/pipermail/gdb-patches/2020-September/171829.html).

I didn’t want to do too much for this patch series in case the whole approach
was wrong.
One of the things I didn’t do (for now) was this as I couldn't find an obvious
way for the "uplevel" command to know which command you are currently
executing.  An option could be to implement something similar way to how
command arguments are kept around with scoped_user_args_level, so we could
keep a stack of all (user and non-user) commands which are being executed.
By checking the latest one you can know what "uplevel -1" would apply to.

Another question I couldn’t answer is what "uplevel -1 …" would do if used
outside a definition.
Should it show an error or should it just invoke the latest defined command?
In the latter case, wouldn’t it confusing that "-1" has a different meaning
inside or outside commands?

Note that there’s another bit of useful work that I didn’t do, that is the
addition of a way of accessing all the untokenized arguments from a command
definition.

> I think that there is another useful check that should be added to
> this patch, I think we should prohibit calling a command further down
> the command stack.  That is we should make this invalid:
> 
>  define cmd_1
>    echo this is level 0 cmd_1\n
>    uplevel 1 cmd_1
>  end
> 
>  define cmd_1
>    echo this is level 1 cmd_1\n
>    uplevel 0 cmd_1
>  end
> 
> What I'd like to see is that when we execute 'uplevel 1 cmd_1' GDB
> throws an error about '... requested command at higher level
> than current command...' or similar.
> 
> The above will eventually error with 'Max user call depth exceeded --
> command aborted.', but I think explicitly catching this case would
> improve the user experience.

How about the case where a command on the same level is invoked?
I considered this (there should be a comment somewhere) but thought it’s
not a real problem as there may be some legitimate cases (for instance
if the arguments are changed) and, if things go wrong, you just get a
max user call depth exceeded.

The same weird but legitimate case may in theory exist also for
invoking a command at a higher level, i.e. the arguments are changed.

> I also wonder how we might handle _really_ redefining a command now
> that past commands hang around.
> 
> When writing the above cmd_1 example the first thing I did was open
> GDB and try to write the commands at the CLI.  I made a typo when
> writing the second version of cmd_1.  So now I'm stuck with a chain:
> 
>  working level 0 cmd_1  --> broken level 1 cmd_1
> 
> Sure I can write a level 2 version that knows to skip over the broken
> level 1, but it might be nice if there was a flag somewhere that I
> could pass to say, no, really replace the last version of this command
> please.

Is this really what a user would want? Or would they want the ability
to delete commands?

> Also, when I define cmd_1 (at the CLI) I get asked:
> 
>  Redefine command "cmd_1"? (y or n)
> 
> Maybe this could/should be changed to:
> 
>  "cmd_1" already exists, type 'o' to override, 'r' to replace, or 'q' to quit:
> 
> I'm thinking about the pager prompt which is more than just y/n.  I
> haven't looked how easy it would be to do something similar here.

I don’t think it would be too difficult but it means that the user must
decide what they want early and they can’t change their minds.
What happens if you accidentally replace a command but you wanted to
override it instead?

> One last thing I note is that this new functionality is very similar
> to the existing "hook-" and "hookpost-" functionality, though I think
> this new approach is much better.

My goal with my patches was to get rid of hooks in my own code.

> I think as a minimum the documentation for the old hook approach
> should be updated to indicate that that technique is deprecated, and
> this new approach should be used instead.

Good point.

> Where I really think we should be going is adding a patch #3 to this
> series that replaces the implementation of the old functionality using
> your new code.  Meaning, when someone writes:
> 
>  define hook-echo
>    echo before\n
>  end
> 
> GDB would internally change this to effectively be:
> 
>  define echo
>    echo before\n
>    uplevel -1 echo
>  end
> 
> this will work for many trivial cases.  There's a few nasty corners,
> like hooks for sub-commands (see the docs on hooking 'target remote'),
> the pseudo-command 'stop', and calling the same command from within a
> hook.

Hm, difficult to say how difficult it could be but I will have a look.

There may also be other subtle different behaviours we don’t know about
and which could break existing scripts. Is this acceptable?

-- 
Marco Barisione