[PATCH 2/2] gdb: Remove C++ symbol aliases from completion list

[PATCH 2/2] gdb: Remove C++ symbol aliases from completion list

[Andrew Burgess]

Consider debugging the following C++ program:

  struct object
  { int a; };

  typedef object *object_p;

  static int
  get_value (object_p obj)
  {
    return obj->a;
  }

  int
  main ()
  {
    object obj;
    obj.a = 0;

    return get_value (&obj);
  }

Now in a GDB session:

  (gdb) complete break get_value
  break get_value(object*)
  break get_value(object_p)

Or:

  (gdb) break get_va<TAB>
  (gdb) break get_value(object<RETURN>
  Function "get_value(object" not defined.
  Make breakpoint pending on future shared library load? (y or [n]) n

The reason this happens is that we add completions based on the
msymbol names and on the symbol names.  For C++ both of these names
include the parameter list, however, the msymbol names have some
differences from the symbol names, for example:

  + typedefs are resolved,
  + whitespace rules are different around pointers,
  + the 'const' keyword is placed differently.

What this means is that the msymbol names and symbol names appear to
be completely different to GDB's completion tracker, and therefore to
readline when it offers the completions.

This commit builds on the previous commit which reworked the
completion_tracker class.  It is now trivial to add a
remove_completion member function, this is then used along with
cp_canonicalize_string_no_typedefs to remove the msymbol aliases from
the completion tracker as we add the symbol names.

Now, for the above program GDB only presents a single completion for
'get_value', which is 'get_value(object_p)'.

It is still possible to reference the symbol using the msymbol name,
so a user can manually type out 'break get_value (object *)' if they
wish and will get the expected behaviour.

I did consider adding an option to make this alias exclusion optional,
in the end I didn't bother as I didn't think it would be very useful,
but I can easily add such an option if people think it would be
useful.

gdb/ChangeLog:

	* completer.c (completion_tracker::remove_completion): Define new
	function.
	* completer.h (completion_tracker::remove_completion): Declare new
	function.
	* symtab.c (completion_list_add_symbol): Remove aliasing msymbols
	when adding a C++ function symbol.

gdb/testsuite/ChangeLog:

	* gdb.linespec/cp-completion-aliases.cc: New file.
	* gdb.linespec/cp-completion-aliases.exp: New file.

Change-Id: Ie5c7c9fc8ecf973072cfb4a9650867104bf7f50c
---
 gdb/ChangeLog                                      |  9 +++
 gdb/completer.c                                    |  9 +++
 gdb/completer.h                                    |  4 ++
 gdb/symtab.c                                       | 18 ++++++
 gdb/testsuite/ChangeLog                            |  5 ++
 .../gdb.linespec/cp-completion-aliases.cc          | 73 ++++++++++++++++++++++
 .../gdb.linespec/cp-completion-aliases.exp         | 57 +++++++++++++++++
 7 files changed, 175 insertions(+)
 create mode 100644 gdb/testsuite/gdb.linespec/cp-completion-aliases.cc
 create mode 100644 gdb/testsuite/gdb.linespec/cp-completion-aliases.exp

diff --git a/gdb/completer.c b/gdb/completer.c
index 93df1018f66..7b66fbf33e1 100644
--- a/gdb/completer.c
+++ b/gdb/completer.c
@@ -1600,6 +1600,15 @@ completion_tracker::add_completions (completion_list &&list)
     add_completion (std::move (candidate));
 }
 
+/* See completer.h.  */
+
+void
+completion_tracker::remove_completion (const char *name)
+{
+  m_entries_hash.erase (name);
+  m_lowest_common_denominator_valid = false;
+}
+
 /* Helper for the make_completion_match_str overloads.  Returns NULL
    as an indication that we want MATCH_NAME exactly.  It is up to the
    caller to xstrdup that string if desired.  */
diff --git a/gdb/completer.h b/gdb/completer.h
index fd4aba79746..6d80c50aa12 100644
--- a/gdb/completer.h
+++ b/gdb/completer.h
@@ -328,6 +328,10 @@ public:
      LIST.  */
   void add_completions (completion_list &&list);
 
+  /* Remove completion matching NAME from the completion list, does nothing
+     if NAME is not already in the completion list.  */
+  void remove_completion (const char *name);
+
   /* Set the quote char to be appended after a unique completion is
      added to the input line.  Set to '\0' to clear.  See
      m_quote_char's description.  */
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 26551372cbb..88ed27c1ca9 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -5261,6 +5261,24 @@ completion_list_add_symbol (completion_tracker &tracker,
   completion_list_add_name (tracker, sym->language (),
 			    sym->natural_name (),
 			    lookup_name, text, word);
+
+  /* C++ function symbols include the parameters within both the msymbol
+     name and the symbol name.  The problem is that the msymbol name will
+     describe the parameters in the most basic way, with typedefs stripped
+     out, while the symbol name will represent the types as they appear in
+     the program.  This means we will see duplicate entries in the
+     completion tracker.  The following converts the symbol name back to
+     the msymbol name and removes the msymbol name from the completion
+     tracker.  */
+  if (sym->language () == language_cplus
+      && SYMBOL_DOMAIN (sym) == VAR_DOMAIN
+      && SYMBOL_CLASS (sym) == LOC_BLOCK)
+    {
+      std::string str
+	= cp_canonicalize_string_no_typedefs (sym->natural_name ());
+      if (!str.empty ())
+	tracker.remove_completion (str.c_str ());
+    }
 }
 
 /* completion_list_add_name wrapper for struct minimal_symbol.  */
diff --git a/gdb/testsuite/gdb.linespec/cp-completion-aliases.cc b/gdb/testsuite/gdb.linespec/cp-completion-aliases.cc
new file mode 100644
index 00000000000..5ef85b401eb
--- /dev/null
+++ b/gdb/testsuite/gdb.linespec/cp-completion-aliases.cc
@@ -0,0 +1,73 @@
+/* This testcase is part of GDB, the GNU debugger.
+
+   Copyright 2019 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>
+
+template<typename T>
+struct magic
+{
+  T x;
+};
+
+struct object
+{
+  int a;
+};
+
+typedef magic<int> int_magic_t;
+
+typedef object *object_p;
+
+typedef const char *my_string_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;
+}
+
+int
+main ()
+{
+  magic<int> m;
+  m.x = 4;
+
+  object obj;
+  obj.a = 0;
+
+  int val = (get_value (&obj) + get_something (&obj)
+	     + get_something ("abc") + grab_it (&m));
+  return val;
+}
diff --git a/gdb/testsuite/gdb.linespec/cp-completion-aliases.exp b/gdb/testsuite/gdb.linespec/cp-completion-aliases.exp
new file mode 100644
index 00000000000..9fb497da7a9
--- /dev/null
+++ b/gdb/testsuite/gdb.linespec/cp-completion-aliases.exp
@@ -0,0 +1,57 @@
+# Copyright 2019 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
+}
+
+# Tests below are about tab-completion, which doesn't work if readline
+# library isn't used.  Check it first.
+
+if { ![readline_is_used] } {
+    untested "no tab completion support without readline"
+    return -1
+}
+
+# Disable the completion limit for the whole testcase.
+gdb_test_no_output "set max-completions unlimited"
+
+test_gdb_complete_tab_unique "break get_v" \
+    "break get_value\\(object_p\\)" " "
+
+test_gdb_complete_cmd_unique "break get_v" \
+    "break get_value\\(object_p\\)"
+
+test_gdb_complete_tab_unique "break gr" \
+    "break grab_it\\(int_magic_t\\*\\)" " "
+
+test_gdb_complete_cmd_unique "break gr" \
+    "break grab_it\\(int_magic_t\\*\\)"
+
+test_gdb_complete_tab_multiple "break get_som" "ething(" \
+    { "get_something(my_string_t)" "get_something(object_p)" }
+
+test_gdb_complete_cmd_multiple "break " "get_som" \
+    { "get_something(my_string_t)" "get_something(object_p)" }
+
+
+
-- 
2.14.5

[PATCH 1/2] gdb: Convert completion tracker to use std types

[Andrew Burgess]

This commit converts the completion_tracker class to use std types,
while maintaining the existing functionality.

As part of this commit I have restructured how the completion tracker
works a little, the lowest common denominator is now calculated lazily
when we need it.  The reason for this is not performance, but instead
to prepare the way for the next commit which will require the
completion track to support removing possible completions.  If we
calculate the lowest common denominator as we add completions then
removing completions becomes much harder.

With the move to std types there is some ugliness relating to how
object ownership is managed that needs to be worked with.  The
completions are delivered to the completion_tracker as managed strings
within a gdb::unique_xmalloc_ptr<char>, and in an ideal world we would
use these objects as the keys and values within the hash.

The problem is that when we try to remove the items from the hash in
order to pass them to readline; I don't believe there is any easy way
to get the gdb::unique_xmalloc_ptr<char> out of the hash without
having it delete the string it's managing without using C++17 language
features, specifically the std::unordered_map::extract method.

For now then I'm holding the raw 'char *' within the unordered_map,
and rely on the completion_tracker object to delete the data if
needed, or to ensure that the data is passed over to readline, which
will do the deletion for us.

One further ugliness of the new code is that the new unordered_map
actually holds 'const char *', this will make the next commit slightly
easier, but does mean some unfortunate casting in this commit.

There should be no user visible changes after this commit.

gdb/ChangeLog:

	* completer.c (completion_tracker::completes_to_completion_word):
	Update for changes in how lowest common denominator is managed.
	(completion_tracker::completion_tracker): Now does nothing.
	(completion_tracker::discard_completions): Update for changes to
	lowest common denominator, and to m_entries_hash.
	(completion_tracker::maybe_add_completion): Likewise.
	(completion_tracker::~completion_tracker): Call
	discard_completions.
	(completion_tracker::recompute_lowest_common_denominator): Update
	for changes to lowest common denominator, and to m_entries_hash.
	(completion_tracker::build_completion_result): Likewise.
	* completer.h: Add 'unordered_map' include.
	(completion_tracker::have_completions): Use m_entries_hash.
	(completion_tracker::completion_set): New typedef.
	(completion_tracker::recompute_lowest_common_denominator): Update
	signature.
	(completion_tracker::m_entries_vec): Delete.
	(completion_tracker::m_entries_hash): Change type.
	(completion_tracker::m_lowest_common_denominator): Change type.
	(completion_tracker::m_lowest_common_denominator_valid): New
	member variable.
	(completion_tracker::m_lowest_common_denominator_max_length): New
	member variable.

Change-Id: Iac5166189103d84bac0a4181324f38ca14227c47
---
 gdb/ChangeLog   |  26 ++++++++++++
 gdb/completer.c | 129 ++++++++++++++++++++++++++++++++------------------------
 gdb/completer.h |  58 ++++++++++++++++---------
 3 files changed, 138 insertions(+), 75 deletions(-)

diff --git a/gdb/completer.c b/gdb/completer.c
index 6658da6d7fb..93df1018f66 100644
--- a/gdb/completer.c
+++ b/gdb/completer.c
@@ -407,9 +407,10 @@ advance_to_filename_complete_word_point (completion_tracker &tracker,
 bool
 completion_tracker::completes_to_completion_word (const char *word)
 {
+  recompute_lowest_common_denominator ();
   if (m_lowest_common_denominator_unique)
     {
-      const char *lcd = m_lowest_common_denominator;
+      const char *lcd = m_lowest_common_denominator.get ();
 
       if (strncmp_iw (word, lcd, strlen (lcd)) == 0)
 	{
@@ -1512,9 +1513,7 @@ int max_completions = 200;
 
 completion_tracker::completion_tracker ()
 {
-  m_entries_hash = htab_create_alloc (INITIAL_COMPLETION_HTAB_SIZE,
-				      htab_hash_string, streq_hash,
-				      NULL, xcalloc, xfree);
+  /* Nothing.  */
 }
 
 /* See completer.h.  */
@@ -1522,25 +1521,23 @@ completion_tracker::completion_tracker ()
 void
 completion_tracker::discard_completions ()
 {
-  xfree (m_lowest_common_denominator);
   m_lowest_common_denominator = NULL;
-
   m_lowest_common_denominator_unique = false;
+  m_lowest_common_denominator_valid = false;
 
-  m_entries_vec.clear ();
-
-  htab_delete (m_entries_hash);
-  m_entries_hash = htab_create_alloc (INITIAL_COMPLETION_HTAB_SIZE,
-				      htab_hash_string, streq_hash,
-				      NULL, xcalloc, xfree);
+  for (const auto &p : m_entries_hash)
+    {
+      xfree ((char *) p.first);
+      xfree ((char *) p.second);
+    }
+  m_entries_hash.clear ();
 }
 
 /* See completer.h.  */
 
 completion_tracker::~completion_tracker ()
 {
-  xfree (m_lowest_common_denominator);
-  htab_delete (m_entries_hash);
+  discard_completions ();
 }
 
 /* See completer.h.  */
