[PATCH 0/3] Add "announce" flag to Python breakpoint creation

[PATCH 0/3] Add "announce" flag to Python breakpoint creation

[Tom Tromey]

I noticed that, when creating a breakpoint from Python, gdb will print
various things.  However, for DAP and presumably other scripting
applications, it's good to be able to control gdb's output.  This
series adds a flag to allow this.

Tom

[PATCH 1/3] Refactor body of bppy_init

[Tom Tromey]

This patch pulls the body of bppy_init out into a separate function.
This will simplify a subsequent patch.
---
 gdb/python/py-breakpoint.c | 161 ++++++++++++++++++++-----------------
 1 file changed, 87 insertions(+), 74 deletions(-)

diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
index 63b18bd0f92..917fd367d06 100644
--- a/gdb/python/py-breakpoint.c
+++ b/gdb/python/py-breakpoint.c
@@ -813,6 +813,90 @@ bppy_init_validate_args (const char *spec, char *source,
   return 1;
 }
 
+/* Helper function for breakpoint creation.  */
+
+static void
+bppy_create_breakpoint (enum bptype type, int access_type, int temporary_bp,
+			int internal_bp, const char *spec,
+			PyObject *qualified, const char *source,
+			const char *function, const char *label,
+			const char *line)
+{
+  switch (type)
+    {
+    case bp_breakpoint:
+    case bp_hardware_breakpoint:
+      {
+	location_spec_up locspec;
+	symbol_name_match_type func_name_match_type
+	  = (qualified != NULL && PyObject_IsTrue (qualified)
+	     ? symbol_name_match_type::FULL
+	     : symbol_name_match_type::WILD);
+
+	if (spec != NULL)
+	  {
+	    gdb::unique_xmalloc_ptr<char>
+	      copy_holder (xstrdup (skip_spaces (spec)));
+	    const char *copy = copy_holder.get ();
+
+	    locspec  = string_to_location_spec (&copy,
+						current_language,
+						func_name_match_type);
+	  }
+	else
+	  {
+	    std::unique_ptr<explicit_location_spec> explicit_loc
+	      (new explicit_location_spec ());
+
+	    explicit_loc->source_filename
+	      = source != nullptr ? xstrdup (source) : nullptr;
+	    explicit_loc->function_name
+	      = function != nullptr ? xstrdup (function) : nullptr;
+	    explicit_loc->label_name
+	      = label != nullptr ? xstrdup (label) : nullptr;
+
+	    if (line != NULL)
+	      explicit_loc->line_offset = linespec_parse_line_offset (line);
+
+	    explicit_loc->func_name_match_type = func_name_match_type;
+
+	    locspec.reset (explicit_loc.release ());
+	  }
+
+	const struct breakpoint_ops *ops
+	  = breakpoint_ops_for_location_spec (locspec.get (), false);
+
+	create_breakpoint (gdbpy_enter::get_gdbarch (),
+			   locspec.get (), NULL, -1, NULL, false,
+			   0,
+			   temporary_bp, type,
+			   0,
+			   AUTO_BOOLEAN_TRUE,
+			   ops,
+			   0, 1, internal_bp, 0);
+	break;
+      }
+    case bp_watchpoint:
+      {
+	spec = skip_spaces (spec);
+
+	if (access_type == hw_write)
+	  watch_command_wrapper (spec, 0, internal_bp);
+	else if (access_type == hw_access)
+	  awatch_command_wrapper (spec, 0, internal_bp);
+	else if (access_type == hw_read)
+	  rwatch_command_wrapper (spec, 0, internal_bp);
+	else
+	  error(_("Cannot understand watchpoint access type."));
+	break;
+      }
+    case bp_catchpoint:
+      error (_("BP_CATCHPOINT not supported"));
+    default:
+      error(_("Do not understand breakpoint type to set."));
+    }
+}
+
 /* Python function to create a new breakpoint.  */
 static int
 bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
