Re: [PATCH v4 7/9] Explicit locations: add UI features for CLI

[Keith Seitz <keiths@redhat.com>]

On 05/17/2015 11:54 PM, Doug Evans wrote:
> Keith Seitz <keiths@redhat.com> writes:
>>  
>> diff --git a/gdb/location.c b/gdb/location.c
>> index 7882b2d..779bcfa 100644
>> --- a/gdb/location.c
>> +++ b/gdb/location.c
>> @@ -442,6 +442,203 @@ event_location_to_string (struct event_location *location)
>>    return EL_STRING (location);
>>  }
>>  
>> +/* 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')
>> +    return NULL;
>> +
>> +  /* 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);
>> +	  *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);
> 
> I suspect this is just following what the existing code does,
> but why not also watch for commas when there's a leading +,-?
> If this is just following code and there's an issue here
> I'd leave it for another day to change.

Good catch. I've implemented this and added a few tests for it.

>> +/* See description in location.h.  */
>> +
>> +struct event_location *
>> +string_to_explicit_location (const char **argp,
>> +			     const struct language_defn *language,
>> +			     int dont_throw)
>> +{
>> +  struct cleanup *cleanup;
>> +  struct event_location *location;
>> +
>> +  /* It is assumed that input beginning with '-' and a non-digit
>> +     character is an explicit location.  */
>> +  if (argp == NULL
>> +      || *argp == '\0'
>> +      || *argp[0] != '-'
>> +      || !isalpha ((*argp)[1]))
>> +    return NULL;
>> +
>> +  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);
>> +
>> +      *argp = skip_spaces_const (*argp);
>> +
>> +      /* Get the argument string.  */
>> +      oarg = explicit_location_lex_one (argp, language);
>> +
>> +      *argp = skip_spaces_const (*argp);
>> +
>> +      /* Use the length of the option to allow abbreviations.  */
>> +      len = strlen (opt);
>> +
>> +      /* All options have a required argument.  Checking for this required
>> +	 argument is deferred until later.  */
>> +      if (strncmp (opt, "-source", len) == 0)
>> +	EL_EXPLICIT (location)->source_filename = oarg;
>> +      else if (strncmp (opt, "-function", len) == 0)
>> +	EL_EXPLICIT (location)->function_name = oarg;
>> +      else if (strncmp (opt, "-line", len) == 0)
>> +	{
>> +	  if (oarg != NULL)
>> +	    {
>> +	      TRY
>> +		{
>> +		  EL_EXPLICIT (location)->line_offset
>> +		    = linespec_parse_line_offset (oarg);
>> +		}
>> +	      CATCH (e, RETURN_MASK_ERROR)
>> +		{
>> +		  xfree (oarg);
> 
> Could other exception types leak oarg here?

Not that I can see. When any successful argument value is parsed, it is
added to the event_location, which has a cleanup on it which will free
any defined members when an exception occurs.

The only two functions (in this loop) that could throw an exception are
explicit_location_lex_one and linespec_parse_line_offset.

In the former case, the option name has a cleanup when parsing the
value. The value is either saved into the event_location or discarded if
we are going to throw an exception.

linespec_parse_line_offset can throw an error (GENERIC_ERROR), but it is
already caught and memory for oarg is freed. Nothing can generate a
RETURN_QUIT as far as I can tell. Did you have a case specifically in mind?

Keith