[RFC,8/8] mi/python: Allow redefinition of python MI commands
Commit Message
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
Comments
>>>>> "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
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
>>>>> "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
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
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
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
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
>>>>> "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
@@ -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
@@ -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
@@ -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;
@@ -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.
new file mode 100644
@@ -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')
+
+