[PATCH] gdb/python: print name of unwinder that claimed frame in debug message

[PATCH] gdb/python: print name of unwinder that claimed frame in debug message

[Simon Marchi]

If we have multiple registered unwinders, this will helps identify which
unwinder was chosen and make it easier to track down potential problems.
Unwinders have a mandatory name argument, which we can use in the
message.

First, make gdb._execute_unwinders return a tuple containing the name,
in addition to the UnwindInfo.  Then, make pyuw_sniffer include the name
in the debug message.

I moved the debug message earlier.  I think it's good to print it as
early as possible, so that we see it in case an assert is hit in the
loop below, for example.

gdb/ChangeLog:

	* python/lib/gdb/__init__.py (_execute_unwinders): Return tuple
	with name of chosen unwinder.
	* python/py-unwind.c (pyuw_sniffer): Print name of chosen
	unwinder in debug message.

Change-Id: Id603545b44a97df2a39dd1872fe1f38ad5059f03
---
 gdb/python/lib/gdb/__init__.py | 14 ++++++++++----
 gdb/python/py-unwind.c         | 32 ++++++++++++++++++++++++++------
 2 files changed, 36 insertions(+), 10 deletions(-)

diff --git a/gdb/python/lib/gdb/__init__.py b/gdb/python/lib/gdb/__init__.py
index d748b3a5827e..f2f38b32af96 100644
--- a/gdb/python/lib/gdb/__init__.py
+++ b/gdb/python/lib/gdb/__init__.py
@@ -90,27 +90,33 @@ def _execute_unwinders(pending_frame):
 
     Arguments:
         pending_frame: gdb.PendingFrame instance.
+
     Returns:
