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

[Andrew Burgess <aburgess@redhat.com>]

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