@@ -1551,17 +1548,15 @@ completion_tracker::maybe_add_completion
    completion_match_for_lcd *match_for_lcd,
    const char *text, const char *word)
 {
-  void **slot;
-
   if (max_completions == 0)
     return false;
 
-  if (htab_elements (m_entries_hash) >= max_completions)
+  if (m_entries_hash.size () >= max_completions)
     return false;
 
-  slot = htab_find_slot (m_entries_hash, name.get (), INSERT);
-  if (*slot == HTAB_EMPTY_ENTRY)
+  if (m_entries_hash.find (name.get ()) == m_entries_hash.end ())
     {
+      /* Not already in the hash.  Add it.  */
       const char *match_for_lcd_str = NULL;
 
       if (match_for_lcd != NULL)
@@ -1573,10 +1568,13 @@ completion_tracker::maybe_add_completion
       gdb::unique_xmalloc_ptr<char> lcd
 	= make_completion_match_str (match_for_lcd_str, text, word);
 
-      recompute_lowest_common_denominator (std::move (lcd));
+      size_t lcd_len = strlen (lcd.get ());
+      auto p = std::make_pair (name.release (), lcd.release ());
+      m_entries_hash.insert (std::move (p));
 
-      *slot = name.get ();
-      m_entries_vec.push_back (std::move (name));
+      m_lowest_common_denominator_valid = false;
+      m_lowest_common_denominator_max_length
+	= std::max (m_lowest_common_denominator_max_length, lcd_len);
     }
 
   return true;
@@ -1982,35 +1980,47 @@ completion_find_completion_word (completion_tracker &tracker, const char *text,
 /* See completer.h.  */
 
 void
-completion_tracker::recompute_lowest_common_denominator
-  (gdb::unique_xmalloc_ptr<char> &&new_match_up)
+completion_tracker::recompute_lowest_common_denominator ()
 {
-  if (m_lowest_common_denominator == NULL)
-    {
-      /* We don't have a lowest common denominator yet, so simply take
-	 the whole NEW_MATCH_UP as being it.  */
-      m_lowest_common_denominator = new_match_up.release ();
-      m_lowest_common_denominator_unique = true;
-    }
-  else
-    {
-      /* Find the common denominator between the currently-known
-	 lowest common denominator and NEW_MATCH_UP.  That becomes the
-	 new lowest common denominator.  */
-      size_t i;
-      const char *new_match = new_match_up.get ();
+  /* We've already done this.  */
+  if (m_lowest_common_denominator_valid)
+    return;
 
-      for (i = 0;
-	   (new_match[i] != '\0'
-	    && new_match[i] == m_lowest_common_denominator[i]);
-	   i++)
-	;
-      if (m_lowest_common_denominator[i] != new_match[i])
+  /* Resize the storage to ensure we have enough space, the plus one gives
+     us space for the trailing null terminator we will include.  */
+  char *tmp = (char *) xrealloc (m_lowest_common_denominator.release (),
+				 m_lowest_common_denominator_max_length + 1);
+  m_lowest_common_denominator.reset (tmp);
+
+  for (const auto &p : m_entries_hash)
+    {
+      if (!m_lowest_common_denominator_valid)
+	{
+	  strcpy (m_lowest_common_denominator.get (), p.second);
+	  m_lowest_common_denominator_unique = true;
+	  m_lowest_common_denominator_valid = true;
+	}
+      else
 	{
-	  m_lowest_common_denominator[i] = '\0';
-	  m_lowest_common_denominator_unique = false;
+	  /* Find the common denominator between the currently-known
+	     lowest common denominator and NEW_MATCH_UP.  That becomes the
+	     new lowest common denominator.  */
+	  size_t i;
+	  const char *new_match = p.second;
+
+	  for (i = 0;
+	       (new_match[i] != '\0'
+		&& new_match[i] == m_lowest_common_denominator.get ()[i]);
+	       i++)
+	    ;
+	  if (m_lowest_common_denominator.get ()[i] != new_match[i])
+	    {
+	      m_lowest_common_denominator.get ()[i] = '\0';
+	      m_lowest_common_denominator_unique = false;
+	    }
 	}
     }
+  m_lowest_common_denominator_valid = true;
 }
 
 /* See completer.h.  */
@@ -2092,19 +2102,17 @@ completion_result
 completion_tracker::build_completion_result (const char *text,
 					     int start, int end)
 {
-  completion_list &list = m_entries_vec;	/* The completions.  */
-
-  if (list.empty ())
+  if (m_entries_hash.empty ())
     return {};
 
   /* +1 for the LCD, and +1 for NULL termination.  */
-  char **match_list = XNEWVEC (char *, 1 + list.size () + 1);
+  char **match_list = XNEWVEC (char *, 1 + m_entries_hash.size () + 1);
 
   /* Build replacement word, based on the LCD.  */
-
+  recompute_lowest_common_denominator ();
   match_list[0]
     = expand_preserving_ws (text, end - start,
-			    m_lowest_common_denominator);
+			    m_lowest_common_denominator.get ());
 
   if (m_lowest_common_denominator_unique)
     {
@@ -2128,13 +2136,22 @@ completion_tracker::build_completion_result (const char *text,
     }
   else
     {
-      int ix;
+      int ix = 0;
 
-      for (ix = 0; ix < list.size (); ++ix)
-	match_list[ix + 1] = list[ix].release ();
+      for (const auto &ptr : m_entries_hash)
+	{
+	  match_list[ix + 1] = (char *) ptr.first;
+	  xfree ((char *) ptr.second);
+	  ix++;
+	}
       match_list[ix + 1] = NULL;
 
-      return completion_result (match_list, list.size (), false);
+      /* We must clear the hash here as ownership of all the entries is
+	 being passed over to readline, and we don't want to try and free
+	 these pointers when we ourselves are destroyed.  */
+      size_t hash_size = m_entries_hash.size ();
+      m_entries_hash.clear ();
+      return completion_result (match_list, hash_size, false);
     }
 }
 
diff --git a/gdb/completer.h b/gdb/completer.h
index 313010fce22..fd4aba79746 100644
--- a/gdb/completer.h
+++ b/gdb/completer.h
@@ -20,6 +20,8 @@
 #include "gdbsupport/gdb_vecs.h"
 #include "command.h"
 
+#include <unordered_map>
+
 /* Types of functions in struct match_list_displayer.  */
 
 struct match_list_displayer;
@@ -389,7 +391,7 @@ public:
 
   /* True if we have any completion match recorded.  */
   bool have_completions () const
-  { return !m_entries_vec.empty (); }
+  { return !m_entries_hash.empty (); }
 
   /* Discard the current completion match list and the current
      LCD.  */
@@ -403,6 +405,15 @@ public:
 
 private:
 
+  /* A map of completions.  The key is the completion, and the value is the
+     string to be used to compute the lowest common denominator.  Both the
+     key and the value are owned by the completion_tracker class while
+     being held in this map, as such completion_tracker must free these
+     strings when finished with them, or pass ownership to someone else who
+     will free them.  */
+  typedef std::unordered_map<const char *, const char *, std::hash<std::string>,
+			     std::equal_to<std::string>> completion_set;
+
   /* Add the completion NAME to the list of generated completions if
      it is not there already.  If false is returned, too many
      completions were found.  */
@@ -410,18 +421,11 @@ private:
 			     completion_match_for_lcd *match_for_lcd,
 			     const char *text, const char *word);
 
-  /* Given a new match, recompute the lowest common denominator (LCD)
-     to hand over to readline.  Normally readline computes this itself
-     based on the whole set of completion matches.  However, some
-     completers want to override readline, in order to be able to
-     provide a LCD that is not really a prefix of the matches, but the
-     lowest common denominator of some relevant substring of each
-     match.  E.g., "b push_ba" completes to
-     "std::vector<..>::push_back", "std::string::push_back", etc., and
-     in this case we want the lowest common denominator to be
-     "push_back" instead of "std::".  */
-  void recompute_lowest_common_denominator
-    (gdb::unique_xmalloc_ptr<char> &&new_match);
+  /* Ensure that the lowest common denominator held in the member variable
+     M_LOWEST_COMMON_DENOMINATOR is valid.  This method must be called if
+     there is any chance that new completions have been added to the
+     tracker before the lowest common denominator is read.  */
+  void recompute_lowest_common_denominator ();
 
   /* Completion match outputs returned by the symbol name matching
      routines (see symbol_name_matcher_ftype).  These results are only
@@ -430,16 +434,13 @@ private:
      symbol name matching routines.  */
   completion_match_result m_completion_match_result;
 
-  /* The completion matches found so far, in a vector.  */
-  completion_list m_entries_vec;
-
   /* The completion matches found so far, in a hash table, for
      duplicate elimination as entries are added.  Otherwise the user
      is left scratching his/her head: readline and complete_command
      will remove duplicates, and if removal of duplicates there brings
      the total under max_completions the user may think gdb quit
      searching too early.  */
-  htab_t m_entries_hash;
+  completion_set m_entries_hash;
 
   /* If non-zero, then this is the quote char that needs to be
      appended after completion (iff we have a unique completion).  We
@@ -472,8 +473,16 @@ private:
   bool m_suppress_append_ws = false;
 
   /* Our idea of lowest common denominator to hand over to readline.
-     See intro.  */
-  char *m_lowest_common_denominator = NULL;
+     Normally readline computes this itself based on the whole set of
+     completion matches.  However, some completers want to override
+     readline, in order to be able to provide a LCD that is not really a
+     prefix of the matches, but the lowest common denominator of some
+     relevant substring of each match.
+
+     E.g., "b push_ba" completes to "std::vector<..>::push_back",
+     "std::string::push_back", etc., and in this case we want the lowest
+     common denominator to be "push_back" instead of "std::".  */
+  gdb::unique_xmalloc_ptr<char> m_lowest_common_denominator = nullptr;
 
   /* If true, the LCD is unique.  I.e., all completions had the same
      MATCH_FOR_LCD substring, even if the completions were different.
@@ -483,6 +492,17 @@ private:
      "function()", instead of showing all the possible
      completions.  */
   bool m_lowest_common_denominator_unique = false;
+
+  /* True if the value in M_LOWEST_COMMON_DENOMINATOR is correct.  This is
+     set to true each time RECOMPUTE_LOWEST_COMMON_DENOMINATOR is called,
+     and reset to false whenever a new completion is added.  */
+  bool m_lowest_common_denominator_valid = false;
+
+  /* We can avoid multiple calls to xrealloc in
+     RECOMPUTE_LOWEST_COMMON_DENOMINATOR, we track the maximum possible
+     size of lowest common denominator, which we know as each completion is
+     added.  */
+  size_t m_lowest_common_denominator_max_length = 0;
 };
 
 /* Return a string to hand off to readline as a completion match
-- 
2.14.5

[PATCH 0/2] Remove C++ Symbol Aliases From Completion List

[Andrew Burgess]

Read patch #2 to understand what the motivation for this series is.
Patch #1 is setup work.

---

Andrew Burgess (2):
  gdb: Convert completion tracker to use std types
  gdb: Remove C++ symbol aliases from completion list

 gdb/ChangeLog                                      |  35 ++++++
 gdb/completer.c                                    | 138 ++++++++++++---------
 gdb/completer.h                                    |  62 ++++++---
 gdb/symtab.c                                       |  18 +++
 gdb/testsuite/ChangeLog                            |   5 +
 .../gdb.linespec/cp-completion-aliases.cc          |  73 +++++++++++
 .../gdb.linespec/cp-completion-aliases.exp         |  57 +++++++++
 7 files changed, 313 insertions(+), 75 deletions(-)
 create mode 100644 gdb/testsuite/gdb.linespec/cp-completion-aliases.cc
 create mode 100644 gdb/testsuite/gdb.linespec/cp-completion-aliases.exp

-- 
2.14.5

Re: [PATCH 2/2] gdb: Remove C++ symbol aliases from completion list

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> This commit builds on the previous commit which reworked the
Andrew> completion_tracker class.  It is now trivial to add a
Andrew> remove_completion member function

Wasn't this just as easy with the htab_t implementation?

I suppose I'm fine with either.

Andrew> gdb/ChangeLog:

Andrew> 	* completer.c (completion_tracker::remove_completion): Define new
Andrew> 	function.
Andrew> 	* completer.h (completion_tracker::remove_completion): Declare new
Andrew> 	function.
Andrew> 	* symtab.c (completion_list_add_symbol): Remove aliasing msymbols
Andrew> 	when adding a C++ function symbol.

This seems like a good idea to me.

Andrew> +/* See completer.h.  */
Andrew> +
Andrew> +void
Andrew> +completion_tracker::remove_completion (const char *name)
Andrew> +{
Andrew> +  m_entries_hash.erase (name);

IIUC, the hash owns the pointers, so this would leak some memory.

Tom

Re: [PATCH 1/2] gdb: Convert completion tracker to use std types

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> For now then I'm holding the raw 'char *' within the unordered_map,
Andrew> and rely on the completion_tracker object to delete the data if
Andrew> needed, or to ensure that the data is passed over to readline, which
Andrew> will do the deletion for us.

Seems reasonable enough.

Andrew> +  for (const auto &p : m_entries_hash)
Andrew> +    {
Andrew> +      xfree ((char *) p.first);
Andrew> +      xfree ((char *) p.second);

Maybe const_cast would be better?

Andrew> +  /* A map of completions.  The key is the completion, and the value is the
Andrew> +     string to be used to compute the lowest common denominator.  Both the
Andrew> +     key and the value are owned by the completion_tracker class while
Andrew> +     being held in this map, as such completion_tracker must free these
Andrew> +     strings when finished with them, or pass ownership to someone else who
Andrew> +     will free them.  */
Andrew> +  typedef std::unordered_map<const char *, const char *, std::hash<std::string>,
Andrew> +			     std::equal_to<std::string>> completion_set;

Does using std::string here mean that a temporary std::string will be
made for each insertion in the map?  And also for comparisons?
That seems expensive if so.

Tom

Re: [PATCH 1/2] gdb: Convert completion tracker to use std types

[Christian Biesinger via gdb-patches]

On Fri, Jan 24, 2020, 20:14 Tom Tromey <tom@tromey.com> wrote:

> >>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:
>
> Andrew> For now then I'm holding the raw 'char *' within the unordered_map,
> Andrew> and rely on the completion_tracker object to delete the data if
> Andrew> needed, or to ensure that the data is passed over to readline,
> which
> Andrew> will do the deletion for us.
>
> Seems reasonable enough.
>
> Andrew> +  for (const auto &p : m_entries_hash)
> Andrew> +    {
> Andrew> +      xfree ((char *) p.first);
> Andrew> +      xfree ((char *) p.second);
>
> Maybe const_cast would be better?
>
> Andrew> +  /* A map of completions.  The key is the completion, and the
> value is the
> Andrew> +     string to be used to compute the lowest common denominator.
> Both the
> Andrew> +     key and the value are owned by the completion_tracker class
> while
> Andrew> +     being held in this map, as such completion_tracker must free
> these
> Andrew> +     strings when finished with them, or pass ownership to
> someone else who
> Andrew> +     will free them.  */
> Andrew> +  typedef std::unordered_map<const char *, const char *,
> std::hash<std::string>,
> Andrew> +                            std::equal_to<std::string>>
> completion_set;
>
> Does using std::string here mean that a temporary std::string will be
> made for each insertion in the map?  And also for comparisons?
> That seems expensive if so.
>

Perhaps that's finally a use case to add hash<gdb::string_view>!


> Tom
>

Re: [PATCH 1/2] gdb: Convert completion tracker to use std types

[Tom Tromey]

>>>>> "Christian" == Christian Biesinger <cbiesinger@google.com> writes:

Christian> Perhaps that's finally a use case to add hash<gdb::string_view>!

I also wonder what will be the thing that will make us finally bring in
gcc's hash table.

Tom

[PATCHv2 3/3] gdb: Remove C++ symbol aliases from completion list

[Andrew Burgess]

Consider debugging the following C++ program:

  struct object
  { int a; };

  typedef object *object_p;

  static int
  get_value (object_p obj)
  {
    return obj->a;
  }

  int
  main ()
  {
    object obj;
    obj.a = 0;

    return get_value (&obj);
  }

Now in a GDB session:

  (gdb) complete break get_value
  break get_value(object*)
  break get_value(object_p)

Or:

  (gdb) break get_va<TAB>
  (gdb) break get_value(object<RETURN>
  Function "get_value(object" not defined.
  Make breakpoint pending on future shared library load? (y or [n]) n

The reason this happens is that we add completions based on the
msymbol names and on the symbol names.  For C++ both of these names
include the parameter list, however, the msymbol names have some
differences from the symbol names, for example:

  + typedefs are resolved,
  + whitespace rules are different around pointers,
  + the 'const' keyword is placed differently.

What this means is that the msymbol names and symbol names appear to
be completely different to GDB's completion tracker, and therefore to
readline when it offers the completions.

This commit builds on the previous commit which reworked the
completion_tracker class.  It is now trivial to add a
remove_completion member function, this is then used along with
cp_canonicalize_string_no_typedefs to remove the msymbol aliases from
the completion tracker as we add the symbol names.

Now, for the above program GDB only presents a single completion for
'get_value', which is 'get_value(object_p)'.

It is still possible to reference the symbol using the msymbol name,
so a user can manually type out 'break get_value (object *)' if they
wish and will get the expected behaviour.

I did consider adding an option to make this alias exclusion optional,
in the end I didn't bother as I didn't think it would be very useful,
but I can easily add such an option if people think it would be
useful.

gdb/ChangeLog:

	* completer.c (completion_tracker::remove_completion): Define new
	function.
	* completer.h (completion_tracker::remove_completion): Declare new
	function.
	* symtab.c (completion_list_add_symbol): Remove aliasing msymbols
	when adding a C++ function symbol.

gdb/testsuite/ChangeLog:

	* gdb.linespec/cp-completion-aliases.cc: New file.
	* gdb.linespec/cp-completion-aliases.exp: New file.

Change-Id: Ie5c7c9fc8ecf973072cfb4a9650867104bf7f50c
---
 gdb/ChangeLog                                      |  9 +++
 gdb/completer.c                                    | 14 +++++
 gdb/completer.h                                    |  4 ++
 gdb/symtab.c                                       | 21 +++++++
 gdb/testsuite/ChangeLog                            |  5 ++
 .../gdb.linespec/cp-completion-aliases.cc          | 73 ++++++++++++++++++++++
 .../gdb.linespec/cp-completion-aliases.exp         | 57 +++++++++++++++++
 7 files changed, 183 insertions(+)
 create mode 100644 gdb/testsuite/gdb.linespec/cp-completion-aliases.cc
 create mode 100644 gdb/testsuite/gdb.linespec/cp-completion-aliases.exp

diff --git a/gdb/completer.c b/gdb/completer.c
index 14c7a579970..67dce30fbe3 100644
--- a/gdb/completer.c
+++ b/gdb/completer.c
@@ -1678,6 +1678,20 @@ completion_tracker::add_completions (completion_list &&list)
     add_completion (std::move (candidate));
 }
 
+/* See completer.h.  */
+
+void
+completion_tracker::remove_completion (const char *name)
+{
+  hashval_t hash = htab_hash_string (name);
+  if (htab_find_slot_with_hash (m_entries_hash, name, hash, NO_INSERT)
+      != NULL)
+    {
+      htab_remove_elt_with_hash (m_entries_hash, name, hash);
+      m_lowest_common_denominator_valid = false;
+    }
+}
+
 /* Helper for the make_completion_match_str overloads.  Returns NULL
    as an indication that we want MATCH_NAME exactly.  It is up to the
    caller to xstrdup that string if desired.  */
diff --git a/gdb/completer.h b/gdb/completer.h
index 7bfe0d58142..fd0d47b206b 100644
--- a/gdb/completer.h
+++ b/gdb/completer.h
@@ -326,6 +326,10 @@ public:
      LIST.  */
   void add_completions (completion_list &&list);
 
+  /* Remove completion matching NAME from the completion list, does nothing
+     if NAME is not already in the completion list.  */
+  void remove_completion (const char *name);
+
   /* Set the quote char to be appended after a unique completion is
      added to the input line.  Set to '\0' to clear.  See
      m_quote_char's description.  */
diff --git a/gdb/symtab.c b/gdb/symtab.c
index f456f4d852d..d23552716f8 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -5269,6 +5269,27 @@ completion_list_add_symbol (completion_tracker &tracker,
   completion_list_add_name (tracker, sym->language (),
 			    sym->natural_name (),
 			    lookup_name, text, word);
+
+  /* C++ function symbols include the parameters within both the msymbol
+     name and the symbol name.  The problem is that the msymbol name will
+     describe the parameters in the most basic way, with typedefs stripped
+     out, while the symbol name will represent the types as they appear in
+     the program.  This means we will see duplicate entries in the
+     completion tracker.  The following converts the symbol name back to
+     the msymbol name and removes the msymbol name from the completion
+     tracker.  */
+  if (sym->language () == language_cplus
+      && SYMBOL_DOMAIN (sym) == VAR_DOMAIN
+      && SYMBOL_CLASS (sym) == LOC_BLOCK)
+    {
+      /* The call to canonicalize returns the empty string if the input
+	 string is already in canonical form, thanks to this we don't
+	 remove the symbol we just added above.  */
+      std::string str
+	= cp_canonicalize_string_no_typedefs (sym->natural_name ());
+      if (!str.empty ())
+	tracker.remove_completion (str.c_str ());
+    }
 }
 
 /* completion_list_add_name wrapper for struct minimal_symbol.  */
diff --git a/gdb/testsuite/gdb.linespec/cp-completion-aliases.cc b/gdb/testsuite/gdb.linespec/cp-completion-aliases.cc
new file mode 100644
index 00000000000..5f2fb5c663a
--- /dev/null
+++ b/gdb/testsuite/gdb.linespec/cp-completion-aliases.cc
@@ -0,0 +1,73 @@
+/* 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>
+
+template<typename T>
+struct magic
+{
+  T x;
+};
+
+struct object
+{
+  int a;
+};
+
+typedef magic<int> int_magic_t;
+
+typedef object *object_p;
+
+typedef const char *my_string_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;
+}
+
+int
+main ()
+{
+  magic<int> m;
+  m.x = 4;
+
+  object obj;
+  obj.a = 0;
+
+  int val = (get_value (&obj) + get_something (&obj)
+	     + get_something ("abc") + grab_it (&m));
+  return val;
+}
diff --git a/gdb/testsuite/gdb.linespec/cp-completion-aliases.exp b/gdb/testsuite/gdb.linespec/cp-completion-aliases.exp
new file mode 100644
index 00000000000..e8fe0bc6af2
--- /dev/null
+++ b/gdb/testsuite/gdb.linespec/cp-completion-aliases.exp
@@ -0,0 +1,57 @@
+# 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
+}
+
+# Tests below are about tab-completion, which doesn't work if readline
+# library isn't used.  Check it first.
+
+if { ![readline_is_used] } {
+    untested "no tab completion support without readline"
+    return -1
+}
+
+# Disable the completion limit for the whole testcase.
+gdb_test_no_output "set max-completions unlimited"
+
+test_gdb_complete_tab_unique "break get_v" \
+    "break get_value\\(object_p\\)" " "
+
+test_gdb_complete_cmd_unique "break get_v" \
+    "break get_value\\(object_p\\)"
+
+test_gdb_complete_tab_unique "break gr" \
+    "break grab_it\\(int_magic_t\\*\\)" " "
+
+test_gdb_complete_cmd_unique "break gr" \
+    "break grab_it\\(int_magic_t\\*\\)"
+
+test_gdb_complete_tab_multiple "break get_som" "ething(" \
+    { "get_something(my_string_t)" "get_something(object_p)" }
+
+test_gdb_complete_cmd_multiple "break " "get_som" \
+    { "get_something(my_string_t)" "get_something(object_p)" }
+
+
+
-- 
2.14.5

[PATCHv2 2/3] gdb: Restructure the completion_tracker class

[Andrew Burgess]

In this commit I rewrite how the completion tracker tracks the
completions, and builds its lowest common denominator (LCD) string.
The LCD string is now built lazily when required, and we only track
the completions in one place, the hash table, rather than maintaining
a separate vector of completions.

The motivation for these changes is that the next commit will add the
ability to remove completions from the list, removing a completion
will invalidate the LCD string, so we need to keep hold of enough
information to recompute the LCD string as needed.

Additionally, keeping the completions in a vector makes removing a
completion expensive, so better to only keep the completions in the
hash table.

This commit doesn't add any new functionality itself, and there should
be no user visible changes after this commit.

For testing, I ran the testsuite as usual, but I also ran some manual
completion tests under valgrind, and didn't get any reports about
leaked memory.

gdb/ChangeLog:

	* completer.c (completion_tracker::completion_hash_entry): Define
	new class.
	(advance_to_filename_complete_word_point): Call
	recompute_lowest_common_denominator.
	(completion_tracker::completion_tracker): Call discard_completions
	to setup the hash table.
	(completion_tracker::discard_completions): Allow for being called
	from the constructor, pass new equal function, and element deleter
	when constructing the hash table.  Initialise new class member
	variables.
	(completion_tracker::maybe_add_completion): Remove use of
	m_entries_vec, and store more information into m_entries_hash.
	(completion_tracker::recompute_lcd_visitor): New function, most
	content taken from...
	(completion_tracker::recompute_lowest_common_denominator):
	...here, this now just visits each item in the hash calling the
	above visitor.
	(completion_tracker::build_completion_result): Remove use of
	m_entries_vec, call recompute_lowest_common_denominator.
	* completer.h (completion_tracker::have_completions): Remove use
	of m_entries_vec.
	(completion_tracker::completion_hash_entry): Declare new class.
	(completion_tracker::recompute_lowest_common_denominator): Change
	function signature.
	(completion_tracker::recompute_lcd_visitor): Declare new function.
	(completion_tracker::m_entries_vec): Delete.
	(completion_tracker::m_entries_hash): Initialize to NULL.
	(completion_tracker::m_lowest_common_denominator_valid): New
	member variable.
	(completion_tracker::m_lowest_common_denominator_max_length): New
	member variable.

Change-Id: I9d1db52c489ca0041b8959ca0d53b7d3af8aea72
---
 gdb/ChangeLog   |  34 ++++++++++
 gdb/completer.c | 195 +++++++++++++++++++++++++++++++++++++++++++++++---------
 gdb/completer.h |  41 +++++++-----
 3 files changed, 222 insertions(+), 48 deletions(-)

diff --git a/gdb/completer.c b/gdb/completer.c
index 619fb8a0285..14c7a579970 100644
--- a/gdb/completer.c
+++ b/gdb/completer.c
@@ -45,6 +45,60 @@
 
 #include "completer.h"
 
+/* See completer.h.  */
+
+class completion_tracker::completion_hash_entry
+{
+public:
+  /* Constructor.  */
+  completion_hash_entry (gdb::unique_xmalloc_ptr<char> name,
+			 gdb::unique_xmalloc_ptr<char> lcd)
+    : m_name (std::move (name)),
+      m_lcd (std::move (lcd))
+  {
+    /* Nothing.  */
+  }
+
+  /* Returns a pointer to the lowest common denominator string.  This
+     string will only be valid while this hash entry is still valid as the
+     string continues to be owned by this hash entry and will be released
+     when this entry is deleted.  */
+  char *get_lcd () const
+  {
+    return m_lcd.get ();
+  }
+
+  /* Get, and release the name field from this hash entry.  This can only
+     be called once, after which the name field is no longer valid.  This
+     should be used to pass ownership of the name to someone else.  */
+  char *release_name ()
+  {
+    return m_name.release ();
+  }
+
+  /* Return true of the name in this hash entry is STR.  */
+  bool is_name_eq (const char *str) const
+  {
+    return strcmp (m_name.get (), str) == 0;
+  }
+
+  /* A static function that can be passed to the htab hash system to be
+     used as a callback that deletes an item from the hash.  */
+  static void deleter (void *arg)
+  {
+    completion_hash_entry *entry = (completion_hash_entry *) arg;
+    delete entry;
+  }
+
+private:
+
+  /* The symbol name stored in this hash entry.  */
+  gdb::unique_xmalloc_ptr<char> m_name;
+
+  /* The lowest common denominator string computed for this hash entry.  */
+  gdb::unique_xmalloc_ptr<char> m_lcd;
+};
+
 /* Misc state that needs to be tracked across several different
    readline completer entry point calls, all related to a single
    completion invocation.  */
@@ -407,6 +461,7 @@ advance_to_filename_complete_word_point (completion_tracker &tracker,
 bool
 completion_tracker::completes_to_completion_word (const char *word)
 {
+  recompute_lowest_common_denominator ();
   if (m_lowest_common_denominator_unique)
     {
       const char *lcd = m_lowest_common_denominator;
@@ -1512,9 +1567,7 @@ int max_completions = 200;
 
 completion_tracker::completion_tracker ()
 {
-  m_entries_hash = htab_create_alloc (INITIAL_COMPLETION_HTAB_SIZE,
-				      htab_hash_string, streq_hash,
-				      NULL, xcalloc, xfree);
+  discard_completions ();
 }
 
 /* See completer.h.  */
@@ -1526,13 +1579,33 @@ completion_tracker::discard_completions ()
   m_lowest_common_denominator = NULL;
 
   m_lowest_common_denominator_unique = false;
+  m_lowest_common_denominator_valid = false;
+
+  /* A null check here allows this function to be used from the
+     constructor.  */
+  if (m_entries_hash != NULL)
+    htab_delete (m_entries_hash);
+
+  /* A callback used by the hash table to compare new entries with existing
+     entries.  We can't use the standard streq_hash function here as the
+     key to our hash is just a single string, while the values we store in
+     the hash are a struct containing multiple strings.  */
+  static auto entry_eq_func
+    = [] (const void *first, const void *second) -> int
+      {
+	/* The FIRST argument is the entry already in the hash table, and
+	   the SECOND argument is the new item being inserted.  */
+	const completion_hash_entry *entry
+	  = (const completion_hash_entry *) first;
+	const char *name_str = (const char *) second;
 
-  m_entries_vec.clear ();
+	return entry->is_name_eq (name_str);
+      };
 
-  htab_delete (m_entries_hash);
   m_entries_hash = htab_create_alloc (INITIAL_COMPLETION_HTAB_SIZE,
-				      htab_hash_string, streq_hash,
-				      NULL, xcalloc, xfree);
+				      htab_hash_string, entry_eq_func,
+				      completion_hash_entry::deleter,
+				      xcalloc, xfree);
 }
 
 /* See completer.h.  */
@@ -1559,7 +1632,8 @@ completion_tracker::maybe_add_completion
   if (htab_elements (m_entries_hash) >= max_completions)
     return false;
 
-  slot = htab_find_slot (m_entries_hash, name.get (), INSERT);
+  hashval_t hash = htab_hash_string (name.get ());
+  slot = htab_find_slot_with_hash (m_entries_hash, name.get (), hash, INSERT);
   if (*slot == HTAB_EMPTY_ENTRY)
     {
       const char *match_for_lcd_str = NULL;
@@ -1573,10 +1647,12 @@ completion_tracker::maybe_add_completion
       gdb::unique_xmalloc_ptr<char> lcd
 	= make_completion_match_str (match_for_lcd_str, text, word);
 
-      recompute_lowest_common_denominator (std::move (lcd));
+      size_t lcd_len = strlen (lcd.get ());
+      *slot = new completion_hash_entry (std::move (name), std::move (lcd));
 
-      *slot = name.get ();
-      m_entries_vec.push_back (std::move (name));
+      m_lowest_common_denominator_valid = false;
+      m_lowest_common_denominator_max_length
+	= std::max (m_lowest_common_denominator_max_length, lcd_len);
     }
 
   return true;
@@ -1982,23 +2058,23 @@ completion_find_completion_word (completion_tracker &tracker, const char *text,
 /* See completer.h.  */
 
 void
-completion_tracker::recompute_lowest_common_denominator
-  (gdb::unique_xmalloc_ptr<char> &&new_match_up)
+completion_tracker::recompute_lcd_visitor (completion_hash_entry *entry)
 {
-  if (m_lowest_common_denominator == NULL)
+  if (!m_lowest_common_denominator_valid)
     {
-      /* We don't have a lowest common denominator yet, so simply take
-	 the whole NEW_MATCH_UP as being it.  */
-      m_lowest_common_denominator = new_match_up.release ();
+      /* This is the first lowest common denominator that we are
+	 considering, just copy it in.  */
+      strcpy (m_lowest_common_denominator, entry->get_lcd ());
       m_lowest_common_denominator_unique = true;
+      m_lowest_common_denominator_valid = true;
     }
   else
     {
-      /* Find the common denominator between the currently-known
-	 lowest common denominator and NEW_MATCH_UP.  That becomes the
-	 new lowest common denominator.  */
+      /* Find the common denominator between the currently-known lowest
+	 common denominator and NEW_MATCH_UP.  That becomes the new lowest
+	 common denominator.  */
       size_t i;
-      const char *new_match = new_match_up.get ();
+      const char *new_match = entry->get_lcd ();
 
       for (i = 0;
 	   (new_match[i] != '\0'
@@ -2015,6 +2091,35 @@ completion_tracker::recompute_lowest_common_denominator
 
 /* See completer.h.  */
 
+void
+completion_tracker::recompute_lowest_common_denominator ()
+{
+  /* We've already done this.  */
+  if (m_lowest_common_denominator_valid)
+    return;
+
+  /* Resize the storage to ensure we have enough space, the plus one gives
+     us space for the trailing null terminator we will include.  */
+  m_lowest_common_denominator
+    = (char *) xrealloc (m_lowest_common_denominator,
+			 m_lowest_common_denominator_max_length + 1);
+
+  /* Callback used to visit each entry in the m_entries_hash.  */
+  auto visitor_func
+    = [] (void **slot, void *info) -> int
+      {
+	completion_tracker *obj = (completion_tracker *) info;
+	completion_hash_entry *entry = (completion_hash_entry *) *slot;
+	obj->recompute_lcd_visitor (entry);
+	return 1;
+      };
+
+  htab_traverse (m_entries_hash, visitor_func, this);
+  m_lowest_common_denominator_valid = true;
+}
+
+/* See completer.h.  */
+
 void
 completion_tracker::advance_custom_word_point_by (int len)
 {
@@ -2092,16 +2197,17 @@ completion_result
 completion_tracker::build_completion_result (const char *text,
 					     int start, int end)
 {
-  completion_list &list = m_entries_vec;	/* The completions.  */
+  size_t element_count = htab_elements (m_entries_hash);
 
-  if (list.empty ())
+  if (element_count == 0)
     return {};
 
   /* +1 for the LCD, and +1 for NULL termination.  */
-  char **match_list = XNEWVEC (char *, 1 + list.size () + 1);
+  char **match_list = XNEWVEC (char *, 1 + element_count + 1);
 
   /* Build replacement word, based on the LCD.  */
 
+  recompute_lowest_common_denominator ();
   match_list[0]
     = expand_preserving_ws (text, end - start,
 			    m_lowest_common_denominator);
@@ -2128,13 +2234,40 @@ completion_tracker::build_completion_result (const char *text,
     }
   else
     {
-      int ix;
-
-      for (ix = 0; ix < list.size (); ++ix)
-	match_list[ix + 1] = list[ix].release ();
-      match_list[ix + 1] = NULL;
-
-      return completion_result (match_list, list.size (), false);
+      /* State object used while building the completion list.  */
+      struct list_builder
+      {
+	list_builder (char **ml)
+	  : match_list (ml),
+	    index (1)
+	{ /* Nothing.  */ }
+
+	/* The list we are filling.  */
+	char **match_list;
+
+	/* The next index in the list to write to.  */
+	int index;
+      };
+      list_builder builder (match_list);
+
+      /* Visit each entry in m_entries_hash and add it to the completion
+	 list, updating the builder state object.  */
+      auto func
+	= [] (void **slot, void *info) -> int
+	  {
+	    completion_hash_entry *entry = (completion_hash_entry *) *slot;
+	    list_builder *state = (list_builder *) info;
+
+	    state->match_list[state->index] = entry->release_name ();
+	    state->index++;
+	    return 1;
+	  };
+
+      /* Build the completion list and add a null at the end.  */
+      htab_traverse_noresize (m_entries_hash, func, &builder);
+      match_list[builder.index] = NULL;
+
+      return completion_result (match_list, builder.index - 1, false);
     }
 }
 
diff --git a/gdb/completer.h b/gdb/completer.h
index 703a5768d11..7bfe0d58142 100644
--- a/gdb/completer.h
+++ b/gdb/completer.h
@@ -389,7 +389,7 @@ public:
 
   /* True if we have any completion match recorded.  */
   bool have_completions () const
-  { return !m_entries_vec.empty (); }
+  { return htab_elements (m_entries_hash) > 0; }
 
   /* Discard the current completion match list and the current
      LCD.  */
@@ -403,6 +403,9 @@ public:
 
 private:
 
+  /* The type that we place into the m_entries_hash hash table.  */
+  class completion_hash_entry;
+
   /* Add the completion NAME to the list of generated completions if
      it is not there already.  If false is returned, too many
      completions were found.  */
@@ -410,18 +413,15 @@ private:
 			     completion_match_for_lcd *match_for_lcd,
 			     const char *text, const char *word);
 
-  /* Given a new match, recompute the lowest common denominator (LCD)
-     to hand over to readline.  Normally readline computes this itself
-     based on the whole set of completion matches.  However, some
-     completers want to override readline, in order to be able to
-     provide a LCD that is not really a prefix of the matches, but the
-     lowest common denominator of some relevant substring of each
-     match.  E.g., "b push_ba" completes to
-     "std::vector<..>::push_back", "std::string::push_back", etc., and
-     in this case we want the lowest common denominator to be
-     "push_back" instead of "std::".  */
-  void recompute_lowest_common_denominator
-    (gdb::unique_xmalloc_ptr<char> &&new_match);
+  /* Ensure that the lowest common denominator held in the member variable
+     M_LOWEST_COMMON_DENOMINATOR is valid.  This method must be called if
+     there is any chance that new completions have been added to the
+     tracker before the lowest common denominator is read.  */
+  void recompute_lowest_common_denominator ();
+
+  /* Callback used from recompute_lowest_common_denominator, called for
+     every entry in m_entries_hash.  */
+  void recompute_lcd_visitor (completion_hash_entry *entry);
 
   /* Completion match outputs returned by the symbol name matching
      routines (see symbol_name_matcher_ftype).  These results are only
@@ -430,16 +430,13 @@ private:
      symbol name matching routines.  */
   completion_match_result m_completion_match_result;
 
-  /* The completion matches found so far, in a vector.  */
-  completion_list m_entries_vec;
-
   /* The completion matches found so far, in a hash table, for
      duplicate elimination as entries are added.  Otherwise the user
      is left scratching his/her head: readline and complete_command
      will remove duplicates, and if removal of duplicates there brings
      the total under max_completions the user may think gdb quit
      searching too early.  */
-  htab_t m_entries_hash;
+  htab_t m_entries_hash = NULL;
 
   /* If non-zero, then this is the quote char that needs to be
      appended after completion (iff we have a unique completion).  We
@@ -483,6 +480,16 @@ private:
      "function()", instead of showing all the possible
      completions.  */
   bool m_lowest_common_denominator_unique = false;
+
+  /* True if the value in M_LOWEST_COMMON_DENOMINATOR is correct.  This is
+     set to true each time RECOMPUTE_LOWEST_COMMON_DENOMINATOR is called,
+     and reset to false whenever a new completion is added.  */
+  bool m_lowest_common_denominator_valid = false;
+
+  /* To avoid calls to xrealloc in RECOMPUTE_LOWEST_COMMON_DENOMINATOR, we
+     track the maximum possible size of the lowest common denominator,
+     which we know as each completion is added.  */
+  size_t m_lowest_common_denominator_max_length = 0;
 };
 
 /* Return a string to hand off to readline as a completion match
-- 
2.14.5

[PATCHv2 0/3] Remove C++ Symbol Aliases From Completion List

[Andrew Burgess]

After feedback to the V1 patch I revisited the first patch in the
series, and decided to drop the conversion to C++ STL types.

This series keeps the existing libiberty hash table data structure,
but otherwise, is basically doing the same thing.

I ran into a small annoyance of needing to add a 'const' into the
libiberty data structure API, which I know needs to be submitted to
GCC, but will be needed here if anyone wants to test the patch.

Feedback welcome.

Thanks,
Andrew


---

Andrew Burgess (3):
  libiberty/hashtab: More const parameters
  gdb: Restructure the completion_tracker class
  gdb: Remove C++ symbol aliases from completion list

 gdb/ChangeLog                                      |  43 +++++
 gdb/completer.c                                    | 209 ++++++++++++++++++---
 gdb/completer.h                                    |  45 +++--
 gdb/symtab.c                                       |  21 +++
 gdb/testsuite/ChangeLog                            |   5 +
 .../gdb.linespec/cp-completion-aliases.cc          |  73 +++++++
 .../gdb.linespec/cp-completion-aliases.exp         |  57 ++++++
 include/ChangeLog                                  |   5 +
 include/hashtab.h                                  |   4 +-
 libiberty/ChangeLog                                |   5 +
 libiberty/hashtab.c                                |   4 +-
 11 files changed, 419 insertions(+), 52 deletions(-)
 create mode 100644 gdb/testsuite/gdb.linespec/cp-completion-aliases.cc
 create mode 100644 gdb/testsuite/gdb.linespec/cp-completion-aliases.exp

-- 
2.14.5

[PATCHv2 1/3] libiberty/hashtab: More const parameters

[Andrew Burgess]

Makes some parameters const in libiberty's hashtab library.

include/ChangeLog:

	* hashtab.h (htab_remove_elt): Make a parameter const.
	(htab_remove_elt_with_hash): Likewise.

libiberty/ChangeLog:

	* hashtab.c (htab_remove_elt): Make a parameter const.
	(htab_remove_elt_with_hash): Likewise.

Change-Id: Id416d5c9274285221533e3128c90485ba27846f2
---
 include/ChangeLog   | 5 +++++
 include/hashtab.h   | 4 ++--
 libiberty/ChangeLog | 5 +++++
 libiberty/hashtab.c | 4 ++--
 4 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/include/hashtab.h b/include/hashtab.h
index d94b54c3c41..6cca342b989 100644
--- a/include/hashtab.h
+++ b/include/hashtab.h
@@ -173,8 +173,8 @@ extern void *	htab_find_with_hash (htab_t, const void *, hashval_t);
 extern void **	htab_find_slot_with_hash (htab_t, const void *,
 					  hashval_t, enum insert_option);
 extern void	htab_clear_slot	(htab_t, void **);
-extern void	htab_remove_elt	(htab_t, void *);
-extern void	htab_remove_elt_with_hash (htab_t, void *, hashval_t);
+extern void	htab_remove_elt	(htab_t, const void *);
+extern void	htab_remove_elt_with_hash (htab_t, const void *, hashval_t);
 
 extern void	htab_traverse (htab_t, htab_trav, void *);
 extern void	htab_traverse_noresize (htab_t, htab_trav, void *);
diff --git a/libiberty/hashtab.c b/libiberty/hashtab.c
index 26c98ce2d68..225e9e540a7 100644
--- a/libiberty/hashtab.c
+++ b/libiberty/hashtab.c
@@ -709,7 +709,7 @@ htab_find_slot (htab_t htab, const PTR element, enum insert_option insert)
    element in the hash table, this function does nothing.  */
 
 void
-htab_remove_elt (htab_t htab, PTR element)
+htab_remove_elt (htab_t htab, const PTR element)
 {
   htab_remove_elt_with_hash (htab, element, (*htab->hash_f) (element));
 }
@@ -720,7 +720,7 @@ htab_remove_elt (htab_t htab, PTR element)
    function does nothing.  */
 
 void
-htab_remove_elt_with_hash (htab_t htab, PTR element, hashval_t hash)
+htab_remove_elt_with_hash (htab_t htab, const PTR element, hashval_t hash)
 {
   PTR *slot;
 
-- 
2.14.5

Re: [PATCHv2 1/3] libiberty/hashtab: More const parameters

[Andrew Burgess]

* Andrew Burgess <andrew.burgess@embecosm.com> [2020-01-28 00:36:57 +0000]:

> Makes some parameters const in libiberty's hashtab library.
> 
> include/ChangeLog:
> 
> 	* hashtab.h (htab_remove_elt): Make a parameter const.
> 	(htab_remove_elt_with_hash): Likewise.
> 
> libiberty/ChangeLog:
> 
> 	* hashtab.c (htab_remove_elt): Make a parameter const.
> 	(htab_remove_elt_with_hash): Likewise.

This patch has now been merged.

Thanks,
Andrew

> 
> Change-Id: Id416d5c9274285221533e3128c90485ba27846f2
> ---
>  include/ChangeLog   | 5 +++++
>  include/hashtab.h   | 4 ++--
>  libiberty/ChangeLog | 5 +++++
>  libiberty/hashtab.c | 4 ++--
>  4 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/include/hashtab.h b/include/hashtab.h
> index d94b54c3c41..6cca342b989 100644
> --- a/include/hashtab.h
> +++ b/include/hashtab.h
> @@ -173,8 +173,8 @@ extern void *	htab_find_with_hash (htab_t, const void *, hashval_t);
>  extern void **	htab_find_slot_with_hash (htab_t, const void *,
>  					  hashval_t, enum insert_option);
>  extern void	htab_clear_slot	(htab_t, void **);
> -extern void	htab_remove_elt	(htab_t, void *);
> -extern void	htab_remove_elt_with_hash (htab_t, void *, hashval_t);
> +extern void	htab_remove_elt	(htab_t, const void *);
> +extern void	htab_remove_elt_with_hash (htab_t, const void *, hashval_t);
>  
>  extern void	htab_traverse (htab_t, htab_trav, void *);
>  extern void	htab_traverse_noresize (htab_t, htab_trav, void *);
> diff --git a/libiberty/hashtab.c b/libiberty/hashtab.c
> index 26c98ce2d68..225e9e540a7 100644
> --- a/libiberty/hashtab.c
> +++ b/libiberty/hashtab.c
> @@ -709,7 +709,7 @@ htab_find_slot (htab_t htab, const PTR element, enum insert_option insert)
>     element in the hash table, this function does nothing.  */
>  
>  void
> -htab_remove_elt (htab_t htab, PTR element)
> +htab_remove_elt (htab_t htab, const PTR element)
>  {
>    htab_remove_elt_with_hash (htab, element, (*htab->hash_f) (element));
>  }
> @@ -720,7 +720,7 @@ htab_remove_elt (htab_t htab, PTR element)
>     function does nothing.  */
>  
>  void
> -htab_remove_elt_with_hash (htab_t htab, PTR element, hashval_t hash)
> +htab_remove_elt_with_hash (htab_t htab, const PTR element, hashval_t hash)
>  {
>    PTR *slot;
>  
> -- 
> 2.14.5
> 

Re: [PATCHv2 0/3] Remove C++ Symbol Aliases From Completion List

[Andrew Burgess]

Ping!

Anyone got any thoughts on parts 2 & 3 of the revised series?

Thanks,
Andrew


* Andrew Burgess <andrew.burgess@embecosm.com> [2020-01-28 00:36:56 +0000]:

> After feedback to the V1 patch I revisited the first patch in the
> series, and decided to drop the conversion to C++ STL types.
> 
> This series keeps the existing libiberty hash table data structure,
> but otherwise, is basically doing the same thing.
> 
> I ran into a small annoyance of needing to add a 'const' into the
> libiberty data structure API, which I know needs to be submitted to
> GCC, but will be needed here if anyone wants to test the patch.
> 
> Feedback welcome.
> 
> Thanks,
> Andrew
> 
> 
> ---
> 
> Andrew Burgess (3):
>   libiberty/hashtab: More const parameters
>   gdb: Restructure the completion_tracker class
>   gdb: Remove C++ symbol aliases from completion list
> 
>  gdb/ChangeLog                                      |  43 +++++
>  gdb/completer.c                                    | 209 ++++++++++++++++++---
>  gdb/completer.h                                    |  45 +++--
>  gdb/symtab.c                                       |  21 +++
>  gdb/testsuite/ChangeLog                            |   5 +
>  .../gdb.linespec/cp-completion-aliases.cc          |  73 +++++++
>  .../gdb.linespec/cp-completion-aliases.exp         |  57 ++++++
>  include/ChangeLog                                  |   5 +
>  include/hashtab.h                                  |   4 +-
>  libiberty/ChangeLog                                |   5 +
>  libiberty/hashtab.c                                |   4 +-
>  11 files changed, 419 insertions(+), 52 deletions(-)
>  create mode 100644 gdb/testsuite/gdb.linespec/cp-completion-aliases.cc
>  create mode 100644 gdb/testsuite/gdb.linespec/cp-completion-aliases.exp
> 
> -- 
> 2.14.5
> 

Re: [PATCHv2 0/3] Remove C++ Symbol Aliases From Completion List

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> Ping!
Andrew> Anyone got any thoughts on parts 2 & 3 of the revised series?

Sorry about that.  I read them now and they look good to me.  Thanks
again for these.

Tom

Re: [PATCHv2 2/3] gdb: Restructure the completion_tracker class

[Luis Machado]

Hi Andrew,

Just a heads-up that this has caused a regression for aarch64-linux in 
gdb.base/completion.exp.

FAIL: gdb.base/completion.exp: complete 'info registers '

For some reason the completion code is printing "info registers pc" 
twice. It may be a quirk in the target, but i thought I'd let you know 
before i investigate this further, in case you have any ideas.

Luis

On 1/27/20 9:36 PM, Andrew Burgess wrote:
> In this commit I rewrite how the completion tracker tracks the
> completions, and builds its lowest common denominator (LCD) string.
> The LCD string is now built lazily when required, and we only track
> the completions in one place, the hash table, rather than maintaining
> a separate vector of completions.
> 
> The motivation for these changes is that the next commit will add the
> ability to remove completions from the list, removing a completion
> will invalidate the LCD string, so we need to keep hold of enough
> information to recompute the LCD string as needed.
> 
> Additionally, keeping the completions in a vector makes removing a
> completion expensive, so better to only keep the completions in the
> hash table.
> 
> This commit doesn't add any new functionality itself, and there should
> be no user visible changes after this commit.
> 
> For testing, I ran the testsuite as usual, but I also ran some manual
> completion tests under valgrind, and didn't get any reports about
> leaked memory.
> 
> gdb/ChangeLog:
> 
> 	* completer.c (completion_tracker::completion_hash_entry): Define
> 	new class.
> 	(advance_to_filename_complete_word_point): Call
> 	recompute_lowest_common_denominator.
> 	(completion_tracker::completion_tracker): Call discard_completions
> 	to setup the hash table.
> 	(completion_tracker::discard_completions): Allow for being called
> 	from the constructor, pass new equal function, and element deleter
> 	when constructing the hash table.  Initialise new class member
> 	variables.
> 	(completion_tracker::maybe_add_completion): Remove use of
> 	m_entries_vec, and store more information into m_entries_hash.
> 	(completion_tracker::recompute_lcd_visitor): New function, most
> 	content taken from...
> 	(completion_tracker::recompute_lowest_common_denominator):
> 	...here, this now just visits each item in the hash calling the
> 	above visitor.
> 	(completion_tracker::build_completion_result): Remove use of
> 	m_entries_vec, call recompute_lowest_common_denominator.
> 	* completer.h (completion_tracker::have_completions): Remove use
> 	of m_entries_vec.
> 	(completion_tracker::completion_hash_entry): Declare new class.
> 	(completion_tracker::recompute_lowest_common_denominator): Change
> 	function signature.
> 	(completion_tracker::recompute_lcd_visitor): Declare new function.
> 	(completion_tracker::m_entries_vec): Delete.
> 	(completion_tracker::m_entries_hash): Initialize to NULL.
> 	(completion_tracker::m_lowest_common_denominator_valid): New
> 	member variable.
> 	(completion_tracker::m_lowest_common_denominator_max_length): New
> 	member variable.
> 
> Change-Id: I9d1db52c489ca0041b8959ca0d53b7d3af8aea72
> ---
>   gdb/ChangeLog   |  34 ++++++++++
>   gdb/completer.c | 195 +++++++++++++++++++++++++++++++++++++++++++++++---------
>   gdb/completer.h |  41 +++++++-----
>   3 files changed, 222 insertions(+), 48 deletions(-)
> 
> diff --git a/gdb/completer.c b/gdb/completer.c
> index 619fb8a0285..14c7a579970 100644
> --- a/gdb/completer.c
> +++ b/gdb/completer.c
> @@ -45,6 +45,60 @@
>   
>   #include "completer.h"
>   
> +/* See completer.h.  */
> +
> +class completion_tracker::completion_hash_entry
> +{
> +public:
> +  /* Constructor.  */
> +  completion_hash_entry (gdb::unique_xmalloc_ptr<char> name,
> +			 gdb::unique_xmalloc_ptr<char> lcd)
> +    : m_name (std::move (name)),
> +      m_lcd (std::move (lcd))
> +  {
> +    /* Nothing.  */
> +  }
> +
> +  /* Returns a pointer to the lowest common denominator string.  This
> +     string will only be valid while this hash entry is still valid as the
> +     string continues to be owned by this hash entry and will be released
> +     when this entry is deleted.  */
> +  char *get_lcd () const
> +  {
> +    return m_lcd.get ();
> +  }
> +
> +  /* Get, and release the name field from this hash entry.  This can only
> +     be called once, after which the name field is no longer valid.  This
> +     should be used to pass ownership of the name to someone else.  */
> +  char *release_name ()
> +  {
> +    return m_name.release ();
> +  }
> +
> +  /* Return true of the name in this hash entry is STR.  */
> +  bool is_name_eq (const char *str) const
> +  {
> +    return strcmp (m_name.get (), str) == 0;
> +  }
> +
> +  /* A static function that can be passed to the htab hash system to be
> +     used as a callback that deletes an item from the hash.  */
> +  static void deleter (void *arg)
> +  {
> +    completion_hash_entry *entry = (completion_hash_entry *) arg;
> +    delete entry;
> +  }
> +
> +private:
> +
> +  /* The symbol name stored in this hash entry.  */
> +  gdb::unique_xmalloc_ptr<char> m_name;
> +
> +  /* The lowest common denominator string computed for this hash entry.  */
> +  gdb::unique_xmalloc_ptr<char> m_lcd;
> +};
> +
>   /* Misc state that needs to be tracked across several different
>      readline completer entry point calls, all related to a single
>      completion invocation.  */
> @@ -407,6 +461,7 @@ advance_to_filename_complete_word_point (completion_tracker &tracker,
>   bool
>   completion_tracker::completes_to_completion_word (const char *word)
>   {
> +  recompute_lowest_common_denominator ();
>     if (m_lowest_common_denominator_unique)
>       {
>         const char *lcd = m_lowest_common_denominator;
> @@ -1512,9 +1567,7 @@ int max_completions = 200;
>   
>   completion_tracker::completion_tracker ()
>   {
> -  m_entries_hash = htab_create_alloc (INITIAL_COMPLETION_HTAB_SIZE,
> -				      htab_hash_string, streq_hash,
> -				      NULL, xcalloc, xfree);
> +  discard_completions ();
>   }
>   
>   /* See completer.h.  */
> @@ -1526,13 +1579,33 @@ completion_tracker::discard_completions ()
>     m_lowest_common_denominator = NULL;
>   
>     m_lowest_common_denominator_unique = false;
> +  m_lowest_common_denominator_valid = false;
> +
> +  /* A null check here allows this function to be used from the
> +     constructor.  */
> +  if (m_entries_hash != NULL)
> +    htab_delete (m_entries_hash);
> +
> +  /* A callback used by the hash table to compare new entries with existing
> +     entries.  We can't use the standard streq_hash function here as the
> +     key to our hash is just a single string, while the values we store in
> +     the hash are a struct containing multiple strings.  */
> +  static auto entry_eq_func
> +    = [] (const void *first, const void *second) -> int
> +      {
> +	/* The FIRST argument is the entry already in the hash table, and
> +	   the SECOND argument is the new item being inserted.  */
> +	const completion_hash_entry *entry
> +	  = (const completion_hash_entry *) first;
> +	const char *name_str = (const char *) second;
>   
> -  m_entries_vec.clear ();
> +	return entry->is_name_eq (name_str);
> +      };
>   
> -  htab_delete (m_entries_hash);
>     m_entries_hash = htab_create_alloc (INITIAL_COMPLETION_HTAB_SIZE,
> -				      htab_hash_string, streq_hash,
> -				      NULL, xcalloc, xfree);
> +				      htab_hash_string, entry_eq_func,
> +				      completion_hash_entry::deleter,
> +				      xcalloc, xfree);
>   }
>   
>   /* See completer.h.  */
> @@ -1559,7 +1632,8 @@ completion_tracker::maybe_add_completion
>     if (htab_elements (m_entries_hash) >= max_completions)
>       return false;
>   
> -  slot = htab_find_slot (m_entries_hash, name.get (), INSERT);
> +  hashval_t hash = htab_hash_string (name.get ());
> +  slot = htab_find_slot_with_hash (m_entries_hash, name.get (), hash, INSERT);
>     if (*slot == HTAB_EMPTY_ENTRY)
>       {
>         const char *match_for_lcd_str = NULL;
> @@ -1573,10 +1647,12 @@ completion_tracker::maybe_add_completion
>         gdb::unique_xmalloc_ptr<char> lcd
>   	= make_completion_match_str (match_for_lcd_str, text, word);
>   
> -      recompute_lowest_common_denominator (std::move (lcd));
> +      size_t lcd_len = strlen (lcd.get ());
> +      *slot = new completion_hash_entry (std::move (name), std::move (lcd));
>   
> -      *slot = name.get ();
> -      m_entries_vec.push_back (std::move (name));
> +      m_lowest_common_denominator_valid = false;
> +      m_lowest_common_denominator_max_length
> +	= std::max (m_lowest_common_denominator_max_length, lcd_len);
>       }
>   
>     return true;
> @@ -1982,23 +2058,23 @@ completion_find_completion_word (completion_tracker &tracker, const char *text,
>   /* See completer.h.  */
>   
>   void
> -completion_tracker::recompute_lowest_common_denominator
> -  (gdb::unique_xmalloc_ptr<char> &&new_match_up)
> +completion_tracker::recompute_lcd_visitor (completion_hash_entry *entry)
>   {
> -  if (m_lowest_common_denominator == NULL)
> +  if (!m_lowest_common_denominator_valid)
>       {
> -      /* We don't have a lowest common denominator yet, so simply take
> -	 the whole NEW_MATCH_UP as being it.  */
> -      m_lowest_common_denominator = new_match_up.release ();
> +      /* This is the first lowest common denominator that we are
> +	 considering, just copy it in.  */
> +      strcpy (m_lowest_common_denominator, entry->get_lcd ());
>         m_lowest_common_denominator_unique = true;
> +      m_lowest_common_denominator_valid = true;
>       }
>     else
>       {
> -      /* Find the common denominator between the currently-known
> -	 lowest common denominator and NEW_MATCH_UP.  That becomes the
> -	 new lowest common denominator.  */
> +      /* Find the common denominator between the currently-known lowest
> +	 common denominator and NEW_MATCH_UP.  That becomes the new lowest
> +	 common denominator.  */
>         size_t i;
> -      const char *new_match = new_match_up.get ();
> +      const char *new_match = entry->get_lcd ();
>   
>         for (i = 0;
>   	   (new_match[i] != '\0'
> @@ -2015,6 +2091,35 @@ completion_tracker::recompute_lowest_common_denominator
>   
>   /* See completer.h.  */
>   
> +void
> +completion_tracker::recompute_lowest_common_denominator ()
> +{
> +  /* We've already done this.  */
> +  if (m_lowest_common_denominator_valid)
> +    return;
> +
> +  /* Resize the storage to ensure we have enough space, the plus one gives
> +     us space for the trailing null terminator we will include.  */
> +  m_lowest_common_denominator
> +    = (char *) xrealloc (m_lowest_common_denominator,
> +			 m_lowest_common_denominator_max_length + 1);
> +
> +  /* Callback used to visit each entry in the m_entries_hash.  */
> +  auto visitor_func
> +    = [] (void **slot, void *info) -> int
> +      {
> +	completion_tracker *obj = (completion_tracker *) info;
> +	completion_hash_entry *entry = (completion_hash_entry *) *slot;
> +	obj->recompute_lcd_visitor (entry);
> +	return 1;
> +      };
> +
> +  htab_traverse (m_entries_hash, visitor_func, this);
> +  m_lowest_common_denominator_valid = true;
> +}
> +
> +/* See completer.h.  */
> +
>   void
>   completion_tracker::advance_custom_word_point_by (int len)
>   {
> @@ -2092,16 +2197,17 @@ completion_result
>   completion_tracker::build_completion_result (const char *text,
>   					     int start, int end)
>   {
> -  completion_list &list = m_entries_vec;	/* The completions.  */
> +  size_t element_count = htab_elements (m_entries_hash);
>   
> -  if (list.empty ())
> +  if (element_count == 0)
>       return {};
>   
>     /* +1 for the LCD, and +1 for NULL termination.  */
> -  char **match_list = XNEWVEC (char *, 1 + list.size () + 1);
> +  char **match_list = XNEWVEC (char *, 1 + element_count + 1);
>   
>     /* Build replacement word, based on the LCD.  */
>   
> +  recompute_lowest_common_denominator ();
>     match_list[0]
>       = expand_preserving_ws (text, end - start,
>   			    m_lowest_common_denominator);
> @@ -2128,13 +2234,40 @@ completion_tracker::build_completion_result (const char *text,
>       }
>     else
>       {
> -      int ix;
> -
> -      for (ix = 0; ix < list.size (); ++ix)
> -	match_list[ix + 1] = list[ix].release ();
> -      match_list[ix + 1] = NULL;
> -
> -      return completion_result (match_list, list.size (), false);
> +      /* State object used while building the completion list.  */
> +      struct list_builder
> +      {
> +	list_builder (char **ml)
> +	  : match_list (ml),
> +	    index (1)
> +	{ /* Nothing.  */ }
> +
> +	/* The list we are filling.  */
> +	char **match_list;
> +
> +	/* The next index in the list to write to.  */
> +	int index;
> +      };
> +      list_builder builder (match_list);
> +
> +      /* Visit each entry in m_entries_hash and add it to the completion
> +	 list, updating the builder state object.  */
> +      auto func
> +	= [] (void **slot, void *info) -> int
> +	  {
> +	    completion_hash_entry *entry = (completion_hash_entry *) *slot;
> +	    list_builder *state = (list_builder *) info;
> +
> +	    state->match_list[state->index] = entry->release_name ();
> +	    state->index++;
> +	    return 1;
> +	  };
> +
> +      /* Build the completion list and add a null at the end.  */
> +      htab_traverse_noresize (m_entries_hash, func, &builder);
> +      match_list[builder.index] = NULL;
> +
> +      return completion_result (match_list, builder.index - 1, false);
>       }
>   }
>   
> diff --git a/gdb/completer.h b/gdb/completer.h
> index 703a5768d11..7bfe0d58142 100644
> --- a/gdb/completer.h
> +++ b/gdb/completer.h
> @@ -389,7 +389,7 @@ public:
>   
>     /* True if we have any completion match recorded.  */
>     bool have_completions () const
> -  { return !m_entries_vec.empty (); }
> +  { return htab_elements (m_entries_hash) > 0; }
>   
>     /* Discard the current completion match list and the current
>        LCD.  */
> @@ -403,6 +403,9 @@ public:
>   
>   private:
>   
> +  /* The type that we place into the m_entries_hash hash table.  */
> +  class completion_hash_entry;
> +
>     /* Add the completion NAME to the list of generated completions if
>        it is not there already.  If false is returned, too many
>        completions were found.  */
> @@ -410,18 +413,15 @@ private:
>   			     completion_match_for_lcd *match_for_lcd,
>   			     const char *text, const char *word);
>   
> -  /* Given a new match, recompute the lowest common denominator (LCD)
> -     to hand over to readline.  Normally readline computes this itself
> -     based on the whole set of completion matches.  However, some
> -     completers want to override readline, in order to be able to
> -     provide a LCD that is not really a prefix of the matches, but the
> -     lowest common denominator of some relevant substring of each
> -     match.  E.g., "b push_ba" completes to
> -     "std::vector<..>::push_back", "std::string::push_back", etc., and
> -     in this case we want the lowest common denominator to be
> -     "push_back" instead of "std::".  */
> -  void recompute_lowest_common_denominator
> -    (gdb::unique_xmalloc_ptr<char> &&new_match);
> +  /* Ensure that the lowest common denominator held in the member variable
> +     M_LOWEST_COMMON_DENOMINATOR is valid.  This method must be called if
> +     there is any chance that new completions have been added to the
> +     tracker before the lowest common denominator is read.  */
> +  void recompute_lowest_common_denominator ();
> +
> +  /* Callback used from recompute_lowest_common_denominator, called for
> +     every entry in m_entries_hash.  */
> +  void recompute_lcd_visitor (completion_hash_entry *entry);
>   
>     /* Completion match outputs returned by the symbol name matching
>        routines (see symbol_name_matcher_ftype).  These results are only
> @@ -430,16 +430,13 @@ private:
>        symbol name matching routines.  */
>     completion_match_result m_completion_match_result;
>   
> -  /* The completion matches found so far, in a vector.  */
> -  completion_list m_entries_vec;
> -
>     /* The completion matches found so far, in a hash table, for
>        duplicate elimination as entries are added.  Otherwise the user
>        is left scratching his/her head: readline and complete_command
>        will remove duplicates, and if removal of duplicates there brings
>        the total under max_completions the user may think gdb quit
>        searching too early.  */
> -  htab_t m_entries_hash;
> +  htab_t m_entries_hash = NULL;
>   
>     /* If non-zero, then this is the quote char that needs to be
>        appended after completion (iff we have a unique completion).  We
> @@ -483,6 +480,16 @@ private:
>        "function()", instead of showing all the possible
>        completions.  */
>     bool m_lowest_common_denominator_unique = false;
> +
> +  /* True if the value in M_LOWEST_COMMON_DENOMINATOR is correct.  This is
> +     set to true each time RECOMPUTE_LOWEST_COMMON_DENOMINATOR is called,
> +     and reset to false whenever a new completion is added.  */
> +  bool m_lowest_common_denominator_valid = false;
> +
> +  /* To avoid calls to xrealloc in RECOMPUTE_LOWEST_COMMON_DENOMINATOR, we
> +     track the maximum possible size of the lowest common denominator,
> +     which we know as each completion is added.  */
> +  size_t m_lowest_common_denominator_max_length = 0;
>   };
>   
>   /* Return a string to hand off to readline as a completion match
> 

[PATCH] gdb: Don't corrupt completions hash when expanding the hash table

[Andrew Burgess]

Commit:

  commit 724fd9ba432a20ef2e3f2c0d6060bff131226816
  Date:   Mon Jan 27 17:37:20 2020 +0000

      gdb: Restructure the completion_tracker class

caused the completion hash table to become corrupted if the table ever
needed to grow beyond its original size of 200 elements.

The hash table stores completion_tracker::completion_hash_entry
objects, but hashes them based on their name, which is only one field
of the object.

When possibly inserting a new element we compute the hash with
htab_hash_string of the new elements name, and then lookup matching
elements using htab_find_slot_with_hash.  If there's not matching
element we create a completion_hash_entry object within the hash
table.

However, when we allocate the hash we pass htab_hash_string to
htab_create_alloc as the hash function, and this is not OK.  This
means that when the hash table needs to grow, existing elements within
the hash are re-hashed by passing the completion_hash_entry pointer to
htab_hash_string, which obviously does not do what we expect.

The solution is to create a new hash function that takes a pointer to
a completion_hash_entry, and then calls htab_hash_string on the name
of the entry only.

This regression was spotted when running the gdb.base/completion.exp
test on the aarch64 target.

gdb/ChangeLog:

	* completer.c (class completion_tracker::completion_hash_entry)
	<hash_name>: New member function.
	(completion_tracker::discard_completions): New callback to hash a
	completion_hash_entry, pass this to htab_create_alloc.

gdb/testsuite/ChangeLog:

	* gdb.base/many-completions.exp: New file.
---
 gdb/ChangeLog                               |  7 ++
 gdb/completer.c                             | 18 +++-
 gdb/testsuite/ChangeLog                     |  4 +
 gdb/testsuite/gdb.base/many-completions.exp | 92 +++++++++++++++++++++
 4 files changed, 120 insertions(+), 1 deletion(-)
 create mode 100644 gdb/testsuite/gdb.base/many-completions.exp

diff --git a/gdb/completer.c b/gdb/completer.c
index 67dce30fbe3..0dd91a7195f 100644
--- a/gdb/completer.c
+++ b/gdb/completer.c
@@ -82,6 +82,12 @@ class completion_tracker::completion_hash_entry
     return strcmp (m_name.get (), str) == 0;
   }
 
+  /* Return the hash value based on the name of the entry.  */
+  hashval_t hash_name () const
+  {
+    return htab_hash_string (m_name.get ());
+  }
+
   /* A static function that can be passed to the htab hash system to be
      used as a callback that deletes an item from the hash.  */
   static void deleter (void *arg)
@@ -1602,8 +1608,18 @@ completion_tracker::discard_completions ()
 	return entry->is_name_eq (name_str);
       };
 
+  /* Callback used by the hash table to compute the hash value for an
+     existing entry.  This is needed when expanding the hash table.  */
+  static auto entry_hash_func
+    = [] (const void *arg) -> hashval_t
+      {
+	const completion_hash_entry *entry
+	  = (const completion_hash_entry *) arg;
+	return entry->hash_name ();
+      };
+
   m_entries_hash = htab_create_alloc (INITIAL_COMPLETION_HTAB_SIZE,
-				      htab_hash_string, entry_eq_func,
+				      entry_hash_func, entry_eq_func,
 				      completion_hash_entry::deleter,
 				      xcalloc, xfree);
 }
diff --git a/gdb/testsuite/gdb.base/many-completions.exp b/gdb/testsuite/gdb.base/many-completions.exp
new file mode 100644
index 00000000000..9597963abba
--- /dev/null
+++ b/gdb/testsuite/gdb.base/many-completions.exp
@@ -0,0 +1,92 @@
+# Copyright 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/>.
+
+# Test the case where we have so many completions that we require the
+# completions hash table within GDB to grow.  Make sure that afte the
+# hash table has grown we try to add duplicate entries into the
+# hash. This checks that GDB doesn't corrupt the hash table when
+# resizing it.
+#
+# In this case we create a test with more functions than the default
+# number of entires in the completion hash table (which is 200), then
+# complete on all function names.
+#
+# GDB will add all the function names from the DWARF, and then from
+# the ELF symbol table, this ensures that we should have duplicates
+# added after resizing the table.
+
+# Create a test source file and return the name of the file.  COUNT is
+# the number of dummy functions to create, this should be more than
+# the default number of entries in the completion hash table within
+# GDB (see gdb/completer.c).
+proc prepare_test_source_file { count } {
+    global gdb_test_file_name
+
+    set filename [standard_output_file "$gdb_test_file_name.c"]
+    set outfile [open $filename w]
+
+    puts $outfile "
+#define MAKE_FUNC(NUM) \\
+  void                 \\
+  func_ ## NUM (void)  \\
+  { /* Nothing.  */ }
+
+#define CALL_FUNC(NUM) \\
+  func_ ## NUM ()
+"
+
+    for { set i 0 } { $i < $count } { incr i } {
+	puts $outfile "MAKE_FUNC ([format {%03d} $i])"
+    }
+
+    puts $outfile "\nint\nmain ()\n{"
+    for { set i 0 } { $i < $count } { incr i } {
+	puts $outfile "  CALL_FUNC ([format {%03d} $i]);"
+    }
+
+    puts $outfile "  return 0;\n}"
+    close $outfile
+
+    return $filename
+}
+
+# Build a source file and compile it.
+set filename [prepare_test_source_file 250]
+standard_testfile $filename
+if {[prepare_for_testing "failed to prepare" "$testfile" $srcfile \
+	 { debug }]} {
+    return -1
+}
+
+# Start the test.
+if {![runto_main]} {
+    fail "couldn't run to main"
+    return
+}
+
+# We don't want to stop gathering completions too early.
+gdb_test_no_output "set max-completions unlimited"
+
+# Collect all possible completions, and check for duplictes.
+set completions [capture_command_output "complete break func_" ""]
+set duplicates 0
+foreach {-> name} [regexp -all -inline -line {^break (\w+\S*)} $completions] {
+    incr all_funcs($name)
+    if { $all_funcs($name) > 1 } {
+	incr duplicates
+	verbose -log "Duplicate entry for '$name' found"
+    }
+}
+gdb_assert { $duplicates == 0 } "duplicate check"
-- 
2.25.1

Re: [PATCH] gdb: Don't corrupt completions hash when expanding the hash table

[Tom Tromey]

>>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:

Andrew> gdb/ChangeLog:

Andrew> 	* completer.c (class completion_tracker::completion_hash_entry)
Andrew> 	<hash_name>: New member function.
Andrew> 	(completion_tracker::discard_completions): New callback to hash a
Andrew> 	completion_hash_entry, pass this to htab_create_alloc.

Andrew> gdb/testsuite/ChangeLog:

Andrew> 	* gdb.base/many-completions.exp: New file.

Thanks.  This looks good.

Tom

Re: [PATCH] gdb: Don't corrupt completions hash when expanding the hash table

[Andrew Burgess]

* Tom Tromey <tom@tromey.com> [2020-04-06 14:27:14 -0600]:

> >>>>> "Andrew" == Andrew Burgess <andrew.burgess@embecosm.com> writes:
> 
> Andrew> gdb/ChangeLog:
> 
> Andrew> 	* completer.c (class completion_tracker::completion_hash_entry)
> Andrew> 	<hash_name>: New member function.
> Andrew> 	(completion_tracker::discard_completions): New callback to hash a
> Andrew> 	completion_hash_entry, pass this to htab_create_alloc.
> 
> Andrew> gdb/testsuite/ChangeLog:
> 
> Andrew> 	* gdb.base/many-completions.exp: New file.
> 
> Thanks.  This looks good.

Sorry for the delay.  This is now pushed.

Thanks,
Andrew

Re: [PATCHv2 3/3] gdb: Remove C++ symbol aliases from completion list

[Pedro Alves]

Hi,

> gdb/ChangeLog:
> 
> 	* completer.c (completion_tracker::remove_completion): Define new
> 	function.
> 	* completer.h (completion_tracker::remove_completion): Declare new
> 	function.
> 	* symtab.c (completion_list_add_symbol): Remove aliasing msymbols
> 	when adding a C++ function symbol.
> 
> gdb/testsuite/ChangeLog:
> 
> 	* gdb.linespec/cp-completion-aliases.cc: New file.
> 	* gdb.linespec/cp-completion-aliases.exp: New file.

This causes GDB to crash for me, when debugging GDB itself, and then
doing "b main<TAB>".

$ ./gdb -data-directory=data-directory ./gdb -ex "complete b main"
GNU gdb (GDB) 10.0.50.20200319-git
...
Reading symbols from ./gdb...
Setting up the environment for debugging gdb.
During symbol reading: unsupported tag: 'DW_TAG_unspecified_type'
During symbol reading: debug info gives source 55 included from file at zero line 0
During symbol reading: debug info gives command-line macro definition with non-zero line 19: _STDC_PREDEF_H 1
Breakpoint 1 at 0xaf48c3: file /home/pedro/gdb/mygit/src/gdbsupport/errors.cc, line 54.
During symbol reading: Member function "~_Sp_counted_base" (offset 0x683070) is virtual but the vtable offset is not specified
During symbol reading: const value length mismatch for 'std::ratio<1, 1000000000>::num', got 8, expected 0
During symbol reading: cannot get low and high bounds for subprogram DIE at 0x694d73
During symbol reading: Child DIE 0x6afc31 and its abstract origin 0x6afbcd have different parents
During symbol reading: Multiple children of DIE 0x6b2668 refer to DIE 0x6b2657 as their abstract origin
Breakpoint 2 at 0x4f6775: file /home/pedro/gdb/mygit/src/gdb/cli/cli-cmds.c, line 201.
Aborted (core dumped)

GDB seemingly crashes due to infinite recursion.  

Here's the top of the stack, showing the recursion starting:

$ gdb ./gdb -c core.4577
...
Program terminated with signal SIGABRT, Aborted.
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
51      }
[Current thread is 1 (Thread 0x7f9d6f25ecc0 (LWP 4577))]
(gdb) bt -44
#51229 0x000000000055533c in inspect_type (info=0x38ff720, ret_comp=0x3ba6b10, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:267
#51230 0x0000000000555a6f in replace_typedefs (info=0x38ff720, ret_comp=0x3ba6b10, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:475
#51231 0x0000000000555a36 in replace_typedefs (info=0x38ff720, ret_comp=0x3ba6b70, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:470
#51232 0x0000000000555800 in replace_typedefs_qualified_name (info=0x38ff720, ret_comp=0x37c7e20, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:389
#51233 0x0000000000555a8c in replace_typedefs (info=0x38ff720, ret_comp=0x37c7e20, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:479
#51234 0x000000000055533c in inspect_type (info=0x38ff720, ret_comp=0x37c7e20, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:267
#51235 0x0000000000555a6f in replace_typedefs (info=0x38ff720, ret_comp=0x37c7e20, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:475
#51236 0x0000000000555a36 in replace_typedefs (info=0x38ff720, ret_comp=0x37c7e80, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:470
#51237 0x0000000000555696 in replace_typedefs_qualified_name (info=0x38ff720, ret_comp=0x37c7dc0, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:357
#51238 0x0000000000555a8c in replace_typedefs (info=0x38ff720, ret_comp=0x37c7dc0, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:479
#51239 0x0000000000555a36 in replace_typedefs (info=0x38ff720, ret_comp=0x37c8400, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:470
#51240 0x0000000000555bd0 in cp_canonicalize_string_full[abi:cxx11](char const*, char const* (*)(type*, void*), void*) (string=0x980b2c0 "gdb::option::boolean_option_def<filename_partial_match_opts>::boolean_option_def(char const*, bool* (*)(filename_partial_match_opts*), void (*)(ui_file*, int, cmd_list_element*, char const*), char con"..., finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:527
#51241 0x0000000000555d28 in cp_canonicalize_string_no_typedefs[abi:cxx11](char const*) (string=0x980b2c0 "gdb::option::boolean_option_def<filename_partial_match_opts>::boolean_option_def(char const*, bool* (*)(filename_partial_match_opts*), void (*)(ui_file*, int, cmd_list_element*, char const*), char con"...) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:550
#51242 0x00000000008c4959 in completion_list_add_symbol (tracker=..., sym=0x980fde0, lookup_name=..., text=0x7fff6159e7f2 "main", word=0x7fff6159e7f2 "main") at /home/pedro/gdb/mygit/src/gdb/symtab.c:5316
#51243 0x00000000008c541b in add_symtab_completions (cust=0x673ba40, tracker=..., mode=complete_symbol_mode::LINESPEC, lookup_name=..., text=0x7fff6159e7f2 "main", word=0x7fff6159e7f2 "main", code=TYPE_CODE_UNDEF) at /home/pedro/gdb/mygit/src/gdb/symtab.c:5576
#51244 0x00000000008c5496 in <lambda(compunit_symtab*)>::operator()(compunit_symtab *) const (__closure=0x7fff6159e230, symtab=0x673ba40) at /home/pedro/gdb/mygit/src/gdb/symtab.c:5697
#51245 0x00000000008c8f14 in gdb::function_view<void(compunit_symtab*)>::<lambda(gdb::fv_detail::erased_callable, compunit_symtab*)>::operator()(gdb::fv_detail::erased_callable, compunit_symtab *) const (__closure=0x0, ecall=..., args#0=0x673ba40) at /home/pedro/gdb/mygit/src/gdb/../gdbsupport/function-view.h:263
#51246 0x00000000008c8f3c in gdb::function_view<void(compunit_symtab*)>::<lambda(gdb::fv_detail::erased_callable, compunit_symtab*)>::_FUN(gdb::fv_detail::erased_callable, compunit_symtab *) () at /home/pedro/gdb/mygit/src/gdb/../gdbsupport/function-view.h:257
#51247 0x0000000000603da8 in gdb::function_view<void (compunit_symtab*)>::operator()(compunit_symtab*) const (this=0x7fff6159df00, args#0=0x673ba40) at /home/pedro/gdb/mygit/src/gdb/../gdbsupport/function-view.h:247
#51248 0x00000000007c8483 in psym_expand_symtabs_matching(objfile *, gdb::function_view<bool(char const*, bool)>, const lookup_name_info &, gdb::function_view<bool(char const*)>, gdb::function_view<void(compunit_symtab*)>, search_domain) (objfile=0x232c6a0, file_matcher=..., lookup_name_in=..., symbol_matcher=..., expansion_notify=..., domain=ALL_DOMAIN) at /home/pedro/gdb/mygit/src/gdb/psymtab.c:1354
#51249 0x00000000008ab6a7 in expand_symtabs_matching(gdb::function_view<bool (char const*, bool)>, lookup_name_info const&, gdb::function_view<bool (char const*)>, gdb::function_view<void (compunit_symtab*)>, search_domain) (file_matcher=..., lookup_name=..., symbol_matcher=..., expansion_notify=..., kind=ALL_DOMAIN) at /home/pedro/gdb/mygit/src/gdb/symfile.c:3788
#51250 0x00000000008c5ab6 in default_collect_symbol_completion_matches_break_on (tracker=..., mode=complete_symbol_mode::LINESPEC, name_match_type=symbol_name_match_type::WILD, text=0x7fff6159e7f2 "main", word=0x7fff6159e7f2 "main", break_on=0xc3468e "", code=TYPE_CODE_UNDEF) at /home/pedro/gdb/mygit/src/gdb/symtab.c:5692
#51251 0x00000000008c5f79 in default_collect_symbol_completion_matches (tracker=..., mode=complete_symbol_mode::LINESPEC, name_match_type=symbol_name_match_type::WILD, text=0x7fff6159e7f2 "main", word=0x7fff6159e7f2 "main", code=TYPE_CODE_UNDEF) at /home/pedro/gdb/mygit/src/gdb/symtab.c:5795
#51252 0x00000000008c5fc2 in collect_symbol_completion_matches (tracker=..., mode=complete_symbol_mode::LINESPEC, name_match_type=symbol_name_match_type::WILD, text=0x7fff6159e7f2 "main", word=0x7fff6159e7f2 "main") at /home/pedro/gdb/mygit/src/gdb/symtab.c:5810
#51253 0x00000000006fcdf2 in linespec_complete_function (tracker=..., function=0x7fff6159e7f2 "main", func_match_type=symbol_name_match_type::WILD, source_filename=0x0) at /home/pedro/gdb/mygit/src/gdb/linespec.c:2816
#51254 0x00000000006fcecf in complete_linespec_component (parser=0x7fff6159e500, tracker=..., text=0x7fff6159e7f2 "main", component=linespec_complete_what::FUNCTION, source_filename=0x0) at /home/pedro/gdb/mygit/src/gdb/linespec.c:2848
#51255 0x00000000006fd63b in linespec_complete (tracker=..., text=0x7fff6159e7f2 "main", match_type=symbol_name_match_type::WILD) at /home/pedro/gdb/mygit/src/gdb/linespec.c:3063
#51256 0x00000000005405b1 in complete_address_and_linespec_locations (tracker=..., text=0x7fff6159e7f2 "main", match_type=symbol_name_match_type::WILD) at /home/pedro/gdb/mygit/src/gdb/completer.c:690
#51257 0x0000000000540eee in location_completer (ignore=0x1cb8320, tracker=..., text=0x7fff6159e7f2 "main") at /home/pedro/gdb/mygit/src/gdb/completer.c:1034
#51258 0x0000000000541024 in location_completer_handle_brkchars (ignore=0x1cb8320, tracker=..., text=0x7fff6159e7f2 "main", word_ignored=0x0) at /home/pedro/gdb/mygit/src/gdb/completer.c:1071
#51259 0x00000000005416e4 in complete_line_internal_normal_command (tracker=..., command=0x7fff6159e7f0 "b main", word=0x0, cmd_args=0x7fff6159e7f2 "main", reason=handle_brkchars, c=0x1cb8320) at /home/pedro/gdb/mygit/src/gdb/completer.c:1306
#51260 0x0000000000541b72 in complete_line_internal_1 (tracker=..., text=0x0, line_buffer=0x7fff615a0905 "b main", point=6, reason=handle_brkchars) at /home/pedro/gdb/mygit/src/gdb/completer.c:1531
#51261 0x0000000000541bb5 in complete_line_internal (tracker=..., text=0x0, line_buffer=0x7fff615a0905 "b main", point=6, reason=handle_brkchars) at /home/pedro/gdb/mygit/src/gdb/completer.c:1550
#51262 0x0000000000542cee in completion_find_completion_word (tracker=..., text=0x7fff615a0905 "b main", quote_char=0x7fff6159eb10) at /home/pedro/gdb/mygit/src/gdb/completer.c:2054
#51263 0x000000000054247a in complete (line=0x7fff615a0905 "b main", word=0x7fff6159eb08, quote_char=0x7fff6159eb10) at /home/pedro/gdb/mygit/src/gdb/completer.c:1770
#51264 0x00000000004f6ed0 in complete_command (arg=0x7fff615a0905 "b main", from_tty=1) at /home/pedro/gdb/mygit/src/gdb/cli/cli-cmds.c:364
#51265 0x00000000004fee70 in do_const_cfunc (c=0x2385760, args=0x7fff615a0905 "b main", from_tty=1) at /home/pedro/gdb/mygit/src/gdb/cli/cli-decode.c:107
#51266 0x0000000000501e28 in cmd_func (cmd=0x2385760, args=0x7fff615a0905 "b main", from_tty=1) at /home/pedro/gdb/mygit/src/gdb/cli/cli-decode.c:1952
#51267 0x000000000090d07d in execute_command (p=0x7fff615a0905 "b main", from_tty=1) at /home/pedro/gdb/mygit/src/gdb/top.c:655
#51268 0x000000000073b9c2 in catch_command_errors (command=0x90cc11 <execute_command(char const*, int)>, arg=0x7fff615a08fc "complete b main", from_tty=1) at /home/pedro/gdb/mygit/src/gdb/main.c:401
#51269 0x000000000073cdaa in captured_main_1 (context=0x7fff6159f070) at /home/pedro/gdb/mygit/src/gdb/main.c:1163
#51270 0x000000000073cf9f in captured_main (data=0x7fff6159f070) at /home/pedro/gdb/mygit/src/gdb/main.c:1188
#51271 0x000000000073d00a in gdb_main (args=0x7fff6159f070) at /home/pedro/gdb/mygit/src/gdb/main.c:1213
#51272 0x000000000041563e in main (argc=6, argv=0x7fff6159f178) at /home/pedro/gdb/mygit/src/gdb/gdb.c:32

This cycle keeps repeating:
#1709 0x000000000055533c in inspect_type (info=0x38ff720, ret_comp=0xd83be10, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:267
#1710 0x0000000000555a6f in replace_typedefs (info=0x38ff720, ret_comp=0xd83be10, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:475
#1711 0x0000000000555a36 in replace_typedefs (info=0x38ff720, ret_comp=0xd83be70, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:470
#1712 0x0000000000555800 in replace_typedefs_qualified_name (info=0x38ff720, ret_comp=0xd839470, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:389
#1713 0x0000000000555a8c in replace_typedefs (info=0x38ff720, ret_comp=0xd839470, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:479


The symbol on which we call cp_canonicalize_string_no_typedefs is:

(top-gdb) p *sym
$4 = {<general_symbol_info> = {m_name = 0x980b2c0 "gdb::option::boolean_option_def<filename_partial_match_opts>::boolean_option_def(char const*, bool* (*)(filename_partial_
match_opts*), void (*)(ui_file*, int, cmd_list_element*, char const*), char con"..., value = {ivalue = 159449440, block = 0x9810160, bytes = 0x9810160 "\212\271\214", addre
ss = 0x9810160, common_block = 0x9810160, chain = 0x9810160}, language_specific = {obstack = 0x0, demangled_name = 0x0}, m_language = language_cplus, ada_mangled = 0, secti
on = -1}, <allocate_on_obstack> = {<No data fields>}, type = 0x980fc20, owner = {symtab = 0x673bdf0, arch = 0x673bdf0}, domain = VAR_DOMAIN, aclass_index = 19, is_objfile_o
wned = 1, is_argument = 0, is_inlined = 0, maybe_copied = 0, subclass = SYMBOL_NONE, line = 157, aux_value = 0x980fe30, hash_next = 0x9821bd0}

(top-gdb) p sym->m_name 
$6 = 0x980b2c0 "gdb::option::boolean_option_def<filename_partial_match_opts>::boolean_option_def(char const*, bool* (*)(filename_partial_match_opts*), void (*)(ui_file*, in t, cmd_list_element*, char const*), char const*, char const*, char const*)"


I'm seeing this on a GDB built with:
 gcc version 7.3.1 20180712 (Red Hat 7.3.1-6) (GCC) 
on Fedora 27.

Anyone else seeing this?

Thanks,
Pedro Alves

[pushed] Don't remove C++ aliases from completions if symbol doesn't match (Re: [PATCHv2 3/3] gdb: Remove C++ symbol aliases from completion list)

[Pedro Alves]

On 5/24/20 12:35 PM, Pedro Alves via Gdb-patches wrote:

> This causes GDB to crash for me, when debugging GDB itself, and then
> doing "b main<TAB>".

...

> GDB seemingly crashes due to infinite recursion.  

> 
> The symbol on which we call cp_canonicalize_string_no_typedefs is:
> 

> (top-gdb) p sym->m_name 
> $6 = 0x980b2c0 "gdb::option::boolean_option_def<filename_partial_match_opts>::boolean_option_def(char const*, bool* (*)(filename_partial_match_opts*), void (*)(ui_file*, in t, cmd_list_element*, char const*), char const*, char const*, char const*)"

Note that this symbol doesn't match "main", so trying to remove
its aliases is pointless work.  I can't do performance testing,
because GDB just crashes for me, but I'd think this caused a
performance regression.

I'm applying this patch, which makes GDB not crash when completing "main",
though of course it still crashes with "complete b gdb::option::", 
so we need to fix that.

The new testcase gdb.linespec/cp-completion-aliases.exp passes with this
patch, though I had to force-disable the completion styling, which itself
broke the testcase...

From e08bd6c5081f4957ddb60117ac94775dcd618db7 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Sun, 24 May 2020 13:32:25 +0100
Subject: [PATCH] Don't remove C++ aliases from completions if symbol doesn't
 match

completion_list_add_symbol currently tries to remove C++ function
aliases from the completions match list even if the symbol passed down
wasn't successfully added to the completion list because it didn't
match.  I.e., we call cp_canonicalize_string_no_typedefs for each and
every C++ function in the program, which is useful work.  This patch
skips that useless work.

gdb/ChangeLog:
2020-05-24  Pedro Alves  <palves@redhat.com>

	* symtab.c (completion_list_add_name): Return boolean indication
	of whether the symbol matched.
	(completion_list_add_symbol): Don't try to remove C++ aliases if
	the symbol didn't match in the first place.
	* symtab.h (completion_list_add_name): Return bool.
---
 gdb/ChangeLog |  8 ++++++++
 gdb/symtab.c  | 13 ++++++++-----
 gdb/symtab.h  |  5 +++--
 3 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 35fdc614ee4..9f65e2df970 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,11 @@
+2020-05-24  Pedro Alves  <palves@redhat.com>
+
+	* symtab.c (completion_list_add_name): Return boolean indication
+	of whether the symbol matched.
+	(completion_list_add_symbol): Don't try to remove C++ aliases if
+	the symbol didn't match in the first place.
+	* symtab.h (completion_list_add_name): Return bool.
+
 2020-05-23  Simon Marchi  <simon.marchi@polymtl.ca>
 
 	* gdbtypes.h (TYPE_FIELD): Remove.  Replace all uses with
diff --git a/gdb/symtab.c b/gdb/symtab.c
index 3f90ea99647..5c4e282c024 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -5260,7 +5260,7 @@ compare_symbol_name (const char *symbol_name, language symbol_language,
 
 /*  See symtab.h.  */
 
-void
+bool
 completion_list_add_name (completion_tracker &tracker,
 			  language symbol_language,
 			  const char *symname,
@@ -5272,7 +5272,7 @@ completion_list_add_name (completion_tracker &tracker,
 
   /* Clip symbols that cannot match.  */
   if (!compare_symbol_name (symname, symbol_language, lookup_name, match_res))
-    return;
+    return false;
 
   /* Refresh SYMNAME from the match string.  It's potentially
      different depending on language.  (E.g., on Ada, the match may be
@@ -5296,6 +5296,8 @@ completion_list_add_name (completion_tracker &tracker,
     tracker.add_completion (std::move (completion),
 			    &match_res.match_for_lcd, text, word);
   }
+
+  return true;
 }
 
 /* completion_list_add_name wrapper for struct symbol.  */
@@ -5306,9 +5308,10 @@ completion_list_add_symbol (completion_tracker &tracker,
 			    const lookup_name_info &lookup_name,
 			    const char *text, const char *word)
 {
-  completion_list_add_name (tracker, sym->language (),
-			    sym->natural_name (),
-			    lookup_name, text, word);
+  if (!completion_list_add_name (tracker, sym->language (),
+				 sym->natural_name (),
+				 lookup_name, text, word))
+    return;
 
   /* C++ function symbols include the parameters within both the msymbol
      name and the symbol name.  The problem is that the msymbol name will
diff --git a/gdb/symtab.h b/gdb/symtab.h
index 05e6a311b87..9972e8125ba 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -2332,8 +2332,9 @@ const char *
 /* Test to see if the symbol of language SYMBOL_LANGUAGE specified by
    SYMNAME (which is already demangled for C++ symbols) matches
    SYM_TEXT in the first SYM_TEXT_LEN characters.  If so, add it to
-   the current completion list.  */
-void completion_list_add_name (completion_tracker &tracker,
+   the current completion list and return true.  Otherwise, return
+   false.  */
+bool completion_list_add_name (completion_tracker &tracker,
 			       language symbol_language,
 			       const char *symname,
 			       const lookup_name_info &lookup_name,

base-commit: bb68f22c8e648032a0d1c1d17353eec599ff5e6a
-- 
2.14.5

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

[Pedro Alves]

Adding Keith, who I believe is the original author of this
whole replace-typedefs machinery.  [ Help? :-) ]

On 5/24/20 12:35 PM, Pedro Alves via Gdb-patches wrote:
> Hi,
> 
>> gdb/ChangeLog:
>>
>> 	* completer.c (completion_tracker::remove_completion): Define new
>> 	function.
>> 	* completer.h (completion_tracker::remove_completion): Declare new
>> 	function.
>> 	* symtab.c (completion_list_add_symbol): Remove aliasing msymbols
>> 	when adding a C++ function symbol.
>>
>> gdb/testsuite/ChangeLog:
>>
>> 	* gdb.linespec/cp-completion-aliases.cc: New file.
>> 	* gdb.linespec/cp-completion-aliases.exp: New file.
> 
> This causes GDB to crash for me, when debugging GDB itself, and then
> doing "b main<TAB>".
> 
> $ ./gdb -data-directory=data-directory ./gdb -ex "complete b main"
> GNU gdb (GDB) 10.0.50.20200319-git
> ...
> Reading symbols from ./gdb...
> Setting up the environment for debugging gdb.
> During symbol reading: unsupported tag: 'DW_TAG_unspecified_type'
> During symbol reading: debug info gives source 55 included from file at zero line 0
> During symbol reading: debug info gives command-line macro definition with non-zero line 19: _STDC_PREDEF_H 1
> Breakpoint 1 at 0xaf48c3: file /home/pedro/gdb/mygit/src/gdbsupport/errors.cc, line 54.
> During symbol reading: Member function "~_Sp_counted_base" (offset 0x683070) is virtual but the vtable offset is not specified
> During symbol reading: const value length mismatch for 'std::ratio<1, 1000000000>::num', got 8, expected 0
> During symbol reading: cannot get low and high bounds for subprogram DIE at 0x694d73
> During symbol reading: Child DIE 0x6afc31 and its abstract origin 0x6afbcd have different parents
> During symbol reading: Multiple children of DIE 0x6b2668 refer to DIE 0x6b2657 as their abstract origin
> Breakpoint 2 at 0x4f6775: file /home/pedro/gdb/mygit/src/gdb/cli/cli-cmds.c, line 201.
> Aborted (core dumped)
> 
> GDB seemingly crashes due to infinite recursion.  
> 
> Here's the top of the stack, showing the recursion starting:
> 
> $ gdb ./gdb -c core.4577
> ...
> Program terminated with signal SIGABRT, Aborted.
> #0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
> 51      }
> [Current thread is 1 (Thread 0x7f9d6f25ecc0 (LWP 4577))]
> (gdb) bt -44
> #51229 0x000000000055533c in inspect_type (info=0x38ff720, ret_comp=0x3ba6b10, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:267
> #51230 0x0000000000555a6f in replace_typedefs (info=0x38ff720, ret_comp=0x3ba6b10, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:475
> #51231 0x0000000000555a36 in replace_typedefs (info=0x38ff720, ret_comp=0x3ba6b70, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:470
> #51232 0x0000000000555800 in replace_typedefs_qualified_name (info=0x38ff720, ret_comp=0x37c7e20, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:389
> #51233 0x0000000000555a8c in replace_typedefs (info=0x38ff720, ret_comp=0x37c7e20, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:479
> #51234 0x000000000055533c in inspect_type (info=0x38ff720, ret_comp=0x37c7e20, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:267
> #51235 0x0000000000555a6f in replace_typedefs (info=0x38ff720, ret_comp=0x37c7e20, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:475
> #51236 0x0000000000555a36 in replace_typedefs (info=0x38ff720, ret_comp=0x37c7e80, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:470
> #51237 0x0000000000555696 in replace_typedefs_qualified_name (info=0x38ff720, ret_comp=0x37c7dc0, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:357
> #51238 0x0000000000555a8c in replace_typedefs (info=0x38ff720, ret_comp=0x37c7dc0, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:479
> #51239 0x0000000000555a36 in replace_typedefs (info=0x38ff720, ret_comp=0x37c8400, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:470
> #51240 0x0000000000555bd0 in cp_canonicalize_string_full[abi:cxx11](char const*, char const* (*)(type*, void*), void*) (string=0x980b2c0 "gdb::option::boolean_option_def<filename_partial_match_opts>::boolean_option_def(char const*, bool* (*)(filename_partial_match_opts*), void (*)(ui_file*, int, cmd_list_element*, char const*), char con"..., finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:527
> #51241 0x0000000000555d28 in cp_canonicalize_string_no_typedefs[abi:cxx11](char const*) (string=0x980b2c0 "gdb::option::boolean_option_def<filename_partial_match_opts>::boolean_option_def(char const*, bool* (*)(filename_partial_match_opts*), void (*)(ui_file*, int, cmd_list_element*, char const*), char con"...) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:550
> #51242 0x00000000008c4959 in completion_list_add_symbol (tracker=..., sym=0x980fde0, lookup_name=..., text=0x7fff6159e7f2 "main", word=0x7fff6159e7f2 "main") at /home/pedro/gdb/mygit/src/gdb/symtab.c:5316
> #51243 0x00000000008c541b in add_symtab_completions (cust=0x673ba40, tracker=..., mode=complete_symbol_mode::LINESPEC, lookup_name=..., text=0x7fff6159e7f2 "main", word=0x7fff6159e7f2 "main", code=TYPE_CODE_UNDEF) at /home/pedro/gdb/mygit/src/gdb/symtab.c:5576
> #51244 0x00000000008c5496 in <lambda(compunit_symtab*)>::operator()(compunit_symtab *) const (__closure=0x7fff6159e230, symtab=0x673ba40) at /home/pedro/gdb/mygit/src/gdb/symtab.c:5697
> #51245 0x00000000008c8f14 in gdb::function_view<void(compunit_symtab*)>::<lambda(gdb::fv_detail::erased_callable, compunit_symtab*)>::operator()(gdb::fv_detail::erased_callable, compunit_symtab *) const (__closure=0x0, ecall=..., args#0=0x673ba40) at /home/pedro/gdb/mygit/src/gdb/../gdbsupport/function-view.h:263
> #51246 0x00000000008c8f3c in gdb::function_view<void(compunit_symtab*)>::<lambda(gdb::fv_detail::erased_callable, compunit_symtab*)>::_FUN(gdb::fv_detail::erased_callable, compunit_symtab *) () at /home/pedro/gdb/mygit/src/gdb/../gdbsupport/function-view.h:257
> #51247 0x0000000000603da8 in gdb::function_view<void (compunit_symtab*)>::operator()(compunit_symtab*) const (this=0x7fff6159df00, args#0=0x673ba40) at /home/pedro/gdb/mygit/src/gdb/../gdbsupport/function-view.h:247
> #51248 0x00000000007c8483 in psym_expand_symtabs_matching(objfile *, gdb::function_view<bool(char const*, bool)>, const lookup_name_info &, gdb::function_view<bool(char const*)>, gdb::function_view<void(compunit_symtab*)>, search_domain) (objfile=0x232c6a0, file_matcher=..., lookup_name_in=..., symbol_matcher=..., expansion_notify=..., domain=ALL_DOMAIN) at /home/pedro/gdb/mygit/src/gdb/psymtab.c:1354
> #51249 0x00000000008ab6a7 in expand_symtabs_matching(gdb::function_view<bool (char const*, bool)>, lookup_name_info const&, gdb::function_view<bool (char const*)>, gdb::function_view<void (compunit_symtab*)>, search_domain) (file_matcher=..., lookup_name=..., symbol_matcher=..., expansion_notify=..., kind=ALL_DOMAIN) at /home/pedro/gdb/mygit/src/gdb/symfile.c:3788
> #51250 0x00000000008c5ab6 in default_collect_symbol_completion_matches_break_on (tracker=..., mode=complete_symbol_mode::LINESPEC, name_match_type=symbol_name_match_type::WILD, text=0x7fff6159e7f2 "main", word=0x7fff6159e7f2 "main", break_on=0xc3468e "", code=TYPE_CODE_UNDEF) at /home/pedro/gdb/mygit/src/gdb/symtab.c:5692
> #51251 0x00000000008c5f79 in default_collect_symbol_completion_matches (tracker=..., mode=complete_symbol_mode::LINESPEC, name_match_type=symbol_name_match_type::WILD, text=0x7fff6159e7f2 "main", word=0x7fff6159e7f2 "main", code=TYPE_CODE_UNDEF) at /home/pedro/gdb/mygit/src/gdb/symtab.c:5795
> #51252 0x00000000008c5fc2 in collect_symbol_completion_matches (tracker=..., mode=complete_symbol_mode::LINESPEC, name_match_type=symbol_name_match_type::WILD, text=0x7fff6159e7f2 "main", word=0x7fff6159e7f2 "main") at /home/pedro/gdb/mygit/src/gdb/symtab.c:5810
> #51253 0x00000000006fcdf2 in linespec_complete_function (tracker=..., function=0x7fff6159e7f2 "main", func_match_type=symbol_name_match_type::WILD, source_filename=0x0) at /home/pedro/gdb/mygit/src/gdb/linespec.c:2816
> #51254 0x00000000006fcecf in complete_linespec_component (parser=0x7fff6159e500, tracker=..., text=0x7fff6159e7f2 "main", component=linespec_complete_what::FUNCTION, source_filename=0x0) at /home/pedro/gdb/mygit/src/gdb/linespec.c:2848
> #51255 0x00000000006fd63b in linespec_complete (tracker=..., text=0x7fff6159e7f2 "main", match_type=symbol_name_match_type::WILD) at /home/pedro/gdb/mygit/src/gdb/linespec.c:3063
> #51256 0x00000000005405b1 in complete_address_and_linespec_locations (tracker=..., text=0x7fff6159e7f2 "main", match_type=symbol_name_match_type::WILD) at /home/pedro/gdb/mygit/src/gdb/completer.c:690
> #51257 0x0000000000540eee in location_completer (ignore=0x1cb8320, tracker=..., text=0x7fff6159e7f2 "main") at /home/pedro/gdb/mygit/src/gdb/completer.c:1034
> #51258 0x0000000000541024 in location_completer_handle_brkchars (ignore=0x1cb8320, tracker=..., text=0x7fff6159e7f2 "main", word_ignored=0x0) at /home/pedro/gdb/mygit/src/gdb/completer.c:1071
> #51259 0x00000000005416e4 in complete_line_internal_normal_command (tracker=..., command=0x7fff6159e7f0 "b main", word=0x0, cmd_args=0x7fff6159e7f2 "main", reason=handle_brkchars, c=0x1cb8320) at /home/pedro/gdb/mygit/src/gdb/completer.c:1306
> #51260 0x0000000000541b72 in complete_line_internal_1 (tracker=..., text=0x0, line_buffer=0x7fff615a0905 "b main", point=6, reason=handle_brkchars) at /home/pedro/gdb/mygit/src/gdb/completer.c:1531
> #51261 0x0000000000541bb5 in complete_line_internal (tracker=..., text=0x0, line_buffer=0x7fff615a0905 "b main", point=6, reason=handle_brkchars) at /home/pedro/gdb/mygit/src/gdb/completer.c:1550
> #51262 0x0000000000542cee in completion_find_completion_word (tracker=..., text=0x7fff615a0905 "b main", quote_char=0x7fff6159eb10) at /home/pedro/gdb/mygit/src/gdb/completer.c:2054
> #51263 0x000000000054247a in complete (line=0x7fff615a0905 "b main", word=0x7fff6159eb08, quote_char=0x7fff6159eb10) at /home/pedro/gdb/mygit/src/gdb/completer.c:1770
> #51264 0x00000000004f6ed0 in complete_command (arg=0x7fff615a0905 "b main", from_tty=1) at /home/pedro/gdb/mygit/src/gdb/cli/cli-cmds.c:364
> #51265 0x00000000004fee70 in do_const_cfunc (c=0x2385760, args=0x7fff615a0905 "b main", from_tty=1) at /home/pedro/gdb/mygit/src/gdb/cli/cli-decode.c:107
> #51266 0x0000000000501e28 in cmd_func (cmd=0x2385760, args=0x7fff615a0905 "b main", from_tty=1) at /home/pedro/gdb/mygit/src/gdb/cli/cli-decode.c:1952
> #51267 0x000000000090d07d in execute_command (p=0x7fff615a0905 "b main", from_tty=1) at /home/pedro/gdb/mygit/src/gdb/top.c:655
> #51268 0x000000000073b9c2 in catch_command_errors (command=0x90cc11 <execute_command(char const*, int)>, arg=0x7fff615a08fc "complete b main", from_tty=1) at /home/pedro/gdb/mygit/src/gdb/main.c:401
> #51269 0x000000000073cdaa in captured_main_1 (context=0x7fff6159f070) at /home/pedro/gdb/mygit/src/gdb/main.c:1163
> #51270 0x000000000073cf9f in captured_main (data=0x7fff6159f070) at /home/pedro/gdb/mygit/src/gdb/main.c:1188
> #51271 0x000000000073d00a in gdb_main (args=0x7fff6159f070) at /home/pedro/gdb/mygit/src/gdb/main.c:1213
> #51272 0x000000000041563e in main (argc=6, argv=0x7fff6159f178) at /home/pedro/gdb/mygit/src/gdb/gdb.c:32
> 
> This cycle keeps repeating:
> #1709 0x000000000055533c in inspect_type (info=0x38ff720, ret_comp=0xd83be10, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:267
> #1710 0x0000000000555a6f in replace_typedefs (info=0x38ff720, ret_comp=0xd83be10, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:475
> #1711 0x0000000000555a36 in replace_typedefs (info=0x38ff720, ret_comp=0xd83be70, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:470
> #1712 0x0000000000555800 in replace_typedefs_qualified_name (info=0x38ff720, ret_comp=0xd839470, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:389
> #1713 0x0000000000555a8c in replace_typedefs (info=0x38ff720, ret_comp=0xd839470, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:479
> 
> 
> The symbol on which we call cp_canonicalize_string_no_typedefs is:
> 
> (top-gdb) p *sym
> $4 = {<general_symbol_info> = {m_name = 0x980b2c0 "gdb::option::boolean_option_def<filename_partial_match_opts>::boolean_option_def(char const*, bool* (*)(filename_partial_
> match_opts*), void (*)(ui_file*, int, cmd_list_element*, char const*), char con"..., value = {ivalue = 159449440, block = 0x9810160, bytes = 0x9810160 "\212\271\214", addre
> ss = 0x9810160, common_block = 0x9810160, chain = 0x9810160}, language_specific = {obstack = 0x0, demangled_name = 0x0}, m_language = language_cplus, ada_mangled = 0, secti
> on = -1}, <allocate_on_obstack> = {<No data fields>}, type = 0x980fc20, owner = {symtab = 0x673bdf0, arch = 0x673bdf0}, domain = VAR_DOMAIN, aclass_index = 19, is_objfile_o
> wned = 1, is_argument = 0, is_inlined = 0, maybe_copied = 0, subclass = SYMBOL_NONE, line = 157, aux_value = 0x980fe30, hash_next = 0x9821bd0}
> 
> (top-gdb) p sym->m_name 
> $6 = 0x980b2c0 "gdb::option::boolean_option_def<filename_partial_match_opts>::boolean_option_def(char const*, bool* (*)(filename_partial_match_opts*), void (*)(ui_file*, in t, cmd_list_element*, char const*), char const*, char const*, char const*)"
> 

I figured out what is special about this symbol.  The issue is that
we have a boolean_option_def typedef in the global namespace in the
program.  Here, in stack.c:

 using boolean_option_def
   = gdb::option::boolean_option_def<frame_print_options>;

actually we have two.  There's another one in valprint.c:

 using boolean_option_def
   = gdb::option::boolean_option_def<value_print_options>;

The recursion happens because we start by breaking the

   gdb::option::boolean_option_def<filename_partial_match_opts>::boolean_option_def(....)

symbol name into demanged parse info components.
This happens in cp_canonicalize_string_no_typedefs -> replace_typedefs.

The tree for that symbol looks like this:

(top-gdb) p d_dump (ret_comp, 0)
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'
  function type
    argument list
      pointer
        const
          builtin type char
      argument list
        pointer
          function type
            pointer
              builtin type bool
            argument list
              pointer
                name 'value_print_options'
        argument list
          pointer
            function type
              builtin type void
              argument list
                pointer
                  name 'ui_file'
                argument list
                  builtin type int
                  argument list
                    pointer
                      name 'cmd_list_element'
                    argument list
                      pointer
                        const
                          builtin type char
          argument list
            pointer
              const
                builtin type char
            argument list
              pointer
                const
                  builtin type char
              argument list
                pointer
                  const
                    builtin type char
$5 = void

Let's just focus on the top part, because that's where
the recursion happens:

 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'

As we walk the tree of components, we get to replace_typedefs_qualified_name,
and enter the loop:

  while (comp->type == DEMANGLE_COMPONENT_QUAL_NAME)
    {
      if (d_left (comp)->type == DEMANGLE_COMPONENT_NAME)
	{

The DEMANGLE_COMPONENT_NAME case is hit twice, once for
"gdb", and another for "option".  After those two,
the next d_left (comp)->type is DEMANGLE_COMPONENT_TEMPLATE.

This now gets us to the else branch within that
DEMANGLE_COMPONENT_QUAL_NAME loop:

      else
	{
	  /* The current node is not a name, so simply replace any
	     typedefs in it.  Then print it to the stream to continue
	     checking for more typedefs in the tree.  */
	  replace_typedefs (info, d_left (comp), finder, data);

which, after some more replace_typedefs recursion, gets us to

	case DEMANGLE_COMPONENT_NAME:
	  inspect_type (info, ret_comp, finder, data);
	  break;

This is for this part of the tree:

         template
           name 'boolean_option_def'

and it's this inspect_type call that's problematic.

[come back here later]

inspect_type does a symbol lookup for "boolean_option_def",
which finds the unrelated typedef at the global namespace.
In this session I'm debugging, it finds the one in valprint.c:

(top-gdb) p *sym
$10 = {<general_symbol_info> = {m_name = 0x3a5dc90 "boolean_option_def", value = {ivalue = 0, block = 0x0, bytes = 0x0, address = 0x0, common_block = 0x0, chain = 0x0}, language_specific = {obstack = 0x0, demangled_name = 0x0}, m_language = language_cplus, ada_mangled = 0, section = -1}, <allocate_on_obstack> = {<No data fields>}, type = 0x3a613f0, owner = {symtab = 0x34ab930, arch = 0x34ab930}, domain = VAR_DOMAIN, aclass_index = 8, is_objfile_owned = 1, is_argument = 0, is_inlined = 0, maybe_copied = 0, subclass = SYMBOL_NONE, line = 2987, aux_value = 0x0, hash_next = 0x3a694e0}
(top-gdb) p *sym.owner.symtab 
$11 = {next = 0x34ab750, compunit_symtab = 0x34ab6d0, linetable = 0x3b83e60, filename = 0x2df4b20 "/home/pedro/gdb/mygit/src/gdb/valprint.c", language = language_cplus, fullname = 0x0}

This symbol's type is of course a typedef, so we try to resolve it.
check_typedef returns this type:

 (top-gdb) p *type->main_type
 $14 = 
 {name = 0x2de5a40 "gdb::option::boolean_option_def<value_print_options>",
  code = TYPE_CODE_STRUCT,
 ...

So, since a typedef was resolved, we get here:

	  /* Turn the result into a new tree.  Note that this
	     tree will contain pointers into NAME, so NAME cannot
	     be free'd until all typedef conversion is done and
	     the final result is converted into a string.  */
	  i = cp_demangled_name_to_comp (name, NULL);
	  if (i != NULL)
	    {
	      /* Merge the two trees.  */
	      cp_merge_demangle_parse_infos (info, ret_comp, i.get ());

	      /* Replace any newly introduced typedefs -- but not
		 if the type is anonymous (that would lead to infinite
		 looping).  */
	      if (!is_anon)
		{
		  fprintf_unfiltered (gdb_stdlog, "replace_typedefs: %s\n",
				      name);

		  replace_typedefs (info, ret_comp, finder, data);

And it's that last replace_typedefs call that triggers the recursion,
since we're now going to process a tree

 gdb::option::boolean_option_def<value_print_options>
              ^^^^^^^^^^^^^^^^^^

that will again end up doing a lookup for that global scope
boolean_option_def typedef, rinse repeat.

Locally, I put a recursion limit in inspect_type, and the resulting 
symbol name that cp_canonicalize_string_no_typedefs returns is:

$18 = std::unique_ptr<char> containing 0x32f81a0 "gdb::option::gdb::option::gdb::option::gdb::option::gdb::option::gdb::option::gdb::option::gdb::option::gdb::option::gdb::
option::gdb::option::gdb::option::gdb::option::gdb::option::gdb::option::gdb::option::gdb::option::gdb::option::gdb::option::gdb::option::gdb::option::boolean_option_def<va
lue_print_options><value_print_options><value_print_options><value_print_options><value_print_options><value_print_options><value_print_options><value_print_options><value_
print_options><value_print_options><value_print_options><value_print_options><value_print_options><value_print_options><value_print_options><value_print_options><value_prin
t_options><value_print_options><value_print_options><value_print_options><value_print_options>::boolean_option_def(char const*, bool* (*)(value_print_options*), void (*)(ui
_file*, int, cmd_list_element*, char const*), char const*, char const*, char const*)"

Obviously broken due to recursion around boolean_option_def.


Now, let's get back to that inspect_type call that I mentioned
was problematic.  See [come back here later] above.

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

so we should be looking for a "boolean_option_def" typedef in
the gdb::option namespace or struct/class.  Right?

The question to me is then, how to do that.
In replace_typedefs_qualified_name, after we've processed
"gdb" and "option", we have "gdb::option::" stored in
the "buf" string_file, which seems perfect.  However, how do we
get that scope down to inspect_type, since we have layers of
replace_typedefs calls before we get there?

I can think of two ways:

#1 - store the current scope in the demangle_parse_info
context object, and have inspect_type consult that.

#2 - Make replace_typedefs_qualified_name
look ahead in the tree, inlining the
DEMANGLE_COMPONENT_TEMPLATE case here, basically
inlining enough forward peeking in order to do a directly
inspect_type call from within replace_typedefs_qualified_name.
Of course, we would likely have to do the same for other
node types, not just DEMANGLE_COMPONENT_TEMPLATE.

And then, I'm not sure we should pass the scope as extra
parameter to inspect_type, or, whether we should replace
the "boolean_option_def" node in the tree with one that has
a full-scoped name "gdb::option::boolean_option_def" name,
similarly to how replace_typedefs_qualified_name
handles the DEMANGLE_COMPONENT_NAME case.

One difficulty I have with this, is that I couldn't find
any case of this code in replace_typedefs_qualified_name:

	  /* The current node is not a name, so simply replace any
	     typedefs in it.  Then print it to the stream to continue
	     checking for more typedefs in the tree.  */
	  replace_typedefs (info, d_left (comp), finder, data);

actually ever replacing any typedef in the testsuite.
I mean, I put an abort() in place if the returned component
as a string is different from the name before the 
replace_typedefs call, and then ran the gdb.cp/ testcases,
and the abort never triggered...

So, in order to get the discussion going, I wrote a patch
for solution #1.  This fixes the issue for me, and causes no
testsuite regressions.  Find it below.

WDYT?

> 
> I'm seeing this on a GDB built with:
>  gcc version 7.3.1 20180712 (Red Hat 7.3.1-6) (GCC) 
> on Fedora 27.
> 
> Anyone else seeing this?

I'm also seeing this when I build GDB with GCC 10 (and debug
that GDB with itself), so it must be others see this too?

From af60bb6c6d7c66307db49e524409bc81f1322907 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Mon, 25 May 2020 13:14:10 +0100
Subject: [PATCH] Fix gdb::option::boolean_option_def recursion

---
 gdb/cp-support.c | 15 +++++++++++----
 gdb/cp-support.h |  5 ++++-
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/gdb/cp-support.c b/gdb/cp-support.c
index 1e54aaea3b1..2e4db8a7007 100644
--- a/gdb/cp-support.c
+++ b/gdb/cp-support.c
@@ -137,14 +137,15 @@ inspect_type (struct demangle_parse_info *info,
 	      canonicalization_ftype *finder,
 	      void *data)
 {
-  char *name;
   struct symbol *sym;
 
   /* Copy the symbol's name from RET_COMP and look it up
      in the symbol table.  */
-  name = (char *) alloca (ret_comp->u.s_name.len + 1);
-  memcpy (name, ret_comp->u.s_name.s, ret_comp->u.s_name.len);
-  name[ret_comp->u.s_name.len] = '\0';
+  std::string name_str;
+  if (info->current_scope != nullptr)
+    name_str = info->current_scope;
+  name_str.append (ret_comp->u.s_name.s, ret_comp->u.s_name.len);
+  const char *name = name_str.c_str ();
 
   /* Ignore any typedefs that should not be substituted.  */
   for (int i = 0; i < ARRAY_SIZE (ignore_typedefs); ++i)
@@ -355,7 +356,13 @@ replace_typedefs_qualified_name (struct demangle_parse_info *info,
 	  /* The current node is not a name, so simply replace any
 	     typedefs in it.  Then print it to the stream to continue
 	     checking for more typedefs in the tree.  */
+
+	  /* The struct/template/function/field/type name should be
+	     qualified in the current scope though.  */
+	  info->current_scope = buf.string ().c_str ();
 	  replace_typedefs (info, d_left (comp), finder, data);
+	  info->current_scope = nullptr;
+
 	  gdb::unique_xmalloc_ptr<char> name
 	    = cp_comp_to_string (d_left (comp), 100);
 	  if (name == NULL)
diff --git a/gdb/cp-support.h b/gdb/cp-support.h
index 6ca898315bb..561de5a94df 100644
--- a/gdb/cp-support.h
+++ b/gdb/cp-support.h
@@ -72,8 +72,11 @@ struct demangle_parse_info
 
   /* Any temporary memory used during typedef replacement.  */
   struct obstack obstack;
-};
 
+  /* The current qualified scope context, during typedef
+     replacement.  */
+  const char *current_scope = nullptr;
+};
 
 /* Functions from cp-support.c.  */
 

base-commit: af2c48d8545cbc24aa6caf9b9f2a49ab6c0aaa7b
-- 
2.14.5

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

[Andrew Burgess]

* Pedro Alves <palves@redhat.com> [2020-05-25 15:34:27 +0100]:

> Adding Keith, who I believe is the original author of this
> whole replace-typedefs machinery.  [ Help? :-) ]
> 
> On 5/24/20 12:35 PM, Pedro Alves via Gdb-patches wrote:
> > Hi,
> > 
> >> gdb/ChangeLog:
> >>
> >> 	* completer.c (completion_tracker::remove_completion): Define new
> >> 	function.
> >> 	* completer.h (completion_tracker::remove_completion): Declare new
> >> 	function.
> >> 	* symtab.c (completion_list_add_symbol): Remove aliasing msymbols
> >> 	when adding a C++ function symbol.
> >>
> >> gdb/testsuite/ChangeLog:
> >>
> >> 	* gdb.linespec/cp-completion-aliases.cc: New file.
> >> 	* gdb.linespec/cp-completion-aliases.exp: New file.
> > 
> > This causes GDB to crash for me, when debugging GDB itself, and then
> > doing "b main<TAB>".
> > 
> > $ ./gdb -data-directory=data-directory ./gdb -ex "complete b main"
> > GNU gdb (GDB) 10.0.50.20200319-git
> > ...
> > Reading symbols from ./gdb...
> > Setting up the environment for debugging gdb.
> > During symbol reading: unsupported tag: 'DW_TAG_unspecified_type'
> > During symbol reading: debug info gives source 55 included from file at zero line 0
> > During symbol reading: debug info gives command-line macro definition with non-zero line 19: _STDC_PREDEF_H 1
> > Breakpoint 1 at 0xaf48c3: file /home/pedro/gdb/mygit/src/gdbsupport/errors.cc, line 54.
> > During symbol reading: Member function "~_Sp_counted_base" (offset 0x683070) is virtual but the vtable offset is not specified
> > During symbol reading: const value length mismatch for 'std::ratio<1, 1000000000>::num', got 8, expected 0
> > During symbol reading: cannot get low and high bounds for subprogram DIE at 0x694d73
> > During symbol reading: Child DIE 0x6afc31 and its abstract origin 0x6afbcd have different parents
> > During symbol reading: Multiple children of DIE 0x6b2668 refer to DIE 0x6b2657 as their abstract origin
> > Breakpoint 2 at 0x4f6775: file /home/pedro/gdb/mygit/src/gdb/cli/cli-cmds.c, line 201.
> > Aborted (core dumped)
> > 
> > GDB seemingly crashes due to infinite recursion.  
> > 
> > Here's the top of the stack, showing the recursion starting:
> > 
> > $ gdb ./gdb -c core.4577
> > ...
> > Program terminated with signal SIGABRT, Aborted.
> > #0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
> > 51      }
> > [Current thread is 1 (Thread 0x7f9d6f25ecc0 (LWP 4577))]
> > (gdb) bt -44
> > #51229 0x000000000055533c in inspect_type (info=0x38ff720, ret_comp=0x3ba6b10, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:267
> > #51230 0x0000000000555a6f in replace_typedefs (info=0x38ff720, ret_comp=0x3ba6b10, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:475
> > #51231 0x0000000000555a36 in replace_typedefs (info=0x38ff720, ret_comp=0x3ba6b70, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:470
> > #51232 0x0000000000555800 in replace_typedefs_qualified_name (info=0x38ff720, ret_comp=0x37c7e20, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:389
> > #51233 0x0000000000555a8c in replace_typedefs (info=0x38ff720, ret_comp=0x37c7e20, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:479
> > #51234 0x000000000055533c in inspect_type (info=0x38ff720, ret_comp=0x37c7e20, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:267
> > #51235 0x0000000000555a6f in replace_typedefs (info=0x38ff720, ret_comp=0x37c7e20, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:475
> > #51236 0x0000000000555a36 in replace_typedefs (info=0x38ff720, ret_comp=0x37c7e80, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:470
> > #51237 0x0000000000555696 in replace_typedefs_qualified_name (info=0x38ff720, ret_comp=0x37c7dc0, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:357
> > #51238 0x0000000000555a8c in replace_typedefs (info=0x38ff720, ret_comp=0x37c7dc0, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:479
> > #51239 0x0000000000555a36 in replace_typedefs (info=0x38ff720, ret_comp=0x37c8400, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:470
> > #51240 0x0000000000555bd0 in cp_canonicalize_string_full[abi:cxx11](char const*, char const* (*)(type*, void*), void*) (string=0x980b2c0 "gdb::option::boolean_option_def<filename_partial_match_opts>::boolean_option_def(char const*, bool* (*)(filename_partial_match_opts*), void (*)(ui_file*, int, cmd_list_element*, char const*), char con"..., finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:527
> > #51241 0x0000000000555d28 in cp_canonicalize_string_no_typedefs[abi:cxx11](char const*) (string=0x980b2c0 "gdb::option::boolean_option_def<filename_partial_match_opts>::boolean_option_def(char const*, bool* (*)(filename_partial_match_opts*), void (*)(ui_file*, int, cmd_list_element*, char const*), char con"...) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:550
> > #51242 0x00000000008c4959 in completion_list_add_symbol (tracker=..., sym=0x980fde0, lookup_name=..., text=0x7fff6159e7f2 "main", word=0x7fff6159e7f2 "main") at /home/pedro/gdb/mygit/src/gdb/symtab.c:5316
> > #51243 0x00000000008c541b in add_symtab_completions (cust=0x673ba40, tracker=..., mode=complete_symbol_mode::LINESPEC, lookup_name=..., text=0x7fff6159e7f2 "main", word=0x7fff6159e7f2 "main", code=TYPE_CODE_UNDEF) at /home/pedro/gdb/mygit/src/gdb/symtab.c:5576
> > #51244 0x00000000008c5496 in <lambda(compunit_symtab*)>::operator()(compunit_symtab *) const (__closure=0x7fff6159e230, symtab=0x673ba40) at /home/pedro/gdb/mygit/src/gdb/symtab.c:5697
> > #51245 0x00000000008c8f14 in gdb::function_view<void(compunit_symtab*)>::<lambda(gdb::fv_detail::erased_callable, compunit_symtab*)>::operator()(gdb::fv_detail::erased_callable, compunit_symtab *) const (__closure=0x0, ecall=..., args#0=0x673ba40) at /home/pedro/gdb/mygit/src/gdb/../gdbsupport/function-view.h:263
> > #51246 0x00000000008c8f3c in gdb::function_view<void(compunit_symtab*)>::<lambda(gdb::fv_detail::erased_callable, compunit_symtab*)>::_FUN(gdb::fv_detail::erased_callable, compunit_symtab *) () at /home/pedro/gdb/mygit/src/gdb/../gdbsupport/function-view.h:257
> > #51247 0x0000000000603da8 in gdb::function_view<void (compunit_symtab*)>::operator()(compunit_symtab*) const (this=0x7fff6159df00, args#0=0x673ba40) at /home/pedro/gdb/mygit/src/gdb/../gdbsupport/function-view.h:247
> > #51248 0x00000000007c8483 in psym_expand_symtabs_matching(objfile *, gdb::function_view<bool(char const*, bool)>, const lookup_name_info &, gdb::function_view<bool(char const*)>, gdb::function_view<void(compunit_symtab*)>, search_domain) (objfile=0x232c6a0, file_matcher=..., lookup_name_in=..., symbol_matcher=..., expansion_notify=..., domain=ALL_DOMAIN) at /home/pedro/gdb/mygit/src/gdb/psymtab.c:1354
> > #51249 0x00000000008ab6a7 in expand_symtabs_matching(gdb::function_view<bool (char const*, bool)>, lookup_name_info const&, gdb::function_view<bool (char const*)>, gdb::function_view<void (compunit_symtab*)>, search_domain) (file_matcher=..., lookup_name=..., symbol_matcher=..., expansion_notify=..., kind=ALL_DOMAIN) at /home/pedro/gdb/mygit/src/gdb/symfile.c:3788
> > #51250 0x00000000008c5ab6 in default_collect_symbol_completion_matches_break_on (tracker=..., mode=complete_symbol_mode::LINESPEC, name_match_type=symbol_name_match_type::WILD, text=0x7fff6159e7f2 "main", word=0x7fff6159e7f2 "main", break_on=0xc3468e "", code=TYPE_CODE_UNDEF) at /home/pedro/gdb/mygit/src/gdb/symtab.c:5692
> > #51251 0x00000000008c5f79 in default_collect_symbol_completion_matches (tracker=..., mode=complete_symbol_mode::LINESPEC, name_match_type=symbol_name_match_type::WILD, text=0x7fff6159e7f2 "main", word=0x7fff6159e7f2 "main", code=TYPE_CODE_UNDEF) at /home/pedro/gdb/mygit/src/gdb/symtab.c:5795
> > #51252 0x00000000008c5fc2 in collect_symbol_completion_matches (tracker=..., mode=complete_symbol_mode::LINESPEC, name_match_type=symbol_name_match_type::WILD, text=0x7fff6159e7f2 "main", word=0x7fff6159e7f2 "main") at /home/pedro/gdb/mygit/src/gdb/symtab.c:5810
> > #51253 0x00000000006fcdf2 in linespec_complete_function (tracker=..., function=0x7fff6159e7f2 "main", func_match_type=symbol_name_match_type::WILD, source_filename=0x0) at /home/pedro/gdb/mygit/src/gdb/linespec.c:2816
> > #51254 0x00000000006fcecf in complete_linespec_component (parser=0x7fff6159e500, tracker=..., text=0x7fff6159e7f2 "main", component=linespec_complete_what::FUNCTION, source_filename=0x0) at /home/pedro/gdb/mygit/src/gdb/linespec.c:2848
> > #51255 0x00000000006fd63b in linespec_complete (tracker=..., text=0x7fff6159e7f2 "main", match_type=symbol_name_match_type::WILD) at /home/pedro/gdb/mygit/src/gdb/linespec.c:3063
> > #51256 0x00000000005405b1 in complete_address_and_linespec_locations (tracker=..., text=0x7fff6159e7f2 "main", match_type=symbol_name_match_type::WILD) at /home/pedro/gdb/mygit/src/gdb/completer.c:690
> > #51257 0x0000000000540eee in location_completer (ignore=0x1cb8320, tracker=..., text=0x7fff6159e7f2 "main") at /home/pedro/gdb/mygit/src/gdb/completer.c:1034
> > #51258 0x0000000000541024 in location_completer_handle_brkchars (ignore=0x1cb8320, tracker=..., text=0x7fff6159e7f2 "main", word_ignored=0x0) at /home/pedro/gdb/mygit/src/gdb/completer.c:1071
> > #51259 0x00000000005416e4 in complete_line_internal_normal_command (tracker=..., command=0x7fff6159e7f0 "b main", word=0x0, cmd_args=0x7fff6159e7f2 "main", reason=handle_brkchars, c=0x1cb8320) at /home/pedro/gdb/mygit/src/gdb/completer.c:1306
> > #51260 0x0000000000541b72 in complete_line_internal_1 (tracker=..., text=0x0, line_buffer=0x7fff615a0905 "b main", point=6, reason=handle_brkchars) at /home/pedro/gdb/mygit/src/gdb/completer.c:1531
> > #51261 0x0000000000541bb5 in complete_line_internal (tracker=..., text=0x0, line_buffer=0x7fff615a0905 "b main", point=6, reason=handle_brkchars) at /home/pedro/gdb/mygit/src/gdb/completer.c:1550
> > #51262 0x0000000000542cee in completion_find_completion_word (tracker=..., text=0x7fff615a0905 "b main", quote_char=0x7fff6159eb10) at /home/pedro/gdb/mygit/src/gdb/completer.c:2054
> > #51263 0x000000000054247a in complete (line=0x7fff615a0905 "b main", word=0x7fff6159eb08, quote_char=0x7fff6159eb10) at /home/pedro/gdb/mygit/src/gdb/completer.c:1770
> > #51264 0x00000000004f6ed0 in complete_command (arg=0x7fff615a0905 "b main", from_tty=1) at /home/pedro/gdb/mygit/src/gdb/cli/cli-cmds.c:364
> > #51265 0x00000000004fee70 in do_const_cfunc (c=0x2385760, args=0x7fff615a0905 "b main", from_tty=1) at /home/pedro/gdb/mygit/src/gdb/cli/cli-decode.c:107
> > #51266 0x0000000000501e28 in cmd_func (cmd=0x2385760, args=0x7fff615a0905 "b main", from_tty=1) at /home/pedro/gdb/mygit/src/gdb/cli/cli-decode.c:1952
> > #51267 0x000000000090d07d in execute_command (p=0x7fff615a0905 "b main", from_tty=1) at /home/pedro/gdb/mygit/src/gdb/top.c:655
> > #51268 0x000000000073b9c2 in catch_command_errors (command=0x90cc11 <execute_command(char const*, int)>, arg=0x7fff615a08fc "complete b main", from_tty=1) at /home/pedro/gdb/mygit/src/gdb/main.c:401
> > #51269 0x000000000073cdaa in captured_main_1 (context=0x7fff6159f070) at /home/pedro/gdb/mygit/src/gdb/main.c:1163
> > #51270 0x000000000073cf9f in captured_main (data=0x7fff6159f070) at /home/pedro/gdb/mygit/src/gdb/main.c:1188
> > #51271 0x000000000073d00a in gdb_main (args=0x7fff6159f070) at /home/pedro/gdb/mygit/src/gdb/main.c:1213
> > #51272 0x000000000041563e in main (argc=6, argv=0x7fff6159f178) at /home/pedro/gdb/mygit/src/gdb/gdb.c:32
> > 
> > This cycle keeps repeating:
> > #1709 0x000000000055533c in inspect_type (info=0x38ff720, ret_comp=0xd83be10, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:267
> > #1710 0x0000000000555a6f in replace_typedefs (info=0x38ff720, ret_comp=0xd83be10, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:475
> > #1711 0x0000000000555a36 in replace_typedefs (info=0x38ff720, ret_comp=0xd83be70, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:470
> > #1712 0x0000000000555800 in replace_typedefs_qualified_name (info=0x38ff720, ret_comp=0xd839470, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:389
> > #1713 0x0000000000555a8c in replace_typedefs (info=0x38ff720, ret_comp=0xd839470, finder=0x0, data=0x0) at /home/pedro/gdb/mygit/src/gdb/cp-support.c:479
> > 
> > 
> > The symbol on which we call cp_canonicalize_string_no_typedefs is:
> > 
> > (top-gdb) p *sym
> > $4 = {<general_symbol_info> = {m_name = 0x980b2c0 "gdb::option::boolean_option_def<filename_partial_match_opts>::boolean_option_def(char const*, bool* (*)(filename_partial_
> > match_opts*), void (*)(ui_file*, int, cmd_list_element*, char const*), char con"..., value = {ivalue = 159449440, block = 0x9810160, bytes = 0x9810160 "\212\271\214", addre
> > ss = 0x9810160, common_block = 0x9810160, chain = 0x9810160}, language_specific = {obstack = 0x0, demangled_name = 0x0}, m_language = language_cplus, ada_mangled = 0, secti
> > on = -1}, <allocate_on_obstack> = {<No data fields>}, type = 0x980fc20, owner = {symtab = 0x673bdf0, arch = 0x673bdf0}, domain = VAR_DOMAIN, aclass_index = 19, is_objfile_o
> > wned = 1, is_argument = 0, is_inlined = 0, maybe_copied = 0, subclass = SYMBOL_NONE, line = 157, aux_value = 0x980fe30, hash_next = 0x9821bd0}
> > 
> > (top-gdb) p sym->m_name 
> > $6 = 0x980b2c0 "gdb::option::boolean_option_def<filename_partial_match_opts>::boolean_option_def(char const*, bool* (*)(filename_partial_match_opts*), void (*)(ui_file*, in t, cmd_list_element*, char const*), char const*, char const*, char const*)"
> > 
> 
> I figured out what is special about this symbol.  The issue is that
> we have a boolean_option_def typedef in the global namespace in the
> program.  Here, in stack.c:
> 
>  using boolean_option_def
>    = gdb::option::boolean_option_def<frame_print_options>;
> 
> actually we have two.  There's another one in valprint.c:
> 
>  using boolean_option_def
>    = gdb::option::boolean_option_def<value_print_options>;
> 
> The recursion happens because we start by breaking the
> 
>    gdb::option::boolean_option_def<filename_partial_match_opts>::boolean_option_def(....)
> 
> symbol name into demanged parse info components.
> This happens in cp_canonicalize_string_no_typedefs -> replace_typedefs.
> 
> The tree for that symbol looks like this:
> 
> (top-gdb) p d_dump (ret_comp, 0)
> 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'
>   function type
>     argument list
>       pointer
>         const
>           builtin type char
>       argument list
>         pointer
>           function type
>             pointer
>               builtin type bool
>             argument list
>               pointer
>                 name 'value_print_options'
>         argument list
>           pointer
>             function type
>               builtin type void
>               argument list
>                 pointer
>                   name 'ui_file'
>                 argument list
>                   builtin type int
>                   argument list
>                     pointer
>                       name 'cmd_list_element'
>                     argument list
>                       pointer
>                         const
>                           builtin type char
>           argument list
>             pointer
>               const
>                 builtin type char
>             argument list
>               pointer
>                 const
>                   builtin type char
>               argument list
>                 pointer
>                   const
>                     builtin type char
> $5 = void
> 
> Let's just focus on the top part, because that's where
> the recursion happens:
> 
>  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'
> 
> As we walk the tree of components, we get to replace_typedefs_qualified_name,
> and enter the loop:
> 
>   while (comp->type == DEMANGLE_COMPONENT_QUAL_NAME)
>     {
>       if (d_left (comp)->type == DEMANGLE_COMPONENT_NAME)
> 	{
> 
> The DEMANGLE_COMPONENT_NAME case is hit twice, once for
> "gdb", and another for "option".  After those two,
> the next d_left (comp)->type is DEMANGLE_COMPONENT_TEMPLATE.
> 
> This now gets us to the else branch within that
> DEMANGLE_COMPONENT_QUAL_NAME loop:
> 
>       else
> 	{
> 	  /* The current node is not a name, so simply replace any
> 	     typedefs in it.  Then print it to the stream to continue
> 	     checking for more typedefs in the tree.  */
> 	  replace_typedefs (info, d_left (comp), finder, data);
> 
> which, after some more replace_typedefs recursion, gets us to
> 
> 	case DEMANGLE_COMPONENT_NAME:
> 	  inspect_type (info, ret_comp, finder, data);
> 	  break;
> 
> This is for this part of the tree:
> 
>          template
>            name 'boolean_option_def'
> 
> and it's this inspect_type call that's problematic.
> 
> [come back here later]
> 
> inspect_type does a symbol lookup for "boolean_option_def",
> which finds the unrelated typedef at the global namespace.
> In this session I'm debugging, it finds the one in valprint.c:
> 
> (top-gdb) p *sym
> $10 = {<general_symbol_info> = {m_name = 0x3a5dc90 "boolean_option_def", value = {ivalue = 0, block = 0x0, bytes = 0x0, address = 0x0, common_block = 0x0, chain = 0x0}, language_specific = {obstack = 0x0, demangled_name = 0x0}, m_language = language_cplus, ada_mangled = 0, section = -1}, <allocate_on_obstack> = {<No data fields>}, type = 0x3a613f0, owner = {symtab = 0x34ab930, arch = 0x34ab930}, domain = VAR_DOMAIN, aclass_index = 8, is_objfile_owned = 1, is_argument = 0, is_inlined = 0, maybe_copied = 0, subclass = SYMBOL_NONE, line = 2987, aux_value = 0x0, hash_next = 0x3a694e0}
> (top-gdb) p *sym.owner.symtab 
> $11 = {next = 0x34ab750, compunit_symtab = 0x34ab6d0, linetable = 0x3b83e60, filename = 0x2df4b20 "/home/pedro/gdb/mygit/src/gdb/valprint.c", language = language_cplus, fullname = 0x0}
> 
> This symbol's type is of course a typedef, so we try to resolve it.
> check_typedef returns this type:
> 
>  (top-gdb) p *type->main_type
>  $14 = 
>  {name = 0x2de5a40 "gdb::option::boolean_option_def<value_print_options>",
>   code = TYPE_CODE_STRUCT,
>  ...
> 
> So, since a typedef was resolved, we get here:
> 
> 	  /* Turn the result into a new tree.  Note that this
> 	     tree will contain pointers into NAME, so NAME cannot
> 	     be free'd until all typedef conversion is done and
> 	     the final result is converted into a string.  */
> 	  i = cp_demangled_name_to_comp (name, NULL);
> 	  if (i != NULL)
> 	    {
> 	      /* Merge the two trees.  */
> 	      cp_merge_demangle_parse_infos (info, ret_comp, i.get ());
> 
> 	      /* Replace any newly introduced typedefs -- but not
> 		 if the type is anonymous (that would lead to infinite
> 		 looping).  */
> 	      if (!is_anon)
> 		{
> 		  fprintf_unfiltered (gdb_stdlog, "replace_typedefs: %s\n",
> 				      name);
> 
> 		  replace_typedefs (info, ret_comp, finder, data);
> 
> And it's that last replace_typedefs call that triggers the recursion,
> since we're now going to process a tree
> 
>  gdb::option::boolean_option_def<value_print_options>
>               ^^^^^^^^^^^^^^^^^^
> 
> that will again end up doing a lookup for that global scope
> boolean_option_def typedef, rinse repeat.
> 
> Locally, I put a recursion limit in inspect_type, and the resulting 
> symbol name that cp_canonicalize_string_no_typedefs returns is:
> 
> $18 = std::unique_ptr<char> containing 0x32f81a0 "gdb::option::gdb::option::gdb::option::gdb::option::gdb::option::gdb::option::gdb::option::gdb::option::gdb::option::gdb::
> option::gdb::option::gdb::option::gdb::option::gdb::option::gdb::option::gdb::option::gdb::option::gdb::option::gdb::option::gdb::option::gdb::option::boolean_option_def<va
> lue_print_options><value_print_options><value_print_options><value_print_options><value_print_options><value_print_options><value_print_options><value_print_options><value_
> print_options><value_print_options><value_print_options><value_print_options><value_print_options><value_print_options><value_print_options><value_print_options><value_prin
> t_options><value_print_options><value_print_options><value_print_options><value_print_options>::boolean_option_def(char const*, bool* (*)(value_print_options*), void (*)(ui
> _file*, int, cmd_list_element*, char const*), char const*, char const*, char const*)"
> 
> Obviously broken due to recursion around boolean_option_def.
> 
> 
> Now, let's get back to that inspect_type call that I mentioned
> was problematic.  See [come back here later] above.
> 
> 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
> 
> so we should be looking for a "boolean_option_def" typedef in
> the gdb::option namespace or struct/class.  Right?
> 
> The question to me is then, how to do that.
> In replace_typedefs_qualified_name, after we've processed
> "gdb" and "option", we have "gdb::option::" stored in
> the "buf" string_file, which seems perfect.  However, how do we
> get that scope down to inspect_type, since we have layers of
> replace_typedefs calls before we get there?
> 
> I can think of two ways:
> 
> #1 - store the current scope in the demangle_parse_info
> context object, and have inspect_type consult that.
> 
> #2 - Make replace_typedefs_qualified_name
> look ahead in the tree, inlining the
> DEMANGLE_COMPONENT_TEMPLATE case here, basically
> inlining enough forward peeking in order to do a directly
> inspect_type call from within replace_typedefs_qualified_name.
> Of course, we would likely have to do the same for other
> node types, not just DEMANGLE_COMPONENT_TEMPLATE.
> 
> And then, I'm not sure we should pass the scope as extra
> parameter to inspect_type, or, whether we should replace
> the "boolean_option_def" node in the tree with one that has
> a full-scoped name "gdb::option::boolean_option_def" name,
> similarly to how replace_typedefs_qualified_name
> handles the DEMANGLE_COMPONENT_NAME case.
> 
> One difficulty I have with this, is that I couldn't find
> any case of this code in replace_typedefs_qualified_name:
> 
> 	  /* The current node is not a name, so simply replace any
> 	     typedefs in it.  Then print it to the stream to continue
> 	     checking for more typedefs in the tree.  */
> 	  replace_typedefs (info, d_left (comp), finder, data);
> 
> actually ever replacing any typedef in the testsuite.
> I mean, I put an abort() in place if the returned component
> as a string is different from the name before the 
> replace_typedefs call, and then ran the gdb.cp/ testcases,
> and the abort never triggered...
> 
> So, in order to get the discussion going, I wrote a patch
> for solution #1.  This fixes the issue for me, and causes no
> testsuite regressions.  Find it below.
> 
> WDYT?
> 
> > 
> > I'm seeing this on a GDB built with:
> >  gcc version 7.3.1 20180712 (Red Hat 7.3.1-6) (GCC) 
> > on Fedora 27.
> > 
> > Anyone else seeing this?
> 
> I'm also seeing this when I build GDB with GCC 10 (and debug
> that GDB with itself), so it must be others see this too?

Yes I can reproduce the original issue you are seeing trying to
complete on main.

You also mentioned a couple of times seeing problems with the test
gdb.linespec/cp-completion-aliases.exp, this I don't see - you said
even with the patch you already pushed you were still seeing issues
with this test, and I can't reproduce this.

Still I think your first patch looks like a nice improvement, so
thanks for that.  The patch below looks like a good solution to me, I
only had one tiny bit of feedback...

> 
> From af60bb6c6d7c66307db49e524409bc81f1322907 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <palves@redhat.com>
> Date: Mon, 25 May 2020 13:14:10 +0100
> Subject: [PATCH] Fix gdb::option::boolean_option_def recursion
> 
> ---
>  gdb/cp-support.c | 15 +++++++++++----
>  gdb/cp-support.h |  5 ++++-
>  2 files changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/gdb/cp-support.c b/gdb/cp-support.c
> index 1e54aaea3b1..2e4db8a7007 100644
> --- a/gdb/cp-support.c
> +++ b/gdb/cp-support.c
> @@ -137,14 +137,15 @@ inspect_type (struct demangle_parse_info *info,
>  	      canonicalization_ftype *finder,
>  	      void *data)
>  {
> -  char *name;
>    struct symbol *sym;
>  
>    /* Copy the symbol's name from RET_COMP and look it up
>       in the symbol table.  */
> -  name = (char *) alloca (ret_comp->u.s_name.len + 1);
> -  memcpy (name, ret_comp->u.s_name.s, ret_comp->u.s_name.len);
> -  name[ret_comp->u.s_name.len] = '\0';
> +  std::string name_str;
> +  if (info->current_scope != nullptr)
> +    name_str = info->current_scope;
> +  name_str.append (ret_comp->u.s_name.s, ret_comp->u.s_name.len);
> +  const char *name = name_str.c_str ();
>  
>    /* Ignore any typedefs that should not be substituted.  */
>    for (int i = 0; i < ARRAY_SIZE (ignore_typedefs); ++i)
> @@ -355,7 +356,13 @@ replace_typedefs_qualified_name (struct demangle_parse_info *info,
>  	  /* The current node is not a name, so simply replace any
>  	     typedefs in it.  Then print it to the stream to continue
>  	     checking for more typedefs in the tree.  */
> +
> +	  /* The struct/template/function/field/type name should be
> +	     qualified in the current scope though.  */
> +	  info->current_scope = buf.string ().c_str ();
>  	  replace_typedefs (info, d_left (comp), finder, data);
> +	  info->current_scope = nullptr;

I don't know if you should be using make_scoped_restore to protect
info->current_scope?

Thanks,
Andrew

> +
>  	  gdb::unique_xmalloc_ptr<char> name
>  	    = cp_comp_to_string (d_left (comp), 100);
>  	  if (name == NULL)
> diff --git a/gdb/cp-support.h b/gdb/cp-support.h
> index 6ca898315bb..561de5a94df 100644
> --- a/gdb/cp-support.h
> +++ b/gdb/cp-support.h
> @@ -72,8 +72,11 @@ struct demangle_parse_info
>  
>    /* Any temporary memory used during typedef replacement.  */
>    struct obstack obstack;
> -};
>  
> +  /* The current qualified scope context, during typedef
> +     replacement.  */
> +  const char *current_scope = nullptr;
> +};
>  
>  /* Functions from cp-support.c.  */
>  
> 
> base-commit: af2c48d8545cbc24aa6caf9b9f2a49ab6c0aaa7b
> -- 
> 2.14.5
> 

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

[Pedro Alves]

On 5/26/20 6:02 PM, Andrew Burgess wrote:
> * Pedro Alves <palves@redhat.com> [2020-05-25 15:34:27 +0100]:
> 

>>> Anyone else seeing this?
>>
>> I'm also seeing this when I build GDB with GCC 10 (and debug
>> that GDB with itself), so it must be others see this too?
> 
> Yes I can reproduce the original issue you are seeing trying to
> complete on main.
> 

Ah, cool.  With current master completing on main should no
longer crash, but you should be able to see it with:

  "gdb gdb::option::[TAB]"

> You also mentioned a couple of times seeing problems with the test
> gdb.linespec/cp-completion-aliases.exp, this I don't see - you said
> even with the patch you already pushed you were still seeing issues
> with this test, and I can't reproduce this.

I don't think I ever said that.  ISTR someone else mentioning seeing
problems with completion tests on the list (Tom de Vries?).  Most (all?)
completion tests got broken with the completion styling.  But Tromey
reversed that feature so that testsuite breakage should be fixed now.

> 
> Still I think your first patch looks like a nice improvement, so
> thanks for that.  The patch below looks like a good solution to me, I
> only had one tiny bit of feedback...
> 
>>

> I don't know if you should be using make_scoped_restore to protect
> info->current_scope?

Yeah, probably.

However, I'm still not clear on whether this is the right fix.
I think we should see what happens if a typedef is really
substituted.

If, say, we really found a typedef called "boolean_option_def"
under gdb::option, what will the name of that resolved typedef
be?  I suppose it will be a fully qualified name, and thus
back in replace_typedefs_qualified_name we should be
replacing the whole qualified name with the new resolved name?
And what if inspect_type resolves a typedef, and then recurses
again into replace_typedefs?  Will leaving info->current_scope
as "gdb::option::" break that?

Thanks,
Pedro Alves

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

[Keith Seitz]

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.

> One difficulty I have with this, is that I couldn't find
> any case of this code in replace_typedefs_qualified_name:
> 
> 	  /* The current node is not a name, so simply replace any
> 	     typedefs in it.  Then print it to the stream to continue
> 	     checking for more typedefs in the tree.  */
> 	  replace_typedefs (info, d_left (comp), finder, data);
> 
> actually ever replacing any typedef in the testsuite.
> I mean, I put an abort() in place if the returned component
> as a string is different from the name before the 
> replace_typedefs call, and then ran the gdb.cp/ testcases,
> and the abort never triggered...

Huh. I'm not sure what to say. Normally, I'm pretty thorough with
coverage testing. Either I missed something during test writing,
or perhaps this has code has since been rendered unnecessary?

Keith

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

[Pedro Alves]

[-- 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

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

[Pedro Alves]

On 5/27/20 8:36 PM, Pedro Alves via Gdb-patches wrote:
> 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...

I ended up going ahead and fixing it all.  I've sent a new
patch as a new thread, here:

  https://sourceware.org/pipermail/gdb-patches/2020-May/169097.html

Thanks for the hint Keith, that was really helpful in setting me
on the right path.

Thanks,
Pedro Alves