This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


On 03/01/2015 01:14 PM, Doug Evans wrote:
> Keith Seitz <keiths@redhat.com> writes:
>
>> +/* A helper function to collect explicit location matches for the given
>> +   LOCATION, which is attempting to match on WORD.  */
>> +
>> +static VEC (char_ptr) *
>> +collect_explicit_location_matches (struct event_location *location,
>> +				   enum explicit_location_match_type what,
>> +				   const char *word)
>> +{
[snip]
>> +    case MATCH_LABEL:
>> +      /* Not supported.  */
>> +      break;
>> +
>> +    default:
>> +      gdb_assert_not_reached (_("unhandled explicit_location_match_type"));
>
> ====
> No need for _() here.

Fixed.

>
>> +    }
>> +
>> +  return matches;
>> +}
>> +
>> +/* A convenience macro to (safely) back up P to the previous
>> +   word.  */
>
> ====
> This comment can be just one line.
> I'd rather a comment run past the soft limit (and still fit within the
> hard limit) than use an extra line just for one word.
>

Fixed.

>> +
>> +static const char *
>> +backup_text_ptr (const char *p, const char *text)
>> +{
>> +  while (p > text && isspace (*p))
>> +    --p;
>> +  for (; p > text && !isspace (p[-1]); --p)
>> +    ;
>> +
>> +  return p;
>> +}
>> +
>
> ====
> Missing function comment.
>

Doh! Added.

>> +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 = backup_text_ptr (word, text);
>> +
>> +  if (*p == '-')
>> +    {
>> +      size_t len = strlen (word);
>> +
>> +      /* Completing on option name.  */
>
> ====
> I couldn't get "b -<tab><tab>" to work. Can do?
>

Unfortunately, "-" is a valid linespec, so no, "b -<TAB><TAB>" will not
do anything.

>> +
>> +      /* Skip over the '-'.  */
>> +      ++p;
>> +
>> +      if (strncmp (p, "source", len) == 0)
[snip]
>> +      /* Now gather matches  */
>> +      matches = collect_explicit_location_matches (location, what, new_word);
>
> ====
> Something's not right here as "b -f <tab>" segv's because new_word is NULL.
>

Yes, indeed. The struct explicit_location uses NULL for unspecified
parameters, and these consequently get passed as new_word to
collect_explicit_location_matches, which passes it to GDB's completion
functions, which do NOT like NULL.

I've fixed this and added tests for them. Thank you for catching this
serious bug.

Keith


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]