@@ -881,80 +965,9 @@ bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
 
   try
     {
-      switch (type)
-	{
-	case bp_breakpoint:
-	case bp_hardware_breakpoint:
-	  {
-	    location_spec_up locspec;
-	    symbol_name_match_type func_name_match_type
-	      = (qualified != NULL && PyObject_IsTrue (qualified)
-		  ? symbol_name_match_type::FULL
-		  : symbol_name_match_type::WILD);
-
-	    if (spec != NULL)
-	      {
-		gdb::unique_xmalloc_ptr<char>
-		  copy_holder (xstrdup (skip_spaces (spec)));
-		const char *copy = copy_holder.get ();
-
-		locspec  = string_to_location_spec (&copy,
-						    current_language,
-						    func_name_match_type);
-	      }
-	    else
-	      {
-		std::unique_ptr<explicit_location_spec> explicit_loc
-		  (new explicit_location_spec ());
-
-		explicit_loc->source_filename
-		  = source != nullptr ? xstrdup (source) : nullptr;
-		explicit_loc->function_name
-		  = function != nullptr ? xstrdup (function) : nullptr;
-		explicit_loc->label_name
-		  = label != nullptr ? xstrdup (label) : nullptr;
-
-		if (line != NULL)
-		  explicit_loc->line_offset
-		    = linespec_parse_line_offset (line.get ());
-
-		explicit_loc->func_name_match_type = func_name_match_type;
-
-		locspec.reset (explicit_loc.release ());
-	      }
-
-	    const struct breakpoint_ops *ops
-	      = breakpoint_ops_for_location_spec (locspec.get (), false);
-
-	    create_breakpoint (gdbpy_enter::get_gdbarch (),
-			       locspec.get (), NULL, -1, NULL, false,
-			       0,
-			       temporary_bp, type,
-			       0,
-			       AUTO_BOOLEAN_TRUE,
-			       ops,
-			       0, 1, internal_bp, 0);
-	    break;
-	  }
-	case bp_watchpoint:
-	  {
-	    spec = skip_spaces (spec);
-
-	    if (access_type == hw_write)
-	      watch_command_wrapper (spec, 0, internal_bp);
-	    else if (access_type == hw_access)
-	      awatch_command_wrapper (spec, 0, internal_bp);
-	    else if (access_type == hw_read)
-	      rwatch_command_wrapper (spec, 0, internal_bp);
-	    else
-	      error(_("Cannot understand watchpoint access type."));
-	    break;
-	  }
-	case bp_catchpoint:
-	  error (_("BP_CATCHPOINT not supported"));
-	default:
-	  error(_("Do not understand breakpoint type to set."));
-	}
+      bppy_create_breakpoint (type, access_type, temporary_bp, internal_bp,
+			      spec, qualified, source, function, label,
+			      line.get ());
     }
   catch (const gdb_exception &except)
     {
-- 
2.34.3

[PATCH 2/3] Fix latent bug in Python breakpoint creation

[Tom Tromey]

While working on the previous patch, I noticed that Python breakpoint
creation does:

-	  = (qualified != NULL && PyObject_IsTrue (qualified)

PyObject_IsTrue can fail, so this is missing an error check.  This
patch adds the missing check.

Note that this could probably be improved by using the "p" format in
the call to gdb_PyArg_ParseTupleAndKeywords, but that was added in
Python 3.3, and I think gdb still supports 3.2.
---
 gdb/python/py-breakpoint.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
index 917fd367d06..39d9bd5dff6 100644
--- a/gdb/python/py-breakpoint.c
+++ b/gdb/python/py-breakpoint.c
@@ -818,7 +818,7 @@ bppy_init_validate_args (const char *spec, char *source,
 static void
 bppy_create_breakpoint (enum bptype type, int access_type, int temporary_bp,
 			int internal_bp, const char *spec,
-			PyObject *qualified, const char *source,
+			int qualified, const char *source,
 			const char *function, const char *label,
 			const char *line)
 {
@@ -829,7 +829,7 @@ bppy_create_breakpoint (enum bptype type, int access_type, int temporary_bp,
       {
 	location_spec_up locspec;
 	symbol_name_match_type func_name_match_type
-	  = (qualified != NULL && PyObject_IsTrue (qualified)
+	  = (qualified
 	     ? symbol_name_match_type::FULL
 	     : symbol_name_match_type::WILD);
 
@@ -916,14 +916,15 @@ bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
   char *label = NULL;
   char *source = NULL;
   char *function = NULL;
-  PyObject * qualified = NULL;
+  PyObject *qualified_obj = nullptr;
+  int qualified = 0;
 
   if (!gdb_PyArg_ParseTupleAndKeywords (args, kwargs, "|siiOOsssOO", keywords,
 					&spec, &type, &access_type,
 					&internal,
 					&temporary, &source,
 					&function, &label, &lineobj,
-					&qualified))
+					&qualified_obj))
     return -1;
 
 
@@ -955,6 +956,13 @@ bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
 	return -1;
     }
 
+  if (qualified_obj != nullptr)
+    {
+      qualified = PyObject_IsTrue (qualified_obj);
+      if (qualified == -1)
+	return -1;
+    }
+
   if (bppy_init_validate_args (spec, source, function, label, line.get (),
 			       type) == -1)
     return -1;
-- 
2.34.3

Re: [PATCH 2/3] Fix latent bug in Python breakpoint creation

[Simon Marchi]

On 12/8/22 14:18, Tom Tromey via Gdb-patches wrote:
> While working on the previous patch, I noticed that Python breakpoint
> creation does:
> 
> -	  = (qualified != NULL && PyObject_IsTrue (qualified)
> 
> PyObject_IsTrue can fail, so this is missing an error check.  This
> patch adds the missing check.
> 
> Note that this could probably be improved by using the "p" format in
> the call to gdb_PyArg_ParseTupleAndKeywords, but that was added in
> Python 3.3, and I think gdb still supports 3.2.
> ---
>  gdb/python/py-breakpoint.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
> index 917fd367d06..39d9bd5dff6 100644
> --- a/gdb/python/py-breakpoint.c
> +++ b/gdb/python/py-breakpoint.c
> @@ -818,7 +818,7 @@ bppy_init_validate_args (const char *spec, char *source,
>  static void
>  bppy_create_breakpoint (enum bptype type, int access_type, int temporary_bp,
>  			int internal_bp, const char *spec,
> -			PyObject *qualified, const char *source,
> +			int qualified, const char *source,

You might as well make this parameter "bool".

Otherwise, LGTM.

Simon

[PATCH 3/3] Let Python breakpoints be created silently

[Tom Tromey]

Currently, a breakpoint created from Python will always announce its
presence; and in some cases (for example a pending breakpoint), other
information will be printed as well.

When scripting gdb, it's useful to be able to control the output in
cases like this.  I debated whether to simply disable the output
entirely, but I thought perhaps some existing code acts as a simple
"break"-like command and wants the output.

This patch adds a new "announce" flag to gdb.Breakpoint.  Setting this
to False will cause gdb to be silent here.
---
 gdb/doc/python.texi                        | 10 ++++++--
 gdb/python/py-breakpoint.c                 | 30 +++++++++++++++++-----
 gdb/testsuite/gdb.python/py-breakpoint.exp |  3 ++-
 3 files changed, 34 insertions(+), 9 deletions(-)

diff --git a/gdb/doc/python.texi b/gdb/doc/python.texi
index 9cbb2f9f57d..bcbd3b271e8 100644
--- a/gdb/doc/python.texi
+++ b/gdb/doc/python.texi
@@ -5910,7 +5910,7 @@ create both breakpoints and watchpoints.  The second accepts separate Python
 arguments similar to @ref{Explicit Locations}, and can only be used to create
 breakpoints.
 
-@defun Breakpoint.__init__ (spec @r{[}, type @r{][}, wp_class @r{][}, internal @r{][}, temporary @r{][}, qualified @r{]})
+@defun Breakpoint.__init__ (spec @r{[}, type @r{][}, wp_class @r{][}, internal @r{][}, temporary @r{][}, qualified @r{][}, announce @r{]})
 Create a new breakpoint according to @var{spec}, which is a string naming the
 location of a breakpoint, or an expression that defines a watchpoint.  The
 string should describe a location in a format recognized by the @code{break}
@@ -5940,9 +5940,15 @@ the function passed in @code{spec} as a fully-qualified name.  It is equivalent
 to @code{break}'s @code{-qualified} flag (@pxref{Linespec Locations} and
 @ref{Explicit Locations}).
 
+The optional @var{announce} argument is a boolean that controls
+whether @var{GDBN} announces the existence of the breakpoint.  The
+default is to announce, meaning that a message is printed.  Setting
+this argument to false will suppress all output from breakpoint
+creation.
+
 @end defun
 
-@defun Breakpoint.__init__ (@r{[} source @r{][}, function @r{][}, label @r{][}, line @r{]}, @r{][} internal @r{][}, temporary @r{][}, qualified @r{]})
+@defun Breakpoint.__init__ (@r{[} source @r{][}, function @r{][}, label @r{][}, line @r{]}, @r{][} internal @r{][}, temporary @r{][}, qualified @r{][}, announce @r{]})
 This second form of creating a new breakpoint specifies the explicit
 location (@pxref{Explicit Locations}) using keywords.  The new breakpoint will
 be created in the specified source file @var{source}, at the specified
diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
index 39d9bd5dff6..f942a1c631e 100644
--- a/gdb/python/py-breakpoint.c
+++ b/gdb/python/py-breakpoint.c
@@ -903,7 +903,8 @@ bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
 {
   static const char *keywords[] = { "spec", "type", "wp_class", "internal",
 				    "temporary","source", "function",
-				    "label", "line", "qualified", NULL };
+				    "label", "line", "qualified",
+				    "announce", nullptr };
   const char *spec = NULL;
   enum bptype type = bp_breakpoint;
   int access_type = hw_write;
@@ -918,13 +919,15 @@ bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
   char *function = NULL;
   PyObject *qualified_obj = nullptr;
   int qualified = 0;
+  PyObject *announce_obj = nullptr;
+  int announce = 1;
 
-  if (!gdb_PyArg_ParseTupleAndKeywords (args, kwargs, "|siiOOsssOO", keywords,
+  if (!gdb_PyArg_ParseTupleAndKeywords (args, kwargs, "|siiOOsssOOO", keywords,
 					&spec, &type, &access_type,
 					&internal,
 					&temporary, &source,
 					&function, &label, &lineobj,
-					&qualified_obj))
+					&qualified_obj, &announce_obj))
     return -1;
 
 
@@ -963,6 +966,13 @@ bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
 	return -1;
     }
 
+  if (announce_obj != nullptr)
+    {
+      announce = PyObject_IsTrue (announce_obj);
+      if (announce == -1)
+	return -1;
+    }
+
   if (bppy_init_validate_args (spec, source, function, label, line.get (),
 			       type) == -1)
     return -1;
@@ -973,9 +983,17 @@ bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
 
   try
     {
-      bppy_create_breakpoint (type, access_type, temporary_bp, internal_bp,
-			      spec, qualified, source, function, label,
-			      line.get ());
+      if (announce)
+	bppy_create_breakpoint (type, access_type, temporary_bp, internal_bp,
+				spec, qualified, source, function,
+				label, line.get ());
+      else
+	execute_fn_to_ui_file (&null_stream, [&] ()
+	  {
+	    bppy_create_breakpoint (type, access_type, temporary_bp,
+				    internal_bp, spec, qualified,
+				    source, function, label, line.get ());
+	  });
     }
   catch (const gdb_exception &except)
     {
diff --git a/gdb/testsuite/gdb.python/py-breakpoint.exp b/gdb/testsuite/gdb.python/py-breakpoint.exp
index e36e87dc291..27f0619443e 100644
--- a/gdb/testsuite/gdb.python/py-breakpoint.exp
+++ b/gdb/testsuite/gdb.python/py-breakpoint.exp
@@ -544,7 +544,8 @@ proc_with_prefix test_bkpt_address {} {
 
 proc_with_prefix test_bkpt_pending {} {
     delete_breakpoints
-    gdb_breakpoint "nosuchfunction" allow-pending
+    gdb_test_no_output "python gdb.Breakpoint(\"nosuchfunction\", announce=False)" \
+	"create pending breakpoint"
     gdb_test "python print (gdb.breakpoints()\[0\].pending)" "True" \
 	"Check pending status of pending breakpoint"
 }
-- 
2.34.3

Re: [PATCH 3/3] Let Python breakpoints be created silently

[Tom Tromey]

>>>>> "Tom" == Tom Tromey via Gdb-patches <gdb-patches@sourceware.org> writes:

Tom> Currently, a breakpoint created from Python will always announce its
Tom> presence; and in some cases (for example a pending breakpoint), other
Tom> information will be printed as well.

Tom> When scripting gdb, it's useful to be able to control the output in
Tom> cases like this.  I debated whether to simply disable the output
Tom> entirely, but I thought perhaps some existing code acts as a simple
Tom> "break"-like command and wants the output.

Tom> This patch adds a new "announce" flag to gdb.Breakpoint.  Setting this
Tom> to False will cause gdb to be silent here.

I think this one still needs a documentation review.

Tom

Re: [PATCH 3/3] Let Python breakpoints be created silently

[Eli Zaretskii]

> From: Tom Tromey <tromey@adacore.com>
> Cc: Tom Tromey <tromey@adacore.com>, Eli Zaretskii <eliz@gnu.org>
> Date: Wed, 11 Jan 2023 10:31:33 -0700
> 
> >>>>> "Tom" == Tom Tromey via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Tom> Currently, a breakpoint created from Python will always announce its
> Tom> presence; and in some cases (for example a pending breakpoint), other
> Tom> information will be printed as well.
> 
> Tom> When scripting gdb, it's useful to be able to control the output in
> Tom> cases like this.  I debated whether to simply disable the output
> Tom> entirely, but I thought perhaps some existing code acts as a simple
> Tom> "break"-like command and wants the output.
> 
> Tom> This patch adds a new "announce" flag to gdb.Breakpoint.  Setting this
> Tom> to False will cause gdb to be silent here.
> 
> I think this one still needs a documentation review.

Sorry for missing it.

The patch is okay, with a single gotcha:

> +The optional @var{announce} argument is a boolean that controls
> +whether @var{GDBN} announces the existence of the breakpoint.  The
           ^^^^
That should be @value, not @var.

Re: [PATCH 3/3] Let Python breakpoints be created silently

[Simon Marchi]

> @@ -5940,9 +5940,15 @@ the function passed in @code{spec} as a fully-qualified name.  It is equivalent
>  to @code{break}'s @code{-qualified} flag (@pxref{Linespec Locations} and
>  @ref{Explicit Locations}).
>  
> +The optional @var{announce} argument is a boolean that controls
> +whether @var{GDBN} announces the existence of the breakpoint.  The
> +default is to announce, meaning that a message is printed.  Setting
> +this argument to false will suppress all output from breakpoint
> +creation.

I assume this means "all CLI output", but any MI / DAP frontend would
still be notified?

Otherwise, the code LGTM.

Simon

Re: [PATCH 3/3] Let Python breakpoints be created silently

[Tom Tromey]

>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

>> @@ -5940,9 +5940,15 @@ the function passed in @code{spec} as a fully-qualified name.  It is equivalent
>> to @code{break}'s @code{-qualified} flag (@pxref{Linespec Locations} and
>> @ref{Explicit Locations}).
>> 
>> +The optional @var{announce} argument is a boolean that controls
>> +whether @var{GDBN} announces the existence of the breakpoint.  The
>> +default is to announce, meaning that a message is printed.  Setting
>> +this argument to false will suppress all output from breakpoint
>> +creation.

Simon> I assume this means "all CLI output", but any MI / DAP frontend would
Simon> still be notified?

Yes, I think that happens through an observer.

Tom

Re: [PATCH 3/3] Let Python breakpoints be created silently

[Pedro Alves]

On 2022-12-08 7:18 p.m., Tom Tromey via Gdb-patches wrote:
> Currently, a breakpoint created from Python will always announce its
> presence; and in some cases (for example a pending breakpoint), other
> information will be printed as well.
> 
> When scripting gdb, it's useful to be able to control the output in
> cases like this.  I debated whether to simply disable the output
> entirely, but I thought perhaps some existing code acts as a simple
> "break"-like command and wants the output.
> 
> This patch adds a new "announce" flag to gdb.Breakpoint.  Setting this
> to False will cause gdb to be silent here.

Wouldn't making the breakpoint "internal" work for the same effect?

Re: [PATCH 3/3] Let Python breakpoints be created silently

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:

Pedro> Wouldn't making the breakpoint "internal" work for the same effect?

No:

(gdb) python gdb.Breakpoint("nosuch", internal=True)
No symbol table is loaded.  Use the "file" command.
(gdb) python gdb.Breakpoint("nosuch", announce=False)
(gdb)

I didn't really think about it though.  I guess 'internal' could maybe
be reused to disable the other message as well.

Tom