[rfa] partial fix for PR gdb/1245

[rfa] partial fix for PR gdb/1245

[David Carlton]

Here's a partial fix for PR gdb/1245, the one about an assertion
failure coming from a demangled name like "int operator<<foo>".  The
best fix there would be to fix the demangler to put a space betwen the
two <'s, but given that this is the second time we've run into that
assertion failure, it's probably best to change it into a complaint,
because demangled name weirdness really shouldn't leave GDB in an
unusable state.

So that's what this patch does.  It ensures that components found by
cp_find_first_component either end in a colon or are the entire
string.  If something else happens, it complains and returns the
entire string.

With this change to the function, some other gdb_asserts (e.g. the one
in cp_entire_prefix_len) become necessary.  I'm ambivalent about
removing correct assertions; I'm happy to either do so or not in this
case, depending on how you see fit.

The next question is: do we want to go any further with a fix for PR
gdb/1245?  On the one hand, the demangler's output really is
incorrect.  On the other hand, even if we fix the demangler's output,
GDB will never do anything useful with the output in this particular
situation, I think: it only occurs in the context of templated
functions, and the fact that their demangled names include the return
value means that the prefixes that we get from them aren't correct in
the first place.  Being a lazy person who has never submitted a patch
to libiberty, I'm inclined to put a real fix for this issue on the
back burner.

Tested on GCC 3.2, DWARF 2, i686-pc-linux-gnu; no new regressions.  OK
to commit on mainline and branch?

David Carlton
carlton@kealia.com

2003-06-25  David Carlton  <carlton@kealia.com>

	Band-aid for PR c++/1245.
	* Makefile.in (cp-support.o): Depend on complaints_h.
	* cp-support.c: Include complaints.h.  Add declaration for
	find_last_component.
	(cp_find_first_component): Separate code into
	cp_find_first_component_aux.
	(cp_find_first_component_aux): Call demangled_name_complaint.
	(demangled_name_complaint): New.

2003-06-25  David Carlton  <carlton@kealia.com>

	* gdb.c++/maint.exp (test_invalid_name): New.
	(test_first_component): Add tests for invalid names.

Index: cp-support.c
===================================================================
RCS file: /cvs/src/src/gdb/cp-support.c,v
retrieving revision 1.5
diff -u -p -r1.5 cp-support.c
--- cp-support.c	12 Jun 2003 15:33:45 -0000	1.5
+++ cp-support.c	26 Jun 2003 00:08:20 -0000
@@ -32,6 +32,16 @@
 #include "frame.h"
 #include "symtab.h"
 #include "block.h"
+#include "complaints.h"
+
+/* Functions related to demangled name parsing.  */
+
+static const char *find_last_component (const char *name);
+
+static unsigned int cp_find_first_component_aux (const char *name,
+						 int permissive);
+
+static void demangled_name_complaint (const char *name);
 
 /* Functions/variables related to overload resolution.  */
 
