[RFC,8/8] mi/python: Allow redefinition of python MI commands

Message ID 20190418152337.6376-9-jan.vrany@fit.cvut.cz
State New
Headers show
Series
  • Create MI commands using python
Related show

Commit Message

Jan Vrany April 18, 2019, 3:23 p.m.
Redefining python MI commands is especially useful when developing
then.

gdb/Changelog:

	* mi/mi-cmds.h (mi_command::is_py_command): New method.
	(mi_command_py::is_py_command) New method.
	* gdb/mi/mi-cmds.c: (mi_command::is_py_command): New method.
	(mi_command_py::is_py_command) New method.
	(insert_mi_cmd_entry): Allow redefinition of python-defined MI commands.

gdb/testsuite/Changelog:

	* gdb.python/py-mi-cmd.exp: Add tests for python-defined MI command
	redefinition.
	* gdb.python/py-mi-cmd-2.py: New file.
---
 gdb/ChangeLog                           |  8 ++++++++
 gdb/mi/mi-cmds.c                        | 17 ++++++++++++++++-
 gdb/mi/mi-cmds.h                        |  5 +++++
 gdb/testsuite/ChangeLog                 |  6 ++++++
 gdb/testsuite/gdb.python/py-mi-cmd-2.py | 13 +++++++++++++
 5 files changed, 48 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.python/py-mi-cmd-2.py

-- 
2.20.1

Comments

Tom Tromey April 25, 2019, 7:50 p.m. | #1
>>>>> "Jan" == Jan Vrany <jan.vrany@fit.cvut.cz> writes:


Jan> Redefining python MI commands is especially useful when developing
Jan> then.

It seems like this could lead to difficulties with dangling pointers.
Some assurance on this would be good.

Tom
Jan Vrany May 3, 2019, 3:26 p.m. | #2
On Thu, 2019-04-25 at 13:50 -0600, Tom Tromey wrote:
> > > > > > "Jan" == Jan Vrany <jan.vrany@fit.cvut.cz> writes:

> 

> Jan> Redefining python MI commands is especially useful when developing

> Jan> then.

> 

> It seems like this could lead to difficulties with dangling pointers.

> Some assurance on this would be good.


I'm not sure what exactly do you mean. However, I changed mi_command_py to
use gdbpy_ref<> instead of PyObject* (or even void*). 

Therefore when one redefines the command:

+  if (mi_cmd_table.find (name) != mi_cmd_table.end ())
+    if (! mi_cmd_table[name]->can_be_redefined ())
+      return false;
+
+  mi_cmd_table[name] = std::move (command);

then the old command (if any) is destructed so Py_DECREF() is called on
Python objech held on by mi_command_py() instance. 

I double checked by tracing it in GDB - I don't know how to write
the test for for this though. 

Does above address your concern? 

Thanks!

Jan

> 

> Tom
Tom Tromey May 6, 2019, 9:40 p.m. | #3
>>>>> "Jan" == Jan Vrany <jan.vrany@fit.cvut.cz> writes:


Jan> Redefining python MI commands is especially useful when developing
Jan> then.

>> It seems like this could lead to difficulties with dangling pointers.

>> Some assurance on this would be good.


Jan> I'm not sure what exactly do you mean. However, I changed mi_command_py to
Jan> use gdbpy_ref<> instead of PyObject* (or even void*). 

Jan> Therefore when one redefines the command:

Jan> +  if (mi_cmd_table.find (name) != mi_cmd_table.end ())
Jan> +    if (! mi_cmd_table[name]->can_be_redefined ())
Jan> +      return false;
Jan> +
Jan> +  mi_cmd_table[name] = std::move (command);

Jan> then the old command (if any) is destructed so Py_DECREF() is called on
Jan> Python objech held on by mi_command_py() instance. 

Jan> I double checked by tracing it in GDB - I don't know how to write
Jan> the test for for this though. 

Jan> Does above address your concern? 

The issue isn't a leak but rather the possibility of using an object
after it's been destroyed.

That is, if an MI command can be running when it is replaced, the code
has to be sure that the command object isn't referenced after the
replacement -- because the old object will have been destroyed.

