[1/1] gdb, python: fix python breakpoint with extra spec

  From: Mihails Strasuns <mihails.strasuns@intel.com>

Python module `Breakpoint` constructor implementation is not
passing through any remaining spec tail after parsing location.

For example this works as expected:

(gdb) python bp1 = Breakpoint ("file:42")

But here "thread 1000" is silently ignored:

(gdb) python bp1 = Breakpoint ("file:42 thread 1000")

This patch modifies `create_breakpoint` function call from the Python
implementation so that full spec string is processed.  Unnecessary
string duplication for "spec" is removed as it is not used after the
call (tests still pass).
---
 gdb/python/py-breakpoint.c                 | 12 +++++-------
 gdb/testsuite/gdb.python/py-breakpoint.exp |  4 +++-
 2 files changed, 8 insertions(+), 8 deletions(-)
  

On 6/2/23 03:59, Christina Schimpe via Gdb-patches wrote:
> For example this works as expected:
> 
> (gdb) python bp1 = Breakpoint ("file:42")
> 
> But here "thread 1000" is silently ignored:
> 
> (gdb) python bp1 = Breakpoint ("file:42 thread 1000")

This doesn't sound particularly idiomatic/python-y.

Gdb already has getters/setters for thread, task, and other
related properties. Are those insufficient in some way?

The documentation for gdb.Breakpoint.__init__ says:

"Create a new breakpoint according to spec, which is a string
naming the location of a breakpoint."

While it happens to work, "thread 1000" is not part of the
location. That create_breakpoint() accepts this is simply a
(messy) implementation detail that has been convenient for the
CLI interpreter.

If it is desired to be able to set breakpoint properties during
construction, is expanding the ctor to accept a keyword not a
viable, if not cleaner, option?

Keith

