This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH v4 7/9] Explicit locations: add UI features for CLI
- From: Doug Evans <xdje42 at gmail dot com>
- To: Keith Seitz <keiths at redhat dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Tue, 26 May 2015 21:26:42 -0700
- Subject: Re: [PATCH v4 7/9] Explicit locations: add UI features for CLI
- Authentication-results: sourceware.org; auth=none
- References: <20150507180523 dot 19629 dot 77846 dot stgit at valrhona dot uglyboxes dot com> <20150507180602 dot 19629 dot 40685 dot stgit at valrhona dot uglyboxes dot com> <m3wq06pe9j dot fsf at sspiff dot org> <555B9FF2 dot 4030203 at redhat dot com>
Keith Seitz <keiths@redhat.com> writes:
>>> +/* 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?
I'm just worried about the robustness (or lack thereof)
to future changes.
Plus the readability.
You've done the research to conclude that the code is ok,
(e.g., linespec_parse_line_offset doesn't call QUIT)
but the casual reader will be left with the question:
"Hmmm, can oarg leak?", and a conscientious reader would then
have to put in the time to do that research too.
It'd be preferable if the code could be read and
understood to be correct without having to do that research.