Re: GDB crash due to infinite recursion in typedef substitution (Re: [PATCHv2 3/3] gdb: Remove C++ symbol aliases from completion list)

[Pedro Alves <palves@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 4797 bytes --]

On 5/26/20 10:17 PM, Keith Seitz wrote:
> On 5/25/20 7:34 AM, Pedro Alves wrote:
>>
>> To recap, that happens when we're processing a DEMANGLE_COMPONENT_QUAL_NAME
>> subtree.  We've processed "gdb", and then "option", and now we're going to
>> process the "boolean_option_def" component:
>>
>>  typed name
>>    qualified name
>>      name 'gdb'
>>      qualified name
>>        name 'option'
>>        qualified name
>>          template
>>            name 'boolean_option_def'      <<< 
>>            template argument list
>>              name 'value_print_options'
>>          name 'boolean_option_def'
>>
>> To me, it looks quite obvious that the problem is
>> that we're looking up that boolean_option_def symbol in the
>> global name namespace.  I mean, this is a qualified symbol
>> name, like:
>>
>>    gdb::option::boolean_option_def
>>
> 
> Hmm. I'm not sure that's how the code was originally written to work...
> replace_typedefs_qualified_name is supposed to "skip over" *_NAME
> components until it finds something that looks like it could be a type name.
> All these name components should simply be copied to the buffer. There's
> no reason to look them up as symbols.
> 
> That would imply that the code simply needs to be modified to look
> at DEMANGLE_COMPONENT_TEMPLATE properly.
> 
> Something like:
> 
> diff --git a/gdb/cp-support.c b/gdb/cp-support.c
> index 1e54aaea3b..7761470b2d 100644
> --- a/gdb/cp-support.c
> +++ b/gdb/cp-support.c
> @@ -314,6 +314,9 @@ replace_typedefs_qualified_name (struct demangle_parse_info *info,
>       substituted name.  */
>    while (comp->type == DEMANGLE_COMPONENT_QUAL_NAME)
>      {
> +      if (d_left (comp)->type == DEMANGLE_COMPONENT_TEMPLATE)
> +       comp = d_left (comp);
> +
>        if (d_left (comp)->type == DEMANGLE_COMPONENT_NAME)
>         {
>           struct demangle_component newobj;
> 
> NOTE: I haven't extensively tested this patch. Fedora 31 system GDB
> crashes or fails to set a breakpoint on one of the completed values -- it
> does complete it okay. On master the above tentative patch seems to work.
> 
> I have much more investigation and testing to do for this before submitting
> anything official, though.

Today I experimented with different kinds of symbols to get a better sense
of what the demangled tree node looks like in different scenarios.
I understand this better now.

I now agree that this is looking like the direction that we need to
take (#2 in my original email).

However, as you have it I don't think is fully correct.  Because, say,
you have a symbol like:

  A::B<int>::C<int>::function()

It seems like that change will make it so that we won't ever
try to substitute a typedef past the first template?

so with e.g., 

 typedef int MyInt;

setting a breakpoint like this works:

 "(gdb) b A::B<MyInt>::C<int>::function()"

but like this doesn't:

 "(gdb) b A::B<int>::C<MyInt>::function()"

I started writing a testcase to try some of these different scenarios, based
on gdb.linespec/cp-completion-aliases-2.exp.  See patches attached.
One of the patches includes the recursion detection I'm using to avoid
crashing GDB.  That patch also includes some hacks to print some debugging
stuff.

I also ran into another case of GDB recursing forever due to a template.
This is when the tree has a qualified name node with the template node on
the right child node instead of on the left.  Seems like we'll need a similar
treatment for templates at the end of replace_typedefs_qualified_name.

Here's how to trigger it with the testcase attached:

(gdb) b NS1::NS2::grab_it(NS1::NS2::magic<int>*) 
warning: inspect_type: max recursion limit reached for NS1
warning: inspect_type: max recursion limit reached for NS1::NS2
warning: inspect_type: max recursion limit reached for magic
warning: inspect_type: max recursion limit reached for NS1
warning: inspect_type: max recursion limit reached for NS1::NS2
warning: inspect_type: max recursion limit reached for magic
Function "NS1::NS2::grab_it(NS1::NS2::magic<int>*)" not defined.
Make breakpoint pending on future shared library load? (y or [n]) 

The tree looks like:

d_dump tree for NS1::NS2::grab_it(NS1::NS2::magic<int>*):
typed name
  qualified name
    name 'NS1'
    qualified name
      name 'NS2'
      name 'grab_it'
  function type
    argument list
      pointer
        qualified name
          name 'NS1'
          qualified name
            name 'NS2'
            template
              name 'magic'
              template argument list
                builtin type int

Seems like this will be more work that I hoped...

Maybe we should start by putting in the recursion limit, to
avoid the crash, and then we can fix the issues more like
other bugs than fatal crashes that make it impossible to debug
gdb itself...


[-- Attachment #2: 0001-recursion-limit-debug-stuff.patch --]
[-- Type: text/x-patch, Size: 6055 bytes --]

From 84772ca2bf6acd84c971adffe0b94d3696ad5a1b Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Wed, 27 May 2020 19:56:18 +0100
Subject: [PATCH 1/2] recursion limit & debug stuff

---
 gdb/cp-support.c        | 82 ++++++++++++++++++++++++++++++++++++++++++++++---
 libiberty/cp-demangle.c |  8 +++--
 2 files changed, 83 insertions(+), 7 deletions(-)

diff --git a/gdb/cp-support.c b/gdb/cp-support.c
index 1e54aaea3b1..9ee8cf74f58 100644
--- a/gdb/cp-support.c
+++ b/gdb/cp-support.c
@@ -126,6 +126,12 @@ cp_already_canonical (const char *string)
     return 0;
 }
 
+/* Recursion counter.  */
+int inspect_type_recursion = 0;
+
+/* Set to true to enable inspect_type debug logs.  */
+static int debug_inspect_type;
+
 /* Inspect the given RET_COMP for its type.  If it is a typedef,
    replace the node with the typedef's tree.
 
@@ -146,11 +152,28 @@ inspect_type (struct demangle_parse_info *info,
   memcpy (name, ret_comp->u.s_name.s, ret_comp->u.s_name.len);
   name[ret_comp->u.s_name.len] = '\0';
 
+  scoped_restore restore = make_scoped_restore (&inspect_type_recursion);
+  ++inspect_type_recursion;
+
+  if (debug_inspect_type)
+    fprintf_unfiltered (gdb_stdlog, "inspect_type: %d, %s\n", inspect_type_recursion, name);
+
+  if (inspect_type_recursion > 200)
+    {
+      warning (_("inspect_type: max recursion limit reached for %s"), name);
+      return 0;
+    }
+
   /* Ignore any typedefs that should not be substituted.  */
   for (int i = 0; i < ARRAY_SIZE (ignore_typedefs); ++i)
     {
       if (strcmp (name, ignore_typedefs[i]) == 0)
-	return 0;
+	{
+	  if (debug_inspect_type)
+	    fprintf_unfiltered (gdb_stdlog,
+				"inspect_type, ignored typedef\n");
+	  return 0;
+	}
     }
 
   sym = NULL;
@@ -161,6 +184,9 @@ inspect_type (struct demangle_parse_info *info,
     }
   catch (const gdb_exception &except)
     {
+      if (debug_inspect_type)
+	fprintf_unfiltered (gdb_stdlog,
+			    "inspect_type, lookup_symbol failed\n");
       return 0;
     }
 
@@ -176,9 +202,16 @@ inspect_type (struct demangle_parse_info *info,
 	    {
 	      ret_comp->u.s_name.s = new_name;
 	      ret_comp->u.s_name.len = strlen (new_name);
+
+	      if (debug_inspect_type)
+		fprintf_unfiltered (gdb_stdlog,
+				    "inspect_type, finder, return 1, new_name: %s\n",
+				    new_name);
 	      return 1;
 	    }
 
+	  if (debug_inspect_type)
+	    fprintf_unfiltered (gdb_stdlog, "inspect_type, finder, return 0\n");
 	  return 0;
 	}
 
@@ -209,7 +242,11 @@ inspect_type (struct demangle_parse_info *info,
 	     as the symbol's name, e.g., "typedef struct foo foo;".  */
 	  if (type->name () != nullptr
 	      && strcmp (type->name (), name) == 0)
-	    return 0;
+	    {
+	      if (debug_inspect_type)
+		fprintf_unfiltered (gdb_stdlog, "inspect_type, same name as original name\n");
+	      return 0;
+	    }
 
 	  is_anon = (type->name () == NULL
 		     && (type->code () == TYPE_CODE_ENUM
@@ -244,6 +281,9 @@ inspect_type (struct demangle_parse_info *info,
 	     in continuing, so just bow out gracefully.  */
 	  catch (const gdb_exception_error &except)
 	    {
+	      if (debug_inspect_type)
+		fprintf_unfiltered (gdb_stdlog, "inspect_type, error: %s\n",
+				    except.message->c_str ());
 	      return 0;
 	    }
 
@@ -264,7 +304,13 @@ inspect_type (struct demangle_parse_info *info,
 		 if the type is anonymous (that would lead to infinite
 		 looping).  */
 	      if (!is_anon)
-		replace_typedefs (info, ret_comp, finder, data);
+		{
+		  if (debug_inspect_type)
+		    fprintf_unfiltered (gdb_stdlog, "replace_typedefs: %s\n",
+					name);
+
+		  replace_typedefs (info, ret_comp, finder, data);
+		}
 	    }
 	  else
 	    {
@@ -505,6 +551,14 @@ replace_typedefs (struct demangle_parse_info *info,
     }
 }
 
+/* From libiberty.  */
+extern "C" void d_dump (struct demangle_component *dc, int indent);
+
+/* Recursion counter.  */
+int cp_canonicalize_string_full_count;
+
+static int debug_canonicalize = 0;
+
 /* Parse STRING and convert it to canonical form, resolving any
    typedefs.  If parsing fails, or if STRING is already canonical,
    return nullptr.  Otherwise return the canonical form.  If
@@ -523,6 +577,16 @@ cp_canonicalize_string_full (const char *string,
   info = cp_demangled_name_to_comp (string, NULL);
   if (info != NULL)
     {
+      scoped_restore restore = make_scoped_restore (&cp_canonicalize_string_full_count);
+      if (cp_canonicalize_string_full_count++ == 0)
+	{
+	  if (debug_canonicalize)
+	    {
+	      fprintf_unfiltered (gdb_stdlog, "d_dump tree for %s:\n", string);
+	      d_dump (info->tree, 0);
+	    }
+	}
+
       /* Replace all the typedefs in the tree.  */
       replace_typedefs (info.get (), info->tree, finder, data);
 
@@ -534,7 +598,17 @@ cp_canonicalize_string_full (const char *string,
       /* Finally, compare the original string with the computed
 	 name, returning NULL if they are the same.  */
       if (strcmp (us.get (), string) == 0)
-	return nullptr;
+	{
+	  if (debug_canonicalize)
+	    fprintf_unfiltered (gdb_stdlog, "raw == canonical\n");
+	  return nullptr;
+	}
+
+      if (debug_canonicalize)
+	{
+	  fprintf_unfiltered (gdb_stdlog, "raw:       %s\n", string);
+	  fprintf_unfiltered (gdb_stdlog, "canonical: %s\n", us.get ());
+	}
 
       return us;
     }
diff --git a/libiberty/cp-demangle.c b/libiberty/cp-demangle.c
index cbfb2f937ca..ef8877d01ec 100644
--- a/libiberty/cp-demangle.c
+++ b/libiberty/cp-demangle.c
@@ -595,9 +595,11 @@ is_fnqual_component_type (enum demangle_component_type type)
 }
 
 
-#ifdef CP_DEMANGLE_DEBUG
+// #ifdef CP_DEMANGLE_DEBUG
 
-static void
+void d_dump (struct demangle_component *dc, int indent);
+
+void
 d_dump (struct demangle_component *dc, int indent)
 {
   int i;
@@ -853,7 +855,7 @@ d_dump (struct demangle_component *dc, int indent)
   d_dump (d_right (dc), indent + 2);
 }
 
-#endif /* CP_DEMANGLE_DEBUG */
+// #endif /* CP_DEMANGLE_DEBUG */
 
 /* Fill in a DEMANGLE_COMPONENT_NAME.  */
 

base-commit: 636edd0018b72d67ee224e868fb277a658e9b984
-- 
2.14.5


[-- Attachment #3: 0002-rough-testcase.patch --]
[-- Type: text/x-patch, Size: 6093 bytes --]

From 61d3b7c315f2621f913c28c5d3b2819994d07c12 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Wed, 27 May 2020 19:31:48 +0100
Subject: [PATCH 2/2] rough testcase

---
 .../gdb.linespec/cp-completion-aliases-2.cc        | 132 +++++++++++++++++++++
 .../gdb.linespec/cp-completion-aliases-2.exp       |  64 ++++++++++
 2 files changed, 196 insertions(+)
 create mode 100644 gdb/testsuite/gdb.linespec/cp-completion-aliases-2.cc
 create mode 100644 gdb/testsuite/gdb.linespec/cp-completion-aliases-2.exp

diff --git a/gdb/testsuite/gdb.linespec/cp-completion-aliases-2.cc b/gdb/testsuite/gdb.linespec/cp-completion-aliases-2.cc
new file mode 100644
index 00000000000..cc1eb0505b8
--- /dev/null
+++ b/gdb/testsuite/gdb.linespec/cp-completion-aliases-2.cc
@@ -0,0 +1,132 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2019-2020 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <cstring>
+
+namespace NS1 {
+
+namespace NS2 {
+
+struct object
+{
+  int a;
+
+  object ();
+};
+
+object::object ()
+{
+}
+
+typedef object *object_p;
+
+typedef const char *my_string_t;
+
+template<typename T>
+struct magic
+{
+  T x;
+
+  magic (object_p)
+  {
+  }
+
+  template<typename I>
+  static void
+  foo (object_p)
+  {
+  }
+};
+
+template<typename T, typename U>
+struct tragic
+{
+  T t;
+  U u;
+
+  tragic (object_p)
+  {
+  }
+
+  template<typename I>
+  static void
+  foo (object_p)
+  {
+  }
+};
+
+template<typename T> using partial = tragic<int, T>;
+
+typedef partial<int> int_tragic_t;
+
+typedef magic<int> int_magic_t;
+
+static int
+get_value (object_p obj)
+{
+  return obj->a;
+}
+
+static int
+get_something (object_p obj)
+{
+  return obj->a;
+}
+
+static int
+get_something (my_string_t msg)
+{
+  return strlen (msg);
+}
+
+static int
+grab_it (int_magic_t *var)
+{
+  return var->x;
+}
+
+}
+
+}
+
+/* These typedefs have the same name as some of the components within
+   NS1, on purpose, to try to confuse GDB.  */
+using NS2 = int;
+using object = NS1::NS2::object;
+using magic = NS1::NS2::magic<int>;
+
+NS2 ns2_int = 0;
+object o;
+magic m (0);
+NS1::NS2::int_tragic_t trag (0);
+
+int
+main ()
+{
+  NS1::NS2::magic<int> ns_m (0);
+  NS1::NS2::magic<int>::foo<int> (0);
+  NS1::NS2::partial<int>::foo<int> (0);
+  ns_m.x = 4;
+
+  NS1::NS2::object ns_obj;
+  ns_obj.a = 0;
+
+  int ns_val = (NS1::NS2::get_value (&ns_obj) + NS1::NS2::get_something (&ns_obj)
+		+ NS1::NS2::get_something ("abc") + NS1::NS2::grab_it (&ns_m));
+
+  return ns_val;
+}
diff --git a/gdb/testsuite/gdb.linespec/cp-completion-aliases-2.exp b/gdb/testsuite/gdb.linespec/cp-completion-aliases-2.exp
new file mode 100644
index 00000000000..b09cd6490e8
--- /dev/null
+++ b/gdb/testsuite/gdb.linespec/cp-completion-aliases-2.exp
@@ -0,0 +1,64 @@
+# Copyright 2019-2020 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+# This file tests GDB's ability to remove symbol aliases from the
+# completion list in C++.
+
+load_lib completion-support.exp
+
+standard_testfile .cc
+
+if {[prepare_for_testing "failed to prepare" $testfile $srcfile {debug}]} {
+    return -1
+}
+
+# Disable the completion limit for the whole testcase.
+gdb_test_no_output "set max-completions unlimited"
+
+# This is what I think should happen, but instead, we lose the
+# object_p typedef somewhere.
+test_gdb_complete_unique \
+    "break -q NS1::NS2::get_v" \
+    "break -q NS1::NS2::get_value(NS1::NS2::object_p)"
+
+# Keith's patch fixes this one:
+test_gdb_complete_unique \
+    "break -q NS1::NS2::magic<int>::mag" \
+    "break -q NS1::NS2::magic<int>::magic(NS1::NS2::object*)"
+
+# This works:
+gdb_test "b NS1::NS2::magic<int>::foo<int>(NS1::NS2::object*)" "Breakpoint .* at .*"
+
+# This works too.  Note that there's a "NS2" typedef in the global
+# namespace.
+gdb_test "b NS1::NS2::magic<NS2>::foo<int>(NS1::NS2::object*)" "Breakpoint .* at .*"
+
+# But this does not work with Keith's patch.  We never get to replace
+# the "NS2" typedef in "foo<NS2>".
+gdb_test "b NS1::NS2::magic<NS2>::foo<NS2>(NS1::NS2::object*)" "Breakpoint .* at .*"
+
+# These work.
+gdb_test "b NS1::NS2::tragic<int, int>::foo<int>(NS1::NS2::object*)" "Breakpoint .* at .*"
+gdb_test "b NS1::NS2::tragic<int, int>::foo<int>(NS1::NS2::object_p)" "Breakpoint .* at .*"
+
+# These don't work because GDB doesn't understand the partial<int>
+# alias template.
+gdb_test "b NS1::NS2::partial<int>::foo<int>(NS1::NS2::object*)" "Breakpoint .* at .*"
+gdb_test "b NS1::NS2::partial<int>::foo<int>(NS1::NS2::object_p)" "Breakpoint .* at .*"
+
+# These crash GDB with recursion around "magic".  There's a "magic"
+# typedef in the global namespace.
+gdb_test "b NS1::NS2::grab_it(NS1::NS2::int_magic_t*)" "Breakpoint .* at .*"
+gdb_test "b NS1::NS2::grab_it(NS1::NS2::magic<int>*)" "Breakpoint .* at .*"
-- 
2.14.5