lookup_name_info::make_ignore_params

A few places in the completion code look for a "(" to find a
function's parameter list, in order to strip it, because psymtabs (and
gdb index) don't include parameter info in the symbol names.

See compare_symbol_name and
default_collect_symbol_completion_matches_break_on.

This is too naive.  Consider:

 ns_overload2_test::([TAB]

We'd want to complete that to:
 ns_overload2_test::(anonymous namespace)::struct_overload2_test

Or:

 b (anonymous namespace)::[TAB]

That currently completes to:

 b (anonymous namespace)

Which is obviously broken.  This patch makes that work.

Also, the current compare_symbol_name hack means that while this
works:

 "b function([TAB]"  ->  "b function()"

This does not:

 "b function ([TAB]"

This patch fixes that.  Whitespace "ignoring" now Just Works, i.e.,
assuming a symbol named "function(int, long)", this:
 b function (   int , lon[TAB]
completes to:
 b function (   int , long)

To address all of this, this patch builds on top of the rest of the
series, and pushes the responsibility of stripping parameters from a
lookup name to the new lookup_name_info object, where we can apply
per-language rules.  Also note that we now only make a version of the
lookup name with parameters stripped out where it's actually required
to do that, in the psymtab and GDB index code.

For C++, the right way to strip parameters is with "cp_remove_params",
which uses a real parser (cp-name-parser.y) to split the name into a
component tree and then discards parameters.

The trouble for completion is that in that case we have an incomplete
name, like "foo::func(int" and thus cp_remove_params throws an error.

This patch sorts that by adding a cp_remove_params_if_any variant of
cp_remove_params that tries removing characters from the end of the
string until cp_remove_params works.  So cp_remove_params_if_any
behaves like this:

With a complete name:

   "foo::func(int)"  => foo::func(int)  # cp_remove_params_1 succeeds the first time.

With an incomplete name:

   "foo::func(int"  => NULL             # cp_remove_params fails the first time.
   "foo::func(in"   => NULL             # and again...
   "foo::func(i"    => NULL             # and again...
   "foo::func("     => NULL             # and again...
   "foo::func"      => "foo::func"      # success!

Note that even if this approach removes significant rightmost
characters, it's still OK, because this parameter stripping is only
necessary for psymtabs and gdb index, where we're determining whether
to expand a symbol table.  Say cp_remove_params_if_any returned
"foo::" above for "foo::func(int".  That'd cause us to expand more
symtabs than ideal (because we'd expand all symtabs with symbols that
start with "foo::", not just "foo::func"), but then when we actually
look for completion matches, we'd still use the original lookup name,
with parameter information ["foo::func(int"], and thus we'll return no
false positive to the user.  Whether the stripping works as intended
and doesn't strip too much is thus covered by a unit test instead of a
testsuite test.

The "if_any" part of the name refers to the fact that while
cp_remove_params returns NULL if the input name has no parameters in
the first place, like:

   "foo::func"      => NULL         # cp_remove_params

cp_remove_params_if_any still returns the function name:

   "foo::func"      => "foo::func"  # cp_remove_params_if_any

gdb/ChangeLog:
2017-11-08  Pedro Alves  <palves@redhat.com>

	* Makefile.in (SUBDIR_UNITTESTS_SRCS): Add
	unittests/lookup_name_info-selftests.c.
	(SUBDIR_UNITTESTS_OBS): Add lookup_name_info-selftests.o.
	* cp-support.c: Include "selftest.h".
	(cp_remove_params_1): Rename from cp_remove_params.  Add
	'require_param' parameter, and handle it.
	(cp_remove_params): Reimplement.
	(cp_remove_params_if_any): New.
	(selftests::quote): New.
	(selftests::check_remove_params): New.
	(selftests::test_cp_remove_params): New.
	(_initialize_cp_support): Install
	selftests::test_cp_remove_params.
	* cp-support.h (cp_remove_params_if_any): Declare.
	* dwarf2read.c :Include "selftest.h".
	(dw2_expand_symtabs_matching_symbol): Use
	lookup_name_info::make_ignore_params.
	(selftests::dw2_expand_symtabs_matching::mock_mapped_index)
	(selftests::dw2_expand_symtabs_matching::string_or_null)
	(selftests::dw2_expand_symtabs_matching::check_match)
	(selftests::dw2_expand_symtabs_matching::test_symbols)
	(selftests::dw2_expand_symtabs_matching::run_test): New.
	(_initialize_dwarf2_read): Register
	selftests::dw2_expand_symtabs_matching::run_test.
	* psymtab.c (psym_expand_symtabs_matching): Use
	lookup_name_info::make_ignore_params.
	* symtab.c (demangle_for_lookup_info::demangle_for_lookup_info):
	If the lookup name wants to ignore parameters, strip them.
	(compare_symbol_name): Remove sym_text/sym_text_len parameters and
	code handling '('.
	(completion_list_add_name): Don't pass down sym_text/sym_text_len.
	(default_collect_symbol_completion_matches_break_on): Don't try to
	strip parameters.
	* symtab.h (lookup_name_info::lookup_name_info): Add
	'ignore_parameters' parameter.
	(lookup_name_info::ignore_parameters)
	(lookup_name_info::make_ignore_params): New methods.
	(lookup_name_info::m_ignore_parameters): New field.
	* unittests/lookup_name_info-selftests.c: New file.

1 files changed, 295 insertions, 5 deletions

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index d715082..389d8f7 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -81,6 +81,7 @@
 #include <algorithm>
 #include <unordered_set>
 #include <unordered_map>
+#include "selftest.h"
 
 typedef struct symbol *symbolp;
 DEF_VEC_P (symbolp);
@@ -4198,13 +4199,15 @@ gdb_index_symbol_name_matcher::matches (const char *symbol_name)
 static void
 dw2_expand_symtabs_matching_symbol
   (mapped_index &index,
-   const lookup_name_info &lookup_name,
+   const lookup_name_info &lookup_name_in,
    gdb::function_view<expand_symtabs_symbol_matcher_ftype> symbol_matcher,
    enum search_domain kind,
    gdb::function_view<void (offset_type)> match_callback)
 {
+  lookup_name_info lookup_name_without_params
+    = lookup_name_in.make_ignore_params ();
   gdb_index_symbol_name_matcher lookup_name_matcher
-    (lookup_name);
+    (lookup_name_without_params);
 
   auto *name_cmp = case_sensitivity == case_sensitive_on ? strcmp : strcasecmp;
 
@@ -4262,7 +4265,7 @@ dw2_expand_symtabs_matching_symbol
     }
 
   const char *cplus
-    = lookup_name.cplus ().lookup_name ().c_str ();
+    = lookup_name_without_params.cplus ().lookup_name ().c_str ();
 
   /* Comparison function object for lower_bound that matches against a
      given symbol name.  */
@@ -4290,7 +4293,7 @@ dw2_expand_symtabs_matching_symbol
   /* Find the lower bound.  */
   auto lower = [&] ()
     {
-      if (lookup_name.completion_mode () && cplus[0] == '\0')
+      if (lookup_name_in.completion_mode () && cplus[0] == '\0')
 	return begin;
       else
 	return std::lower_bound (begin, end, cplus, lookup_compare_lower);
@@ -4299,7 +4302,7 @@ dw2_expand_symtabs_matching_symbol
   /* Find the upper bound.  */
   auto upper = [&] ()
     {
-      if (lookup_name.completion_mode ())
+      if (lookup_name_in.completion_mode ())
 	{
 	  /* The string frobbing below won't work if the string is
 	     empty.  We don't need it then, anyway -- if we're
@@ -4365,6 +4368,288 @@ dw2_expand_symtabs_matching_symbol
   static_assert (sizeof (prev) > sizeof (offset_type), "");
 }
 
+#if GDB_SELF_TEST
+
+namespace selftests { namespace dw2_expand_symtabs_matching {
+
+/* A wrapper around mapped_index that builds a mock mapped_index, from
+   the symbol list passed as parameter to the constructor.  */
+class mock_mapped_index
+{
+public:
+  template<size_t N>
+  mock_mapped_index (const char *(&symbols)[N])
+    : mock_mapped_index (symbols, N)
+  {}
+
+  /* Access the built index.  */
+  mapped_index &index ()
+  { return m_index; }
+
+  /* Disable copy.  */
+  mock_mapped_index(const mock_mapped_index &) = delete;
+  void operator= (const mock_mapped_index &) = delete;
+
+private:
+  mock_mapped_index (const char **symbols, size_t symbols_size)
+  {
+    /* No string can live at offset zero.  Add a dummy entry.  */
+    obstack_grow_str0 (&m_constant_pool, "");
+
+    for (size_t i = 0; i < symbols_size; i++)
+      {
+	const char *sym = symbols[i];
+	size_t offset = obstack_object_size (&m_constant_pool);
+	obstack_grow_str0 (&m_constant_pool, sym);
+	m_symbol_table.push_back (offset);
+	m_symbol_table.push_back (0);
+      };
+
+    m_index.constant_pool = (const char *) obstack_base (&m_constant_pool);
+    m_index.symbol_table = m_symbol_table.data ();
+    m_index.symbol_table_slots = m_symbol_table.size () / 2;
+  }
+
+public:
+  /* The built mapped_index.  */
+  mapped_index m_index{};
+
+  /* The storage that the built mapped_index uses for symbol and
+     constant pool tables.  */
+  std::vector<offset_type> m_symbol_table;
+  auto_obstack m_constant_pool;
+};
+
+/* Convenience function that converts a NULL pointer to a "<null>"
+   string, to pass to print routines.  */
+
+static const char *
+string_or_null (const char *str)
+{
+  return str != NULL ? str : "<null>";
+}
+
+/* Check if a lookup_name_info built from
+   NAME/MATCH_TYPE/COMPLETION_MODE matches the symbols in the mock
+   index.  EXPECTED_LIST is the list of expected matches, in expected
+   matching order.  If no match expected, then an empty list is
+   specified.  Returns true on success.  On failure prints a warning
+   indicating the file:line that failed, and returns false.  */
+
+static bool
+check_match (const char *file, int line,
+	     mock_mapped_index &mock_index,
+	     const char *name, symbol_name_match_type match_type,
+	     bool completion_mode,
+	     std::initializer_list<const char *> expected_list)
+{
+  lookup_name_info lookup_name (name, match_type, completion_mode);
+
+  bool matched = true;
+
+  auto mismatch = [&] (const char *expected_str,
+		       const char *got)
+  {
+    warning (_("%s:%d: match_type=%s, looking-for=\"%s\", "
+	       "expected=\"%s\", got=\"%s\"\n"),
+	     file, line,
+	     (match_type == symbol_name_match_type::FULL
+	      ? "FULL" : "WILD"),
+	     name, string_or_null (expected_str), string_or_null (got));
+    matched = false;
+  };
+
+  auto expected_it = expected_list.begin ();
+  auto expected_end = expected_list.end ();
+
+  dw2_expand_symtabs_matching_symbol (mock_index.index (), lookup_name,
+				      NULL, ALL_DOMAIN,
+				      [&] (offset_type idx)
+  {
+    const char *matched_name = mock_index.index ().symbol_name_at (idx);
+    const char *expected_str
+      = expected_it == expected_end ? NULL : *expected_it++;
+
+    if (expected_str == NULL || strcmp (expected_str, matched_name) != 0)
+      mismatch (expected_str, matched_name);
+  });
+
+  const char *expected_str
+  = expected_it == expected_end ? NULL : *expected_it++;
+  if (expected_str != NULL)
+    mismatch (expected_str, NULL);
+
+  return matched;
+}
+
+/* The symbols added to the mock mapped_index for testing (in
+   canonical form).  */
+static const char *test_symbols[] = {
+  "function",
+  "std::bar",
+  "std::zfunction",
+  "std::zfunction2",
+  "w1::w2",
+  "ns::foo<char*>",
+  "ns::foo<int>",
+  "ns::foo<long>",
+
+  /* A name with all sorts of complications.  Starts with "z" to make
+     it easier for the completion tests below.  */
+#define Z_SYM_NAME \
+  "z::std::tuple<(anonymous namespace)::ui*, std::bar<(anonymous namespace)::ui> >" \
+    "::tuple<(anonymous namespace)::ui*, " \
+    "std::default_delete<(anonymous namespace)::ui>, void>"
+
+  Z_SYM_NAME
+};
+
+static void
+run_test ()
+{
+  mock_mapped_index mock_index (test_symbols);
+
+  /* We let all tests run until the end even if some fails, for debug
+     convenience.  */
+  bool any_mismatch = false;
+
+  /* Create the expected symbols list (an initializer_list).  Needed
+     because lists have commas, and we need to pass them to CHECK,
+     which is a macro.  */
+#define EXPECT(...) { __VA_ARGS__ }
+
+  /* Wrapper for check_match that passes down the current
+     __FILE__/__LINE__.  */
+#define CHECK_MATCH(NAME, MATCH_TYPE, COMPLETION_MODE, EXPECTED_LIST)	\
+  any_mismatch |= !check_match (__FILE__, __LINE__,			\
+				mock_index,				\
+				NAME, MATCH_TYPE, COMPLETION_MODE,	\
+				EXPECTED_LIST)
+
+  /* Identity checks.  */
+  for (const char *sym : test_symbols)
+    {
+      /* Should be able to match all existing symbols.  */
+      CHECK_MATCH (sym, symbol_name_match_type::FULL, false,
+		   EXPECT (sym));
+
+      /* Should be able to match all existing symbols with
+	 parameters.  */
+      std::string with_params = std::string (sym) + "(int)";
+      CHECK_MATCH (with_params.c_str (), symbol_name_match_type::FULL, false,
+		   EXPECT (sym));
+
+      /* Should be able to match all existing symbols with
+	 parameters and qualifiers.  */
+      with_params = std::string (sym) + " ( int ) const";
+      CHECK_MATCH (with_params.c_str (), symbol_name_match_type::FULL, false,
+		   EXPECT (sym));
+
+      /* This should really find sym, but cp-name-parser.y doesn't
+	 know about lvalue/rvalue qualifiers yet.  */
+      with_params = std::string (sym) + " ( int ) &&";
+      CHECK_MATCH (with_params.c_str (), symbol_name_match_type::FULL, false,
+		   {});
+    }
+
+  /* Check that completion mode works at each prefix of the expected
+     symbol name.  */
+  {
+    static const char str[] = "function(int)";
+    size_t len = strlen (str);
+    std::string lookup;
+
+    for (size_t i = 1; i < len; i++)
+      {
+	lookup.assign (str, i);
+	CHECK_MATCH (lookup.c_str (), symbol_name_match_type::FULL, true,
+		     EXPECT ("function"));
+      }
+  }
+
+  /* While "w" is a prefix of both components, the match function
+     should still only be called once.  */
+  {
+    CHECK_MATCH ("w", symbol_name_match_type::FULL, true,
+		 EXPECT ("w1::w2"));
+  }
+
+  /* Same, with a "complicated" symbol.  */
+  {
+    static const char str[] = Z_SYM_NAME;
+    size_t len = strlen (str);
+    std::string lookup;
+
+    for (size_t i = 1; i < len; i++)
+      {
+	lookup.assign (str, i);
+	CHECK_MATCH (lookup.c_str (), symbol_name_match_type::FULL, true,
+		     EXPECT (Z_SYM_NAME));
+      }
+  }
+
+  /* In FULL mode, an incomplete symbol doesn't match.  */
+  {
+    CHECK_MATCH ("std::zfunction(int", symbol_name_match_type::FULL, false,
+		 {});
+  }
+
+  /* A complete symbol with parameters matches any overload, since the
+     index has no overload info.  */
+  {
+    CHECK_MATCH ("std::zfunction(int)", symbol_name_match_type::FULL, true,
+		 EXPECT ("std::zfunction", "std::zfunction2"));
+  }
+
+  /* Check that whitespace is ignored appropriately.  A symbol with a
+     template argument list. */
+  {
+    static const char expected[] = "ns::foo<int>";
+    CHECK_MATCH ("ns :: foo < int > ", symbol_name_match_type::FULL, false,
+		 EXPECT (expected));
+  }
+
+  /* Check that whitespace is ignored appropriately.  A symbol with a
+     template argument list that includes a pointer.  */
+  {
+    static const char expected[] = "ns::foo<char*>";
+    /* Try both completion and non-completion modes.  */
+    static const bool completion_mode[2] = {false, true};
+    for (size_t i = 0; i < 2; i++)
+      {
+	CHECK_MATCH ("ns :: foo < char * >", symbol_name_match_type::FULL,
+		     completion_mode[i], EXPECT (expected));
+
+	CHECK_MATCH ("ns :: foo < char * > (int)", symbol_name_match_type::FULL,
+		     completion_mode[i], EXPECT (expected));
+      }
+  }
+
+  {
+    /* Check method qualifiers are ignored.  */
+    static const char expected[] = "ns::foo<char*>";
+    CHECK_MATCH ("ns :: foo < char * >  ( int ) const",
+		 symbol_name_match_type::FULL, true, EXPECT (expected));
+    CHECK_MATCH ("ns :: foo < char * >  ( int ) &&",
+		 symbol_name_match_type::FULL, true, EXPECT (expected));
+  }
+
+  /* Test lookup names that don't match anything.  */
+  {
+    CHECK_MATCH ("doesntexist", symbol_name_match_type::FULL, false,
+		 {});
+  }
+
+  SELF_CHECK (!any_mismatch);
+
+#undef EXPECT
+#undef CHECK_MATCH
+}
+
+}} // namespace selftests::dw2_expand_symtabs_matching
+
+#endif /* GDB_SELF_TEST */
+
 /* Helper for dw2_expand_matching symtabs.  Called on each symbol
    matched, to expand corresponding CUs that were marked.  IDX is the
    index of the symbol name that matched.  */
@@ -24454,4 +24739,9 @@ Usage: save gdb-index DIRECTORY"),
 					&dwarf2_block_frame_base_locexpr_funcs);
   dwarf2_loclist_block_index = register_symbol_block_impl (LOC_BLOCK,
 					&dwarf2_block_frame_base_loclist_funcs);
+
+#if GDB_SELF_TEST
+  selftests::register_test ("dw2_expand_symtabs_matching",
+			    selftests::dw2_expand_symtabs_matching::run_test);
+#endif
 }