* Re: [RFA] Fix regression introduced in "break *<EXPR>" by explicit location patches (was: "Re: [PATCH v6 4/9] Explicit locations: introduce address locations")
@ 2016-01-20 23:39 Doug Evans
2016-01-21 10:40 ` Joel Brobecker
0 siblings, 1 reply; 3+ messages in thread
From: Doug Evans @ 2016-01-20 23:39 UTC (permalink / raw)
To: Joel Brobecker; +Cc: Keith Seitz, gdb-patches
Joel Brobecker writes:
> Hi Keith,
>
> What do you think of the attached patch? There is also a testcase,
> which is slightly different from the scenario that triggered this
> exchange, but which also has the advantage of not requiring PIE,
> which makes the test a little more universal, I think.
>
> gdb/ChangeLog:
>
> * location.h (new_address_location): Add new parameters
> "addr_string" and "addr_string_len".
> (get_address_string_location): Add declaration.
> * location.c (new_address_location): Add new parameters
> "addr_string" and "addr_string_len". If not NULL, store
> a copy of the addr_string in the new location as well.
> (get_address_string_location): New function.
> (string_to_event_location): Update call to new_address_location.
> * linespec.c (event_location_to_sals) <ADDRESS_LOCATION>:
> Save the event location in the parser's state before
> passing it to convert_address_location_to_sals.
> * breakpoint.c (create_thread_event_breakpoint): Update call
> to new_address_location.
> (init_breakpoint_sal): Get the event location's string, if any,
> and use it to update call to new_address_location.
> * python/py-finishbreakpoint.c (bpfinishpy_init):
> Update call to new_address_location.
> * spu-tdep.c (spu_catch_start): Likewise.
>
> * config/djgpp/fnchange.lst: Add entries for
> gdb/testsuite/gdb.base/break-fun-addr1.c and
> gdb/testsuite/gdb.base/break-fun-addr2.c.
>
> gdb/testsuite/ChangeLog:
>
> * gdb.base/break-fun-addr.exp: New file.
> * gdb.base/break-fun-addr1.c: New file.
> * gdb.base/break-fun-addr2.c: New file.
>
> Tested on x86_64-linux.
Hi.
One nit.
>...
> diff --git a/gdb/location.c b/gdb/location.c
> index 37285f3..e43ebf1 100644
> --- a/gdb/location.c
> +++ b/gdb/location.c
>...
> @@ -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. */
> +
Given that the argument must be an address location,
"get_address_location_string" reads better to me.
LGTM with that change.
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [RFA] Fix regression introduced in "break *<EXPR>" by explicit location patches (was: "Re: [PATCH v6 4/9] Explicit locations: introduce address locations") 2016-01-20 23:39 [RFA] Fix regression introduced in "break *<EXPR>" by explicit location patches (was: "Re: [PATCH v6 4/9] Explicit locations: introduce address locations") Doug Evans @ 2016-01-21 10:40 ` Joel Brobecker 0 siblings, 0 replies; 3+ messages in thread From: Joel Brobecker @ 2016-01-21 10:40 UTC (permalink / raw) To: Doug Evans; +Cc: Keith Seitz, gdb-patches [-- Attachment #1: Type: text/plain, Size: 2805 bytes --] > > gdb/ChangeLog: > > > > * location.h (new_address_location): Add new parameters > > "addr_string" and "addr_string_len". > > (get_address_string_location): Add declaration. > > * location.c (new_address_location): Add new parameters > > "addr_string" and "addr_string_len". If not NULL, store > > a copy of the addr_string in the new location as well. > > (get_address_string_location): New function. > > (string_to_event_location): Update call to new_address_location. > > * linespec.c (event_location_to_sals) <ADDRESS_LOCATION>: > > Save the event location in the parser's state before > > passing it to convert_address_location_to_sals. > > * breakpoint.c (create_thread_event_breakpoint): Update call > > to new_address_location. > > (init_breakpoint_sal): Get the event location's string, if any, > > and use it to update call to new_address_location. > > * python/py-finishbreakpoint.c (bpfinishpy_init): > > Update call to new_address_location. > > * spu-tdep.c (spu_catch_start): Likewise. > > > > * config/djgpp/fnchange.lst: Add entries for > > gdb/testsuite/gdb.base/break-fun-addr1.c and > > gdb/testsuite/gdb.base/break-fun-addr2.c. > > > > gdb/testsuite/ChangeLog: > > > > * gdb.base/break-fun-addr.exp: New file. > > * gdb.base/break-fun-addr1.c: New file. > > * gdb.base/break-fun-addr2.c: New file. > > > > Tested on x86_64-linux. > > +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. */ > > + > > Given that the argument must be an address location, > "get_address_location_string" reads better to me. I am not sure what my exact thinking was, at the time, and whether it was flawed or not, but I chose the naming at the time to be consistent with the other functions that looked similar in they functionality. The ideal name for the function would probably be something like "get_address_string_from_location", in other words, what I meant is to return the "address string" (as opposed to the address itself). For now, I pushed the patch as is, but if you think the name should be changed, here is a patch that does that. So, it's ready to go in, if you give the signal. What I would (counter-)propose, on the other hand, is that we look at renaming the function to use "_from_location". Or, maybe have the functions start with "location_" (Eg: location_get_address_string, which actually might be closer to our typical namespace strategy). -- Joel [-- Attachment #2: 0001-rename-get_address_string_location-into-get_address_.patch --] [-- Type: text/x-diff, Size: 2533 bytes --] From 493d020497093787700e1a055a25f8d6471ea416 Mon Sep 17 00:00:00 2001 From: Joel Brobecker <brobecker@adacore.com> Date: Thu, 21 Jan 2016 14:26:33 +0400 Subject: [PATCH] rename get_address_string_location into get_address_location_string The new function name that sounds more correct. gdb/ChangeLog: * location.h, linespec.c, location.c (get_address_location_string): Renames get_address_string_location. --- gdb/ChangeLog | 5 +++++ gdb/linespec.c | 2 +- gdb/location.c | 2 +- gdb/location.h | 2 +- 4 files changed, 8 insertions(+), 3 deletions(-) diff --git a/gdb/ChangeLog b/gdb/ChangeLog index a377a32..598ac9d 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,5 +1,10 @@ 2016-01-21 Joel Brobecker <brobecker@adacore.com> + * location.h, linespec.c, location.c (get_address_location_string): + Renames get_address_string_location. + +2016-01-21 Joel Brobecker <brobecker@adacore.com> + * location.h (new_address_location): Add new parameters "addr_string" and "addr_string_len". (get_address_string_location): Add declaration. diff --git a/gdb/linespec.c b/gdb/linespec.c index 2360cc1..9c8a27a 100644 --- a/gdb/linespec.c +++ b/gdb/linespec.c @@ -2513,7 +2513,7 @@ event_location_to_sals (linespec_parser *parser, case ADDRESS_LOCATION: { - const char *addr_string = get_address_string_location (location); + const char *addr_string = get_address_location_string (location); CORE_ADDR addr = get_address_location (location); if (addr_string != NULL) diff --git a/gdb/location.c b/gdb/location.c index e43ebf1..88ae61d 100644 --- a/gdb/location.c +++ b/gdb/location.c @@ -138,7 +138,7 @@ get_address_location (const struct event_location *location) /* See description in location.h. */ const char * -get_address_string_location (const struct event_location *location) +get_address_location_string (const struct event_location *location) { gdb_assert (EL_TYPE (location) == ADDRESS_LOCATION); return EL_STRING (location); diff --git a/gdb/location.h b/gdb/location.h index b2cf45e..2b04def 100644 --- a/gdb/location.h +++ b/gdb/location.h @@ -145,7 +145,7 @@ extern CORE_ADDR of the given event_location (which must be of type ADDRESS_LOCATION). */ extern const char * - get_address_string_location (const struct event_location *location); + get_address_location_string (const struct event_location *location); /* Create a new probe location. The return result is malloc'd and should be freed with delete_event_location. */ -- 2.5.0 ^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH v6 0/9] Series short description
@ 2015-08-05 23:28 Keith Seitz
2015-08-05 23:30 ` [PATCH v6 4/9] Explicit locations: introduce address locations Keith Seitz
0 siblings, 1 reply; 3+ messages in thread
From: Keith Seitz @ 2015-08-05 23:28 UTC (permalink / raw)
To: gdb-patches
This series is the latest revision of the locations API redesign
which introduces the concept of explicit locations (in addition to
address, linespec, and probe locations).
Several of the patches have already been approved, but I am reposting
the entire (rebased) series.
---
Keith Seitz (9):
Explicit locations: rename "address string"/"addr_string" to "location"
Explicit locations: introduce new struct event_location-based API
Explicit locations: use new location API
Explicit locations: introduce address locations
Explicit locations: introduce probe locations
Explicit locations: introduce explicit locations
Explicit locations: add UI features for CLI
Explicit locations: MI support for explicit locations
Explicit locations: documentation updates
gdb/Makefile.in | 6
gdb/NEWS | 4
gdb/ax-gdb.c | 8
gdb/break-catch-throw.c | 23 +
gdb/breakpoint.c | 788 ++++++++++++++++------------
gdb/breakpoint.h | 51 +-
gdb/cli/cli-cmds.c | 47 +-
gdb/completer.c | 218 +++++++-
gdb/doc/gdb.texinfo | 244 ++++++---
gdb/elfread.c | 4
gdb/guile/scm-breakpoint.c | 23 +
gdb/linespec.c | 582 ++++++++++++---------
gdb/linespec.h | 42 +
gdb/location.c | 727 ++++++++++++++++++++++++++
gdb/location.h | 238 ++++++++
gdb/mi/mi-cmd-break.c | 76 ++-
gdb/probe.c | 20 -
gdb/probe.h | 6
gdb/python/py-breakpoint.c | 12
gdb/python/py-finishbreakpoint.c | 16 -
gdb/python/python.c | 26 -
gdb/remote.c | 10
gdb/spu-tdep.c | 11
gdb/testsuite/gdb.base/dprintf-pending.exp | 10
gdb/testsuite/gdb.base/help.exp | 2
gdb/testsuite/gdb.linespec/3explicit.c | 28 +
gdb/testsuite/gdb.linespec/cpexplicit.cc | 63 ++
gdb/testsuite/gdb.linespec/cpexplicit.exp | 112 ++++
gdb/testsuite/gdb.linespec/explicit.c | 56 ++
gdb/testsuite/gdb.linespec/explicit.exp | 410 +++++++++++++++
gdb/testsuite/gdb.linespec/explicit2.c | 24 +
gdb/testsuite/gdb.linespec/ls-errs.exp | 57 ++
gdb/testsuite/gdb.mi/mi-break.exp | 82 +++
gdb/testsuite/gdb.mi/mi-dprintf.exp | 12
gdb/testsuite/lib/gdb.exp | 6
gdb/testsuite/lib/mi-support.exp | 16 -
gdb/tracepoint.c | 16 -
gdb/tracepoint.h | 2
38 files changed, 3285 insertions(+), 793 deletions(-)
create mode 100644 gdb/location.c
create mode 100644 gdb/location.h
create mode 100644 gdb/testsuite/gdb.linespec/3explicit.c
create mode 100644 gdb/testsuite/gdb.linespec/cpexplicit.cc
create mode 100644 gdb/testsuite/gdb.linespec/cpexplicit.exp
create mode 100644 gdb/testsuite/gdb.linespec/explicit.c
create mode 100644 gdb/testsuite/gdb.linespec/explicit.exp
create mode 100644 gdb/testsuite/gdb.linespec/explicit2.c
^ permalink raw reply [flat|nested] 3+ messages in thread* [PATCH v6 4/9] Explicit locations: introduce address locations 2015-08-05 23:28 [PATCH v6 0/9] Series short description Keith Seitz @ 2015-08-05 23:30 ` Keith Seitz 2015-12-14 7:11 ` Joel Brobecker 0 siblings, 1 reply; 3+ messages in thread From: Keith Seitz @ 2015-08-05 23:30 UTC (permalink / raw) To: gdb-patches *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. --- gdb/breakpoint.c | 18 ---- gdb/linespec.c | 192 +++++++++++++++----------------------- gdb/linespec.h | 5 + gdb/location.c | 61 ++++++++++++ gdb/location.h | 17 +++ gdb/python/py-finishbreakpoint.c | 8 -- gdb/spu-tdep.c | 5 - 7 files changed, 163 insertions(+), 143 deletions(-) diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index af76fdc..06af762 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -7651,19 +7651,14 @@ delete_std_terminate_breakpoint (void) struct breakpoint * create_thread_event_breakpoint (struct gdbarch *gdbarch, CORE_ADDR address) { - char *tmp; struct breakpoint *b; - struct cleanup *cleanup; b = create_internal_breakpoint (gdbarch, address, bp_thread_event, &internal_breakpoint_ops); b->enable_state = bp_enabled; /* location has to be used or breakpoint_re_set will delete me. */ - tmp = xstrprintf ("*%s", paddress (b->loc->gdbarch, b->loc->address)); - cleanup = make_cleanup (xfree, tmp); - b->location = new_linespec_location (&tmp); - do_cleanups (cleanup); + b->location = new_address_location (b->loc->address); update_global_location_list_nothrow (UGLL_MAY_INSERT); @@ -9235,16 +9230,7 @@ init_breakpoint_sal (struct breakpoint *b, struct gdbarch *gdbarch, if (location != NULL) b->location = location; else - { - char *tmp; - struct cleanup *cleanup; - - tmp = xstrprintf ("*%s", - paddress (b->loc->gdbarch, b->loc->address)); - cleanup = make_cleanup (xfree, tmp); - b->location = new_linespec_location (&tmp); - do_cleanups (cleanup); - } + b->location = new_address_location (b->loc->address); b->filter = filter; } diff --git a/gdb/linespec.c b/gdb/linespec.c index 7cebe39..65c55df 100644 --- a/gdb/linespec.c +++ b/gdb/linespec.c @@ -312,7 +312,7 @@ static void iterate_over_file_blocks (struct symtab *symtab, static void initialize_defaults (struct symtab **default_symtab, int *default_line); -static CORE_ADDR linespec_expression_to_pc (const char **exp_ptr); +CORE_ADDR linespec_expression_to_pc (const char **exp_ptr); static struct symtabs_and_lines decode_objc (struct linespec_state *self, linespec_p ls, @@ -1789,79 +1789,69 @@ static void canonicalize_linespec (struct linespec_state *state, const linespec_p ls) { char *tmp; + struct ui_file *buf; + int need_colon = 0; + struct cleanup *cleanup; /* If canonicalization was not requested, no need to do anything. */ if (!state->canonical) return; - /* Shortcut expressions, which can only appear by themselves. */ - if (ls->expression != NULL) + buf = mem_fileopen (); + cleanup = make_cleanup_ui_file_delete (buf); + + if (ls->source_filename) { - tmp = ASTRDUP (ls->expression); - state->canonical->location = new_linespec_location (&tmp); + fputs_unfiltered (ls->source_filename, buf); + need_colon = 1; } - else - { - struct ui_file *buf; - int need_colon = 0; - struct cleanup *cleanup; - buf = mem_fileopen (); - cleanup = make_cleanup_ui_file_delete (buf); + if (ls->function_name) + { + if (need_colon) + fputc_unfiltered (':', buf); + fputs_unfiltered (ls->function_name, buf); + need_colon = 1; + } - if (ls->source_filename) - { - fputs_unfiltered (ls->source_filename, buf); - need_colon = 1; - } + if (ls->label_name) + { + if (need_colon) + fputc_unfiltered (':', buf); - if (ls->function_name) + if (ls->function_name == NULL) { - if (need_colon) - fputc_unfiltered (':', buf); - fputs_unfiltered (ls->function_name, buf); - need_colon = 1; + struct symbol *s; + + /* No function was specified, so add the symbol name. */ + gdb_assert (ls->labels.function_symbols != NULL + && (VEC_length (symbolp, ls->labels.function_symbols) + == 1)); + s = VEC_index (symbolp, ls->labels.function_symbols, 0); + fputs_unfiltered (SYMBOL_NATURAL_NAME (s), buf); + fputc_unfiltered (':', buf); } - if (ls->label_name) - { - if (need_colon) - fputc_unfiltered (':', buf); - - if (ls->function_name == NULL) - { - struct symbol *s; - - /* No function was specified, so add the symbol name. */ - gdb_assert (ls->labels.function_symbols != NULL - && (VEC_length (symbolp, ls->labels.function_symbols) - == 1)); - s = VEC_index (symbolp, ls->labels.function_symbols, 0); - fputs_unfiltered (SYMBOL_NATURAL_NAME (s), buf); - fputc_unfiltered (':', buf); - } - - fputs_unfiltered (ls->label_name, buf); - need_colon = 1; - state->canonical->special_display = 1; - } - - if (ls->line_offset.sign != LINE_OFFSET_UNKNOWN) - { - if (need_colon) - fputc_unfiltered (':', buf); - fprintf_filtered (buf, "%s%d", - (ls->line_offset.sign == LINE_OFFSET_NONE ? "" - : (ls->line_offset.sign - == LINE_OFFSET_PLUS ? "+" : "-")), - ls->line_offset.offset); - } + fputs_unfiltered (ls->label_name, buf); + need_colon = 1; + state->canonical->special_display = 1; + } - tmp = ui_file_xstrdup (buf, NULL); - make_cleanup (xfree, tmp); - state->canonical->location = new_linespec_location (&tmp); - do_cleanups (cleanup); + if (ls->line_offset.sign != LINE_OFFSET_UNKNOWN) + { + if (need_colon) + fputc_unfiltered (':', buf); + fprintf_filtered (buf, "%s%d", + (ls->line_offset.sign == LINE_OFFSET_NONE ? "" + : (ls->line_offset.sign + == LINE_OFFSET_PLUS ? "+" : "-")), + ls->line_offset.offset); } + + tmp = ui_file_xstrdup (buf, NULL); + make_cleanup (xfree, tmp); + state->canonical->location = new_linespec_location (&tmp); + do_cleanups (cleanup); } /* Given a line offset in LS, construct the relevant SALs. */ @@ -2015,6 +2005,24 @@ create_sals_line_offset (struct linespec_state *self, return values; } +/* Convert the given ADDRESS into SaLs. */ + +static struct symtabs_and_lines +convert_address_location_to_sals (struct linespec_state *self, + CORE_ADDR address) +{ + struct symtab_and_line sal; + struct symtabs_and_lines sals = {NULL, 0}; + + sal = find_pc_line (address, 0); + sal.pc = address; + sal.section = find_pc_overlay (address); + sal.explicit_pc = 1; + add_sal_to_sals (self, &sals, &sal, core_addr_to_string (address), 1); + + return sals; +} + /* Create and return SALs from the linespec LS. */ static struct symtabs_and_lines @@ -2022,18 +2030,7 @@ convert_linespec_to_sals (struct linespec_state *state, linespec_p ls) { struct symtabs_and_lines sals = {NULL, 0}; - if (ls->expression != NULL) - { - struct symtab_and_line sal; - - /* We have an expression. No other attribute is allowed. */ - sal = find_pc_line (ls->expr_pc, 0); - sal.pc = ls->expr_pc; - sal.section = find_pc_overlay (ls->expr_pc); - sal.explicit_pc = 1; - add_sal_to_sals (state, &sals, &sal, ls->expression, 1); - } - else if (ls->labels.label_symbols != NULL) + if (ls->labels.label_symbols != NULL) { /* We have just a bunch of functions/methods or labels. */ int i; @@ -2131,8 +2128,7 @@ convert_linespec_to_sals (struct linespec_state *state, linespec_p ls) The basic grammar of linespecs: - linespec -> expr_spec | var_spec | basic_spec - expr_spec -> '*' STRING + linespec -> var_spec | basic_spec var_spec -> '$' (STRING | NUMBER) basic_spec -> file_offset_spec | function_spec | label_spec @@ -2223,33 +2219,7 @@ parse_linespec (linespec_parser *parser, const char *arg) token = linespec_lexer_lex_one (parser); /* It must be either LSTOKEN_STRING or LSTOKEN_NUMBER. */ - if (token.type == LSTOKEN_STRING && *LS_TOKEN_STOKEN (token).ptr == '*') - { - char *expr; - const char *copy; - - /* User specified an expression, *EXPR. */ - copy = expr = copy_token_string (token); - cleanup = make_cleanup (xfree, expr); - PARSER_RESULT (parser)->expr_pc = linespec_expression_to_pc (©); - discard_cleanups (cleanup); - PARSER_RESULT (parser)->expression = expr; - - /* This is a little hacky/tricky. If linespec_expression_to_pc - did not evaluate the entire token, then we must find the - string COPY inside the original token buffer. */ - if (*copy != '\0') - { - PARSER_STREAM (parser) = strstr (parser->lexer.saved_arg, copy); - gdb_assert (PARSER_STREAM (parser) != NULL); - } - - /* Consume the token. */ - linespec_lexer_consume_token (parser); - - goto convert_to_sals; - } - else if (token.type == LSTOKEN_STRING && *LS_TOKEN_STOKEN (token).ptr == '$') + if (token.type == LSTOKEN_STRING && *LS_TOKEN_STOKEN (token).ptr == '$') { char *var; @@ -2470,20 +2440,6 @@ linespec_lex_to_end (char **stringp) token = linespec_lexer_peek_token (&parser); if (token.type == LSTOKEN_COMMA) break; - - /* For addresses advance the parser stream past - any parsed input and stop lexing. */ - if (token.type == LSTOKEN_STRING - && *LS_TOKEN_STOKEN (token).ptr == '*') - { - const char *arg; - - arg = *stringp; - (void) linespec_expression_to_pc (&arg); - PARSER_STREAM (&parser) = arg; - break; - } - token = linespec_lexer_consume_token (&parser); } while (token.type != LSTOKEN_EOI && token.type != LSTOKEN_KEYWORD); @@ -2517,6 +2473,12 @@ event_location_to_sals (linespec_parser *parser, } break; + case ADDRESS_LOCATION: + result + = convert_address_location_to_sals (PARSER_STATE (parser), + get_address_location (location)); + break; + default: gdb_assert_not_reached ("unhandled event location type"); } @@ -2702,7 +2664,7 @@ initialize_defaults (struct symtab **default_symtab, int *default_line) /* Evaluate the expression pointed to by EXP_PTR into a CORE_ADDR, advancing EXP_PTR past any parsed text. */ -static CORE_ADDR +CORE_ADDR linespec_expression_to_pc (const char **exp_ptr) { if (current_program_space->executing_startup) diff --git a/gdb/linespec.h b/gdb/linespec.h index 840bae5..391ed26 100644 --- a/gdb/linespec.h +++ b/gdb/linespec.h @@ -161,4 +161,9 @@ extern const char *linespec_lexer_lex_keyword (const char *p); STRINGP will be advanced to this point. */ extern void linespec_lex_to_end (char **stringp); + +/* Evaluate the expression pointed to by EXP_PTR into a CORE_ADDR, + advancing EXP_PTR past any parsed text. */ + +extern CORE_ADDR linespec_expression_to_pc (const char **exp_ptr); #endif /* defined (LINESPEC_H) */ diff --git a/gdb/location.c b/gdb/location.c index 39e09c1..c1f4e19 100644 --- a/gdb/location.c +++ b/gdb/location.c @@ -45,6 +45,10 @@ struct event_location probes. */ char *addr_string; #define EL_LINESPEC(PTR) ((PTR)->u.addr_string) + + /* An address in the inferior. */ + CORE_ADDR address; +#define EL_ADDRESS(PTR) (PTR)->u.address } u; /* Cached string representation of this location. This is used, e.g., to @@ -95,6 +99,28 @@ get_linespec_location (const struct event_location *location) /* See description in location.h. */ struct event_location * +new_address_location (CORE_ADDR addr) +{ + struct event_location *location; + + location = XCNEW (struct event_location); + EL_TYPE (location) = ADDRESS_LOCATION; + EL_ADDRESS (location) = addr; + return location; +} + +/* See description in location.h. */ + +CORE_ADDR +get_address_location (const struct event_location *location) +{ + gdb_assert (EL_TYPE (location) == ADDRESS_LOCATION); + return EL_ADDRESS (location); +} + +/* See description in location.h. */ + +struct event_location * copy_event_location (const struct event_location *src) { struct event_location *dst; @@ -111,6 +137,10 @@ copy_event_location (const struct event_location *src) EL_LINESPEC (dst) = xstrdup (EL_LINESPEC (src)); break; + case ADDRESS_LOCATION: + EL_ADDRESS (dst) = EL_ADDRESS (src); + break; + default: gdb_assert_not_reached ("unknown event location type"); } @@ -151,6 +181,10 @@ delete_event_location (struct event_location *location) xfree (EL_LINESPEC (location)); break; + case ADDRESS_LOCATION: + /* Nothing to do. */ + break; + default: gdb_assert_not_reached ("unknown event location type"); } @@ -176,6 +210,12 @@ event_location_to_string_const (const struct event_location *location) result = xstrdup (EL_LINESPEC (location)); break; + case ADDRESS_LOCATION: + result + = xstrprintf ("*%s", + core_addr_to_string (EL_ADDRESS (location))); + break; + default: gdb_assert_not_reached ("unknown event location type"); } @@ -203,7 +243,23 @@ string_to_event_location (char **stringp, { struct event_location *location; - location = new_linespec_location (stringp); + /* First, check if the string is an 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); + *stringp += arg - orig; + } + else + { + /* Everything else is a linespec. */ + location = new_linespec_location (stringp); + } + return location; } @@ -218,6 +274,9 @@ event_location_empty_p (const struct event_location *location) /* Linespecs are never "empty." (NULL is a valid linespec) */ return 0; + case ADDRESS_LOCATION: + return 0; + default: gdb_assert_not_reached ("unknown event location type"); } diff --git a/gdb/location.h b/gdb/location.h index 0e31c0a..5fd1dca 100644 --- a/gdb/location.h +++ b/gdb/location.h @@ -28,7 +28,10 @@ struct event_location; enum event_location_type { /* A traditional linespec. */ - LINESPEC_LOCATION + LINESPEC_LOCATION, + + /* An address in the inferior. */ + ADDRESS_LOCATION }; /* Return the type of the given event location. */ @@ -64,6 +67,18 @@ extern struct event_location * extern const char * get_linespec_location (const struct event_location *location); +/* Create a new address location. The return result is malloc'd + and should be freed with delete_event_location. */ + +extern struct event_location * + new_address_location (CORE_ADDR addr); + +/* Return the address location (a CORE_ADDR) of the given event_location + (which must be of type ADDRESS_LOCATION). */ + +extern CORE_ADDR + get_address_location (const struct event_location *location); + /* Free an event location and any associated data. */ extern void delete_event_location (struct event_location *location); diff --git a/gdb/python/py-finishbreakpoint.c b/gdb/python/py-finishbreakpoint.c index 671fd23..e543bb3 100644 --- a/gdb/python/py-finishbreakpoint.c +++ b/gdb/python/py-finishbreakpoint.c @@ -169,8 +169,7 @@ bpfinishpy_init (PyObject *self, PyObject *args, PyObject *kwargs) struct frame_id frame_id; PyObject *internal = NULL; int internal_bp = 0; - CORE_ADDR finish_pc, pc; - char small_buf[100], *p; + CORE_ADDR pc; struct symbol *function; if (!PyArg_ParseTupleAndKeywords (args, kwargs, "|OO", keywords, @@ -301,10 +300,7 @@ bpfinishpy_init (PyObject *self, PyObject *args, PyObject *kwargs) struct cleanup *back_to; /* Set a breakpoint on the return address. */ - finish_pc = get_frame_pc (prev_frame); - xsnprintf (small_buf, sizeof (small_buf), "*%s", hex_string (finish_pc)); - p = small_buf; - location = new_linespec_location (&p); + location = new_address_location (get_frame_pc (prev_frame)); 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 58d6671..75372df 100644 --- a/gdb/spu-tdep.c +++ b/gdb/spu-tdep.c @@ -1953,7 +1953,6 @@ spu_catch_start (struct objfile *objfile) { struct bound_minimal_symbol minsym; struct compunit_symtab *cust; - char buf[32], *p; CORE_ADDR pc; struct event_location *location; struct cleanup *back_to; @@ -2000,9 +1999,7 @@ spu_catch_start (struct objfile *objfile) /* Use a numerical address for the set_breakpoint command to avoid having the breakpoint re-set incorrectly. */ - xsnprintf (buf, sizeof buf, "*%s", core_addr_to_string (pc)); - p = buf; - location = new_linespec_location (&p); + location = new_address_location (pc); back_to = make_cleanup_delete_event_location (location); create_breakpoint (get_objfile_arch (objfile), location, NULL /* cond_string */, -1 /* thread */, ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v6 4/9] Explicit locations: introduce address locations 2015-08-05 23:30 ` [PATCH v6 4/9] Explicit locations: introduce address locations Keith Seitz @ 2015-12-14 7:11 ` Joel Brobecker 2015-12-14 20:56 ` Keith Seitz 0 siblings, 1 reply; 3+ messages in thread From: Joel Brobecker @ 2015-12-14 7:11 UTC (permalink / raw) To: Keith Seitz; +Cc: gdb-patches [-- Attachment #1: Type: text/plain, Size: 4264 bytes --] 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 [-- Attachment #2: 0001-WIP-Fix-for-break-func_name-address-before-running-P.patch --] [-- Type: text/x-diff, Size: 6406 bytes --] 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v6 4/9] Explicit locations: introduce address locations 2015-12-14 7:11 ` Joel Brobecker @ 2015-12-14 20:56 ` Keith Seitz 2015-12-15 13:40 ` Joel Brobecker 0 siblings, 1 reply; 3+ messages in thread From: Keith Seitz @ 2015-12-14 20:56 UTC (permalink / raw) To: Joel Brobecker; +Cc: gdb-patches 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v6 4/9] Explicit locations: introduce address locations 2015-12-14 20:56 ` Keith Seitz @ 2015-12-15 13:40 ` Joel Brobecker 2016-01-17 15:32 ` [RFA] Fix regression introduced in "break *<EXPR>" by explicit location patches (was: "Re: [PATCH v6 4/9] Explicit locations: introduce address locations") Joel Brobecker 0 siblings, 1 reply; 3+ messages in thread From: Joel Brobecker @ 2015-12-15 13:40 UTC (permalink / raw) To: Keith Seitz; +Cc: gdb-patches > 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! -- Joel ^ permalink raw reply [flat|nested] 3+ messages in thread
* [RFA] Fix regression introduced in "break *<EXPR>" by explicit location patches (was: "Re: [PATCH v6 4/9] Explicit locations: introduce address locations") 2015-12-15 13:40 ` Joel Brobecker @ 2016-01-17 15:32 ` Joel Brobecker 0 siblings, 0 replies; 3+ messages in thread From: Joel Brobecker @ 2016-01-17 15:32 UTC (permalink / raw) To: Keith Seitz; +Cc: gdb-patches [-- Attachment #1: Type: text/plain, Size: 1705 bytes --] Hi Keith, What do you think of the attached patch? There is also a testcase, which is slightly different from the scenario that triggered this exchange, but which also has the advantage of not requiring PIE, which makes the test a little more universal, I think. gdb/ChangeLog: * location.h (new_address_location): Add new parameters "addr_string" and "addr_string_len". (get_address_string_location): Add declaration. * location.c (new_address_location): Add new parameters "addr_string" and "addr_string_len". If not NULL, store a copy of the addr_string in the new location as well. (get_address_string_location): New function. (string_to_event_location): Update call to new_address_location. * linespec.c (event_location_to_sals) <ADDRESS_LOCATION>: Save the event location in the parser's state before passing it to convert_address_location_to_sals. * breakpoint.c (create_thread_event_breakpoint): Update call to new_address_location. (init_breakpoint_sal): Get the event location's string, if any, and use it to update call to new_address_location. * python/py-finishbreakpoint.c (bpfinishpy_init): Update call to new_address_location. * spu-tdep.c (spu_catch_start): Likewise. * config/djgpp/fnchange.lst: Add entries for gdb/testsuite/gdb.base/break-fun-addr1.c and gdb/testsuite/gdb.base/break-fun-addr2.c. gdb/testsuite/ChangeLog: * gdb.base/break-fun-addr.exp: New file. * gdb.base/break-fun-addr1.c: New file. * gdb.base/break-fun-addr2.c: New file. Tested on x86_64-linux. Thanks! -- Joel [-- Attachment #2: 0001-Fix-regression-introduced-in-break-EXPR-by-explicit-.patch --] [-- Type: text/x-diff, Size: 16498 bytes --] From 56921ae04d132975387a2b40d376c55fde37bfd3 Mon Sep 17 00:00:00 2001 From: Joel Brobecker <brobecker@adacore.com> Date: Tue, 8 Dec 2015 19:04:56 +0100 Subject: [PATCH] Fix regression introduced in "break *<EXPR>" by explicit location patches. A relatively recent patch support for explicit locations, and part of that patch cleaned up the way we parse breakpoint locations. Unfortunatly, a small regression crept in for "*<EXPR>" breakpoint locations. In particular, on PIE programs, one can see the issue by doing the following, with any program: (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 Just for the record, this regression was introduced by: commit a06efdd6effd149a1d392df8d62824e44872003a Date: Tue Aug 11 17:09:35 2015 -0700 Subject: Explicit locations: introduce address locations 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. This patch plugs that hole simply by saving the original expression used to compute the address as part of the address location, so as to then re-evaluate that expression during breakpoint re-set. gdb/ChangeLog: * location.h (new_address_location): Add new parameters "addr_string" and "addr_string_len". (get_address_string_location): Add declaration. * location.c (new_address_location): Add new parameters "addr_string" and "addr_string_len". If not NULL, store a copy of the addr_string in the new location as well. (get_address_string_location): New function. (string_to_event_location): Update call to new_address_location. * linespec.c (event_location_to_sals) <ADDRESS_LOCATION>: Save the event location in the parser's state before passing it to convert_address_location_to_sals. * breakpoint.c (create_thread_event_breakpoint): Update call to new_address_location. (init_breakpoint_sal): Get the event location's string, if any, and use it to update call to new_address_location. * python/py-finishbreakpoint.c (bpfinishpy_init): Update call to new_address_location. * spu-tdep.c (spu_catch_start): Likewise. * config/djgpp/fnchange.lst: Add entries for gdb/testsuite/gdb.base/break-fun-addr1.c and gdb/testsuite/gdb.base/break-fun-addr2.c. gdb/testsuite/ChangeLog: * gdb.base/break-fun-addr.exp: New file. * gdb.base/break-fun-addr1.c: New file. * gdb.base/break-fun-addr2.c: New file. --- gdb/breakpoint.c | 15 +++++- gdb/config/djgpp/fnchange.lst | 2 + gdb/linespec.c | 24 +++++++-- gdb/location.c | 16 +++++- gdb/location.h | 15 ++++-- gdb/python/py-finishbreakpoint.c | 2 +- gdb/spu-tdep.c | 2 +- gdb/testsuite/gdb.base/break-fun-addr.exp | 84 +++++++++++++++++++++++++++++++ gdb/testsuite/gdb.base/break-fun-addr1.c | 22 ++++++++ gdb/testsuite/gdb.base/break-fun-addr2.c | 28 +++++++++++ 10 files changed, 198 insertions(+), 12 deletions(-) create mode 100644 gdb/testsuite/gdb.base/break-fun-addr.exp create mode 100644 gdb/testsuite/gdb.base/break-fun-addr1.c create mode 100644 gdb/testsuite/gdb.base/break-fun-addr2.c diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c index 72da4ef..1c637a7 100644 --- a/gdb/breakpoint.c +++ b/gdb/breakpoint.c @@ -7766,7 +7766,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); @@ -9333,7 +9333,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/config/djgpp/fnchange.lst b/gdb/config/djgpp/fnchange.lst index 5495308..21a8071 100644 --- a/gdb/config/djgpp/fnchange.lst +++ b/gdb/config/djgpp/fnchange.lst @@ -407,6 +407,8 @@ @V@/gdb/testsuite/gdb.base/bitfields2.c @V@/gdb/testsuite/gdb.base/bitfiel2.c @V@/gdb/testsuite/gdb.base/bitfields2.exp @V@/gdb/testsuite/gdb.base/bitfiel2.exp @V@/gdb/testsuite/gdb.base/break-entry.exp @V@/gdb/testsuite/gdb.base/brkentry.exp +@V@/gdb/testsuite/gdb.base/break-fun-addr1.c @V@/gdb/testsuite/gdb.base/b-f-a1.c +@V@/gdb/testsuite/gdb.base/break-fun-addr2.c @V@/gdb/testsuite/gdb.base/b-f-a2.c @V@/gdb/testsuite/gdb.base/coremaker2.c @V@/gdb/testsuite/gdb.base/core2maker.c @V@/gdb/testsuite/gdb.base/hashline1.exp @V@/gdb/testsuite/gdb.base/hash1line.exp @V@/gdb/testsuite/gdb.base/hashline2.exp @V@/gdb/testsuite/gdb.base/hash2line.exp diff --git a/gdb/linespec.c b/gdb/linespec.c index 50951bd..4c7dab3 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 37285f3..e43ebf1 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 bc53884..b2cf45e 100644 --- a/gdb/location.h +++ b/gdb/location.h @@ -126,11 +126,14 @@ extern struct event_location * extern const char * get_linespec_location (const struct event_location *location); -/* Create a new address location. The return result is malloc'd - and should be freed with delete_event_location. */ +/* Create a new address location. + ADDR is the address corresponding to this event_location. + ADDR_STRING, a string of ADDR_STRING_LEN characters, is + the expression that was parsed to determine the address ADDR. */ 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 +141,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 bff6dba..a0513b5 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 95ca8ab..bf3b289 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 */, diff --git a/gdb/testsuite/gdb.base/break-fun-addr.exp b/gdb/testsuite/gdb.base/break-fun-addr.exp new file mode 100644 index 0000000..e8bed3f --- /dev/null +++ b/gdb/testsuite/gdb.base/break-fun-addr.exp @@ -0,0 +1,84 @@ +# Copyright 2016 Free Software Foundation, Inc. + +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation; either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see <http://www.gnu.org/licenses/>. + +# The purpose of this testcase is to verify that, when using a breakpoint +# location of the form "*<EXPR>" (Eg: "*main"), GDB is able to start +# the program and stop at the correct location. With programs built +# as PIE, this means that GDB needs to re-evaluate the location once +# the program as started, since PIE ensures that the address of all +# symbols have changed after load. +# +# PIE is not always supported by the target system, so instead of +# creating a testcase building executables with PIE, this testcase +# takes a slightly different approach. It builds a first program, +# breaks on *main, and then runs to that breakpoint. It then builds +# a second program, different from the first one, and loads that +# executable within the same GDB session. Similarly to the PIE case, +# the address of main should be different, and therefore GDB should +# recalculate it. We verify that by checking that running to that +# breakpoint still works, and that we land at the first instruction +# of that function in both cases. + +set testfile1 "break-fun-addr1" +set srcfile1 ${testfile1}.c +set binfile1 [standard_output_file ${testfile1}] + +if { [gdb_compile "${srcdir}/${subdir}/${srcfile1}" "${binfile1}" executable {debug}] != "" } { + untested "Couldn't compile ${srcfile1}" + return -1 +} + +# Start the debugger with the first executable, put a breakpoint +# on the first instruction of function "main" ("*main"), then +# run to that breakpoint. + +clean_restart ${binfile1} + +with_test_prefix "${binfile1}" { + + gdb_test "break *main" \ + "Breakpoint.*at.* file .*$srcfile1, line .*" \ + + gdb_run_cmd + gdb_test "" \ + "Breakpoint.* main \\(\\) at .*$srcfile1:.*" \ + "run to breakpoint at *main" + + # Verify also that we stopped at the start of the function... + gdb_test "p \$pc == main" " = 1" +} + +set testfile2 "break-fun-addr2" +set srcfile2 ${testfile2}.c +set binfile2 [standard_output_file ${testfile2}] + +if { [gdb_compile "${srcdir}/${subdir}/${srcfile2}" "${binfile2}" executable {debug}] != "" } { + untested "Couldn't compile ${srcfile2}" + return -1 +} + +# Now, keeping the same GDB process (so as to keep the same breakpoint), +# start a new debugging session with a different executable. +gdb_load ${binfile2} + +with_test_prefix "${binfile2}" { + + gdb_run_cmd + gdb_test "" \ + "Breakpoint.* main \\(\\) at .*$srcfile2:.*" \ + "run to breakpoint at *main" + + gdb_test "p \$pc == main" " = 1" +} diff --git a/gdb/testsuite/gdb.base/break-fun-addr1.c b/gdb/testsuite/gdb.base/break-fun-addr1.c new file mode 100644 index 0000000..1545b21 --- /dev/null +++ b/gdb/testsuite/gdb.base/break-fun-addr1.c @@ -0,0 +1,22 @@ +/* This testcase is part of GDB, the GNU debugger. + + Copyright 2016 Free Software Foundation, Inc. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. */ + +int +main (void) +{ + return 0; +} diff --git a/gdb/testsuite/gdb.base/break-fun-addr2.c b/gdb/testsuite/gdb.base/break-fun-addr2.c new file mode 100644 index 0000000..13eec05 --- /dev/null +++ b/gdb/testsuite/gdb.base/break-fun-addr2.c @@ -0,0 +1,28 @@ +/* This testcase is part of GDB, the GNU debugger. + + Copyright 2016 Free Software Foundation, Inc. + + This program is free software; you can redistribute it and/or modify + it under the terms of the GNU General Public License as published by + the Free Software Foundation; either version 3 of the License, or + (at your option) any later version. + + This program is distributed in the hope that it will be useful, + but WITHOUT ANY WARRANTY; without even the implied warranty of + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + GNU General Public License for more details. + + You should have received a copy of the GNU General Public License + along with this program. If not, see <http://www.gnu.org/licenses/>. */ + +int +compute_something (int i) +{ + return i - 1; +} + +int +main (void) +{ + return compute_something (1); +} -- 2.5.0 ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2016-01-21 10:40 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-01-20 23:39 [RFA] Fix regression introduced in "break *<EXPR>" by explicit location patches (was: "Re: [PATCH v6 4/9] Explicit locations: introduce address locations") Doug Evans 2016-01-21 10:40 ` Joel Brobecker -- strict thread matches above, loose matches on Subject: below -- 2015-08-05 23:28 [PATCH v6 0/9] Series short description Keith Seitz 2015-08-05 23:30 ` [PATCH v6 4/9] Explicit locations: introduce address locations Keith Seitz 2015-12-14 7:11 ` Joel Brobecker 2015-12-14 20:56 ` Keith Seitz 2015-12-15 13:40 ` Joel Brobecker 2016-01-17 15:32 ` [RFA] Fix regression introduced in "break *<EXPR>" by explicit location patches (was: "Re: [PATCH v6 4/9] Explicit locations: introduce address locations") Joel Brobecker
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).