[v6,4/9] Explicit locations: introduce address locations
Commit Message
Hi Keith,
On Wed, Aug 05, 2015 at 04:30:04PM -0700, Keith Seitz wrote:
> *This patch has previously been approved.*
>
> This patch adds support for address locations, of the form "*ADDR".
> [Support for address linespecs has been removed/replaced by this "new"
> location type.] This patch also converts any existing address locations
> from its previous linespec type.
>
> gdb/ChangeLog:
>
> * breakpoint.c (create_thread_event_breakpoint, init_breakpoint_sal):
> Convert linespec to address location.
> * linespec.c (canonicalize_linespec): Do not handle address
> locations here.
> (convert_address_location_to_sals): New function; contents moved
> from ...
> (convert_linespc_to_sals): ... here.
> (parse_linespec): Remove address locations from linespec grammar.
> Remove handling of address locations.
> (linespec_lex_to_end): Remove handling of address linespecs.
> (event_location_to_sals): Handle ADDRESS_LOCATION.
> (linespec_expression_to_pc): Export.
> * linespec.h (linespec_expression_to_pc): Add declaration.
> * location.c (struct event_location.u) <address>: New member.
> (new_address_location, get_address_location): New functions.
> (copy_event_location, delete_event_location, event_location_to_string)
> (string_to_event_location, event_location_empty_p): Handle address
> locations.
> * location.h (enum event_location_type): Add ADDRESS_LOCATION.
> (new_address_location, get_address_location): Declare.
> * python/py-finishbreakpoint.c (bpfinishpy_init): Convert linespec
> to address location.
> * spu-tdep.c (spu_catch_start): Likewise.
We just found an issue with this patch. For instance, if you try
to debug a program (any program) which has PIE enabled, you'll see:
(gdb) b *main
Breakpoint 1 at 0x51a: file hello.c, line 3.
(gdb) run
Starting program: /[...]/hello
Error in re-setting breakpoint 1: Warning:
Cannot insert breakpoint 1.
Cannot access memory at address 0x51a
Warning:
Cannot insert breakpoint 1.
Cannot access memory at address 0x51a
What happens is that the patch makes the implicit assumption that
the address computed the first time is static, as if it was designed
to only support litteral expressions (Eg. "*0x1234"). This allows
the shortcut of not re-computing the breakpoint location's address
when re-setting breakpoints.
However, this does not work in general, as demonstrated in the example
above. As a side note, an interesting side effect is that, before
running the program, "info break" shows (correctly):
(gdb) info break
Num Type Disp Enb Address What
1 breakpoint keep y 0x0000051a in main at hello.c:3
But after running the program, we now get ("What" is empty):
(gdb) info break
Num Type Disp Enb Address What
1 breakpoint keep y 0x0000051a
For my initial attempt at fixing this, I tried to extend what you did
to handle *EXPR as an ADDRESS_LOCATION event_location, and the attached
patch is an attempt to do just that. One of the holes I had to plug was
the fact that the original expression was not stored anywhere (which
makes sense, given that we explicitly skipping the re-evaluation).
I re-used the EL_STRING part of the location rather than making the
address field in the event_location union a struct of its own holding
both address and expression.
In any case, the patch attached seems to work, at least for the case
above. It's currently only lightly tested, and also only a prototype
where documentation and memory management are missing. But while working
on this approach, I had a growing feeling that we needed to step back
and re-evaluate whether using an ADDRESS_LOCATION for handling those
was the right approach at all. For instance, in light of the above,
would it make better sense to just handle "*EXPR" the same way we handle
non-explicit locations? We could keep ADDRESS_LOCATION event locations
for cases where we know the breakpoint's address is indeed static
(Eg: thread-event breakpoints), but I'm wondering if the gain is
really worth the extra code...
WDYT? I admit I'm a little lost still between the various layers
of locations, event_locations, etc. Do you want to take it from there?
Thanks!
Comments
Hi, Joel,
Thank you for bringing this to my attention. I was wondering when a bug
about this mega-patch would surface!
[Aside: Bah humbug. There is no test covering this simple/common use case!]
On 12/13/2015 11:11 PM, Joel Brobecker wrote:
> We just found an issue with this patch. For instance, if you try
> to debug a program (any program) which has PIE enabled, you'll see:
>
> (gdb) b *main
> Breakpoint 1 at 0x51a: file hello.c, line 3.
> (gdb) run
> Starting program: /[...]/hello
> Error in re-setting breakpoint 1: Warning:
> Cannot insert breakpoint 1.
> Cannot access memory at address 0x51a
>
> Warning:
> Cannot insert breakpoint 1.
> Cannot access memory at address 0x51a
>
> What happens is that the patch makes the implicit assumption that
> the address computed the first time is static, as if it was designed
> to only support litteral expressions (Eg. "*0x1234"). This allows
> the shortcut of not re-computing the breakpoint location's address
> when re-setting breakpoints.
Ha. Yeah. That would be a bug. :-)
> For my initial attempt at fixing this, I tried to extend what you did
> to handle *EXPR as an ADDRESS_LOCATION event_location, and the attached
> patch is an attempt to do just that. One of the holes I had to plug was
> the fact that the original expression was not stored anywhere (which
> makes sense, given that we explicitly skipping the re-evaluation).
> I re-used the EL_STRING part of the location rather than making the
> address field in the event_location union a struct of its own holding
> both address and expression.
That looks like the correct approach at fixing the bug. Or at least, I'm
fairly confident your patch is (darn close if not identical) to what I
would have done.
> But while working on this approach, I had a growing feeling that we
> needed to step back and re-evaluate whether using an ADDRESS_LOCATION
> for handling those was the right approach at all.
Obviously, this is a decision which needs to be made before I can commit
to any course of action.
> For instance, in light of the above, would it make better sense to
> just handle "*EXPR" the same way we handle non-explicit locations?
I've given this some thought in an attempt to convincingly steer the
direction in favor of removing address locations or keeping them.
More on this after I offer this opinion (all $0.000000000002 of it):
> We could keep ADDRESS_LOCATION event locations for cases where we
> know the breakpoint's address is indeed static.
Regardless of how address locations are specified via the UI, I have a
pretty strong preference that they are all treated alike, and I think
you'd probably wouldn't (completely) disagree.
So this boils down to whether address locations are linespecs or are
their own class of location.
This question is a little difficult for me to definitely answer.
Certainly address locations are treated differently than true linespecs.
No colons are permitted; after the expression is parsed, any further
linespec-y options are ignored (actually error). Likewise on the
explicit side, -address precludes the use of any other
(location-specific) option.
If we later added some sort of address-y specific feature, it could be
introduced in a linespec-y way using a ':' (break *EXPR:option / break
option:*EXPR).
On the explicit side, we'd probably have a new option name (or share an
existing one).
Regardless of how we chose to implement this expansion, though, it is
(quite likely) incompatible with generic source:func:label:line
linespecs. So once again, we have an all-or-nothing representation.
Use this set of rules for "linespec locations" and this set of rules for
"address locations." [I have tried to linguistically distinguish between
our historical linespecs (which include *EXPR) and "linespec locations"
(which currently exclude *EXPR).]
One could argue that this either-or behavior supports maintaining
separate location types.
One could also argue that this behavior is degenerate. It's a special
case. [Wow, am I running for political office or what?!? Nothing like a
simple, straightforward answer to a question!]
I guess when it boils down to it, I don't really have a super strong
preference for either case, but I do have a preference for maintaining
addresses as a separate location type. What can I say? I hate special
cases! :-P
> WDYT? I admit I'm a little lost still between the various layers
> of locations, event_locations, etc. Do you want to take it from there?
I am more than happy to fix anything related to this code in any
appropriate manner dictated by maintainers. [Of course, if this involves
a massive rewrite, I will have to clear with my management!]
Let me know how you and other maintainers would like me to proceed.
Keith
> I guess when it boils down to it, I don't really have a super strong
> preference for either case, but I do have a preference for maintaining
> addresses as a separate location type. What can I say? I hate special
> cases! :-P
>
> > WDYT? I admit I'm a little lost still between the various layers
> > of locations, event_locations, etc. Do you want to take it from there?
>
> I am more than happy to fix anything related to this code in any
> appropriate manner dictated by maintainers. [Of course, if this involves
> a massive rewrite, I will have to clear with my management!]
>
> Let me know how you and other maintainers would like me to proceed.
Thanks for your insights, Keith. I agree the choice is not clearly
black or white; but because you probably know this area better than
anyone, I'm inclined to follow your suggestion. Given that the patch
I sent is close to what you would have written, I think it makes sense
for me to see this through, and have you help with the review? Any
comments on the current one before I proceed with a more official
submission?
Thanks!
From a39966dcad6f4c32a10a724d03d842cceb3a4533 Mon Sep 17 00:00:00 2001
From: Joel Brobecker <brobecker@adacore.com>
Date: Tue, 8 Dec 2015 19:04:56 +0100
Subject: [PATCH] WIP: Fix for "break *<func_name>'address" before running PIE
program.
For OC04-018.
---
gdb/breakpoint.c | 15 +++++++++++++--
gdb/linespec.c | 24 +++++++++++++++++++++---
gdb/location.c | 16 ++++++++++++++--
gdb/location.h | 9 ++++++++-
gdb/python/py-finishbreakpoint.c | 2 +-
gdb/spu-tdep.c | 2 +-
6 files changed, 58 insertions(+), 10 deletions(-)
@@ -7752,7 +7752,7 @@ create_thread_event_breakpoint (struct gdbarch *gdbarch, CORE_ADDR address)
b->enable_state = bp_enabled;
/* location has to be used or breakpoint_re_set will delete me. */
- b->location = new_address_location (b->loc->address);
+ b->location = new_address_location (b->loc->address, NULL, 0);
update_global_location_list_nothrow (UGLL_MAY_INSERT);
@@ -9330,7 +9330,18 @@ init_breakpoint_sal (struct breakpoint *b, struct gdbarch *gdbarch,
if (location != NULL)
b->location = location;
else
- b->location = new_address_location (b->loc->address);
+ {
+ const char *addr_string = NULL;
+ int addr_string_len = 0;
+
+ if (location != NULL)
+ addr_string = event_location_to_string (location);
+ if (addr_string != NULL)
+ addr_string_len = strlen (addr_string);
+
+ b->location = new_address_location (b->loc->address,
+ addr_string, addr_string_len);
+ }
b->filter = filter;
}
@@ -2498,9 +2498,27 @@ event_location_to_sals (linespec_parser *parser,
break;
case ADDRESS_LOCATION:
- result
- = convert_address_location_to_sals (PARSER_STATE (parser),
- get_address_location (location));
+ {
+ const char *addr_string = get_address_string_location (location);
+ CORE_ADDR addr = get_address_location (location);
+
+ if (addr_string != NULL)
+ {
+ char *expr = xstrdup (addr_string);
+ const char *const_expr = expr;
+ struct cleanup *cleanup = make_cleanup (xfree, expr);
+
+ addr = linespec_expression_to_pc (&const_expr);
+ if (PARSER_STATE (parser)->canonical != NULL)
+ PARSER_STATE (parser)->canonical->location
+ = copy_event_location (location);
+
+ do_cleanups (cleanup);
+ }
+
+ result = convert_address_location_to_sals (PARSER_STATE (parser),
+ addr);
+ }
break;
case EXPLICIT_LOCATION:
@@ -113,13 +113,16 @@ get_linespec_location (const struct event_location *location)
/* See description in location.h. */
struct event_location *
-new_address_location (CORE_ADDR addr)
+new_address_location (CORE_ADDR addr, const char *addr_string,
+ int addr_string_len)
{
struct event_location *location;
location = XCNEW (struct event_location);
EL_TYPE (location) = ADDRESS_LOCATION;
EL_ADDRESS (location) = addr;
+ if (addr_string != NULL)
+ EL_STRING (location) = xstrndup (addr_string, addr_string_len);
return location;
}
@@ -134,6 +137,15 @@ get_address_location (const struct event_location *location)
/* See description in location.h. */
+const char *
+get_address_string_location (const struct event_location *location)
+{
+ gdb_assert (EL_TYPE (location) == ADDRESS_LOCATION);
+ return EL_STRING (location);
+}
+
+/* See description in location.h. */
+
struct event_location *
new_probe_location (const char *probe)
{
@@ -635,7 +647,7 @@ string_to_event_location (char **stringp,
orig = arg = *stringp;
addr = linespec_expression_to_pc (&arg);
- location = new_address_location (addr);
+ location = new_address_location (addr, orig, arg - orig);
*stringp += arg - orig;
}
else
@@ -130,7 +130,8 @@ extern const char *
and should be freed with delete_event_location. */
extern struct event_location *
- new_address_location (CORE_ADDR addr);
+ new_address_location (CORE_ADDR addr, const char *addr_string,
+ int addr_string_len);
/* Return the address location (a CORE_ADDR) of the given event_location
(which must be of type ADDRESS_LOCATION). */
@@ -138,6 +139,12 @@ extern struct event_location *
extern CORE_ADDR
get_address_location (const struct event_location *location);
+/* Return the expression (a string) that was used to compute the address
+ of the given event_location (which must be of type ADDRESS_LOCATION). */
+
+extern const char *
+ get_address_string_location (const struct event_location *location);
+
/* Create a new probe location. The return result is malloc'd
and should be freed with delete_event_location. */
@@ -297,7 +297,7 @@ bpfinishpy_init (PyObject *self, PyObject *args, PyObject *kwargs)
struct cleanup *back_to;
/* Set a breakpoint on the return address. */
- location = new_address_location (get_frame_pc (prev_frame));
+ location = new_address_location (get_frame_pc (prev_frame), NULL, 0);
back_to = make_cleanup_delete_event_location (location);
create_breakpoint (python_gdbarch,
location, NULL, thread, NULL,
@@ -2001,7 +2001,7 @@ spu_catch_start (struct objfile *objfile)
/* Use a numerical address for the set_breakpoint command to avoid having
the breakpoint re-set incorrectly. */
- location = new_address_location (pc);
+ location = new_address_location (pc, NULL, 0);
back_to = make_cleanup_delete_event_location (location);
create_breakpoint (get_objfile_arch (objfile), location,
NULL /* cond_string */, -1 /* thread */,
--
2.1.4