[PATCH 0/4] Debug printing for some python/*.c files

[PATCH 0/4] Debug printing for some python/*.c files

[Andrew Burgess]

I wanted to add some debug printing to python/py-breakpoint.c so I
thought I'd use the new style debug printing.

As python/py-unwind.c is the only other python/*.c to have debug
printing I thought I'd update that to the new style debug output too.

But to make that change cleanly I needed to get rid of
fprint_frame_id, so I did that.

Then I realised the debug printing in py-unwind.c wasn't documented in
the manual, so I did that too.

---

Andrew Burgess (4):
  gdb: replace fprint_frame_id
  gdb/py: convert debug logging in py-unwind to use new scheme
  gdb/py: add some debugging to py-breakpoint.c
  gdb/doc: document 'set debug py-unwind'

 gdb/ChangeLog              | 45 ++++++++++++++++++++
 gdb/doc/ChangeLog          | 10 +++++
 gdb/doc/python.texi        | 18 ++++++++
 gdb/dummy-frame.c          |  3 +-
 gdb/frame.c                | 84 ++++++++++++++++----------------------
 gdb/frame.h                |  8 ++--
 gdb/guile/scm-frame.c      | 11 ++---
 gdb/python/py-breakpoint.c | 59 ++++++++++++++++++++++++--
 gdb/python/py-frame.c      |  6 +--
 gdb/python/py-unwind.c     | 60 +++++++++++++++++----------
 10 files changed, 212 insertions(+), 92 deletions(-)

-- 
2.25.4

[PATCH 1/4] gdb: replace fprint_frame_id

[Andrew Burgess]

Replace fprint_frame_id with a member function frame_id::to_string
that returns a std::string.  Convert all of the previous users of
fprint_frame_id to use the new member function.  This means that
instead of writing things like this:

  fprintf_unfiltered (file, " id=");
  fprint_frame_id (file, s->id.id);

We can write this:

  fprintf_unfiltered (file, " id=%s", s->id.id.to_string ().c_str ());

There should be no user visible changes after this commit.

gdb/ChangeLog:

	* dummy-frame.c (fprint_dummy_frames): Convert use of
	fprint_frame_id to use frame_id::to_string.
	* frame.c (fprint_field): Delete.
	(fprint_frame_id): Moved to...
	(frame_id::to_string): ...this, rewritten to return a string.
	(fprint_frame): Convert use of fprint_frame_id to use
	frame_id::to_string.
	(compute_frame_id): Likewise.
	(frame_id_p): Likewise.
	(frame_id_eq): Likewise.
	(frame_id_inner): Likewise.
	* frame.h (struct frame_id) <to_string>: New member function.
	(fprint_frame_id): Delete declaration.
	* guile/scm-frame.c (frscm_print_frame_smob): Convert use of
	fprint_frame_id to use frame_id::to_string.
	* python/py-frame.c (frame_object_to_frame_info): Likewise.
	* python/py-unwind.c (unwind_infopy_str): Likewise.
	(pyuw_this_id): Likewise.
---
 gdb/ChangeLog          | 21 +++++++++++
 gdb/dummy-frame.c      |  3 +-
 gdb/frame.c            | 84 ++++++++++++++++++------------------------
 gdb/frame.h            |  8 ++--
 gdb/guile/scm-frame.c  | 11 ++----
 gdb/python/py-frame.c  |  6 +--
 gdb/python/py-unwind.c | 10 ++---
 7 files changed, 68 insertions(+), 75 deletions(-)

diff --git a/gdb/dummy-frame.c b/gdb/dummy-frame.c
index 6bbcbba48bf..155dec377f3 100644
--- a/gdb/dummy-frame.c
+++ b/gdb/dummy-frame.c
@@ -408,8 +408,7 @@ fprint_dummy_frames (struct ui_file *file)
     {
       gdb_print_host_address (s, file);
       fprintf_unfiltered (file, ":");
-      fprintf_unfiltered (file, " id=");
-      fprint_frame_id (file, s->id.id);
+      fprintf_unfiltered (file, " id=%s", s->id.id.to_string ().c_str ());
       fprintf_unfiltered (file, ", ptid=%s",
 			  target_pid_to_str (s->id.thread->ptid).c_str ());
       fprintf_unfiltered (file, "\n");
diff --git a/gdb/frame.c b/gdb/frame.c
index 2032abb2738..cd10f3f0770 100644
--- a/gdb/frame.c
+++ b/gdb/frame.c
@@ -373,43 +373,44 @@ show_backtrace_limit (struct ui_file *file, int from_tty,
 		    value);
 }
 
+/* See frame.h.  */
 
-static void
-fprint_field (struct ui_file *file, const char *name, int p, CORE_ADDR addr)
+std::string
+frame_id::to_string () const
 {
-  if (p)
-    fprintf_unfiltered (file, "%s=%s", name, hex_string (addr));
-  else
-    fprintf_unfiltered (file, "!%s", name);
-}
+  const struct frame_id &id = *this;
 
-void
-fprint_frame_id (struct ui_file *file, struct frame_id id)
-{
-  fprintf_unfiltered (file, "{");
+  std::string res = "{";
 
   if (id.stack_status == FID_STACK_INVALID)
-    fprintf_unfiltered (file, "!stack");
+    res += "!stack";
   else if (id.stack_status == FID_STACK_UNAVAILABLE)
-    fprintf_unfiltered (file, "stack=<unavailable>");
+    res += "stack=<unavailable>";
   else if (id.stack_status == FID_STACK_SENTINEL)
-    fprintf_unfiltered (file, "stack=<sentinel>");
+    res += "stack=<sentinel>";
   else if (id.stack_status == FID_STACK_OUTER)
-    fprintf_unfiltered (file, "stack=<outer>");
+    res += "stack=<outer>";
   else
-    fprintf_unfiltered (file, "stack=%s", hex_string (id.stack_addr));
-
-  fprintf_unfiltered (file, ",");
+    res += std::string ("stack=") + hex_string (id.stack_addr);
 
-  fprint_field (file, "code", id.code_addr_p, id.code_addr);
-  fprintf_unfiltered (file, ",");
+  /* Helper function to format 'N=A' if P is true, otherwise '!N'.  */
+  auto field_to_string = [] (const char *n, bool p, CORE_ADDR a) -> std::string
+  {
+    if (p)
+      return std::string (n) + "=" + core_addr_to_string (a);
+    else
+      return std::string ("!") + std::string (n);
+  };
 
-  fprint_field (file, "special", id.special_addr_p, id.special_addr);
+  res += (std::string (",")
+	  + field_to_string ("code", id.code_addr_p, id.code_addr)
+	  + std::string (",")
+	  + field_to_string ("special", id.special_addr_p, id.special_addr));
 
   if (id.artificial_depth)
-    fprintf_unfiltered (file, ",artificial=%d", id.artificial_depth);
-
-  fprintf_unfiltered (file, "}");
+    res += ",artificial=" + std::to_string (id.artificial_depth);
+  res += "}";
+  return res;
 }
 
 static void
@@ -492,7 +493,7 @@ fprint_frame (struct ui_file *file, struct frame_info *fi)
   else if (fi->this_id.p == frame_id_status::COMPUTING)
     fprintf_unfiltered (file, "<computing>");
   else
-    fprint_frame_id (file, fi->this_id.value);
+    fprintf_unfiltered (file, "%s", fi->this_id.value.to_string ().c_str ());
   fprintf_unfiltered (file, ",");
 
   fprintf_unfiltered (file, "func=");
@@ -592,11 +593,8 @@ compute_frame_id (struct frame_info *fi)
       fi->this_id.p = frame_id_status::COMPUTED;
 
       if (frame_debug)
