[02/10] gdb/python: make the gdb.unwinder.Unwinder class more robust

Message ID af80968e052a4478698bfb2177c1eca6d0f940f6.1678460067.git.aburgess@redhat.com
State New
Headers
Series Improvements & Cleanup For Python Unwinder API |

Commit Message

Andrew Burgess March 10, 2023, 2:55 p.m. UTC
  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(-)
  

Comments

Eli Zaretskii March 10, 2023, 3:32 p.m. UTC | #1
> 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>
  
Andrew Burgess March 14, 2023, 10:06 a.m. UTC | #2
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")
  
Eli Zaretskii March 14, 2023, 12:57 p.m. UTC | #3
> 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.
  
Simon Marchi March 31, 2023, 2:15 a.m. UTC | #4
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
  
Andrew Burgess April 3, 2023, 10:02 a.m. UTC | #5
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
  

Patch

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")