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

[Keith Seitz <keiths@redhat.com>]

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);
-
   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;
 	case 'o':
 	  /* Operator names can screw up the recursion.  */
 	  if (operator_possible