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

[Andrew Burgess <andrew.burgess@embecosm.com>]

* Marco Barisione <mbarisione@undo.io> [2020-10-05 12:44:03 +0100]:

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

I think this would be a fine approach.  I'd pack the command pointer
and arguments into a single object and store these in a newly renamed
scoped_user_args_level I think.

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

Yeah I don't think the last case would be a good way to go, but what
about executing the last but one version of the command, this would
seem exactly inline with the behaviour inside a command, so:

  (gdb) define cmd_1
  echo v1\n
  end
  (gdb) define cmd_1
  echo v2\n
  uplevel -1 cmd_1
  end
  (gdb) cmd_1
  v2
  v1
  (gdb) uplevel 0 cmd_1
  v1
  (gdb) uplevel -1 cmd
  v1

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

But at the same level can't you just call the command by name with no
'uplevel' at all?

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

Yeah... no.

I'm sure it's possible to craft an example where this might work, but
I think we should start off by blocking this, then if someone comes up
with a really good example of why this is needed, and is the _only_
way to solve there problem then we can rip the check out.

In general I think you could only call to a higher level if you had
full control over how the commands are written and the order they are
registered, in which case (I boldly claim) there would be a better way
to rewrite the code to avoid calling to a higher level.

IF you don't have full control over what was registered later, or the
order in which parts are registered then this would never work.

In short I think blocking this will help far more than it hurts.

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

Deleting too might be useful, but I think the case I gave is not
unique to me, I tried writing a command, I got it slightly wrong.
Sure I can delete and start again, but being able to just do:

  (gdb) define --redefine cmd_1
  ...

would be nice.

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

Well, GDB offers a programmable environment, we don't allow undoing
most (or hardly any) actions, so I think users are OK with the idea
that some action might not be reversible.

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

Nothing is completely risk free, and I don't know how well tested the
hooks support is, but it would be nice if we could fully replace hooks
with your code (which is a much better approach).

Thanks,
Andrew

> 
> -- 
> Marco Barisione
>