[RFA 7/9] Explicit locations v2 - CLI for explicit locations

Doug Evans dje@google.com
Wed Aug 6 04:46:00 GMT 2014


Keith Seitz writes:
 > This is also an update to this patch based on reviews of earlier patches.
 > 
 > One additional hunk (mentioned below) was added to tests for when glibc 
 > debuginfo is installed.
 > 
 > Keith
 > 
 > Changes since last version:
 >    - use make_cleanup_delete_event_location
 >    - string_to_explicit_location remove SAVE_SPEC
 >    - Add additional completion test pattern for when glibc debuginfo
 >      installed
 > 
 > ChangeLog
 > 2014-05-29  Keith Seitz  <keiths@redhat.com>
 > 
 > 	* completer.c: Include location.h.
 > 	(enum match_type): New enum.
 > 	(location_completer): Rename to ...
 > 	(linespec_completer): ... this.
 > 	(collect_explicit_location_matches): New function.
 > 	(explicit_location_completer): New function.
 > 	(location_completer): "New" function; handle linespec
 > 	and explicit location completions.
 > 	(complete_line_internal): Remove all location completer-specific
 > 	handling.
 > 	* linespec.c (linespec_lexer_lex_keyword): Export.
 > 	(is_ada_operator): Ditto.
 > 	(find_toplevel_char): Ditto.
 > 	(linespec_parse_line_offset): Ditto.
 > 	Issue error if STRING is not numerical.
 > 	(gdb_get_linespec_parser_quote_characters): New function.
 > 	* linespec.h (linespec_parse_line_offset): Declare.
 > 	(location_completer): Declare.
 > 	(get_gdb_linespec_parser_quote_characters): Declare.
 > 	(is_ada_operator): Declare.
 > 	(find_toplevel_char): Declare.
 > 	* location.c (explicit_to_event_location): New function.
 > 	(explicit_location_lex_one): New function.
 > 	(string_to_explicit_location): New function.
 > 	(string_to_event_location): Handle explicit locations.
 > 	* location.h (explicit_to_event_location): Declare.
 > 	(string_to_explicit_location): Declare.
 > 
 > testsuite/ChangeLog
 > 2014-05-29  Keith Seitz  <keiths@redhat.com>
 > 
 > 	* gdb.base/save-bp.exp: Add tests for address locations
 > 	and explicit locations, pending and not-pending, with and
 > 	without conditions, including resolved pending breakpoints.
 > 	* gdb.linespec/3explicit.c: New file.
 > 	* gdb.linespec/cpexplicit.cc: New file.
 > 	* gdb.linespec/cpexplicit.exp: New file.
 > 	* gdb.linespec/explicit.c: New file.
 > 	* gdb.linespec/explicit.exp: New file.
 > 	* gdb.linespec/explicit2.c: New file.
 > 	* gdb.linespec/ls-errs.exp: Add explicit location tests.
 > 
 > diff --git a/gdb/completer.c b/gdb/completer.c
 > index 94f70a9..e26cc2d 100644
 > --- a/gdb/completer.c
 > +++ b/gdb/completer.c
 > @@ -25,6 +25,7 @@
 >  #include "gdb_assert.h"
 >  #include "exceptions.h"
 >  #include "gdb_signals.h"
 > +#include "location.h"
 >  
 >  #include "cli/cli-decode.h"
 >  
 > @@ -41,6 +42,21 @@
 >  
 >  #include "completer.h"
 >  
 > +/* An enumeration of the various things a user might
 > +   attempt to complete for a location.  */
 > +
 > +enum explicit_location_match_type
 > +{
 > +    /* The filename of a source file.  */
 > +    MATCH_SOURCE,
 > +
 > +    /* The name of a function or method.  */
 > +    MATCH_FUNCTION,
 > +
 > +    /* The name of a label.  */
 > +    MATCH_LABEL
 > +};
 > +
 >  /* Prototypes for local functions.  */
 >  static
 >  char *line_completion_function (const char *text, int matches, 
 > @@ -173,7 +189,7 @@ filename_completer (struct cmd_list_element *ignore,
 >    return return_val;
 >  }
 >  
 > -/* Complete on locations, which might be of two possible forms:
 > +/* Complete on linespecs, which might be of two possible forms:
 >  
 >         file:line
 >     or
 > @@ -182,9 +198,9 @@ filename_completer (struct cmd_list_element *ignore,
 >     This is intended to be used in commands that set breakpoints
 >     etc.  */
 >  
 > -VEC (char_ptr) *
 > -location_completer (struct cmd_list_element *ignore, 
 > -		    const char *text, const char *word)
 > +static VEC (char_ptr) *
 > +linespec_location_completer (struct cmd_list_element *ignore,
 > +			     const char *text, const char *word)
 >  {
 >    int n_syms, n_files, ix;
 >    VEC (char_ptr) *fn_list = NULL;
 > @@ -331,6 +347,169 @@ location_completer (struct cmd_list_element *ignore,
 >    return list;
 >  }
 >  
 > +/* A helper function to collect explicit location matches for the given
 > +   LOCATION, which is attempting to match on WHICH.  */

s/WHICH/WORD/ ?

 > +
 > +static VEC (char_ptr) *
 > +collect_explicit_location_matches (struct event_location *location,
 > +				   enum explicit_location_match_type what,
 > +				   const char *word)
 > +{
 > +  VEC (char_ptr) *matches = NULL;
 > +  struct explicit_location *explicit = EVENT_LOCATION_EXPLICIT (location);
 > +
 > +  switch (what)
 > +    {
 > +    case MATCH_SOURCE:
 > +      {
 > +	const char *text = (explicit->source_filename == NULL
 > +			    ? "" : explicit->source_filename);
 > +
 > +	matches = make_source_files_completion_list (text, word);
 > +      }
 > +      break;
 > +
 > +    case MATCH_FUNCTION:
 > +      {
 > +	const char *text = (explicit->function_name == NULL
 > +			    ? "" : explicit->function_name);
 > +
 > +	if (explicit->source_filename != NULL)
 > +	  matches
 > +	    = make_file_symbol_completion_list (text, word,
 > +						explicit->source_filename);

Wrap if/else/etc clauses longer than one line in { }.

 > +	else
 > +	  matches = make_symbol_completion_list (text, word);
 > +      }
 > +      break;
 > +
 > +    case MATCH_LABEL:
 > +      /* Not supported.  */
 > +      break;
 > +
 > +    default:
 > +      gdb_assert_not_reached (_("unhandled which_explicit"));

"unhandled explicit_location_match_type" ?

 > +    }
 > +
 > +  return matches;
 > +}
 > +
 > +/* A convenience macro to (safely) back up P to the previous
 > +   word.  */
 > +
 > +#define BACKUP(P,S)					\

I'd prefer this be a function.

 > +  do							\
 > +    {							\
 > +      while ((P) > (S) && isspace (*(P)))		\
 > +	--(P);						\
 > +      for (; (P) > (S) && !isspace ((P)[-1]); --(P))	\
 > +	;						\
 > +    }							\
 > +  while (0)
 > +
 > +static VEC (char_ptr) *
 > +explicit_location_completer (struct cmd_list_element *ignore,
 > +			     struct event_location *location,
 > +			     const char *text, const char *word)
 > +{
 > +  const char *p;
 > +  VEC (char_ptr) *matches = NULL;
 > +
 > +  /* Find the beginning of the word.  This is necessary because
 > +     we need to know if we are completing an option name or value.  We
 > +     don't get the leading '-' from the completer.  */
 > +  p = word;
 > +  BACKUP (p, text);
 > +
 > +  if (*p == '-')
 > +    {
 > +      size_t len = strlen (word);
 > +
 > +      /* Completing on option name.  */
 > +      if (strncmp (word, "source", len) == 0)
 > +	VEC_safe_push (char_ptr, matches, xstrdup ("source"));
 > +      else if (strncmp (word, "function", len) == 0)
 > +	VEC_safe_push (char_ptr, matches, xstrdup ("function"));
 > +      else if (*word == 'l')
 > +	{
 > +	  if (strncmp (word, "line", len) == 0)
 > +	    VEC_safe_push (char_ptr, matches, xstrdup ("line"));
 > +	  if (strncmp (word, "label", len) == 0)
 > +	    VEC_safe_push (char_ptr, matches, xstrdup ("label"));
 > +	}

Keeping this style (special casing multiple words that start with
the same letter) over time adds a bit more complexity than necessary.
Although "suboptimal" a series of if's and no else's would be simpler.

btw, What happens if one tries to complete on "-li"?
Where does "word" point in this case?
[Should all the strncmp's use p+1 instead of word?]

 > +    }
 > +  else
 > +    {
 > +      /* Completing on value (or unknown).  Get the previous word to see what
 > +	 the user is completing on.  */
 > +      size_t len, offset;
 > +      const char *new_word, *end;
 > +      enum explicit_location_match_type what;
 > +      struct explicit_location *explicit = EVENT_LOCATION_EXPLICIT (location);
 > +
 > +      /* Backup P to the previous word, which should be the option
 > +	 the user is attempting to complete.  */
 > +      offset = word - p;
 > +      end = --p;
 > +      BACKUP (p, text);
 > +      len = end - p;
 > +
 > +      if (strncmp (p, "-source", len) == 0)
 > +	{
 > +	  what = MATCH_SOURCE;
 > +	  new_word = explicit->source_filename + offset;
 > +	}
 > +      else if (strncmp (p, "-function", len) == 0)
 > +	{
 > +	  what = MATCH_FUNCTION;
 > +	  new_word = explicit->function_name + offset;
 > +	}
 > +      else if (strncmp (p, "-label", len) == 0)
 > +	{
 > +	  what = MATCH_LABEL;
 > +	  new_word = explicit->label_name + offset;
 > +	}
 > +      else
 > +	{
 > +	  /* The user isn't completing on any valid option name,
 > +	     e.g., "break -source foo.c [tab]".  */
 > +	  return NULL;
 > +	}
 > +
 > +      /* Now gather matches  */
 > +      matches = collect_explicit_location_matches (location, what, new_word);
 > +    }
 > +
 > +  return matches;
 > +}

I didn't parse this logic with a fine-toothed comb.
I'm assuming it's correct (for now at least).

 > +
 > +/* A completer for locations.  */
 > +
 > +VEC (char_ptr) *
 > +location_completer (struct cmd_list_element *ignore,
 > +		    const char *text, const char *word)
 > +{
 > +  VEC (char_ptr) *matches = NULL;
 > +  const char *copy = text;
 > +  struct event_location *location;
 > +
 > +  location = string_to_explicit_location (&copy, current_language, 1);
 > +  if (location != NULL)
 > +    {
 > +      matches = explicit_location_completer (ignore, location, text, word);
 > +      delete_event_location (location);

Do we know if there are absolutely zero occasions in which
explicit_location_completer can throw an exception?
Or do we need to free location via a cleanup?

 > +    }
 > +  else
 > +    {
 > +      /* This is an address or linespec location.
 > +	 Right now both of these are handled by the (old) linespec
 > +	 completer.  */
 > +      matches = linespec_location_completer (ignore, text, word);
 > +    }
 > +
 > +  return matches;
 > +}
 > +
 >  /* Helper for expression_completer which recursively adds field and
 >     method names from TYPE, a struct or union type, to the array
 >     OUTPUT.  */
 > @@ -668,16 +847,6 @@ complete_line_internal (const char *text,
 >  		      rl_completer_word_break_characters =
 >  			gdb_completer_file_name_break_characters;
 >  		    }
 > -		  else if (c->completer == location_completer)
 > -		    {
 > -		      /* Commands which complete on locations want to
 > -			 see the entire argument.  */
 > -		      for (p = word;
 > -			   p > tmp_command
 > -			     && p[-1] != ' ' && p[-1] != '\t';
 > -			   p--)
 > -			;
 > -		    }

Removing this code is sure nice to see. :-)

 >  		  if (reason != handle_brkchars && c->completer != NULL)
 >  		    list = (*c->completer) (c, p, word);
 >  		}
 > @@ -743,14 +912,6 @@ complete_line_internal (const char *text,
 >  		  rl_completer_word_break_characters =
 >  		    gdb_completer_file_name_break_characters;
 >  		}
 > -	      else if (c->completer == location_completer)
 > -		{
 > -		  for (p = word;
 > -		       p > tmp_command
 > -			 && p[-1] != ' ' && p[-1] != '\t';
 > -		       p--)
 > -		    ;
 > -		}
 >  	      if (reason != handle_brkchars && c->completer != NULL)
 >  		list = (*c->completer) (c, p, word);
 >  	    }
 > diff --git a/gdb/linespec.c b/gdb/linespec.c
 > index f141a86..aa29e64 100644
 > --- a/gdb/linespec.c
 > +++ b/gdb/linespec.c
 > @@ -328,8 +328,6 @@ static int compare_symbols (const void *a, const void *b);
 >  
 >  static int compare_msymbols (const void *a, const void *b);
 >  
 > -static const char *find_toplevel_char (const char *s, char c);
 > -
 >  /* Permitted quote characters for the parser.  This is different from the
 >     completer's quote characters to allow backward compatibility with the
 >     previous parser.  */
 > @@ -379,7 +377,7 @@ linespec_lexer_lex_number (linespec_parser *parser, linespec_token *tokenp)
 >  /* Does P represent one of the keywords?  If so, return
 >     the keyword.  If not, return NULL.  */
 >  
 > -static const char *
 > +const char *
 >  linespec_lexer_lex_keyword (const char *p)
 >  {
 >    int i;
 > @@ -405,7 +403,7 @@ linespec_lexer_lex_keyword (const char *p)
 >  /* Does STRING represent an Ada operator?  If so, return the length
 >     of the decoded operator name.  If not, return 0.  */

Duplicate function comment with what's in linespec.h.
[Probably others as well.  Either remove the comment from linespec.h
or replace this one with "See linespec.h."
I know current thinking is to put the comments in the header,
but I don't insist on it.]

 >  
 > -static int
 > +int
 >  is_ada_operator (const char *string)
 >  {
 >    const struct ada_opname_map *mapping;
 > @@ -1118,7 +1116,7 @@ find_methods (struct type *t, const char *name,
 >     strings.  Also, ignore the char within a template name, like a ','
 >     within foo<int, int>.  */
 >  
 > -static const char *
 > +const char *
 >  find_toplevel_char (const char *s, char c)
 >  {
 >    int quoted = 0;		/* zero if we're not in quotes;
 > @@ -1531,9 +1529,10 @@ source_file_not_found_error (const char *name)
 >  
 >  /* Parse and return a line offset in STRING.  */
 >  
 > -static struct line_offset
 > +struct line_offset
 >  linespec_parse_line_offset (const char *string)
 >  {
 > +  const char *start = string;
 >    struct line_offset line_offset = {0, LINE_OFFSET_NONE};
 >  
 >    if (*string == '+')
 > @@ -1547,6 +1546,9 @@ linespec_parse_line_offset (const char *string)
 >        ++string;
 >      }
 >  
 > +  if (!isdigit (*string))
 > +    error (_("malformed line offset: \"%s\""), start);
 > +

This is independent of this patch, right?
If so, PLEASE don't move it to another patch on my account.
Just checking.
[For this patch I don't want to be too pedantic!]

 >    /* Right now, we only allow base 10 for offsets.  */
 >    line_offset.offset = atoi (string);
 >    return line_offset;
 > @@ -3790,3 +3792,11 @@ make_cleanup_destroy_linespec_result (struct linespec_result *ls)
 >  {
 >    return make_cleanup (cleanup_linespec_result, ls);
 >  }
 > +
 > +/* Return the quote characters permitted by the linespec parser.  */
 > +
 > +const char *
 > +get_gdb_linespec_parser_quote_characters (void)
 > +{
 > +  return linespec_quote_characters;
 > +}
 > diff --git a/gdb/linespec.h b/gdb/linespec.h
 > index df5820a..87a678d 100644
 > --- a/gdb/linespec.h
 > +++ b/gdb/linespec.h
 > @@ -152,6 +152,36 @@ extern struct symtabs_and_lines decode_line_with_current_source (char *, int);
 >  
 >  extern struct symtabs_and_lines decode_line_with_last_displayed (char *, int);
 >  
 > +/* Parse a line offset from STRING.  */
 > +
 > +extern struct line_offset linespec_parse_line_offset (const char *string);
 > +
 > +/* A completion handler for locations.  */
 > +
 > +VEC (char_ptr) *location_completer (struct cmd_list_element *ignore,
 > +				    const char *text, const char *word);
 > +
 > +/* Return the quote characters permitted by the linespec parser.  */
 > +
 > +extern const char *get_gdb_linespec_parser_quote_characters (void);
 > +
 > +/* Does STRING represent an Ada operator?  If so, return the length
 > +   of the decoded operator name.  If not, return 0.  */
 > +
 > +extern int is_ada_operator (const char *string);
 > +
 > +/* Find an instance of the character C in the string S that is outside
 > +   of all parenthesis pairs, single-quoted strings, and double-quoted
 > +   strings.  Also, ignore the char within a template name, like a ','
 > +   within foo<int, int>.  */
 > +
 > +extern const char *find_toplevel_char (const char *s, char c);
 > +
 > +/* Does P represent one of the keywords?  If so, return
 > +   the keyword.  If not, return NULL.  */
 > +
 > +extern const char *linespec_lexer_lex_keyword (const char *p);
 > +
 >  /* Evaluate the expression pointed to by EXP_PTR into a CORE_ADDR,
 >     advancing EXP_PTR past any parsed text.  */
 >  
 > diff --git a/gdb/location.c b/gdb/location.c
 > index 86d4232..cfcbc46 100644
 > --- a/gdb/location.c
 > +++ b/gdb/location.c
 > @@ -379,6 +379,214 @@ event_location_to_string (struct event_location *location)
 >    return location->as_string;
 >  }
 >  
 > +/* A lexer for explicit locations.  This function will advance INP
 > +   past any strings that it lexes.  Returns a malloc'd copy of the
 > +   lexed string or NULL if no lexing was done.  */
 > +
 > +static char *
 > +explicit_location_lex_one (const char **inp,
 > +			   const struct language_defn *language)
 > +{
 > +  const char *start = *inp;
 > +
 > +  if (*start != '\0')

This is one big "if" that consumes most of the function.
Seems like it would simplify things to just early exit if *start == '\0'.

 > +    {
 > +      /* If quoted, skip to the ending quote.  */
 > +      if (strchr (get_gdb_linespec_parser_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 (!(language->la_language == language_ada
 > +		&& quote_char == '\"' && is_ada_operator (start)))
 > +	    {
 > +	      const char *end = find_toplevel_char (start + 1, quote_char);
 > +
 > +	      if (end == NULL)
 > +		error (_("Unmatched quote,  %s."), start);

extra space after ,

 > +	      *inp = end + 1;
 > +	      return savestring (start + 1, *inp - start - 2);
 > +	    }
 > +	}
 > +
 > +      /* If the input starts with '-' or '+', the string ends with the next
 > +	 whitespace.  */
 > +      if (*start == '-' || *start == '+')
 > +	*inp = skip_to_space_const (*inp);
 > +      else
 > +	{
 > +	  /* Handle numbers first, stopping at the next whitespace or ','.  */
 > +	  while (isdigit (*inp[0]))
 > +	    ++(*inp);
 > +	  if (*inp[0] == '\0' || isspace (*inp[0]) || *inp[0] == ',')
 > +	    return savestring (start, *inp - start);
 > +
 > +	  /* Otherwise stop at the next occurrence of "SPACE -", '\0',
 > +	     keyword, or ','.  */
 > +	  *inp = start;
 > +	  while ((*inp)[0]
 > +		 && (*inp)[0] != ','
 > +		 && !(isspace ((*inp)[0])
 > +		      && ((*inp)[1] == '-'
 > +			  || linespec_lexer_lex_keyword (&(*inp)[1]))))
 > +	    {
 > +	      /* Special case: C++ operator,.  */
 > +	      if (language->la_language == language_cplus
 > +		  && strncmp (*inp, "operator", 8)
 > +		  && (*inp)[9] == ',')
 > +		(*inp) += 9;
 > +	      ++(*inp);
 > +	    }
 > +	}
 > +    }
 > +
 > +  if (*inp - start > 0)
 > +    return savestring (start, *inp - start);
 > +
 > +  return NULL;
 > +}
 > +
 > +/* Attempt to convert the input string in *ARGP into an explicit location.
 > +   ARGP is advanced past any processed input.  Returns an event_location

*ARGP is advanced past ...

 > +   (malloc'd) if an explicit location was successfully found in *ARGP,
 > +   NULL otherwise.
 > +
 > +   IF !DONT_THROW, this function may call error() if *ARGP looks like
 > +   properly formed input, e.g., if it is called with missing argument
 > +   parameters or invalid  options.  If DONT_THROW is non-zero, this function

extra space after invalid

 > +   will not throw any exceptions.  */
 > +
 > +struct event_location *
 > +string_to_explicit_location (const char **argp,
 > +			     const struct language_defn *language,
 > +			     int dont_throw)
 > +{
 > +  /* It is assumed that input beginning with '-' and a non-digit
 > +     character is an explicit location.  */
 > +  if (argp != NULL && *argp != NULL)

The function comment doesn't indicate that argp can be NULL.
Can it?
If it can be NULL I'd just early exit.

Also, this is another large "if" that covers most of
the function.  I rarely find these easier to read than
those that early exit and remove the extra level of
indentation (especially as the size of the function grows).

 > +    {
 > +      const char *copy, *orig;
 > +
 > +      orig = copy = *argp;
 > +      if ((*argp[0] == '-' && isalpha ((*argp)[1])))

Is this another early exit opportunity?

 > +	{
 > +	  char *s, *str;
 > +	  struct cleanup *cleanup;
 > +	  struct event_location *location;
 > +
 > +	  location = new_explicit_location (NULL);
 > +	  cleanup = make_cleanup_delete_event_location (location);
 > +
 > +	  /* Process option/argument pairs.  dprintf_command
 > +	     requires that processing stop on ','.  */
 > +	  while ((*argp)[0] != '\0' && (*argp)[0] != ',')
 > +	    {
 > +	      int len;
 > +	      char *opt, *oarg;
 > +	      const char *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_location_lex_one (argp, language);
 > +	      inner = make_cleanup (xfree, opt);
 > +
 > +	      /* Skip any whitespace.  */

One can argue this comment and the one below (and any others)
are unnecessary.  In this particular case I don't mind them,
but I'm sure others will so might as well delete them.

 > +	      *argp = skip_spaces_const (*argp);
 > +
 > +	      /* Get the argument string.  */
 > +	      oarg = explicit_location_lex_one (argp, language);
 > +
 > +	      /* Skip any whitespace.  */
 > +	      *argp = skip_spaces_const (*argp);
 > +
 > +	      /* Use the length of the option to allow abbreviations.  */
 > +	      len = strlen (opt);

Is "-" by itself flagged as an error?
[e.g., if the user (mis)typed "- source mumble"]

 > +
 > +	      /* All options have a required argument.  */

It would be good to add a comment here saying the test to
ensure oarg is non-NULL is done later.
The reader at this point will be wondering why we're assigning
oarg which may be NULL even though it's a required argument.

 > +	      if (strncmp (opt, "-source", len) == 0)
 > +		EVENT_LOCATION_EXPLICIT (location)->source_filename = oarg;
 > +	      else if (strncmp (opt, "-function", len) == 0)
 > +		EVENT_LOCATION_EXPLICIT (location)->function_name = oarg;
 > +	      else if (strncmp (opt, "-line", len) == 0)
 > +		{
 > +		  if (oarg != NULL)
 > +		    {
 > +		      volatile struct gdb_exception e;
 > +
 > +		      TRY_CATCH (e, RETURN_MASK_ERROR)
 > +			{
 > +			  EVENT_LOCATION_EXPLICIT (location)->line_offset
 > +			    = linespec_parse_line_offset (oarg);
 > +			}
 > +
 > +		      xfree (oarg);
 > +		      if (e.reason < 0 && !dont_throw)
 > +			throw_exception (e);
 > +		    }
 > +		}
 > +	      else if (strncmp (opt, "-label", len) == 0)
 > +		EVENT_LOCATION_EXPLICIT (location)->label_name = oarg;
 > +	      /* Only emit an "invalid argument" error for options
 > +		 that look like option strings.  */
 > +	      else if (opt[0] == '-' && !isdigit (opt[1]))
 > +		{
 > +		  xfree (oarg);
 > +		  if (!dont_throw)
 > +		    {
 > +		      error (_("invalid explicit location argument, \"%s\""),
 > +			     opt);
 > +		    }
 > +		}
 > +	      else
 > +		{

Still need to xfree (oarg) here?

 > +		  /* Trailing garbage.  This will be handled by
 > +		     one of the callers.  */

The caller still gets a location object, and yet we've rewound.
This is a bit confusing.  Can you clarify?

Also, do we need to do_cleanups (inner) before breaking out
of the while loop?

 > +		  *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 argument" error before any missing
 > +		 argument error.  */
 > +	      if (oarg == NULL && !dont_throw)
 > +		error (_("missing argument for \"%s\""), opt);
 > +
 > +	      /* The option/argument pari was successfully processed;
 > +		 oarg belongs to the explicit location, and opt should
 > +		 be freed.  */
 > +	      do_cleanups (inner);
 > +	    }
 > +
 > +	  /* One special error check:  If a source filename was given
 > +	     without offset, function, or label, issue an error.  */
 > +	  if (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)
 > +	      && !dont_throw)
 > +	    error (_("Source filename requires function, label, or "
 > +		     "line offset."));
 > +
 > +	  discard_cleanups (cleanup);
 > +	  return location;
 > +	}
 > +    }
 > +
 > +  /* Not an explicit location.  */
 > +  return NULL;
 > +}
 > +
 >  /* Parse the user input in *STRINGP and turn it into a struct
 >     event_location, advancing STRINGP past any parsed input.
 >     Return value is malloc'd.  */
 > @@ -414,10 +622,24 @@ string_to_event_location (char **stringp,
 >  	}
 >        else
 >  	{
 > -	  /* Everything else is a linespec.  */
 > -	  location = new_linespec_location (*stringp);
 > -	  if (*stringp != NULL)
 > -	    *stringp += strlen (*stringp);
 > +	  const char *arg, *orig;
 > +
 > +	  /* Next, try an explicit location.  */
 > +	  orig = arg = *stringp;
 > +	  location = string_to_explicit_location (&arg, language, 0);
 > +	  if (location != NULL)
 > +	    {
 > +	      /* It was a valid explicit location.  Advance STRINGP to
 > +		 the end of input.  */
 > +	      *stringp += arg - orig;
 > +	    }
 > +	  else
 > +	    {
 > +	      /* Everything else is a linespec.  */
 > +	      location = new_linespec_location (*stringp);
 > +	      if (*stringp != NULL)
 > +		*stringp += strlen (*stringp);

Having just read that dprintf requires explicit location parsing
to stop on ",", it's odd to see linespecs consume the entire line.
IOW, the different handling between the above "*stringp += arg - orig"
and this "*stringp += strlen (*stringp)" is confusing.
Can you add a clarifying comment to "*stringp += strlen (*stringp)"
explaining why it's done that way?

 > +	    }
 >  	}
 >      }
 >  
 > diff --git a/gdb/location.h b/gdb/location.h
 > index 709d6f8..54b11d1 100644
 > --- a/gdb/location.h
 > +++ b/gdb/location.h
 > @@ -215,4 +215,19 @@ extern struct event_location *
 >  
 >  extern int event_location_empty_p (const struct event_location *location);
 >  
 > +/* Attempt to convert the input string in *ARGP into an explicit location.
 > +   ARGP is advanced past any processed input.  Returns an event_location
 > +   (malloc'd) if an explicit location was successfully found in *ARGP,
 > +   NULL otherwise.
 > +
 > +   IF !DONT_THROW, this function may call error() if *ARGP looks like
 > +   properly formed input, e.g., if it is called with missing argument
 > +   parameters or invalid  options.  If DONT_THROW is non-zero, this function
 > +   will not throw any exceptions.  */

Duplicate function comment.

 > +
 > +extern struct event_location *
 > +	string_to_explicit_location (const char **argp,
 > +				     const struct language_defn *langauge,
 > +				     int dont_throw);
 > +
 >  #endif /* LOCATIONS_H */

I didn't review the attached testcases, I was at a good stopping point
and needed a break. :-)



More information about the Gdb-patches mailing list