This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [RFA 3/5] Explicit linespecs - implementation
- From: Jan Kratochvil <jan dot kratochvil at redhat dot com>
- To: Keith Seitz <keiths at redhat dot com>
- Cc: "gdb-patches at sourceware dot org ml" <gdb-patches at sourceware dot org>
- Date: Mon, 30 Jul 2012 17:58:24 +0200
- Subject: Re: [RFA 3/5] Explicit linespecs - implementation
- References: <50120F6C.2000308@redhat.com>
On Fri, 27 Jul 2012 05:47:56 +0200, Keith Seitz wrote:
> Questions/comments/concerns?
This adds yet another way how to specify linespecs. Have you considered
removing the b->addr_string field completely? What are the reasons for
conditional, thread, task not being already parsed into struct
explicit_linespec? For example probes could also be in explicit_linespec.
I would see explicit_linespec as enum + union. "expression" would be already
one enum type, classical linespec other type, probe yet another type, maybe
some more.
This way explicit_linespec would be always non-NULL and addr_string would be no
longer passed along. Currently all the use of both explicit_linespec and
addr_string together seem error prone to me.
Thanks,
Jan
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index 35d55ba..1058a35 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -3486,7 +3486,7 @@ update_breakpoints_after_exec (void)
> /* Without a symbolic address, we have little hope of the
> pre-exec() address meaning the same thing in the post-exec()
> a.out. */
> - if (b->addr_string == NULL)
> + if (b->addr_string == NULL && b->explicit == NULL)
> {
> delete_breakpoint (b);
> continue;
> @@ -5688,7 +5688,19 @@ print_breakpoint_location (struct breakpoint *b,
> do_cleanups (stb_chain);
> }
> else
> - ui_out_field_string (uiout, "pending", b->addr_string);
> + {
> + char *where = b->addr_string;
const char *
> + struct cleanup *free_where = make_cleanup (null_cleanup, NULL);
> +
> + if (b->explicit != NULL)
> + {
> + where = explicit_linespec_to_string (b->explicit, b->addr_string);
> + make_cleanup (xfree, where);
Here you will need to use a different variable.
In fact it would be easier then to just call ui_out_field_string two times.
> + }
> +
> + ui_out_field_string (uiout, "pending", where);
> + do_cleanups (free_where);
> + }
>
> if (loc && is_breakpoint (b)
> && breakpoint_condition_evaluation_mode () == condition_evaluation_target
> @@ -5757,6 +5769,14 @@ bptype_string (enum bptype type)
> return bptypes[(int) type].description;
> }
>
> +/* Throw an exception for unsupported explicit linespec. */
> +
> +static void ATTRIBUTE_NORETURN
> +explicit_linespec_unsupported (enum bptype type)
> +{
> + error (_("explicit linespec not supported for %s"), bptype_string (type));
Error should be a sentence, capital 'E', "is not".
> +}
> +
> /* Print B to gdb_stdout. */
>
> static void
> @@ -9009,6 +9029,8 @@ init_breakpoint_sal (struct breakpoint *b, struct gdbarch *gdbarch,
> me. */
> b->addr_string
> = xstrprintf ("*%s", paddress (b->loc->gdbarch, b->loc->address));
> + if (els != NULL)
> + b->explicit = copy_explicit_linespec (els);
> b->filter = filter;
> }
>
> @@ -9040,7 +9062,7 @@ create_breakpoint_sal (struct gdbarch *gdbarch,
> old_chain = make_cleanup (xfree, b);
>
> init_breakpoint_sal (b, gdbarch,
> - sals, NULL, addr_string,
> + sals, els, addr_string,
> filter, cond_string, extra_string,
> type, disposition,
> thread, task, ignore_count,
> @@ -9094,7 +9116,7 @@ create_breakpoints_sal (struct gdbarch *gdbarch,
>
> make_cleanup (xfree, filter_string);
> create_breakpoint_sal (gdbarch, lsal->sals,
> - NULL, addr_string,
> + canonical->explicit, addr_string,
> filter_string,
> cond_string, extra_string,
> type, disposition,
> @@ -9121,8 +9143,9 @@ parse_breakpoint_sals (explicit_linespec *els, char **address,
>
> /* If no arg given, or if first arg is 'if ', use the default
> breakpoint. */
> - if ((*address) == NULL
> - || (strncmp ((*address), "if", 2) == 0 && isspace ((*address)[2])))
> + if (els == NULL
> + && ((*address) == NULL
> + || (strncmp ((*address), "if", 2) == 0 && isspace ((*address)[2]))))
> {
> /* The last displayed codepoint, if it's valid, is our default breakpoint
> address. */
> @@ -9167,17 +9190,33 @@ parse_breakpoint_sals (explicit_linespec *els, char **address,
>
> ObjC: However, don't match an Objective-C method name which
> may have a '+' or '-' succeeded by a '['. */
> - if (last_displayed_sal_is_valid ()
> - && (!cursal.symtab
> - || ((strchr ("+-", (*address)[0]) != NULL)
> - && ((*address)[1] != '['))))
> - decode_line_full (address, DECODE_LINE_FUNFIRSTLINE,
> - get_last_displayed_symtab (),
> - get_last_displayed_line (),
> - canonical, NULL, NULL);
> + if (els != NULL)
> + {
> + if (last_displayed_sal_is_valid ())
> + decode_explicit_linespec (els, DECODE_LINE_FUNFIRSTLINE,
> + get_last_displayed_symtab (),
> + get_last_displayed_line (),
> + canonical, NULL, NULL);
> + else
> + decode_explicit_linespec (els, DECODE_LINE_FUNFIRSTLINE,
> + NULL, 0, canonical,
> + NULL, NULL);
> + }
> else
> - decode_line_full (address, DECODE_LINE_FUNFIRSTLINE,
> - cursal.symtab, cursal.line, canonical, NULL, NULL);
> + {
> + if (last_displayed_sal_is_valid ()
> + && (!cursal.symtab
> + || ((strchr ("+-", (*address)[0]) != NULL)
> + && ((*address)[1] != '['))))
> + decode_line_full (address, DECODE_LINE_FUNFIRSTLINE,
> + get_last_displayed_symtab (),
> + get_last_displayed_line (),
> + canonical, NULL, NULL);
> + else
> + decode_line_full (address, DECODE_LINE_FUNFIRSTLINE,
> + cursal.symtab, cursal.line, canonical,
> + NULL, NULL);
> + }
> }
> }
>
> @@ -9536,6 +9575,8 @@ create_breakpoint_1 (struct gdbarch *gdbarch,
>
> init_raw_breakpoint_without_location (b, gdbarch, type_wanted, ops);
>
> + if (canonical->explicit != NULL)
> + b->explicit = copy_explicit_linespec (canonical->explicit);
> if (canonical->addr_string)
> b->addr_string = xstrdup (canonical->addr_string);
> if (parse_condition_and_thread)
> @@ -9636,10 +9677,12 @@ create_breakpoint (struct gdbarch *gdbarch, explicit_linespec *els,
>
> back_to = make_cleanup_destroy_linespec_result (&canonical);
>
> - if (canonical.addr_string == NULL)
> + if (canonical.explicit == NULL)
> {
> if (addr_start)
> canonical.addr_string = xstrdup (addr_start);
I do not understand it much, using ADDR_START without ELS no longer makes
sense? Parameter ELS of create_breakpoint is undocumented.
> + if (els != NULL)
> + canonical.explicit = copy_explicit_linespec (els);
> }
>
> result = create_breakpoint_1 (gdbarch, &canonical, arg, cond_string,
> @@ -9666,7 +9709,9 @@ break_command_1 (char *arg, int flag, int from_tty)
> enum bptype type_wanted = (flag & BP_HARDWAREFLAG
> ? bp_hardware_breakpoint
> : bp_breakpoint);
> + explicit_linespec *els;
> struct breakpoint_ops *ops;
> + struct cleanup *back_to;
> const char *arg_cp = arg;
>
> /* Matching breakpoints on probes. */
> @@ -9675,8 +9720,11 @@ break_command_1 (char *arg, int flag, int from_tty)
> else
> ops = &bkpt_breakpoint_ops;
>
> + /* Check for an explicit linespec. */
> + els = string_to_explicit_linespec (&arg);
> + back_to = make_cleanup (free_explicit_linespec, els);
> create_breakpoint (get_current_arch (),
> - NULL, arg,
> + els, arg,
> NULL, 0, NULL, 1 /* parse arg */,
> tempflag, type_wanted,
> 0 /* Ignore count */,
> @@ -9686,6 +9734,7 @@ break_command_1 (char *arg, int flag, int from_tty)
> 1 /* enabled */,
> 0 /* internal */,
> 0);
> + do_cleanups (back_to);
> }
>
> /* Helper function for break_command_1 and disassemble_command. */
> @@ -12639,7 +12688,17 @@ say_where (struct breakpoint *b)
> single string. */
> if (b->loc == NULL)
> {
> - printf_filtered (_(" (%s) pending."), b->addr_string);
> + char *where = b->addr_string;
> + struct cleanup *free_where = make_cleanup (null_cleanup, NULL);
> +
> + if (b->explicit != NULL)
> + {
> + where = explicit_linespec_to_string (b->explicit, b->addr_string);
> + make_cleanup (xfree, where);
> + }
Similar const vs. non-const comment like above.
> +
> + printf_filtered (_(" (%s) pending."), where);
> + do_cleanups (free_where);
> }
> else
> {
> @@ -12702,6 +12761,7 @@ base_breakpoint_dtor (struct breakpoint *self)
> xfree (self->addr_string);
> xfree (self->filter);
> xfree (self->addr_string_range_end);
> + free_explicit_linespec (self->explicit);
> }
>
> static struct bp_location *
> @@ -12854,7 +12914,7 @@ static void
> bkpt_re_set (struct breakpoint *b)
> {
> /* FIXME: is this still reachable? */
> - if (b->addr_string == NULL)
> + if (b->addr_string == NULL && b->explicit == NULL)
> {
> /* Anything without a string can't be re-set. */
> delete_breakpoint (b);
> @@ -13237,6 +13297,9 @@ bkpt_probe_create_sals_from_address (explicit_linespec *els, char **arg,
> {
> struct linespec_sals lsal;
>
> + if (els != NULL)
> + explicit_linespec_unsupported (type_wanted);
> +
> lsal.sals = parse_probes (arg, canonical);
>
> *copy_arg = xstrdup (canonical->addr_string);
> @@ -13249,6 +13312,9 @@ static void
> bkpt_probe_decode_linespec (struct breakpoint *b, explicit_linespec *els,
> char **s, struct symtabs_and_lines *sals)
> {
> + if (els != NULL)
> + explicit_linespec_unsupported (b->type);
> +
> *sals = parse_probes (s, NULL);
> if (!sals->sals)
> error (_("probe not found"));
> @@ -13461,7 +13527,7 @@ strace_marker_create_breakpoints_sal (struct gdbarch *gdbarch,
>
> tp = XCNEW (struct tracepoint);
> init_breakpoint_sal (&tp->base, gdbarch, expanded,
> - NULL, addr_string, NULL,
> + canonical->explicit, addr_string, NULL,
> cond_string, extra_string,
> type_wanted, disposition,
> thread, task, ignore_count, ops,
> @@ -13487,6 +13553,9 @@ strace_marker_decode_linespec (struct breakpoint *b, explicit_linespec *els,
> {
> struct tracepoint *tp = (struct tracepoint *) b;
>
> + if (els != NULL)
> + explicit_linespec_unsupported (b->type);
> +
> *sals = decode_static_tracepoint_spec (s);
> if (sals->nelts > tp->static_trace_marker_id_idx)
> {
> @@ -14127,7 +14196,7 @@ breakpoint_re_set_default (struct breakpoint *b)
> struct symtabs_and_lines expanded = {0};
> struct symtabs_and_lines expanded_end = {0};
>
> - sals = addr_string_to_sals (b, NULL, b->addr_string, &found);
> + sals = addr_string_to_sals (b, b->explicit, b->addr_string, &found);
>
> if (found)
> {
> @@ -14137,7 +14206,7 @@ breakpoint_re_set_default (struct breakpoint *b)
>
> if (b->addr_string_range_end)
> {
> - sals_end = addr_string_to_sals (b, NULL, b->addr_string_range_end,
> + sals_end = addr_string_to_sals (b, b->explicit, b->addr_string_range_end,
> &found);
> if (found)
> {
> @@ -14196,10 +14265,15 @@ decode_linespec_default (struct breakpoint *b, explicit_linespec *els,
> struct linespec_result canonical;
>
> init_linespec_result (&canonical);
> - decode_line_full (s, DECODE_LINE_FUNFIRSTLINE,
> - (struct symtab *) NULL, 0,
> - &canonical, multiple_symbols_all,
> - b->filter);
> + if (els == NULL)
> + decode_line_full (s, DECODE_LINE_FUNFIRSTLINE,
> + (struct symtab *) NULL, 0,
Just drop the cast (struct symtab *) (yes, it is just code move).
> + &canonical, multiple_symbols_all,
> + b->filter);
> + else
> + decode_explicit_linespec (els, DECODE_LINE_FUNFIRSTLINE,
> + NULL, 0, &canonical, multiple_symbols_all,
> + b->filter);
>
> /* We should get 0 or 1 resulting SALs. */
> gdb_assert (VEC_length (linespec_sals, canonical.sals) < 2);
> diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
> index 43be84a..3212f4f 100644
> --- a/gdb/breakpoint.h
> +++ b/gdb/breakpoint.h
> @@ -683,6 +683,11 @@ struct breakpoint
> /* String we used to set the breakpoint (malloc'd). */
> char *addr_string;
>
> + /* The explicit linespec used to set the breakpoint (malloc'd).
> + This may be computed by decode_line_full/decode_explicit_linespec
> + or set by the user. */
It can be sometimes NULL. When? [maybe obsolete comment now after the top
discussion]
> + struct explicit_linespec *explicit;
> +
> /* The filter that should be passed to decode_line_full when
> re-setting this breakpoint. This may be NULL, but otherwise is
> allocated with xmalloc. */
> diff --git a/gdb/linespec.c b/gdb/linespec.c
> index 2b84515..fcbf3dd 100644
> --- a/gdb/linespec.c
> +++ b/gdb/linespec.c
> @@ -1664,9 +1664,14 @@ canonicalize_linespec (struct linespec_state *state, linespec_p ls)
> if (!state->canonical)
> return;
>
> + state->canonical->explicit = new_explicit_linespec ();
> +
> /* Shortcut expressions, which can only appear by themselves. */
> if (ls->expression != NULL)
> - state->canonical->addr_string = xstrdup (ls->expression);
> + {
> + state->canonical->addr_string = xstrdup (ls->expression);
> + state->canonical->explicit->expression = xstrdup (ls->expression);
> + }
> else
> {
> struct ui_file *buf;
> @@ -1676,6 +1681,8 @@ canonicalize_linespec (struct linespec_state *state, linespec_p ls)
> if (ls->source_filename)
> {
> fputs_unfiltered (ls->source_filename, buf);
> + state->canonical->explicit->source_filename
> + = xstrdup (ls->source_filename);
> need_colon = 1;
> }
>
> @@ -1684,6 +1691,8 @@ canonicalize_linespec (struct linespec_state *state, linespec_p ls)
> if (need_colon)
> fputc_unfiltered (':', buf);
> fputs_unfiltered (ls->function_name, buf);
> + state->canonical->explicit->function_name
> + = xstrdup (ls->function_name);
> need_colon = 1;
> }
>
> @@ -1706,6 +1715,7 @@ canonicalize_linespec (struct linespec_state *state, linespec_p ls)
> }
>
> fputs_unfiltered (ls->label_name, buf);
> + state->canonical->explicit->label_name = xstrdup (ls->label_name);
> need_colon = 1;
> state->canonical->special_display = 1;
> }
> @@ -1714,11 +1724,13 @@ canonicalize_linespec (struct linespec_state *state, linespec_p ls)
> {
> 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);
> + state->canonical->explicit->offset
> + = xstrprintf ("%s%d",
> + (ls->line_offset.sign == LINE_OFFSET_NONE ? ""
> + : (ls->line_offset.sign
> + == LINE_OFFSET_PLUS ? "+" : "-")),
> + ls->line_offset.offset);
> + fputs_filtered (state->canonical->explicit->offset, buf);
> }
>
> state->canonical->addr_string = ui_file_xstrdup (buf, NULL);
> @@ -2385,6 +2397,125 @@ decode_line_full (char **argptr, int flags,
> do_cleanups (cleanups);
> }
>
> +/* A parsing function for explicit linespecs. */
> +
> +static struct symtabs_and_lines
> +parse_explicit_linespec (linespec_parser *parser, void *data)
> +{
> + VEC (symbolp) *symbols, *labels;
> + VEC (minsym_and_objfile_d) *minimal_symbols;
It is only a style but I would prefer moving them into their blocks where they
are used (even if they will have to be declared multiple times).
> + explicit_linespec *els = (explicit_linespec *) data;
> +
> + if (els->expression != NULL)
> + {
> + char *copy = xstrdup (els->expression);
> + struct cleanup *cleanup = make_cleanup (xfree, copy);
> +
> + /* Convert the expression to a PC and save the result. */
> + PARSER_RESULT (parser)->expr_pc = linespec_expression_to_pc (©);
> + PARSER_RESULT (parser)->expression = xstrdup (els->expression);
> + do_cleanups (cleanup);
> + }
> + else
> + {
> + if (els->source_filename != NULL)
> + {
> + volatile struct gdb_exception except;
> +
> + TRY_CATCH (except, RETURN_MASK_ERROR)
> + {
> + PARSER_RESULT (parser)->file_symtabs
> + = symtabs_from_filename (els->source_filename);
> + }
> +
> + if (except.reason < 0
> + || PARSER_RESULT (parser)->file_symtabs == NULL)
> + source_file_not_found_error (els->source_filename);
> +
> + /* Save the source filename in the parser's result. */
> + PARSER_RESULT (parser)->source_filename
> + = xstrdup (els->source_filename);
> + }
> + else
> + {
> + /* A NULL entry means to use the default symtab. */
> + VEC_safe_push (symtab_p, PARSER_RESULT (parser)->file_symtabs, NULL);
> + }
> +
> + if (els->function_name != NULL)
> + {
> + find_linespec_symbols (PARSER_STATE (parser),
> + PARSER_RESULT (parser)->file_symtabs,
> + els->function_name, &symbols,
> + &minimal_symbols);
> +
> + if (symbols == NULL && minimal_symbols == NULL)
> + symbol_not_found_error (els->function_name,
> + PARSER_RESULT (parser)->source_filename);
> +
> + PARSER_RESULT (parser)->function_name = xstrdup (els->function_name);
> + PARSER_RESULT (parser)->function_symbols = symbols;
> + PARSER_RESULT (parser)->minimal_symbols = minimal_symbols;
> + }
> +
> + if (els->label_name != NULL)
> + {
> + symbols = NULL;
> + labels = find_label_symbols (PARSER_STATE (parser),
> + PARSER_RESULT (parser)->function_symbols,
> + &symbols, els->label_name);
> +
> + if (labels == NULL)
> + undefined_label_error (PARSER_RESULT (parser)->function_name,
> + els->label_name);
> +
> + PARSER_RESULT (parser)->label_name = xstrdup (els->label_name);
> + PARSER_RESULT (parser)->labels.label_symbols = labels;
> + PARSER_RESULT (parser)->labels.function_symbols = symbols;
> + }
> +
> + if (els->offset != NULL)
> + PARSER_RESULT (parser)->line_offset
> + = linespec_parse_line_offset (els->offset);
> +
> + /* One special error check: If SOURCE_FILENAME was given without
> + OFFSET, FUNCTION_NAME, or LABEL_NAME, issue an error. */
> + if (PARSER_RESULT (parser)->source_filename != NULL
> + && PARSER_RESULT (parser)->function_name == NULL
> + && PARSER_RESULT (parser)->label_name == NULL
> + && PARSER_RESULT (parser)->line_offset.sign == LINE_OFFSET_UNKNOWN)
> + error (_("Source filename requires function, label, or offset."));
> + }
> +
> + /* Convert the "parse" to SALs. */
> + return convert_linespec_to_sals (PARSER_STATE (parser),
> + PARSER_RESULT (parser));
> +}
> +
> +/* Decode the explicit linespec in ELS. The other parameters are
> + the same as decode_line_full. */
> +
> +void
> +decode_explicit_linespec (explicit_linespec *els, int flags,
> + struct symtab *default_symtab,
> + int default_line, struct linespec_result *canonical,
> + const char *select_mode,
> + const char *filter)
> +{
> + linespec_parser parser;
> + struct cleanup *cleanups;
> +
> + gdb_assert (canonical != NULL);
> + gdb_assert ((flags & DECODE_LINE_LIST_MODE) == 0);
> +
> + linespec_parser_new (&parser, parse_explicit_linespec, flags,
> + current_language, default_symtab,
> + default_line, canonical);
> + cleanups = make_cleanup (linespec_parser_delete, &parser);
> + do_decode_line (&parser, els, select_mode, filter);
> + do_cleanups (cleanups);
> +}
> +
> /* See linespec.h. */
>
> struct symtabs_and_lines
> @@ -2487,7 +2618,8 @@ linespec_expression_to_pc (char **exp_ptr)
> throw_error (NOT_FOUND_ERROR, _("cannot evaluate expressions while "
> "program space is in startup"));
>
> - (*exp_ptr)++;
> + if (**exp_ptr == '*')
> + (*exp_ptr)++;
This is not right:
(gdb) l 1
1 int main (void) { return 0; }
2 int (*a) (void) = main;
3 int (**b) (void) = &a;
(gdb) p a
$1 = (int (*)(void)) 0x4004ec <main>
(gdb) p b
$2 = (int (**)(void)) 0x601018 <a>
(gdb) break -address b
Breakpoint 1 at 0x601018
(gdb) break -address *b
Note: breakpoint 1 also set at pc 0x601018.
Breakpoint 2 at 0x601018
(gdb) break -address (*b)
Breakpoint 3 at 0x4004ec: file 12.c, line 1.
(gdb)
'*b' and '(*b)' should behave the same, they are not.
> return value_as_address (parse_to_comma_and_eval (exp_ptr));
> }
>
> @@ -3573,6 +3705,7 @@ destroy_linespec_result (struct linespec_result *ls)
> struct linespec_sals *lsal;
>
> xfree (ls->addr_string);
> + free_explicit_linespec (ls->explicit);
> for (i = 0; VEC_iterate (linespec_sals, ls->sals, i, lsal); ++i)
> {
> xfree (lsal->canonical);
> @@ -3596,3 +3729,299 @@ make_cleanup_destroy_linespec_result (struct linespec_result *ls)
> {
> return make_cleanup (cleanup_linespec_result, ls);
> }
> +
> +/* Allocate and initialize a new explicit_linespec. */
> +
> +explicit_linespec *
> +new_explicit_linespec (void)
> +{
> + return XCNEW (explicit_linespec);
> +}
> +
> +/* Free the explicit_linespec represented by OBJ. */
> +
> +void
> +free_explicit_linespec (void *obj)
> +{
> + explicit_linespec *els = (explicit_linespec *) obj;
> +
> + if (els != NULL)
> + {
> + xfree ((char *) els->source_filename);
> + xfree ((char *) els->function_name);
> + xfree ((char *) els->label_name);
> + xfree ((char *) els->offset);
> + xfree ((char *) els->expression);
> + xfree (els);
> + }
> +}
> +
> +/* A lexer for explicit linespecs. This function will advance INP past
> + any strings that it returns. Returns a malloc'd copy of the
> + lexed string or NULL if no lexing was done. */
> +
> +static char *
> +explicit_lex_one (char **inp)
Function does not modify the string itself, so it should be 'const char **'.
> +{
> + char *start = *inp;
> +
> + if (*start != '\0')
IMO rather return if (*start == '\0'), the function is needlessly indented
right now.
> + {
> + int lexing_number = 0;
> +
> + /* If quoted, skip to the ending quote. */
> + if (strchr (linespec_quote_characters, *start))
> + {
> + char quote_char = *start;
> +
> + /* If the input is not an ada operator, skip to the matching
> + closing quote and return the string. */
> + if (!(current_language->la_language == language_ada
> + && quote_char == '\"' && is_ada_operator (start)))
> + {
> + const char *end = skip_quote_char (start + 1, quote_char);
> +
> + if (end == NULL)
> + error (_("unmatched quote"));
It could print:
error (_("unmatched quote starting at: %s"), start);
> + *inp = (char *) end + 1;
> + return savestring (start + 1, *inp - start - 2);
> + }
> + }
> +
> + /* Are we lexing a number? */
> + if ((start[0] == '-' || start[0] == '+')
> + && start[1] && isdigit (start[1]))
> + {
> + /* The input is a relative offset. */
> + lexing_number = 1;
> +
> + /* Skip the +/-. */
> + ++(*inp);
> + }
> + else if (isdigit (*start))
> + lexing_number = 1;
> +
> + /* If the input starts with '-', the string ends with the next
> + whitespace. */
> + if (*start == '-')
> + {
> + while ((*inp)[0] && !isspace ((*inp)[0]))
> + ++(*inp);
> + }
> + /* If lexing a number, stop at the first non-digit character. */
> + else if (lexing_number)
> + {
> + while ((*inp)[0] && isdigit ((*inp)[0]))
> + ++(*inp);
> + }
> + else
> + {
> + /* Otherwise, stop at the next occurrence of "SPACE -", '\0', or
> + keyword. */
> + while ((*inp)[0]
> + && !(isspace ((*inp)[0])
> + && ((*inp)[1] == '-'
> + || linespec_lexer_lex_keyword (&(*inp)[1]))))
> + ++(*inp);
> + }
> + }
> +
> + if (*inp - start > 0)
> + return savestring (start, *inp - start);
> +
> + return NULL;
> +}
> +
> +/* Attempt to convert the address string in *ARGP into an explicit_linespec.
> + Returns the explicit linespec (which must be free'd by the caller) and
> + advances ARGP over all processed input. Returns NULL if *ARGP does
> + not describe an explicit linespec.
> +
> + This function may call error() if the *ARGP looks like properly formed
> + input, e.g., if it is called with missing argument parameters or
> + invalid options. */
> +
> +explicit_linespec *
> +string_to_explicit_linespec (char **argp)
> +{
> +
Excessive empty line.
> + /* It is assumed that linespecs beginning with '-' and a non-digit
> + character is an explicit linespec. */
> + if (argp != NULL && *argp != NULL)
> + {
> + const char *copy = *argp;
> +
> + if (probe_linespec_to_ops (©) != NULL)
> + {
> + /* Explicit linespecs with probes is not supported. */
> + return NULL;
> + }
> +
> + if ((*argp)[0] == '-' && isalpha ((*argp)[1]))
> + {
> + explicit_linespec *els;
> + struct cleanup *cleanup;
> +
> + els = new_explicit_linespec ();
> + cleanup = make_cleanup (free_explicit_linespec, els);
> +
> + /* Process option/argument pairs. */
> + while ((*argp)[0] != '\0')
> + {
> + int len;
> + char *opt, *oarg, *start;
> + struct cleanup *inner;
> +
> + /* If *ARGP starts with a keyword, stop processing
> + options. */
> + if (linespec_lexer_lex_keyword (*argp) != NULL)
> + break;
> +
> + /* Mark the start of the string in case we need to rewind. */
> + start = *argp;
> +
> + /* Get the option string. */
> + opt = explicit_lex_one (argp);
> + inner = make_cleanup (xfree, opt);
> +
> + /* Skip any whitespace. */
> + *argp = skip_spaces (*argp);
> +
> + /* Get the argument string. */
> + oarg = explicit_lex_one (argp);
> + make_cleanup (xfree, oarg);
> +
> + /* Skip any whitespace. */
> + *argp = skip_spaces (*argp);
> +
> + /* Use the length of the option to allow abbreviations. */
> + len = strlen (opt);
> +
> + /* All options have a required argument. */
> + if (strncmp (opt, "-source", len) == 0)
> + els->source_filename = oarg;
> + else if (strncmp (opt, "-function", len) == 0)
> + els->function_name = oarg;
> + else if (strncmp (opt, "-label", len) == 0)
> + els->label_name = oarg;
> + else if (strncmp (opt, "-offset", len) == 0)
> + els->offset = oarg;
> + else if (strncmp (opt, "-address", len) == 0)
> + els->expression = oarg;
I do not see these options documented in "help break".
> + /* Only emit an "invalid argument" error for options
> + that look like option strings. */
> + else if (opt[0] == '-' && !isdigit (opt[1]))
> + error (_("invalid linespec argument, \"%s\""), opt);
Errors should be a sentence.
> + else
> + {
> + /* Trailing garbage. This will be handled by
> + one of the callers. */
> + *argp = start;
> + break;
> + }
> +
> + /* It's a little lame to error after the fact, but in this
> + case, it provides a much better user experience to issue
> + the "invalid linespec argument" error before any missing
> + argument error. */
> + if (oarg == NULL)
> + error (_("missing argument for \"%s\""), opt);
Errors should be a sentence.
> +
> + /* The option/argument pair was successfully processed,
> + that memory now belongs to the explicit_linespec. */
> + discard_cleanups (inner);
> + }
> +
> + /* A valid explicit linespec was constructed. */
> + discard_cleanups (cleanup);
> + return els;
> + }
> + }
> +
> + /* Not an explicit linespec. */
> + return NULL;
> +}
> +
> +/* Deep copy the explicit linespec given by SRC and return
> + the copy. */
> +
> +explicit_linespec *
> +copy_explicit_linespec (explicit_linespec *src)
> +{
> + explicit_linespec *copy;
> +
> + copy = XCNEW (explicit_linespec);
> +#define STRDUP_IF(MBR) \
> + do { \
> + if (src->MBR != NULL) \
> + copy->MBR = xstrdup (src->MBR); \
> + } \
> + while (0)
A nitpick but this macro does not have GNU Coding Style.
> + STRDUP_IF (source_filename);
> + STRDUP_IF (function_name);
> + STRDUP_IF (label_name);
> + STRDUP_IF (offset);
> + STRDUP_IF (expression);
> +#undef STRDUP_IF
> + return copy;
> +}
[...]
struct breakpoint:
struct explicit_linespec *explicit;
init_breakpoint_sal:
explicit_linespec *els,
style: Couldn't you use uniformly either struct explicit_linespec or
explicit_linespec (IMO struct explicit_linespec as GDB is using struct) and
also to either els or explicit (IMO explicit), sometimes you use just
explicit but sometimes explicit_linespec (free_explicit_linespec), IMO just use
explicit_linespec everywhere, too many new names for me for a single
patch/feature.