* [PATCH 0/1]Python breakpoint with extra spec @ 2023-06-02 10:59 Christina Schimpe 2023-06-02 10:59 ` [PATCH 1/1] gdb, python: fix python " Christina Schimpe 0 siblings, 1 reply; 8+ messages in thread From: Christina Schimpe @ 2023-06-02 10:59 UTC (permalink / raw) To: gdb-patches Hi all, this patch was originally written by Mihails Strasuns, I just rebased it onto current master. Regards, Christina Mihails Strasuns (1): gdb, python: fix python breakpoint with extra spec gdb/python/py-breakpoint.c | 12 +++++------- gdb/testsuite/gdb.python/py-breakpoint.exp | 4 +++- 2 files changed, 8 insertions(+), 8 deletions(-) -- 2.25.1 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/1] gdb, python: fix python breakpoint with extra spec 2023-06-02 10:59 [PATCH 0/1]Python breakpoint with extra spec Christina Schimpe @ 2023-06-02 10:59 ` Christina Schimpe 2023-06-02 15:07 ` Keith Seitz 0 siblings, 1 reply; 8+ messages in thread From: Christina Schimpe @ 2023-06-02 10:59 UTC (permalink / raw) To: gdb-patches 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(-) 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 (©, + 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\"" \ -- 2.25.1 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] gdb, python: fix python breakpoint with extra spec 2023-06-02 10:59 ` [PATCH 1/1] gdb, python: fix python " Christina Schimpe @ 2023-06-02 15:07 ` Keith Seitz 2023-06-05 7:36 ` Schimpe, Christina 0 siblings, 1 reply; 8+ messages in thread From: Keith Seitz @ 2023-06-02 15:07 UTC (permalink / raw) To: Christina Schimpe, gdb-patches 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 (©, > + 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\"" \ ^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH 1/1] gdb, python: fix python breakpoint with extra spec 2023-06-02 15:07 ` Keith Seitz @ 2023-06-05 7:36 ` Schimpe, Christina 2023-06-06 15:39 ` Keith Seitz 0 siblings, 1 reply; 8+ messages in thread From: Schimpe, Christina @ 2023-06-05 7:36 UTC (permalink / raw) To: Keith Seitz, gdb-patches 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] gdb, python: fix python breakpoint with extra spec 2023-06-05 7:36 ` Schimpe, Christina @ 2023-06-06 15:39 ` Keith Seitz 2023-06-09 8:02 ` Schimpe, Christina 0 siblings, 1 reply; 8+ messages in thread From: Keith Seitz @ 2023-06-06 15:39 UTC (permalink / raw) To: Schimpe, Christina, gdb-patches 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH 1/1] gdb, python: fix python breakpoint with extra spec 2023-06-06 15:39 ` Keith Seitz @ 2023-06-09 8:02 ` Schimpe, Christina 2023-07-03 8:36 ` [PING][PATCH " Schimpe, Christina 0 siblings, 1 reply; 8+ messages in thread From: Schimpe, Christina @ 2023-06-09 8:02 UTC (permalink / raw) To: Keith Seitz, gdb-patches, Tom Tromey 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PING][PATCH 1/1] gdb, python: fix python breakpoint with extra spec 2023-06-09 8:02 ` Schimpe, Christina @ 2023-07-03 8:36 ` Schimpe, Christina 2023-07-06 15:46 ` Tom Tromey 0 siblings, 1 reply; 8+ messages in thread From: Schimpe, Christina @ 2023-07-03 8:36 UTC (permalink / raw) To: Keith Seitz, gdb-patches, Tom Tromey 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PING][PATCH 1/1] gdb, python: fix python breakpoint with extra spec 2023-07-03 8:36 ` [PING][PATCH " Schimpe, Christina @ 2023-07-06 15:46 ` Tom Tromey 0 siblings, 0 replies; 8+ messages in thread From: Tom Tromey @ 2023-07-06 15:46 UTC (permalink / raw) To: Schimpe, Christina via Gdb-patches Cc: Keith Seitz, Tom Tromey, Schimpe, Christina >>>>> 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2023-07-06 15:46 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2023-06-02 10:59 [PATCH 0/1]Python breakpoint with extra spec Christina Schimpe 2023-06-02 10:59 ` [PATCH 1/1] gdb, python: fix python " Christina Schimpe 2023-06-02 15:07 ` Keith Seitz 2023-06-05 7:36 ` Schimpe, Christina 2023-06-06 15:39 ` Keith Seitz 2023-06-09 8:02 ` Schimpe, Christina 2023-07-03 8:36 ` [PING][PATCH " Schimpe, Christina 2023-07-06 15:46 ` Tom Tromey
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).