Re: [PATCH v6 4/9] Explicit locations: introduce address locations

[Joel Brobecker <brobecker at adacore dot com>]

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!
-- 
Joel

>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(-)

diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
index ce21a4c..9f05dab 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -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;
 }
 
diff --git a/gdb/linespec.c b/gdb/linespec.c
index b2233b9..264ef3a 100644
--- a/gdb/linespec.c
+++ b/gdb/linespec.c
@@ -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:
diff --git a/gdb/location.c b/gdb/location.c
index 626f016..1097a5d 100644
--- a/gdb/location.c
+++ b/gdb/location.c
@@ -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
diff --git a/gdb/location.h b/gdb/location.h
index 932e3ce..2a5e14d 100644
--- a/gdb/location.h
+++ b/gdb/location.h
@@ -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.  */
 
diff --git a/gdb/python/py-finishbreakpoint.c b/gdb/python/py-finishbreakpoint.c
index 45a7d87..541f712 100644
--- a/gdb/python/py-finishbreakpoint.c
+++ b/gdb/python/py-finishbreakpoint.c
@@ -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,
diff --git a/gdb/spu-tdep.c b/gdb/spu-tdep.c
index c94b46e..8029aee 100644
--- a/gdb/spu-tdep.c
+++ b/gdb/spu-tdep.c
@@ -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