[v9,06/10] python: Add clear() to gdb.Record.
Commit Message
This function allows to clear the trace data from python, forcing to
re-decode the trace for successive commands.
This will be used in future ptwrite patches, to trigger re-decoding when
the ptwrite filter changes.
---
gdb/doc/python.texi | 5 +++++
gdb/python/py-record-btrace.c | 13 +++++++++++++
gdb/python/py-record-btrace.h | 3 +++
gdb/python/py-record.c | 16 ++++++++++++++++
gdb/testsuite/gdb.python/py-record-btrace.exp | 4 ++++
5 files changed, 41 insertions(+)
Comments
> Cc: Felix Willgerodt <felix.willgerodt@intel.com>
> Date: Tue, 4 Jul 2023 14:35:56 +0200
> From: Felix Willgerodt via Gdb-patches <gdb-patches@sourceware.org>
>
> This function allows to clear the trace data from python, forcing to
> re-decode the trace for successive commands.
> This will be used in future ptwrite patches, to trigger re-decoding when
> the ptwrite filter changes.
> ---
> gdb/doc/python.texi | 5 +++++
> gdb/python/py-record-btrace.c | 13 +++++++++++++
> gdb/python/py-record-btrace.h | 3 +++
> gdb/python/py-record.c | 16 ++++++++++++++++
> gdb/testsuite/gdb.python/py-record-btrace.exp | 4 ++++
> 5 files changed, 41 insertions(+)
The documentation part is OK, thanks.
Is this addition NEWS-worthy?
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
> -----Original Message-----
> From: Eli Zaretskii <eliz@gnu.org>
> Sent: Dienstag, 4. Juli 2023 14:46
> To: Willgerodt, Felix <felix.willgerodt@intel.com>
> Cc: gdb-patches@sourceware.org; Metzger, Markus T
> <markus.t.metzger@intel.com>; simark@simark.ca
> Subject: Re: [PATCH v9 06/10] python: Add clear() to gdb.Record.
>
> > Cc: Felix Willgerodt <felix.willgerodt@intel.com>
> > Date: Tue, 4 Jul 2023 14:35:56 +0200
> > From: Felix Willgerodt via Gdb-patches <gdb-patches@sourceware.org>
> >
> > This function allows to clear the trace data from python, forcing to
> > re-decode the trace for successive commands.
> > This will be used in future ptwrite patches, to trigger re-decoding when
> > the ptwrite filter changes.
> > ---
> > gdb/doc/python.texi | 5 +++++
> > gdb/python/py-record-btrace.c | 13 +++++++++++++
> > gdb/python/py-record-btrace.h | 3 +++
> > gdb/python/py-record.c | 16 ++++++++++++++++
> > gdb/testsuite/gdb.python/py-record-btrace.exp | 4 ++++
> > 5 files changed, 41 insertions(+)
>
> The documentation part is OK, thanks.
>
> Is this addition NEWS-worthy?
>
> Reviewed-By: Eli Zaretskii <eliz@gnu.org>
Thanks! Good question, I so far decided no, as it isn't really a big feature.
Not sure, if it is GDB policy to mention every new command, I can for sure
add a mention if you think it make sense.
Felix
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
> From: "Willgerodt, Felix" <felix.willgerodt@intel.com>
> CC: "gdb-patches@sourceware.org" <gdb-patches@sourceware.org>, "Metzger,
> Markus T" <markus.t.metzger@intel.com>, "simark@simark.ca" <simark@simark.ca>
> Date: Wed, 5 Jul 2023 10:03:11 +0000
>
> > Is this addition NEWS-worthy?
> >
> > Reviewed-By: Eli Zaretskii <eliz@gnu.org>
>
> Thanks! Good question, I so far decided no, as it isn't really a big feature.
> Not sure, if it is GDB policy to mention every new command, I can for sure
> add a mention if you think it make sense.
Yes, I think we mention every new command in NEWS.
>>>>> Felix Willgerodt via Gdb-patches <gdb-patches@sourceware.org> writes:
> This function allows to clear the trace data from python, forcing to
> re-decode the trace for successive commands.
> This will be used in future ptwrite patches, to trigger re-decoding when
> the ptwrite filter changes.
> +PyObject *
> +recpy_bt_clear (PyObject *self, PyObject *args)
> +{
> + const recpy_record_object * const record = (recpy_record_object *) self;
> + thread_info *const tinfo = record->thread;
Normally in the Python layer, some care must be taken to ensure that
something sensible happens when a Python object outlives some underlying
gdb object. That is why some types have an 'is_valid' method and why
there are the various *_REQUIRE_VALID macros.
It isn't a problem with this patch per se but it seems to me that this
code does not handle this situation properly.
Tom
> -----Original Message-----
> From: Tom Tromey <tom@tromey.com>
> Sent: Donnerstag, 6. Juli 2023 18:12
> To: Felix Willgerodt via Gdb-patches <gdb-patches@sourceware.org>
> Cc: Metzger, Markus T <markus.t.metzger@intel.com>; simark@simark.ca;
> Willgerodt, Felix <felix.willgerodt@intel.com>
> Subject: Re: [PATCH v9 06/10] python: Add clear() to gdb.Record.
>
> >>>>> Felix Willgerodt via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> > This function allows to clear the trace data from python, forcing to
> > re-decode the trace for successive commands.
> > This will be used in future ptwrite patches, to trigger re-decoding when
> > the ptwrite filter changes.
>
> > +PyObject *
> > +recpy_bt_clear (PyObject *self, PyObject *args)
> > +{
> > + const recpy_record_object * const record = (recpy_record_object *) self;
> > + thread_info *const tinfo = record->thread;
>
> Normally in the Python layer, some care must be taken to ensure that
> something sensible happens when a Python object outlives some underlying
> gdb object. That is why some types have an 'is_valid' method and why
> there are the various *_REQUIRE_VALID macros.
>
> It isn't a problem with this patch per se but it seems to me that this
> code does not handle this situation properly.
>
> Tom
Thanks for the review, I am not sure I understand your point completely.
In this patch, standalone, I don't see any such problem, so you are probably
referring to the whole series.
Note that there is already "maintenance btrace clear" in CLI. So if anything
bad could happen, it would already be exposed by that command as well.
But yes, that is a maintenance command so maybe not 100% comparable.
Why we need this patch in this series, is that once a function call history
or instruction call history is decoded, it will not be re-decoded until you
continue to the next BP and re-issue the command.
And that makes some sense, as the decoding can be costly.
Now, if a user registers a new ptwrite filter, and we don't re-decode the
trace, he will be confused. As he still sees the old trace and not the
output of his newly registered filter function. We could state that that
is a limitation, or check for this and warn the user. But I decided to instead
clear the trace data to force re-decoding the function and instruction
history.
I tested this a bit, and you can too with upstream using the maintenance
command. I didn't see any problems, any subsequent commands would
use the new trace data. And any gdb python objects would also fetch that
new trace data or print an error if the trace data is empty.
Felix
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
>> > +PyObject *
>> > +recpy_bt_clear (PyObject *self, PyObject *args)
>> > +{
>> > + const recpy_record_object * const record = (recpy_record_object *) self;
>> > + thread_info *const tinfo = record->thread;
>> Normally in the Python layer, some care must be taken to ensure that
>> something sensible happens when a Python object outlives some underlying
>> gdb object. That is why some types have an 'is_valid' method and why
>> there are the various *_REQUIRE_VALID macros.
> Thanks for the review, I am not sure I understand your point completely.
> In this patch, standalone, I don't see any such problem, so you are probably
> referring to the whole series.
What I mean is consider this sequence:
* Run some inferior
* Use the py-record-btrace code -- however you do that -- to create one
of these objects and stash it in some global.
* Kill or restart the inferior.
* Do any operation on that stored global that references
btpy_list_object::thread
I'd expect a crash or UAF in this situation because the thread has been
destroyed.
In other Python-layer modules, care is taken to decouple the lifetime of
the Python object wrappers from their underlying gdb objects. This way,
methods on such objects raise a Python exception rather than causing a
gdb crash.
For example, the gdb.InferiorThread type has this exact same setup, but
solved the problem by arranging to clear the 'thread' member when the
thread dies:
gdb::observers::thread_exit.attach (delete_thread_object, "py-inferior");
and then all the InferiorThread methods call this:
THPY_REQUIRE_VALID (thread_obj);
to ensure that the underlying thread exists.
Tom
> -----Original Message-----
> From: Tom Tromey <tom@tromey.com>
> Sent: Donnerstag, 13. Juli 2023 18:45
> To: Willgerodt, Felix via Gdb-patches <gdb-patches@sourceware.org>
> Cc: Tom Tromey <tom@tromey.com>; Willgerodt, Felix
> <felix.willgerodt@intel.com>; Metzger, Markus T <markus.t.metzger@intel.com>;
> simark@simark.ca
> Subject: Re: [PATCH v9 06/10] python: Add clear() to gdb.Record.
>
> >> > +PyObject *
> >> > +recpy_bt_clear (PyObject *self, PyObject *args)
> >> > +{
> >> > + const recpy_record_object * const record = (recpy_record_object *) self;
> >> > + thread_info *const tinfo = record->thread;
>
> >> Normally in the Python layer, some care must be taken to ensure that
> >> something sensible happens when a Python object outlives some underlying
> >> gdb object. That is why some types have an 'is_valid' method and why
> >> there are the various *_REQUIRE_VALID macros.
>
> > Thanks for the review, I am not sure I understand your point completely.
>
> > In this patch, standalone, I don't see any such problem, so you are probably
> > referring to the whole series.
>
> What I mean is consider this sequence:
>
> * Run some inferior
>
> * Use the py-record-btrace code -- however you do that -- to create one
> of these objects and stash it in some global.
>
> * Kill or restart the inferior.
>
> * Do any operation on that stored global that references
> btpy_list_object::thread
>
> I'd expect a crash or UAF in this situation because the thread has been
> destroyed.
The code does check if we have a valid btinfo object (and therefore thread)
and errors out.
In this experiments, I saved some globals, ran the program till exit
and then went back to python again:
>>> r = gdb.current_recording()
>>> h = r.instruction_history
>>> hf = r.function_call_history
>>> x = h[0]
>>> y = hf[2]
>>> quit
(gdb) c
Continuing.
[Inferior 1 (process 119679) exited with code 011]
(gdb) pi
>>> y
<gdb.RecordFunctionSegment object at 0x7f3e9c35be10>
>>> x
<gdb.RecordInstruction object at 0x7f3e9c35be70>
>>> h
<gdb.BtraceObjectList object at 0x7f3e9c1bd530>
>>> hf
<gdb.BtraceObjectList object at 0x7f3e9c1bd7f0>
>>> y.symbol
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
gdb.error: No such function segment.
>>> x.decoded
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
gdb.error: No such instruction.
>>> h[0]
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
gdb.error: No such instruction.
>>> hf[0]
<gdb.RecordFunctionSegment object at 0x7f3e9c35bd50>
>>> hf[0].symbol
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
gdb.error: No such function segment.
You see the same thing if you use btrace.clear() or maint btrace clear.
The same checks are added in my patch for the aux object. See the function
recpy_bt_aux_data. And the functions btrace_insn_from_recpy_insn and
btrace_func_from_recpy_func in upstream.
Also note that this behavior wasn't added by my patch series.
It merely adds a new object type with the same behavior.
Felix
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
@@ -4051,6 +4051,11 @@ A @code{gdb.Record} object has the following methods:
Move the replay position to the given @var{instruction}.
@end defun
+@defun Record.clear ()
+Clear the trace data of the current recording. This forces re-decoding of the
+trace for successive commands.
+@end defun
+
The common @code{gdb.Instruction} class that recording method specific
instruction objects inherit from, has the following attributes:
@@ -845,6 +845,19 @@ recpy_bt_goto (PyObject *self, PyObject *args)
Py_RETURN_NONE;
}
+/* Implementation of BtraceRecord.clear (self) -> None. */
+
+PyObject *
+recpy_bt_clear (PyObject *self, PyObject *args)
+{
+ const recpy_record_object * const record = (recpy_record_object *) self;
+ thread_info *const tinfo = record->thread;
+
+ btrace_clear (tinfo);
+
+ Py_RETURN_NONE;
+}
+
/* BtraceList methods. */
static PyMethodDef btpy_list_methods[] =
@@ -31,6 +31,9 @@ extern PyObject *recpy_bt_format (PyObject *self, void *closure);
/* Implementation of record.goto (instruction) -> None. */
extern PyObject *recpy_bt_goto (PyObject *self, PyObject *value);
+/* Implementation of BtraceRecord.clear (self) -> None. */
+extern PyObject *recpy_bt_clear (PyObject *self, PyObject *args);
+
/* Implementation of record.instruction_history [list]. */
extern PyObject *recpy_bt_instruction_history (PyObject *self, void *closure);
@@ -115,6 +115,19 @@ recpy_goto (PyObject *self, PyObject *value)
return PyErr_Format (PyExc_NotImplementedError, _("Not implemented."));
}
+/* Implementation of record.clear () -> None. */
+
+static PyObject *
+recpy_clear (PyObject *self, PyObject *value)
+{
+ const recpy_record_object * const obj = (recpy_record_object *) self;
+
+ if (obj->method == RECORD_METHOD_BTRACE)
+ return recpy_bt_clear (self, value);
+
+ return PyErr_Format (PyExc_NotImplementedError, _("Not implemented."));
+}
+
/* Implementation of record.replay_position [instruction] */
static PyObject *
@@ -523,6 +536,9 @@ static PyMethodDef recpy_record_methods[] = {
{ "goto", recpy_goto, METH_VARARGS,
"goto (instruction|function_call) -> None.\n\
Rewind to given location."},
+ { "clear", recpy_clear, METH_VARARGS,
+ "clear () -> None.\n\
+Clears the trace."},
{ NULL }
};
@@ -144,6 +144,10 @@ with_test_prefix "instruction " {
gdb_test "python print(i.decoded)" ".*"
gdb_test "python print(i.size)" "$decimal"
gdb_test "python print(i.is_speculative)" "False"
+ gdb_test_no_output "python r.clear()"
+ gdb_test "python insn = r.instruction_history"
+ gdb_test_no_output "python i = insn\[0\]"
+ gdb_test "python print(i.size)" "$decimal"
}
with_test_prefix "function call" {