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

classic Classic list List threaded Threaded
26 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[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

Reply | Threaded
Open this post in threaded view
|

[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

Reply | Threaded
Open this post in threaded view
|

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

Andrew Burgess
In reply to this post by 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

Reply | Threaded
Open this post in threaded view
|

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

Tom Tromey-2
In reply to this post by Andrew Burgess
>>>>> "Andrew" == Andrew Burgess <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Tom Tromey-2
In reply to this post by Andrew Burgess
>>>>> "Andrew" == Andrew Burgess <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Sourceware - gdb-patches mailing list
In reply to this post by Tom Tromey-2
On Fri, Jan 24, 2020, 20:14 Tom Tromey <[hidden email]> wrote:

> >>>>> "Andrew" == Andrew Burgess <[hidden email]> 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
>
Reply | Threaded
Open this post in threaded view
|

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

Tom Tromey-2
>>>>> "Christian" == Christian Biesinger <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Andrew Burgess
In reply to this post by 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

Reply | Threaded
Open this post in threaded view
|

[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

Reply | Threaded
Open this post in threaded view
|

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

Andrew Burgess
In reply to this post by 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

Reply | Threaded
Open this post in threaded view
|

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

Andrew Burgess
In reply to this post by 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

Reply | Threaded
Open this post in threaded view
|

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

Andrew Burgess
In reply to this post by Andrew Burgess
* Andrew Burgess <[hidden email]> [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
>
Reply | Threaded
Open this post in threaded view
|

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

Andrew Burgess
In reply to this post by Andrew Burgess
Ping!

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

Thanks,
Andrew


* Andrew Burgess <[hidden email]> [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
>
Reply | Threaded
Open this post in threaded view
|

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

Tom Tromey-2
>>>>> "Andrew" == Andrew Burgess <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Sourceware - gdb-patches mailing list
In reply to this post by Andrew Burgess
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
>
Reply | Threaded
Open this post in threaded view
|

[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

Reply | Threaded
Open this post in threaded view
|

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

Tom Tromey-2
>>>>> "Andrew" == Andrew Burgess <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Andrew Burgess
* Tom Tromey <[hidden email]> [2020-04-06 14:27:14 -0600]:

> >>>>> "Andrew" == Andrew Burgess <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

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

Sourceware - gdb-patches mailing list
In reply to this post by Andrew Burgess
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

Reply | Threaded
Open this post in threaded view
|

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

Sourceware - gdb-patches mailing list
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 <[hidden email]>
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  <[hidden email]>

        * 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  <[hidden email]>
+
+ * 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  <[hidden email]>
 
  * 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


12