@@ -199,21 +209,31 @@ method_name_from_physname (const char *p
    boundary of the first component: so, given 'A::foo' or 'A::B::foo'
    it returns the 1, and given 'foo', it returns 0.  */
 
-/* Well, that's what it should do when called externally, but to make
-   the recursion easier, it also stops if it reaches an unexpected ')'
-   or '>'.  */
+/* The character in NAME indexed by the return value is guaranteed to
+   always be either ':' or '\0'.  */
 
 /* NOTE: carlton/2003-03-13: This function is currently only intended
    for internal use: it's probably not entirely safe when called on
-   user-generated input, because some of the 'index += 2' lines might
-   go past the end of malformed input.  */
+   user-generated input, because some of the 'index += 2' lines in
+   cp_find_first_component_aux might go past the end of malformed
+   input.  */
+
+unsigned int cp_find_first_component (const char *name)
+{
+  return cp_find_first_component_aux (name, 0);
+}
+
+/* Helper function for cp_find_first_component.  Like that function,
+   it returns the length of the first component of NAME, but to make
+   the recursion easier, it also stops if it reaches an unexpected ')'
+   or '>' if the value of PERMISSIVE is nonzero.  */
 
 /* Let's optimize away calls to strlen("operator").  */
 
 #define LENGTH_OF_OPERATOR 8
 
 unsigned int
-cp_find_first_component (const char *name)
+cp_find_first_component_aux (const char *name, int permissive)
 {
   unsigned int index = 0;
   /* Operator names can show up in unexpected places.  Since these can
@@ -234,11 +254,15 @@ cp_find_first_component (const char *nam
 	     terminating the component or a '::' between two
 	     components.  (Hence the '+ 2'.)  */
 	  index += 1;
-	  for (index += cp_find_first_component (name + index);
+	  for (index += cp_find_first_component_aux (name + index, 1);
 	       name[index] != '>';
-	       index += cp_find_first_component (name + index))
+	       index += cp_find_first_component_aux (name + index, 1))
 	    {
-	      gdb_assert (name[index] == ':');
+	      if (name[index] != ':')
+		{
+		  demangled_name_complaint (name);
+		  return strlen (name);
+		}
 	      index += 2;
 	    }
 	  operator_possible = 1;
@@ -246,17 +270,30 @@ cp_find_first_component (const char *nam
 	case '(':
 	  /* Similar comment as to '<'.  */
 	  index += 1;
-	  for (index += cp_find_first_component (name + index);
+	  for (index += cp_find_first_component_aux (name + index, 1);
 	       name[index] != ')';
-	       index += cp_find_first_component (name + index))
+	       index += cp_find_first_component_aux (name + index, 1))
 	    {
-	      gdb_assert (name[index] == ':');
+	      if (name[index] != ':')
+		{
+		  demangled_name_complaint (name);
+		  return strlen (name);
+		}
 	      index += 2;
 	    }
 	  operator_possible = 1;
 	  break;
 	case '>':
 	case ')':
+	  if (permissive)
+	    {
+	      return index;
+	    }
+	  else
+	    {
+	      demangled_name_complaint (name);
+	      return strlen (name);
+	    }
 	case '\0':
 	case ':':
 	  return index;
@@ -313,6 +350,16 @@ cp_find_first_component (const char *nam
 	  break;
 	}
     }
+}
+
+/* Complain about a demangled name that we don't know how to parse.
+   NAME is the demangled name in question.  */
+
+static void
+demangled_name_complaint (const char *name)
+{
+  complaint (&symfile_complaints,
+	     "unexpected demangled name '%s'", name);
 }
 
 /* If NAME is the fully-qualified name of a C++
Index: Makefile.in
===================================================================
RCS file: /cvs/src/src/gdb/Makefile.in,v
retrieving revision 1.410
diff -u -p -r1.410 Makefile.in
--- Makefile.in	21 Jun 2003 23:14:43 -0000	1.410
+++ Makefile.in	26 Jun 2003 00:09:28 -0000
@@ -1645,7 +1645,7 @@ cp-namespace.o: cp-namespace.c $(defs_h)
 	$(symtab_h) $(symfile_h) $(gdb_assert_h) $(block_h)
 cp-support.o: cp-support.c $(defs_h) $(cp_support_h) $(gdb_string_h) \
 	$(demangle_h) $(gdb_assert_h) $(gdbcmd_h) $(dictionary_h) \
-	$(objfiles_h) $(frame_h) $(block_h)
+	$(objfiles_h) $(frame_h) $(block_h) $(complaints_h)
 cp-valprint.o: cp-valprint.c $(defs_h) $(gdb_obstack_h) $(symtab_h) \
 	$(gdbtypes_h) $(expression_h) $(value_h) $(command_h) $(gdbcmd_h) \
 	$(demangle_h) $(annotate_h) $(gdb_string_h) $(c_lang_h) $(target_h) \
Index: maint.exp
===================================================================
RCS file: /cvs/src/src/gdb/testsuite/gdb.c++/maint.exp,v
retrieving revision 1.2
diff -u -p -r1.2 maint.exp
--- maint.exp	23 Apr 2003 23:45:24 -0000	1.2
+++ maint.exp	26 Jun 2003 00:09:34 -0000
@@ -45,7 +45,19 @@ proc test_single_component {name} {
     gdb_test "maint cp first_component $name" "$matchname"
 }
 
+# This is used when NAME is invalid.
+proc test_invalid_name {name} {
+    set matchname [string_to_regexp "$name"]
+    gdb_test "maint cp first_component $name" \
+	"During symbol reading, unexpected demangled name '$matchname'.\r\n$matchname"
+}
+
 proc test_first_component {} {
+    # The function in question might complain; make sure that we see
+    # all complaints.
+
+    gdb_test "set complaints -1" ""
+
     test_single_component "foo"
     test_single_component "operator<<"
     test_single_component "operator>>"
@@ -79,6 +91,16 @@ proc test_first_component {} {
     gdb_test "maint cp first_component foo::bar::baz" "foo"
     gdb_test "maint cp first_component C<A>::bar" "C<A>"
     gdb_test "maint cp first_component C<std::basic_streambuf<wchar_t,std::char_traits<wchar_t> > >::bar" "C<std::basic_streambuf<wchar_t,std::char_traits<wchar_t> > >"
+
+    # Make sure we behave appropriately on invalid input.
+
+    # NOTE: carlton/2003-06-25: As of today, the demangler can in fact
+    # produce examples like the third case below: there really should
+    # be a space between the two <'s.  See PR gdb/1245.
+
+    test_invalid_name "foo<"
+    test_invalid_name "foo("
+    test_invalid_name "bool operator<<char>"
 }
 
 gdb_exit

Re: [rfa] partial fix for PR gdb/1245

[Daniel Jacobowitz]

On Wed, Jun 25, 2003 at 05:27:09PM -0700, David Carlton wrote:
> Here's a partial fix for PR gdb/1245, the one about an assertion
> failure coming from a demangled name like "int operator<<foo>".  The
> best fix there would be to fix the demangler to put a space betwen the
> two <'s, but given that this is the second time we've run into that
> assertion failure, it's probably best to change it into a complaint,
> because demangled name weirdness really shouldn't leave GDB in an
> unusable state.
> 
> So that's what this patch does.  It ensures that components found by
> cp_find_first_component either end in a colon or are the entire
> string.  If something else happens, it complains and returns the
> entire string.
> 
> With this change to the function, some other gdb_asserts (e.g. the one
> in cp_entire_prefix_len) become necessary.  I'm ambivalent about
> removing correct assertions; I'm happy to either do so or not in this
> case, depending on how you see fit.
> 
> The next question is: do we want to go any further with a fix for PR
> gdb/1245?  On the one hand, the demangler's output really is
> incorrect.  On the other hand, even if we fix the demangler's output,
> GDB will never do anything useful with the output in this particular
> situation, I think: it only occurs in the context of templated
> functions, and the fact that their demangled names include the return
> value means that the prefixes that we get from them aren't correct in
> the first place.  Being a lazy person who has never submitted a patch
> to libiberty, I'm inclined to put a real fix for this issue on the
> back burner.
> 
> Tested on GCC 3.2, DWARF 2, i686-pc-linux-gnu; no new regressions.  OK
> to commit on mainline and branch?

Yes, this is OK as a bandaid.  As I explained in the PR, we can
correctly handle this, but not asserting is good enough for the moment.

If you fix the two style nits, you can check this in:

> +unsigned int cp_find_first_component (const char *name)

Line break there.

>  	case ')':
> +	  if (permissive)
> +	    {
> +	      return index;
> +	    }

Please drop the useless pair of braces.


-- 
Daniel Jacobowitz
MontaVista Software                         Debian GNU/Linux Developer

Re: [rfa] partial fix for PR gdb/1245

[Andrew Cagney]

>  	case ')':
>> +	  if (permissive)
>> +	    {
>> +	      return index;
>> +	    }
> 
> 
> Please drop the useless pair of braces.

BTW, there are advantages and disasvantages to constantly adding and 
deleting extra brances.  If they are included, follow-on diffs such as:

	if (permissive)
	  {
+	    process_index (&index);
	    return index;
	  }

read much easier :-)

It'ss a gnu coding standard loose loose situtation :-)

Andrew

Re: [rfa] partial fix for PR gdb/1245

[David Carlton]

On Sun, 29 Jun 2003 17:06:42 -0400, Daniel Jacobowitz <drow@mvista.com> said:

> Yes, this is OK as a bandaid.  As I explained in the PR, we can
> correctly handle this, but not asserting is good enough for the
> moment.

Great, thanks.  Though I just sent a reply to the PR saying that I
disagree with you about what fix is correct, but never mind that; it's
really not a high priority issue once this patch goes in, I think.

>> case ')':
>> +	  if (permissive)
>> +	    {
>> +	      return index;
>> +	    }

> Please drop the useless pair of braces.

I guess I did that to balance the else clause, but I'm happy to do it
your way.

Committed on mainline and branch.

David Carlton
carlton@kealia.com