python/19506 -- gdb.Breakpoint address location regression
Commit Message
When the locations API was committed, it assumed that all valid
arguments to the gdb.Breakpoint command were a linespec (aside from
keywords describing various breakpoint properties). However, address
locations are a separate class of locations which were overlooked by my
patch.
This patch introduces a new function analogous to the CLI function
string_to_event_location. This new function only handles address and
linespec locations. I have made no attempt to fully implement explicit
locations.
This patch fixes python/19506:
(gdb) python gdb.Breakpoint("*main")
Traceback (most recent call last):
File "<string>", line 1, in <module>
RuntimeError: Function "*main" not defined.
Error while executing Python code.
Now:
(gdb) python gdb.Breakpoint("*main")
Breakpoint 1 at 0x4005fb: file ../../../src/gdb/testsuite/gdb.python/py-breakpoint.c, line 32.
gdb/ChangeLog
* python/py-breakpoint.c (python_string_to_event_location): New
function.
(bppy_init): Use python_string_to_event_location instead of
new_linespec_location.
gdb/testsuite/gdb.python
* gdb.python/py-breakpoint.exp (test_bkpt_address): New proc.
(toplevel): Call test_bkpt_address.
---
gdb/python/py-breakpoint.c | 35 +++++++++++++++++++++++++++++-
gdb/testsuite/gdb.python/py-breakpoint.exp | 31 ++++++++++++++++++++++++++
2 files changed, 65 insertions(+), 1 deletion(-)
Comments
Hi Keith,
On Thu, Jan 21, 2016 at 02:05:26PM -0800, Keith Seitz wrote:
> When the locations API was committed, it assumed that all valid
> arguments to the gdb.Breakpoint command were a linespec (aside from
> keywords describing various breakpoint properties). However, address
> locations are a separate class of locations which were overlooked by my
> patch.
>
> This patch introduces a new function analogous to the CLI function
> string_to_event_location. This new function only handles address and
> linespec locations. I have made no attempt to fully implement explicit
> locations.
>
> This patch fixes python/19506:
>
> (gdb) python gdb.Breakpoint("*main")
> Traceback (most recent call last):
> File "<string>", line 1, in <module>
> RuntimeError: Function "*main" not defined.
> Error while executing Python code.
>
> Now:
>
> (gdb) python gdb.Breakpoint("*main")
> Breakpoint 1 at 0x4005fb: file ../../../src/gdb/testsuite/gdb.python/py-breakpoint.c, line 32.
>
> gdb/ChangeLog
>
> * python/py-breakpoint.c (python_string_to_event_location): New
> function.
> (bppy_init): Use python_string_to_event_location instead of
> new_linespec_location.
>
> gdb/testsuite/gdb.python
>
> * gdb.python/py-breakpoint.exp (test_bkpt_address): New proc.
> (toplevel): Call test_bkpt_address.
Silly question, but does this work with " *main" (extra leading
space)?
It's too bad you have to explicitly check for '*' in the string.
Perhaps we should delegate that part to the linespec module?
Perhaps other areas might want to have the same (eg. the guile
breakpoint module).
On 01/26/2016 04:22 AM, Joel Brobecker wrote:
> Silly question, but does this work with " *main" (extra leading
> space)?
A very valid question, in fact. Answer: It works *now*! ;-)
> It's too bad you have to explicitly check for '*' in the string.
> Perhaps we should delegate that part to the linespec module?
> Perhaps other areas might want to have the same (eg. the guile
> breakpoint module).
[Apologies for waxing a little philosophical here...]
Here we are again with the same question as last time: should address
locations be considered during linespec parsing? To be honest, I keep
bouncing back and forth on this (a bit).
IMO we have two opposing "forces" involved in this decision:
1) Ease of use
2) Separation of APIs
For me, #1 really boils down to UI writers "passing the buck" down to
gdb's internals (via CLI generalisms). With the location API work, I
tried to make this layer a little more distinct/separate. UIs are
responsible for implementing their own representations of locations in a
way most consistent with their implementation specifics. [Example: using
'gdb.Breakpoint("-source foo.c -line 3")' seems downright wrong to me.]
The CLI representation is well known, and since we already had a
"parser" for it, MI, Python, and Guile just followed suit, reusing the
syntax. Hey, it was easy! [And there is *nothing* wrong with that decision.]
#2 is really just a theoretical concept. It isn't strictly necessary,
but it sure would be nice to have [YMMV]. While I like encapsulating
small(er) bits of separate functionality into separate APIs, it
sometimes seems more work than it is worth.
However, I think there is a trade-off here that can maintain (much of)
both of these desirable traits. I missed that when I implemented the
locations patches.
I failed to acknowledge that we have this legacy "re-using" of CLI
idioms like linespecs, and I should have made concessions to facilitate
re-using this "legacy" syntax. [I say "legacy" to mean pre-locations
linespecs compared to today's linespec location. An unfortunately subtle
choice of words, I know.]
In light of this, I am suggesting that instead of requiring
python/mi/guile/whatever to implement their own string_to_event_location
functions, that the current string_to_event_location be split into a
"legacy" portion (that deals with address, probe, and linespec
locations) and a newer (and separate) explicit-handling form (for the
CLI only).
This leaves python/guile to implement an explicit location specification
in a manner more consistent with those interpreters, e.g., python could
use named arguments to gdb.Breakpoint:
gdb.Breakpoint(source="foo.c", line="3")
This way we wouldn't "force" these languages to use the CLI's
argument/value paradigm. I've already done something similar for MI. [MI
is currently using string_to_event_location, which means it will
erroneously support the CLI's explicit syntax in addition to its own!
Bug! My bad! Have patch.]
In short:
1) Remove explicit locations from string_to_event_location.
Use this "new" function /everywhere/ legacy linespec support is
desired. In my working tree, I call this
string_to_event_location_basic.
2) string_to_event_locations can now be refactored to do two things:
check for an explicit location OR call the _basic version to deal
with linespec, address, and probe locations.
WDYT? [I can send a small patch series to address/clean up all of these,
if you think this is a reasonable resolution.]
Keith
On 21/01/16 22:05, Keith Seitz wrote:
> When the locations API was committed, it assumed that all valid > arguments to the gdb.Breakpoint command were a linespec (aside from > keywords describing various breakpoint properties). However, address > locations are a separate class of locations which were overlooked by my > patch. > > This patch introduces a new function analogous to the CLI function > string_to_event_location. This new function only handles address and > linespec locations. I have made no attempt to fully implement explicit > locations. > > This patch fixes python/19506: > > (gdb) python gdb.Breakpoint("*main") > Traceback (most recent call last): > File "<string>", line 1, in <module> > RuntimeError: Function "*main" not defined. > Error while executing Python code. > > Now: > > (gdb) python gdb.Breakpoint("*main") > Breakpoint 1 at 0x4005fb: file ../../../src/gdb/testsuite/gdb.python/py-breakpoint.c, line 32. > > gdb/ChangeLog > > * python/py-breakpoint.c (python_string_to_event_location): New > function. > (bppy_init): Use
python_string_to_event_location instead of > new_linespec_location. > > gdb/testsuite/gdb.python > > * gdb.python/py-breakpoint.exp (test_bkpt_address): New proc. > (toplevel): Call test_bkpt_address.
It looks fine to me, Keith, beyond the further musings of linespec
that follows earlier in the thread and you end up proceeding in this
direction.
Cheers
Phil
> [Apologies for waxing a little philosophical here...]
[Apologies for triggering the discussion ;-)...]
> For me, #1 really boils down to UI writers "passing the buck" down to
> gdb's internals (via CLI generalisms). With the location API work, I
> tried to make this layer a little more distinct/separate. UIs are
> responsible for implementing their own representations of locations in a
> way most consistent with their implementation specifics. [Example: using
> 'gdb.Breakpoint("-source foo.c -line 3")' seems downright wrong to me.]
Agreed.
> In light of this, I am suggesting that instead of requiring
> python/mi/guile/whatever to implement their own string_to_event_location
> functions, that the current string_to_event_location be split into a
> "legacy" portion (that deals with address, probe, and linespec
> locations) and a newer (and separate) explicit-handling form (for the
> CLI only).
>
> This leaves python/guile to implement an explicit location specification
> in a manner more consistent with those interpreters, e.g., python could
> use named arguments to gdb.Breakpoint:
>
> gdb.Breakpoint(source="foo.c", line="3")
I agree that each extension language should have their own clean API
for creating breakpoints using explicit locations. That's a lot
cleaner.
> This way we wouldn't "force" these languages to use the CLI's
> argument/value paradigm. I've already done something similar for MI. [MI
> is currently using string_to_event_location, which means it will
> erroneously support the CLI's explicit syntax in addition to its own!
> Bug! My bad! Have patch.]
>
> In short:
> 1) Remove explicit locations from string_to_event_location.
> Use this "new" function /everywhere/ legacy linespec support is
> desired. In my working tree, I call this
> string_to_event_location_basic.
> 2) string_to_event_locations can now be refactored to do two things:
> check for an explicit location OR call the _basic version to deal
> with linespec, address, and probe locations.
>
> WDYT? [I can send a small patch series to address/clean up all of these,
> if you think this is a reasonable resolution.]
It sounds like a good idea, as I think it'll factorize the code.
In the grand scheme of things, the current situation is not all
that bad, though, so I wouldn't say that this absolutely needs
to be done ahead of everything else.
So, what I would suggest is first push the current patch if
people agree with it. That way, we are covered for the release
branch. And then, if you have some time to look at this
enhancement before we release 7.11, then we can look at
possibly backporting it.
Did anyone review the patch? Typically, Doug is the one reviewing
those patches, but I can take a look as well.
Thanks, Keith!
On 01/31/2016 07:32 PM, Joel Brobecker wrote:
>
> It sounds like a good idea, as I think it'll factorize the code.
> In the grand scheme of things, the current situation is not all
> that bad, though, so I wouldn't say that this absolutely needs
> to be done ahead of everything else.
>
> So, what I would suggest is first push the current patch if
> people agree with it. That way, we are covered for the release
> branch. And then, if you have some time to look at this
> enhancement before we release 7.11, then we can look at
> possibly backporting it.
>
> Did anyone review the patch? Typically, Doug is the one reviewing
> those patches, but I can take a look as well.
I don't think I actually posted my patch. I wanted to test the waters
first. :-)
I'll get right to this before trying to combust brain cells on 19474.
Keith
@@ -31,6 +31,7 @@
#include "arch-utils.h"
#include "language.h"
#include "location.h"
+#include "linespec.h"
/* Number of live breakpoints. */
static int bppy_live;
@@ -624,6 +625,38 @@ bppy_get_ignore_count (PyObject *self, void *closure)
return PyInt_FromLong (self_bp->bp->ignore_count);
}
+/* Attempt to convert the string in *STRINGP to an event_location.
+ STRINGP is advanced past any processed input. Returns an event_location
+ (malloc'd).
+
+ The return result must be freed with delete_event_location. */
+
+static struct event_location *
+python_string_to_event_location (char **stringp)
+{
+ struct event_location *location;
+
+ /* First check if the string represents a address location. */
+ if (*stringp != NULL && **stringp == '*')
+ {
+ const char *arg, *orig;
+ CORE_ADDR addr;
+
+ orig = arg = *stringp;
+ addr = linespec_expression_to_pc (&arg);
+ location = new_address_location (addr, orig, arg - orig);
+ *stringp += arg - orig;
+ }
+ else
+ {
+ /* Explicit locations are not yet implemented, so everything else
+ must be a linespec location. */
+ location = new_linespec_location (stringp);
+ }
+
+ return location;
+}
+
/* Python function to create a new breakpoint. */
static int
bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
@@ -672,7 +705,7 @@ bppy_init (PyObject *self, PyObject *args, PyObject *kwargs)
{
struct event_location *location;
- location = new_linespec_location (©);
+ location = python_string_to_event_location (©);
make_cleanup_delete_event_location (location);
create_breakpoint (python_gdbarch,
location, NULL, -1, NULL,
@@ -462,6 +462,36 @@ proc test_bkpt_temporary { } {
}
}
+# Test address locations.
+
+proc test_bkpt_address {} {
+ global gdb_prompt decimal srcfile
+
+ # Delete all breakpoints
+ delete_breakpoints
+
+ gdb_test "python gdb.Breakpoint(\"*main\")" \
+ ".*Breakpoint ($decimal)+ at .*$srcfile, line ($decimal)+\."
+
+ gdb_py_test_silent_cmd \
+ "python main_loc = gdb.parse_and_eval(\"main\").address" \
+ "eval address of main" 0
+
+ # Python 2 vs 3 ... Check `int' first. If that fails, try `long'.
+ gdb_test_multiple "python main_addr = int(main_loc)" "int value of main" {
+ -re "Traceback.*$gdb_prompt $" {
+ gdb_test_no_output "python main_addr = long(main_loc)" \
+ "long value of main"
+ }
+ -re "$gdb_prompt $" {
+ pass "int value of main"
+ }
+ }
+
+ gdb_test "python gdb.Breakpoint(\"*{}\".format(str(main_addr)))" \
+ ".*Breakpoint ($decimal)+ at .*$srcfile, line ($decimal)+\."
+}
+
test_bkpt_basic
test_bkpt_deletion
test_bkpt_cond_and_cmds
@@ -470,3 +500,4 @@ test_watchpoints
test_bkpt_internal
test_bkpt_eval_funcs
test_bkpt_temporary
+test_bkpt_address