Re: [PATCH 0/5] create GDB/MI commands using python

[Jan Vrany <jan.vrany@labware.com>]

On Fri, 2022-01-21 at 15:22 +0000, Andrew Burgess wrote:
> * Jan Vrany via Gdb-patches <gdb-patches@sourceware.org> [2022-01-18 15:13:01 +0000]:
> 
> > On Tue, 2022-01-18 at 13:55 +0000, Andrew Burgess wrote:
> > > * Jan Vrany via Gdb-patches <gdb-patches@sourceware.org> [2022-01-17 12:44:20 +0000]:
> > > 
> > > > This is a restart of an earlier attempts to allow custom
> > > > GDB/MI commands written in Python.
> > > 
> > > Thanks for continuing to work on this feature.
> > > 
> > > I too had been looking at getting the remaining patches from your
> > > series upstream, and I'd like to discuss some of the differences
> > > between the approaches we took.
> > 
> > Great, thanks a lot!
> > 
> > > 
> > > At the end of this mail you'll find my current work-in-progress
> > > patch, it definitely needs the docs and NEWS entries adding, as well
> > > as a few extra tests.  However, functionality wise I think its mostly
> > > there.
> > > 
> > > My patch includes two sets of tests, gdb.python/py-mi-cmd.exp and
> > > gdb.python/py-mi-cmd-orig.exp.  The former is based on your tests, but
> > > with some tweaks based on changes I made.  The latter set is your
> > > tests taken from this m/l thread.
> > > 
> > > When running the gdb.python/py-mi-cmd-orig.exp tests there should be
> > > just 2 failures, both related to the same feature which is not present
> > > in my version, that is, the ability of a command to redefine itself,
> > > like this:
> > > 
> > >       class my_command(gdb.MICommand):
> > >         def invoke(self, args):
> > >           my_command("-blah")
> > >           return None
> > > 
> > >        my_command("-blah")
> > > 
> > > this works with your version, but not with mine, this is because I'm
> > > using python's own reference counting to track when a command can be
> > > redefined or not, and, when you are within a commands invoke method
> > > the reference count of the containing object is incremented, and this
> > > prevents gdb from deleting the command.
> > > 
> > > My question then, is how important is this feature, and what use case
> > > do you see for this?  Or, was support for this just a happy side
> > > effect of the implementation approach you chose?
> > 
> > Initially I did not think of it and it was - IIRC - pointed out in
> > some review. I thought to be a corner case, but it turned out to be
> > (potentiality very useful feature. 
> > 
> > While developing custom commands, pretty printers and so on, it is useful
> > to be able to "reload" Python code into running GDB without need to restart 
> > it. To support that, I created a "pr" command which reloads all custom python 
> > code (well, tries to at least), see 
> > 
> > https://urldefense.proofpoint.com/v2/url?u=https-3A__swing.fit.cvut.cz_hg_jv-2Dvdb_file_tip_python_vdb_cli.py-23l20&d=DwIFAw&c=sPZ6DeHLiehUHQWKIrsNwWp3t7snrE-az24ztT0w7Jc&r=WpFFGgYa98Yp-c29WHTCwU1wAGFBvszA6a4RzgpMSqc&m=aunQh9rOF7oPv3b6WEJbNxADRSv4-8Dy2BMsx38ToFU&s=jnwDUKw3EN3ruyHZoFfwtAJykINwXr92bHBvZHlvn1k&e= 
> > 
> > So ultimately, this custom "pr" command's invoke() causes redefinition
> > of the "pr" command itself. I have not done it yet, but having 
> > menu item/icon in (my) GDB frontend using something like -python-reload`
> > seems desirable from UX POV.
> 
> Thanks, that's not an unreasonable use case.  I've tweaked things so
> that this case is now supported.
> 
> I've gone through that patch and ported over your NEWS and doc
> changes, with some updates based on Eli's feedback, as well as some
> new content to cover some of the changes in my patch.
> 
> For the testing, I have, for now, kept gdb.python/py-mi-cmd-orig.exp,
> which is your test file taken from your patch series.  I added some
> 'setup_kfail' markers to 7 tests that now fail due to changes in error
> strings, hopefully, this will allow you to review what changed.
> However, except for the differences in error string, everything your
> original series supported is now supported by this patch.
> 
> From a user's point of view, I think the only differences with your
> patch are:
> 
>  1. New .name attribute, that returns the name of the command as a
>     string, here's an example session:
> 
>     (gdb) python
>     >class MyCommand(gdb.MICommand):
>     >  def __init__(self):
>     >    super(MyCommand, self).__init__("-my-command")
>     >  def invoke(self, args):
>     >    return None
>     >
>     >end
>     (gdb) python cmd = MyCommand()
>     (gdb) python print(cmd.name)
>     -my-command
>     (gdb)
> 
>  2. New .installed attribute, that allows a command to be installed or
>     removed from the mi command table, here's an example session:
> 
>     (gdb) python
>     >class MyCommand(gdb.MICommand):
>     >  def __init__(self, name, message):
>     >    self._message = message
>     >    super(MyCommand, self).__init__(name)
>     >
>     >  def invoke(self, args):
>     >    return self._message
>     >
>     >end
>     (gdb) python cmd1 = MyCommand("-my-command", "cmd1")
>     (gdb) python print(cmd1.installed)
>     True
>     (gdb) interpreter-exec mi "-my-command"
>     ^done,result="cmd1"
>     (gdb) python cmd2 = MyCommand("-my-command", "cmd2")
>     (gdb) python print(cmd2.installed)
>     True
>     (gdb) python print(cmd1.installed)
>     False
>     (gdb) interpreter-exec mi "-my-command"
>     ^done,result="cmd2"
>     (gdb) python cmd1.installed = True
>     (gdb) python print(cmd2.installed)
>     False
>     (gdb) python print(cmd1.installed)
>     True
>     (gdb) interpreter-exec mi "-my-command"
>     ^done,result="cmd1"
>     (gdb) python cmd1.installed = False
>     (gdb) interpreter-exec mi "-my-command"
>     ^error,msg="Undefined MI command: my-command",code="undefined-command"
>     (gdb)
> 
>  3. The top level result name can be changed from 'result' to anything
>     the user wants, here's an example session:
> 
>     (gdb) python
>     >class MyCommand(gdb.MICommand):
>     >  def __init__(self):
>     >    super(MyCommand, self).__init__("-my-command", "greeting")
>     >  def invoke(self, args):
>     >    return "Hello World"
>     >
>     >end
>     (gdb) python cmd = MyCommand()
>     (gdb) interpreter-exec mi "-my-command"
>     ^done,greeting="Hello World"
>     (gdb)
> 
> I'd love to hear your feedback on this iteration.

I'm personally happy with this iteration. I like your improvements 
to the Python API! I also tested it briefly with my Python code & 
frontend just to see and it worked just fine. 

Perhaps, I'd rename mi_commands module variable to _mi_commands to 
convey the fact it is "internal" to the implementation and should not
be touched (?).

Thanks!

Jan