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

[Andrew Burgess <aburgess@redhat.com>]

Jan Vrany via Gdb-patches <gdb-patches@sourceware.org> writes:

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

That's great news.

>
> 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, using the '_' prefix was mentioned for another patch I was
working on; I'll definitely make this change.

I'll give this a little more time, hopefully we'll get some more reviews
in.

Thanks,
Andrew