From patchwork Wed Nov 8 16:18:34 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Pedro Alves X-Patchwork-Id: 24157 Received: (qmail 76273 invoked by alias); 8 Nov 2017 16:18:43 -0000 Mailing-List: contact gdb-patches-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: gdb-patches-owner@sourceware.org Delivered-To: mailing list gdb-patches@sourceware.org Received: (qmail 75825 invoked by uid 89); 8 Nov 2017 16:18:43 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-25.8 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_LAZY_DOMAIN_SECURITY, KAM_STOCKGEN, RP_MATCHES_RCVD, SPF_HELO_PASS autolearn=ham version=3.3.2 spammy=clip, tracker, smoke X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 08 Nov 2017 16:18:37 +0000 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 0657E356DC for ; Wed, 8 Nov 2017 16:18:36 +0000 (UTC) Received: from [127.0.0.1] (ovpn04.gateway.prod.ext.ams2.redhat.com [10.39.146.4]) by smtp.corp.redhat.com (Postfix) with ESMTP id E873A649A7; Wed, 8 Nov 2017 16:18:34 +0000 (UTC) Subject: Re: [PATCH 28/40] lookup_name_info::make_ignore_params To: Keith Seitz , gdb-patches@sourceware.org References: <1496406158-12663-1-git-send-email-palves@redhat.com> <1496406158-12663-29-git-send-email-palves@redhat.com> <74eadbb2-06b1-cad9-1214-6757e662f4b3@redhat.com> From: Pedro Alves Message-ID: <31a2c994-9aca-b065-02df-8ad5dfed4e12@redhat.com> Date: Wed, 8 Nov 2017 16:18:34 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <74eadbb2-06b1-cad9-1214-6757e662f4b3@redhat.com> On 08/08/2017 09:55 PM, Keith Seitz wrote: > On 06/02/2017 05:22 AM, Pedro Alves wrote: >> gdb/ChangeLog: >> yyyy-mm-dd Pedro Alves >> >> * 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's parameter, and handle it. > ^^ > typo > Fixed. >> @@ -833,12 +834,14 @@ cp_func_name (const char *full_name) >> return ret; >> } >> >> -/* DEMANGLED_NAME is the name of a function, including parameters and >> - (optionally) a return type. Return the name of the function without >> - parameters or return type, or NULL if we can not parse the name. */ >> +/* Helper for cp_remove_params. DEMANGLED_NAME is the name of a >> + function, including parameters and (optionally) a return type. >> + Return the name of the function without parameters or return type, >> + or NULL if we can not parse the name. If COMPLETION_MODE is true, >> + then tolerate a non-existing or unbalanced parameter list. */ > > s/COMPLETION_MODE/REQUIRE_PARAMS/ ? Whoops, yes, it was named completion_mode at some point before I posted this. > >> -gdb::unique_xmalloc_ptr >> -cp_remove_params (const char *demangled_name) >> +static gdb::unique_xmalloc_ptr >> +cp_remove_params_1 (const char *demangled_name, bool require_params) >> { >> bool done = false; >> struct demangle_component *ret_comp; >> @@ -874,10 +877,60 @@ cp_remove_params (const char *demangled_name) > [snip] >> +/* DEMANGLED_NAME is the name of a function, (optionally) including >> + parameters and (optionally) a return type. Return the name of the >> + function without parameters or return type, or NULL if we can not >> + parse the name. If COMPLETION_MODE is true, then tolerate a >> + non-existing or unbalanced parameter list. */ >> + > > Shouldn't this comment be in cp-support.h? Fixed. > >> +gdb::unique_xmalloc_ptr >> +cp_remove_params_if_any (const char *qualified, bool completion_mode) >> +{ >> + /* Trying to remove parameters from the empty string fails. If >> + we're completing / matching everything, avoid returning NULL > [snip] >> + /* Shouldn't this parse? Looks like a bug in >> + cp_demangled_name_to_comp. */ > > Is there a bugzilla (or some other tracking) for this? Indeed. I've filed now and added a reference here. > >> +#if 0 >> + CHECK ("A::foo::func(int)", >> + "A::foo::func"); >> +#else >> + CHECK_INCOMPL ("A::foo::func(int)", >> + "A::foo"); >> +#endif >> + >> + CHECK_INCOMPL ("A::foo> + "A::foo"); >> + >> +#undef CHECK >> +#undef CHECK_INCOMPL >> +} >> + >> +} // namespace selftests >> + >> +#endif /* GDB_SELF_CHECK */ >> + >> /* Don't allow just "maintenance cplus". */ >> >> static void >> diff --git a/gdb/cp-support.h b/gdb/cp-support.h >> index dd42415..1cef3f7 100644 >> --- a/gdb/cp-support.h >> +++ b/gdb/cp-support.h >> @@ -97,6 +97,9 @@ extern char *cp_func_name (const char *full_name); >> >> extern gdb::unique_xmalloc_ptr cp_remove_params (const char *qualified); >> >> +extern gdb::unique_xmalloc_ptr cp_remove_params_if_any >> + (const char *qualified, bool completion_mode); > > [Add comment from cp-support.c?] Fixed. > >> + >> extern struct symbol **make_symbol_overload_list (const char *, >> const char *); >> >> diff --git a/gdb/symtab.h b/gdb/symtab.h >> index 4c8b1f6..a452804 100644 >> --- a/gdb/symtab.h >> +++ b/gdb/symtab.h >> @@ -176,6 +178,16 @@ class lookup_name_info final >> symbol_name_match_type match_type () const { return m_match_type; } >> bool completion_mode () const { return m_completion_mode; } >> const std::string &name () const { return m_name; } >> + const bool ignore_parameters () const { return m_ignore_parameters; } >> + >> + /* Return a version of this lookup name that is usable with >> + comparisons against symbols have have no parameter info, such as > ^^^^^^^^^ > typo Fixed. > >> + psymbols and GDB index symbols. */ >> + lookup_name_info make_ignore_params () const >> + { >> + return lookup_name_info (m_name, m_match_type, m_completion_mode, >> + true /* ignore params */); >> + } >> >> /* Get the search name hash for searches in language LANG. */ >> unsigned int search_name_hash (language lang) const > Here's what I pushed. From c62446b12b32ce57d2b40cdb0c1baa7fc1677d82 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Wed, 8 Nov 2017 14:49:10 +0000 Subject: [PATCH] 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 * 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. --- gdb/ChangeLog | 42 ++++ gdb/Makefile.in | 2 + gdb/cp-support.c | 187 +++++++++++++++++- gdb/cp-support.h | 8 + gdb/dwarf2read.c | 300 ++++++++++++++++++++++++++++- gdb/psymtab.c | 4 +- gdb/symtab.c | 65 ++----- gdb/symtab.h | 15 +- gdb/unittests/lookup_name_info-selftests.c | 111 +++++++++++ 9 files changed, 675 insertions(+), 59 deletions(-) create mode 100644 gdb/unittests/lookup_name_info-selftests.c diff --git a/gdb/ChangeLog b/gdb/ChangeLog index 9776cf2..8abe455 100644 --- a/gdb/ChangeLog +++ b/gdb/ChangeLog @@ -1,5 +1,47 @@ 2017-11-08 Pedro Alves + * 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. + +2017-11-08 Pedro Alves + * dwarf2read.c (dw2_expand_marked_cus) (dw2_expand_symtabs_matching_symbol): Remove forward declarations. (dw2_expand_symtabs_matching): Move further below. diff --git a/gdb/Makefile.in b/gdb/Makefile.in index 6fe9b38..5e01816 100644 --- a/gdb/Makefile.in +++ b/gdb/Makefile.in @@ -530,6 +530,7 @@ SUBDIR_UNITTESTS_SRCS = \ unittests/common-utils-selftests.c \ unittests/environ-selftests.c \ unittests/function-view-selftests.c \ + unittests/lookup_name_info-selftests.c \ unittests/memrange-selftests.c \ unittests/offset-type-selftests.c \ unittests/optional-selftests.c \ @@ -542,6 +543,7 @@ SUBDIR_UNITTESTS_OBS = \ common-utils-selftests.o \ environ-selftests.o \ function-view-selftests.o \ + lookup_name_info-selftests.o \ memrange-selftests.o \ offset-type-selftests.o \ optional-selftests.o \ diff --git a/gdb/cp-support.c b/gdb/cp-support.c index defe509..1cab69b 100644 --- a/gdb/cp-support.c +++ b/gdb/cp-support.c @@ -36,6 +36,7 @@ #include #include "gdb_setjmp.h" #include "safe-ctype.h" +#include "selftest.h" #define d_left(dc) (dc)->u.s_binary.left #define d_right(dc) (dc)->u.s_binary.right @@ -830,12 +831,14 @@ cp_func_name (const char *full_name) return ret.release (); } -/* DEMANGLED_NAME is the name of a function, including parameters and - (optionally) a return type. Return the name of the function without - parameters or return type, or NULL if we can not parse the name. */ +/* Helper for cp_remove_params. DEMANGLED_NAME is the name of a + function, including parameters and (optionally) a return type. + Return the name of the function without parameters or return type, + or NULL if we can not parse the name. If REQUIRE_PARAMS is false, + then tolerate a non-existing or unbalanced parameter list. */ -gdb::unique_xmalloc_ptr -cp_remove_params (const char *demangled_name) +static gdb::unique_xmalloc_ptr +cp_remove_params_1 (const char *demangled_name, bool require_params) { bool done = false; struct demangle_component *ret_comp; @@ -871,10 +874,56 @@ cp_remove_params (const char *demangled_name) /* What we have now should be a function. Return its name. */ if (ret_comp->type == DEMANGLE_COMPONENT_TYPED_NAME) ret = cp_comp_to_string (d_left (ret_comp), 10); + else if (!require_params + && (ret_comp->type == DEMANGLE_COMPONENT_NAME + || ret_comp->type == DEMANGLE_COMPONENT_QUAL_NAME + || ret_comp->type == DEMANGLE_COMPONENT_TEMPLATE)) + ret = cp_comp_to_string (ret_comp, 10); return ret; } +/* DEMANGLED_NAME is the name of a function, including parameters and + (optionally) a return type. Return the name of the function + without parameters or return type, or NULL if we can not parse the + name. */ + +gdb::unique_xmalloc_ptr +cp_remove_params (const char *demangled_name) +{ + return cp_remove_params_1 (demangled_name, true); +} + +/* See cp-support.h. */ + +gdb::unique_xmalloc_ptr +cp_remove_params_if_any (const char *demangled_name, bool completion_mode) +{ + /* Trying to remove parameters from the empty string fails. If + we're completing / matching everything, avoid returning NULL + which would make callers interpret the result as an error. */ + if (demangled_name[0] == '\0' && completion_mode) + return gdb::unique_xmalloc_ptr (xstrdup ("")); + + gdb::unique_xmalloc_ptr without_params + = cp_remove_params_1 (demangled_name, false); + + if (without_params == NULL && completion_mode) + { + std::string copy = demangled_name; + + while (!copy.empty ()) + { + copy.pop_back (); + without_params = cp_remove_params_1 (copy.c_str (), false); + if (without_params != NULL) + break; + } + } + + return without_params; +} + /* Here are some random pieces of trivia to keep in mind while trying to take apart demangled names: @@ -1600,6 +1649,129 @@ cp_get_symbol_name_matcher (const lookup_name_info &lookup_name) return cp_fq_symbol_name_matches; } +#if GDB_SELF_TEST + +namespace selftests { + +/* If non-NULL, return STR wrapped in quotes. Otherwise, return a + "" string (with no quotes). */ + +static std::string +quote (const char *str) +{ + if (str != NULL) + return std::string (1, '\"') + str + '\"'; + else + return ""; +} + +/* Check that removing parameter info out of NAME produces EXPECTED. + COMPLETION_MODE indicates whether we're testing normal and + completion mode. FILE and LINE are used to provide better test + location information in case ithe check fails. */ + +static void +check_remove_params (const char *file, int line, + const char *name, const char *expected, + bool completion_mode) +{ + gdb::unique_xmalloc_ptr result + = cp_remove_params_if_any (name, completion_mode); + + if ((expected == NULL) != (result == NULL) + || (expected != NULL + && strcmp (result.get (), expected) != 0)) + { + error (_("%s:%d: make-paramless self-test failed: (completion=%d) " + "\"%s\" -> %s, expected %s"), + file, line, completion_mode, name, + quote (result.get ()).c_str (), quote (expected).c_str ()); + } +} + +/* Entry point for cp_remove_params unit tests. */ + +static void +test_cp_remove_params () +{ + /* Check that removing parameter info out of NAME produces EXPECTED. + Checks both normal and completion modes. */ +#define CHECK(NAME, EXPECTED) \ + do \ + { \ + check_remove_params (__FILE__, __LINE__, NAME, EXPECTED, false); \ + check_remove_params (__FILE__, __LINE__, NAME, EXPECTED, true); \ + } \ + while (0) + + /* Similar, but used when NAME is incomplete -- i.e., is has + unbalanced parentheses. In this case, looking for the exact name + should fail / return empty. */ +#define CHECK_INCOMPL(NAME, EXPECTED) \ + do \ + { \ + check_remove_params (__FILE__, __LINE__, NAME, NULL, false); \ + check_remove_params (__FILE__, __LINE__, NAME, EXPECTED, true); \ + } \ + while (0) + + CHECK ("function()", "function"); + CHECK_INCOMPL ("function(", "function"); + CHECK ("function() const", "function"); + + CHECK ("(anonymous namespace)::A::B::C", + "(anonymous namespace)::A::B::C"); + + CHECK ("A::(anonymous namespace)", + "A::(anonymous namespace)"); + + CHECK_INCOMPL ("A::(anonymou", "A"); + + CHECK ("A::foo()", + "A::foo"); + + CHECK_INCOMPL ("A::foo(", + "A::foo"); + + CHECK ("A::foo<(anonymous namespace)::B>::func(int)", + "A::foo<(anonymous namespace)::B>::func"); + + CHECK_INCOMPL ("A::foo<(anonymous namespace)::B>::func(in", + "A::foo<(anonymous namespace)::B>::func"); + + CHECK_INCOMPL ("A::foo<(anonymous namespace)::B>::", + "A::foo<(anonymous namespace)::B>"); + + CHECK_INCOMPL ("A::foo<(anonymous namespace)::B>:", + "A::foo<(anonymous namespace)::B>"); + + CHECK ("A::foo<(anonymous namespace)::B>", + "A::foo<(anonymous namespace)::B>"); + + CHECK_INCOMPL ("A::foo<(anonymous namespace)::B", + "A::foo"); + + /* Shouldn't this parse? Looks like a bug in + cp_demangled_name_to_comp. See PR c++/22411. */ +#if 0 + CHECK ("A::foo::func(int)", + "A::foo::func"); +#else + CHECK_INCOMPL ("A::foo::func(int)", + "A::foo"); +#endif + + CHECK_INCOMPL ("A::foo cp_remove_params (const char *demanged_name); +/* DEMANGLED_NAME is the name of a function, (optionally) including + parameters and (optionally) a return type. Return the name of the + function without parameters or return type, or NULL if we can not + parse the name. If COMPLETION_MODE is true, then tolerate a + non-existing or unbalanced parameter list. */ +extern gdb::unique_xmalloc_ptr cp_remove_params_if_any + (const char *demangled_name, bool completion_mode); + extern struct symbol **make_symbol_overload_list (const char *, const char *); 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 #include #include +#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 symbol_matcher, enum search_domain kind, gdb::function_view 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 + 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 m_symbol_table; + auto_obstack m_constant_pool; +}; + +/* Convenience function that converts a NULL pointer to a "" + string, to pass to print routines. */ + +static const char * +string_or_null (const char *str) +{ + return str != NULL ? str : ""; +} + +/* 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 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", + "ns::foo", + "ns::foo", + + /* 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"; + 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"; + /* 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"; + 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 } diff --git a/gdb/psymtab.c b/gdb/psymtab.c index d7881d2..29d40dc 100644 --- a/gdb/psymtab.c +++ b/gdb/psymtab.c @@ -1390,13 +1390,15 @@ static void psym_expand_symtabs_matching (struct objfile *objfile, gdb::function_view file_matcher, - const lookup_name_info &lookup_name, + const lookup_name_info &lookup_name_in, gdb::function_view symbol_matcher, gdb::function_view expansion_notify, enum search_domain domain) { struct partial_symtab *ps; + lookup_name_info lookup_name = lookup_name_in.make_ignore_params (); + /* Clear the search flags. */ ALL_OBJFILE_PSYMTABS_REQUIRED (objfile, ps) { diff --git a/gdb/symtab.c b/gdb/symtab.c index aecee8f..2d09f94 100644 --- a/gdb/symtab.c +++ b/gdb/symtab.c @@ -1766,6 +1766,20 @@ demangle_for_lookup_info::demangle_for_lookup_info { demangle_result_storage storage; + if (lookup_name.ignore_parameters () && lang == language_cplus) + { + gdb::unique_xmalloc_ptr without_params + = cp_remove_params_if_any (lookup_name.name ().c_str (), + lookup_name.completion_mode ()); + + if (without_params != NULL) + { + m_demangled_name = demangle_for_lookup (without_params.get (), + lang, storage); + return; + } + } + m_demangled_name = demangle_for_lookup (lookup_name.name ().c_str (), lang, storage); } @@ -4619,20 +4633,11 @@ rbreak_command (const char *regexp, int from_tty) } -/* Evaluate if NAME matches SYM_TEXT and SYM_TEXT_LEN. - - Either sym_text[sym_text_len] != '(' and then we search for any - symbol starting with SYM_TEXT text. - - Otherwise sym_text[sym_text_len] == '(' and then we require symbol name to - be terminated at that point. Partial symbol tables do not have parameters - information. */ +/* Evaluate if SYMNAME matches LOOKUP_NAME. */ static int -compare_symbol_name (const char *name, - language symbol_language, +compare_symbol_name (const char *symbol_name, language symbol_language, const lookup_name_info &lookup_name, - const char *sym_text, int sym_text_len, completion_match_result &match_res) { const language_defn *lang; @@ -4654,23 +4659,7 @@ compare_symbol_name (const char *name, symbol_name_matcher_ftype *name_match = language_get_symbol_name_matcher (lang, lookup_name); - /* Clip symbols that cannot match. */ - if (!name_match (name, lookup_name, &match_res.match)) - return 0; - - if (sym_text[sym_text_len] == '(') - { - /* User searches for `name(someth...'. Require NAME to be terminated. - Normally psymtabs and gdbindex have no parameter types so '\0' will be - present but accept even parameters presence. In this case this - function is in fact strcmp_iw but whitespace skipping is not supported - for tab completion. */ - - if (name[sym_text_len] != '\0' && name[sym_text_len] != '(') - return 0; - } - - return 1; + return name_match (symbol_name, lookup_name, &match_res.match); } /* See symtab.h. */ @@ -4687,10 +4676,7 @@ completion_list_add_name (completion_tracker &tracker, = tracker.reset_completion_match_result (); /* Clip symbols that cannot match. */ - if (!compare_symbol_name (symname, symbol_language, - lookup_name, - sym_text, sym_text_len, - match_res)) + if (!compare_symbol_name (symname, symbol_language, lookup_name, match_res)) return; /* Refresh SYMNAME from the match string. It's potentially @@ -5014,21 +5000,6 @@ default_collect_symbol_completion_matches_break_on sym_text_len = strlen (sym_text); - /* Prepare SYM_TEXT_LEN for compare_symbol_name. */ - - if (current_language->la_language == language_cplus - || current_language->la_language == language_fortran) - { - /* These languages may have parameters entered by user but they are never - present in the partial symbol tables. */ - - const char *cs = (const char *) memchr (sym_text, '(', sym_text_len); - - if (cs) - sym_text_len = cs - sym_text; - } - gdb_assert (sym_text[sym_text_len] == '\0' || sym_text[sym_text_len] == '('); - lookup_name_info lookup_name (std::string (sym_text, sym_text_len), name_match_type, true); diff --git a/gdb/symtab.h b/gdb/symtab.h index 5dfe953..8cd3496 100644 --- a/gdb/symtab.h +++ b/gdb/symtab.h @@ -167,9 +167,11 @@ class lookup_name_info final /* Create a new object. */ lookup_name_info (std::string name, symbol_name_match_type match_type, - bool completion_mode = false) + bool completion_mode = false, + bool ignore_parameters = false) : m_match_type (match_type), m_completion_mode (completion_mode), + m_ignore_parameters (ignore_parameters), m_name (std::move (name)) {} @@ -177,6 +179,16 @@ class lookup_name_info final symbol_name_match_type match_type () const { return m_match_type; } bool completion_mode () const { return m_completion_mode; } const std::string &name () const { return m_name; } + const bool ignore_parameters () const { return m_ignore_parameters; } + + /* Return a version of this lookup name that is usable with + comparisons against symbols have no parameter info, such as + psymbols and GDB index symbols. */ + lookup_name_info make_ignore_params () const + { + return lookup_name_info (m_name, m_match_type, m_completion_mode, + true /* ignore params */); + } /* Get the search name hash for searches in language LANG. */ unsigned int search_name_hash (language lang) const @@ -253,6 +265,7 @@ private: /* The lookup info as passed to the ctor. */ symbol_name_match_type m_match_type; bool m_completion_mode; + bool m_ignore_parameters; std::string m_name; /* Language-specific info. These fields are filled lazily the first diff --git a/gdb/unittests/lookup_name_info-selftests.c b/gdb/unittests/lookup_name_info-selftests.c new file mode 100644 index 0000000..b35a020 --- /dev/null +++ b/gdb/unittests/lookup_name_info-selftests.c @@ -0,0 +1,111 @@ +/* Self tests for lookup_name_info for GDB, the GNU debugger. + + Copyright (C) 2017 Free Software Foundation, Inc. + + This file is part of GDB. + + 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 . */ + +#include "defs.h" +#include "selftest.h" +#include "symtab.h" + +namespace selftests { +namespace lookup_name { + +/* Check that removing parameter info out of NAME produces EXPECTED. + COMPLETION_MODE indicates whether we're testing normal and + completion mode. FILE and LINE are used to provide better test + location information in case the check fails. */ + +static void +check_make_paramless (const char *file, int line, + enum language lang, + const char *name, const char *expected, + bool completion_mode) +{ + lookup_name_info lookup_name (name, symbol_name_match_type::FULL, + completion_mode, true /* ignore_parameters */); + const std::string &result = lookup_name.language_lookup_name (lang); + + if (result != expected) + { + error (_("%s:%d: make-paramless self-test failed: (completion=%d, lang=%d) " + "\"%s\" -> \"%s\", expected \"%s\""), + file, line, completion_mode, lang, name, + result.c_str (), expected); + } +} + +static void +run_tests () +{ + /* Helper for CHECK and CHECK_INCOMPL. */ +#define CHECK_1(INCOMPLETE, LANG, NAME, EXPECTED) \ + do \ + { \ + check_make_paramless (__FILE__, __LINE__, \ + LANG, NAME, \ + (INCOMPLETE) ? "" : (EXPECTED), false); \ + check_make_paramless (__FILE__, __LINE__, \ + LANG, NAME, EXPECTED, true); \ + } \ + while (0) + + /* Check that removing parameter info out of NAME produces EXPECTED. + Checks both normal and completion modes. */ +#define CHECK(LANG, NAME, EXPECTED) \ + CHECK_1(false, LANG, NAME, EXPECTED) + + /* Similar, but used when NAME is incomplete -- i.e., NAME has + unbalanced parentheses. In this case, looking for the exact name + should fail / return empty. */ +#define CHECK_INCOMPL(LANG, NAME, EXPECTED) \ + CHECK_1 (true, LANG, NAME, EXPECTED) + + /* None of these languages support function overloading like C++ + does, so building a nameless lookup name ends up being just the + same as any other lookup name. Mainly this serves as smoke test + that C++-specific code doesn't mess up with other languages that + use some other scoping character ('.' vs '::'). */ + CHECK (language_ada, "pck.ada_hello", "pck__ada_hello"); + CHECK (language_go, "pck.go_hello", "pck.go_hello"); + CHECK (language_fortran, "mod::func", "mod::func"); + + /* D does support function overloading similar to C++, but we're + missing support for stripping parameters. At least make sure the + input name is preserved unmodified. */ + CHECK (language_d, "pck.d_hello", "pck.d_hello"); + + /* Just a few basic tests to make sure + lookup_name_info::make_paramless is well integrated with + cp_remove_params_if_any. gdb/cp-support.c has comprehensive + testing of C++ specifics. */ + CHECK (language_cplus, "function()", "function"); + CHECK (language_cplus, "function() const", "function"); + CHECK (language_cplus, "A::B::C()", "A::B::C"); + CHECK (language_cplus, "A::B::C", "A::B::C"); + +#undef CHECK +#undef CHECK_INCOMPL +} + +}} // namespace selftests::lookup_name + +void +_initialize_lookup_name_info_selftests () +{ + selftests::register_test ("lookup_name_info", + selftests::lookup_name::run_tests); +}