This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v3 4/9] Explicit locations: introduce address locations
- From: Doug Evans <xdje42 at gmail dot com>
- To: Keith Seitz <keiths at redhat dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Sun, 01 Mar 2015 13:02:13 -0800
- Subject: Re: [PATCH v3 4/9] Explicit locations: introduce address locations
- Authentication-results: sourceware.org; auth=none
- References: <20150217220619 dot 1312 dot 39861 dot stgit at valrhona dot uglyboxes dot com> <20150217220646 dot 1312 dot 56251 dot stgit at valrhona dot uglyboxes dot com>
Keith Seitz <keiths@redhat.com> writes:
> 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): Convert
> linespec to address location.
> (init_breakpoint_sal): Likewise.
> * 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): Handle address locations.
> (delete_event_location): Likewise.
> (event_location_to_string): Likewise.
> (string_to_event_location): Likewise.
> (event_location_empty_p): Likewise.
> * 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.
Hi.
A few comments inline.
grep for ^====
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 5c3be10..ec0c6f8 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -7629,19 +7629,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);
>
> @@ -9614,16 +9609,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 29e32e3..9c36d6c 100644
> --- a/gdb/linespec.c
> +++ b/gdb/linespec.c
> @@ -316,7 +316,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,
> @@ -1768,79 +1768,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. */
> @@ -1994,6 +1984,27 @@ 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);
> +
> + if (self->canonical != NULL)
> + self->canonical->location = new_address_location (address);
====
Modifying self->canonical->location is an undocumented side-effect.
Can you augment the function comment to document and explain this?
> +
> + return sals;
> +}
> +
> /* Create and return SALs from the linespec LS. */
>
> static struct symtabs_and_lines
> @@ -2001,18 +2012,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;
> @@ -2110,8 +2110,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
> @@ -2207,33 +2206,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;
>
> @@ -2457,20 +2430,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, *orig;
> -
> - orig = arg = *stringp;
> - (void) linespec_expression_to_pc (&arg);
> - PARSER_STREAM (&parser) += arg - orig;
> - break;
> - }
> -
> token = linespec_lexer_consume_token (&parser);
>
> /* Keywords are okay after the first token. */
> @@ -2508,6 +2467,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");
> }
> @@ -2693,7 +2658,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 5f5bc71..8055f55 100644
> --- a/gdb/linespec.h
> +++ b/gdb/linespec.h
> @@ -156,4 +156,9 @@ extern struct symtabs_and_lines decode_line_with_last_displayed (char *, int);
> 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 992f21e..39db10e 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 bbb408a..d145dfa 100644
> --- a/gdb/python/py-finishbreakpoint.c
> +++ b/gdb/python/py-finishbreakpoint.c
> @@ -166,9 +166,8 @@ 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;
> + CORE_ADDR pc;
> volatile struct gdb_exception except;
> - char small_buf[100], *p;
> struct symbol *function;
>
> if (!PyArg_ParseTupleAndKeywords (args, kwargs, "|OO", keywords,
> @@ -291,10 +290,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 86af2ae..61dc98d 100644
> --- a/gdb/spu-tdep.c
> +++ b/gdb/spu-tdep.c
> @@ -1957,7 +1957,6 @@ spu_catch_start (struct objfile *objfile)
> CORE_ADDR pc;
> struct event_location *location;
> struct cleanup *back_to;
> - char *p;
>
> /* Do this only if requested by "set spu stop-on-load on". */
> if (!spu_stop_on_load_p)
> @@ -2001,8 +2000,7 @@ spu_catch_start (struct objfile *objfile)
>
> /* Use a numerical address for the set_breakpoint command to avoid having
> the breakpoint re-set incorrectly. */
> - p = ASTRDUP (core_addr_to_string (pc));
> - 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 */,