From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 12153 invoked by alias); 3 Aug 2014 01:50:19 -0000 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 Received: (qmail 8922 invoked by uid 89); 3 Aug 2014 01:50:07 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.5 required=5.0 tests=AWL,BAYES_00,KAM_STOCKGEN,RCVD_IN_DNSWL_LOW,RP_MATCHES_RCVD,SPF_PASS autolearn=no version=3.3.2 X-HELO: mail-ie0-f201.google.com Received: from mail-ie0-f201.google.com (HELO mail-ie0-f201.google.com) (209.85.223.201) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Sun, 03 Aug 2014 01:49:33 +0000 Received: by mail-ie0-f201.google.com with SMTP id tr6so1641584ieb.0 for ; Sat, 02 Aug 2014 18:49:30 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:mime-version:content-type :content-transfer-encoding:message-id:date:to:cc:subject:in-reply-to :references; bh=HQ+bu4wYrTXSWiRLgbP6zBgH2BpE8h2KNvfU+xfqrWI=; b=eYQwGxNjFlyRbMC3f2JgoOj2ibH28+5F8XyqIJ0FGuEDNo09taMssWr00CPQWZYt04 ERoQa2UAVu7+BlypDW/8AAyZgmFEUkgFyDKVXb8odDIYYnuEmTs2jx15KeTIpHOR42Tq 4fWFqeLvd4F5cZHkiKNbe7kcb9zSk5fGh4w4xFEbMt3aeJiTdRLfqQP14VuK8GCgSp9x 9yEMpXJTS0msx1iSH0KUsJJJ5IuC/0qiGCS2gNk6j+dN1U+gv1qKqfydQUzssSqLOAwt 52xWk1HTvj+Gb98uEkX82L2Al/UXf+ff7naBqMjr34xZfVmZ2bo/t7Of2pgHbRF5/vrN lqqA== X-Gm-Message-State: ALoCoQm4N9KrOap658iW67Y6wAQ0hAsgw0p7W9WB3rfKj852BWH7tQPArWR/9HjDEMPu09RB7PwW X-Received: by 10.43.69.13 with SMTP id ya13mr19640357icb.33.1407030570260; Sat, 02 Aug 2014 18:49:30 -0700 (PDT) Received: from corp2gmr1-2.hot.corp.google.com (corp2gmr1-2.hot.corp.google.com [172.24.189.93]) by gmr-mx.google.com with ESMTPS id z50si806923yhb.3.2014.08.02.18.49.30 for (version=TLSv1.1 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Sat, 02 Aug 2014 18:49:30 -0700 (PDT) Received: from ruffy.mtv.corp.google.com (ruffy.mtv.corp.google.com [172.17.128.44]) by corp2gmr1-2.hot.corp.google.com (Postfix) with ESMTP id 944605A4327; Sat, 2 Aug 2014 18:49:29 -0700 (PDT) From: Doug Evans MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 7bit Message-ID: <21469.38185.804.425392@ruffy.mtv.corp.google.com> Date: Sun, 03 Aug 2014 01:50:00 -0000 To: Keith Seitz Cc: "gdb-patches@sourceware.org ml" Subject: Re: [RFA 6/9] Explicit locations v2 - Add explicit locations In-Reply-To: <5388CBA2.1060703@redhat.com> References: <536BC6C6.6040501@redhat.com> <5388CBA2.1060703@redhat.com> X-IsSubscribed: yes X-SW-Source: 2014-08/txt/msg00036.txt.bz2 Hi. Several comments inline. I still have patches 7,8,9 to go. My plan is to get to them next week (fingers crossed, knock on wood, and all that :-)). Keith Seitz writes: > This is an update to this patch to incorporate changes from reviews of > earlier patches. > > Keith > > Changes since last version: > - remove SAVE_SPEC stuff > - struct linespec: use struct explicit_location instead of > all those strings/line_offset. > - Add PARSER_EXPLICIT and use it where needed > - No need to "convert linespec to explicit" -- the parser > now builds an explicit location of the parsed result. > - move string constructions out of canonicalize_linespec and into > its own function > - update comments on grammar of linespecs: expr_spec is now > no longer considered a linespec. > - add explicit_to_string_internal and use it to implement > explicit_location_to_string/explicit_location_to_linespec > - new_explicit_location > - remove explicit_to_event_location > > ChangeLog > 2014-05-29 Keith Seitz > > * breakpoint.c (create_overlay_breakpoint): Convert linespec > into explicit location. > (create_longjmp_master_breakpoint): Likewise. > (create_std_terminate_master_breakpoint): Likewise. > (create_exception_master_breakpoint): Likewise. > (create_breakpoint): Update the SAVE_SPEC for explicit locations. > (update_static_tracepoint): Convert linespec into explicit location. > (location_to_sals): Save the string representation of the location > for pending locations which were resolved. > * linespec.c (enum offset_relative_sign): Move to location.h. > (struct line_offset): Likewise. > (struct linespec) > : Replace with ... > : ... this. > : New member. > (PARSER_EXPLICIT): New accessor macro. > (undefined_label_error): New function. > (source_file_not_found_error): New function. > (linespec_parse_basic): The parser result is now an explicit location. > Use PARSER_EXPLICIT to access it. > Use undefined_label_error. > (canonicalize_linespec): Convert canonical linespec into explicit > location. > Move string representation of location to explicit_location_to_linespec > and use it and explicit_location_to_string to save string > representations of the canonical location. > (create_sals_line_offset): Use PARSER_EXPLICIT to access the parser's > explicit location result. > (convert_linespec_to_sals): `ls' contains an explicit location. > Update all references. > (convert_explicit_location_to_sals): New function. > (linespec_parse_basic): Use PARSER_EXPLICIT to access the parser > result's explicit location. > (linespec_state_constructor): Initialize is_linespec. > Use PARSER_EXPLICIT. > (linespec_parser_delete): Use PARSER_EXPLICIT to access the parser's > result. > (event_location_to_sals): For linespec locations, set is_linespec. > Handle explicit locations. > (decode_objc): 'ls' contains an explicit location now. Update all > references. > (symtabs_from_filename): Use source_file_not_found_error. > * location.c (initialize_explicit_location): New function. > (initialize_event_location): Initialize explicit locations. > (copy_event_location): Handle explicit locations. > (explicit_to_string_internal): New function; most of contents moved > from canonicalize_linespec. > (explicit_location_to_string): New function. > (explicit_location_to_linespec): New function. > (delete_event_location): Handle explicit locations. > (event_location_to_string_const): Likewise. > (event_location_empty_p): Likewise. > * location.h (enum offset_relative_sign): Move here from linespec.h. > (struct line_offset): Likewise. > (enum event_location_type): Add EXPLICIT_LOCATION. > (struct explicit_location): New structure. > (struct event_location.u) : New member. > (EVENT_LOCATION_EXPLICIT): Define accessor macro. > (explicit_location_to_string): Declare. > (explicit_location_to_linespec): Declare. > (initialize_explicit_location): Declare. > > diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c > index 345b104..d8c74c0 100644 > --- a/gdb/breakpoint.c > +++ b/gdb/breakpoint.c > @@ -3281,6 +3281,7 @@ create_overlay_event_breakpoint (void) > struct breakpoint *b; > struct breakpoint_objfile_data *bp_objfile_data; > CORE_ADDR addr; > + struct explicit_location explicit; > > bp_objfile_data = get_breakpoint_objfile_data (objfile); > > @@ -3305,7 +3306,9 @@ create_overlay_event_breakpoint (void) > b = create_internal_breakpoint (get_objfile_arch (objfile), addr, > bp_overlay_event, > &internal_breakpoint_ops); > - b->location = new_linespec_location (func_name); > + initialize_explicit_location (&explicit); > + explicit.function_name = ASTRDUP (func_name); > + b->location = new_explicit_location (&explicit); > > if (overlay_debugging == ovly_auto) > { > @@ -3402,6 +3405,7 @@ create_longjmp_master_breakpoint (void) > struct breakpoint *b; > const char *func_name; > CORE_ADDR addr; > + struct explicit_location explicit; > > if (msym_not_found_p (bp_objfile_data->longjmp_msym[i].minsym)) > continue; > @@ -3424,7 +3428,9 @@ create_longjmp_master_breakpoint (void) > addr = BMSYMBOL_VALUE_ADDRESS (bp_objfile_data->longjmp_msym[i]); > b = create_internal_breakpoint (gdbarch, addr, bp_longjmp_master, > &internal_breakpoint_ops); > - b->location = new_linespec_location (func_name); > + initialize_explicit_location (&explicit); > + explicit.function_name = ASTRDUP (func_name); > + b->location = new_explicit_location (&explicit); > b->enable_state = bp_disabled; > } > } > @@ -3455,6 +3461,7 @@ create_std_terminate_master_breakpoint (void) > { > struct breakpoint *b; > struct breakpoint_objfile_data *bp_objfile_data; > + struct explicit_location explicit; > > bp_objfile_data = get_breakpoint_objfile_data (objfile); > > @@ -3480,7 +3487,9 @@ create_std_terminate_master_breakpoint (void) > b = create_internal_breakpoint (get_objfile_arch (objfile), addr, > bp_std_terminate_master, > &internal_breakpoint_ops); > - b->location = new_linespec_location (func_name); > + initialize_explicit_location (&explicit); > + explicit.function_name = ASTRDUP (func_name); > + b->location = new_explicit_location (&explicit); > b->enable_state = bp_disabled; > } > } > @@ -3504,6 +3513,7 @@ create_exception_master_breakpoint (void) > struct gdbarch *gdbarch; > struct breakpoint_objfile_data *bp_objfile_data; > CORE_ADDR addr; > + struct explicit_location explicit; > > bp_objfile_data = get_breakpoint_objfile_data (objfile); > > @@ -3584,7 +3594,9 @@ create_exception_master_breakpoint (void) > ¤t_target); > b = create_internal_breakpoint (gdbarch, addr, bp_exception_master, > &internal_breakpoint_ops); > - b->location = new_linespec_location (func_name); > + initialize_explicit_location (&explicit); > + explicit.function_name = ASTRDUP (func_name); > + b->location = new_explicit_location (&explicit); > b->enable_state = bp_disabled; > } > > @@ -10010,6 +10022,20 @@ create_breakpoint (struct gdbarch *gdbarch, > init_raw_breakpoint_without_location (b, gdbarch, type_wanted, ops); > b->location = copy_event_location (location); > > + /* If the location has a string representation, > + save it to the breakpoint's location string cache, since this > + may be used to save the breakpoint to a file. */ > + if (EVENT_LOCATION_TYPE (b->location) == EXPLICIT_LOCATION) > + { > + char *old = event_location_to_string_const (location); > + > + b->location->as_string > + = xstrprintf ("%s%s%s", old, > + (extra_string == NULL ? "" : " "), > + (extra_string == NULL ? "" : extra_string)); Here's another case where a file outside of the event-location-specific code is reaching into the internal implementation details of an event location. Plus it's a bit confusing to do this: b->location = copy_event_location (location); and then change the as_string representation of the location we just copied, but only for EXPLICIT_LOCATION. E.g., As the reader I'm left wondering what happens to extra_string if the event location was a different kind. Can you elaborate on what's going on here? > + xfree (old); > + } > + > if (parse_arg) > b->cond_string = NULL; > else > @@ -10022,7 +10048,17 @@ create_breakpoint (struct gdbarch *gdbarch, > } > b->cond_string = cond_string; > } > - b->extra_string = NULL; > + > + /* For explicit locations, EXTRA_STRING may contain breakpoint > + condition information. Make a private copy of this for the > + breakpoint. */ The comment predicates itself on being specific to explicit locations, but the code doesn't check for EXPLICIT_LOCATION. Can you elaborate on what's going on here? > + if (extra_string != NULL && *extra_string != '\0') > + { > + b->extra_string = xstrdup (extra_string); > + make_cleanup (xfree, b->extra_string); > + } > + else > + b->extra_string = NULL; > b->ignore_count = ignore_count; > b->disposition = tempflag ? disp_del : disp_donttouch; > b->condition_not_parsed = 1; > @@ -14212,6 +14248,7 @@ update_static_tracepoint (struct breakpoint *b, struct symtab_and_line sal) > struct symbol *sym; > struct static_tracepoint_marker *tpmarker; > struct ui_out *uiout = current_uiout; > + struct explicit_location explicit; > > tpmarker = VEC_index (static_tracepoint_marker_p, markers, 0); > > @@ -14253,11 +14290,12 @@ update_static_tracepoint (struct breakpoint *b, struct symtab_and_line sal) > b->loc->symtab = sym != NULL ? sal2.symtab : NULL; > > delete_event_location (b->location); > - b->location = new_linespec_location (NULL); > - EVENT_LOCATION_LINESPEC (b->location) > - = xstrprintf ("%s:%d", > - symtab_to_filename_for_display (sal2.symtab), > - b->loc->line_number); > + initialize_explicit_location (&explicit); > + explicit.source_filename > + = ASTRDUP (symtab_to_filename_for_display (sal2.symtab)); > + explicit.line_offset.offset = b->loc->line_number; > + explicit.line_offset.sign = LINE_OFFSET_NONE; > + b->location = new_explicit_location (&explicit); Hmmm, the name new_explicit_location is ambiguous. Does it return an explicit_location* or an event_location* ? I like the consistency with new_linespec_location, new_address_location, etc. so I understand why it's the latter, though my first guess would be the former (though that would be confusing given that we're passing it an explicit_location*). No need to change anything, just thinking out loud. > > /* Might be nice to check if function changed, and warn if > so. */ > diff --git a/gdb/linespec.c b/gdb/linespec.c > index deda86e..f141a86 100644 > --- a/gdb/linespec.c > +++ b/gdb/linespec.c > @@ -66,73 +66,26 @@ typedef struct bound_minimal_symbol bound_minimal_symbol_d; > > DEF_VEC_O (bound_minimal_symbol_d); > > -/* An enumeration of possible signs for a line offset. */ > -enum offset_relative_sign > -{ > - /* No sign */ > - LINE_OFFSET_NONE, > - > - /* A plus sign ("+") */ > - LINE_OFFSET_PLUS, > - > - /* A minus sign ("-") */ > - LINE_OFFSET_MINUS, > - > - /* A special "sign" for unspecified offset. */ > - LINE_OFFSET_UNKNOWN > -}; > - > -/* A line offset in a linespec. */ > - > -struct line_offset > -{ > - /* Line offset and any specified sign. */ > - int offset; > - enum offset_relative_sign sign; > -}; > - > /* A linespec. Elements of this structure are filled in by a parser > (either parse_linespec or some other function). The structure is > then converted into SALs by convert_linespec_to_sals. */ > > struct linespec > { > - /* An expression and the resulting PC. Specifying an expression > - currently precludes the use of other members. */ > - > - /* The expression entered by the user. */ > - const char *expression; > - > - /* The resulting PC expression derived from evaluating EXPRESSION. */ > - CORE_ADDR expr_pc; > - > - /* Any specified file symtabs. */ > - > - /* The user-supplied source filename or NULL if none was specified. */ > - const char *source_filename; > + /* An explicit location describing the SaLs. */ > + struct explicit_location explicit; Does this read better to you? [here, and at all uses] struct explicit_location location; Dunno. When I read "explicit" I'm left wondering "Does that mean there's an implicit or some other possibility for the location that I have to think about?" The struct definition is local to linespec.c so the scope of any confusion is nicely confined, so I wouldn't make any change unless you think it's worth it. > > /* The list of symtabs to search to which to limit the search. May not > - be NULL. If SOURCE_FILENAME is NULL (no user-specified filename), > - FILE_SYMTABS should contain one single NULL member. This will > - cause the code to use the default symtab. */ > + be NULL. If explicit.SOURCE_FILENAME is NULL (no user-specified > + filename), FILE_SYMTABS should contain one single NULL member. This > + will cause the code to use the default symtab. */ > VEC (symtab_ptr) *file_symtabs; > > - /* The name of a function or method and any matching symbols. */ > - > - /* The user-specified function name. If no function name was > - supplied, this may be NULL. */ > - const char *function_name; > - > /* A list of matching function symbols and minimal symbols. Both lists > may be NULL if no matching symbols were found. */ > VEC (symbolp) *function_symbols; > VEC (bound_minimal_symbol_d) *minimal_symbols; > > - /* The name of a label and matching symbols. */ > - > - /* The user-specified label name. */ > - const char *label_name; > - > /* A structure of matching label symbols and the corresponding > function symbol in which the label was found. Both may be NULL > or both must be non-NULL. */ > @@ -141,10 +94,6 @@ struct linespec > VEC (symbolp) *label_symbols; > VEC (symbolp) *function_symbols; > } labels; > - > - /* Line offset. It may be LINE_OFFSET_UNKNOWN, meaning that no > - offset was specified. */ > - struct line_offset line_offset; > }; > typedef struct linespec *linespec_p; > > @@ -197,6 +146,9 @@ struct linespec_state > /* This is a set of address_entry objects which is used to prevent > duplicate symbols from being entered into the result. */ > htab_t addr_set; > + > + /* Are we building a linespec? */ > + int is_linespec; > }; > > /* This is a helper object that is used when collecting symbols into a > @@ -307,6 +259,10 @@ struct ls_parser > }; > typedef struct ls_parser linespec_parser; > > +/* A convenience macro for accessing the explicit location result of > + the parser. */ > +#define PARSER_EXPLICIT(PPTR) (&PARSER_RESULT ((PPTR))->explicit) > + > /* Prototypes for local functions. */ > > static void iterate_over_file_blocks (struct symtab *symtab, > @@ -1550,6 +1506,29 @@ unexpected_linespec_error (linespec_parser *parser) > token_type_strings[token.type]); > } > > +/* Throw an undefined label error. */ > + > +static void ATTRIBUTE_NORETURN > +undefined_label_error (const char *function, const char *label) > +{ > + if (function != NULL) > + throw_error (NOT_FOUND_ERROR, > + _("No label \"%s\" defined in function \"%s\"."), > + label, function); > + else > + throw_error (NOT_FOUND_ERROR, > + _("No label \"%s\" defined in current function."), > + label); > +} > + > +/* Throw a source file not found error. */ > + > +static void ATTRIBUTE_NORETURN > +source_file_not_found_error (const char *name) > +{ > + throw_error (NOT_FOUND_ERROR, _("No source file named %s."), name); > +} > + > /* Parse and return a line offset in STRING. */ > > static struct line_offset > @@ -1596,7 +1575,7 @@ linespec_parse_basic (linespec_parser *parser) > /* Record the line offset and get the next token. */ > name = copy_token_string (token); > cleanup = make_cleanup (xfree, name); > - PARSER_RESULT (parser)->line_offset = linespec_parse_line_offset (name); > + PARSER_EXPLICIT (parser)->line_offset = linespec_parse_line_offset (name); > do_cleanups (cleanup); > > /* Get the next token. */ > @@ -1633,7 +1612,7 @@ linespec_parse_basic (linespec_parser *parser) > { > PARSER_RESULT (parser)->function_symbols = symbols; > PARSER_RESULT (parser)->minimal_symbols = minimal_symbols; > - PARSER_RESULT (parser)->function_name = name; > + PARSER_EXPLICIT (parser)->function_name = name; > symbols = NULL; > discard_cleanups (cleanup); > } > @@ -1647,7 +1626,7 @@ linespec_parse_basic (linespec_parser *parser) > { > PARSER_RESULT (parser)->labels.label_symbols = labels; > PARSER_RESULT (parser)->labels.function_symbols = symbols; > - PARSER_RESULT (parser)->label_name = name; > + PARSER_EXPLICIT (parser)->label_name = name; > symbols = NULL; > discard_cleanups (cleanup); > } > @@ -1655,14 +1634,14 @@ linespec_parse_basic (linespec_parser *parser) > && *LS_TOKEN_STOKEN (token).ptr == '$') > { > /* User specified a convenience variable or history value. */ > - PARSER_RESULT (parser)->line_offset > + PARSER_EXPLICIT (parser)->line_offset > = linespec_parse_variable (PARSER_STATE (parser), name); > > - if (PARSER_RESULT (parser)->line_offset.sign == LINE_OFFSET_UNKNOWN) > + if (PARSER_EXPLICIT (parser)->line_offset.sign == LINE_OFFSET_UNKNOWN) > { > /* The user-specified variable was not valid. Do not > throw an error here. parse_linespec will do it for us. */ > - PARSER_RESULT (parser)->function_name = name; > + PARSER_EXPLICIT (parser)->function_name = name; > discard_cleanups (cleanup); > return; > } > @@ -1677,7 +1656,7 @@ linespec_parse_basic (linespec_parser *parser) > an error here. parse_linespec will do it for us. */ > > /* Save a copy of the name we were trying to lookup. */ > - PARSER_RESULT (parser)->function_name = name; > + PARSER_EXPLICIT (parser)->function_name = name; > discard_cleanups (cleanup); > return; > } > @@ -1697,7 +1676,7 @@ linespec_parse_basic (linespec_parser *parser) > get the next token. */ > name = copy_token_string (token); > cleanup = make_cleanup (xfree, name); > - PARSER_RESULT (parser)->line_offset > + PARSER_EXPLICIT (parser)->line_offset > = linespec_parse_line_offset (name); > do_cleanups (cleanup); > > @@ -1717,16 +1696,15 @@ linespec_parse_basic (linespec_parser *parser) > { > PARSER_RESULT (parser)->labels.label_symbols = labels; > PARSER_RESULT (parser)->labels.function_symbols = symbols; > - PARSER_RESULT (parser)->label_name = name; > + PARSER_EXPLICIT (parser)->label_name = name; > symbols = NULL; > discard_cleanups (cleanup); > } > else > { > /* We don't know what it was, but it isn't a label. */ > - throw_error (NOT_FOUND_ERROR, > - _("No label \"%s\" defined in function \"%s\"."), > - name, PARSER_RESULT (parser)->function_name); > + undefined_label_error (PARSER_EXPLICIT (parser)->function_name, > + name); > } > > /* Check for a line offset. */ > @@ -1744,7 +1722,7 @@ linespec_parse_basic (linespec_parser *parser) > name = copy_token_string (token); > cleanup = make_cleanup (xfree, name); > > - PARSER_RESULT (parser)->line_offset > + PARSER_EXPLICIT (parser)->line_offset > = linespec_parse_line_offset (name); > do_cleanups (cleanup); > > @@ -1761,42 +1739,19 @@ linespec_parse_basic (linespec_parser *parser) > } > > /* Canonicalize the linespec contained in LS. The result is saved into > - STATE->canonical. */ > + STATE->canonical. This function handles both linespec and explicit > + locations. */ > > static void > canonicalize_linespec (struct linespec_state *state, linespec_p ls) > { > - struct ui_file *buf; > - int need_colon = 0; > - > /* If canonicalization was not requested, no need to do anything. */ > if (!state->canonical) No requested change here. I just wanted to note that here's a good example where "if (state->canonical == NULL)" would be clearer. At this point in my reading for half a second I wondered if canonical was a boolean, and was confused because the function comment said the result was stored there. > return; > > - buf = mem_fileopen (); > - > - state->canonical->location = new_linespec_location (NULL); > - > - if (ls->source_filename) > + if (ls->explicit.label_name) > { > - fputs_unfiltered (ls->source_filename, buf); > - need_colon = 1; > - } > - > - if (ls->function_name) > - { > - if (need_colon) > - fputc_unfiltered (':', buf); > - fputs_unfiltered (ls->function_name, buf); > - need_colon = 1; > - } > - > - if (ls->label_name) > - { > - if (need_colon) > - fputc_unfiltered (':', buf); > - > - if (ls->function_name == NULL) > + if (ls->explicit.function_name == NULL) > { > struct symbol *s; > > @@ -1805,29 +1760,22 @@ canonicalize_linespec (struct linespec_state *state, linespec_p ls) > && (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); > + ls->explicit.function_name = xstrdup (SYMBOL_NATURAL_NAME (s)); This is a side-effect not adequately explained in the function comment. It's not clear from the function comment that LS is modified. Is the side-effect necessary (in which case let's make the function comment clearer), or can we avoid it? > } > > - 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); > - } > + /* Save everything as an explicit location. */ > + state->canonical->location = new_explicit_location (&ls->explicit); > > - EVENT_LOCATION_LINESPEC (state->canonical->location) > - = ui_file_xstrdup (buf, NULL); > - ui_file_delete (buf); > + /* Save a string representation of this linespec. */ > + if (state->is_linespec) > + state->canonical->location->as_string > + = explicit_location_to_linespec (&ls->explicit); > + else > + state->canonical->location->as_string > + = explicit_location_to_string (&ls->explicit); Why do we need to set as_string here again? Can users of it call event_location_to_string themselves? There's still the state->is_linespec distinction of course, feels odd to have it though: something outside of an event_location is deciding what to store there, feels like an abstraction violation. > } > > /* Given a line offset in LS, construct the relevant SALs. */ > @@ -1867,18 +1815,18 @@ create_sals_line_offset (struct linespec_state *self, > use_default = 1; > } > > - val.line = ls->line_offset.offset; > - switch (ls->line_offset.sign) > + val.line = ls->explicit.line_offset.offset; > + switch (ls->explicit.line_offset.sign) > { > case LINE_OFFSET_PLUS: > - if (ls->line_offset.offset == 0) > + if (ls->explicit.line_offset.offset == 0) > val.line = 5; > if (use_default) > val.line = self->default_line + val.line; > break; > > case LINE_OFFSET_MINUS: > - if (ls->line_offset.offset == 0) > + if (ls->explicit.line_offset.offset == 0) > val.line = 15; > if (use_default) > val.line = self->default_line - val.line; > @@ -1970,9 +1918,9 @@ create_sals_line_offset (struct linespec_state *self, > > if (values.nelts == 0) > { > - if (ls->source_filename) > + if (ls->explicit.source_filename) > throw_error (NOT_FOUND_ERROR, _("No line %d in file \"%s\"."), > - val.line, ls->source_filename); > + val.line, ls->explicit.source_filename); > else > throw_error (NOT_FOUND_ERROR, _("No line %d in the current file."), > val.line); > @@ -2072,13 +2020,13 @@ convert_linespec_to_sals (struct linespec_state *state, linespec_p ls) > } > } > } > - else if (ls->line_offset.sign != LINE_OFFSET_UNKNOWN) > + else if (ls->explicit.line_offset.sign != LINE_OFFSET_UNKNOWN) > { > /* Only an offset was specified. */ > sals = create_sals_line_offset (state, ls); > > /* Make sure we have a filename for canonicalization. */ > - if (ls->source_filename == NULL) > + if (ls->explicit.source_filename == NULL) > { > const char *fullname = symtab_to_fullname (state->default_symtab); > > @@ -2086,7 +2034,7 @@ convert_linespec_to_sals (struct linespec_state *state, linespec_p ls) > form so that displaying SOURCE_FILENAME can follow the current > FILENAME_DISPLAY_STRING setting. But as it is used only rarely > it has been kept for code simplicity only in absolute form. */ > - ls->source_filename = xstrdup (fullname); > + ls->explicit.source_filename = xstrdup (fullname); > } > } > else > @@ -2103,6 +2051,73 @@ convert_linespec_to_sals (struct linespec_state *state, linespec_p ls) > return sals; > } > > +/* Convert the explicit location EXPLICIT into SaLs. */ > + > +static struct symtabs_and_lines > +convert_explicit_location_to_sals (struct linespec_state *self, > + linespec_p result, > + struct explicit_location *explicit) How many of these parameters can be const? > +{ > + VEC (symbolp) *symbols, *labels; > + VEC (bound_minimal_symbol_d) *minimal_symbols; > + > + if (explicit->source_filename != NULL) > + { > + volatile struct gdb_exception except; > + > + TRY_CATCH (except, RETURN_MASK_ERROR) > + { > + result->file_symtabs > + = symtabs_from_filename (explicit->source_filename); > + } > + > + if (except.reason < 0 || result->file_symtabs == NULL) > + source_file_not_found_error (explicit->source_filename); > + > + result->explicit.source_filename = xstrdup (explicit->source_filename); > + } > + else > + { > + /* A NULL entry means to use the default symtab. */ > + VEC_safe_push (symtab_ptr, result->file_symtabs, NULL); > + } > + > + if (explicit->function_name != NULL) > + { > + find_linespec_symbols (self, result->file_symtabs, > + explicit->function_name, &symbols, > + &minimal_symbols); > + > + if (symbols == NULL && minimal_symbols == NULL) > + symbol_not_found_error (explicit->function_name, > + result->explicit.source_filename); > + > + result->explicit.function_name = xstrdup (explicit->function_name); > + result->function_symbols = symbols; > + result->minimal_symbols = minimal_symbols; > + } > + > + if (explicit->label_name != NULL) > + { > + symbols = NULL; > + labels = find_label_symbols (self, result->function_symbols, > + &symbols, explicit->label_name); > + > + if (labels == NULL) > + undefined_label_error (result->explicit.function_name, > + explicit->label_name); > + > + result->explicit.label_name = xstrdup (explicit->label_name); > + result->labels.label_symbols = labels; > + result->labels.function_symbols = symbols; > + } > + > + if (explicit->line_offset.sign != LINE_OFFSET_UNKNOWN) > + result->explicit.line_offset = explicit->line_offset; > + > + return convert_linespec_to_sals (self, result); > +} > + > /* Parse a string that specifies a linespec. > Pass the address of a char * variable; that variable will be > advanced over the characters actually parsed. > @@ -2215,13 +2230,13 @@ parse_linespec (linespec_parser *parser, const char **argptr) > /* User specified a convenience variable or history value. */ > var = copy_token_string (token); > cleanup = make_cleanup (xfree, var); > - PARSER_RESULT (parser)->line_offset > + PARSER_EXPLICIT (parser)->line_offset > = linespec_parse_variable (PARSER_STATE (parser), var); > do_cleanups (cleanup); > > /* If a line_offset wasn't found (VAR is the name of a user > variable/function), then skip to normal symbol processing. */ > - if (PARSER_RESULT (parser)->line_offset.sign != LINE_OFFSET_UNKNOWN) > + if (PARSER_EXPLICIT (parser)->line_offset.sign != LINE_OFFSET_UNKNOWN) > { > /* Consume this token. */ > linespec_lexer_consume_token (parser); > @@ -2257,7 +2272,7 @@ parse_linespec (linespec_parser *parser, const char **argptr) > if (file_exception.reason >= 0) > { > /* Symtabs were found for the file. Record the filename. */ > - PARSER_RESULT (parser)->source_filename = user_filename; > + PARSER_EXPLICIT (parser)->source_filename = user_filename; > > /* Get the next token. */ > token = linespec_lexer_consume_token (parser); > @@ -2294,7 +2309,7 @@ parse_linespec (linespec_parser *parser, const char **argptr) > > if (PARSER_RESULT (parser)->function_symbols == NULL > && PARSER_RESULT (parser)->labels.label_symbols == NULL > - && PARSER_RESULT (parser)->line_offset.sign == LINE_OFFSET_UNKNOWN > + && PARSER_EXPLICIT (parser)->line_offset.sign == LINE_OFFSET_UNKNOWN > && PARSER_RESULT (parser)->minimal_symbols == NULL) > { > /* The linespec didn't parse. Re-throw the file exception if > @@ -2303,8 +2318,8 @@ parse_linespec (linespec_parser *parser, const char **argptr) > throw_exception (file_exception); > > /* Otherwise, the symbol is not found. */ > - symbol_not_found_error (PARSER_RESULT (parser)->function_name, > - PARSER_RESULT (parser)->source_filename); > + symbol_not_found_error (PARSER_EXPLICIT (parser)->function_name, > + PARSER_EXPLICIT (parser)->source_filename); > } > > convert_to_sals: > @@ -2342,6 +2357,7 @@ linespec_state_constructor (struct linespec_state *self, > self->program_space = current_program_space; > self->addr_set = htab_create_alloc (10, hash_address_entry, eq_address_entry, > xfree, xcalloc, xfree); > + self->is_linespec = 0; > } > > /* Initialize a new linespec parser. */ > @@ -2356,7 +2372,7 @@ linespec_parser_new (linespec_parser *parser, > memset (parser, 0, sizeof (linespec_parser)); > parser->lexer.current.type = LSTOKEN_CONSUMED; > memset (PARSER_RESULT (parser), 0, sizeof (struct linespec)); > - PARSER_RESULT (parser)->line_offset.sign = LINE_OFFSET_UNKNOWN; > + PARSER_EXPLICIT (parser)->line_offset.sign = LINE_OFFSET_UNKNOWN; > linespec_state_constructor (PARSER_STATE (parser), flags, language, > default_symtab, default_line, canonical); > } > @@ -2376,10 +2392,9 @@ linespec_parser_delete (void *arg) > { > linespec_parser *parser = (linespec_parser *) arg; > > - xfree ((char *) PARSER_RESULT (parser)->expression); > - xfree ((char *) PARSER_RESULT (parser)->source_filename); > - xfree ((char *) PARSER_RESULT (parser)->label_name); > - xfree ((char *) PARSER_RESULT (parser)->function_name); > + xfree (PARSER_EXPLICIT (parser)->source_filename); > + xfree (PARSER_EXPLICIT (parser)->label_name); > + xfree (PARSER_EXPLICIT (parser)->function_name); > > if (PARSER_RESULT (parser)->file_symtabs != NULL) > VEC_free (symtab_ptr, PARSER_RESULT (parser)->file_symtabs); > @@ -2415,6 +2430,7 @@ event_location_to_sals (linespec_parser *parser, > const char *copy, *orig; > volatile struct gdb_exception except; > > + PARSER_STATE (parser)->is_linespec = 1; > TRY_CATCH (except, RETURN_MASK_ERROR) > { > orig = copy = EVENT_LOCATION_LINESPEC (location); > @@ -2433,6 +2449,17 @@ event_location_to_sals (linespec_parser *parser, > EVENT_LOCATION_ADDRESS (location)); > break; > > + case EXPLICIT_LOCATION: > + { > + struct explicit_location *explicit; > + > + explicit = EVENT_LOCATION_EXPLICIT (location); > + result = convert_explicit_location_to_sals (PARSER_STATE (parser), > + PARSER_RESULT (parser), > + explicit); > + } > + break; > + > case PROBE_LOCATION: > /* Probes are handled by their own decoders. */ > gdb_assert_not_reached ("attempt to decode probe location"); > @@ -2691,7 +2718,7 @@ decode_objc (struct linespec_state *self, linespec_p ls, const char **argptr) > memcpy (saved_arg, *argptr, new_argptr - *argptr); > saved_arg[new_argptr - *argptr] = '\0'; > > - ls->function_name = xstrdup (saved_arg); > + ls->explicit.function_name = xstrdup (saved_arg); > ls->function_symbols = info.result.symbols; > ls->minimal_symbols = info.result.minimal_symbols; > values = convert_linespec_to_sals (self, ls); > @@ -2701,9 +2728,9 @@ decode_objc (struct linespec_state *self, linespec_p ls, const char **argptr) > self->canonical->pre_expanded = 1; > self->canonical->location = new_linespec_location (NULL); > > - if (ls->source_filename) > + if (ls->explicit.source_filename) > EVENT_LOCATION_LINESPEC (self->canonical->location) > - = xstrprintf ("%s:%s", ls->source_filename, saved_arg); > + = xstrprintf ("%s:%s", ls->explicit.source_filename, saved_arg); > else > EVENT_LOCATION_LINESPEC (self->canonical->location) > = xstrdup (saved_arg); > @@ -3082,7 +3109,7 @@ symtabs_from_filename (const char *filename) > throw_error (NOT_FOUND_ERROR, > _("No symbol table is loaded. " > "Use the \"file\" command.")); > - throw_error (NOT_FOUND_ERROR, _("No source file named %s."), filename); > + source_file_not_found_error (filename); > } > > return result; > diff --git a/gdb/location.c b/gdb/location.c > index eafa27d..86d4232 100644 > --- a/gdb/location.c > +++ b/gdb/location.c > @@ -28,6 +28,15 @@ > #include > #include > > +/* Initialize the given explicit location. */ > + > +void > +initialize_explicit_location (struct explicit_location *explicit) > +{ > + memset (explicit, 0, sizeof (struct explicit_location)); > + explicit->line_offset.sign = LINE_OFFSET_UNKNOWN; > +} > + > /* Initialize the given LOCATION. */ > > void > @@ -36,6 +45,8 @@ initialize_event_location (struct event_location *location, > { > memset (location, 0, sizeof (struct event_location)); > EVENT_LOCATION_TYPE (location) = type; > + if (type == EXPLICIT_LOCATION) > + initialize_explicit_location (EVENT_LOCATION_EXPLICIT (location)); > } > > /* Create a new linespec location. The return result is malloc'd > @@ -82,6 +93,49 @@ new_probe_location (const char *probe) > return location; > } > > +/* Create a new explicit location. If not NULL, EXPLICIT is checked for > + validity. If invalid, an exception is thrown. > + > + The return result is malloc'd and should be freed with > + delete_event_location. */ > + > +struct event_location * > +new_explicit_location (const struct explicit_location *explicit) > +{ > + struct event_location tmp; > + > + initialize_event_location (&tmp, EXPLICIT_LOCATION); > + > + if (explicit != NULL) > + { > + if (explicit->source_filename != NULL) > + { > + /* Error check -- we must have one of the other > + parameters specified. */ > + if (explicit->function_name == NULL > + && explicit->label_name == NULL > + && explicit->line_offset.sign == LINE_OFFSET_UNKNOWN) > + error (_("Source filename requires function, label, or " > + "line offset.")); How about moving this error check into the caller(s) and have an assert here instead? One could write a utility to error check an explicit_location, and the callers could call that. > + > + EVENT_LOCATION_EXPLICIT (&tmp)->source_filename > + = explicit->source_filename; > + } > + > + if (explicit->function_name != NULL) > + EVENT_LOCATION_EXPLICIT (&tmp)->function_name > + = explicit->function_name; > + > + if (explicit->label_name != NULL) > + EVENT_LOCATION_EXPLICIT (&tmp)->label_name = explicit->label_name; > + > + if (explicit->line_offset.sign != LINE_OFFSET_UNKNOWN) > + EVENT_LOCATION_EXPLICIT (&tmp)->line_offset = explicit->line_offset; > + } > + > + return copy_event_location (&tmp); > +} > + > /* Return a copy of the given SRC location. */ > > struct event_location * > @@ -104,6 +158,24 @@ copy_event_location (const struct event_location *src) > EVENT_LOCATION_ADDRESS (dst) = EVENT_LOCATION_ADDRESS (src); > break; > > + case EXPLICIT_LOCATION: > + if (EVENT_LOCATION_EXPLICIT (src)->source_filename != NULL) > + EVENT_LOCATION_EXPLICIT (dst)->source_filename > + = xstrdup (EVENT_LOCATION_EXPLICIT (src)->source_filename); > + > + if (EVENT_LOCATION_EXPLICIT (src)->function_name != NULL) > + EVENT_LOCATION_EXPLICIT (dst)->function_name > + = xstrdup (EVENT_LOCATION_EXPLICIT (src)->function_name); > + > + if (EVENT_LOCATION_EXPLICIT (src)->label_name != NULL) > + EVENT_LOCATION_EXPLICIT (dst)->label_name > + = xstrdup (EVENT_LOCATION_EXPLICIT (src)->label_name); > + > + EVENT_LOCATION_EXPLICIT (dst)->line_offset > + = EVENT_LOCATION_EXPLICIT (src)->line_offset; > + break; > + > + > case PROBE_LOCATION: > EVENT_LOCATION_PROBE (dst) = xstrdup (EVENT_LOCATION_PROBE (src)); > break; > @@ -115,6 +187,87 @@ copy_event_location (const struct event_location *src) > return dst; > } > > +/* This convenience function returns a malloc'd string which > + represents the location in EXPLICIT. > + > + AS_LINESPEC is non-zero if this string should be a linespec. > + Otherwise it will be output in explicit form. */ > + > +static char * > +explicit_to_string_internal (int as_linespec, > + const struct explicit_location *explicit) > +{ > + struct ui_file *buf; > + char space, *result; > + int need_space = 0; > + > + space = as_linespec ? ':' : ' '; > + buf = mem_fileopen (); > + > + if (explicit->source_filename != NULL) > + { > + if (!as_linespec) > + fputs_unfiltered ("-source ", buf); > + fputs_unfiltered (explicit->source_filename, buf); > + need_space = 1; > + } > + > + if (explicit->function_name != NULL) > + { > + if (need_space) > + fputc_unfiltered (space, buf); > + if (!as_linespec) > + fputs_unfiltered ("-function ", buf); > + fputs_unfiltered (explicit->function_name, buf); > + need_space = 1; > + } > + > + if (explicit->label_name != NULL) > + { > + if (need_space) > + fputc_unfiltered (space, buf); > + if (!as_linespec) > + fputs_unfiltered ("-label ", buf); > + fputs_unfiltered (explicit->label_name, buf); > + need_space = 1; > + } > + > + if (explicit->line_offset.sign != LINE_OFFSET_UNKNOWN) > + { > + if (need_space) > + fputc_unfiltered (space, buf); > + if (!as_linespec) > + fputs_unfiltered ("-line ", buf); > + fprintf_filtered (buf, "%s%d", > + (explicit->line_offset.sign == LINE_OFFSET_NONE ? "" > + : (explicit->line_offset.sign > + == LINE_OFFSET_PLUS ? "+" : "-")), > + explicit->line_offset.offset); > + } > + > + result = ui_file_xstrdup (buf, NULL); > + ui_file_delete (buf); > + return result; > +} > + > +/* Return a malloc'd explicit string representation of the given > + explicit location. The location must already be canonicalized/valid. */ > + > +char * > +explicit_location_to_string (const struct explicit_location *explicit) > +{ > + return explicit_to_string_internal (0, explicit); > +} > + > +/* Return a malloc'd linespec string representation of the given > + explicit location. The location must already be canonicalized/valid. */ > + > +char * > +explicit_location_to_linespec (const struct explicit_location *explicit) > +{ > + return explicit_to_string_internal (1, explicit); > +} > + > /* A cleanup function for struct event_location. */ > > static void > @@ -152,6 +305,12 @@ delete_event_location (struct event_location *location) > /* Nothing to do. */ > break; > > + case EXPLICIT_LOCATION: > + xfree (EVENT_LOCATION_EXPLICIT (location)->source_filename); > + xfree (EVENT_LOCATION_EXPLICIT (location)->function_name); > + xfree (EVENT_LOCATION_EXPLICIT (location)->label_name); > + break; > + > case PROBE_LOCATION: > xfree (EVENT_LOCATION_PROBE (location)); > break; > @@ -189,6 +348,10 @@ event_location_to_string_const (const struct event_location *location) > core_addr_to_string (EVENT_LOCATION_ADDRESS (location))); > break; > > + case EXPLICIT_LOCATION: > + result = explicit_location_to_string (EVENT_LOCATION_EXPLICIT (location)); > + break; > + > case PROBE_LOCATION: > result = xstrdup (EVENT_LOCATION_PROBE (location)); > break; > @@ -274,6 +437,14 @@ event_location_empty_p (const struct event_location *location) > case ADDRESS_LOCATION: > return 0; > > + case EXPLICIT_LOCATION: > + return (EVENT_LOCATION_EXPLICIT (location) == NULL > + || (EVENT_LOCATION_EXPLICIT (location)->source_filename == NULL > + && EVENT_LOCATION_EXPLICIT (location)->function_name == NULL > + && EVENT_LOCATION_EXPLICIT (location)->label_name == NULL > + && (EVENT_LOCATION_EXPLICIT (location)->line_offset.sign > + == LINE_OFFSET_UNKNOWN))); > + > case PROBE_LOCATION: > return EVENT_LOCATION_PROBE (location) == NULL; > > diff --git a/gdb/location.h b/gdb/location.h > index e6f14d9..709d6f8 100644 > --- a/gdb/location.h > +++ b/gdb/location.h > @@ -21,6 +21,32 @@ > > struct language_defn; > > +/* An enumeration of possible signs for a line offset. */ > + > +enum offset_relative_sign > +{ > + /* No sign */ > + LINE_OFFSET_NONE, > + > + /* A plus sign ("+") */ > + LINE_OFFSET_PLUS, > + > + /* A minus sign ("-") */ > + LINE_OFFSET_MINUS, > + > + /* A special "sign" for unspecified offset. */ > + LINE_OFFSET_UNKNOWN > +}; > + > +/* A line offset in a location. */ > + > +struct line_offset > +{ > + /* Line offset and any specified sign. */ > + int offset; > + enum offset_relative_sign sign; > +}; > + > /* An enumeration of the various ways to specify a stop event > location (used with create_breakpoint). */ > > @@ -32,10 +58,35 @@ enum event_location_type > /* An address in the inferior. */ > ADDRESS_LOCATION, > > + /* An explicit location. */ > + EXPLICIT_LOCATION, > + > /* A probe location. */ > PROBE_LOCATION > }; > > +/* An explicit location. This structure is used to bypass the > + parsing done on linespecs. It still has the same requirements > + as linespecs, though. For example, source_filename requires > + at least one other field. */ > + > +struct explicit_location > +{ > + /* The source filename. Malloc'd. */ > + char *source_filename; > + > + /* The function name. Malloc'd. */ > + char *function_name; > + > + /* The name of a label. Malloc'd. */ > + char *label_name; > + > + /* A line offset relative to the start of the symbol > + identified by the above fields or the current symtab > + if the other fields are NULL. */ > + struct line_offset line_offset; > +}; > + > /* An event location used to set a stop event in the inferior. > This structure is an amalgam of the various ways > to specify where a stop event should be set. */ > @@ -58,6 +109,10 @@ struct event_location > /* An address in the inferior. */ > CORE_ADDR address; > #define EVENT_LOCATION_ADDRESS(S) ((S)->u.address) > + > + /* An explicit location. */ > + struct explicit_location explicit; > +#define EVENT_LOCATION_EXPLICIT(S) (&((S)->u.explicit)) > } u; > > /* Cached string representation of this location. This is used to > @@ -65,6 +120,18 @@ struct event_location > char *as_string; > }; > > +/* Return a malloc'd explicit string representation of the given > + explicit location. The location must already be canonicalized/valid. */ > + > +extern char * > + explicit_location_to_string (const struct explicit_location *explicit); > + > +/* Return a malloc'd linespec string representation of the given > + explicit location. The location must already be canonicalized/valid. */ > + > +extern char * > + explicit_location_to_linespec (const struct explicit_location *explicit); > + > /* Return a string representation of the LOCATION. > This function may return NULL for unspecified linespecs, > e.g, EVENT_LOCATION_LINESPEC and addr_string is NULL. > @@ -108,6 +175,15 @@ extern struct event_location * > extern struct event_location * > new_probe_location (const char *probe); > > +/* Create a new explicit location. If not NULL, EXPLICIT is checked for > + validity. If invalid, an exception is thrown. > + > + The return result is malloc'd and should be freed with > + delete_event_location. */ Copy of function comment in .c file. Delete one. > + > +extern struct event_location * > + new_explicit_location (const struct explicit_location *explicit); > + > /* Return a copy of the given SRC location. */ > > extern struct event_location * > @@ -118,6 +194,10 @@ extern struct event_location * > extern void initialize_event_location (struct event_location *location, > enum event_location_type type); > > +/* Initialize the given explicit location. */ > + > +extern void initialize_explicit_location (struct explicit_location *explicit); > + > /* Attempt to convert the input string in *ARGP into an event location. > ARGP is advanced past any processed input. Returns a event_location > (malloc'd) if an event location was successfully found in *ARGP,