> This patch modifies `create_breakpoint` function call from the Python
> implementation so that full spec string is processed.  Unnecessary
> string duplication for "spec" is removed as it is not used after the
> call (tests still pass).
> ---
>   gdb/python/py-breakpoint.c                 | 12 +++++-------
>   gdb/testsuite/gdb.python/py-breakpoint.exp |  4 +++-
>   2 files changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/gdb/python/py-breakpoint.c b/gdb/python/py-breakpoint.c
> index d11fc64df20..3a2f8f5f2b8 100644
> --- a/gdb/python/py-breakpoint.c
> +++ b/gdb/python/py-breakpoint.c
> @@ -893,6 +893,8 @@ bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
>     bppy_pending_object->number = -1;
>     bppy_pending_object->bp = NULL;
>   
> +  spec = skip_spaces (spec);
> +
>     try
>       {
>         switch (type)
> @@ -908,11 +910,7 @@ bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
>   
>   	    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,
> +		locspec  = string_to_location_spec (&spec,
>   						    current_language,
>   						    func_name_match_type);
>   	      }
> @@ -941,8 +939,8 @@ bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
>   	      = breakpoint_ops_for_location_spec (locspec.get (), false);
>   
>   	    create_breakpoint (gdbpy_enter::get_gdbarch (),
> -			       locspec.get (), NULL, -1, NULL, false,
> -			       0,
> +			       locspec.get (), NULL, -1, spec, false,
> +			       1,
>   			       temporary_bp, type,
>   			       0,
>   			       AUTO_BOOLEAN_TRUE,
> diff --git a/gdb/testsuite/gdb.python/py-breakpoint.exp b/gdb/testsuite/gdb.python/py-breakpoint.exp
> index 76094c95d10..3c747677fc7 100644
> --- a/gdb/testsuite/gdb.python/py-breakpoint.exp
> +++ b/gdb/testsuite/gdb.python/py-breakpoint.exp
> @@ -170,8 +170,10 @@ proc_with_prefix test_bkpt_cond_and_cmds { } {
>   
>       # Test conditional setting.
>       set bp_location1 [gdb_get_line_number "Break at multiply."]
> -    gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (\"$bp_location1\")" \
> +    gdb_py_test_silent_cmd "python bp1 = gdb.Breakpoint (\"$bp_location1 thread 1\")" \
>   	"Set breakpoint" 0
> +    gdb_test "python print (bp1.thread == 1)" "True" \
> +	"Extra thread spec has been parsed"
>       gdb_continue_to_breakpoint "Break at multiply" \
>   	".*Break at multiply.*"
>       gdb_py_test_silent_cmd "python bp1.condition = \"i == 5\"" \
  

Hi Keith, 

Thanks a lot for your review.

> If it is desired to be able to set breakpoint properties during construction, is
> expanding the ctor to accept a keyword not a viable, if not cleaner, option?

I understand, but still I am not sure. Let me try to explain.

> "Create a new breakpoint according to spec, which is a string naming the
> location of a breakpoint."
> 
> While it happens to work, "thread 1000" is not part of the location.

The entire section says: 
"Create a new breakpoint according to 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 break command (see Setting Breakpoints) or, in the case of a watchpoint, by the watch command (see Setting Watchpoints)"

So I tried to figure out what this location format should look like and found this: https://sourceware.org/gdb/onlinedocs/gdb/Set-Breaks.html

"break locspec
Set a breakpoint at all the code locations in your program that result from resolving the given locspec. locspec can specify a function name, a line number, an address of an instruction, and more. See Location Specifications, for the various forms of locspec. The breakpoint will stop your program just before it executes the instruction at the address of any of the breakpoint’s code locations.
...

It is also possible to insert a breakpoint that will stop the program only if a specific thread (see Thread-Specific Breakpoints) or a specific task (see Ada Tasks) hits that breakpoint.
"

which finally brought me to Thread-Specific Breakpoints: https://sourceware.org/gdb/onlinedocs/gdb/Thread_002dSpecific-Breakpoints.html#Thread_002dSpecific-Breakpoints

"break locspec thread thread-id
break locspec thread thread-id if …
locspec specifies a code location or locations in your program. See Location Specifications, for details."

So from my understanding the suggested fix is doing what the documentation describes.

Christina
Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
  

Hi,

Thank you for engaging with me.

On 6/5/23 00:36, Schimpe, Christina wrote:
> 
> The entire section says:
> "Create a new breakpoint according to 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 break command (see Setting Breakpoints) or, in the case of a watchpoint, by the watch command (see Setting Watchpoints)"

Right but the inclusion of a thread, task, or condition limitation is simply
a byproduct of the way create_breakpoint is as much an implementation of the
UI as it is the actual "backend" (or API) that does the work of creating
breakpoints.

> So I tried to figure out what this location format should look like and found this: https://sourceware.org/gdb/onlinedocs/gdb/Set-Breaks.html
> 
> "break locspec
> Set a breakpoint at all the code locations in your program that result from resolving the given locspec. locspec can specify a function name, a line number, an address of an instruction, and more. See Location Specifications, for the various forms of locspec. The breakpoint will stop your program just before it executes the instruction at the address of any of the breakpoint’s code locations.
> ...

The section on "Location Specifications" mentioned therein:

https://sourceware.org/gdb/onlinedocs/gdb/Location-Specifications.html

No location specification type mentions threads or tasks. These are
not "code locations."

> It is also possible to insert a breakpoint that will stop the program
> only if a specific thread (see Thread-Specific Breakpoints) or a
> specific task (see Ada Tasks) hits that breakpoint. "


That describes the "break" command. MI does not work the same way. There
is a canonical way to do this in MI, and my argument is that Python or
Guile should work similarly.

> which finally brought me to Thread-Specific Breakpoints:
> https://sourceware.org/gdb/onlinedocs/gdb/Thread_002dSpecific-Breakpoints.html#Thread_002dSpecific-Breakpoints
>
>  "break locspec thread thread-id break locspec thread thread-id if … 
> locspec specifies a code location or locations in your program. See
> Location Specifications, for details."

Note that this is talking again about the "break" *command* not locations.
A thread or task is not a code location.

So my original question remains: Why is a more python-y approach,
utilizing Breakpoint.thread, not a better/more consistent solution?

In any case, we have an impasse, and maintainers will have to chime
in.

I really do appreciate your patch and hope that there is a path for
this missing functionality.

Keith
  

Hi, 

I just figured out that this was already posted in February 2021 by Mihails:
https://sourceware.org/pipermail/gdb-patches/2021-February/176572.html

I added Tom to this conversation, as he reviewed this once:
https://sourceware.org/pipermail/gdb-patches/2021-February/176575.html

This is Mihails' final comment:
https://sourceware.org/pipermail/gdb-patches/2021-February/176578.html
He points to the same part of the docs:
".. create a new breakpoint according to 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 break command ...
Based on this wording I have understood that the intention was for any valid "break" spec string to also be valid "Breakpoint" spec argument. "
 
> So my original question remains: Why is a more python-y approach, utilizing
> Breakpoint.thread, not a better/more consistent solution?

Maybe both options are valid, as Mihails commented on this:
 "Personally I also think it doesn't harm to support both object-like API and this, considering how low of an effort it is."

Thanks,
Christina

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
  

Kindly pinging for thoughts!

Thanks,
Christina

> -----Original Message-----
> From: Schimpe, Christina
> Sent: Friday, June 9, 2023 10:02 AM
> To: Keith Seitz <keiths@redhat.com>; gdb-patches@sourceware.org; Tom Tromey
> <tom@tromey.com>
> Subject: RE: [PATCH 1/1] gdb, python: fix python breakpoint with extra spec
> 
> Hi,
> 
> I just figured out that this was already posted in February 2021 by Mihails:
> https://sourceware.org/pipermail/gdb-patches/2021-February/176572.html
> 
> I added Tom to this conversation, as he reviewed this once:
> https://sourceware.org/pipermail/gdb-patches/2021-February/176575.html
> 
> This is Mihails' final comment:
> https://sourceware.org/pipermail/gdb-patches/2021-February/176578.html
> He points to the same part of the docs:
> ".. create a new breakpoint according to 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 break command ...
> Based on this wording I have understood that the intention was for any valid
> "break" spec string to also be valid "Breakpoint" spec argument. "
> 
> > So my original question remains: Why is a more python-y approach, utilizing
> > Breakpoint.thread, not a better/more consistent solution?
> 
> Maybe both options are valid, as Mihails commented on this:
>  "Personally I also think it doesn't harm to support both object-like API and this,
> considering how low of an effort it is."
> 
> Thanks,
> Christina

Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de <http://www.intel.de>
Managing Directors: Christin Eisenschmid, Sharon Heck, Tiffany Doon Silva  
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928
  

>>>>> Schimpe, Christina via Gdb-patches <gdb-patches@sourceware.org> writes:

> Kindly pinging for thoughts!

Hi.  I tend to agree with Keith here -- the breakpoint constructor
should accept a linespec, but not the other things that are parsed by
the CLI.  (And FWIW, we erred in making it possible to set a watchpoint
this way, we should have introduced a new class.)

It would be fine by me to add new parameters to the Breakpoint
constructor to set the thread/task/condition.  And, if you want to be
able to parse a breakpoint specification the same way that "break" does,
I'd also support some kind of parse function that does the parsing and
returns a dictionary that could be passed to the constructor.

thanks,
Tom