From patchwork Mon Dec 14 07:11:13 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Joel Brobecker X-Patchwork-Id: 10000 Received: (qmail 101761 invoked by alias); 14 Dec 2015 07:11:27 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 101745 invoked by uid 89); 14 Dec 2015 07:11:26 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.5 required=5.0 tests=AWL, BAYES_00, KAM_LAZY_DOMAIN_SECURITY, RCVD_IN_DNSWL_LOW, T_FILL_THIS_FORM_SHORT autolearn=no version=3.3.2 X-HELO: rock.gnat.com Received: from rock.gnat.com (HELO rock.gnat.com) (205.232.38.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA encrypted) ESMTPS; Mon, 14 Dec 2015 07:11:24 +0000 Received: from localhost (localhost.localdomain [127.0.0.1]) by filtered-rock.gnat.com (Postfix) with ESMTP id E2E6411688D; Mon, 14 Dec 2015 02:11:22 -0500 (EST) Received: from rock.gnat.com ([127.0.0.1]) by localhost (rock.gnat.com [127.0.0.1]) (amavisd-new, port 10024) with LMTP id clrVRDEkT42h; Mon, 14 Dec 2015 02:11:22 -0500 (EST) Received: from joel.gnat.com (localhost.localdomain [127.0.0.1]) by rock.gnat.com (Postfix) with ESMTP id 6A93F116887; Mon, 14 Dec 2015 02:11:22 -0500 (EST) Received: by joel.gnat.com (Postfix, from userid 1000) id 47849405C0; Mon, 14 Dec 2015 11:11:13 +0400 (RET) Date: Mon, 14 Dec 2015 11:11:13 +0400 From: Joel Brobecker To: Keith Seitz Cc: gdb-patches@sourceware.org Subject: Re: [PATCH v6 4/9] Explicit locations: introduce address locations Message-ID: <20151214071113.GA6230@adacore.com> References: <20150805232802.21646.88440.stgit@valrhona.uglyboxes.com> <20150805232951.21646.67733.stgit@valrhona.uglyboxes.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20150805232951.21646.67733.stgit@valrhona.uglyboxes.com> User-Agent: Mutt/1.5.23 (2014-03-12) 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)
: 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! From a39966dcad6f4c32a10a724d03d842cceb3a4533 Mon Sep 17 00:00:00 2001 From: Joel Brobecker Date: Tue, 8 Dec 2015 19:04:56 +0100 Subject: [PATCH] WIP: Fix for "break *'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