-        gdb.UnwindInfo instance or None.
+        Tuple with:
+
+	  [0] gdb.UnwindInfo instance
+	  [1] Name of unwinder that claimed the frame (type `str`)
+
+	or None, if no unwinder has claimed the frame.
     """
     for objfile in objfiles():
         for unwinder in objfile.frame_unwinders:
             if unwinder.enabled:
                 unwind_info = unwinder(pending_frame)
                 if unwind_info is not None:
-                    return unwind_info
+                    return (unwind_info, unwinder.name)
 
     for unwinder in current_progspace().frame_unwinders:
         if unwinder.enabled:
             unwind_info = unwinder(pending_frame)
             if unwind_info is not None:
-                return unwind_info
+                return (unwind_info, unwinder.name)
 
     for unwinder in frame_unwinders:
         if unwinder.enabled:
             unwind_info = unwinder(pending_frame)
             if unwind_info is not None:
-                return unwind_info
+                return (unwind_info, unwinder.name)
 
     return None
 
diff --git a/gdb/python/py-unwind.c b/gdb/python/py-unwind.c
index 7c195eb539da..7c8b73c2e680 100644
--- a/gdb/python/py-unwind.c
+++ b/gdb/python/py-unwind.c
@@ -524,27 +524,48 @@ pyuw_sniffer (const struct frame_unwind *self, struct frame_info *this_frame,
       return 0;
     }
 
-  gdbpy_ref<> pyo_unwind_info
+  /* A (gdb.UnwindInfo, str) tuple, or None.  */
+  gdbpy_ref<> pyo_execute_ret
     (PyObject_CallFunctionObjArgs (pyo_execute.get (),
 				   pyo_pending_frame.get (), NULL));
-  if (pyo_unwind_info == NULL)
+  if (pyo_execute_ret == nullptr)
     {
       /* If the unwinder is cancelled due to a Ctrl-C, then propagate
 	 the Ctrl-C as a GDB exception instead of swallowing it.  */
       gdbpy_print_stack_or_quit ();
       return 0;
     }
-  if (pyo_unwind_info == Py_None)
+  if (pyo_execute_ret == Py_None)
     return 0;
 
+  /* Verify the return value of _execute_unwinders is a tuple of size 2.  */
+  gdb_assert (PyTuple_Check (pyo_execute_ret.get ()));
+  gdb_assert (PyTuple_GET_SIZE (pyo_execute_ret.get ()) == 2);
+
+  if (pyuw_debug)
+    {
+      PyObject *pyo_unwinder_name = PyTuple_GET_ITEM (pyo_execute_ret.get (), 1);
+      const char *name;
+
+      /* We never enforce that the name is a string, so check the type just
+         in case somebody passed something else.  */
+      if (PyUnicode_Check (pyo_unwinder_name))
+	name = PyUnicode_AsUTF8 (pyo_unwinder_name);
+      else
+	name = "<unwinder name is not a string>";
+
+      pyuw_debug_printf ("frame claimed by unwinder %s", name);
+    }
+
   /* Received UnwindInfo, cache data.  */
-  if (PyObject_IsInstance (pyo_unwind_info.get (),
+  PyObject *pyo_unwind_info = PyTuple_GET_ITEM (pyo_execute_ret.get (), 0);
+  if (PyObject_IsInstance (pyo_unwind_info,
 			   (PyObject *) &unwind_info_object_type) <= 0)
     error (_("A Unwinder should return gdb.UnwindInfo instance."));
 
   {
     unwind_info_object *unwind_info =
-      (unwind_info_object *) pyo_unwind_info.get ();
+      (unwind_info_object *) pyo_unwind_info;
     int reg_count = unwind_info->saved_regs->size ();
 
     cached_frame
@@ -575,7 +596,6 @@ pyuw_sniffer (const struct frame_unwind *self, struct frame_info *this_frame,
   }
 
   *cache_ptr = cached_frame;
-  pyuw_debug_printf ("frame claimed");
   return 1;
 }
 
-- 
2.30.1

Re: [PATCH] gdb/python: print name of unwinder that claimed frame in debug message

[Andrew Burgess]

* Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> [2021-05-10 11:49:31 -0400]:

> If we have multiple registered unwinders, this will helps identify which
> unwinder was chosen and make it easier to track down potential problems.
> Unwinders have a mandatory name argument, which we can use in the
> message.
> 
> First, make gdb._execute_unwinders return a tuple containing the name,
> in addition to the UnwindInfo.  Then, make pyuw_sniffer include the name
> in the debug message.
> 
> I moved the debug message earlier.  I think it's good to print it as
> early as possible, so that we see it in case an assert is hit in the
> loop below, for example.
> 
> gdb/ChangeLog:
> 
> 	* python/lib/gdb/__init__.py (_execute_unwinders): Return tuple
> 	with name of chosen unwinder.
> 	* python/py-unwind.c (pyuw_sniffer): Print name of chosen
> 	unwinder in debug message.
> 
> Change-Id: Id603545b44a97df2a39dd1872fe1f38ad5059f03
> ---
>  gdb/python/lib/gdb/__init__.py | 14 ++++++++++----
>  gdb/python/py-unwind.c         | 32 ++++++++++++++++++++++++++------
>  2 files changed, 36 insertions(+), 10 deletions(-)
> 
> diff --git a/gdb/python/lib/gdb/__init__.py b/gdb/python/lib/gdb/__init__.py
> index d748b3a5827e..f2f38b32af96 100644
> --- a/gdb/python/lib/gdb/__init__.py
> +++ b/gdb/python/lib/gdb/__init__.py
> @@ -90,27 +90,33 @@ def _execute_unwinders(pending_frame):
>  
>      Arguments:
>          pending_frame: gdb.PendingFrame instance.
> +
>      Returns:
> -        gdb.UnwindInfo instance or None.
> +        Tuple with:
> +
> +	  [0] gdb.UnwindInfo instance
> +	  [1] Name of unwinder that claimed the frame (type `str`)
> +
> +	or None, if no unwinder has claimed the frame.
>      """
>      for objfile in objfiles():
>          for unwinder in objfile.frame_unwinders:
>              if unwinder.enabled:
>                  unwind_info = unwinder(pending_frame)
>                  if unwind_info is not None:
> -                    return unwind_info
> +                    return (unwind_info, unwinder.name)
>  
>      for unwinder in current_progspace().frame_unwinders:
>          if unwinder.enabled:
>              unwind_info = unwinder(pending_frame)
>              if unwind_info is not None:
> -                return unwind_info
> +                return (unwind_info, unwinder.name)
>  
>      for unwinder in frame_unwinders:
>          if unwinder.enabled:
>              unwind_info = unwinder(pending_frame)
>              if unwind_info is not None:
> -                return unwind_info
> +                return (unwind_info, unwinder.name)
>  
>      return None
>  
> diff --git a/gdb/python/py-unwind.c b/gdb/python/py-unwind.c
> index 7c195eb539da..7c8b73c2e680 100644
> --- a/gdb/python/py-unwind.c
> +++ b/gdb/python/py-unwind.c
> @@ -524,27 +524,48 @@ pyuw_sniffer (const struct frame_unwind *self, struct frame_info *this_frame,
>        return 0;
>      }
>  
> -  gdbpy_ref<> pyo_unwind_info
> +  /* A (gdb.UnwindInfo, str) tuple, or None.  */
> +  gdbpy_ref<> pyo_execute_ret
>      (PyObject_CallFunctionObjArgs (pyo_execute.get (),
>  				   pyo_pending_frame.get (), NULL));
> -  if (pyo_unwind_info == NULL)
> +  if (pyo_execute_ret == nullptr)
>      {
>        /* If the unwinder is cancelled due to a Ctrl-C, then propagate
>  	 the Ctrl-C as a GDB exception instead of swallowing it.  */
>        gdbpy_print_stack_or_quit ();
>        return 0;
>      }
> -  if (pyo_unwind_info == Py_None)
> +  if (pyo_execute_ret == Py_None)
>      return 0;
>  
> +  /* Verify the return value of _execute_unwinders is a tuple of size 2.  */
> +  gdb_assert (PyTuple_Check (pyo_execute_ret.get ()));
> +  gdb_assert (PyTuple_GET_SIZE (pyo_execute_ret.get ()) == 2);