-	{
-	  fprintf_unfiltered (gdb_stdlog, "-> ");
-	  fprint_frame_id (gdb_stdlog, fi->this_id.value);
-	  fprintf_unfiltered (gdb_stdlog, " }\n");
-	}
+	fprintf_unfiltered (gdb_stdlog, "-> %s }\n",
+			    fi->this_id.value.to_string ().c_str ());
     }
   catch (const gdb_exception &ex)
     {
@@ -748,11 +746,8 @@ frame_id_p (frame_id l)
   bool p = l.stack_status != FID_STACK_INVALID;
 
   if (frame_debug)
-    {
-      fprintf_unfiltered (gdb_stdlog, "{ frame_id_p (l=");
-      fprint_frame_id (gdb_stdlog, l);
-      fprintf_unfiltered (gdb_stdlog, ") -> %d }\n", p);
-    }
+    fprintf_unfiltered (gdb_stdlog, "{ frame_id_p (l=%s) -> %d }\n",
+			l.to_string ().c_str (), p);
 
   return p;
 }
@@ -796,13 +791,8 @@ frame_id_eq (frame_id l, frame_id r)
     eq = true;
 
   if (frame_debug)
-    {
-      fprintf_unfiltered (gdb_stdlog, "{ frame_id_eq (l=");
-      fprint_frame_id (gdb_stdlog, l);
-      fprintf_unfiltered (gdb_stdlog, ",r=");
-      fprint_frame_id (gdb_stdlog, r);
-      fprintf_unfiltered (gdb_stdlog, ") -> %d }\n", eq);
-    }
+    fprintf_unfiltered (gdb_stdlog, "{ frame_id_eq (l=%s,r=%s) -> %d }\n",
+			l.to_string ().c_str (), r.to_string ().c_str (), eq);
 
   return eq;
 }
@@ -879,13 +869,9 @@ frame_id_inner (struct gdbarch *gdbarch, struct frame_id l, struct frame_id r)
     inner = gdbarch_inner_than (gdbarch, l.stack_addr, r.stack_addr);
 
   if (frame_debug)
-    {
-      fprintf_unfiltered (gdb_stdlog, "{ frame_id_inner (l=");
-      fprint_frame_id (gdb_stdlog, l);
-      fprintf_unfiltered (gdb_stdlog, ",r=");
-      fprint_frame_id (gdb_stdlog, r);
-      fprintf_unfiltered (gdb_stdlog, ") -> %d }\n", inner);
-    }
+    fprintf_unfiltered (gdb_stdlog, "{ frame_id_inner (l=%s,r=%s) -> %d }\n",
+			l.to_string ().c_str (), r.to_string ().c_str (),
+			inner);
 
   return inner;
 }
diff --git a/gdb/frame.h b/gdb/frame.h
index 597a45967ed..da52522ad2a 100644
--- a/gdb/frame.h
+++ b/gdb/frame.h
@@ -169,6 +169,9 @@ struct frame_id
      Caller of inlined function will have it zero, each more inner called frame
      will have it increasingly one, two etc.  Similarly for TAILCALL_FRAME.  */
   int artificial_depth;
+
+  /* Return a string representation of this frame id.  */
+  std::string to_string () const;
 };
 
 /* Save and restore the currently selected frame.  */
@@ -258,11 +261,6 @@ extern bool frame_id_artificial_p (frame_id l);
 /* Returns true when L and R identify the same frame.  */
 extern bool frame_id_eq (frame_id l, frame_id r);
 
-/* Write the internal representation of a frame ID on the specified
-   stream.  */
-extern void fprint_frame_id (struct ui_file *file, struct frame_id id);
-
-
 /* Frame types.  Some are real, some are signal trampolines, and some
    are completely artificial (dummy).  */
 
