* [PATCH 01/10] gdb/doc: spring clean the Python unwinders documentation
2023-03-10 14:55 [PATCH 00/10] Improvements & Cleanup For Python Unwinder API Andrew Burgess
@ 2023-03-10 14:55 ` Andrew Burgess
2023-03-10 15:24 ` Eli Zaretskii
2023-03-10 14:55 ` [PATCH 02/10] gdb/python: make the gdb.unwinder.Unwinder class more robust Andrew Burgess
` (9 subsequent siblings)
10 siblings, 1 reply; 37+ messages in thread
From: Andrew Burgess @ 2023-03-10 14:55 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess
The documentation for the Python Unwinders API could do with some
improvement. The 'Unwinder Skeleton Code' has an error: it says
'unwinders' when it should say 'unwinder' in one case.
Additionally, by placing the 'Unwinder Skeleton Code' before the
section 'Registering an Unwinder' we have skipping including the
registration line in the skeleton code. But this is confusion for
users (I think) as the skeleton code is almost complete, except for
one missing line which the user has to figure out for themselves. By
reordering the sections, it is now obvious that the registration
should be included in the skeleton code, and the example is therefore
almost complete.
Additionally, in the example skeleton code the way in which the
frame-id was being built (using the current stack point and program
counter is (a) not correct, and (b) counter to what is laid out in the
'Unwinder Input' section when describing building a frame-id.
I've removed the incorrect code and replaced it with more generic
comments indicating what needs to be done. As the actual actions that
need to be performed are both architecture specific, and dependent on
the function being unwound, it's almost impossible to include more
exact code here, but I think what I'm proposing is less misleading
than what we had before.
I've also added more cross references.
---
gdb/doc/python.texi | 82 ++++++++++++++++++++++++---------------------
1 file changed, 44 insertions(+), 38 deletions(-)
diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index b04f1de2ddf..9ab0aca7d57 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -2784,9 +2784,10 @@
@defun PendingFrame.create_unwind_info (frame_id)
Returns a new @code{gdb.UnwindInfo} instance identified by given
-@var{frame_id}. The argument is used to build @value{GDBN}'s frame ID
-using one of functions provided by @value{GDBN}. @var{frame_id}'s attributes
-determine which function will be used, as follows:
+@var{frame_id}. The @var{frame_id} is used internally by @value{GDBN}
+to identify the frames within the current thread's stack. The
+attributes of @var{frame_id} determine what type of frame ID is
+created within @value{GDBN}:
@table @code
@item sp, pc
@@ -2841,6 +2842,34 @@
@var{value} is a register value (a @code{gdb.Value} object).
@end defun
+@subheading Registering an Unwinder
+
+Object files and program spaces can have unwinders registered with
+them. In addition, you can also register unwinders globally.
+
+The @code{gdb.unwinders} module provides the function to register an
+unwinder:
+
+@defun gdb.unwinder.register_unwinder (locus, unwinder, replace=False)
+@var{locus} specifies to which unwinder list to prepend the
+@var{unwinder}. It can be either an object file (@pxref{Objfiles In
+Python}), a program space (@pxref{Progspaces In Python}), or
+@code{None}, in which case the unwinder is registered globally. The
+newly added @var{unwinder} will be called before any other unwinder
+from the same locus. Two unwinders in the same locus cannot have the
+same name. An attempt to add an unwinder with an already existing
+name raises an exception unless @var{replace} is @code{True}, in which
+case the old unwinder is deleted and the new unwinder is registered in
+its place.
+@end defun
+
+@subheading Unwinder Precedence
+
+@value{GDBN} first calls the unwinders from all the object files in no
+particular order, then the unwinders from the current program space,
+then the globally registered unwinders, and finally the unwinders
+builtin to @value{GDBN}.
+
@subheading Unwinder Skeleton Code
@value{GDBN} comes with the module containing the base @code{Unwinder}
@@ -2848,7 +2877,7 @@
follows:
@smallexample
-from gdb.unwinders import Unwinder
+from gdb.unwinder import Unwinder
class FrameId(object):
def __init__(self, sp, pc):
@@ -2857,53 +2886,30 @@
class MyUnwinder(Unwinder):
- def __init__(....):
- super(MyUnwinder, self).__init___(<expects unwinder name argument>)
+ def __init__(self):
+ super().__init___("MyUnwinder_Name")
- def __call__(pending_frame):
+ def __call__(self, pending_frame):
if not <we recognize frame>:
return None
- # Create UnwindInfo. Usually the frame is identified by the stack
- # pointer and the program counter.
- sp = pending_frame.read_register(<SP number>)
- pc = pending_frame.read_register(<PC number>)
+
+ # Create a FrameID. Usually the frame is identified by a
+ # stack pointer and the function address.
+ sp = ... compute a stack address ...
+ pc = ... compute function address ...
unwind_info = pending_frame.create_unwind_info(FrameId(sp, pc))
- # Find the values of the registers in the caller's frame and
+ # Find the values of the registers in the caller's frame and
# save them in the result:
- unwind_info.add_saved_register(<register>, <value>)
+ unwind_info.add_saved_register(<register-number>, <register-value>)
....
# Return the result:
return unwind_info
+gdb.unwinder.register_unwinder(<locus>, MyUnwinder(), <replace>)
@end smallexample
-@subheading Registering an Unwinder
-
-Object files and program spaces can have unwinders registered with
-them. In addition, you can also register unwinders globally.
-
-The @code{gdb.unwinders} module provides the function to register an
-unwinder:
-
-@defun gdb.unwinder.register_unwinder (locus, unwinder, replace=False)
-@var{locus} specifies to which unwinder list to prepend the
-@var{unwinder}. It can be either an object file, a program space, or
-@code{None}, in which case the unwinder is registered globally. The
-newly added @var{unwinder} will be called before any other unwinder from the
-same locus. Two unwinders in the same locus cannot have the same
-name. An attempt to add an unwinder with an already existing name raises
-an exception unless @var{replace} is @code{True}, in which case the
-old unwinder is deleted.
-@end defun
-
-@subheading Unwinder Precedence
-
-@value{GDBN} first calls the unwinders from all the object files in no
-particular order, then the unwinders from the current program space,
-and finally the unwinders from @value{GDBN}.
-
@node Xmethods In Python
@subsubsection Xmethods In Python
@cindex xmethods in Python
--
2.25.4
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 01/10] gdb/doc: spring clean the Python unwinders documentation
2023-03-10 14:55 ` [PATCH 01/10] gdb/doc: spring clean the Python unwinders documentation Andrew Burgess
@ 2023-03-10 15:24 ` Eli Zaretskii
2023-03-14 9:27 ` Andrew Burgess
0 siblings, 1 reply; 37+ messages in thread
From: Eli Zaretskii @ 2023-03-10 15:24 UTC (permalink / raw)
To: Andrew Burgess; +Cc: gdb-patches
> Cc: Andrew Burgess <aburgess@redhat.com>
> Date: Fri, 10 Mar 2023 14:55:18 +0000
> From: Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org>
>
> The documentation for the Python Unwinders API could do with some
> improvement. The 'Unwinder Skeleton Code' has an error: it says
> 'unwinders' when it should say 'unwinder' in one case.
>
> Additionally, by placing the 'Unwinder Skeleton Code' before the
> section 'Registering an Unwinder' we have skipping including the
> registration line in the skeleton code. But this is confusion for
> users (I think) as the skeleton code is almost complete, except for
> one missing line which the user has to figure out for themselves. By
> reordering the sections, it is now obvious that the registration
> should be included in the skeleton code, and the example is therefore
> almost complete.
>
> Additionally, in the example skeleton code the way in which the
> frame-id was being built (using the current stack point and program
> counter is (a) not correct, and (b) counter to what is laid out in the
> 'Unwinder Input' section when describing building a frame-id.
>
> I've removed the incorrect code and replaced it with more generic
> comments indicating what needs to be done. As the actual actions that
> need to be performed are both architecture specific, and dependent on
> the function being unwound, it's almost impossible to include more
> exact code here, but I think what I'm proposing is less misleading
> than what we had before.
>
> I've also added more cross references.
> ---
> gdb/doc/python.texi | 82 ++++++++++++++++++++++++---------------------
> 1 file changed, 44 insertions(+), 38 deletions(-)
Thanks.
> @defun PendingFrame.create_unwind_info (frame_id)
> Returns a new @code{gdb.UnwindInfo} instance identified by given
> -@var{frame_id}. The argument is used to build @value{GDBN}'s frame ID
> -using one of functions provided by @value{GDBN}. @var{frame_id}'s attributes
> -determine which function will be used, as follows:
> +@var{frame_id}. The @var{frame_id} is used internally by @value{GDBN}
> +to identify the frames within the current thread's stack. The
> +attributes of @var{frame_id} determine what type of frame ID is
> +created within @value{GDBN}: ^^^^^^^^
I think the "ID" part there should be dropped (you are talking about
creating frames, not IDs, right?).
> +Object files and program spaces can have unwinders registered with
> +them. In addition, you can also register unwinders globally.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Either "in addition" or "also", but not both.
> +same name. An attempt to add an unwinder with an already existing
> +name raises an exception unless @var{replace} is @code{True}, in which
> +case the old unwinder is deleted and the new unwinder is registered in
> +its place. ^^
^^^^^^^^^
I believe "in its stead" is better English-wise.
> +@subheading Unwinder Precedence
> +
> +@value{GDBN} first calls the unwinders from all the object files in no
> +particular order, then the unwinders from the current program space,
> +then the globally registered unwinders, and finally the unwinders
> +builtin to @value{GDBN}.
This text is too short to justify a separate @subheading, IMO.
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 01/10] gdb/doc: spring clean the Python unwinders documentation
2023-03-10 15:24 ` Eli Zaretskii
@ 2023-03-14 9:27 ` Andrew Burgess
2023-03-14 12:56 ` Eli Zaretskii
0 siblings, 1 reply; 37+ messages in thread
From: Andrew Burgess @ 2023-03-14 9:27 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: gdb-patches
Eli Zaretskii <eliz@gnu.org> writes:
>> Cc: Andrew Burgess <aburgess@redhat.com>
>> Date: Fri, 10 Mar 2023 14:55:18 +0000
>> From: Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org>
>>
>> The documentation for the Python Unwinders API could do with some
>> improvement. The 'Unwinder Skeleton Code' has an error: it says
>> 'unwinders' when it should say 'unwinder' in one case.
>>
>> Additionally, by placing the 'Unwinder Skeleton Code' before the
>> section 'Registering an Unwinder' we have skipping including the
>> registration line in the skeleton code. But this is confusion for
>> users (I think) as the skeleton code is almost complete, except for
>> one missing line which the user has to figure out for themselves. By
>> reordering the sections, it is now obvious that the registration
>> should be included in the skeleton code, and the example is therefore
>> almost complete.
>>
>> Additionally, in the example skeleton code the way in which the
>> frame-id was being built (using the current stack point and program
>> counter is (a) not correct, and (b) counter to what is laid out in the
>> 'Unwinder Input' section when describing building a frame-id.
>>
>> I've removed the incorrect code and replaced it with more generic
>> comments indicating what needs to be done. As the actual actions that
>> need to be performed are both architecture specific, and dependent on
>> the function being unwound, it's almost impossible to include more
>> exact code here, but I think what I'm proposing is less misleading
>> than what we had before.
>>
>> I've also added more cross references.
>> ---
>> gdb/doc/python.texi | 82 ++++++++++++++++++++++++---------------------
>> 1 file changed, 44 insertions(+), 38 deletions(-)
>
> Thanks.
>
>> @defun PendingFrame.create_unwind_info (frame_id)
>> Returns a new @code{gdb.UnwindInfo} instance identified by given
>> -@var{frame_id}. The argument is used to build @value{GDBN}'s frame ID
>> -using one of functions provided by @value{GDBN}. @var{frame_id}'s attributes
>> -determine which function will be used, as follows:
>> +@var{frame_id}. The @var{frame_id} is used internally by @value{GDBN}
>> +to identify the frames within the current thread's stack. The
>> +attributes of @var{frame_id} determine what type of frame ID is
>> +created within @value{GDBN}: ^^^^^^^^
>
> I think the "ID" part there should be dropped (you are talking about
> creating frames, not IDs, right?).
Good spot. I dropped the 'ID'.
>
>> +Object files and program spaces can have unwinders registered with
>> +them. In addition, you can also register unwinders globally.
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Either "in addition" or "also", but not both.
This sentence has become:
In addition, you can register unwinders globally.
>
>> +same name. An attempt to add an unwinder with an already existing
>> +name raises an exception unless @var{replace} is @code{True}, in which
>> +case the old unwinder is deleted and the new unwinder is registered in
>> +its place. ^^
> ^^^^^^^^^
> I believe "in its stead" is better English-wise.
Changed to '... this command installed in its stead.'
>
>> +@subheading Unwinder Precedence
>> +
>> +@value{GDBN} first calls the unwinders from all the object files in no
>> +particular order, then the unwinders from the current program space,
>> +then the globally registered unwinders, and finally the unwinders
>> +builtin to @value{GDBN}.
>
> This text is too short to justify a separate @subheading, IMO.
I folded this section into the previous function description. The
previous function is how to register unwinders, so adding information
about the order in which registered unwinders are called feels like a
good fit.
>
> Reviewed-By: Eli Zaretskii <eliz@gnu.org>
I added your tag, but the updated patch is below if you want to
take another pass.
Thank,
Andrew
---
commit 861c24ec4d306d7604818cff0cd5c6063292376f
Author: Andrew Burgess <aburgess@redhat.com>
Date: Wed Mar 8 15:26:32 2023 +0000
gdb/doc: spring clean the Python unwinders documentation
The documentation for the Python Unwinders API could do with some
improvement. The 'Unwinder Skeleton Code' has an error: it says
'unwinders' when it should say 'unwinder' in one case.
Additionally, by placing the 'Unwinder Skeleton Code' before the
section 'Registering an Unwinder' we have skipping including the
registration line in the skeleton code. But this is confusion for
users (I think) as the skeleton code is almost complete, except for
one missing line which the user has to figure out for themselves. By
reordering the sections, it is now obvious that the registration
should be included in the skeleton code, and the example is therefore
almost complete.
Additionally, in the example skeleton code the way in which the
frame-id was being built (using the current stack point and program
counter is (a) not correct, and (b) counter to what is laid out in the
'Unwinder Input' section when describing building a frame-id.
I've removed the incorrect code and replaced it with more generic
comments indicating what needs to be done. As the actual actions that
need to be performed are both architecture specific, and dependent on
the function being unwound, it's almost impossible to include more
exact code here, but I think what I'm proposing is less misleading
than what we had before.
I've also added more cross references.
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index b04f1de2ddf..54d5660543a 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -2784,9 +2784,10 @@
@defun PendingFrame.create_unwind_info (frame_id)
Returns a new @code{gdb.UnwindInfo} instance identified by given
-@var{frame_id}. The argument is used to build @value{GDBN}'s frame ID
-using one of functions provided by @value{GDBN}. @var{frame_id}'s attributes
-determine which function will be used, as follows:
+@var{frame_id}. The @var{frame_id} is used internally by @value{GDBN}
+to identify the frames within the current thread's stack. The
+attributes of @var{frame_id} determine what type of frame is
+created within @value{GDBN}:
@table @code
@item sp, pc
@@ -2841,6 +2842,32 @@
@var{value} is a register value (a @code{gdb.Value} object).
@end defun
+@subheading Registering an Unwinder
+
+Object files and program spaces can have unwinders registered with
+them. In addition, you can register unwinders globally.
+
+The @code{gdb.unwinders} module provides the function to register an
+unwinder:
+
+@defun gdb.unwinder.register_unwinder (locus, unwinder, replace=False)
+@var{locus} specifies to which unwinder list to prepend the
+@var{unwinder}. It can be either an object file (@pxref{Objfiles In
+Python}), a program space (@pxref{Progspaces In Python}), or
+@code{None}, in which case the unwinder is registered globally. The
+newly added @var{unwinder} will be called before any other unwinder
+from the same locus. Two unwinders in the same locus cannot have the
+same name. An attempt to add an unwinder with an already existing
+name raises an exception unless @var{replace} is @code{True}, in which
+case the old unwinder is deleted and the new unwinder is registered in
+its place.
+
+@value{GDBN} first calls the unwinders from all the object files in no
+particular order, then the unwinders from the current program space,
+then the globally registered unwinders, and finally the unwinders
+builtin to @value{GDBN}.
+@end defun
+
@subheading Unwinder Skeleton Code
@value{GDBN} comes with the module containing the base @code{Unwinder}
@@ -2848,7 +2875,7 @@
follows:
@smallexample
-from gdb.unwinders import Unwinder
+from gdb.unwinder import Unwinder
class FrameId(object):
def __init__(self, sp, pc):
@@ -2857,53 +2884,30 @@
class MyUnwinder(Unwinder):
- def __init__(....):
- super(MyUnwinder, self).__init___(<expects unwinder name argument>)
+ def __init__(self):
+ super().__init___("MyUnwinder_Name")
- def __call__(pending_frame):
+ def __call__(self, pending_frame):
if not <we recognize frame>:
return None
- # Create UnwindInfo. Usually the frame is identified by the stack
- # pointer and the program counter.
- sp = pending_frame.read_register(<SP number>)
- pc = pending_frame.read_register(<PC number>)
+
+ # Create a FrameID. Usually the frame is identified by a
+ # stack pointer and the function address.
+ sp = ... compute a stack address ...
+ pc = ... compute function address ...
unwind_info = pending_frame.create_unwind_info(FrameId(sp, pc))
- # Find the values of the registers in the caller's frame and
+ # Find the values of the registers in the caller's frame and
# save them in the result:
- unwind_info.add_saved_register(<register>, <value>)
+ unwind_info.add_saved_register(<register-number>, <register-value>)
....
# Return the result:
return unwind_info
+gdb.unwinder.register_unwinder(<locus>, MyUnwinder(), <replace>)
@end smallexample
-@subheading Registering an Unwinder
-
-Object files and program spaces can have unwinders registered with
-them. In addition, you can also register unwinders globally.
-
-The @code{gdb.unwinders} module provides the function to register an
-unwinder:
-
-@defun gdb.unwinder.register_unwinder (locus, unwinder, replace=False)
-@var{locus} specifies to which unwinder list to prepend the
-@var{unwinder}. It can be either an object file, a program space, or
-@code{None}, in which case the unwinder is registered globally. The
-newly added @var{unwinder} will be called before any other unwinder from the
-same locus. Two unwinders in the same locus cannot have the same
-name. An attempt to add an unwinder with an already existing name raises
-an exception unless @var{replace} is @code{True}, in which case the
-old unwinder is deleted.
-@end defun
-
-@subheading Unwinder Precedence
-
-@value{GDBN} first calls the unwinders from all the object files in no
-particular order, then the unwinders from the current program space,
-and finally the unwinders from @value{GDBN}.
-
@node Xmethods In Python
@subsubsection Xmethods In Python
@cindex xmethods in Python
@@ -4398,7 +4402,7 @@
commands. Setting this attribute to @code{True} will install the
command for use. If there is already a Python command with this name
installed, the currently installed command will be uninstalled, and
-this command installed in its place.
+this command installed in its stead.
@end defvar
The following code snippet shows how a two trivial MI command can be
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 01/10] gdb/doc: spring clean the Python unwinders documentation
2023-03-14 9:27 ` Andrew Burgess
@ 2023-03-14 12:56 ` Eli Zaretskii
2023-03-16 14:30 ` Andrew Burgess
0 siblings, 1 reply; 37+ messages in thread
From: Eli Zaretskii @ 2023-03-14 12:56 UTC (permalink / raw)
To: Andrew Burgess; +Cc: gdb-patches
> From: Andrew Burgess <aburgess@redhat.com>
> Cc: gdb-patches@sourceware.org
> Date: Tue, 14 Mar 2023 09:27:29 +0000
>
> > Reviewed-By: Eli Zaretskii <eliz@gnu.org>
>
> I added your tag, but the updated patch is below if you want to
> take another pass.
You are a GO.
Thanks.
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 01/10] gdb/doc: spring clean the Python unwinders documentation
2023-03-14 12:56 ` Eli Zaretskii
@ 2023-03-16 14:30 ` Andrew Burgess
0 siblings, 0 replies; 37+ messages in thread
From: Andrew Burgess @ 2023-03-16 14:30 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: gdb-patches
Eli Zaretskii <eliz@gnu.org> writes:
>> From: Andrew Burgess <aburgess@redhat.com>
>> Cc: gdb-patches@sourceware.org
>> Date: Tue, 14 Mar 2023 09:27:29 +0000
>>
>> > Reviewed-By: Eli Zaretskii <eliz@gnu.org>
>>
>> I added your tag, but the updated patch is below if you want to
>> take another pass.
>
> You are a GO.
I pushed this patch.
Thanks,
Andrew
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 02/10] gdb/python: make the gdb.unwinder.Unwinder class more robust
2023-03-10 14:55 [PATCH 00/10] Improvements & Cleanup For Python Unwinder API Andrew Burgess
2023-03-10 14:55 ` [PATCH 01/10] gdb/doc: spring clean the Python unwinders documentation Andrew Burgess
@ 2023-03-10 14:55 ` Andrew Burgess
2023-03-10 15:32 ` Eli Zaretskii
2023-03-31 2:15 ` Simon Marchi
2023-03-10 14:55 ` [PATCH 03/10] gdb/python: remove unneeded nullptr check in frapy_block Andrew Burgess
` (8 subsequent siblings)
10 siblings, 2 replies; 37+ messages in thread
From: Andrew Burgess @ 2023-03-10 14:55 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess
This commit makes a few related changes to the gdb.unwinder.Unwinder
class attributes:
1. The 'name' attribute is now a read-only attribute. This prevents
user code from changing the name after registering the unwinder. It
seems very unlikely that any user is actually trying to do this in
the wild, so I'm not very worried that this will upset anyone,
2. We now validate that the name is a string in the
Unwinder.__init__ method, and throw an error if this is not the
case. Hopefully nobody was doing this in the wild. This should
make it easier to ensure the 'info unwinder' command shows sane
output (how to display a non-string name for an unwinder?),
3. The 'enabled' attribute is now implemented with a getter and
setter. In the setter we ensure that the new value is a boolean,
but the real important change is that we call
'gdb.invalidate_cached_frames()'. This means that the backtrace
will be updated if a user manually disables an unwinder (rather than
calling the 'disable unwinder' command). It is not unreasonable to
think that a user might register multiple unwinders (relating to
some project) and have one command that disables/enables all the
related unwinders. This command might operate by poking the enabled
attribute of each unwinder object directly, after this commit, this
would now work correctly.
There's tests for all the changes, and lots of documentation updates
that both cover the new changes, but also further improve (I think)
the general documentation for GDB's Unwinder API.
---
gdb/NEWS | 14 ++++
gdb/doc/python.texi | 60 +++++++++++++++-
gdb/python/lib/gdb/unwinder.py | 23 ++++++-
gdb/testsuite/gdb.python/py-unwind.exp | 94 ++++++++++++++++++++++++--
gdb/testsuite/gdb.python/py-unwind.py | 13 +++-
5 files changed, 191 insertions(+), 13 deletions(-)
diff --git a/gdb/NEWS b/gdb/NEWS
index c32ff92c98a..ed0f86e79ec 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -96,6 +96,20 @@ show always-read-ctf
without a thread restriction. The same is also true for the 'task'
field of an Ada task-specific breakpoint.
+* Python API
+
+ ** The gdb.unwinder.Unwinder.name attribute is now read-only.
+
+ ** The name argument passed to gdb.unwinder.Unwinder.__init__ must
+ now be of type 'str' otherwise a TypeError will be raised.
+
+ ** The gdb.unwinder.Unwinder.enabled attribute can now only accept
+ values of type 'bool'. Changing this attribute will now
+ invalidate GDB's frame-cache, which means GDB will need to
+ rebuild its frame-cache when next required - either with, or
+ without the particular unwinder, depending on how 'enabled' was
+ changed.
+
*** Changes in GDB 13
* MI version 1 is deprecated, and will be removed in GDB 14.
diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index 9ab0aca7d57..c36130210ba 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -2842,6 +2842,33 @@
@var{value} is a register value (a @code{gdb.Value} object).
@end defun
+@subheading The @code{gdb.unwinder} Module
+
+@value{GDBN} comes with a @code{gdb.unwinder} module which contains
+the following class:
+
+@deftp {class} gdb.unwinder.Unwinder
+The @code{Unwinder} class is a base class from which user created
+unwinders can derive, though it is not required that unwinders derive
+from this class, so long any user created unwinder has the required
+@code{name} and @code{enabled} attributes.
+
+@defun gdb.unwinder.Unwinder.__init__(@var{name})
+The @var{name} is a string used to identify this unwinder within some
+@value{GDBN} commands.
+@end defun
+
+@defvar gdb.unwinder.name
+A read-only attribute which is a string, the name of this unwinder.
+@end defvar
+
+@defvar gdb.unwinder.enabled
+A modifiable attribute containing a boolean, when @code{True} the
+unwinder is enabled, and will be used by @value{GDBN}. When
+@code{False} the unwinder has been disabled, and will not be used.
+@end defvar
+@end deftp
+
@subheading Registering an Unwinder
Object files and program spaces can have unwinders registered with
@@ -2872,9 +2899,7 @@
@subheading Unwinder Skeleton Code
-@value{GDBN} comes with the module containing the base @code{Unwinder}
-class. Derive your unwinder class from it and structure the code as
-follows:
+Here is an example of how and structure a user created unwinder:
@smallexample
from gdb.unwinder import Unwinder
@@ -2910,6 +2935,35 @@
gdb.unwinder.register_unwinder(<locus>, MyUnwinder(), <replace>)
@end smallexample
+@subheading Managing Registered Unwinders
+@value{GDBN} defines 3 new commands to manage registered unwinders.
+These are:
+
+@table @code
+@item info unwinder @r{[} @var{locus} @r{[} @var{name-regexp} @r{]} @r{]}
+Lists all registered unwinders. Arguments @var{locus} and
+@var{name-regexp} are both optional and can be used to filter which
+unwinders are listed.
+
+The @var{locus} argument should be either @kbd{global},
+@kbd{progspace}, or the name of an object file. Only unwinders
+registered for the specified locus will be listed.
+
+The @var{name-regexp} is a regular expression used to match against
+unwinder names. When trying to match against unwinder names that
+include a string enclose @var{name-regexp} in quotes.
+@item disable unwinder @r{[} @var{locus} @r{[} @var{name-regexp} @r{]} @r{]}
+The @var{locus} and @var{name-regexp} are interpreted as in @kbd{info
+unwinder} above, but instead of listing the matching unwinders, all of
+the matching unwinders are disabled. The @code{enabled} field of each
+matching unwinder is set to @code{False}.
+@item enable unwinder @r{[} @var{locus} @r{[} @var{name-regexp} @r{]} @r{]}
+The @var{locus} and @var{name-regexp} are interpreted as in @kbd{info
+unwinder} above, but instead of listing the matching unwinders, all of
+the matching unwinders are enabled. The @code{enabled} field of each
+matching unwinder is set to @code{True}.
+@end table
+
@node Xmethods In Python
@subsubsection Xmethods In Python
@cindex xmethods in Python
diff --git a/gdb/python/lib/gdb/unwinder.py b/gdb/python/lib/gdb/unwinder.py
index a854d8dbdaa..1303245c054 100644
--- a/gdb/python/lib/gdb/unwinder.py
+++ b/gdb/python/lib/gdb/unwinder.py
@@ -35,8 +35,27 @@ class Unwinder(object):
Args:
name: An identifying name for the unwinder.
"""
- self.name = name
- self.enabled = True
+
+ if not isinstance(name, str):
+ raise TypeError("incorrect type for name: %s" % type(name))
+
+ self._name = name
+ self._enabled = True
+
+ @property
+ def name(self):
+ return self._name
+
+ @property
+ def enabled(self):
+ return self._enabled
+
+ @enabled.setter
+ def enabled(self, value):
+ if not isinstance(value, bool):
+ raise TypeError("incorrect type for enabled attribute: %s" % type(value))
+ self._enabled = value
+ gdb.invalidate_cached_frames()
def __call__(self, pending_frame):
"""GDB calls this method to unwind a frame.
diff --git a/gdb/testsuite/gdb.python/py-unwind.exp b/gdb/testsuite/gdb.python/py-unwind.exp
index 5bf9ae129ac..337e5dc2504 100644
--- a/gdb/testsuite/gdb.python/py-unwind.exp
+++ b/gdb/testsuite/gdb.python/py-unwind.exp
@@ -40,25 +40,105 @@ if {![runto_main]} {
return 0
}
+# Check for the corrupt backtrace.
+proc check_for_broken_backtrace {testname} {
+ gdb_test_sequence "where" $testname {
+ "\\r\\n#0 .* corrupt_frame_inner \\(\\) at "
+ "\\r\\n#1 .* corrupt_frame_outer \\(\\) at "
+ "Backtrace stopped: frame did not save the PC"
+ }
+}
+
+# Check for the correct backtrace.
+proc check_for_fixed_backtrace {testname} {
+ gdb_test_sequence "where" $testname {
+ "\\r\\n#0 .* corrupt_frame_inner \\(\\) at "
+ "\\r\\n#1 .* corrupt_frame_outer \\(\\) at "
+ "\\r\\n#2 .* main \\(.*\\) at"
+ }
+}
+
+# Check the 'info unwinder' output.
+proc check_info_unwinder {testname enabled} {
+ if {$enabled} {
+ set suffix ""
+ } else {
+ set suffix " \\\[disabled\\\]"
+ }
+
+ gdb_test_sequence "info unwinder" $testname \
+ [list \
+ "Global:" \
+ " test unwinder${suffix}"]
+}
+
set pyfile [gdb_remote_download host ${srcdir}/${subdir}/${testfile}.py]
gdb_breakpoint [gdb_get_line_number "break backtrace-broken"]
+gdb_continue_to_breakpoint "break backtrace-broken"
+
+check_for_broken_backtrace "Backtrace is initially broken"
+
gdb_test "source ${pyfile}" "Python script imported" \
- "import python scripts"
+ "import python scripts"
-gdb_continue_to_breakpoint "break backtrace-broken"
-gdb_test_sequence "where" "Backtrace restored by unwinder" {
- "\\r\\n#0 .* corrupt_frame_inner \\(\\) at "
- "\\r\\n#1 .* corrupt_frame_outer \\(\\) at "
- "\\r\\n#2 .* main \\(.*\\) at"
-}
+check_info_unwinder "info unwinder after loading script" on
+
+check_for_fixed_backtrace "check backtrace after loading unwinder"
# Check that the Python unwinder frames can be flushed / released.
gdb_test "maint flush register-cache" "Register cache flushed\\." "flush frames"
+check_for_fixed_backtrace "check backtrace after flush"
+
+# Try to disable the unwinder but instead set the enabled field to a
+# non boolean value. This should fail. Check the 'info unwinder'
+# output to be sure.
+gdb_test "python global_test_unwinder.enabled = \"off\"" \
+ [multi_line \
+ "TypeError: incorrect type for enabled attribute: <class 'str'>" \
+ "Error while executing Python code\\."]
+check_info_unwinder "info unwinder after failed disable" on
+
+# While we're doing silly stuff, lets try to change the name of this
+# unwider. Doing this is bad as the new name might clash with an
+# already registered name, which violates the promises made during
+# 'register_unwinder'.
+gdb_test "python global_test_unwinder.name = \"foo\"" \
+ [multi_line \
+ "AttributeError: can't set attribute" \
+ "Error while executing Python code\\."]
+check_info_unwinder "info unwinder after failed name change" on
+
+# Now actually disable the unwinder by manually adjusting the
+# 'enabled' attribute. Check that the stack is once again broken, and
+# that the unwinder shows as disabled in the 'info unwinder' output.
+gdb_test_no_output "python global_test_unwinder.enabled = False"
+check_for_broken_backtrace "stack is broken after disabling"
+check_info_unwinder "info unwinder after manually disabling" off
+
+# Now enable the unwinder using the 'enable unwinder' command.
+gdb_test "enable unwinder global \"test unwinder\"" \
+ "1 unwinder enabled"
+check_for_fixed_backtrace "check backtrace after enabling with command"
+check_info_unwinder "info unwinder after command enabled" on
+
+# And now disable using the command and check the stack is once again
+# broken, and that the 'info unwinder' output updates correctly.
+gdb_test "disable unwinder global \"test unwinder\"" \
+ "1 unwinder disabled"
+check_for_broken_backtrace "stack is broken after command disabling"
+check_info_unwinder "info unwinder after command disabling" off
+
# Check that invalid register names cause errors.
gdb_test "python print(add_saved_register_error)" "True" \
"add_saved_register error"
gdb_test "python print(read_register_error)" "True" \
"read_register error"
+
+# Try to create an unwinder object with a non-string name.
+gdb_test "python obj = simple_unwinder(True)" \
+ [multi_line \
+ "TypeError: incorrect type for name: <class 'bool'>" \
+ "Error while executing Python code\\."]
diff --git a/gdb/testsuite/gdb.python/py-unwind.py b/gdb/testsuite/gdb.python/py-unwind.py
index f7ad8e45a35..edd2e30eb9b 100644
--- a/gdb/testsuite/gdb.python/py-unwind.py
+++ b/gdb/testsuite/gdb.python/py-unwind.py
@@ -130,5 +130,16 @@ class TestUnwinder(Unwinder):
return None
-gdb.unwinder.register_unwinder(None, TestUnwinder(), True)
+global_test_unwinder = TestUnwinder()
+gdb.unwinder.register_unwinder(None, global_test_unwinder, True)
+
+
+class simple_unwinder(Unwinder):
+ def __init__(self, name):
+ super().__init__(name)
+
+ def __call__(self, pending_frame):
+ return None
+
+
print("Python script imported")
--
2.25.4
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 02/10] gdb/python: make the gdb.unwinder.Unwinder class more robust
2023-03-10 14:55 ` [PATCH 02/10] gdb/python: make the gdb.unwinder.Unwinder class more robust Andrew Burgess
@ 2023-03-10 15:32 ` Eli Zaretskii
2023-03-14 10:06 ` Andrew Burgess
2023-03-31 2:15 ` Simon Marchi
1 sibling, 1 reply; 37+ messages in thread
From: Eli Zaretskii @ 2023-03-10 15:32 UTC (permalink / raw)
To: Andrew Burgess; +Cc: gdb-patches
> Cc: Andrew Burgess <aburgess@redhat.com>
> Date: Fri, 10 Mar 2023 14:55:19 +0000
> From: Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org>
>
> gdb/NEWS | 14 ++++
> gdb/doc/python.texi | 60 +++++++++++++++-
> gdb/python/lib/gdb/unwinder.py | 23 ++++++-
> gdb/testsuite/gdb.python/py-unwind.exp | 94 ++++++++++++++++++++++++--
> gdb/testsuite/gdb.python/py-unwind.py | 13 +++-
> 5 files changed, 191 insertions(+), 13 deletions(-)
Thanks.
> diff --git a/gdb/NEWS b/gdb/NEWS
> index c32ff92c98a..ed0f86e79ec 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -96,6 +96,20 @@ show always-read-ctf
> without a thread restriction. The same is also true for the 'task'
> field of an Ada task-specific breakpoint.
>
> +* Python API
> +
> + ** The gdb.unwinder.Unwinder.name attribute is now read-only.
> +
> + ** The name argument passed to gdb.unwinder.Unwinder.__init__ must
> + now be of type 'str' otherwise a TypeError will be raised.
> +
> + ** The gdb.unwinder.Unwinder.enabled attribute can now only accept
> + values of type 'bool'. Changing this attribute will now
> + invalidate GDB's frame-cache, which means GDB will need to
> + rebuild its frame-cache when next required - either with, or
> + without the particular unwinder, depending on how 'enabled' was
> + changed.
> +
> *** Changes in GDB 13
This part is OK.
> +@deftp {class} gdb.unwinder.Unwinder
> +The @code{Unwinder} class is a base class from which user created
> +unwinders can derive, though it is not required that unwinders derive
> +from this class, so long any user created unwinder has the required
^^^^^^^
"so long as", I believe.
> +@defun gdb.unwinder.Unwinder.__init__(@var{name})
> +The @var{name} is a string used to identify this unwinder within some
> +@value{GDBN} commands.
What do you mean by "identify...within a command"? The "within" part
confuses me.
> +@defvar gdb.unwinder.enabled
> +A modifiable attribute containing a boolean, when @code{True} the
^
A semi-colon there would be better, I think. Also, a comma after
@code{True} is in order.
> +unwinder is enabled, and will be used by @value{GDBN}. When
> +@code{False} the unwinder has been disabled, and will not be used.
Likewise, a comma after @code{False}.
> +Here is an example of how and structure a user created unwinder:
Something is amiss in this sentence.
> +@subheading Managing Registered Unwinders
> +@value{GDBN} defines 3 new commands to manage registered unwinders.
Why "new"? And why say how many of them are there -- that is not
future-proof.
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 02/10] gdb/python: make the gdb.unwinder.Unwinder class more robust
2023-03-10 15:32 ` Eli Zaretskii
@ 2023-03-14 10:06 ` Andrew Burgess
2023-03-14 12:57 ` Eli Zaretskii
0 siblings, 1 reply; 37+ messages in thread
From: Andrew Burgess @ 2023-03-14 10:06 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: gdb-patches
Eli Zaretskii <eliz@gnu.org> writes:
>> Cc: Andrew Burgess <aburgess@redhat.com>
>> Date: Fri, 10 Mar 2023 14:55:19 +0000
>> From: Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org>
>>
>> gdb/NEWS | 14 ++++
>> gdb/doc/python.texi | 60 +++++++++++++++-
>> gdb/python/lib/gdb/unwinder.py | 23 ++++++-
>> gdb/testsuite/gdb.python/py-unwind.exp | 94 ++++++++++++++++++++++++--
>> gdb/testsuite/gdb.python/py-unwind.py | 13 +++-
>> 5 files changed, 191 insertions(+), 13 deletions(-)
>
> Thanks.
>
>> diff --git a/gdb/NEWS b/gdb/NEWS
>> index c32ff92c98a..ed0f86e79ec 100644
>> --- a/gdb/NEWS
>> +++ b/gdb/NEWS
>> @@ -96,6 +96,20 @@ show always-read-ctf
>> without a thread restriction. The same is also true for the 'task'
>> field of an Ada task-specific breakpoint.
>>
>> +* Python API
>> +
>> + ** The gdb.unwinder.Unwinder.name attribute is now read-only.
>> +
>> + ** The name argument passed to gdb.unwinder.Unwinder.__init__ must
>> + now be of type 'str' otherwise a TypeError will be raised.
>> +
>> + ** The gdb.unwinder.Unwinder.enabled attribute can now only accept
>> + values of type 'bool'. Changing this attribute will now
>> + invalidate GDB's frame-cache, which means GDB will need to
>> + rebuild its frame-cache when next required - either with, or
>> + without the particular unwinder, depending on how 'enabled' was
>> + changed.
>> +
>> *** Changes in GDB 13
>
> This part is OK.
>
>> +@deftp {class} gdb.unwinder.Unwinder
>> +The @code{Unwinder} class is a base class from which user created
>> +unwinders can derive, though it is not required that unwinders derive
>> +from this class, so long any user created unwinder has the required
> ^^^^^^^
> "so long as", I believe.
Fixed.
>
>> +@defun gdb.unwinder.Unwinder.__init__(@var{name})
>> +The @var{name} is a string used to identify this unwinder within some
>> +@value{GDBN} commands.
>
> What do you mean by "identify...within a command"? The "within" part
> confuses me.
I reworded this, and added a cross reference to the commands I'm talking
about. Hopefully this makes this clearer:
The @var{name} is a string used to reference this unwinder within some
@value{GDBN} commands (@pxref{Managing Registered Unwinders}).
>
>> +@defvar gdb.unwinder.enabled
>> +A modifiable attribute containing a boolean, when @code{True} the
> ^
> A semi-colon there would be better, I think. Also, a comma after
> @code{True} is in order.
Fixed.
>
>> +unwinder is enabled, and will be used by @value{GDBN}. When
>> +@code{False} the unwinder has been disabled, and will not be used.
>
> Likewise, a comma after @code{False}.
Fixed.
>
>> +Here is an example of how and structure a user created unwinder:
>
> Something is amiss in this sentence.
Changed to:
Here is an example of how to structure a user created unwinder:
>
>> +@subheading Managing Registered Unwinders
>> +@value{GDBN} defines 3 new commands to manage registered unwinders.
>
> Why "new"? And why say how many of them are there -- that is not
> future-proof.
Dropped the 'new'.
>
> Reviewed-By: Eli Zaretskii <eliz@gnu.org>
I added your tag, but the updated patch is below if you want to take
another look.
Thanks,
Andrew
---
commit 859c150b211616c26a862453dbc5069e91b4cbba
Author: Andrew Burgess <aburgess@redhat.com>
Date: Wed Mar 8 16:11:30 2023 +0000
gdb/python: make the gdb.unwinder.Unwinder class more robust
This commit makes a few related changes to the gdb.unwinder.Unwinder
class attributes:
1. The 'name' attribute is now a read-only attribute. This prevents
user code from changing the name after registering the unwinder. It
seems very unlikely that any user is actually trying to do this in
the wild, so I'm not very worried that this will upset anyone,
2. We now validate that the name is a string in the
Unwinder.__init__ method, and throw an error if this is not the
case. Hopefully nobody was doing this in the wild. This should
make it easier to ensure the 'info unwinder' command shows sane
output (how to display a non-string name for an unwinder?),
3. The 'enabled' attribute is now implemented with a getter and
setter. In the setter we ensure that the new value is a boolean,
but the real important change is that we call
'gdb.invalidate_cached_frames()'. This means that the backtrace
will be updated if a user manually disables an unwinder (rather than
calling the 'disable unwinder' command). It is not unreasonable to
think that a user might register multiple unwinders (relating to
some project) and have one command that disables/enables all the
related unwinders. This command might operate by poking the enabled
attribute of each unwinder object directly, after this commit, this
would now work correctly.
There's tests for all the changes, and lots of documentation updates
that both cover the new changes, but also further improve (I think)
the general documentation for GDB's Unwinder API.
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
diff --git a/gdb/NEWS b/gdb/NEWS
index cc262f1f8a6..99e3d0bf222 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -104,6 +104,20 @@ show always-read-ctf
without a thread restriction. The same is also true for the 'task'
field of an Ada task-specific breakpoint.
+* Python API
+
+ ** The gdb.unwinder.Unwinder.name attribute is now read-only.
+
+ ** The name argument passed to gdb.unwinder.Unwinder.__init__ must
+ now be of type 'str' otherwise a TypeError will be raised.
+
+ ** The gdb.unwinder.Unwinder.enabled attribute can now only accept
+ values of type 'bool'. Changing this attribute will now
+ invalidate GDB's frame-cache, which means GDB will need to
+ rebuild its frame-cache when next required - either with, or
+ without the particular unwinder, depending on how 'enabled' was
+ changed.
+
*** Changes in GDB 13
* MI version 1 is deprecated, and will be removed in GDB 14.
diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index 54d5660543a..8af2d1faea3 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -2842,6 +2842,33 @@
@var{value} is a register value (a @code{gdb.Value} object).
@end defun
+@subheading The @code{gdb.unwinder} Module
+
+@value{GDBN} comes with a @code{gdb.unwinder} module which contains
+the following class:
+
+@deftp {class} gdb.unwinder.Unwinder
+The @code{Unwinder} class is a base class from which user created
+unwinders can derive, though it is not required that unwinders derive
+from this class, so long as any user created unwinder has the required
+@code{name} and @code{enabled} attributes.
+
+@defun gdb.unwinder.Unwinder.__init__(@var{name})
+The @var{name} is a string used to reference this unwinder within some
+@value{GDBN} commands (@pxref{Managing Registered Unwinders}).
+@end defun
+
+@defvar gdb.unwinder.name
+A read-only attribute which is a string, the name of this unwinder.
+@end defvar
+
+@defvar gdb.unwinder.enabled
+A modifiable attribute containing a boolean; when @code{True}, the
+unwinder is enabled, and will be used by @value{GDBN}. When
+@code{False}, the unwinder has been disabled, and will not be used.
+@end defvar
+@end deftp
+
@subheading Registering an Unwinder
Object files and program spaces can have unwinders registered with
@@ -2870,9 +2897,7 @@
@subheading Unwinder Skeleton Code
-@value{GDBN} comes with the module containing the base @code{Unwinder}
-class. Derive your unwinder class from it and structure the code as
-follows:
+Here is an example of how to structure a user created unwinder:
@smallexample
from gdb.unwinder import Unwinder
@@ -2908,6 +2933,36 @@
gdb.unwinder.register_unwinder(<locus>, MyUnwinder(), <replace>)
@end smallexample
+@anchor{Managing Registered Unwinders}
+@subheading Managing Registered Unwinders
+@value{GDBN} defines 3 commands to manage registered unwinders. These
+are:
+
+@table @code
+@item info unwinder @r{[} @var{locus} @r{[} @var{name-regexp} @r{]} @r{]}
+Lists all registered unwinders. Arguments @var{locus} and
+@var{name-regexp} are both optional and can be used to filter which
+unwinders are listed.
+
+The @var{locus} argument should be either @kbd{global},
+@kbd{progspace}, or the name of an object file. Only unwinders
+registered for the specified locus will be listed.
+
+The @var{name-regexp} is a regular expression used to match against
+unwinder names. When trying to match against unwinder names that
+include a string enclose @var{name-regexp} in quotes.
+@item disable unwinder @r{[} @var{locus} @r{[} @var{name-regexp} @r{]} @r{]}
+The @var{locus} and @var{name-regexp} are interpreted as in @kbd{info
+unwinder} above, but instead of listing the matching unwinders, all of
+the matching unwinders are disabled. The @code{enabled} field of each
+matching unwinder is set to @code{False}.
+@item enable unwinder @r{[} @var{locus} @r{[} @var{name-regexp} @r{]} @r{]}
+The @var{locus} and @var{name-regexp} are interpreted as in @kbd{info
+unwinder} above, but instead of listing the matching unwinders, all of
+the matching unwinders are enabled. The @code{enabled} field of each
+matching unwinder is set to @code{True}.
+@end table
+
@node Xmethods In Python
@subsubsection Xmethods In Python
@cindex xmethods in Python
diff --git a/gdb/python/lib/gdb/unwinder.py b/gdb/python/lib/gdb/unwinder.py
index a854d8dbdaa..1303245c054 100644
--- a/gdb/python/lib/gdb/unwinder.py
+++ b/gdb/python/lib/gdb/unwinder.py
@@ -35,8 +35,27 @@ class Unwinder(object):
Args:
name: An identifying name for the unwinder.
"""
- self.name = name
- self.enabled = True
+
+ if not isinstance(name, str):
+ raise TypeError("incorrect type for name: %s" % type(name))
+
+ self._name = name
+ self._enabled = True
+
+ @property
+ def name(self):
+ return self._name
+
+ @property
+ def enabled(self):
+ return self._enabled
+
+ @enabled.setter
+ def enabled(self, value):
+ if not isinstance(value, bool):
+ raise TypeError("incorrect type for enabled attribute: %s" % type(value))
+ self._enabled = value
+ gdb.invalidate_cached_frames()
def __call__(self, pending_frame):
"""GDB calls this method to unwind a frame.
diff --git a/gdb/testsuite/gdb.python/py-unwind.exp b/gdb/testsuite/gdb.python/py-unwind.exp
index 5bf9ae129ac..337e5dc2504 100644
--- a/gdb/testsuite/gdb.python/py-unwind.exp
+++ b/gdb/testsuite/gdb.python/py-unwind.exp
@@ -40,25 +40,105 @@ if {![runto_main]} {
return 0
}
+# Check for the corrupt backtrace.
+proc check_for_broken_backtrace {testname} {
+ gdb_test_sequence "where" $testname {
+ "\\r\\n#0 .* corrupt_frame_inner \\(\\) at "
+ "\\r\\n#1 .* corrupt_frame_outer \\(\\) at "
+ "Backtrace stopped: frame did not save the PC"
+ }
+}
+
+# Check for the correct backtrace.
+proc check_for_fixed_backtrace {testname} {
+ gdb_test_sequence "where" $testname {
+ "\\r\\n#0 .* corrupt_frame_inner \\(\\) at "
+ "\\r\\n#1 .* corrupt_frame_outer \\(\\) at "
+ "\\r\\n#2 .* main \\(.*\\) at"
+ }
+}
+
+# Check the 'info unwinder' output.
+proc check_info_unwinder {testname enabled} {
+ if {$enabled} {
+ set suffix ""
+ } else {
+ set suffix " \\\[disabled\\\]"
+ }
+
+ gdb_test_sequence "info unwinder" $testname \
+ [list \
+ "Global:" \
+ " test unwinder${suffix}"]
+}
+
set pyfile [gdb_remote_download host ${srcdir}/${subdir}/${testfile}.py]
gdb_breakpoint [gdb_get_line_number "break backtrace-broken"]
+gdb_continue_to_breakpoint "break backtrace-broken"
+
+check_for_broken_backtrace "Backtrace is initially broken"
+
gdb_test "source ${pyfile}" "Python script imported" \
- "import python scripts"
+ "import python scripts"
-gdb_continue_to_breakpoint "break backtrace-broken"
-gdb_test_sequence "where" "Backtrace restored by unwinder" {
- "\\r\\n#0 .* corrupt_frame_inner \\(\\) at "
- "\\r\\n#1 .* corrupt_frame_outer \\(\\) at "
- "\\r\\n#2 .* main \\(.*\\) at"
-}
+check_info_unwinder "info unwinder after loading script" on
+
+check_for_fixed_backtrace "check backtrace after loading unwinder"
# Check that the Python unwinder frames can be flushed / released.
gdb_test "maint flush register-cache" "Register cache flushed\\." "flush frames"
+check_for_fixed_backtrace "check backtrace after flush"
+
+# Try to disable the unwinder but instead set the enabled field to a
+# non boolean value. This should fail. Check the 'info unwinder'
+# output to be sure.
+gdb_test "python global_test_unwinder.enabled = \"off\"" \
+ [multi_line \
+ "TypeError: incorrect type for enabled attribute: <class 'str'>" \
+ "Error while executing Python code\\."]
+check_info_unwinder "info unwinder after failed disable" on
+
+# While we're doing silly stuff, lets try to change the name of this
+# unwider. Doing this is bad as the new name might clash with an
+# already registered name, which violates the promises made during
+# 'register_unwinder'.
+gdb_test "python global_test_unwinder.name = \"foo\"" \
+ [multi_line \
+ "AttributeError: can't set attribute" \
+ "Error while executing Python code\\."]
+check_info_unwinder "info unwinder after failed name change" on
+
+# Now actually disable the unwinder by manually adjusting the
+# 'enabled' attribute. Check that the stack is once again broken, and
+# that the unwinder shows as disabled in the 'info unwinder' output.
+gdb_test_no_output "python global_test_unwinder.enabled = False"
+check_for_broken_backtrace "stack is broken after disabling"
+check_info_unwinder "info unwinder after manually disabling" off
+
+# Now enable the unwinder using the 'enable unwinder' command.
+gdb_test "enable unwinder global \"test unwinder\"" \
+ "1 unwinder enabled"
+check_for_fixed_backtrace "check backtrace after enabling with command"
+check_info_unwinder "info unwinder after command enabled" on
+
+# And now disable using the command and check the stack is once again
+# broken, and that the 'info unwinder' output updates correctly.
+gdb_test "disable unwinder global \"test unwinder\"" \
+ "1 unwinder disabled"
+check_for_broken_backtrace "stack is broken after command disabling"
+check_info_unwinder "info unwinder after command disabling" off
+
# Check that invalid register names cause errors.
gdb_test "python print(add_saved_register_error)" "True" \
"add_saved_register error"
gdb_test "python print(read_register_error)" "True" \
"read_register error"
+
+# Try to create an unwinder object with a non-string name.
+gdb_test "python obj = simple_unwinder(True)" \
+ [multi_line \
+ "TypeError: incorrect type for name: <class 'bool'>" \
+ "Error while executing Python code\\."]
diff --git a/gdb/testsuite/gdb.python/py-unwind.py b/gdb/testsuite/gdb.python/py-unwind.py
index f7ad8e45a35..edd2e30eb9b 100644
--- a/gdb/testsuite/gdb.python/py-unwind.py
+++ b/gdb/testsuite/gdb.python/py-unwind.py
@@ -130,5 +130,16 @@ class TestUnwinder(Unwinder):
return None
-gdb.unwinder.register_unwinder(None, TestUnwinder(), True)
+global_test_unwinder = TestUnwinder()
+gdb.unwinder.register_unwinder(None, global_test_unwinder, True)
+
+
+class simple_unwinder(Unwinder):
+ def __init__(self, name):
+ super().__init__(name)
+
+ def __call__(self, pending_frame):
+ return None
+
+
print("Python script imported")
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 02/10] gdb/python: make the gdb.unwinder.Unwinder class more robust
2023-03-14 10:06 ` Andrew Burgess
@ 2023-03-14 12:57 ` Eli Zaretskii
0 siblings, 0 replies; 37+ messages in thread
From: Eli Zaretskii @ 2023-03-14 12:57 UTC (permalink / raw)
To: Andrew Burgess; +Cc: gdb-patches
> From: Andrew Burgess <aburgess@redhat.com>
> Cc: gdb-patches@sourceware.org
> Date: Tue, 14 Mar 2023 10:06:22 +0000
>
> > Reviewed-By: Eli Zaretskii <eliz@gnu.org>
>
> I added your tag, but the updated patch is below if you want to take
> another look.
OK to install the documentation parts.
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 02/10] gdb/python: make the gdb.unwinder.Unwinder class more robust
2023-03-10 14:55 ` [PATCH 02/10] gdb/python: make the gdb.unwinder.Unwinder class more robust Andrew Burgess
2023-03-10 15:32 ` Eli Zaretskii
@ 2023-03-31 2:15 ` Simon Marchi
2023-04-03 10:02 ` Andrew Burgess
1 sibling, 1 reply; 37+ messages in thread
From: Simon Marchi @ 2023-03-31 2:15 UTC (permalink / raw)
To: Andrew Burgess, gdb-patches
On 3/10/23 09:55, Andrew Burgess via Gdb-patches wrote:
> This commit makes a few related changes to the gdb.unwinder.Unwinder
> class attributes:
>
> 1. The 'name' attribute is now a read-only attribute. This prevents
> user code from changing the name after registering the unwinder. It
> seems very unlikely that any user is actually trying to do this in
> the wild, so I'm not very worried that this will upset anyone,
>
> 2. We now validate that the name is a string in the
> Unwinder.__init__ method, and throw an error if this is not the
> case. Hopefully nobody was doing this in the wild. This should
> make it easier to ensure the 'info unwinder' command shows sane
> output (how to display a non-string name for an unwinder?),
>
> 3. The 'enabled' attribute is now implemented with a getter and
> setter. In the setter we ensure that the new value is a boolean,
> but the real important change is that we call
> 'gdb.invalidate_cached_frames()'. This means that the backtrace
> will be updated if a user manually disables an unwinder (rather than
> calling the 'disable unwinder' command). It is not unreasonable to
> think that a user might register multiple unwinders (relating to
> some project) and have one command that disables/enables all the
> related unwinders. This command might operate by poking the enabled
> attribute of each unwinder object directly, after this commit, this
> would now work correctly.
>
> There's tests for all the changes, and lots of documentation updates
> that both cover the new changes, but also further improve (I think)
> the general documentation for GDB's Unwinder API.
Hi Andrew,
With this commit, I see:
python global_test_unwinder.name = "foo"^M
Traceback (most recent call last):^M
File "<string>", line 1, in <module>^M
AttributeError: can't set attribute 'name'^M
Error while executing Python code.^M
(gdb) FAIL: gdb.python/py-unwind.exp: python global_test_unwinder.name = "foo"
Simon
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 02/10] gdb/python: make the gdb.unwinder.Unwinder class more robust
2023-03-31 2:15 ` Simon Marchi
@ 2023-04-03 10:02 ` Andrew Burgess
0 siblings, 0 replies; 37+ messages in thread
From: Andrew Burgess @ 2023-04-03 10:02 UTC (permalink / raw)
To: Simon Marchi, gdb-patches
Simon Marchi <simark@simark.ca> writes:
> On 3/10/23 09:55, Andrew Burgess via Gdb-patches wrote:
>> This commit makes a few related changes to the gdb.unwinder.Unwinder
>> class attributes:
>>
>> 1. The 'name' attribute is now a read-only attribute. This prevents
>> user code from changing the name after registering the unwinder. It
>> seems very unlikely that any user is actually trying to do this in
>> the wild, so I'm not very worried that this will upset anyone,
>>
>> 2. We now validate that the name is a string in the
>> Unwinder.__init__ method, and throw an error if this is not the
>> case. Hopefully nobody was doing this in the wild. This should
>> make it easier to ensure the 'info unwinder' command shows sane
>> output (how to display a non-string name for an unwinder?),
>>
>> 3. The 'enabled' attribute is now implemented with a getter and
>> setter. In the setter we ensure that the new value is a boolean,
>> but the real important change is that we call
>> 'gdb.invalidate_cached_frames()'. This means that the backtrace
>> will be updated if a user manually disables an unwinder (rather than
>> calling the 'disable unwinder' command). It is not unreasonable to
>> think that a user might register multiple unwinders (relating to
>> some project) and have one command that disables/enables all the
>> related unwinders. This command might operate by poking the enabled
>> attribute of each unwinder object directly, after this commit, this
>> would now work correctly.
>>
>> There's tests for all the changes, and lots of documentation updates
>> that both cover the new changes, but also further improve (I think)
>> the general documentation for GDB's Unwinder API.
>
> Hi Andrew,
>
> With this commit, I see:
>
> python global_test_unwinder.name = "foo"^M
> Traceback (most recent call last):^M
> File "<string>", line 1, in <module>^M
> AttributeError: can't set attribute 'name'^M
> Error while executing Python code.^M
> (gdb) FAIL: gdb.python/py-unwind.exp: python global_test_unwinder.name = "foo"
Sorry about that. Turns out that older version of Python don't include
the attribute name in the error message, I was seeing this:
(gdb) python global_test_unwinder.name = "foo"
Traceback (most recent call last):
File "<string>", line 1, in <module>
AttributeError: can't set attribute
Error while executing Python code.
(gdb)
I've updated the test pattern so either is acceptable. I went ahead and
pushed this fix.
Thanks,
Andrew
---
commit 4c148f65fc148918d0be15607938770ad8c46e36
Author: Andrew Burgess <aburgess@redhat.com>
Date: Mon Apr 3 10:56:10 2023 +0100
gdb/testsuite: fix failure in gdb.python/py-unwind.exp
A potential test failure was introduced with commit:
commit 6bf5f25bb150c0fbcb125e3ee466ba8f9680310b
Date: Wed Mar 8 16:11:30 2023 +0000
gdb/python: make the gdb.unwinder.Unwinder class more robust
In this commit a new test was added, however the expected output
pattern varies depending on which Python version GDB is linked
against.
Older versions of Python result in output like this:
(gdb) python global_test_unwinder.name = "foo"
Traceback (most recent call last):
File "<string>", line 1, in <module>
AttributeError: can't set attribute
Error while executing Python code.
(gdb)
While more recent versions of Python give a similar, but slightly more
verbose error message, like this:
(gdb) python global_test_unwinder.name = "foo"
Traceback (most recent call last):
File "<string>", line 1, in <module>
AttributeError: can't set attribute 'name'
Error while executing Python code.
(gdb)
The test was only accepting the first version of the output. This
commit extends the test pattern so that either version will be
accepted.
diff --git a/gdb/testsuite/gdb.python/py-unwind.exp b/gdb/testsuite/gdb.python/py-unwind.exp
index fddf4f15393..d0a1960058b 100644
--- a/gdb/testsuite/gdb.python/py-unwind.exp
+++ b/gdb/testsuite/gdb.python/py-unwind.exp
@@ -107,7 +107,7 @@ check_info_unwinder "info unwinder after failed disable" on
# 'register_unwinder'.
gdb_test "python global_test_unwinder.name = \"foo\"" \
[multi_line \
- "AttributeError: can't set attribute" \
+ "AttributeError: can't set attribute(?: 'name')?" \
"Error while executing Python code\\."]
check_info_unwinder "info unwinder after failed name change" on
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 03/10] gdb/python: remove unneeded nullptr check in frapy_block
2023-03-10 14:55 [PATCH 00/10] Improvements & Cleanup For Python Unwinder API Andrew Burgess
2023-03-10 14:55 ` [PATCH 01/10] gdb/doc: spring clean the Python unwinders documentation Andrew Burgess
2023-03-10 14:55 ` [PATCH 02/10] gdb/python: make the gdb.unwinder.Unwinder class more robust Andrew Burgess
@ 2023-03-10 14:55 ` Andrew Burgess
2023-03-10 14:55 ` [PATCH 04/10] gdb/python: add PENDING_FRAMEPY_REQUIRE_VALID macro in py-unwind.c Andrew Burgess
` (7 subsequent siblings)
10 siblings, 0 replies; 37+ messages in thread
From: Andrew Burgess @ 2023-03-10 14:55 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess
Spotted a redundant nullptr check in python/py-frame.c in the function
frapy_block. This was introduced in commit 57126e4a45e3000e when we
expanded an earlier check in return early if the pointer in question
is nullptr.
There should be no user visible changes after this commit.
---
gdb/python/py-frame.c | 8 +-------
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/gdb/python/py-frame.c b/gdb/python/py-frame.c
index ecd55d2e568..00cd4bee492 100644
--- a/gdb/python/py-frame.c
+++ b/gdb/python/py-frame.c
@@ -304,13 +304,7 @@ frapy_block (PyObject *self, PyObject *args)
return NULL;
}
- if (block)
- {
- return block_to_block_object
- (block, fn_block->function ()->objfile ());
- }
-
- Py_RETURN_NONE;
+ return block_to_block_object (block, fn_block->function ()->objfile ());
}
--
2.25.4
^ permalink raw reply [flat|nested] 37+ messages in thread* [PATCH 04/10] gdb/python: add PENDING_FRAMEPY_REQUIRE_VALID macro in py-unwind.c
2023-03-10 14:55 [PATCH 00/10] Improvements & Cleanup For Python Unwinder API Andrew Burgess
` (2 preceding siblings ...)
2023-03-10 14:55 ` [PATCH 03/10] gdb/python: remove unneeded nullptr check in frapy_block Andrew Burgess
@ 2023-03-10 14:55 ` Andrew Burgess
2023-03-10 14:55 ` [PATCH 05/10] gdb/python: add some additional methods to gdb.PendingFrame Andrew Burgess
` (6 subsequent siblings)
10 siblings, 0 replies; 37+ messages in thread
From: Andrew Burgess @ 2023-03-10 14:55 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess
This commit copies the pattern that is present in many other py-*.c
files: having a single macro to check that the Python object is still
valid.
This cleans up the code a little throughout the py-unwind.c file.
Some of the exception messages will change slightly with this commit,
though the type of the exceptions is still ValueError in all cases.
I started writing some tests for this change and immediately ran into
a problem: GDB would crash. It turns out that the PendingFrame
objects are not being marked as invalid!
In pyuw_sniffer where the pending frames are created, we make use of a
scoped_restore to invalidate the pending frame objects. However, this
only restores the pending_frame_object::frame_info field to its
previous value -- and it turns out we never actually give this field
an initial value, it's left undefined.
So, when the scoped_restore (called invalidate_frame) performs its
cleanup, it actually restores the frame_info field to an undefined
value. If this undefined value is not nullptr then any future
accesses to the PendingFrame object result in undefined behaviour and
most likely, a crash.
As part of this commit I now initialize the frame_info field, which
ensures all the new tests now pass.
---
gdb/python/py-unwind.c | 53 +++++++++++++-------------
gdb/testsuite/gdb.python/py-unwind.exp | 19 +++++++++
gdb/testsuite/gdb.python/py-unwind.py | 7 ++++
3 files changed, 53 insertions(+), 26 deletions(-)
diff --git a/gdb/python/py-unwind.c b/gdb/python/py-unwind.c
index ee776bf8dea..12b14616363 100644
--- a/gdb/python/py-unwind.c
+++ b/gdb/python/py-unwind.c
@@ -52,6 +52,17 @@ show_pyuw_debug (struct ui_file *file, int from_tty,
#define PYUW_SCOPED_DEBUG_ENTER_EXIT \
scoped_debug_enter_exit (pyuw_debug, "py-unwind")
+/* Require a valid pending frame. */
+#define PENDING_FRAMEPY_REQUIRE_VALID(pending_frame) \
+ do { \
+ if ((pending_frame)->frame_info == nullptr) \
+ { \
+ PyErr_SetString (PyExc_ValueError, \
+ _("gdb.PendingFrame is invalid.")); \
+ return nullptr; \
+ } \
+ } while (0)
+
struct pending_frame_object
{
PyObject_HEAD
@@ -215,21 +226,20 @@ unwind_infopy_str (PyObject *self)
}
/* Create UnwindInfo instance for given PendingFrame and frame ID.
- Sets Python error and returns NULL on error. */
+ Sets Python error and returns NULL on error.
+
+ The PYO_PENDING_FRAME object must be valid. */
static PyObject *
pyuw_create_unwind_info (PyObject *pyo_pending_frame,
struct frame_id frame_id)
{
+ gdb_assert (((pending_frame_object *) pyo_pending_frame)->frame_info
+ != nullptr);
+
unwind_info_object *unwind_info
- = PyObject_New (unwind_info_object, &unwind_info_object_type);
+ = PyObject_New (unwind_info_object, &unwind_info_object_type);
- if (((pending_frame_object *) pyo_pending_frame)->frame_info == NULL)
- {
- PyErr_SetString (PyExc_ValueError,
- "Attempting to use stale PendingFrame");
- return NULL;
- }
unwind_info->frame_id = frame_id;
Py_INCREF (pyo_pending_frame);
unwind_info->pending_frame = pyo_pending_frame;
@@ -365,15 +375,11 @@ static PyObject *
pending_framepy_read_register (PyObject *self, PyObject *args)
{
pending_frame_object *pending_frame = (pending_frame_object *) self;
+ PENDING_FRAMEPY_REQUIRE_VALID (pending_frame);
+
int regnum;
PyObject *pyo_reg_id;
- if (pending_frame->frame_info == NULL)
- {
- PyErr_SetString (PyExc_ValueError,
- "Attempting to read register from stale PendingFrame");
- return NULL;
- }
if (!PyArg_UnpackTuple (args, "read_register", 1, 1, &pyo_reg_id))
return NULL;
if (!gdbpy_parse_register_id (pending_frame->gdbarch, pyo_reg_id, ®num))
@@ -417,6 +423,8 @@ pending_framepy_create_unwind_info (PyObject *self, PyObject *args)
CORE_ADDR pc;
CORE_ADDR special;
+ PENDING_FRAMEPY_REQUIRE_VALID ((pending_frame_object *) self);
+
if (!PyArg_ParseTuple (args, "O:create_unwind_info", &pyo_frame_id))
return NULL;
if (!pyuw_object_attribute_to_pointer (pyo_frame_id, "sp", &sp))
@@ -451,12 +459,8 @@ pending_framepy_architecture (PyObject *self, PyObject *args)
{
pending_frame_object *pending_frame = (pending_frame_object *) self;
- if (pending_frame->frame_info == NULL)
- {
- PyErr_SetString (PyExc_ValueError,
- "Attempting to read register from stale PendingFrame");
- return NULL;
- }
+ PENDING_FRAMEPY_REQUIRE_VALID (pending_frame);
+
return gdbarch_to_arch_object (pending_frame->gdbarch);
}
@@ -467,12 +471,8 @@ pending_framepy_level (PyObject *self, PyObject *args)
{
pending_frame_object *pending_frame = (pending_frame_object *) self;
- if (pending_frame->frame_info == NULL)
- {
- PyErr_SetString (PyExc_ValueError,
- "Attempting to read stack level from stale PendingFrame");
- return NULL;
- }
+ PENDING_FRAMEPY_REQUIRE_VALID (pending_frame);
+
int level = frame_relative_level (pending_frame->frame_info);
return gdb_py_object_from_longest (level).release ();
}
@@ -538,6 +538,7 @@ pyuw_sniffer (const struct frame_unwind *self, frame_info_ptr this_frame,
return 0;
}
pfo->gdbarch = gdbarch;
+ pfo->frame_info = nullptr;
scoped_restore invalidate_frame = make_scoped_restore (&pfo->frame_info,
this_frame);
diff --git a/gdb/testsuite/gdb.python/py-unwind.exp b/gdb/testsuite/gdb.python/py-unwind.exp
index 337e5dc2504..3e214ee0f45 100644
--- a/gdb/testsuite/gdb.python/py-unwind.exp
+++ b/gdb/testsuite/gdb.python/py-unwind.exp
@@ -142,3 +142,22 @@ gdb_test "python obj = simple_unwinder(True)" \
[multi_line \
"TypeError: incorrect type for name: <class 'bool'>" \
"Error while executing Python code\\."]
+
+# Now register the simple_unwinder with a valid name, and use the
+# unwinder to capture a PendingFrame object.
+gdb_test_no_output "python obj = simple_unwinder(\"simple\")"
+gdb_test_no_output "python gdb.unwinder.register_unwinder(None, obj)"
+check_for_broken_backtrace "backtrace to capture a PendingFrame object"
+
+# Call methods on the captured gdb.PendingFrame and check we see the
+# expected error.
+gdb_test_no_output "python pf = captured_pending_frame"
+foreach cmd {"pf.read_register(\"pc\")" \
+ "pf.create_unwind_info(None)" \
+ "pf.architecture()" \
+ "pf.level()"} {
+ gdb_test "python $cmd" \
+ [multi_line \
+ "ValueError: gdb\\.PendingFrame is invalid\\." \
+ "Error while executing Python code\\."]
+}
diff --git a/gdb/testsuite/gdb.python/py-unwind.py b/gdb/testsuite/gdb.python/py-unwind.py
index edd2e30eb9b..b30e843e7e5 100644
--- a/gdb/testsuite/gdb.python/py-unwind.py
+++ b/gdb/testsuite/gdb.python/py-unwind.py
@@ -133,12 +133,19 @@ class TestUnwinder(Unwinder):
global_test_unwinder = TestUnwinder()
gdb.unwinder.register_unwinder(None, global_test_unwinder, True)
+# This is filled in by the simple_unwinder class.
+captured_pending_frame = None
+
class simple_unwinder(Unwinder):
def __init__(self, name):
super().__init__(name)
def __call__(self, pending_frame):
+ global captured_pending_frame
+
+ if captured_pending_frame is None:
+ captured_pending_frame = pending_frame
return None
--
2.25.4
^ permalink raw reply [flat|nested] 37+ messages in thread* [PATCH 05/10] gdb/python: add some additional methods to gdb.PendingFrame
2023-03-10 14:55 [PATCH 00/10] Improvements & Cleanup For Python Unwinder API Andrew Burgess
` (3 preceding siblings ...)
2023-03-10 14:55 ` [PATCH 04/10] gdb/python: add PENDING_FRAMEPY_REQUIRE_VALID macro in py-unwind.c Andrew Burgess
@ 2023-03-10 14:55 ` Andrew Burgess
2023-03-10 15:42 ` Eli Zaretskii
2023-03-10 14:55 ` [PATCH 06/10] gdb/python: add __repr__ for PendingFrame and UnwindInfo Andrew Burgess
` (5 subsequent siblings)
10 siblings, 1 reply; 37+ messages in thread
From: Andrew Burgess @ 2023-03-10 14:55 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess
The gdb.Frame class has far more methods than gdb.PendingFrame. Given
that a PendingFrame hasn't yet been claimed by an unwinder, there is a
limit to which methods we can add to it, but many of the methods that
the Frame class has, the PendingFrame class could also support.
In this commit I've added those methods to PendingFrame that I believe
are safe.
In terms of implementation: if I was starting from scratch then I
would implement many of these (or most of these) as attributes rather
than methods. However, given both Frame and PendingFrame are just
different representation of a frame, I think there is value in keeping
the interface for the two classes the same. For this reason
everything here is a method -- that's what the Frame class does.
The new methods I've added are:
- gdb.PendingFrame.is_valid(): Return True if the pending frame
object is valid.
- gdb.PendingFrame.name(): Return the name for the frame's function,
or None.
- gdb.PendingFrame.pc(): Return the $pc register value for this
frame.
- gdb.PendingFrame.language(): Return a string containing the
language for this frame, or None.
- gdb.PendingFrame.find_sal(): Return a gdb.Symtab_and_line object
for the current location within the pending frame, or None.
- gdb.PendingFrame.block(): Return a gdb.Block for the current
pending frame, or None.
- gdb.PendingFrame.function(): Return a gdb.Symbol for the current
pending frame, or None.
In every case I've just copied the implementation over from gdb.Frame
and cleaned the code slightly e.g. NULL to nullptr. Additionally each
function required a small update to reflect the PendingFrame type, but
that's pretty minor.
There are tests for all the new methods.
For more extensive testing, I added the following code to the file
gdb/python/lib/command/unwinders.py:
from gdb.unwinder import Unwinder
class TestUnwinder(Unwinder):
def __init__(self):
super().__init__("XXX_TestUnwinder_XXX")
def __call__(self,pending_frame):
lang = pending_frame.language()
try:
block = pending_frame.block()
assert isinstance(block, gdb.Block)
except RuntimeError as rte:
assert str(rte) == "Cannot locate block for frame."
function = pending_frame.function()
arch = pending_frame.architecture()
assert arch is None or isinstance(arch, gdb.Architecture)
name = pending_frame.name()
assert name is None or isinstance(name, str)
valid = pending_frame.is_valid()
pc = pending_frame.pc()
sal = pending_frame.find_sal()
assert sal is None or isinstance(sal, gdb.Symtab_and_line)
return None
gdb.unwinder.register_unwinder(None, TestUnwinder())
This registers a global unwinder that calls each of the new
PendingFrame methods and checks the result is of an acceptable type.
The unwinder never claims any frames though, so shouldn't change how
GDB actually behaves.
I then ran the testsuite. There was only a single regression, a test
that uses 'disable unwinder' and expects a single unwinder to be
disabled -- the extra unwinder is now disabled too, which changes the
test output. So I'm reasonably confident that the new methods are not
going to crash GDB.
---
gdb/NEWS | 20 +++
gdb/doc/python.texi | 40 +++++
gdb/python/py-unwind.c | 221 +++++++++++++++++++++++++
gdb/testsuite/gdb.python/py-unwind.exp | 33 +++-
gdb/testsuite/gdb.python/py-unwind.py | 104 ++++++++++++
5 files changed, 417 insertions(+), 1 deletion(-)
diff --git a/gdb/NEWS b/gdb/NEWS
index ed0f86e79ec..0d9049ff134 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -110,6 +110,26 @@ show always-read-ctf
without the particular unwinder, depending on how 'enabled' was
changed.
+ ** New methods added to the gdb.PendingFrame class. These methods
+ have the same behaviour as the corresponding methods on
+ gdb.Frame. The new methods are:
+
+ - gdb.PendingFrame.name(): Return the name for the frame's
+ function, or None.
+ - gdb.PendingFrame.is_valid(): Return True if the pending frame
+ object is valid.
+ - gdb.PendingFrame.pc(): Return the $pc register value for this
+ frame.
+ - gdb.PendingFrame.language(): Return a string containing the
+ language for this frame, or None.
+ - gdb.PendingFrame.find_sal(): Return a gdb.Symtab_and_line
+ object for the current location within the pending frame, or
+ None.
+ - gdb.PendingFrame.block(): Return a gdb.Block for the current
+ pending frame, or None.
+ - gdb.PendingFrame.function(): Return a gdb.Symbol for the
+ current pending frame, or None.
+
*** Changes in GDB 13
* MI version 1 is deprecated, and will be removed in GDB 14.
diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index c36130210ba..1c4239841af 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -2830,6 +2830,46 @@
@xref{Frames, ,Stack Frames}.
@end defun
+@defun PendingFrame.name ()
+Returns the function name of this pending frame, or @code{None} if it
+can't be obtained.
+@end defun
+
+@defun PendingFrame.is_valid ()
+Returns true if the @code{gdb.PendingFrame} object is valid, false if
+not. A pending frame object becomes invalid when the call to the
+unwinder, for which the pending frame was created, returns.
+
+All @code{gdb.PendingFrame} methods, except this one, will raise an
+exception if the pending frame object is invalid at the time the
+method is called.
+@end defun
+
+@defun PendingFrame.pc ()
+Returns the pending frame's resume address.
+@end defun
+
+@defun PendingFrame.block ()
+Return the pending frame's code block (@pxref{Blocks In Python}). If
+the frame does not have a block -- for example, if there is no
+debugging information for the code in question -- then this will raise
+a @code{RuntimeError} exception.
+@end defun
+
+@defun PendingFrame.function ()
+Return the symbol for the function corresponding to this pending frame.
+@xref{Symbols In Python}.
+@end defun
+
+@defun PendingFrame.find_sal ()
+Return the pending frame's symtab and line object (@pxref{Symbol
+Tables In Python}).
+@end defun
+
+@defun PendingFrame.language ()
+Return a string, the source language for this pending frame.
+@end defun
+
@subheading Unwinder Output: UnwindInfo
Use @code{PendingFrame.create_unwind_info} method described above to
diff --git a/gdb/python/py-unwind.c b/gdb/python/py-unwind.c
index 12b14616363..5ed3ff37e64 100644
--- a/gdb/python/py-unwind.c
+++ b/gdb/python/py-unwind.c
@@ -28,6 +28,10 @@
#include "regcache.h"
#include "valprint.h"
#include "user-regs.h"
+#include "stack.h"
+#include "charset.h"
+#include "block.h"
+
/* Debugging of Python unwinders. */
@@ -412,6 +416,200 @@ pending_framepy_read_register (PyObject *self, PyObject *args)
return result;
}
+/* Implement PendingFrame.is_valid(). Return True if this pending frame
+ object is still valid. */
+
+static PyObject *
+pending_framepy_is_valid (PyObject *self, PyObject *args)
+{
+ pending_frame_object *pending_frame = (pending_frame_object *) self;
+
+ if (pending_frame->frame_info == nullptr)
+ Py_RETURN_FALSE;
+
+ Py_RETURN_TRUE;
+}
+
+/* Implement PendingFrame.name(). Return a string that is the name of the
+ function for this frame, or None if the name can't be found. */
+
+static PyObject *
+pending_framepy_name (PyObject *self, PyObject *args)
+{
+ pending_frame_object *pending_frame = (pending_frame_object *) self;
+
+ PENDING_FRAMEPY_REQUIRE_VALID (pending_frame);
+
+ gdb::unique_xmalloc_ptr<char> name;
+
+ try
+ {
+ enum language lang;
+ frame_info_ptr frame = pending_frame->frame_info;
+
+ name = find_frame_funname (frame, &lang, nullptr);
+ }
+ catch (const gdb_exception &except)
+ {
+ GDB_PY_HANDLE_EXCEPTION (except);
+ }
+
+ if (name != nullptr)
+ return PyUnicode_Decode (name.get (), strlen (name.get ()),
+ host_charset (), nullptr);
+
+ Py_RETURN_NONE;
+}
+
+/* Implement gdb.PendingFrame.pc(). Returns an integer containing the
+ frame's current $pc value. */
+
+static PyObject *
+pending_framepy_pc (PyObject *self, PyObject *args)
+{
+ pending_frame_object *pending_frame = (pending_frame_object *) self;
+
+ PENDING_FRAMEPY_REQUIRE_VALID (pending_frame);
+
+ CORE_ADDR pc = 0;
+
+ try
+ {
+ pc = get_frame_pc (pending_frame->frame_info);
+ }
+ catch (const gdb_exception &except)
+ {
+ GDB_PY_HANDLE_EXCEPTION (except);
+ }
+
+ return gdb_py_object_from_ulongest (pc).release ();
+}
+
+/* Implement gdb.PendingFrame.language(). Return the name of the language
+ for this frame. */
+
+static PyObject *
+pending_framepy_language (PyObject *self, PyObject *args)
+{
+ pending_frame_object *pending_frame = (pending_frame_object *) self;
+
+ PENDING_FRAMEPY_REQUIRE_VALID (pending_frame);
+
+ try
+ {
+ frame_info_ptr fi = pending_frame->frame_info;
+
+ enum language lang = get_frame_language (fi);
+ const language_defn *lang_def = language_def (lang);
+
+ return host_string_to_python_string (lang_def->name ()).release ();
+ }
+ catch (const gdb_exception &except)
+ {
+ GDB_PY_HANDLE_EXCEPTION (except);
+ }
+
+ Py_RETURN_NONE;
+}
+
+/* Implement PendingFrame.find_sal(). Return the PendingFrame's symtab and
+ line. */
+
+static PyObject *
+pending_framepy_find_sal (PyObject *self, PyObject *args)
+{
+ pending_frame_object *pending_frame = (pending_frame_object *) self;
+
+ PENDING_FRAMEPY_REQUIRE_VALID (pending_frame);
+
+ PyObject *sal_obj = nullptr;
+
+ try
+ {
+ frame_info_ptr frame = pending_frame->frame_info;
+
+ symtab_and_line sal = find_frame_sal (frame);
+ sal_obj = symtab_and_line_to_sal_object (sal);
+ }
+ catch (const gdb_exception &except)
+ {
+ GDB_PY_HANDLE_EXCEPTION (except);
+ }
+
+ return sal_obj;
+}
+
+/* Implement PendingFrame.block(). Return a gdb.Block for the pending
+ frame's code, or raise RuntimeError if the block can't be found. */
+
+static PyObject *
+pending_framepy_block (PyObject *self, PyObject *args)
+{
+ pending_frame_object *pending_frame = (pending_frame_object *) self;
+
+ PENDING_FRAMEPY_REQUIRE_VALID (pending_frame);
+
+ frame_info_ptr frame = pending_frame->frame_info;
+ const struct block *block = nullptr, *fn_block;
+
+ try
+ {
+ block = get_frame_block (frame, nullptr);
+ }
+ catch (const gdb_exception &except)
+ {
+ GDB_PY_HANDLE_EXCEPTION (except);
+ }
+
+ for (fn_block = block;
+ fn_block != nullptr && fn_block->function () == nullptr;
+ fn_block = fn_block->superblock ())
+ ;
+
+ if (block == nullptr
+ || fn_block == nullptr
+ || fn_block->function () == nullptr)
+ {
+ PyErr_SetString (PyExc_RuntimeError,
+ _("Cannot locate block for frame."));
+ return nullptr;
+ }
+
+ return block_to_block_object (block, fn_block->function ()->objfile ());
+}
+
+/* Implement gdb.PendingFrame.function(). Return a gdb.Symbol
+ representing the function of this frame, or None if no suitable symbol
+ can be found. */
+
+static PyObject *
+pending_framepy_function (PyObject *self, PyObject *args)
+{
+ pending_frame_object *pending_frame = (pending_frame_object *) self;
+
+ PENDING_FRAMEPY_REQUIRE_VALID (pending_frame);
+
+ struct symbol *sym = nullptr;
+
+ try
+ {
+ enum language funlang;
+ frame_info_ptr frame = pending_frame->frame_info;
+
+ gdb::unique_xmalloc_ptr<char> funname
+ = find_frame_funname (frame, &funlang, &sym);
+ }
+ catch (const gdb_exception &except)
+ {
+ GDB_PY_HANDLE_EXCEPTION (except);
+ }
+
+ if (sym != nullptr)
+ return symbol_to_symbol_object (sym);
+
+ Py_RETURN_NONE;
+}
+
/* Implementation of
PendingFrame.create_unwind_info (self, frameId) -> UnwindInfo. */
@@ -737,6 +935,29 @@ static PyMethodDef pending_frame_object_methods[] =
pending_framepy_architecture, METH_NOARGS,
"architecture () -> gdb.Architecture\n"
"The architecture for this PendingFrame." },
+ { "name",
+ pending_framepy_name, METH_NOARGS,
+ "name() -> String.\n\
+Return the function name of the frame, or None if it can't be determined." },
+ { "is_valid",
+ pending_framepy_is_valid, METH_NOARGS,
+ "is_valid () -> Boolean.\n\
+Return true if this PendingFrame is valid, false if not." },
+ { "pc",
+ pending_framepy_pc, METH_NOARGS,
+ "pc () -> Long.\n\
+Return the frame's resume address." },
+ { "language", pending_framepy_language, METH_NOARGS,
+ "The language of this frame." },
+ { "find_sal", pending_framepy_find_sal, METH_NOARGS,
+ "find_sal () -> gdb.Symtab_and_line.\n\
+Return the frame's symtab and line." },
+ { "block", pending_framepy_block, METH_NOARGS,
+ "block () -> gdb.Block.\n\
+Return the frame's code block." },
+ { "function", pending_framepy_function, METH_NOARGS,
+ "function () -> gdb.Symbol.\n\
+Returns the symbol for the function corresponding to this frame." },
{ "level", pending_framepy_level, METH_NOARGS,
"The stack level of this frame." },
{NULL} /* Sentinel */
diff --git a/gdb/testsuite/gdb.python/py-unwind.exp b/gdb/testsuite/gdb.python/py-unwind.exp
index 3e214ee0f45..f670da5a7cd 100644
--- a/gdb/testsuite/gdb.python/py-unwind.exp
+++ b/gdb/testsuite/gdb.python/py-unwind.exp
@@ -149,15 +149,46 @@ gdb_test_no_output "python obj = simple_unwinder(\"simple\")"
gdb_test_no_output "python gdb.unwinder.register_unwinder(None, obj)"
check_for_broken_backtrace "backtrace to capture a PendingFrame object"
+# Check the captured PendingFrame is not valid.
+gdb_test "python print(captured_pending_frame.is_valid())" "False"
+
# Call methods on the captured gdb.PendingFrame and check we see the
# expected error.
gdb_test_no_output "python pf = captured_pending_frame"
foreach cmd {"pf.read_register(\"pc\")" \
"pf.create_unwind_info(None)" \
"pf.architecture()" \
- "pf.level()"} {
+ "pf.level()" \
+ "pf.name()" \
+ "pf.pc()" \
+ "pf.language()" \
+ "pf.find_sal()" \
+ "pf.block()" \
+ "pf.function()" } {
gdb_test "python $cmd" \
[multi_line \
"ValueError: gdb\\.PendingFrame is invalid\\." \
"Error while executing Python code\\."]
}
+
+# Turn on the useful unwinder so we have the full backtrace again, and
+# disable the simple unwinder -- because we can!
+gdb_test "enable unwinder global \"test unwinder\"" \
+ "1 unwinder enabled" \
+ "re-enable 'test unwinder' so we can check PendingFrame methods"
+gdb_test "disable unwinder global \"simple\"" \
+ "1 unwinder disabled"
+check_for_fixed_backtrace \
+ "check backtrace before testing PendingFrame methods"
+
+# Gather information about every frame.
+gdb_test_no_output "python capture_all_frame_information()"
+gdb_test_no_output "python gdb.newest_frame().select()"
+gdb_test_no_output "python pspace = gdb.selected_inferior().progspace"
+gdb_test_no_output "python obj = validating_unwinder()"
+gdb_test_no_output "python gdb.unwinder.register_unwinder(pspace, obj)"
+
+check_for_fixed_backtrace \
+ "check backtrace to validate all information"
+
+gdb_test_no_output "python check_all_frame_information_matched()"
diff --git a/gdb/testsuite/gdb.python/py-unwind.py b/gdb/testsuite/gdb.python/py-unwind.py
index b30e843e7e5..8e3c1f398bc 100644
--- a/gdb/testsuite/gdb.python/py-unwind.py
+++ b/gdb/testsuite/gdb.python/py-unwind.py
@@ -144,9 +144,113 @@ class simple_unwinder(Unwinder):
def __call__(self, pending_frame):
global captured_pending_frame
+ assert pending_frame.is_valid()
+
if captured_pending_frame is None:
captured_pending_frame = pending_frame
return None
+# Return a dictionary of information about FRAME.
+def capture_frame_information(frame):
+ name = frame.name()
+ level = frame.level()
+ language = frame.language()
+ function = frame.function()
+ architecture = frame.architecture()
+ pc = frame.pc()
+ sal = frame.find_sal()
+ try:
+ block = frame.block()
+ assert isinstance(block, gdb.Block)
+ except RuntimeError as rte:
+ assert str(rte) == "Cannot locate block for frame."
+ block = "RuntimeError: " + str(rte)
+
+ return {
+ "name": name,
+ "level": level,
+ "language": language,
+ "function": function,
+ "architecture": architecture,
+ "pc": pc,
+ "sal": sal,
+ "block": block,
+ }
+
+
+# List of information about each frame. The index into this list is
+# the frame level. This is populated by
+# capture_all_frame_information.
+all_frame_information = []
+
+# Fill in the global ALL_FRAME_INFORMATION list.
+def capture_all_frame_information():
+ global all_frame_information
+
+ all_frame_information = []
+
+ gdb.newest_frame().select()
+ frame = gdb.selected_frame()
+ count = 0
+
+ while frame is not None:
+ frame.select()
+ info = capture_frame_information(frame)
+ level = info["level"]
+ info["matched"] = False
+
+ while len(all_frame_information) <= level:
+ all_frame_information.append(None)
+
+ assert all_frame_information[level] is None
+ all_frame_information[level] = info
+
+ if frame.name == "main" or count > 10:
+ break
+
+ count += 1
+ frame = frame.older()
+
+
+# Assert that every entry in the global ALL_FRAME_INFORMATION list was
+# matched by the validating_unwinder.
+def check_all_frame_information_matched():
+ global all_frame_information
+ for entry in all_frame_information:
+ assert entry["matched"]
+
+
+# An unwinder that doesn't match any frames. What it does do is
+# lookup information from the PendingFrame object and compare it
+# against information stored in the global ALL_FRAME_INFORMATION list.
+class validating_unwinder(Unwinder):
+ def __init__(self):
+ super().__init__("validating_unwinder")
+
+ def __call__(self, pending_frame):
+ info = capture_frame_information(pending_frame)
+ level = info["level"]
+
+ global all_frame_information
+ old_info = all_frame_information[level]
+
+ assert old_info is not None
+ assert not old_info["matched"]
+
+ for key, value in info.items():
+ assert key in old_info, f"{key} not in old_info"
+ assert type(value) == type(old_info[key])
+ if isinstance(value, gdb.Block):
+ assert value.start == old_info[key].start
+ assert value.end == old_info[key].end
+ assert value.is_static == old_info[key].is_static
+ assert value.is_global == old_info[key].is_global
+ else:
+ assert str(value) == str(old_info[key])
+
+ old_info["matched"] = True
+ return None
+
+
print("Python script imported")
--
2.25.4
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 05/10] gdb/python: add some additional methods to gdb.PendingFrame
2023-03-10 14:55 ` [PATCH 05/10] gdb/python: add some additional methods to gdb.PendingFrame Andrew Burgess
@ 2023-03-10 15:42 ` Eli Zaretskii
2023-03-14 10:18 ` Andrew Burgess
0 siblings, 1 reply; 37+ messages in thread
From: Eli Zaretskii @ 2023-03-10 15:42 UTC (permalink / raw)
To: Andrew Burgess; +Cc: gdb-patches
> Cc: Andrew Burgess <aburgess@redhat.com>
> Date: Fri, 10 Mar 2023 14:55:22 +0000
> From: Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org>
>
> gdb/NEWS | 20 +++
> gdb/doc/python.texi | 40 +++++
> gdb/python/py-unwind.c | 221 +++++++++++++++++++++++++
> gdb/testsuite/gdb.python/py-unwind.exp | 33 +++-
> gdb/testsuite/gdb.python/py-unwind.py | 104 ++++++++++++
> 5 files changed, 417 insertions(+), 1 deletion(-)
Thanks.
> diff --git a/gdb/NEWS b/gdb/NEWS
> index ed0f86e79ec..0d9049ff134 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -110,6 +110,26 @@ show always-read-ctf
> without the particular unwinder, depending on how 'enabled' was
> changed.
>
> + ** New methods added to the gdb.PendingFrame class. These methods
> + have the same behaviour as the corresponding methods on
> + gdb.Frame. The new methods are:
> +
> + - gdb.PendingFrame.name(): Return the name for the frame's
> + function, or None.
> + - gdb.PendingFrame.is_valid(): Return True if the pending frame
> + object is valid.
> + - gdb.PendingFrame.pc(): Return the $pc register value for this
> + frame.
> + - gdb.PendingFrame.language(): Return a string containing the
> + language for this frame, or None.
"containing the language"? I think this is better:
Return the language of this frame, as a string, or None.
> + - gdb.PendingFrame.find_sal(): Return a gdb.Symtab_and_line
> + object for the current location within the pending frame, or
> + None.
> + - gdb.PendingFrame.block(): Return a gdb.Block for the current
> + pending frame, or None.
> + - gdb.PendingFrame.function(): Return a gdb.Symbol for the
> + current pending frame, or None.
Btw, why do you follow each method name with a "()"? That looks like
a call with no arguments, which is not what you mean, right?
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 05/10] gdb/python: add some additional methods to gdb.PendingFrame
2023-03-10 15:42 ` Eli Zaretskii
@ 2023-03-14 10:18 ` Andrew Burgess
2023-03-14 12:59 ` Eli Zaretskii
0 siblings, 1 reply; 37+ messages in thread
From: Andrew Burgess @ 2023-03-14 10:18 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: gdb-patches
Eli Zaretskii <eliz@gnu.org> writes:
>> Cc: Andrew Burgess <aburgess@redhat.com>
>> Date: Fri, 10 Mar 2023 14:55:22 +0000
>> From: Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org>
>>
>> gdb/NEWS | 20 +++
>> gdb/doc/python.texi | 40 +++++
>> gdb/python/py-unwind.c | 221 +++++++++++++++++++++++++
>> gdb/testsuite/gdb.python/py-unwind.exp | 33 +++-
>> gdb/testsuite/gdb.python/py-unwind.py | 104 ++++++++++++
>> 5 files changed, 417 insertions(+), 1 deletion(-)
>
> Thanks.
>
>> diff --git a/gdb/NEWS b/gdb/NEWS
>> index ed0f86e79ec..0d9049ff134 100644
>> --- a/gdb/NEWS
>> +++ b/gdb/NEWS
>> @@ -110,6 +110,26 @@ show always-read-ctf
>> without the particular unwinder, depending on how 'enabled' was
>> changed.
>>
>> + ** New methods added to the gdb.PendingFrame class. These methods
>> + have the same behaviour as the corresponding methods on
>> + gdb.Frame. The new methods are:
>> +
>> + - gdb.PendingFrame.name(): Return the name for the frame's
>> + function, or None.
>> + - gdb.PendingFrame.is_valid(): Return True if the pending frame
>> + object is valid.
>> + - gdb.PendingFrame.pc(): Return the $pc register value for this
>> + frame.
>> + - gdb.PendingFrame.language(): Return a string containing the
>> + language for this frame, or None.
>
> "containing the language"? I think this is better:
>
> Return the language of this frame, as a string, or None.
Changed to use your wording.
>
>> + - gdb.PendingFrame.find_sal(): Return a gdb.Symtab_and_line
>> + object for the current location within the pending frame, or
>> + None.
>> + - gdb.PendingFrame.block(): Return a gdb.Block for the current
>> + pending frame, or None.
>> + - gdb.PendingFrame.function(): Return a gdb.Symbol for the
>> + current pending frame, or None.
>
> Btw, why do you follow each method name with a "()"? That looks like
> a call with no arguments, which is not what you mean, right?
I did indeed mean a call with no arguments. These would be used like
this:
class TestUnwinder(Unwinder):
def __init__(self):
super().__init__("Unwinder_Name")
def __call__(self, pending_frame):
is_valid = pending_frame.is_valid()
name = pending_frame.name()
pc = pending_frame.pc()
language = pending_frame.language()
sal = pending_frame.find_sal()
block = pending_frame.block()
function = pending_frame.function()
Maybe you thought these would be better implemented as attributes? If
you did then I 100% agree, but I think these have to be methods in order
to match the existing Frame API. If we use the same API for
PendingFrame and Frame then a user can write code that will work on
either object type.
Does this address your concerns? Or did I not understand?
Thanks,
Andrew
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 05/10] gdb/python: add some additional methods to gdb.PendingFrame
2023-03-14 10:18 ` Andrew Burgess
@ 2023-03-14 12:59 ` Eli Zaretskii
2023-03-16 14:28 ` Andrew Burgess
0 siblings, 1 reply; 37+ messages in thread
From: Eli Zaretskii @ 2023-03-14 12:59 UTC (permalink / raw)
To: Andrew Burgess; +Cc: gdb-patches
> From: Andrew Burgess <aburgess@redhat.com>
> Cc: gdb-patches@sourceware.org
> Date: Tue, 14 Mar 2023 10:18:09 +0000
>
> Eli Zaretskii <eliz@gnu.org> writes:
>
> >> + - gdb.PendingFrame.find_sal(): Return a gdb.Symtab_and_line
> >> + object for the current location within the pending frame, or
> >> + None.
> >> + - gdb.PendingFrame.block(): Return a gdb.Block for the current
> >> + pending frame, or None.
> >> + - gdb.PendingFrame.function(): Return a gdb.Symbol for the
> >> + current pending frame, or None.
> >
> > Btw, why do you follow each method name with a "()"? That looks like
> > a call with no arguments, which is not what you mean, right?
>
> I did indeed mean a call with no arguments. These would be used like
> this:
>
> class TestUnwinder(Unwinder):
> def __init__(self):
> super().__init__("Unwinder_Name")
>
> def __call__(self, pending_frame):
> is_valid = pending_frame.is_valid()
> name = pending_frame.name()
> pc = pending_frame.pc()
> language = pending_frame.language()
> sal = pending_frame.find_sal()
> block = pending_frame.block()
> function = pending_frame.function()
That's not my point. AFAIU, the text on which I commented documents
the methods and what each one of them does. Then the "()" has no
place there, since you are naming the methods, not showing how to call
them. Right?
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 05/10] gdb/python: add some additional methods to gdb.PendingFrame
2023-03-14 12:59 ` Eli Zaretskii
@ 2023-03-16 14:28 ` Andrew Burgess
2023-03-16 14:46 ` Eli Zaretskii
0 siblings, 1 reply; 37+ messages in thread
From: Andrew Burgess @ 2023-03-16 14:28 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: gdb-patches
Eli Zaretskii <eliz@gnu.org> writes:
>> From: Andrew Burgess <aburgess@redhat.com>
>> Cc: gdb-patches@sourceware.org
>> Date: Tue, 14 Mar 2023 10:18:09 +0000
>>
>> Eli Zaretskii <eliz@gnu.org> writes:
>>
>> >> + - gdb.PendingFrame.find_sal(): Return a gdb.Symtab_and_line
>> >> + object for the current location within the pending frame, or
>> >> + None.
>> >> + - gdb.PendingFrame.block(): Return a gdb.Block for the current
>> >> + pending frame, or None.
>> >> + - gdb.PendingFrame.function(): Return a gdb.Symbol for the
>> >> + current pending frame, or None.
>> >
>> > Btw, why do you follow each method name with a "()"? That looks like
>> > a call with no arguments, which is not what you mean, right?
>>
>> I did indeed mean a call with no arguments. These would be used like
>> this:
>>
>> class TestUnwinder(Unwinder):
>> def __init__(self):
>> super().__init__("Unwinder_Name")
>>
>> def __call__(self, pending_frame):
>> is_valid = pending_frame.is_valid()
>> name = pending_frame.name()
>> pc = pending_frame.pc()
>> language = pending_frame.language()
>> sal = pending_frame.find_sal()
>> block = pending_frame.block()
>> function = pending_frame.function()
>
> That's not my point. AFAIU, the text on which I commented documents
> the methods and what each one of them does. Then the "()" has no
> place there, since you are naming the methods, not showing how to call
> them. Right?
I like to think we're documenting how to use the API, which includes how
to call them. I do end up being a user of the Python API docs pretty
extensively, and when I do I'm looking for how do I call this method,
and what arguments should I be passing.
As far as I can tell the most common style in the docs is to include the
argument list, and I think it would be more confusing if we only
included the argument list for the case when there were some actual
arguments. Why leave the user to infer the empty argument list when we
can just go ahead and say it.
Finally, this becomes more confusing I think with Python that supports
both methods (requires parenthesis to call) and attributes which don't
require parenthesis to access. Thus under the current scheme we have:
@defvar ClassName.Attribute
Contains a value.
@end defvar
@defun ClassName.NoArgsMethod()
Does stuff.
@end defun
@defun ClassName.TakesArgs(@var{arg1}, @var{arg2})
Does other stuff.
@end defun
If I've understood your correctly (sorry if I have not), then you are
suggesting dropping the '()' from the 'NoArgsMethod' case. But I think
this would be inconsistent with the 'TakesArgs' method, and risks
confusion with the 'Attribute'.
Thanks,
Andrew
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 05/10] gdb/python: add some additional methods to gdb.PendingFrame
2023-03-16 14:28 ` Andrew Burgess
@ 2023-03-16 14:46 ` Eli Zaretskii
2023-03-16 17:26 ` Andrew Burgess
0 siblings, 1 reply; 37+ messages in thread
From: Eli Zaretskii @ 2023-03-16 14:46 UTC (permalink / raw)
To: Andrew Burgess; +Cc: gdb-patches
> From: Andrew Burgess <aburgess@redhat.com>
> Cc: gdb-patches@sourceware.org
> Date: Thu, 16 Mar 2023 14:28:46 +0000
>
> Eli Zaretskii <eliz@gnu.org> writes:
>
> >> >> + - gdb.PendingFrame.find_sal(): Return a gdb.Symtab_and_line
> >> >> + object for the current location within the pending frame, or
> >> >> + None.
> >> >> + - gdb.PendingFrame.block(): Return a gdb.Block for the current
> >> >> + pending frame, or None.
> >> >> + - gdb.PendingFrame.function(): Return a gdb.Symbol for the
> >> >> + current pending frame, or None.
> >> >
> >> > Btw, why do you follow each method name with a "()"? That looks like
> >> > a call with no arguments, which is not what you mean, right?
> >>
> >> I did indeed mean a call with no arguments. These would be used like
> >> this:
> >>
> >> class TestUnwinder(Unwinder):
> >> def __init__(self):
> >> super().__init__("Unwinder_Name")
> >>
> >> def __call__(self, pending_frame):
> >> is_valid = pending_frame.is_valid()
> >> name = pending_frame.name()
> >> pc = pending_frame.pc()
> >> language = pending_frame.language()
> >> sal = pending_frame.find_sal()
> >> block = pending_frame.block()
> >> function = pending_frame.function()
> >
> > That's not my point. AFAIU, the text on which I commented documents
> > the methods and what each one of them does. Then the "()" has no
> > place there, since you are naming the methods, not showing how to call
> > them. Right?
>
> I like to think we're documenting how to use the API, which includes how
> to call them.
That's true in Texinfo, but my comment was about NEWS.
> @defvar ClassName.Attribute
> Contains a value.
> @end defvar
>
> @defun ClassName.NoArgsMethod()
> Does stuff.
> @end defun
>
> @defun ClassName.TakesArgs(@var{arg1}, @var{arg2})
> Does other stuff.
> @end defun
>
> If I've understood your correctly (sorry if I have not), then you are
> suggesting dropping the '()' from the 'NoArgsMethod' case.
Not in Texinfo, no. In Texinfo, we do show the signature when we
document a function or a method. But NEWS is mostly free-form text,
and in free-form text there's no reason to append a signature whenever
you mention the name of a function or a method.
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 05/10] gdb/python: add some additional methods to gdb.PendingFrame
2023-03-16 14:46 ` Eli Zaretskii
@ 2023-03-16 17:26 ` Andrew Burgess
2023-03-16 19:54 ` Eli Zaretskii
0 siblings, 1 reply; 37+ messages in thread
From: Andrew Burgess @ 2023-03-16 17:26 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: gdb-patches
Eli Zaretskii <eliz@gnu.org> writes:
>> From: Andrew Burgess <aburgess@redhat.com>
>> Cc: gdb-patches@sourceware.org
>> Date: Thu, 16 Mar 2023 14:28:46 +0000
>>
>> Eli Zaretskii <eliz@gnu.org> writes:
>>
>> >> >> + - gdb.PendingFrame.find_sal(): Return a gdb.Symtab_and_line
>> >> >> + object for the current location within the pending frame, or
>> >> >> + None.
>> >> >> + - gdb.PendingFrame.block(): Return a gdb.Block for the current
>> >> >> + pending frame, or None.
>> >> >> + - gdb.PendingFrame.function(): Return a gdb.Symbol for the
>> >> >> + current pending frame, or None.
>> >> >
>> >> > Btw, why do you follow each method name with a "()"? That looks like
>> >> > a call with no arguments, which is not what you mean, right?
>> >>
>> >> I did indeed mean a call with no arguments. These would be used like
>> >> this:
>> >>
>> >> class TestUnwinder(Unwinder):
>> >> def __init__(self):
>> >> super().__init__("Unwinder_Name")
>> >>
>> >> def __call__(self, pending_frame):
>> >> is_valid = pending_frame.is_valid()
>> >> name = pending_frame.name()
>> >> pc = pending_frame.pc()
>> >> language = pending_frame.language()
>> >> sal = pending_frame.find_sal()
>> >> block = pending_frame.block()
>> >> function = pending_frame.function()
>> >
>> > That's not my point. AFAIU, the text on which I commented documents
>> > the methods and what each one of them does. Then the "()" has no
>> > place there, since you are naming the methods, not showing how to call
>> > them. Right?
>>
>> I like to think we're documenting how to use the API, which includes how
>> to call them.
>
> That's true in Texinfo, but my comment was about NEWS.
OK, now I just feel like an idiot :/
Sorry for wasting your time.
The updated NEW entry is:
** New methods added to the gdb.PendingFrame class. These methods
have the same behaviour as the corresponding methods on
gdb.Frame. The new methods are:
- gdb.PendingFrame.name: Return the name for the frame's
function, or None.
- gdb.PendingFrame.is_valid: Return True if the pending frame
object is valid.
- gdb.PendingFrame.pc: Return the $pc register value for this
frame.
- gdb.PendingFrame.language: Return a string containing the
language for this frame, or None.
- gdb.PendingFrame.find_sal: Return a gdb.Symtab_and_line
object for the current location within the pending frame, or
None.
- gdb.PendingFrame.block: Return a gdb.Block for the current
pending frame, or None.
- gdb.PendingFrame.function: Return a gdb.Symbol for the
current pending frame, or None.
The complete patch with this NEWS entry change is below for your
consideration.
Thanks for your patience,
Andrew
---
commit 4dcad36a4a58e9c19189cb966d68d051483c1929
Author: Andrew Burgess <aburgess@redhat.com>
Date: Wed Mar 8 16:11:45 2023 +0000
gdb/python: add some additional methods to gdb.PendingFrame
The gdb.Frame class has far more methods than gdb.PendingFrame. Given
that a PendingFrame hasn't yet been claimed by an unwinder, there is a
limit to which methods we can add to it, but many of the methods that
the Frame class has, the PendingFrame class could also support.
In this commit I've added those methods to PendingFrame that I believe
are safe.
In terms of implementation: if I was starting from scratch then I
would implement many of these (or most of these) as attributes rather
than methods. However, given both Frame and PendingFrame are just
different representation of a frame, I think there is value in keeping
the interface for the two classes the same. For this reason
everything here is a method -- that's what the Frame class does.
The new methods I've added are:
- gdb.PendingFrame.is_valid: Return True if the pending frame
object is valid.
- gdb.PendingFrame.name: Return the name for the frame's function,
or None.
- gdb.PendingFrame.pc: Return the $pc register value for this
frame.
- gdb.PendingFrame.language: Return a string containing the
language for this frame, or None.
- gdb.PendingFrame.find_sal: Return a gdb.Symtab_and_line object
for the current location within the pending frame, or None.
- gdb.PendingFrame.block: Return a gdb.Block for the current
pending frame, or None.
- gdb.PendingFrame.function: Return a gdb.Symbol for the current
pending frame, or None.
In every case I've just copied the implementation over from gdb.Frame
and cleaned the code slightly e.g. NULL to nullptr. Additionally each
function required a small update to reflect the PendingFrame type, but
that's pretty minor.
There are tests for all the new methods.
For more extensive testing, I added the following code to the file
gdb/python/lib/command/unwinders.py:
from gdb.unwinder import Unwinder
class TestUnwinder(Unwinder):
def __init__(self):
super().__init__("XXX_TestUnwinder_XXX")
def __call__(self,pending_frame):
lang = pending_frame.language()
try:
block = pending_frame.block()
assert isinstance(block, gdb.Block)
except RuntimeError as rte:
assert str(rte) == "Cannot locate block for frame."
function = pending_frame.function()
arch = pending_frame.architecture()
assert arch is None or isinstance(arch, gdb.Architecture)
name = pending_frame.name()
assert name is None or isinstance(name, str)
valid = pending_frame.is_valid()
pc = pending_frame.pc()
sal = pending_frame.find_sal()
assert sal is None or isinstance(sal, gdb.Symtab_and_line)
return None
gdb.unwinder.register_unwinder(None, TestUnwinder())
This registers a global unwinder that calls each of the new
PendingFrame methods and checks the result is of an acceptable type.
The unwinder never claims any frames though, so shouldn't change how
GDB actually behaves.
I then ran the testsuite. There was only a single regression, a test
that uses 'disable unwinder' and expects a single unwinder to be
disabled -- the extra unwinder is now disabled too, which changes the
test output. So I'm reasonably confident that the new methods are not
going to crash GDB.
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
diff --git a/gdb/NEWS b/gdb/NEWS
index 99e3d0bf222..da66fb1da8d 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -118,6 +118,26 @@ show always-read-ctf
without the particular unwinder, depending on how 'enabled' was
changed.
+ ** New methods added to the gdb.PendingFrame class. These methods
+ have the same behaviour as the corresponding methods on
+ gdb.Frame. The new methods are:
+
+ - gdb.PendingFrame.name: Return the name for the frame's
+ function, or None.
+ - gdb.PendingFrame.is_valid: Return True if the pending frame
+ object is valid.
+ - gdb.PendingFrame.pc: Return the $pc register value for this
+ frame.
+ - gdb.PendingFrame.language: Return a string containing the
+ language for this frame, or None.
+ - gdb.PendingFrame.find_sal: Return a gdb.Symtab_and_line
+ object for the current location within the pending frame, or
+ None.
+ - gdb.PendingFrame.block: Return a gdb.Block for the current
+ pending frame, or None.
+ - gdb.PendingFrame.function: Return a gdb.Symbol for the
+ current pending frame, or None.
+
*** Changes in GDB 13
* MI version 1 is deprecated, and will be removed in GDB 14.
diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index 8af2d1faea3..2e5d79d4074 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -2830,6 +2830,46 @@
@xref{Frames, ,Stack Frames}.
@end defun
+@defun PendingFrame.name ()
+Returns the function name of this pending frame, or @code{None} if it
+can't be obtained.
+@end defun
+
+@defun PendingFrame.is_valid ()
+Returns true if the @code{gdb.PendingFrame} object is valid, false if
+not. A pending frame object becomes invalid when the call to the
+unwinder, for which the pending frame was created, returns.
+
+All @code{gdb.PendingFrame} methods, except this one, will raise an
+exception if the pending frame object is invalid at the time the
+method is called.
+@end defun
+
+@defun PendingFrame.pc ()
+Returns the pending frame's resume address.
+@end defun
+
+@defun PendingFrame.block ()
+Return the pending frame's code block (@pxref{Blocks In Python}). If
+the frame does not have a block -- for example, if there is no
+debugging information for the code in question -- then this will raise
+a @code{RuntimeError} exception.
+@end defun
+
+@defun PendingFrame.function ()
+Return the symbol for the function corresponding to this pending frame.
+@xref{Symbols In Python}.
+@end defun
+
+@defun PendingFrame.find_sal ()
+Return the pending frame's symtab and line object (@pxref{Symbol
+Tables In Python}).
+@end defun
+
+@defun PendingFrame.language ()
+Return the language of this frame, as a string, or None.
+@end defun
+
@subheading Unwinder Output: UnwindInfo
Use @code{PendingFrame.create_unwind_info} method described above to
diff --git a/gdb/python/py-unwind.c b/gdb/python/py-unwind.c
index 12b14616363..5ed3ff37e64 100644
--- a/gdb/python/py-unwind.c
+++ b/gdb/python/py-unwind.c
@@ -28,6 +28,10 @@
#include "regcache.h"
#include "valprint.h"
#include "user-regs.h"
+#include "stack.h"
+#include "charset.h"
+#include "block.h"
+
/* Debugging of Python unwinders. */
@@ -412,6 +416,200 @@ pending_framepy_read_register (PyObject *self, PyObject *args)
return result;
}
+/* Implement PendingFrame.is_valid(). Return True if this pending frame
+ object is still valid. */
+
+static PyObject *
+pending_framepy_is_valid (PyObject *self, PyObject *args)
+{
+ pending_frame_object *pending_frame = (pending_frame_object *) self;
+
+ if (pending_frame->frame_info == nullptr)
+ Py_RETURN_FALSE;
+
+ Py_RETURN_TRUE;
+}
+
+/* Implement PendingFrame.name(). Return a string that is the name of the
+ function for this frame, or None if the name can't be found. */
+
+static PyObject *
+pending_framepy_name (PyObject *self, PyObject *args)
+{
+ pending_frame_object *pending_frame = (pending_frame_object *) self;
+
+ PENDING_FRAMEPY_REQUIRE_VALID (pending_frame);
+
+ gdb::unique_xmalloc_ptr<char> name;
+
+ try
+ {
+ enum language lang;
+ frame_info_ptr frame = pending_frame->frame_info;
+
+ name = find_frame_funname (frame, &lang, nullptr);
+ }
+ catch (const gdb_exception &except)
+ {
+ GDB_PY_HANDLE_EXCEPTION (except);
+ }
+
+ if (name != nullptr)
+ return PyUnicode_Decode (name.get (), strlen (name.get ()),
+ host_charset (), nullptr);
+
+ Py_RETURN_NONE;
+}
+
+/* Implement gdb.PendingFrame.pc(). Returns an integer containing the
+ frame's current $pc value. */
+
+static PyObject *
+pending_framepy_pc (PyObject *self, PyObject *args)
+{
+ pending_frame_object *pending_frame = (pending_frame_object *) self;
+
+ PENDING_FRAMEPY_REQUIRE_VALID (pending_frame);
+
+ CORE_ADDR pc = 0;
+
+ try
+ {
+ pc = get_frame_pc (pending_frame->frame_info);
+ }
+ catch (const gdb_exception &except)
+ {
+ GDB_PY_HANDLE_EXCEPTION (except);
+ }
+
+ return gdb_py_object_from_ulongest (pc).release ();
+}
+
+/* Implement gdb.PendingFrame.language(). Return the name of the language
+ for this frame. */
+
+static PyObject *
+pending_framepy_language (PyObject *self, PyObject *args)
+{
+ pending_frame_object *pending_frame = (pending_frame_object *) self;
+
+ PENDING_FRAMEPY_REQUIRE_VALID (pending_frame);
+
+ try
+ {
+ frame_info_ptr fi = pending_frame->frame_info;
+
+ enum language lang = get_frame_language (fi);
+ const language_defn *lang_def = language_def (lang);
+
+ return host_string_to_python_string (lang_def->name ()).release ();
+ }
+ catch (const gdb_exception &except)
+ {
+ GDB_PY_HANDLE_EXCEPTION (except);
+ }
+
+ Py_RETURN_NONE;
+}
+
+/* Implement PendingFrame.find_sal(). Return the PendingFrame's symtab and
+ line. */
+
+static PyObject *
+pending_framepy_find_sal (PyObject *self, PyObject *args)
+{
+ pending_frame_object *pending_frame = (pending_frame_object *) self;
+
+ PENDING_FRAMEPY_REQUIRE_VALID (pending_frame);
+
+ PyObject *sal_obj = nullptr;
+
+ try
+ {
+ frame_info_ptr frame = pending_frame->frame_info;
+
+ symtab_and_line sal = find_frame_sal (frame);
+ sal_obj = symtab_and_line_to_sal_object (sal);
+ }
+ catch (const gdb_exception &except)
+ {
+ GDB_PY_HANDLE_EXCEPTION (except);
+ }
+
+ return sal_obj;
+}
+
+/* Implement PendingFrame.block(). Return a gdb.Block for the pending
+ frame's code, or raise RuntimeError if the block can't be found. */
+
+static PyObject *
+pending_framepy_block (PyObject *self, PyObject *args)
+{
+ pending_frame_object *pending_frame = (pending_frame_object *) self;
+
+ PENDING_FRAMEPY_REQUIRE_VALID (pending_frame);
+
+ frame_info_ptr frame = pending_frame->frame_info;
+ const struct block *block = nullptr, *fn_block;
+
+ try
+ {
+ block = get_frame_block (frame, nullptr);
+ }
+ catch (const gdb_exception &except)
+ {
+ GDB_PY_HANDLE_EXCEPTION (except);
+ }
+
+ for (fn_block = block;
+ fn_block != nullptr && fn_block->function () == nullptr;
+ fn_block = fn_block->superblock ())
+ ;
+
+ if (block == nullptr
+ || fn_block == nullptr
+ || fn_block->function () == nullptr)
+ {
+ PyErr_SetString (PyExc_RuntimeError,
+ _("Cannot locate block for frame."));
+ return nullptr;
+ }
+
+ return block_to_block_object (block, fn_block->function ()->objfile ());
+}
+
+/* Implement gdb.PendingFrame.function(). Return a gdb.Symbol
+ representing the function of this frame, or None if no suitable symbol
+ can be found. */
+
+static PyObject *
+pending_framepy_function (PyObject *self, PyObject *args)
+{
+ pending_frame_object *pending_frame = (pending_frame_object *) self;
+
+ PENDING_FRAMEPY_REQUIRE_VALID (pending_frame);
+
+ struct symbol *sym = nullptr;
+
+ try
+ {
+ enum language funlang;
+ frame_info_ptr frame = pending_frame->frame_info;
+
+ gdb::unique_xmalloc_ptr<char> funname
+ = find_frame_funname (frame, &funlang, &sym);
+ }
+ catch (const gdb_exception &except)
+ {
+ GDB_PY_HANDLE_EXCEPTION (except);
+ }
+
+ if (sym != nullptr)
+ return symbol_to_symbol_object (sym);
+
+ Py_RETURN_NONE;
+}
+
/* Implementation of
PendingFrame.create_unwind_info (self, frameId) -> UnwindInfo. */
@@ -737,6 +935,29 @@ static PyMethodDef pending_frame_object_methods[] =
pending_framepy_architecture, METH_NOARGS,
"architecture () -> gdb.Architecture\n"
"The architecture for this PendingFrame." },
+ { "name",
+ pending_framepy_name, METH_NOARGS,
+ "name() -> String.\n\
+Return the function name of the frame, or None if it can't be determined." },
+ { "is_valid",
+ pending_framepy_is_valid, METH_NOARGS,
+ "is_valid () -> Boolean.\n\
+Return true if this PendingFrame is valid, false if not." },
+ { "pc",
+ pending_framepy_pc, METH_NOARGS,
+ "pc () -> Long.\n\
+Return the frame's resume address." },
+ { "language", pending_framepy_language, METH_NOARGS,
+ "The language of this frame." },
+ { "find_sal", pending_framepy_find_sal, METH_NOARGS,
+ "find_sal () -> gdb.Symtab_and_line.\n\
+Return the frame's symtab and line." },
+ { "block", pending_framepy_block, METH_NOARGS,
+ "block () -> gdb.Block.\n\
+Return the frame's code block." },
+ { "function", pending_framepy_function, METH_NOARGS,
+ "function () -> gdb.Symbol.\n\
+Returns the symbol for the function corresponding to this frame." },
{ "level", pending_framepy_level, METH_NOARGS,
"The stack level of this frame." },
{NULL} /* Sentinel */
diff --git a/gdb/testsuite/gdb.python/py-unwind.exp b/gdb/testsuite/gdb.python/py-unwind.exp
index 3e214ee0f45..f670da5a7cd 100644
--- a/gdb/testsuite/gdb.python/py-unwind.exp
+++ b/gdb/testsuite/gdb.python/py-unwind.exp
@@ -149,15 +149,46 @@ gdb_test_no_output "python obj = simple_unwinder(\"simple\")"
gdb_test_no_output "python gdb.unwinder.register_unwinder(None, obj)"
check_for_broken_backtrace "backtrace to capture a PendingFrame object"
+# Check the captured PendingFrame is not valid.
+gdb_test "python print(captured_pending_frame.is_valid())" "False"
+
# Call methods on the captured gdb.PendingFrame and check we see the
# expected error.
gdb_test_no_output "python pf = captured_pending_frame"
foreach cmd {"pf.read_register(\"pc\")" \
"pf.create_unwind_info(None)" \
"pf.architecture()" \
- "pf.level()"} {
+ "pf.level()" \
+ "pf.name()" \
+ "pf.pc()" \
+ "pf.language()" \
+ "pf.find_sal()" \
+ "pf.block()" \
+ "pf.function()" } {
gdb_test "python $cmd" \
[multi_line \
"ValueError: gdb\\.PendingFrame is invalid\\." \
"Error while executing Python code\\."]
}
+
+# Turn on the useful unwinder so we have the full backtrace again, and
+# disable the simple unwinder -- because we can!
+gdb_test "enable unwinder global \"test unwinder\"" \
+ "1 unwinder enabled" \
+ "re-enable 'test unwinder' so we can check PendingFrame methods"
+gdb_test "disable unwinder global \"simple\"" \
+ "1 unwinder disabled"
+check_for_fixed_backtrace \
+ "check backtrace before testing PendingFrame methods"
+
+# Gather information about every frame.
+gdb_test_no_output "python capture_all_frame_information()"
+gdb_test_no_output "python gdb.newest_frame().select()"
+gdb_test_no_output "python pspace = gdb.selected_inferior().progspace"
+gdb_test_no_output "python obj = validating_unwinder()"
+gdb_test_no_output "python gdb.unwinder.register_unwinder(pspace, obj)"
+
+check_for_fixed_backtrace \
+ "check backtrace to validate all information"
+
+gdb_test_no_output "python check_all_frame_information_matched()"
diff --git a/gdb/testsuite/gdb.python/py-unwind.py b/gdb/testsuite/gdb.python/py-unwind.py
index b30e843e7e5..8e3c1f398bc 100644
--- a/gdb/testsuite/gdb.python/py-unwind.py
+++ b/gdb/testsuite/gdb.python/py-unwind.py
@@ -144,9 +144,113 @@ class simple_unwinder(Unwinder):
def __call__(self, pending_frame):
global captured_pending_frame
+ assert pending_frame.is_valid()
+
if captured_pending_frame is None:
captured_pending_frame = pending_frame
return None
+# Return a dictionary of information about FRAME.
+def capture_frame_information(frame):
+ name = frame.name()
+ level = frame.level()
+ language = frame.language()
+ function = frame.function()
+ architecture = frame.architecture()
+ pc = frame.pc()
+ sal = frame.find_sal()
+ try:
+ block = frame.block()
+ assert isinstance(block, gdb.Block)
+ except RuntimeError as rte:
+ assert str(rte) == "Cannot locate block for frame."
+ block = "RuntimeError: " + str(rte)
+
+ return {
+ "name": name,
+ "level": level,
+ "language": language,
+ "function": function,
+ "architecture": architecture,
+ "pc": pc,
+ "sal": sal,
+ "block": block,
+ }
+
+
+# List of information about each frame. The index into this list is
+# the frame level. This is populated by
+# capture_all_frame_information.
+all_frame_information = []
+
+# Fill in the global ALL_FRAME_INFORMATION list.
+def capture_all_frame_information():
+ global all_frame_information
+
+ all_frame_information = []
+
+ gdb.newest_frame().select()
+ frame = gdb.selected_frame()
+ count = 0
+
+ while frame is not None:
+ frame.select()
+ info = capture_frame_information(frame)
+ level = info["level"]
+ info["matched"] = False
+
+ while len(all_frame_information) <= level:
+ all_frame_information.append(None)
+
+ assert all_frame_information[level] is None
+ all_frame_information[level] = info
+
+ if frame.name == "main" or count > 10:
+ break
+
+ count += 1
+ frame = frame.older()
+
+
+# Assert that every entry in the global ALL_FRAME_INFORMATION list was
+# matched by the validating_unwinder.
+def check_all_frame_information_matched():
+ global all_frame_information
+ for entry in all_frame_information:
+ assert entry["matched"]
+
+
+# An unwinder that doesn't match any frames. What it does do is
+# lookup information from the PendingFrame object and compare it
+# against information stored in the global ALL_FRAME_INFORMATION list.
+class validating_unwinder(Unwinder):
+ def __init__(self):
+ super().__init__("validating_unwinder")
+
+ def __call__(self, pending_frame):
+ info = capture_frame_information(pending_frame)
+ level = info["level"]
+
+ global all_frame_information
+ old_info = all_frame_information[level]
+
+ assert old_info is not None
+ assert not old_info["matched"]
+
+ for key, value in info.items():
+ assert key in old_info, f"{key} not in old_info"
+ assert type(value) == type(old_info[key])
+ if isinstance(value, gdb.Block):
+ assert value.start == old_info[key].start
+ assert value.end == old_info[key].end
+ assert value.is_static == old_info[key].is_static
+ assert value.is_global == old_info[key].is_global
+ else:
+ assert str(value) == str(old_info[key])
+
+ old_info["matched"] = True
+ return None
+
+
print("Python script imported")
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 05/10] gdb/python: add some additional methods to gdb.PendingFrame
2023-03-16 17:26 ` Andrew Burgess
@ 2023-03-16 19:54 ` Eli Zaretskii
0 siblings, 0 replies; 37+ messages in thread
From: Eli Zaretskii @ 2023-03-16 19:54 UTC (permalink / raw)
To: Andrew Burgess; +Cc: gdb-patches
> From: Andrew Burgess <aburgess@redhat.com>
> Cc: gdb-patches@sourceware.org
> Date: Thu, 16 Mar 2023 17:26:47 +0000
>
> The updated NEW entry is:
>
> ** New methods added to the gdb.PendingFrame class. These methods
> have the same behaviour as the corresponding methods on
> gdb.Frame. The new methods are:
>
> - gdb.PendingFrame.name: Return the name for the frame's
> function, or None.
> - gdb.PendingFrame.is_valid: Return True if the pending frame
> object is valid.
> - gdb.PendingFrame.pc: Return the $pc register value for this
> frame.
> - gdb.PendingFrame.language: Return a string containing the
> language for this frame, or None.
> - gdb.PendingFrame.find_sal: Return a gdb.Symtab_and_line
> object for the current location within the pending frame, or
> None.
> - gdb.PendingFrame.block: Return a gdb.Block for the current
> pending frame, or None.
> - gdb.PendingFrame.function: Return a gdb.Symbol for the
> current pending frame, or None.
>
> The complete patch with this NEWS entry change is below for your
> consideration.
LGTM, thanks.
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 06/10] gdb/python: add __repr__ for PendingFrame and UnwindInfo
2023-03-10 14:55 [PATCH 00/10] Improvements & Cleanup For Python Unwinder API Andrew Burgess
` (4 preceding siblings ...)
2023-03-10 14:55 ` [PATCH 05/10] gdb/python: add some additional methods to gdb.PendingFrame Andrew Burgess
@ 2023-03-10 14:55 ` Andrew Burgess
2023-03-10 14:55 ` [PATCH 07/10] gdb/python: remove Py_TPFLAGS_BASETYPE from gdb.UnwindInfo Andrew Burgess
` (4 subsequent siblings)
10 siblings, 0 replies; 37+ messages in thread
From: Andrew Burgess @ 2023-03-10 14:55 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess
Having a useful __repr__ method can make debugging Python code that
little bit easier. This commit adds __repr__ for gdb.PendingFrame and
gdb.UnwindInfo classes, along with some tests.
---
gdb/python/py-unwind.c | 67 +++++++++++++++++++++++++-
gdb/testsuite/gdb.python/py-unwind.exp | 19 ++++++++
gdb/testsuite/gdb.python/py-unwind.py | 16 +++++-
3 files changed, 99 insertions(+), 3 deletions(-)
diff --git a/gdb/python/py-unwind.c b/gdb/python/py-unwind.c
index 5ed3ff37e64..1e94c243163 100644
--- a/gdb/python/py-unwind.c
+++ b/gdb/python/py-unwind.c
@@ -229,6 +229,38 @@ unwind_infopy_str (PyObject *self)
return PyUnicode_FromString (stb.c_str ());
}
+/* Implement UnwindInfo.__repr__(). */
+
+static PyObject *
+unwind_infopy_repr (PyObject *self)
+{
+ unwind_info_object *unwind_info = (unwind_info_object *) self;
+ pending_frame_object *pending_frame
+ = (pending_frame_object *) (unwind_info->pending_frame);
+ frame_info_ptr frame = pending_frame->frame_info;
+
+ if (frame == nullptr)
+ return PyUnicode_FromFormat ("<%s for an invalid frame>",
+ Py_TYPE (self)->tp_name);
+
+ std::string saved_reg_names;
+ struct gdbarch *gdbarch = pending_frame->gdbarch;
+
+ for (const saved_reg ® : *unwind_info->saved_regs)
+ {
+ const char *name = gdbarch_register_name (gdbarch, reg.number);
+ if (saved_reg_names.empty ())
+ saved_reg_names = name;
+ else
+ saved_reg_names = (saved_reg_names + ", ") + name;
+ }
+
+ return PyUnicode_FromFormat ("<%s frame #%d, saved_regs=(%s)>",
+ Py_TYPE (self)->tp_name,
+ frame_relative_level (frame),
+ saved_reg_names.c_str ());
+}
+
/* Create UnwindInfo instance for given PendingFrame and frame ID.
Sets Python error and returns NULL on error.
@@ -372,6 +404,37 @@ pending_framepy_str (PyObject *self)
return PyUnicode_FromFormat ("SP=%s,PC=%s", sp_str, pc_str);
}
+/* Implement PendingFrame.__repr__(). */
+
+static PyObject *
+pending_framepy_repr (PyObject *self)
+{
+ pending_frame_object *pending_frame = (pending_frame_object *) self;
+ frame_info_ptr frame = pending_frame->frame_info;
+
+ if (frame == nullptr)
+ return PyUnicode_FromFormat ("<%s (invalid)>", Py_TYPE (self)->tp_name);
+
+ const char *sp_str = nullptr;
+ const char *pc_str = nullptr;
+
+ try
+ {
+ sp_str = core_addr_to_string_nz (get_frame_sp (frame));
+ pc_str = core_addr_to_string_nz (get_frame_pc (frame));
+ }
+ catch (const gdb_exception &except)
+ {
+ GDB_PY_HANDLE_EXCEPTION (except);
+ }
+
+ return PyUnicode_FromFormat ("<%s level=%d, sp=%s, pc=%s>",
+ Py_TYPE (self)->tp_name,
+ frame_relative_level (frame),
+ sp_str,
+ pc_str);
+}
+
/* Implementation of gdb.PendingFrame.read_register (self, reg) -> gdb.Value.
Returns the value of register REG as gdb.Value instance. */
@@ -974,7 +1037,7 @@ PyTypeObject pending_frame_object_type =
0, /* tp_getattr */
0, /* tp_setattr */
0, /* tp_compare */
- 0, /* tp_repr */
+ pending_framepy_repr, /* tp_repr */
0, /* tp_as_number */
0, /* tp_as_sequence */
0, /* tp_as_mapping */
@@ -1024,7 +1087,7 @@ PyTypeObject unwind_info_object_type =
0, /* tp_getattr */
0, /* tp_setattr */
0, /* tp_compare */
- 0, /* tp_repr */
+ unwind_infopy_repr, /* tp_repr */
0, /* tp_as_number */
0, /* tp_as_sequence */
0, /* tp_as_mapping */
diff --git a/gdb/testsuite/gdb.python/py-unwind.exp b/gdb/testsuite/gdb.python/py-unwind.exp
index f670da5a7cd..7e6ac9a8623 100644
--- a/gdb/testsuite/gdb.python/py-unwind.exp
+++ b/gdb/testsuite/gdb.python/py-unwind.exp
@@ -152,6 +152,25 @@ check_for_broken_backtrace "backtrace to capture a PendingFrame object"
# Check the captured PendingFrame is not valid.
gdb_test "python print(captured_pending_frame.is_valid())" "False"
+# Check the __repr__ of an invalid PendingFrame.
+gdb_test "python print(repr(captured_pending_frame))" \
+ "<gdb.PendingFrame \\(invalid\\)>"
+
+# Check the __repr__ of an UnwindInfo for an invalid PendingFrame.
+gdb_test "python print(captured_unwind_info)"
+gdb_test "python print(repr(captured_unwind_info))" \
+ "<gdb.UnwindInfo for an invalid frame>"
+
+# Check the repr of a PendingFrame that was copied (as a string) at a
+# time the PendingFrame was valid.
+gdb_test "python print(captured_pending_frame_repr)" \
+ "<gdb.PendingFrame level=0, sp=$hex, pc=$hex>"
+
+# Check the repr of an UnwindInfo that was copied (as a string) at a
+# time the UnwindInfo was valid.
+gdb_test "python print(captured_unwind_info_repr)" \
+ "<gdb.UnwindInfo frame #0, saved_regs=\\(rip, rbp, rsp\\)>"
+
# Call methods on the captured gdb.PendingFrame and check we see the
# expected error.
gdb_test_no_output "python pf = captured_pending_frame"
diff --git a/gdb/testsuite/gdb.python/py-unwind.py b/gdb/testsuite/gdb.python/py-unwind.py
index 8e3c1f398bc..f8f04b7f514 100644
--- a/gdb/testsuite/gdb.python/py-unwind.py
+++ b/gdb/testsuite/gdb.python/py-unwind.py
@@ -133,8 +133,11 @@ class TestUnwinder(Unwinder):
global_test_unwinder = TestUnwinder()
gdb.unwinder.register_unwinder(None, global_test_unwinder, True)
-# This is filled in by the simple_unwinder class.
+# These are filled in by the simple_unwinder class.
captured_pending_frame = None
+captured_pending_frame_repr = None
+captured_unwind_info = None
+captured_unwind_info_repr = None
class simple_unwinder(Unwinder):
@@ -143,11 +146,22 @@ class simple_unwinder(Unwinder):
def __call__(self, pending_frame):
global captured_pending_frame
+ global captured_pending_frame_repr
+ global captured_unwind_info
+ global captured_unwind_info_repr
assert pending_frame.is_valid()
if captured_pending_frame is None:
captured_pending_frame = pending_frame
+ captured_pending_frame_repr = repr(pending_frame)
+ fid = FrameId(gdb.Value(0x123), gdb.Value(0x456))
+ uw = pending_frame.create_unwind_info(fid)
+ uw.add_saved_register("rip", gdb.Value(0x123))
+ uw.add_saved_register("rbp", gdb.Value(0x456))
+ uw.add_saved_register("rsp", gdb.Value(0x789))
+ captured_unwind_info = uw
+ captured_unwind_info_repr = repr(uw)
return None
--
2.25.4
^ permalink raw reply [flat|nested] 37+ messages in thread* [PATCH 07/10] gdb/python: remove Py_TPFLAGS_BASETYPE from gdb.UnwindInfo
2023-03-10 14:55 [PATCH 00/10] Improvements & Cleanup For Python Unwinder API Andrew Burgess
` (5 preceding siblings ...)
2023-03-10 14:55 ` [PATCH 06/10] gdb/python: add __repr__ for PendingFrame and UnwindInfo Andrew Burgess
@ 2023-03-10 14:55 ` Andrew Burgess
2023-03-10 14:55 ` [PATCH 08/10] gdb: have value_as_address call unpack_pointer Andrew Burgess
` (3 subsequent siblings)
10 siblings, 0 replies; 37+ messages in thread
From: Andrew Burgess @ 2023-03-10 14:55 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess
It is not currently possible to directly create gdb.UnwindInfo
instances, they need to be created by calling
gdb.PendingFrame.create_unwind_info so that the newly created
UnwindInfo can be linked to the pending frame.
As such there's no tp_init method defined for UnwindInfo.
A consequence of all this is that it doesn't really make sense to
allow sub-classing of gdb.UnwindInfo. Any sub-class can't call the
parents __init__ method to correctly link up the PendingFrame
object (there is no parent __init__ method). And any instances that
sub-classes UnwindInfo but doesn't call the parent __init__ is going
to be invalid for use in GDB.
This commit removes the Py_TPFLAGS_BASETYPE flag from the UnwindInfo
class, which prevents the class being sub-classed. Then I've added a
test to check that this is indeed prevented.
Any functional user code will not have any issues with this change.
---
gdb/python/py-unwind.c | 2 +-
gdb/testsuite/gdb.python/py-unwind.exp | 17 +++++++++++++++++
2 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/gdb/python/py-unwind.c b/gdb/python/py-unwind.c
index 1e94c243163..432a26a760a 100644
--- a/gdb/python/py-unwind.c
+++ b/gdb/python/py-unwind.c
@@ -1097,7 +1097,7 @@ PyTypeObject unwind_info_object_type =
0, /* tp_getattro */
0, /* tp_setattro */
0, /* tp_as_buffer */
- Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE, /* tp_flags */
+ Py_TPFLAGS_DEFAULT, /* tp_flags */
"GDB UnwindInfo object", /* tp_doc */
0, /* tp_traverse */
0, /* tp_clear */
diff --git a/gdb/testsuite/gdb.python/py-unwind.exp b/gdb/testsuite/gdb.python/py-unwind.exp
index 7e6ac9a8623..d65b8447525 100644
--- a/gdb/testsuite/gdb.python/py-unwind.exp
+++ b/gdb/testsuite/gdb.python/py-unwind.exp
@@ -211,3 +211,20 @@ check_for_fixed_backtrace \
"check backtrace to validate all information"
gdb_test_no_output "python check_all_frame_information_matched()"
+
+# Check we can't sub-class from gdb.UnwindInfo.
+gdb_test_multiline "Sub-class gdb.UnwindInfo " \
+ "python" "" \
+ "class my_unwind_info(gdb.UnwindInfo):" "" \
+ " def __init__(self):" "" \
+ " pass" "" \
+ "end" \
+ [multi_line \
+ "TypeError: type 'gdb\\.UnwindInfo' is not an acceptable base type" \
+ "Error while executing Python code\\."]
+
+# Check we can't directly instantiate a gdb.UnwindInfo.
+gdb_test "python uw = gdb.UnwindInfo()" \
+ [multi_line \
+ "TypeError: cannot create 'gdb\\.UnwindInfo' instances" \
+ "Error while executing Python code\\."]
--
2.25.4
^ permalink raw reply [flat|nested] 37+ messages in thread* [PATCH 08/10] gdb: have value_as_address call unpack_pointer
2023-03-10 14:55 [PATCH 00/10] Improvements & Cleanup For Python Unwinder API Andrew Burgess
` (6 preceding siblings ...)
2023-03-10 14:55 ` [PATCH 07/10] gdb/python: remove Py_TPFLAGS_BASETYPE from gdb.UnwindInfo Andrew Burgess
@ 2023-03-10 14:55 ` Andrew Burgess
2023-03-10 15:28 ` Tom Tromey
2023-03-10 14:55 ` [PATCH 09/10] gdb/python: Allow gdb.UnwindInfo to be created with non gdb.Value args Andrew Burgess
` (2 subsequent siblings)
10 siblings, 1 reply; 37+ messages in thread
From: Andrew Burgess @ 2023-03-10 14:55 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess
While refactoring some other code in gdb/python/* I wanted to merge
two code paths. One path calls value_as_address, while the other
calls unpack_pointer.
I suspect calling value_as_address is the correct choice, but, while
examining the code I noticed that value_as_address calls unpack_long
rather than unpack_pointer.
Under the hood, unpack_pointer does just call unpack_long so there's
no real difference here, but it feels like value_as_address should
call unpack_pointer.
I've updated the code to use unpack_pointer, and changed a related
comment to say that we call unpack_pointer. I've also adjusted the
header comment on value_as_address. The existing header refers to
some code that is now commented out.
Rather than trying to describe the whole algorithm of
value_as_address, which is already well commented within the function,
I've just trimmed the comment on value_as_address to be a brief
summary of what the function does.
There should be no user visible changes after this commit.
---
gdb/value.c | 9 ++++-----
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/gdb/value.c b/gdb/value.c
index 7b4df338304..d432b29b61b 100644
--- a/gdb/value.c
+++ b/gdb/value.c
@@ -2551,9 +2551,8 @@ value_as_long (struct value *val)
return unpack_long (val->type (), val->contents ().data ());
}
-/* Extract a value as a C pointer. Does not deallocate the value.
- Note that val's type may not actually be a pointer; value_as_long
- handles all the cases. */
+/* Extract a value as a C pointer. Does not deallocate the value. */
+
CORE_ADDR
value_as_address (struct value *val)
{
@@ -2592,7 +2591,7 @@ value_as_address (struct value *val)
to COERCE_ARRAY below actually does all the usual unary
conversions, which includes converting values of type `function'
to `pointer to function'. This is the challenging conversion
- discussed above. Then, `unpack_long' will convert that pointer
+ discussed above. Then, `unpack_pointer' will convert that pointer
back into an address.
So, suppose the user types `disassemble foo' on an architecture
@@ -2653,7 +2652,7 @@ value_as_address (struct value *val)
return gdbarch_integer_to_address (gdbarch, val->type (),
val->contents ().data ());
- return unpack_long (val->type (), val->contents ().data ());
+ return unpack_pointer (val->type (), val->contents ().data ());
#endif
}
\f
--
2.25.4
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 08/10] gdb: have value_as_address call unpack_pointer
2023-03-10 14:55 ` [PATCH 08/10] gdb: have value_as_address call unpack_pointer Andrew Burgess
@ 2023-03-10 15:28 ` Tom Tromey
2023-03-10 22:08 ` Andrew Burgess
0 siblings, 1 reply; 37+ messages in thread
From: Tom Tromey @ 2023-03-10 15:28 UTC (permalink / raw)
To: Andrew Burgess via Gdb-patches; +Cc: Andrew Burgess
>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:
Andrew> +/* Extract a value as a C pointer. Does not deallocate the value. */
I think that 'deallocate' sentence can be removed.
I don't know when that was last relevant, values are essentially never
deallocated by hand.
Tom
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 08/10] gdb: have value_as_address call unpack_pointer
2023-03-10 15:28 ` Tom Tromey
@ 2023-03-10 22:08 ` Andrew Burgess
0 siblings, 0 replies; 37+ messages in thread
From: Andrew Burgess @ 2023-03-10 22:08 UTC (permalink / raw)
To: Tom Tromey, Andrew Burgess via Gdb-patches
Tom Tromey <tom@tromey.com> writes:
>>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Andrew> +/* Extract a value as a C pointer. Does not deallocate the value. */
>
> I think that 'deallocate' sentence can be removed.
> I don't know when that was last relevant, values are essentially never
> deallocated by hand.
Thanks, have updated the comment in my local tree to read:
/* Extract a value as a C pointer. */
Thanks,
Andrew
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 09/10] gdb/python: Allow gdb.UnwindInfo to be created with non gdb.Value args
2023-03-10 14:55 [PATCH 00/10] Improvements & Cleanup For Python Unwinder API Andrew Burgess
` (7 preceding siblings ...)
2023-03-10 14:55 ` [PATCH 08/10] gdb: have value_as_address call unpack_pointer Andrew Burgess
@ 2023-03-10 14:55 ` Andrew Burgess
2023-03-10 15:34 ` Tom Tromey
2023-03-10 15:38 ` Eli Zaretskii
2023-03-10 14:55 ` [PATCH 10/10] gdb/python: Add new gdb.unwinder.FrameId class Andrew Burgess
2023-03-29 16:27 ` [PATCH 00/10] Improvements & Cleanup For Python Unwinder API Tom Tromey
10 siblings, 2 replies; 37+ messages in thread
From: Andrew Burgess @ 2023-03-10 14:55 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess
Currently when creating a gdb.UnwindInfo object a user must call
gdb.PendingFrame.create_unwind_info and pass a frame-id object.
The frame-id object should have at least a 'sp' attribute, and
probably a 'pc' attribute too (it can also, in some cases have a
'special' attribute).
Currently all of these frame-id attributes need to be gdb.Value
objects, but the only reason for that requirement is that we have some
code in py-unwind.c that only handles gdb.Value objects.
If instead we switch to using get_addr_from_python in py-utils.c then
we will support both gdb.Value objects and also raw numbers, which
might make things simpler in some cases.
So, I started rewriting pyuw_object_attribute_to_pointer (in
py-unwind.c) to use get_addr_from_python. However, while looking at
the code I noticed a problem.
The pyuw_object_attribute_to_pointer function returns a boolean flag,
if everything goes OK we return true, but we return false in two
cases, (1) when the attribute is not present, which might be
acceptable, or might be an error, and (2) when we get an error trying
to extract the attribute value, in which case a Python error will have
been set.
Now in pending_framepy_create_unwind_info we have this code:
if (!pyuw_object_attribute_to_pointer (pyo_frame_id, "sp", &sp))
{
PyErr_SetString (PyExc_ValueError,
_("frame_id should have 'sp' attribute."));
return NULL;
}
Notice how we always set an error. This will override any error that
is already set.
So, if you create a frame-id object that has an 'sp' attribute, but
the attribute is not a gdb.Value, then currently we fail to extract
the attribute value (it's not a gdb.Value) and set this error in
pyuw_object_attribute_to_pointer:
rc = pyuw_value_obj_to_pointer (pyo_value.get (), addr);
if (!rc)
PyErr_Format (
PyExc_ValueError,
_("The value of the '%s' attribute is not a pointer."),
attr_name);
Then we return to pending_framepy_create_unwind_info and immediately
override this error with the error about 'sp' being missing.
This all feels very confused.
Here's my proposed solution: pyuw_object_attribute_to_pointer will now
return a tri-state enum, with states OK, MISSING, or ERROR. The
meanings of these states are:
OK - Attribute exists and was extracted fine,
MISSING - Attribute doesn't exist, no Python error was set.
ERROR - Attribute does exist, but there was an error while
extracting it, a Python error was set.
We need to update pending_framepy_create_unwind_info, the only user of
pyuw_object_attribute_to_pointer, but now I think things are much
clearer. Errors from lower levels are not blindly overridden with the
generic meaningless error message, but we still get the "missing 'sp'
attribute" error when appropriate.
This change also includes the switch to get_addr_from_python which was
what started this whole journey.
For well behaving user code there should be no visible changes after
this commit.
For user code that hits an error, hopefully the new errors should be
more helpful in figuring out what's gone wrong.
Additionally, users can now use integers for the 'sp' and 'pc'
attributes in their frame-id objects if that is useful.
---
gdb/NEWS | 4 +
gdb/doc/python.texi | 3 +-
gdb/python/py-unwind.c | 113 ++++++++++++++-----------
gdb/testsuite/gdb.python/py-unwind.exp | 36 ++++++++
gdb/testsuite/gdb.python/py-unwind.py | 6 +-
5 files changed, 111 insertions(+), 51 deletions(-)
diff --git a/gdb/NEWS b/gdb/NEWS
index 0d9049ff134..c4f7de11c6e 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -130,6 +130,10 @@ show always-read-ctf
- gdb.PendingFrame.function(): Return a gdb.Symbol for the
current pending frame, or None.
+ ** The frame-id passed to gdb.PendingFrame.create_unwind_info can
+ now use either an integer or a gdb.Value object for each of its
+ 'sp', 'pc', and 'special' attributes.
+
*** Changes in GDB 13
* MI version 1 is deprecated, and will be removed in GDB 14.
diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index 1c4239841af..3074b5250a3 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -2815,7 +2815,8 @@
this.
@end table
-Each attribute value should be an instance of @code{gdb.Value}.
+Each attribute value should either be an instance of @code{gdb.Value}
+or an integer.
@end defun
diff --git a/gdb/python/py-unwind.c b/gdb/python/py-unwind.c
index 432a26a760a..409dbd3a470 100644
--- a/gdb/python/py-unwind.c
+++ b/gdb/python/py-unwind.c
@@ -132,58 +132,63 @@ extern PyTypeObject pending_frame_object_type
extern PyTypeObject unwind_info_object_type
CPYCHECKER_TYPE_OBJECT_FOR_TYPEDEF ("unwind_info_object");
-/* Convert gdb.Value instance to inferior's pointer. Return 1 on success,
- 0 on failure. */
+/* An enum returned by pyuw_object_attribute_to_pointer, a function which
+ is used to extract an attribute from a Python object. */
-static int
-pyuw_value_obj_to_pointer (PyObject *pyo_value, CORE_ADDR *addr)
+enum class pyuw_get_attr_code
{
- int rc = 0;
- struct value *value;
+ /* The attribute was present, and its value was successfully extracted. */
+ ATTR_OK,
- try
- {
- if ((value = value_object_to_value (pyo_value)) != NULL)
- {
- *addr = unpack_pointer (value->type (),
- value->contents ().data ());
- rc = 1;
- }
- }
- catch (const gdb_exception &except)
- {
- gdbpy_convert_exception (except);
- }
- return rc;
-}
+ /* The attribute was not present, or was present and its value was None.
+ No Python error has been set. */
+ ATTR_MISSING,
-/* Get attribute from an object and convert it to the inferior's
- pointer value. Return 1 if attribute exists and its value can be
- converted. Otherwise, if attribute does not exist or its value is
- None, return 0. In all other cases set Python error and return
- 0. */
+ /* The attribute was present, but there was some error while trying to
+ get the value from the attribute. A Python error will be set when
+ this is returned. */
+ ATTR_ERROR,
+};
-static int
+/* Get the attribute named ATTR_NAME from the object PYO and convert it to
+ an inferior pointer value, placing the pointer in *ADDR.
+
+ Return pyuw_get_attr_code::ATTR_OK if the attribute was present and its
+ value was successfully written into *ADDR. For any other return value
+ the contents of *ADDR are undefined.
+
+ Return pyuw_get_attr_code::ATTR_MISSING if the attribute was not
+ present, or it was present but its value was None. The contents of
+ *ADDR are undefined in this case. No Python error will be set in this
+ case.
+
+ Return pyuw_get_attr_code::ATTR_ERROR if the attribute was present, but
+ there was some error while extracting the attribute's value. A Python
+ error will be set in this case. The contents of *ADDR are undefined. */
+
+static pyuw_get_attr_code
pyuw_object_attribute_to_pointer (PyObject *pyo, const char *attr_name,
CORE_ADDR *addr)
{
- int rc = 0;
+ if (!PyObject_HasAttrString (pyo, attr_name))
+ return pyuw_get_attr_code::ATTR_MISSING;
- if (PyObject_HasAttrString (pyo, attr_name))
+ gdbpy_ref<> pyo_value (PyObject_GetAttrString (pyo, attr_name));
+ if (pyo_value == nullptr)
{
- gdbpy_ref<> pyo_value (PyObject_GetAttrString (pyo, attr_name));
+ gdb_assert (PyErr_Occurred ());
+ return pyuw_get_attr_code::ATTR_ERROR;
+ }
+ if (pyo_value == Py_None)
+ return pyuw_get_attr_code::ATTR_MISSING;
- if (pyo_value != NULL && pyo_value != Py_None)
- {
- rc = pyuw_value_obj_to_pointer (pyo_value.get (), addr);
- if (!rc)
- PyErr_Format (
- PyExc_ValueError,
- _("The value of the '%s' attribute is not a pointer."),
- attr_name);
- }
+ if (get_addr_from_python (pyo_value.get (), addr) < 0)
+ {
+ gdb_assert (PyErr_Occurred ());
+ return pyuw_get_attr_code::ATTR_ERROR;
}
- return rc;
+
+ return pyuw_get_attr_code::ATTR_OK;
}
/* Called by the Python interpreter to obtain string representation
@@ -687,13 +692,18 @@ pending_framepy_create_unwind_info (PyObject *self, PyObject *args)
PENDING_FRAMEPY_REQUIRE_VALID ((pending_frame_object *) self);
if (!PyArg_ParseTuple (args, "O:create_unwind_info", &pyo_frame_id))
- return NULL;
- if (!pyuw_object_attribute_to_pointer (pyo_frame_id, "sp", &sp))
+ return nullptr;
+
+ pyuw_get_attr_code code
+ = pyuw_object_attribute_to_pointer (pyo_frame_id, "sp", &sp);
+ if (code == pyuw_get_attr_code::ATTR_MISSING)
{
PyErr_SetString (PyExc_ValueError,
_("frame_id should have 'sp' attribute."));
- return NULL;
+ return nullptr;
}
+ else if (code == pyuw_get_attr_code::ATTR_ERROR)
+ return nullptr;
/* The logic of building frame_id depending on the attributes of
the frame_id object:
@@ -704,13 +714,20 @@ pending_framepy_create_unwind_info (PyObject *self, PyObject *args)
Y Y N frame_id_build (sp, pc)
Y Y Y frame_id_build_special (sp, pc, special)
*/
- if (!pyuw_object_attribute_to_pointer (pyo_frame_id, "pc", &pc))
+ code = pyuw_object_attribute_to_pointer (pyo_frame_id, "pc", &pc);
+ if (code == pyuw_get_attr_code::ATTR_ERROR)
+ return nullptr;
+ else if (code == pyuw_get_attr_code::ATTR_MISSING)
return pyuw_create_unwind_info (self, frame_id_build_wild (sp));
- if (!pyuw_object_attribute_to_pointer (pyo_frame_id, "special", &special))
+
+ code = pyuw_object_attribute_to_pointer (pyo_frame_id, "special", &special);
+ if (code == pyuw_get_attr_code::ATTR_ERROR)
+ return nullptr;
+ else if (code == pyuw_get_attr_code::ATTR_MISSING)
return pyuw_create_unwind_info (self, frame_id_build (sp, pc));
- else
- return pyuw_create_unwind_info (self,
- frame_id_build_special (sp, pc, special));
+
+ return pyuw_create_unwind_info (self,
+ frame_id_build_special (sp, pc, special));
}
/* Implementation of PendingFrame.architecture (self) -> gdb.Architecture. */
diff --git a/gdb/testsuite/gdb.python/py-unwind.exp b/gdb/testsuite/gdb.python/py-unwind.exp
index d65b8447525..fddf4f15393 100644
--- a/gdb/testsuite/gdb.python/py-unwind.exp
+++ b/gdb/testsuite/gdb.python/py-unwind.exp
@@ -200,6 +200,42 @@ gdb_test "disable unwinder global \"simple\"" \
check_for_fixed_backtrace \
"check backtrace before testing PendingFrame methods"
+# Turn the 'simple' unwinder back on.
+gdb_test "enable unwinder global \"simple\"" \
+ "1 unwinder enabled"
+
+# Replace the "simple" unwinder with a new version that doesn't set
+# the 'sp' attribute. Also the 'pc' attribute is invalid, but we'll
+# hit the missing 'sp' error first.
+with_test_prefix "frame-id 'sp' is None" {
+ gdb_test_no_output "python obj = simple_unwinder(\"simple\", None, \"xyz\")"
+ gdb_test_no_output "python gdb.unwinder.register_unwinder(None, obj, replace=True)"
+ gdb_test_no_output "python captured_pending_frame = None"
+ gdb_test "backtrace" \
+ "Python Exception <class 'ValueError'>: frame_id should have 'sp' attribute\\.\r\n.*"
+}
+
+# Replace the "simple" unwinder with a new version that sets the 'sp'
+# attribute to an invalid value. Also the 'pc' attribute is invalid, but we'll
+# hit the invalid 'sp' error first.
+with_test_prefix "frame-id 'sp' is invalid" {
+ gdb_test_no_output "python obj = simple_unwinder(\"simple\", \"jkl\", \"xyz\")"
+ gdb_test_no_output "python gdb.unwinder.register_unwinder(None, obj, replace=True)"
+ gdb_test_no_output "python captured_pending_frame = None"
+ gdb_test "backtrace" \
+ "Python Exception <class 'ValueError'>: invalid literal for int\\(\\) with base 10: 'jkl'\r\n.*"
+}
+
+# Replace the "simple" unwinder with a new version that sets the 'sp'
+# to a valid value, but set the 'pc' attribute to an invalid value.
+with_test_prefix "frame-id 'pc' is invalid" {
+ gdb_test_no_output "python obj = simple_unwinder(\"simple\", 0x123, \"xyz\")"
+ gdb_test_no_output "python gdb.unwinder.register_unwinder(None, obj, replace=True)"
+ gdb_test_no_output "python captured_pending_frame = None"
+ gdb_test "backtrace" \
+ "Python Exception <class 'ValueError'>: invalid literal for int\\(\\) with base 10: 'xyz'\r\n.*"
+}
+
# Gather information about every frame.
gdb_test_no_output "python capture_all_frame_information()"
gdb_test_no_output "python gdb.newest_frame().select()"
diff --git a/gdb/testsuite/gdb.python/py-unwind.py b/gdb/testsuite/gdb.python/py-unwind.py
index f8f04b7f514..dbabb006e4b 100644
--- a/gdb/testsuite/gdb.python/py-unwind.py
+++ b/gdb/testsuite/gdb.python/py-unwind.py
@@ -141,8 +141,10 @@ captured_unwind_info_repr = None
class simple_unwinder(Unwinder):
- def __init__(self, name):
+ def __init__(self, name, sp=0x123, pc=0x456):
super().__init__(name)
+ self._sp = sp
+ self._pc = pc
def __call__(self, pending_frame):
global captured_pending_frame
@@ -155,7 +157,7 @@ class simple_unwinder(Unwinder):
if captured_pending_frame is None:
captured_pending_frame = pending_frame
captured_pending_frame_repr = repr(pending_frame)
- fid = FrameId(gdb.Value(0x123), gdb.Value(0x456))
+ fid = FrameId(self._sp, self._pc)
uw = pending_frame.create_unwind_info(fid)
uw.add_saved_register("rip", gdb.Value(0x123))
uw.add_saved_register("rbp", gdb.Value(0x456))
--
2.25.4
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 09/10] gdb/python: Allow gdb.UnwindInfo to be created with non gdb.Value args
2023-03-10 14:55 ` [PATCH 09/10] gdb/python: Allow gdb.UnwindInfo to be created with non gdb.Value args Andrew Burgess
@ 2023-03-10 15:34 ` Tom Tromey
2023-03-10 22:16 ` Andrew Burgess
2023-03-10 15:38 ` Eli Zaretskii
1 sibling, 1 reply; 37+ messages in thread
From: Tom Tromey @ 2023-03-10 15:34 UTC (permalink / raw)
To: Andrew Burgess via Gdb-patches; +Cc: Andrew Burgess
>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:
Andrew> Currently all of these frame-id attributes need to be gdb.Value
Andrew> objects, but the only reason for that requirement is that we have some
Andrew> code in py-unwind.c that only handles gdb.Value objects.
Andrew> If instead we switch to using get_addr_from_python in py-utils.c then
Andrew> we will support both gdb.Value objects and also raw numbers, which
Andrew> might make things simpler in some cases.
FWIW this was https://sourceware.org/bugzilla/show_bug.cgi?id=19314
which I closed for reasons I no longer really understand.
Tom
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 09/10] gdb/python: Allow gdb.UnwindInfo to be created with non gdb.Value args
2023-03-10 15:34 ` Tom Tromey
@ 2023-03-10 22:16 ` Andrew Burgess
2023-03-11 14:47 ` Tom Tromey
0 siblings, 1 reply; 37+ messages in thread
From: Andrew Burgess @ 2023-03-10 22:16 UTC (permalink / raw)
To: Tom Tromey, Andrew Burgess via Gdb-patches
Tom Tromey <tom@tromey.com> writes:
>>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:
>
> Andrew> Currently all of these frame-id attributes need to be gdb.Value
> Andrew> objects, but the only reason for that requirement is that we have some
> Andrew> code in py-unwind.c that only handles gdb.Value objects.
>
> Andrew> If instead we switch to using get_addr_from_python in py-utils.c then
> Andrew> we will support both gdb.Value objects and also raw numbers, which
> Andrew> might make things simpler in some cases.
>
> FWIW this was https://sourceware.org/bugzilla/show_bug.cgi?id=19314
> which I closed for reasons I no longer really understand.
That would indeed appear to be a similar issue, but it's not the exact
same problem - at least, the code mentioned in that bug isn't touched by
this series.
This patch is specifically about the frame-id object which needs to be
passed to the PendingFrame.create_unwind_info call. That frame-id
object needs to have 'sp' and 'pc' attributes, which previously had to
be gdb.Value objects, but can now be raw integers.
The code mentioned in that bug is for the
UnwindInfo.add_saved_register method, which takes a register-id and a
register value, the value, it would seem, also has to be a gdb.Value.
I haven't tried to change that, but I could take a look.
Thanks,
Andrew
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 09/10] gdb/python: Allow gdb.UnwindInfo to be created with non gdb.Value args
2023-03-10 22:16 ` Andrew Burgess
@ 2023-03-11 14:47 ` Tom Tromey
0 siblings, 0 replies; 37+ messages in thread
From: Tom Tromey @ 2023-03-11 14:47 UTC (permalink / raw)
To: Andrew Burgess; +Cc: Tom Tromey, Andrew Burgess via Gdb-patches
>>>>> "Andrew" == Andrew Burgess <aburgess@redhat.com> writes:
Andrew> The code mentioned in that bug is for the
Andrew> UnwindInfo.add_saved_register method, which takes a register-id and a
Andrew> register value, the value, it would seem, also has to be a gdb.Value.
Andrew> I haven't tried to change that, but I could take a look.
Don't do it on my account. IIRC this was a mild inconvenience when
writing the SpiderMonkey unwinder, but now that that exists the problem
doesn't seem so important :-)
Tom
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 09/10] gdb/python: Allow gdb.UnwindInfo to be created with non gdb.Value args
2023-03-10 14:55 ` [PATCH 09/10] gdb/python: Allow gdb.UnwindInfo to be created with non gdb.Value args Andrew Burgess
2023-03-10 15:34 ` Tom Tromey
@ 2023-03-10 15:38 ` Eli Zaretskii
1 sibling, 0 replies; 37+ messages in thread
From: Eli Zaretskii @ 2023-03-10 15:38 UTC (permalink / raw)
To: Andrew Burgess; +Cc: gdb-patches
> Cc: Andrew Burgess <aburgess@redhat.com>
> Date: Fri, 10 Mar 2023 14:55:26 +0000
> From: Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org>
>
> gdb/NEWS | 4 +
> gdb/doc/python.texi | 3 +-
> gdb/python/py-unwind.c | 113 ++++++++++++++-----------
> gdb/testsuite/gdb.python/py-unwind.exp | 36 ++++++++
> gdb/testsuite/gdb.python/py-unwind.py | 6 +-
> 5 files changed, 111 insertions(+), 51 deletions(-)
>
> diff --git a/gdb/NEWS b/gdb/NEWS
> index 0d9049ff134..c4f7de11c6e 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -130,6 +130,10 @@ show always-read-ctf
> - gdb.PendingFrame.function(): Return a gdb.Symbol for the
> current pending frame, or None.
>
> + ** The frame-id passed to gdb.PendingFrame.create_unwind_info can
> + now use either an integer or a gdb.Value object for each of its
> + 'sp', 'pc', and 'special' attributes.
> +
> *** Changes in GDB 13
>
> * MI version 1 is deprecated, and will be removed in GDB 14.
> diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
> index 1c4239841af..3074b5250a3 100644
> --- a/gdb/doc/python.texi
> +++ b/gdb/doc/python.texi
> @@ -2815,7 +2815,8 @@
> this.
> @end table
>
> -Each attribute value should be an instance of @code{gdb.Value}.
> +Each attribute value should either be an instance of @code{gdb.Value}
> +or an integer.
>
> @end defun
OK for the documentation parts, thanks.
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
^ permalink raw reply [flat|nested] 37+ messages in thread
* [PATCH 10/10] gdb/python: Add new gdb.unwinder.FrameId class
2023-03-10 14:55 [PATCH 00/10] Improvements & Cleanup For Python Unwinder API Andrew Burgess
` (8 preceding siblings ...)
2023-03-10 14:55 ` [PATCH 09/10] gdb/python: Allow gdb.UnwindInfo to be created with non gdb.Value args Andrew Burgess
@ 2023-03-10 14:55 ` Andrew Burgess
2023-03-10 15:36 ` Eli Zaretskii
2023-03-29 16:27 ` [PATCH 00/10] Improvements & Cleanup For Python Unwinder API Tom Tromey
10 siblings, 1 reply; 37+ messages in thread
From: Andrew Burgess @ 2023-03-10 14:55 UTC (permalink / raw)
To: gdb-patches; +Cc: Andrew Burgess
When writing an unwinder it is necessary to create a new class to act
as a frame-id. This new class is almost certainly just going to set a
'sp' and 'pc' attribute within the instance.
This commit adds a little helper class gdb.unwinder.FrameId that does
this job. Users can make use of this to avoid having to write out
standard boilerplate code any time they write an unwinder.
Of course, if the user wants their FrameId class to be more
complicated in some way, then they can still write their own class,
just like they could before.
I've simplified the example code in the documentation to now use the
new helper class, and I've also made use of this helper within the
testsuite.
Any existing user code will continue to work just as it did before
after this change.
---
gdb/NEWS | 5 +++
gdb/doc/python.texi | 49 ++++++++++++++++++++++-----
gdb/python/lib/gdb/unwinder.py | 26 ++++++++++++++
gdb/testsuite/gdb.python/py-unwind.py | 16 +--------
4 files changed, 73 insertions(+), 23 deletions(-)
diff --git a/gdb/NEWS b/gdb/NEWS
index c4f7de11c6e..c5aa256c1bd 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -134,6 +134,11 @@ show always-read-ctf
now use either an integer or a gdb.Value object for each of its
'sp', 'pc', and 'special' attributes.
+ ** A new class gdb.unwinder.FrameId has been added. Instrances of
+ this class are constructed with 'sp' (stack-pointer) and 'pc'
+ (program-counter) values, and can be used as the frame-id when
+ calling gdb.PendingFrame.create_unwind_info.
+
*** Changes in GDB 13
* MI version 1 is deprecated, and will be removed in GDB 14.
diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index 3074b5250a3..bbcaf3fd166 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -2782,6 +2782,7 @@
It also provides a factory method to create a @code{gdb.UnwindInfo}
instance to be returned to @value{GDBN}:
+@anchor{gdb.PendingFrame.create_unwind_info}
@defun PendingFrame.create_unwind_info (frame_id)
Returns a new @code{gdb.UnwindInfo} instance identified by given
@var{frame_id}. The @var{frame_id} is used internally by @value{GDBN}
@@ -2818,6 +2819,10 @@
Each attribute value should either be an instance of @code{gdb.Value}
or an integer.
+A helper class is provided in the @code{gdb.unwinder} module that can
+be used to represent a frame-id
+(@pxref{gdb.unwinder.FrameId,,@code{gdb.unwinder.FrameId}}).
+
@end defun
@defun PendingFrame.architecture ()
@@ -2886,7 +2891,7 @@
@subheading The @code{gdb.unwinder} Module
@value{GDBN} comes with a @code{gdb.unwinder} module which contains
-the following class:
+the following classes:
@deftp {class} gdb.unwinder.Unwinder
The @code{Unwinder} class is a base class from which user created
@@ -2910,6 +2915,40 @@
@end defvar
@end deftp
+@anchor{gdb.unwinder.FrameId}
+@deftp {class} gdb.unwinder.FrameId
+This is a class suitable for being used as the frame-id when calling
+@code{gdb.PendingFrame.create_unwind_info}. It is not required to use
+this class, any class with the required attribute
+(@pxref{gdb.PendingFrame.create_unwind_info,,@code{gdb.PendingFrame.create_unwind_info}})
+will be accepted, but in most cases this class will be sufficient.
+
+@code{gdb.unwinder.FrameId} has the following method:
+
+@defun gdb.unwinder.FrameId.__init__(@var{sp}, @var{pc}, @var{special} = @code{None})
+The @var{sp} and @var{pc} arguments are required and should be either
+a @code{gdb.Value} object, or an integer.
+
+The @var{special} argument is optional, if specified it should be a
+@code{gdb.Value} object, or an integer.
+@end defun
+
+@code{gdb.unwinder.FrameId} has the following read-only attributes:
+
+@defvar gdb.unwinder.sp
+The @var{sp} value passed to the constructor.
+@end defvar
+
+@defvar gdb.unwinder.pc
+The @var{pc} value passed to the constructor.
+@end defvar
+
+@defvar gdb.unwinder.special
+The @var{special} value passed to the constructor, or @code{None} if
+no such value was passed.
+@end defvar
+@end deftp
+
@subheading Registering an Unwinder
Object files and program spaces can have unwinders registered with
@@ -2943,13 +2982,7 @@
Here is an example of how and structure a user created unwinder:
@smallexample
-from gdb.unwinder import Unwinder
-
-class FrameId(object):
- def __init__(self, sp, pc):
- self.sp = sp
- self.pc = pc
-
+from gdb.unwinder import Unwinder, FrameId
class MyUnwinder(Unwinder):
def __init__(self):
diff --git a/gdb/python/lib/gdb/unwinder.py b/gdb/python/lib/gdb/unwinder.py
index 1303245c054..140b84d3374 100644
--- a/gdb/python/lib/gdb/unwinder.py
+++ b/gdb/python/lib/gdb/unwinder.py
@@ -69,6 +69,32 @@ class Unwinder(object):
raise NotImplementedError("Unwinder __call__.")
+class FrameId(object):
+ """A Frame-ID class for use when creating gdb.UnwindInfo objects.
+
+ Attributes (all read-only):
+ pc: Program counter value.
+ sp: The stack-pointer value.
+ special: An alternative stack-pointer value, can be None."""
+
+ def __init__(self, sp, pc, special=None):
+ self._sp = sp
+ self._pc = pc
+ self._special = special
+
+ @property
+ def sp(self):
+ return self._sp
+
+ @property
+ def pc(self):
+ return self._pc
+
+ @property
+ def special(self):
+ return self._special
+
+
def register_unwinder(locus, unwinder, replace=False):
"""Register unwinder in given locus.
diff --git a/gdb/testsuite/gdb.python/py-unwind.py b/gdb/testsuite/gdb.python/py-unwind.py
index dbabb006e4b..4e110c51e3b 100644
--- a/gdb/testsuite/gdb.python/py-unwind.py
+++ b/gdb/testsuite/gdb.python/py-unwind.py
@@ -14,7 +14,7 @@
# along with this program. If not, see <http://www.gnu.org/licenses/>.
import gdb
-from gdb.unwinder import Unwinder
+from gdb.unwinder import Unwinder, FrameId
# These are set to test whether invalid register names cause an error.
@@ -22,20 +22,6 @@ add_saved_register_error = False
read_register_error = False
-class FrameId(object):
- def __init__(self, sp, pc):
- self._sp = sp
- self._pc = pc
-
- @property
- def sp(self):
- return self._sp
-
- @property
- def pc(self):
- return self._pc
-
-
class TestUnwinder(Unwinder):
AMD64_RBP = 6
AMD64_RSP = 7
--
2.25.4
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 10/10] gdb/python: Add new gdb.unwinder.FrameId class
2023-03-10 14:55 ` [PATCH 10/10] gdb/python: Add new gdb.unwinder.FrameId class Andrew Burgess
@ 2023-03-10 15:36 ` Eli Zaretskii
2023-03-14 10:58 ` Andrew Burgess
0 siblings, 1 reply; 37+ messages in thread
From: Eli Zaretskii @ 2023-03-10 15:36 UTC (permalink / raw)
To: Andrew Burgess; +Cc: gdb-patches
> Cc: Andrew Burgess <aburgess@redhat.com>
> Date: Fri, 10 Mar 2023 14:55:27 +0000
> From: Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org>
>
> When writing an unwinder it is necessary to create a new class to act
> as a frame-id. This new class is almost certainly just going to set a
> 'sp' and 'pc' attribute within the instance.
>
> This commit adds a little helper class gdb.unwinder.FrameId that does
> this job. Users can make use of this to avoid having to write out
> standard boilerplate code any time they write an unwinder.
>
> Of course, if the user wants their FrameId class to be more
> complicated in some way, then they can still write their own class,
> just like they could before.
>
> I've simplified the example code in the documentation to now use the
> new helper class, and I've also made use of this helper within the
> testsuite.
>
> Any existing user code will continue to work just as it did before
> after this change.
> ---
> gdb/NEWS | 5 +++
> gdb/doc/python.texi | 49 ++++++++++++++++++++++-----
> gdb/python/lib/gdb/unwinder.py | 26 ++++++++++++++
> gdb/testsuite/gdb.python/py-unwind.py | 16 +--------
> 4 files changed, 73 insertions(+), 23 deletions(-)
Thanks.
> diff --git a/gdb/NEWS b/gdb/NEWS
> index c4f7de11c6e..c5aa256c1bd 100644
> --- a/gdb/NEWS
> +++ b/gdb/NEWS
> @@ -134,6 +134,11 @@ show always-read-ctf
> now use either an integer or a gdb.Value object for each of its
> 'sp', 'pc', and 'special' attributes.
>
> + ** A new class gdb.unwinder.FrameId has been added. Instrances of
^^^^^^^^^^
A typo.
> +A helper class is provided in the @code{gdb.unwinder} module that can
> +be used to represent a frame-id
> +(@pxref{gdb.unwinder.FrameId,,@code{gdb.unwinder.FrameId}}).
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Is this justified? does using the 3rd arg of @pxref produce a much
better cross-reference, in both Info and printed formats?
> +The @var{special} argument is optional, if specified it should be a
^ ^
Semi-colon there, and a comma missing after "if specified".
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 10/10] gdb/python: Add new gdb.unwinder.FrameId class
2023-03-10 15:36 ` Eli Zaretskii
@ 2023-03-14 10:58 ` Andrew Burgess
2023-03-14 13:00 ` Eli Zaretskii
0 siblings, 1 reply; 37+ messages in thread
From: Andrew Burgess @ 2023-03-14 10:58 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: gdb-patches
Eli Zaretskii <eliz@gnu.org> writes:
>> Cc: Andrew Burgess <aburgess@redhat.com>
>> Date: Fri, 10 Mar 2023 14:55:27 +0000
>> From: Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org>
>>
>> When writing an unwinder it is necessary to create a new class to act
>> as a frame-id. This new class is almost certainly just going to set a
>> 'sp' and 'pc' attribute within the instance.
>>
>> This commit adds a little helper class gdb.unwinder.FrameId that does
>> this job. Users can make use of this to avoid having to write out
>> standard boilerplate code any time they write an unwinder.
>>
>> Of course, if the user wants their FrameId class to be more
>> complicated in some way, then they can still write their own class,
>> just like they could before.
>>
>> I've simplified the example code in the documentation to now use the
>> new helper class, and I've also made use of this helper within the
>> testsuite.
>>
>> Any existing user code will continue to work just as it did before
>> after this change.
>> ---
>> gdb/NEWS | 5 +++
>> gdb/doc/python.texi | 49 ++++++++++++++++++++++-----
>> gdb/python/lib/gdb/unwinder.py | 26 ++++++++++++++
>> gdb/testsuite/gdb.python/py-unwind.py | 16 +--------
>> 4 files changed, 73 insertions(+), 23 deletions(-)
>
> Thanks.
>
>> diff --git a/gdb/NEWS b/gdb/NEWS
>> index c4f7de11c6e..c5aa256c1bd 100644
>> --- a/gdb/NEWS
>> +++ b/gdb/NEWS
>> @@ -134,6 +134,11 @@ show always-read-ctf
>> now use either an integer or a gdb.Value object for each of its
>> 'sp', 'pc', and 'special' attributes.
>>
>> + ** A new class gdb.unwinder.FrameId has been added. Instrances of
> ^^^^^^^^^^
> A typo.
Fixed.
>
>> +A helper class is provided in the @code{gdb.unwinder} module that can
>> +be used to represent a frame-id
>> +(@pxref{gdb.unwinder.FrameId,,@code{gdb.unwinder.FrameId}}).
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> Is this justified? does using the 3rd arg of @pxref produce a much
> better cross-reference, in both Info and printed formats?
Removed use of the third arg.
>
>> +The @var{special} argument is optional, if specified it should be a
> ^ ^
> Semi-colon there, and a comma missing after "if specified".
Fixed.
>
> Reviewed-By: Eli Zaretskii <eliz@gnu.org>
I added you tag, but the updated patch is below in case you wanted
another look.
Thanks,
Andrew
---
commit ecab23fd2e2e7644835cdf526ad49ebc574167ed
Author: Andrew Burgess <aburgess@redhat.com>
Date: Fri Mar 10 12:29:58 2023 +0000
gdb/python: Add new gdb.unwinder.FrameId class
When writing an unwinder it is necessary to create a new class to act
as a frame-id. This new class is almost certainly just going to set a
'sp' and 'pc' attribute within the instance.
This commit adds a little helper class gdb.unwinder.FrameId that does
this job. Users can make use of this to avoid having to write out
standard boilerplate code any time they write an unwinder.
Of course, if the user wants their FrameId class to be more
complicated in some way, then they can still write their own class,
just like they could before.
I've simplified the example code in the documentation to now use the
new helper class, and I've also made use of this helper within the
testsuite.
Any existing user code will continue to work just as it did before
after this change.
Reviewed-By: Eli Zaretskii <eliz@gnu.org>
diff --git a/gdb/NEWS b/gdb/NEWS
index 3b2b0e0675c..b64b050e1ed 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -142,6 +142,11 @@ show always-read-ctf
now use either an integer or a gdb.Value object for each of its
'sp', 'pc', and 'special' attributes.
+ ** A new class gdb.unwinder.FrameId has been added. Instances of
+ this class are constructed with 'sp' (stack-pointer) and 'pc'
+ (program-counter) values, and can be used as the frame-id when
+ calling gdb.PendingFrame.create_unwind_info.
+
*** Changes in GDB 13
* MI version 1 is deprecated, and will be removed in GDB 14.
diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index c11aa746184..48c6a148567 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -2782,6 +2782,7 @@
It also provides a factory method to create a @code{gdb.UnwindInfo}
instance to be returned to @value{GDBN}:
+@anchor{gdb.PendingFrame.create_unwind_info}
@defun PendingFrame.create_unwind_info (frame_id)
Returns a new @code{gdb.UnwindInfo} instance identified by given
@var{frame_id}. The @var{frame_id} is used internally by @value{GDBN}
@@ -2818,6 +2819,10 @@
Each attribute value should either be an instance of @code{gdb.Value}
or an integer.
+A helper class is provided in the @code{gdb.unwinder} module that can
+be used to represent a frame-id
+(@pxref{gdb.unwinder.FrameId}).
+
@end defun
@defun PendingFrame.architecture ()
@@ -2886,7 +2891,7 @@
@subheading The @code{gdb.unwinder} Module
@value{GDBN} comes with a @code{gdb.unwinder} module which contains
-the following class:
+the following classes:
@deftp {class} gdb.unwinder.Unwinder
The @code{Unwinder} class is a base class from which user created
@@ -2910,6 +2915,40 @@
@end defvar
@end deftp
+@anchor{gdb.unwinder.FrameId}
+@deftp {class} gdb.unwinder.FrameId
+This is a class suitable for being used as the frame-id when calling
+@code{gdb.PendingFrame.create_unwind_info}. It is not required to use
+this class, any class with the required attribute
+(@pxref{gdb.PendingFrame.create_unwind_info}) will be accepted, but in
+most cases this class will be sufficient.
+
+@code{gdb.unwinder.FrameId} has the following method:
+
+@defun gdb.unwinder.FrameId.__init__(@var{sp}, @var{pc}, @var{special} = @code{None})
+The @var{sp} and @var{pc} arguments are required and should be either
+a @code{gdb.Value} object, or an integer.
+
+The @var{special} argument is optional; if specified, it should be a
+@code{gdb.Value} object, or an integer.
+@end defun
+
+@code{gdb.unwinder.FrameId} has the following read-only attributes:
+
+@defvar gdb.unwinder.sp
+The @var{sp} value passed to the constructor.
+@end defvar
+
+@defvar gdb.unwinder.pc
+The @var{pc} value passed to the constructor.
+@end defvar
+
+@defvar gdb.unwinder.special
+The @var{special} value passed to the constructor, or @code{None} if
+no such value was passed.
+@end defvar
+@end deftp
+
@subheading Registering an Unwinder
Object files and program spaces can have unwinders registered with
@@ -2941,13 +2980,7 @@
Here is an example of how to structure a user created unwinder:
@smallexample
-from gdb.unwinder import Unwinder
-
-class FrameId(object):
- def __init__(self, sp, pc):
- self.sp = sp
- self.pc = pc
-
+from gdb.unwinder import Unwinder, FrameId
class MyUnwinder(Unwinder):
def __init__(self):
diff --git a/gdb/python/lib/gdb/unwinder.py b/gdb/python/lib/gdb/unwinder.py
index 1303245c054..140b84d3374 100644
--- a/gdb/python/lib/gdb/unwinder.py
+++ b/gdb/python/lib/gdb/unwinder.py
@@ -69,6 +69,32 @@ class Unwinder(object):
raise NotImplementedError("Unwinder __call__.")
+class FrameId(object):
+ """A Frame-ID class for use when creating gdb.UnwindInfo objects.
+
+ Attributes (all read-only):
+ pc: Program counter value.
+ sp: The stack-pointer value.
+ special: An alternative stack-pointer value, can be None."""
+
+ def __init__(self, sp, pc, special=None):
+ self._sp = sp
+ self._pc = pc
+ self._special = special
+
+ @property
+ def sp(self):
+ return self._sp
+
+ @property
+ def pc(self):
+ return self._pc
+
+ @property
+ def special(self):
+ return self._special
+
+
def register_unwinder(locus, unwinder, replace=False):
"""Register unwinder in given locus.
diff --git a/gdb/testsuite/gdb.python/py-unwind.py b/gdb/testsuite/gdb.python/py-unwind.py
index dbabb006e4b..4e110c51e3b 100644
--- a/gdb/testsuite/gdb.python/py-unwind.py
+++ b/gdb/testsuite/gdb.python/py-unwind.py
@@ -14,7 +14,7 @@
# along with this program. If not, see <http://www.gnu.org/licenses/>.
import gdb
-from gdb.unwinder import Unwinder
+from gdb.unwinder import Unwinder, FrameId
# These are set to test whether invalid register names cause an error.
@@ -22,20 +22,6 @@ add_saved_register_error = False
read_register_error = False
-class FrameId(object):
- def __init__(self, sp, pc):
- self._sp = sp
- self._pc = pc
-
- @property
- def sp(self):
- return self._sp
-
- @property
- def pc(self):
- return self._pc
-
-
class TestUnwinder(Unwinder):
AMD64_RBP = 6
AMD64_RSP = 7
^ permalink raw reply [flat|nested] 37+ messages in thread* Re: [PATCH 10/10] gdb/python: Add new gdb.unwinder.FrameId class
2023-03-14 10:58 ` Andrew Burgess
@ 2023-03-14 13:00 ` Eli Zaretskii
0 siblings, 0 replies; 37+ messages in thread
From: Eli Zaretskii @ 2023-03-14 13:00 UTC (permalink / raw)
To: Andrew Burgess; +Cc: gdb-patches
> From: Andrew Burgess <aburgess@redhat.com>
> Cc: gdb-patches@sourceware.org
> Date: Tue, 14 Mar 2023 10:58:07 +0000
>
> > Reviewed-By: Eli Zaretskii <eliz@gnu.org>
>
> I added you tag, but the updated patch is below in case you wanted
> another look.
The docs parts are okay, thanks.
^ permalink raw reply [flat|nested] 37+ messages in thread
* Re: [PATCH 00/10] Improvements & Cleanup For Python Unwinder API
2023-03-10 14:55 [PATCH 00/10] Improvements & Cleanup For Python Unwinder API Andrew Burgess
` (9 preceding siblings ...)
2023-03-10 14:55 ` [PATCH 10/10] gdb/python: Add new gdb.unwinder.FrameId class Andrew Burgess
@ 2023-03-29 16:27 ` Tom Tromey
10 siblings, 0 replies; 37+ messages in thread
From: Tom Tromey @ 2023-03-29 16:27 UTC (permalink / raw)
To: Andrew Burgess via Gdb-patches; +Cc: Andrew Burgess
>>>>> "Andrew" == Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org> writes:
Andrew> For <reasons> I ended up writing a small project using GDB's Unwinder
Andrew> Python API. Though I could do everything I wanted, GDB's
Andrew> documentation in this area seemed a little lacking.
Andrew> So I started out just planning to cleanup the docs.
Andrew> But based on my experiences using the API, and after reading the docs
Andrew> through a couple of times, it felt like there were some small
Andrew> improvements that could be made to the API to make the lives of users
Andrew> easier.
Andrew> This series doesn't make any real changes to the API, but after this
Andrew> series the docs should be more complete, and the API a little more
Andrew> robust and easier to use.
Andrew> Oh, and tests. Lots more tests.
FWIW, I read through these and it all looked reasonable to me.
Reviewed-By: Tom Tromey <tom@tromey.com>
thanks,
Tom
^ permalink raw reply [flat|nested] 37+ messages in thread