Tom
Jan Vrany May 7, 2019, 11:25 a.m. | #4
On Mon, 2019-05-06 at 15:40 -0600, Tom Tromey wrote:
> Jan> Does above address your concern? 

> 

> The issue isn't a leak but rather the possibility of using an object

> after it's been destroyed.

> 

> That is, if an MI command can be running when it is replaced, the code

> has to be sure that the command object isn't referenced after the

> replacement -- because the old object will have been destroyed.

> 


I see. I just added a test for this case into "almost finished" 
v2 of the patch series. There, this problem is kind of avoided by 
making sure that in mi_command_py::invoke anything from "this" 
mi_command_py object is not accessed AFTER calling the python code. 

However I agree that using shared_ptr is more robust solution.

Thanks! 

Jan



> Tom
Simon Marchi May 7, 2019, 1:09 p.m. | #5
On 2019-05-07 7:25 a.m., Jan Vrany wrote:
> I see. I just added a test for this case into "almost finished" 

> v2 of the patch series. There, this problem is kind of avoided by 

> making sure that in mi_command_py::invoke anything from "this" 

> mi_command_py object is not accessed AFTER calling the python code. 

> 

> However I agree that using shared_ptr is more robust solution.


If we know that we don't access that pointer after it is possibly stale, and
we document that fact properly, I think we can keep what you had initially.
Using shared_ptr has a cost, and it's not really essential here.

Simon
Jan Vrany May 7, 2019, 1:19 p.m. | #6
On Tue, 2019-05-07 at 09:09 -0400, Simon Marchi wrote:
> On 2019-05-07 7:25 a.m., Jan Vrany wrote:

> > I see. I just added a test for this case into "almost finished" 

> > v2 of the patch series. There, this problem is kind of avoided by 

> > making sure that in mi_command_py::invoke anything from "this" 

> > mi_command_py object is not accessed AFTER calling the python code. 

> > 

> > However I agree that using shared_ptr is more robust solution.

> 

> If we know that we don't access that pointer after it is possibly stale, and

> we document that fact properly, I think we can keep what you had initially.

> Using shared_ptr has a cost, and it's not really essential here.

> 


All right. Thanks!

Jan

> Simon
Simon Marchi May 8, 2019, 12:10 a.m. | #7
On 2019-05-07 9:19 a.m., Jan Vrany wrote:
> On Tue, 2019-05-07 at 09:09 -0400, Simon Marchi wrote:

>> On 2019-05-07 7:25 a.m., Jan Vrany wrote:

>>> I see. I just added a test for this case into "almost finished" 

>>> v2 of the patch series. There, this problem is kind of avoided by 

>>> making sure that in mi_command_py::invoke anything from "this" 

>>> mi_command_py object is not accessed AFTER calling the python code. 

>>>

>>> However I agree that using shared_ptr is more robust solution.

>>

>> If we know that we don't access that pointer after it is possibly stale, and

>> we document that fact properly, I think we can keep what you had initially.

>> Using shared_ptr has a cost, and it's not really essential here.

>>

> 

> All right. Thanks!


Well that's my opinion, let's see what Tom thinks about it.

Simon
Tom Tromey May 8, 2019, 6 p.m. | #8
>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:


>>> If we know that we don't access that pointer after it is possibly stale, and

>>> we document that fact properly, I think we can keep what you had initially.

>>> Using shared_ptr has a cost, and it's not really essential here.

>>> 

>> 

>> All right. Thanks!


Simon> Well that's my opinion, let's see what Tom thinks about it.

I'm of two minds.

On the one hand, this sort of approach is fragile.  It's susceptible to
introducing bugs in the future.

On the other hand, it seems somewhat unlikely to actually be a cause of
bugs, since it's doubtful that more code will be added here.

I tend to suspect that using shared_ptr here is not too expensive, but I
don't mind either way I guess.

Tom

