[RFA 7/9] Explicit locations v2 - CLI for explicit locations
Keith Seitz
keiths@redhat.com
Wed Sep 3 19:33:00 GMT 2014
On 08/05/2014 09:46 PM, Doug Evans wrote:
> > +/* A helper function to collect explicit location matches for the given
> > + LOCATION, which is attempting to match on WHICH. */
>
> s/WHICH/WORD/ ?
:-) Fixed.
>
> > +
> > +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 { }.
Done.
> > + 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" ?
Better. Fixed.
>
> > + }
> > +
> > + 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.
Done.
> > +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.
Done.
> btw, What happens if one tries to complete on "-li"?
Um.. It works? :-)
> Where does "word" point in this case?
> [Should all the strncmp's use p+1 instead of word?]
Yes, indeed it should! I've adjusted the patch.
[snip]
> > + /* 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).
"I find your lack of faith disturbing." [obligatory Star Wars sarcasm]
Believe me when I say that I hope it is correct, too. I don't need more
bugs to fix!
> > +
> > +/* 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 (©, 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?
Probably safer to use a cleanup anyway. Fixed.
> > @@ -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. :-)
I didn't see the need for this kinda of specialization. IMO, it should
have gone into the completer function anyway.
> > @@ -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.]
I've fixed all of these, adding "See description in foo.h."
> > @@ -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!]
Not strictly, no. It is presently not possible to trigger this error via
the current linespec parser.
> > 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'.
Done.
>
> > + {
> > + /* 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 ,
Fixed.
> > +
> > +/* 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 ...
>
Fixed.
> > + (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
>
Fixed.
> > + 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.
Right, argp cannot be NULL, but there's no harm guarding against it.
Likewise *argp cannot be NULL (for an explicit location). Only linespec
locations may do that.
I've added an early exit for this.
> 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?
Done.
> > + {
> > + 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.
Done.
> > + *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"]
It is not an error, no, but it is not a valid explicit location. "-" by
itself (as a linespec) *is* valid (or at least it is currently). I've
added a test for this [and fixed a bug].
>
> > +
> > + /* 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.
Done.
> > + 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?
Yeah, there was a little mix-up. Fixed.
>
> > + /* 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?
>
After more testing, I have decided to handle this slightly differently.
In this case, we reset argp (to where it was before we attempt to find
the current option/value pair) and immediately return the found location
(if any). This now passes all my tests, and I think it will make more sense.
> Also, do we need to do_cleanups (inner) before breaking out
> of the while loop?
Yes.
> > @@ -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?
I've added a simple comment explaining that we need to "let the linespec
parser sort it out." While almost a comical comment, it is true.
Something could be done to clean this up, but I'd rather not add to what
is already an enormously complex patchset.
But say the word, and I will attempt it.
> > @@ -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.
Removed.
> > +
> > +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. :-)
I hear you!
Thank you for persevering!
Updated patch attached.
Keith
Changes since last revision:
- collect_explicit_location_matches: WHICH -> WORD
wrap multi-line stmts in {}
Change gdb_assert_not_reached text
- location_completer: use a cleanup for deleting location.
- explicit_lcoation_lex_one: early return for "" input
removed extra space in string
string_to_explicit_locaiton: advanced past *ARGP
removed extra space in comment.
add early escape
remove unused variables orig & copy & s & str.
found another problem with -line and oarg check at end of loop --
oarg was already freed!
- fixed duplicate function comments
- tring_to_explicit_location needs to return an explicit location.
The trailing garbage is handled by init_breakpoint_sal.
find_thread_and_condition in create_breakpoint.
If we found something that looks like an explicit location, we return
it and stop processing the string when we do. We *can't* know what
else may be around -- and more is permitted.
- remove duplicate headers from linespec.c
- *string = '\0' OKAY for linespec_parse_basic_offset.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: explicit-ui-cli.patch
Type: text/x-patch
Size: 50496 bytes
Desc: not available
URL: <http://sourceware.org/pipermail/gdb-patches/attachments/20140903/e31a9c97/attachment.bin>
More information about the Gdb-patches
mailing list