From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21987 invoked by alias); 30 Jul 2012 15:59:18 -0000 Received: (qmail 21968 invoked by uid 22791); 30 Jul 2012 15:59:14 -0000 X-SWARE-Spam-Status: No, hits=-6.3 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,RCVD_IN_DNSWL_HI,RCVD_IN_HOSTKARMA_W,SPF_HELO_PASS,T_RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 30 Jul 2012 15:58:53 +0000 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q6UFwq92010479 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 30 Jul 2012 11:58:52 -0400 Received: from host2.jankratochvil.net (ovpn-116-33.ams2.redhat.com [10.36.116.33]) by int-mx01.intmail.prod.int.phx2.redhat.com (8.13.8/8.13.8) with ESMTP id q6UFwWMf000413 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Mon, 30 Jul 2012 11:58:37 -0400 Date: Mon, 30 Jul 2012 15:59:00 -0000 From: Jan Kratochvil To: Keith Seitz Cc: "gdb-patches@sourceware.org ml" Subject: Re: [RFA 3/5] Explicit linespecs - implementation Message-ID: <20120730155824.GA4283@host2.jankratochvil.net> References: <50120F6C.2000308@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <50120F6C.2000308@redhat.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-IsSubscribed: yes Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org X-SW-Source: 2012-07/txt/msg00758.txt.bz2 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
(gdb) p b $2 = (int (**)(void)) 0x601018 (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.