Patch

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 1fd49e703f..1fb1cfc2e2 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,11 @@ 
+2018-12-12  Jan Vrany  <jan.vrany@fit.cvut.cz>
+
+	* mi/mi-cmds.h (mi_command::is_py_command): New method.
+	(mi_command_py::is_py_command) New method.
+	* gdb/mi/mi-cmds.c: (mi_command::is_py_command): New method.
+	(mi_command_py::is_py_command) New method.
+	(insert_mi_cmd_entry): Allow redefinition of python-defined MI commands.
+
 2018-12-10  Jan Vrany  <jan.vrany@fit.cvut.cz>
 
 	* python/py-micmd.c (py_mi_invoke): Handle exceptions thrown in Python
diff --git a/gdb/mi/mi-cmds.c b/gdb/mi/mi-cmds.c
index c4332e59cf..95abdac080 100644
--- a/gdb/mi/mi-cmds.c
+++ b/gdb/mi/mi-cmds.c
@@ -43,7 +43,8 @@  insert_mi_cmd_entry (mi_cmd_up command)
   const std::string &name = command->name ();
 
   if (mi_cmd_table.find (name) != mi_cmd_table.end ())
-    return false;
+    if (! mi_cmd_table[name]->is_py_command ())
+      return false;
 
   mi_cmd_table[name] = std::move (command);
 
@@ -82,6 +83,13 @@  mi_command::mi_command (const char *name, int *suppress_notification)
     m_suppress_notification (suppress_notification)
 {}
 
+bool
+mi_command::is_py_command()
+{
+  return false;
+}
+
+
 std::unique_ptr<scoped_restore_tmpl<int>>
 mi_command::do_suppress_notification ()
 {
@@ -152,6 +160,13 @@  mi_command_py::invoke (struct mi_parse *parse)
 
 }
 
+bool
+mi_command_py::is_py_command()
+{
+  return true;
+}
+
+
 /* Initialize the available MI commands.  */
 
 static void
diff --git a/gdb/mi/mi-cmds.h b/gdb/mi/mi-cmds.h
index a89e265de7..1f199a6434 100644
--- a/gdb/mi/mi-cmds.h
+++ b/gdb/mi/mi-cmds.h
@@ -140,6 +140,9 @@  class mi_command
     /* Execute the MI command.  */
     virtual void invoke (struct mi_parse *parse) = 0;
 
+    /* Return TRUE if command is python command, FALSE otherwise. */
+    virtual bool is_py_command();
+    
   protected:
     std::unique_ptr<scoped_restore_tmpl<int>> do_suppress_notification ();
 
@@ -188,6 +191,8 @@  class mi_command_py : public mi_command
 		   void *object);
     void invoke (struct mi_parse *parse) override;
 
+    bool is_py_command() override;
+
   private:
     void *pyobj;
 
diff --git a/gdb/testsuite/ChangeLog b/gdb/testsuite/ChangeLog
index a71b3f25db..0172eada9e 100644
--- a/gdb/testsuite/ChangeLog
+++ b/gdb/testsuite/ChangeLog
@@ -1,3 +1,9 @@ 
+2018-12-12  Jan Vrany  <jan.vrany@fit.cvut.cz>
+
+	* gdb.python/py-mi-cmd.exp: Add tests for python-defined MI command
+	redefinition. 
+	* gdb.python/py-mi-cmd-2.py: New file.
+
 2018-12-10  Jan Vrany  <jan.vrany@fit.cvut.cz>
 
 	* gdb.python/py-mi-cmd.exp: New file.
diff --git a/gdb/testsuite/gdb.python/py-mi-cmd-2.py b/gdb/testsuite/gdb.python/py-mi-cmd-2.py
new file mode 100644
index 0000000000..5d5929d2fd
--- /dev/null
+++ b/gdb/testsuite/gdb.python/py-mi-cmd-2.py
@@ -0,0 +1,13 @@ 
+import gdb
+
+class pycmd(gdb.MICommand):
+    def invoke(self, argv):
+        if argv[0] == 'bogus':
+            return "not really"
+        else:
+            raise gdb.GdbError("Invalid parameter: %s" % argv[0])
+
+pycmd('-pycmd')
+pycmd('-info-gdb-mi-command')
+
+