Maybe being a little paranoid, but I wonder if we should avoid
asserting on the interface between GDB and Python code?  In theory I
guess a user could modify the Python code in some incompatible way.
Perhaps a check and error (...) call would be better?

Otherwise, this looks good to me.

Thanks,
Andrew


> +
> +  if (pyuw_debug)
> +    {
> +      PyObject *pyo_unwinder_name = PyTuple_GET_ITEM (pyo_execute_ret.get (), 1);
> +      const char *name;
> +
> +      /* We never enforce that the name is a string, so check the type just
> +         in case somebody passed something else.  */
> +      if (PyUnicode_Check (pyo_unwinder_name))
> +	name = PyUnicode_AsUTF8 (pyo_unwinder_name);
> +      else
> +	name = "<unwinder name is not a string>";
> +
> +      pyuw_debug_printf ("frame claimed by unwinder %s", name);
> +    }
> +
>    /* Received UnwindInfo, cache data.  */
> -  if (PyObject_IsInstance (pyo_unwind_info.get (),
> +  PyObject *pyo_unwind_info = PyTuple_GET_ITEM (pyo_execute_ret.get (), 0);
> +  if (PyObject_IsInstance (pyo_unwind_info,
>  			   (PyObject *) &unwind_info_object_type) <= 0)
>      error (_("A Unwinder should return gdb.UnwindInfo instance."));
>  
>    {
>      unwind_info_object *unwind_info =
> -      (unwind_info_object *) pyo_unwind_info.get ();
> +      (unwind_info_object *) pyo_unwind_info;
>      int reg_count = unwind_info->saved_regs->size ();
>  
>      cached_frame
> @@ -575,7 +596,6 @@ pyuw_sniffer (const struct frame_unwind *self, struct frame_info *this_frame,
>    }
>  
>    *cache_ptr = cached_frame;
> -  pyuw_debug_printf ("frame claimed");
>    return 1;
>  }
>  
> -- 
> 2.30.1
> 

Re: [PATCH] gdb/python: print name of unwinder that claimed frame in debug message

[Simon Marchi]

On 2021-05-10 12:45 p.m., Andrew Burgess wrote:
> Maybe being a little paranoid, but I wonder if we should avoid
> asserting on the interface between GDB and Python code?  In theory I
> guess a user could modify the Python code in some incompatible way.
> Perhaps a check and error (...) call would be better?

I thought about this, and in my opinion the GDB Python code is
"internal" code as much as the C code.  If a user modifies the Python
code we ship, then they modify GDB, it's their problem.  To me, it's the
same as if they modified the C source code, rebuilt GDB, and the
complained about an assertion failure that would happen as a result of
their change.

> Otherwise, this looks good to me.

Thanks, I'll push it as-is.

Simon

Re: [PATCH] gdb/python: print name of unwinder that claimed frame in debug message

[Simon Marchi]

On 2021-05-10 12:53 p.m., Simon Marchi via Gdb-patches wrote:
> On 2021-05-10 12:45 p.m., Andrew Burgess wrote:
>> Maybe being a little paranoid, but I wonder if we should avoid
>> asserting on the interface between GDB and Python code?  In theory I
>> guess a user could modify the Python code in some incompatible way.
>> Perhaps a check and error (...) call would be better?
> 
> I thought about this, and in my opinion the GDB Python code is
> "internal" code as much as the C code.  If a user modifies the Python
> code we ship, then they modify GDB, it's their problem.  To me, it's the
> same as if they modified the C source code, rebuilt GDB, and the
> complained about an assertion failure that would happen as a result of
> their change.
> 
>> Otherwise, this looks good to me.
> 
> Thanks, I'll push it as-is.

Actually, I thought about testing it with Python 2 (we still support
Python 2 right?) and it doesn't build.  I'll address that and send a new
version.

Simon

[PATCH v2] gdb/python: print name of unwinder that claimed frame in debug message

[Simon Marchi]

New in v2:

 - use python_string_to_host_string to get C string from Python string.

If we have multiple registered unwinders, this will helps identify which
unwinder was chosen and make it easier to track down potential problems.
Unwinders have a mandatory name argument, which we can use in the
message.

First, make gdb._execute_unwinders return a tuple containing the name,
in addition to the UnwindInfo.  Then, make pyuw_sniffer include the name
in the debug message.

I moved the debug message earlier.  I think it's good to print it as
early as possible, so that we see it in case an assert is hit in the
loop below, for example.

gdb/ChangeLog:

	* python/lib/gdb/__init__.py (_execute_unwinders): Return tuple
	with name of chosen unwinder.
	* python/py-unwind.c (pyuw_sniffer): Print name of chosen
	unwinder in debug message.

Change-Id: Id603545b44a97df2a39dd1872fe1f38ad5059f03
---
 gdb/ChangeLog                  |  7 +++++++
 gdb/python/lib/gdb/__init__.py | 14 +++++++++----
 gdb/python/py-unwind.c         | 36 +++++++++++++++++++++++++++-------
 3 files changed, 46 insertions(+), 11 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 52d5faac8a9d..067cb6378b9f 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,10 @@
+2021-05-10  Simon Marchi  <simon.marchi@polymtl.ca>
+
+	* python/lib/gdb/__init__.py (_execute_unwinders): Return tuple
+	with name of chosen unwinder.
+	* python/py-unwind.c (pyuw_sniffer): Print name of chosen
+	unwinder in debug message.
+
 2021-05-10  Simon Marchi  <simon.marchi@polymtl.ca>
 
 	* nat/linux-waitpid.c (status_to_str): Show signal name.
diff --git a/gdb/python/lib/gdb/__init__.py b/gdb/python/lib/gdb/__init__.py
index d748b3a5827e..f2f38b32af96 100644
--- a/gdb/python/lib/gdb/__init__.py
+++ b/gdb/python/lib/gdb/__init__.py
@@ -90,27 +90,33 @@ def _execute_unwinders(pending_frame):
 
     Arguments:
         pending_frame: gdb.PendingFrame instance.
+
     Returns:
-        gdb.UnwindInfo instance or None.
+        Tuple with:
+
+	  [0] gdb.UnwindInfo instance
+	  [1] Name of unwinder that claimed the frame (type `str`)
+
+	or None, if no unwinder has claimed the frame.
     """
     for objfile in objfiles():
         for unwinder in objfile.frame_unwinders:
             if unwinder.enabled:
                 unwind_info = unwinder(pending_frame)
                 if unwind_info is not None:
-                    return unwind_info
+                    return (unwind_info, unwinder.name)
 
     for unwinder in current_progspace().frame_unwinders:
         if unwinder.enabled:
             unwind_info = unwinder(pending_frame)
             if unwind_info is not None:
-                return unwind_info
+                return (unwind_info, unwinder.name)
 
     for unwinder in frame_unwinders:
         if unwinder.enabled:
             unwind_info = unwinder(pending_frame)
             if unwind_info is not None:
-                return unwind_info
+                return (unwind_info, unwinder.name)
 
     return None
 
diff --git a/gdb/python/py-unwind.c b/gdb/python/py-unwind.c
index 7c195eb539da..0a997e35072a 100644
--- a/gdb/python/py-unwind.c
+++ b/gdb/python/py-unwind.c
@@ -518,33 +518,56 @@ pyuw_sniffer (const struct frame_unwind *self, struct frame_info *this_frame,
     }
   gdbpy_ref<> pyo_execute (PyObject_GetAttrString (gdb_python_module,
 						   "_execute_unwinders"));
-  if (pyo_execute == NULL)
+  if (pyo_execute == nullptr)
     {
       gdbpy_print_stack ();
       return 0;
     }
 
-  gdbpy_ref<> pyo_unwind_info
+  /* A (gdb.UnwindInfo, str) tuple, or None.  */
+  gdbpy_ref<> pyo_execute_ret
     (PyObject_CallFunctionObjArgs (pyo_execute.get (),
 				   pyo_pending_frame.get (), NULL));
-  if (pyo_unwind_info == NULL)
+  if (pyo_execute_ret == nullptr)
     {
       /* If the unwinder is cancelled due to a Ctrl-C, then propagate
 	 the Ctrl-C as a GDB exception instead of swallowing it.  */
       gdbpy_print_stack_or_quit ();
       return 0;
     }
-  if (pyo_unwind_info == Py_None)
+  if (pyo_execute_ret == Py_None)
     return 0;
 
+  /* Verify the return value of _execute_unwinders is a tuple of size 2.  */
+  gdb_assert (PyTuple_Check (pyo_execute_ret.get ()));
+  gdb_assert (PyTuple_GET_SIZE (pyo_execute_ret.get ()) == 2);
+
+  if (pyuw_debug)
+    {
+      PyObject *pyo_unwinder_name = PyTuple_GET_ITEM (pyo_execute_ret.get (), 1);
+      gdb::unique_xmalloc_ptr<char> name
+	= python_string_to_host_string (pyo_unwinder_name);
+
+      /* This could happen if the user passed something else than a string
+	 as the unwinder's name.  */
+      if (name == nullptr)
+	{
+	  gdbpy_print_stack ();
+	  name = make_unique_xstrdup ("<failed to get unwinder name>");
+	}
+
+      pyuw_debug_printf ("frame claimed by unwinder %s", name.get ());
+    }
+
   /* Received UnwindInfo, cache data.  */
-  if (PyObject_IsInstance (pyo_unwind_info.get (),
+  PyObject *pyo_unwind_info = PyTuple_GET_ITEM (pyo_execute_ret.get (), 0);
+  if (PyObject_IsInstance (pyo_unwind_info,
 			   (PyObject *) &unwind_info_object_type) <= 0)
     error (_("A Unwinder should return gdb.UnwindInfo instance."));
 
   {
     unwind_info_object *unwind_info =
-      (unwind_info_object *) pyo_unwind_info.get ();
+      (unwind_info_object *) pyo_unwind_info;
     int reg_count = unwind_info->saved_regs->size ();
 
     cached_frame
@@ -575,7 +598,6 @@ pyuw_sniffer (const struct frame_unwind *self, struct frame_info *this_frame,
   }
 
   *cache_ptr = cached_frame;
-  pyuw_debug_printf ("frame claimed");
   return 1;
 }
 
-- 
2.30.1

Re: [PATCH v2] gdb/python: print name of unwinder that claimed frame in debug message

[Simon Marchi]

On 2021-05-10 1:23 p.m., Simon Marchi wrote:
> New in v2:
> 
>  - use python_string_to_host_string to get C string from Python string.
> 
> If we have multiple registered unwinders, this will helps identify which
> unwinder was chosen and make it easier to track down potential problems.
> Unwinders have a mandatory name argument, which we can use in the
> message.
> 
> First, make gdb._execute_unwinders return a tuple containing the name,
> in addition to the UnwindInfo.  Then, make pyuw_sniffer include the name
> in the debug message.
> 
> I moved the debug message earlier.  I think it's good to print it as
> early as possible, so that we see it in case an assert is hit in the
> loop below, for example.

I pushed this.

Simon