Re: [PATCH] Fix problem handling colon in linespec, PR breakpoints/18303

[Don Breazeal <donb at codesourcery dot com>]

On 1/11/2016 2:34 PM, Doug Evans wrote:
> Keith Seitz writes:
>   > Hi,
>   >
>   > Thank you for pointing this out and supplying a patch (and *tests*!).
>   >
>   > On 01/08/2016 10:44 AM, Don Breazeal wrote:
>   >
>   > > During GDB's parsing of the linespec, when the filename was not found  
> in
>   > > the program's symbols, GDB tried to determine if the filename string
>   > > could be some other valid identifier.  Eventually it would get to a  
> point
>   > > where it concluded that the Windows logical volume, or whatever was to  
> the
>   > > left of the colon, was a C++ namespace.  When it found only one colon,  
> it
>   > > would assert.
>   >
>   > I have to admit, when I first read this, my reaction was that this is a
>   > symbol lookup error. After investigating it a bit, I think it is. [More
>   > rationale below.]
>   >
>   > > GDB's linespec grammar allows for several different linespecs that  
> contain
>   > > ':'.  The only one that has a number after the colon is  
> filename:linenum.
>   >
>   > True, but for how long? I don't know. The parser actually does/could (or
>   > for some brief time *did*) support offsets just about anywhere, e.g.,
>   > foo.c:function:label:3. That support was removed and legacy (buggy)
>   > behavior of ignoring the offset was maintained in the parser/sal
>   > converter. There is no reason we couldn't support it, though.
>   >
>   > > There is another possible solution.  After no symbols are found for the
>   > > file and we determine that it is a filename:linenum style linespec, we
>   > > could just consume the filename token and parse the rest of the
>   > > linespec.  That works as well, but there is no advantage to it.
>   >
>   > And, of course, I came up with an entirely different solution. :-)
>   >
>   > Essentially what is happening is that the linespec parser is calling
>   > lookup_symbol on the user input (e.g., "spaces: and :colons.cc"). That
>   > is causing several problems, all which assume the input is well-formed.
>   > As you've discovered, that is a pretty poor assumption. Malformed (user)
>   > input should not cause an assertion failure IMO.
>   >
>   > > I created two new tests for this.  One is just gdb.linespec/ls-errs.exp
>   > > copied and converted to use C++ instead of C, and to add the Windows
>   > > logical drive case.  The other is an MI test to provide a regression
>   > > test for the specific case reported in PR 18303.
>   >
>   > EXCELLENT! Thank you so much for doing this. These tests were
>   > outrageously useful while attempting to understand the problem.
>   >
>   > With that said, I offer *for discussion* this alternative solution,
>   > which removes the assumption that input to lookup_symbol is/must be
>   > well-formed.
>   >
>   > [There is one additional related/necessary fixlet snuck in here... The
>   > C++ name parser always assumed that ':' was followed by another ':'. Bad
>   > assumption. So it would return an incorrect result in the malformed  
> case.]
>   >
>   > WDYT?
>   >
>   > Keith
>   >
>   > [apologies on mailer wrapping]
>   >
>   > Author: Keith Seitz <keiths@redhat.com>
>   > Date:   Fri Jan 8 14:00:22 2016 -0800
>   >
>   > Tolerate malformed input for lookup_symbol-called functions.
>   >
>   > lookup_symbol is often called with user input. Consequently, any
>   > function called from lookup_symbol{,_in_language} should attempt to deal
>   > with malformed input gracefully. After all, malformed user input is
>   > not a programming/API error.
>   >
>   > This patch does not attempt to find/correct all instances of this.
>   > It only fixes locations in the code that trigger test suite failures.
>   >
>   >     gdb/ChangeLog
>   >
>   >     	* cp-namespace.c (cp_lookup_bare_symbol): Do not assert on
>   >     	user input.
>   >     	(cp_search_static_and_baseclasses): Return null_block_symbol for
>   >     	malformed input.
>   >     	Remove assertions.
>   >     	* cp-support.c (cp_find_first_component_aux): Do not return
>   >     	a prefix length for ':' unless the next character is also ':'.
>   >
>   > diff --git a/gdb/cp-namespace.c b/gdb/cp-namespace.c
>   > index 72002d6..aa225fe 100644
>   > --- a/gdb/cp-namespace.c
>   > +++ b/gdb/cp-namespace.c
>   > @@ -166,12 +166,6 @@ cp_lookup_bare_symbol (const struct language_defn
>   > *langdef,
>   >  {
>   >    struct block_symbol sym;
>   >
>   > -  /* Note: We can't do a simple assert for ':' not being in NAME because
>   > -     ':' may be in the args of a template spec.  This isn't intended to  
> be
>   > -     a complete test, just cheap and documentary.  */
>   > -  if (strchr (name, '<') == NULL && strchr (name, '(') == NULL)
>   > -    gdb_assert (strchr (name, ':') == NULL);
>   > -
> 
> Heya.
> 
> The assert is intended to catch (some) violations of this
> (from the function comment):
> 
>     NAME is guaranteed to not have any scope (no "::") in its name, though
>     if for example NAME is a template spec then "::" may appear in the
>     argument list.
> 
> This is an invariant on what name can (and cannot) be.
> IOW, it is wrong to call this function with name == "foo::bar",
> handling that is the caller's responsibility.
> Thus I think having an assert here is ok and good
> (as long as it tests for the correct thing!).
> 
> Whether it is ok to call this with name == "c:mumble" is the issue.
> [or even "c:::mumble" or whatever else a user could type that
> this function's caller isn't expected to handle]
> On that I'm kinda ambivalent, but I like having the assert
> watch for the stated invariant.
> 
> Thoughts?

Hi Doug

I don't think the change in question directly relates to the issue my
original patch was intended to address (PR 18303) -- with only the other
changes in Keith's patch, that problem is solved.  Maybe this change
could be dealt with in a separate patch?  Keith?

Regardless, I tried a bunch of different commands and didn't ever see a
"name" containing a ':' passed to cp_lookup_bare_symbol.  Is there a way
to make that happen?  If there is a way, it seems like this assertion:

gdb_assert (cp_entire_prefix_len (name) == 0);

would address the issue while still allowing "c:mumble".  In some cases
it would be a redundant call to cp_entire_prefix_len, but that might be
better than trying to re-implement the functionality in an expression
passed to gdb_assert.

Thanks
--Don

> 
>   >    sym = lookup_symbol_in_static_block (name, block, domain);
>   >    if (sym.symbol != NULL)
>   >      return sym;
>   > @@ -246,10 +240,9 @@ cp_search_static_and_baseclasses (const char *name,
>   >    struct block_symbol klass_sym;
>   >    struct type *klass_type;
>   >
>   > -  /* The test here uses <= instead of < because Fortran also uses this,
>   > -     and the module.exp testcase will pass "modmany::" for NAME here.   
> */
>   > -  gdb_assert (prefix_len + 2 <= strlen (name));
>   > -  gdb_assert (name[prefix_len + 1] == ':');
>   > +  /* Check for malformed input.  */
>   > +  if (prefix_len + 2 > strlen (name) || name[prefix_len + 1] != ':')
>   > +    return null_block_symbol;
>   >
>   >    /* Find the name of the class and the name of the method, variable,
>   > etc.  */
>   >
>   > diff --git a/gdb/cp-support.c b/gdb/cp-support.c
>   > index df127c4..a71c6ad 100644
>   > --- a/gdb/cp-support.c
>   > +++ b/gdb/cp-support.c
>   > @@ -1037,8 +1037,13 @@ cp_find_first_component_aux (const char *name,
>   > int permissive)
>   >  	      return strlen (name);
>   >  	    }
>   >  	case '\0':
>   > -	case ':':
>   >  	  return index;
>   > +	case ':':
>   > +	  /* ':' marks a component iff the next character is also a ':'.
>   > +	     Otherwise it is probably malformed input.  */
>   > +	  if (name[index + 1] == ':')
>   > +	    return index;
>   > +	  break;
> 
> What if name[index+2] is also ':'? :-)
> 
> [btw, I think "a::::b" is illegal (note four colons),
> but I don't know that for sure]
> 
>   >  	case 'o':
>   >  	  /* Operator names can screw up the recursion.  */
>   >  	  if (operator_possible
>   >
>