diff --git a/gdb/guile/scm-frame.c b/gdb/guile/scm-frame.c
index 9d5dfa698bc..eb32f9a2ef0 100644
--- a/gdb/guile/scm-frame.c
+++ b/gdb/guile/scm-frame.c
@@ -156,14 +156,9 @@ frscm_print_frame_smob (SCM self, SCM port, scm_print_state *pstate)
 {
   frame_smob *f_smob = (frame_smob *) SCM_SMOB_DATA (self);
 
-  gdbscm_printf (port, "#<%s ", frame_smob_name);
-
-  string_file strfile;
-  fprint_frame_id (&strfile, f_smob->frame_id);
-  gdbscm_printf (port, "%s", strfile.c_str ());
-
-  scm_puts (">", port);
-
+  gdbscm_printf (port, "#<%s %s>",
+		 frame_smob_name,
+		 f_smob->frame_id.to_string ().c_str ());
   scm_remember_upto_here_1 (self);
 
   /* Non-zero means success.  */
diff --git a/gdb/python/py-frame.c b/gdb/python/py-frame.c
index 8e32ba55de4..c8eab5291ea 100644
--- a/gdb/python/py-frame.c
+++ b/gdb/python/py-frame.c
@@ -79,10 +79,8 @@ frame_object_to_frame_info (PyObject *obj)
 static PyObject *
 frapy_str (PyObject *self)
 {
-  string_file strfile;
-
-  fprint_frame_id (&strfile, ((frame_object *) self)->frame_id);
-  return PyString_FromString (strfile.c_str ());
+  const frame_id &fid = ((frame_object *) self)->frame_id;
+  return PyString_FromString (fid.to_string ().c_str ());
 }
 
 /* Implementation of gdb.Frame.is_valid (self) -> Boolean.
diff --git a/gdb/python/py-unwind.c b/gdb/python/py-unwind.c
index 4b25c485b8c..c82fa3dbe97 100644
--- a/gdb/python/py-unwind.c
+++ b/gdb/python/py-unwind.c
@@ -163,8 +163,7 @@ unwind_infopy_str (PyObject *self)
   unwind_info_object *unwind_info = (unwind_info_object *) self;
   string_file stb;
 
-  stb.puts ("Frame ID: ");
-  fprint_frame_id (&stb, unwind_info->frame_id);
+  stb.printf ("Frame ID: %s", unwind_info->frame_id.to_string ().c_str ());
   {
     const char *sep = "";
     struct value_print_options opts;
@@ -433,11 +432,8 @@ pyuw_this_id (struct frame_info *this_frame, void **cache_ptr,
 {
   *this_id = ((cached_frame_info *) *cache_ptr)->frame_id;
   if (pyuw_debug >= 1)
-    {
-      fprintf_unfiltered (gdb_stdlog, "%s: frame_id: ", __FUNCTION__);
-      fprint_frame_id (gdb_stdlog, *this_id);
-      fprintf_unfiltered (gdb_stdlog, "\n");
-    }
+    fprintf_unfiltered (gdb_stdlog, "%s: frame_id: %s\n", __FUNCTION__,
+			this_id->to_string ().c_str ());
 }
 
 /* frame_unwind.prev_register.  */
-- 
2.25.4

Re: [PATCH 1/4] gdb: replace fprint_frame_id

[Simon Marchi]

On 2021-05-08 1:07 p.m., Andrew Burgess wrote:
> Replace fprint_frame_id with a member function frame_id::to_string
> that returns a std::string.  Convert all of the previous users of
> fprint_frame_id to use the new member function.  This means that
> instead of writing things like this:
> 
>   fprintf_unfiltered (file, " id=");
>   fprint_frame_id (file, s->id.id);
> 
> We can write this:
> 
>   fprintf_unfiltered (file, " id=%s", s->id.id.to_string ().c_str ());
> 
> There should be no user visible changes after this commit.

I have converted frame.c to the new style also, but I haven't got around
to submit it yet.

What I did was:


      if (frame_debug)
	{
	  string_file debug_file;
	  fprintf_unfiltered (&debug_file, "  -> ");
	  fprint_frame_id (&debug_file, fi->this_id.value);
	  frame_debug_printf ("%s", debug_file.c_str ());
	}

But I like your version better, it's much simpler to use.  We can see
with your patch how it makes the code using it simpler.

LGTM.

Simon

[PATCH 2/4] gdb/py: convert debug logging in py-unwind to use new scheme

[Andrew Burgess]

Converts the debug print out in python/py-unwind.c to use the new
debug printing scheme.  I have also modified what is printed in a few
places, for example, rather than printing frame pointers, I now print
the frame level, this matches what we do in the general 'set debug
frame' tracing, and is usually more helpful (I think).

I also added a couple of ENTER/EXIT scope printers.

gdb/ChangeLog:

	* python/py-unwind.c (pyuw_debug): Convert to bool.
	(show_pyuw_debug): New function.
	(pyuw_debug_printf): Define.
	(PYUW_SCOPED_DEBUG_ENTER_EXIT): Define.
	(pyuw_this_id): Convert to new debug print macros.
	(pyuw_prev_register): Likewise.
	(pyuw_sniffer): Likewise.
	(pyuw_dealloc_cache): Likewise.
	(_initialize_py_unwind): Update now pyuw_debug is a bool, and add
	show function when registering.
---
 gdb/ChangeLog          | 13 ++++++++++
 gdb/python/py-unwind.c | 54 +++++++++++++++++++++++++++++-------------
 2 files changed, 51 insertions(+), 16 deletions(-)

diff --git a/gdb/python/py-unwind.c b/gdb/python/py-unwind.c
index c82fa3dbe97..6dc1502acd3 100644
--- a/gdb/python/py-unwind.c
+++ b/gdb/python/py-unwind.c
@@ -28,8 +28,28 @@
 #include "regcache.h"
 #include "valprint.h"
 
-#define TRACE_PY_UNWIND(level, args...) if (pyuw_debug >= level)  \
-  { fprintf_unfiltered (gdb_stdlog, args); }
+/* Debugging of Python unwinders.  */
+
+static bool pyuw_debug;
+
+/* Implementation of "show debug py-unwind".  */
+
+static void
+show_pyuw_debug (struct ui_file *file, int from_tty,
+		 struct cmd_list_element *c, const char *value)
+{
+  fprintf_filtered (file, _("Python unwinder debugging is %s.\n"), value);
+}
+
+/* Print an "py-unwind" debug statement.  */
+
+#define pyuw_debug_printf(fmt, ...) \
+  debug_prefixed_printf_cond (pyuw_debug, "py_unwind",fmt, ##__VA_ARGS__)
+
+/* Print "py_unwind" enter/exit debug statements.  */
+
+#define PYUW_SCOPED_DEBUG_ENTER_EXIT \
+  scoped_debug_enter_exit (pyuw_debug, "py_unwind")
 
 struct pending_frame_object
 {
@@ -96,8 +116,6 @@ extern PyTypeObject pending_frame_object_type
 extern PyTypeObject unwind_info_object_type
     CPYCHECKER_TYPE_OBJECT_FOR_TYPEDEF ("unwind_info_object");
 
-static unsigned int pyuw_debug = 0;
-
 static struct gdbarch_data *pyuw_gdbarch_data;
 
 /* Convert gdb.Value instance to inferior's pointer.  Return 1 on success,
@@ -431,9 +449,7 @@ pyuw_this_id (struct frame_info *this_frame, void **cache_ptr,
 	      struct frame_id *this_id)
 {
   *this_id = ((cached_frame_info *) *cache_ptr)->frame_id;
-  if (pyuw_debug >= 1)
-    fprintf_unfiltered (gdb_stdlog, "%s: frame_id: %s\n", __FUNCTION__,
-			this_id->to_string ().c_str ());
+  pyuw_debug_printf ("frame_id: %s", this_id->to_string ().c_str ());
 }
 
 /* frame_unwind.prev_register.  */
@@ -442,12 +458,14 @@ static struct value *
 pyuw_prev_register (struct frame_info *this_frame, void **cache_ptr,
 		    int regnum)
 {
+  PYUW_SCOPED_DEBUG_ENTER_EXIT;
+
   cached_frame_info *cached_frame = (cached_frame_info *) *cache_ptr;
   cached_reg_t *reg_info = cached_frame->reg;
   cached_reg_t *reg_info_end = reg_info + cached_frame->reg_count;
 
-  TRACE_PY_UNWIND (1, "%s (frame=%p,...,reg=%d)\n", __FUNCTION__, this_frame,
-		   regnum);
+  pyuw_debug_printf ("frame=%d,reg=%d",
+		     frame_relative_level (this_frame), regnum);
   for (; reg_info < reg_info_end; ++reg_info)
     {
       if (regnum == reg_info->num)
@@ -463,14 +481,17 @@ static int
 pyuw_sniffer (const struct frame_unwind *self, struct frame_info *this_frame,
 	      void **cache_ptr)
 {
+  PYUW_SCOPED_DEBUG_ENTER_EXIT;
+
   struct gdbarch *gdbarch = (struct gdbarch *) (self->unwind_data);
   cached_frame_info *cached_frame;
 
   gdbpy_enter enter_py (gdbarch, current_language);
 
-  TRACE_PY_UNWIND (3, "%s (SP=%s, PC=%s)\n", __FUNCTION__,
-		   paddress (gdbarch, get_frame_sp (this_frame)),
-		   paddress (gdbarch, get_frame_pc (this_frame)));
+  pyuw_debug_printf ("frame=%d, sp=%s, pc=%s",
+		     frame_relative_level (this_frame),
+		     paddress (gdbarch, get_frame_sp (this_frame)),
+		     paddress (gdbarch, get_frame_pc (this_frame)));
 
   /* Create PendingFrame instance to pass to sniffers.  */
   pending_frame_object *pfo = PyObject_New (pending_frame_object,
@@ -554,6 +575,7 @@ pyuw_sniffer (const struct frame_unwind *self, struct frame_info *this_frame,
   }
 
   *cache_ptr = cached_frame;
+  pyuw_debug_printf ("frame claimed");
   return 1;
 }
 
@@ -562,7 +584,7 @@ pyuw_sniffer (const struct frame_unwind *self, struct frame_info *this_frame,
 static void
 pyuw_dealloc_cache (struct frame_info *this_frame, void *cache)
 {
-  TRACE_PY_UNWIND (3, "%s: enter", __FUNCTION__);
+  PYUW_SCOPED_DEBUG_ENTER_EXIT;
   cached_frame_info *cached_frame = (cached_frame_info *) cache;
 
   for (int i = 0; i < cached_frame->reg_count; i++)
@@ -614,13 +636,13 @@ void _initialize_py_unwind ();
 void
 _initialize_py_unwind ()
 {
-  add_setshow_zuinteger_cmd
+  add_setshow_boolean_cmd
       ("py-unwind", class_maintenance, &pyuw_debug,
 	_("Set Python unwinder debugging."),
 	_("Show Python unwinder debugging."),
-	_("When non-zero, Python unwinder debugging is enabled."),
-	NULL,
+	_("When on, Python unwinder debugging is enabled."),
 	NULL,
+	show_pyuw_debug,
 	&setdebuglist, &showdebuglist);
   pyuw_gdbarch_data
     = gdbarch_data_register_post_init (pyuw_gdbarch_data_init);
-- 
2.25.4

Re: [PATCH 2/4] gdb/py: convert debug logging in py-unwind to use new scheme

[Simon Marchi]

On 2021-05-08 1:07 p.m., Andrew Burgess wrote:
> Converts the debug print out in python/py-unwind.c to use the new
> debug printing scheme.  I have also modified what is printed in a few
> places, for example, rather than printing frame pointers, I now print
> the frame level, this matches what we do in the general 'set debug
> frame' tracing, and is usually more helpful (I think).
> 
> I also added a couple of ENTER/EXIT scope printers.
> 
> gdb/ChangeLog:
> 
> 	* python/py-unwind.c (pyuw_debug): Convert to bool.
> 	(show_pyuw_debug): New function.
> 	(pyuw_debug_printf): Define.
> 	(PYUW_SCOPED_DEBUG_ENTER_EXIT): Define.
> 	(pyuw_this_id): Convert to new debug print macros.
> 	(pyuw_prev_register): Likewise.
> 	(pyuw_sniffer): Likewise.
> 	(pyuw_dealloc_cache): Likewise.
> 	(_initialize_py_unwind): Update now pyuw_debug is a bool, and add
> 	show function when registering.
> ---
>  gdb/ChangeLog          | 13 ++++++++++
>  gdb/python/py-unwind.c | 54 +++++++++++++++++++++++++++++-------------
>  2 files changed, 51 insertions(+), 16 deletions(-)
> 
> diff --git a/gdb/python/py-unwind.c b/gdb/python/py-unwind.c
> index c82fa3dbe97..6dc1502acd3 100644
> --- a/gdb/python/py-unwind.c
> +++ b/gdb/python/py-unwind.c
> @@ -28,8 +28,28 @@
>  #include "regcache.h"
>  #include "valprint.h"
>  
> -#define TRACE_PY_UNWIND(level, args...) if (pyuw_debug >= level)  \
> -  { fprintf_unfiltered (gdb_stdlog, args); }
> +/* Debugging of Python unwinders.  */
> +
> +static bool pyuw_debug;
> +
> +/* Implementation of "show debug py-unwind".  */
> +
> +static void
> +show_pyuw_debug (struct ui_file *file, int from_tty,
> +		 struct cmd_list_element *c, const char *value)
> +{
> +  fprintf_filtered (file, _("Python unwinder debugging is %s.\n"), value);
> +}
> +
> +/* Print an "py-unwind" debug statement.  */
> +
> +#define pyuw_debug_printf(fmt, ...) \
> +  debug_prefixed_printf_cond (pyuw_debug, "py_unwind",fmt, ##__VA_ARGS__)

Missing space before "fmt".  Did you copy this from infrun.h?  Because
this one is missing that space too, and I keep copying it, and it makes
me mad, but I've been to lazy to fix it...  one day I will.

So far the "component" names use dashes, not underscores (they
typicallly match the "set debug" option that control them).  So I'd
suggest using "py-unwind".

> +
> +/* Print "py_unwind" enter/exit debug statements.  */
> +
> +#define PYUW_SCOPED_DEBUG_ENTER_EXIT \
> +  scoped_debug_enter_exit (pyuw_debug, "py_unwind")
>  
>  struct pending_frame_object
>  {
> @@ -96,8 +116,6 @@ extern PyTypeObject pending_frame_object_type
>  extern PyTypeObject unwind_info_object_type
>      CPYCHECKER_TYPE_OBJECT_FOR_TYPEDEF ("unwind_info_object");
>  
> -static unsigned int pyuw_debug = 0;
> -
>  static struct gdbarch_data *pyuw_gdbarch_data;
>  
>  /* Convert gdb.Value instance to inferior's pointer.  Return 1 on success,
> @@ -431,9 +449,7 @@ pyuw_this_id (struct frame_info *this_frame, void **cache_ptr,
>  	      struct frame_id *this_id)
>  {
>    *this_id = ((cached_frame_info *) *cache_ptr)->frame_id;
> -  if (pyuw_debug >= 1)
> -    fprintf_unfiltered (gdb_stdlog, "%s: frame_id: %s\n", __FUNCTION__,
> -			this_id->to_string ().c_str ());
> +  pyuw_debug_printf ("frame_id: %s", this_id->to_string ().c_str ());
>  }
>  
>  /* frame_unwind.prev_register.  */
> @@ -442,12 +458,14 @@ static struct value *
>  pyuw_prev_register (struct frame_info *this_frame, void **cache_ptr,
>  		    int regnum)
>  {
> +  PYUW_SCOPED_DEBUG_ENTER_EXIT;
> +
>    cached_frame_info *cached_frame = (cached_frame_info *) *cache_ptr;
>    cached_reg_t *reg_info = cached_frame->reg;
>    cached_reg_t *reg_info_end = reg_info + cached_frame->reg_count;
>  
> -  TRACE_PY_UNWIND (1, "%s (frame=%p,...,reg=%d)\n", __FUNCTION__, this_frame,
> -		   regnum);
> +  pyuw_debug_printf ("frame=%d,reg=%d",
> +		     frame_relative_level (this_frame), regnum);

I'd suggest using a space after the comma, it helps readability a bit.

>    for (; reg_info < reg_info_end; ++reg_info)
>      {
>        if (regnum == reg_info->num)
> @@ -463,14 +481,17 @@ static int
>  pyuw_sniffer (const struct frame_unwind *self, struct frame_info *this_frame,
>  	      void **cache_ptr)
>  {
> +  PYUW_SCOPED_DEBUG_ENTER_EXIT;
> +
>    struct gdbarch *gdbarch = (struct gdbarch *) (self->unwind_data);
>    cached_frame_info *cached_frame;
>  
>    gdbpy_enter enter_py (gdbarch, current_language);
>  
> -  TRACE_PY_UNWIND (3, "%s (SP=%s, PC=%s)\n", __FUNCTION__,
> -		   paddress (gdbarch, get_frame_sp (this_frame)),
> -		   paddress (gdbarch, get_frame_pc (this_frame)));
> +  pyuw_debug_printf ("frame=%d, sp=%s, pc=%s",
> +		     frame_relative_level (this_frame),
> +		     paddress (gdbarch, get_frame_sp (this_frame)),
> +		     paddress (gdbarch, get_frame_pc (this_frame)));
>  
>    /* Create PendingFrame instance to pass to sniffers.  */
>    pending_frame_object *pfo = PyObject_New (pending_frame_object,
> @@ -554,6 +575,7 @@ pyuw_sniffer (const struct frame_unwind *self, struct frame_info *this_frame,
>    }
>  
>    *cache_ptr = cached_frame;
> +  pyuw_debug_printf ("frame claimed");

I don't really know this area very well, so my request may not make
sense, but is it possible that have multiple Python unwinders
registered?  If so, is there a name (maybe the Python class name) we
could print here, to say which unwinder claimed the frame?  I'm thinking
it could help in case the unwinder that claims the frame is not the one
you expect.

But the patch LGTM without that, it's just an idea.

Simon

Re: [PATCH 2/4] gdb/py: convert debug logging in py-unwind to use new scheme

[Andrew Burgess]

* Simon Marchi <simon.marchi@polymtl.ca> [2021-05-08 20:32:13 -0400]:

> On 2021-05-08 1:07 p.m., Andrew Burgess wrote:
> > Converts the debug print out in python/py-unwind.c to use the new
> > debug printing scheme.  I have also modified what is printed in a few
> > places, for example, rather than printing frame pointers, I now print
> > the frame level, this matches what we do in the general 'set debug
> > frame' tracing, and is usually more helpful (I think).
> > 
> > I also added a couple of ENTER/EXIT scope printers.
> > 
> > gdb/ChangeLog:
> > 
> > 	* python/py-unwind.c (pyuw_debug): Convert to bool.
> > 	(show_pyuw_debug): New function.
> > 	(pyuw_debug_printf): Define.
> > 	(PYUW_SCOPED_DEBUG_ENTER_EXIT): Define.
> > 	(pyuw_this_id): Convert to new debug print macros.
> > 	(pyuw_prev_register): Likewise.
> > 	(pyuw_sniffer): Likewise.
> > 	(pyuw_dealloc_cache): Likewise.
> > 	(_initialize_py_unwind): Update now pyuw_debug is a bool, and add
> > 	show function when registering.
> > ---
> >  gdb/ChangeLog          | 13 ++++++++++
> >  gdb/python/py-unwind.c | 54 +++++++++++++++++++++++++++++-------------
> >  2 files changed, 51 insertions(+), 16 deletions(-)
> > 
> > diff --git a/gdb/python/py-unwind.c b/gdb/python/py-unwind.c
> > index c82fa3dbe97..6dc1502acd3 100644
> > --- a/gdb/python/py-unwind.c
> > +++ b/gdb/python/py-unwind.c
> > @@ -28,8 +28,28 @@
> >  #include "regcache.h"
> >  #include "valprint.h"
> >  
> > -#define TRACE_PY_UNWIND(level, args...) if (pyuw_debug >= level)  \
> > -  { fprintf_unfiltered (gdb_stdlog, args); }
> > +/* Debugging of Python unwinders.  */
> > +
> > +static bool pyuw_debug;
> > +
> > +/* Implementation of "show debug py-unwind".  */
> > +
> > +static void
> > +show_pyuw_debug (struct ui_file *file, int from_tty,
> > +		 struct cmd_list_element *c, const char *value)
> > +{
> > +  fprintf_filtered (file, _("Python unwinder debugging is %s.\n"), value);
> > +}
> > +
> > +/* Print an "py-unwind" debug statement.  */
> > +
> > +#define pyuw_debug_printf(fmt, ...) \
> > +  debug_prefixed_printf_cond (pyuw_debug, "py_unwind",fmt, ##__VA_ARGS__)
> 
> Missing space before "fmt".  Did you copy this from infrun.h?  Because
> this one is missing that space too, and I keep copying it, and it makes
> me mad, but I've been to lazy to fix it...  one day I will.
> 
> So far the "component" names use dashes, not underscores (they
> typicallly match the "set debug" option that control them).  So I'd
> suggest using "py-unwind".
> 
> > +
> > +/* Print "py_unwind" enter/exit debug statements.  */
> > +
> > +#define PYUW_SCOPED_DEBUG_ENTER_EXIT \
> > +  scoped_debug_enter_exit (pyuw_debug, "py_unwind")
> >  
> >  struct pending_frame_object
> >  {
> > @@ -96,8 +116,6 @@ extern PyTypeObject pending_frame_object_type
> >  extern PyTypeObject unwind_info_object_type
> >      CPYCHECKER_TYPE_OBJECT_FOR_TYPEDEF ("unwind_info_object");
> >  
> > -static unsigned int pyuw_debug = 0;
> > -
> >  static struct gdbarch_data *pyuw_gdbarch_data;
> >  
> >  /* Convert gdb.Value instance to inferior's pointer.  Return 1 on success,
> > @@ -431,9 +449,7 @@ pyuw_this_id (struct frame_info *this_frame, void **cache_ptr,
> >  	      struct frame_id *this_id)
> >  {
> >    *this_id = ((cached_frame_info *) *cache_ptr)->frame_id;
> > -  if (pyuw_debug >= 1)
> > -    fprintf_unfiltered (gdb_stdlog, "%s: frame_id: %s\n", __FUNCTION__,
> > -			this_id->to_string ().c_str ());
> > +  pyuw_debug_printf ("frame_id: %s", this_id->to_string ().c_str ());
> >  }
> >  
> >  /* frame_unwind.prev_register.  */
> > @@ -442,12 +458,14 @@ static struct value *
> >  pyuw_prev_register (struct frame_info *this_frame, void **cache_ptr,
> >  		    int regnum)
> >  {
> > +  PYUW_SCOPED_DEBUG_ENTER_EXIT;
> > +
> >    cached_frame_info *cached_frame = (cached_frame_info *) *cache_ptr;
> >    cached_reg_t *reg_info = cached_frame->reg;
> >    cached_reg_t *reg_info_end = reg_info + cached_frame->reg_count;
> >  
> > -  TRACE_PY_UNWIND (1, "%s (frame=%p,...,reg=%d)\n", __FUNCTION__, this_frame,
> > -		   regnum);
> > +  pyuw_debug_printf ("frame=%d,reg=%d",
> > +		     frame_relative_level (this_frame), regnum);
> 
> I'd suggest using a space after the comma, it helps readability a bit.
> 
> >    for (; reg_info < reg_info_end; ++reg_info)
> >      {
> >        if (regnum == reg_info->num)
> > @@ -463,14 +481,17 @@ static int
> >  pyuw_sniffer (const struct frame_unwind *self, struct frame_info *this_frame,
> >  	      void **cache_ptr)
> >  {
> > +  PYUW_SCOPED_DEBUG_ENTER_EXIT;
> > +
> >    struct gdbarch *gdbarch = (struct gdbarch *) (self->unwind_data);
> >    cached_frame_info *cached_frame;
> >  
> >    gdbpy_enter enter_py (gdbarch, current_language);
> >  
> > -  TRACE_PY_UNWIND (3, "%s (SP=%s, PC=%s)\n", __FUNCTION__,
> > -		   paddress (gdbarch, get_frame_sp (this_frame)),
> > -		   paddress (gdbarch, get_frame_pc (this_frame)));
> > +  pyuw_debug_printf ("frame=%d, sp=%s, pc=%s",
> > +		     frame_relative_level (this_frame),
> > +		     paddress (gdbarch, get_frame_sp (this_frame)),
> > +		     paddress (gdbarch, get_frame_pc (this_frame)));
> >  
> >    /* Create PendingFrame instance to pass to sniffers.  */
> >    pending_frame_object *pfo = PyObject_New (pending_frame_object,
> > @@ -554,6 +575,7 @@ pyuw_sniffer (const struct frame_unwind *self, struct frame_info *this_frame,
> >    }
> >  
> >    *cache_ptr = cached_frame;
> > +  pyuw_debug_printf ("frame claimed");
> 
> I don't really know this area very well, so my request may not make
> sense, but is it possible that have multiple Python unwinders
> registered?  If so, is there a name (maybe the Python class name) we
> could print here, to say which unwinder claimed the frame?  I'm thinking
> it could help in case the unwinder that claims the frame is not the one
> you expect.

I'm not sure what helpful things could be printed here.  The actual
code that finds the "correct" unwinder is elsewhere (see
_execute_unwinders in python/lib/gdb/__init__.py), back in this C
function all we get given is a gdb.UnwindInfo object - and in most
cases that's all it will ever be, there's nothing really identifying
it as having come from any particular unwinder.

For now I've just left this as it is, but would be more than happy if
someone has a good idea for what could be printed.

Thanks,
Andrew

Re: [PATCH 2/4] gdb/py: convert debug logging in py-unwind to use new scheme

[Simon Marchi]

On 2021-05-09 12:10 p.m., Andrew Burgess wrote:
> I'm not sure what helpful things could be printed here.  The actual
> code that finds the "correct" unwinder is elsewhere (see
> _execute_unwinders in python/lib/gdb/__init__.py), back in this C
> function all we get given is a gdb.UnwindInfo object - and in most
> cases that's all it will ever be, there's nothing really identifying
> it as having come from any particular unwinder.

We can change gdb._execute_unwinders to return the name of the unwinder.
For example:


From 16347d43242a5e96e1a839e1d9b006bb9fb85f85 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>
Date: Sun, 9 May 2021 22:13:29 -0400
Subject: [PATCH] gdb: print name of unwinder that claimed the frame

Change-Id: Id603545b44a97df2a39dd1872fe1f38ad5059f03
---
 gdb/python/lib/gdb/__init__.py        | 13 +++++++++----
 gdb/python/py-unwind.c                | 27 +++++++++++++++++++++------
 gdb/testsuite/gdb.python/py-unwind.py |  2 +-
 3 files changed, 31 insertions(+), 11 deletions(-)

diff --git a/gdb/python/lib/gdb/__init__.py b/gdb/python/lib/gdb/__init__.py
index d748b3a5827e..3640d142910b 100644
--- a/gdb/python/lib/gdb/__init__.py
+++ b/gdb/python/lib/gdb/__init__.py
@@ -91,26 +91,31 @@ 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..58e8b6b519e8 100644
--- a/gdb/python/py-unwind.c
+++ b/gdb/python/py-unwind.c
@@ -524,27 +524,41 @@ 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 == NULL)
     {
       /* 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;
 
+  if (!PyTuple_Check (pyo_execute_ret.get ()))
+    error (_("Unexpected return value type from gdb._execute_unwinders."));
+
+  if (PyTuple_GET_SIZE (pyo_execute_ret.get ()) != 2)
+    error (_("Unexpected size of the tuple returned by gdb._execute_unwinders."));
+
+  PyObject *pyo_unwind_info = PyTuple_GET_ITEM (pyo_execute_ret.get (), 0);
+
   /* Received UnwindInfo, cache data.  */
-  if (PyObject_IsInstance (pyo_unwind_info.get (),
+  if (PyObject_IsInstance (pyo_unwind_info,
 			   (PyObject *) &unwind_info_object_type) <= 0)
     error (_("A Unwinder should return gdb.UnwindInfo instance."));
 
+  PyObject *pyo_unwinder_name = PyTuple_GET_ITEM (pyo_execute_ret.get (), 1);
+
+  if (!PyUnicode_Check (pyo_unwinder_name))
+    error (_("Unexpected type for the unwinder name."));
+
   {
     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 +589,8 @@ pyuw_sniffer (const struct frame_unwind *self, struct frame_info *this_frame,
   }
 
   *cache_ptr = cached_frame;
-  pyuw_debug_printf ("frame claimed");
+  pyuw_debug_printf ("frame claimed by unwinder %s",
+		     PyUnicode_AsUTF8 (pyo_unwinder_name));
   return 1;
 }
 
diff --git a/gdb/testsuite/gdb.python/py-unwind.py b/gdb/testsuite/gdb.python/py-unwind.py
index 931e979d28eb..b71d07abf246 100644
--- a/gdb/testsuite/gdb.python/py-unwind.py
+++ b/gdb/testsuite/gdb.python/py-unwind.py
@@ -37,7 +37,7 @@ class TestUnwinder(Unwinder):
     AMD64_RIP = None
 
     def __init__(self):
-        Unwinder.__init__(self, "test unwinder")
+        Unwinder.__init__(self, "test🍌unwinder")
         self.char_ptr_t = gdb.lookup_type("unsigned char").pointer()
         self.char_ptr_ptr_t = self.char_ptr_t.pointer()
         self._last_arch = None
-- 
2.30.1

Re: [PATCH 2/4] gdb/py: convert debug logging in py-unwind to use new scheme

[Andrew Burgess]

* Simon Marchi <simon.marchi@polymtl.ca> [2021-05-09 22:29:25 -0400]:

> On 2021-05-09 12:10 p.m., Andrew Burgess wrote:
> > I'm not sure what helpful things could be printed here.  The actual
> > code that finds the "correct" unwinder is elsewhere (see
> > _execute_unwinders in python/lib/gdb/__init__.py), back in this C
> > function all we get given is a gdb.UnwindInfo object - and in most
> > cases that's all it will ever be, there's nothing really identifying
> > it as having come from any particular unwinder.
> 
> We can change gdb._execute_unwinders to return the name of the unwinder.
> For example:
> 
> 
> From 16347d43242a5e96e1a839e1d9b006bb9fb85f85 Mon Sep 17 00:00:00 2001
> From: Simon Marchi <simon.marchi@polymtl.ca>
> Date: Sun, 9 May 2021 22:13:29 -0400
> Subject: [PATCH] gdb: print name of unwinder that claimed the frame
> 
> Change-Id: Id603545b44a97df2a39dd1872fe1f38ad5059f03
> ---
>  gdb/python/lib/gdb/__init__.py        | 13 +++++++++----
>  gdb/python/py-unwind.c                | 27 +++++++++++++++++++++------
>  gdb/testsuite/gdb.python/py-unwind.py |  2 +-
>  3 files changed, 31 insertions(+), 11 deletions(-)
> 
> diff --git a/gdb/python/lib/gdb/__init__.py b/gdb/python/lib/gdb/__init__.py
> index d748b3a5827e..3640d142910b 100644
> --- a/gdb/python/lib/gdb/__init__.py
> +++ b/gdb/python/lib/gdb/__init__.py
> @@ -91,26 +91,31 @@ 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..58e8b6b519e8 100644
> --- a/gdb/python/py-unwind.c
> +++ b/gdb/python/py-unwind.c
> @@ -524,27 +524,41 @@ 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 == NULL)
>      {
>        /* 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;
>  
> +  if (!PyTuple_Check (pyo_execute_ret.get ()))
> +    error (_("Unexpected return value type from gdb._execute_unwinders."));
> +
> +  if (PyTuple_GET_SIZE (pyo_execute_ret.get ()) != 2)
> +    error (_("Unexpected size of the tuple returned by gdb._execute_unwinders."));
> +
> +  PyObject *pyo_unwind_info = PyTuple_GET_ITEM (pyo_execute_ret.get (), 0);
> +
>    /* Received UnwindInfo, cache data.  */
> -  if (PyObject_IsInstance (pyo_unwind_info.get (),
> +  if (PyObject_IsInstance (pyo_unwind_info,
>  			   (PyObject *) &unwind_info_object_type) <= 0)
>      error (_("A Unwinder should return gdb.UnwindInfo instance."));
>  
> +  PyObject *pyo_unwinder_name = PyTuple_GET_ITEM (pyo_execute_ret.get (), 1);
> +
> +  if (!PyUnicode_Check (pyo_unwinder_name))
> +    error (_("Unexpected type for the unwinder name."));
> +
>    {
>      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 +589,8 @@ pyuw_sniffer (const struct frame_unwind *self, struct frame_info *this_frame,
>    }
>  
>    *cache_ptr = cached_frame;
> -  pyuw_debug_printf ("frame claimed");
> +  pyuw_debug_printf ("frame claimed by unwinder %s",
> +		     PyUnicode_AsUTF8 (pyo_unwinder_name));
>    return 1;
>  }
>  
> diff --git a/gdb/testsuite/gdb.python/py-unwind.py b/gdb/testsuite/gdb.python/py-unwind.py
> index 931e979d28eb..b71d07abf246 100644
> --- a/gdb/testsuite/gdb.python/py-unwind.py
> +++ b/gdb/testsuite/gdb.python/py-unwind.py
> @@ -37,7 +37,7 @@ class TestUnwinder(Unwinder):
>      AMD64_RIP = None
>  
>      def __init__(self):
> -        Unwinder.__init__(self, "test unwinder")
> +        Unwinder.__init__(self, "test🍌unwinder")

Am I going crazy, or is that a banana? I guess you were testing
PyUnicode_AsUTF8... might be worth a comment though!

Otherwise, looks like a good solution.  Are you happy to push this, or
would you like me to do it?

Thanks,
Andrew


>          self.char_ptr_t = gdb.lookup_type("unsigned char").pointer()
>          self.char_ptr_ptr_t = self.char_ptr_t.pointer()
>          self._last_arch = None
> -- 
> 2.30.1
> 

Re: [PATCH 2/4] gdb/py: convert debug logging in py-unwind to use new scheme

[Simon Marchi]

On 2021-05-10 4:20 a.m., Andrew Burgess wrote:
>> diff --git a/gdb/testsuite/gdb.python/py-unwind.py b/gdb/testsuite/gdb.python/py-unwind.py
>> index 931e979d28eb..b71d07abf246 100644
>> --- a/gdb/testsuite/gdb.python/py-unwind.py
>> +++ b/gdb/testsuite/gdb.python/py-unwind.py
>> @@ -37,7 +37,7 @@ class TestUnwinder(Unwinder):
>>      AMD64_RIP = None
>>  
>>      def __init__(self):
>> -        Unwinder.__init__(self, "test unwinder")
>> +        Unwinder.__init__(self, "test🍌unwinder")
> 
> Am I going crazy, or is that a banana? I guess you were testing
> PyUnicode_AsUTF8... might be worth a comment though!

Haha, woops!  Yes, it was to test non-trivial UTF-8 strings.  But no, it
shouldn't be left there.

> 
> Otherwise, looks like a good solution.  Are you happy to push this, or
> would you like me to do it?

I can do it.

Simon

[PATCH 3/4] gdb/py: add some debugging to py-breakpoint.c

[Andrew Burgess]

Adds some new debugging to python/py-breakpoint.c.

gdb/ChangeLog:

	* python/py-breakpoint.c (pybp_debug): New static global.
	(show_pybp_debug): New function.
	(pybp_debug_printf): Define.
	(PYBP_SCOPED_DEBUG_ENTER_EXIT): Define.
	(gdbpy_breakpoint_created): Add some debugging.
	(gdbpy_breakpoint_deleted): Likewise.
	(gdbpy_breakpoint_modified): Likewise.
	(_initialize_py_breakpoint): New function.

gdb/doc/ChangeLog:

	* python.texinfo (Python Commands): Document 'set debug
	py-breakpoint' and 'show debug py-breakpoint'.
---
 gdb/ChangeLog              | 11 +++++++
 gdb/doc/ChangeLog          |  5 ++++
 gdb/doc/python.texi        | 11 +++++++
 gdb/python/py-breakpoint.c | 59 ++++++++++++++++++++++++++++++++++++--
 4 files changed, 83 insertions(+), 3 deletions(-)

diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index 482d328de44..6d45e98a8af 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -153,6 +153,17 @@
 the @code{script-extension} setting.  @xref{Extending GDB, ,Extending GDB}.
 @end table
 
+The following commands are intended to help debug @value{GDBN} itself:
+
+@table @code
+@kindex set debug py-breakpoint
+@kindex show debug py-breakpoint
+@item set debug py-breakpoint on@r{|}off
+@itemx show debug py-breakpoint
+When @samp{on} @value{GDBN} prints debug messages related to the
+Python breakpoint API.  This is @samp{off} by default.
+@end table
+
 @node Python API
 @subsection Python API
 @cindex python api
diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
index b4140713613..8bae0f830f9 100644
--- a/gdb/python/py-breakpoint.c
+++ b/gdb/python/py-breakpoint.c
@@ -34,6 +34,29 @@
 #include "py-event.h"
 #include "linespec.h"
 
+/* Debugging of Python breakpoints.  */
+
+static bool pybp_debug;
+
+/* Implementation of "show debug py-unwind".  */
+
+static void
+show_pybp_debug (struct ui_file *file, int from_tty,
+		 struct cmd_list_element *c, const char *value)
+{
+  fprintf_filtered (file, _("Python breakpoint debugging is %s.\n"), value);
+}
+
+/* Print an "py-breakpoint" debug statement.  */
+
+#define pybp_debug_printf(fmt, ...) \
+  debug_prefixed_printf_cond (pybp_debug, "py_breakpoint",fmt, ##__VA_ARGS__)
+
+/* Print "py-breakpoint" enter/exit debug statements.  */
+
+#define PYBP_SCOPED_DEBUG_ENTER_EXIT \
+  scoped_debug_enter_exit (pybp_debug, "py_breakpoint")
+
 /* Number of live breakpoints.  */
 static int bppy_live;
 
@@ -1005,10 +1028,15 @@ gdbpy_breakpoint_has_cond (const struct extension_language_defn *extlang,
 static void
 gdbpy_breakpoint_created (struct breakpoint *bp)
 {
+  PYBP_SCOPED_DEBUG_ENTER_EXIT;
+
   gdbpy_breakpoint_object *newbp;
 
   if (!user_breakpoint_p (bp) && bppy_pending_object == NULL)
-    return;
+    {
+      pybp_debug_printf ("not attaching python object to this breakpoint");
+      return;
+    }
 
   if (bp->type != bp_breakpoint
       && bp->type != bp_hardware_breakpoint
@@ -1016,7 +1044,10 @@ gdbpy_breakpoint_created (struct breakpoint *bp)
       && bp->type != bp_hardware_watchpoint
       && bp->type != bp_read_watchpoint
       && bp->type != bp_access_watchpoint)
-    return;
+    {
+      pybp_debug_printf ("is not a breakpoint or watchpoint");
+      return;
+    }
 
   struct gdbarch *garch = bp->gdbarch ? bp->gdbarch : get_current_arch ();
   gdbpy_enter enter_py (garch, current_language);
@@ -1026,9 +1057,13 @@ gdbpy_breakpoint_created (struct breakpoint *bp)
       newbp = bppy_pending_object;
       Py_INCREF (newbp);
       bppy_pending_object = NULL;
+      pybp_debug_printf ("attaching existing breakpoint object");
     }
   else
-    newbp = PyObject_New (gdbpy_breakpoint_object, &breakpoint_object_type);
+    {
+      newbp = PyObject_New (gdbpy_breakpoint_object, &breakpoint_object_type);
+      pybp_debug_printf ("attaching new breakpoint object");
+    }
   if (newbp)
     {
       newbp->number = bp->number;
@@ -1057,6 +1092,8 @@ gdbpy_breakpoint_created (struct breakpoint *bp)
 static void
 gdbpy_breakpoint_deleted (struct breakpoint *b)
 {
+  PYBP_SCOPED_DEBUG_ENTER_EXIT;
+
   int num = b->number;
   struct breakpoint *bp = NULL;
 
@@ -1087,6 +1124,8 @@ gdbpy_breakpoint_deleted (struct breakpoint *b)
 static void
 gdbpy_breakpoint_modified (struct breakpoint *b)
 {
+  PYBP_SCOPED_DEBUG_ENTER_EXIT;
+
   int num = b->number;
   struct breakpoint *bp = NULL;
 
@@ -1285,3 +1324,17 @@ PyTypeObject breakpoint_object_type =
   bppy_init,			  /* tp_init */
   0,				  /* tp_alloc */
 };
+
+void _initialize_py_breakpoint ();
+void
+_initialize_py_breakpoint ()
+{
+  add_setshow_boolean_cmd
+      ("py-breakpoint", class_maintenance, &pybp_debug,
+	_("Set Python breakpoint debugging."),
+	_("Show Python breakpoint debugging."),
+	_("When on, Python breakpoint debugging is enabled."),
+	NULL,
+	show_pybp_debug,
+	&setdebuglist, &showdebuglist);
+}
-- 
2.25.4

Re: [PATCH 3/4] gdb/py: add some debugging to py-breakpoint.c

[Eli Zaretskii]

> From: Andrew Burgess <andrew.burgess@embecosm.com>
> Date: Sat,  8 May 2021 18:07:36 +0100
> 
> Adds some new debugging to python/py-breakpoint.c.
> 
> gdb/ChangeLog:
> 
> 	* python/py-breakpoint.c (pybp_debug): New static global.
> 	(show_pybp_debug): New function.
> 	(pybp_debug_printf): Define.
> 	(PYBP_SCOPED_DEBUG_ENTER_EXIT): Define.
> 	(gdbpy_breakpoint_created): Add some debugging.
> 	(gdbpy_breakpoint_deleted): Likewise.
> 	(gdbpy_breakpoint_modified): Likewise.
> 	(_initialize_py_breakpoint): New function.
> 
> gdb/doc/ChangeLog:
> 
> 	* python.texinfo (Python Commands): Document 'set debug
> 	py-breakpoint' and 'show debug py-breakpoint'.

The documentation part is okay.

> +@table @code
> +@kindex set debug py-breakpoint
> +@kindex show debug py-breakpoint
> +@item set debug py-breakpoint on@r{|}off
> +@itemx show debug py-breakpoint
> +When @samp{on} @value{GDBN} prints debug messages related to the
                 ^
Please add a comma there.

Re: [PATCH 3/4] gdb/py: add some debugging to py-breakpoint.c

[Simon Marchi]

On 2021-05-08 1:07 p.m., Andrew Burgess wrote:
> Adds some new debugging to python/py-breakpoint.c.
> 
> gdb/ChangeLog:
> 
> 	* python/py-breakpoint.c (pybp_debug): New static global.
> 	(show_pybp_debug): New function.
> 	(pybp_debug_printf): Define.
> 	(PYBP_SCOPED_DEBUG_ENTER_EXIT): Define.
> 	(gdbpy_breakpoint_created): Add some debugging.
> 	(gdbpy_breakpoint_deleted): Likewise.
> 	(gdbpy_breakpoint_modified): Likewise.
> 	(_initialize_py_breakpoint): New function.
> 
> gdb/doc/ChangeLog:
> 
> 	* python.texinfo (Python Commands): Document 'set debug
> 	py-breakpoint' and 'show debug py-breakpoint'.
> ---
>  gdb/ChangeLog              | 11 +++++++
>  gdb/doc/ChangeLog          |  5 ++++
>  gdb/doc/python.texi        | 11 +++++++
>  gdb/python/py-breakpoint.c | 59 ++++++++++++++++++++++++++++++++++++--
>  4 files changed, 83 insertions(+), 3 deletions(-)
> 
> diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
> index 482d328de44..6d45e98a8af 100644
> --- a/gdb/doc/python.texi
> +++ b/gdb/doc/python.texi
> @@ -153,6 +153,17 @@
>  the @code{script-extension} setting.  @xref{Extending GDB, ,Extending GDB}.
>  @end table
>  
> +The following commands are intended to help debug @value{GDBN} itself:
> +
> +@table @code
> +@kindex set debug py-breakpoint
> +@kindex show debug py-breakpoint
> +@item set debug py-breakpoint on@r{|}off
> +@itemx show debug py-breakpoint
> +When @samp{on} @value{GDBN} prints debug messages related to the
> +Python breakpoint API.  This is @samp{off} by default.
> +@end table
> +
>  @node Python API
>  @subsection Python API
>  @cindex python api
> diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
> index b4140713613..8bae0f830f9 100644
> --- a/gdb/python/py-breakpoint.c
> +++ b/gdb/python/py-breakpoint.c
> @@ -34,6 +34,29 @@
>  #include "py-event.h"
>  #include "linespec.h"
>  
> +/* Debugging of Python breakpoints.  */
> +
> +static bool pybp_debug;
> +
> +/* Implementation of "show debug py-unwind".  */

py-unwind -> py-breakpoint

> +
> +static void
> +show_pybp_debug (struct ui_file *file, int from_tty,
> +		 struct cmd_list_element *c, const char *value)
> +{
> +  fprintf_filtered (file, _("Python breakpoint debugging is %s.\n"), value);
> +}
> +
> +/* Print an "py-breakpoint" debug statement.  */
> +
> +#define pybp_debug_printf(fmt, ...) \
> +  debug_prefixed_printf_cond (pybp_debug, "py_breakpoint",fmt, ##__VA_ARGS__)

Same space missing.  OK, ok, I'll push a patch to fix this once I'm done
reviewing.

> +
> +/* Print "py-breakpoint" enter/exit debug statements.  */
> +
> +#define PYBP_SCOPED_DEBUG_ENTER_EXIT \
> +  scoped_debug_enter_exit (pybp_debug, "py_breakpoint")

Same comment as the previous patch, I would suggest "py-breakpoint".

Otherwise, LGTM.

Simon

Re: [PATCH 3/4] gdb/py: add some debugging to py-breakpoint.c

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> Adds some new debugging to python/py-breakpoint.c.
Andrew> gdb/ChangeLog:

Andrew> +    {
Andrew> +      pybp_debug_printf ("not attaching python object to this breakpoint");
Andrew> +      return;
Andrew> +    }
 
Since it is just a debugging print, and since it is unfiltered, this
doesn't matter much; but it's worth noting that generally calling gdb
code from inside the Python layer can be a problem, because if an
exception is thrown the results can be bad.  (There is some code using
exceptions in Python, but that code was written to catch them before
returning to Python...)

Tom

[PATCH 4/4] gdb/doc: document 'set debug py-unwind'

[Andrew Burgess]

When the 'set debug py-unwind' flag was added, it was never documented
in the manual.  This commit adds some text for this command to the
manual.

gdb/doc/ChangeLog:

	* python.texinfo (Python Commands): Document 'set debug
	py-unwind' and 'show debug py-unwind'.
---
 gdb/doc/ChangeLog   | 5 +++++
 gdb/doc/python.texi | 7 +++++++
 2 files changed, 12 insertions(+)

diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index 6d45e98a8af..6e380e3e694 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -162,6 +162,13 @@
 @itemx show debug py-breakpoint
 When @samp{on} @value{GDBN} prints debug messages related to the
 Python breakpoint API.  This is @samp{off} by default.
+
+@kindex set debug py-unwind
+@kindex show debug py-unwind
+@item set debug py-unwind on@r{|}off
+@itemx show debug py-unwind
+When @samp{on} @value{GDBN} prints debug messages related to the
+Python unwinder API.  This is @samp{off} by default.
 @end table
 
 @node Python API
-- 
2.25.4

Re: [PATCH 4/4] gdb/doc: document 'set debug py-unwind'

[Eli Zaretskii]

> From: Andrew Burgess <andrew.burgess@embecosm.com>
> Date: Sat,  8 May 2021 18:07:37 +0100
> 
> When the 'set debug py-unwind' flag was added, it was never documented
> in the manual.  This commit adds some text for this command to the
> manual.
> 
> gdb/doc/ChangeLog:
> 
> 	* python.texinfo (Python Commands): Document 'set debug
> 	py-unwind' and 'show debug py-unwind'.

This is okay, with the missing comma added:

> +@itemx show debug py-unwind
> +When @samp{on} @value{GDBN} prints debug messages related to the
                 ^
Here.

Thanks.