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

Keith Seitz
2015-05-19T20:41:27Z

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