From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-ot1-x344.google.com (mail-ot1-x344.google.com [IPv6:2607:f8b0:4864:20::344]) by sourceware.org (Postfix) with ESMTPS id 5E298385B831 for ; Mon, 23 Mar 2020 17:52:10 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 5E298385B831 Received: by mail-ot1-x344.google.com with SMTP id 22so8828432otf.0 for ; Mon, 23 Mar 2020 10:52:10 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=mCYAOOsLIwlJe0Gk9DbnO/JHUH2S5o5z7m6hu8TjHgU=; b=uSBTdhurfOCnwwIzOTu6TZGFOuTSMZK4ZdJswP1zHhKNCSOKazJTs0wrGW7FCox/F0 AO1QWHdtFdEVBsp5/lsUFhoGk/aqXAI/ABUNCszk5tOBBhAGdMiuqkLtLHCOal7O9FbD NpKuJJ4Yyk0GVCNt6emDIld5Z77ga8ZSOignGqE6yCAoJDNJXXMRq+pnyrEYb/h3FGd/ d7XGeVIHNYwOfrLgn+vdR3KX+hjzxGK9Z3Caj0RGpxy+vqYlLSFRz7jQR77rHjPR5DZo 0bCA4Ag3hzjdjgODJManXzZJyt3llcSdaX3V3kvfDfEdaW+wR3j5OhpbzKT/Y1Ktk8lA UVFw== X-Gm-Message-State: ANhLgQ1QcmFtdN/y5wrtZNvsbwVPXPGrnHH8Hu6g/JuQBnUNp9V6qcb5 NcMFHW77Lyg2R86ghiNlhf9KdPfGq8kSAsJRvy+/lqab X-Google-Smtp-Source: ADFU+vuA2KzKvpuOQXaSEMSB03/8KuF+bN7AkO0OyGmbeJDaZPfWh3iPwsB6evCz/MvaW2B8WtPLvHaoUdk3SsnxvqM= X-Received: by 2002:a9d:1a4:: with SMTP id e33mr18229650ote.343.1584985929216; Mon, 23 Mar 2020 10:52:09 -0700 (PDT) MIME-Version: 1.0 References: <20200320204935.19509-1-tromey@adacore.com> <20200320204935.19509-3-tromey@adacore.com> In-Reply-To: <20200320204935.19509-3-tromey@adacore.com> From: Christian Biesinger Date: Mon, 23 Mar 2020 12:51:32 -0500 Message-ID: Subject: Re: [PATCH 2/2] Avoid copying in lookup_name_info To: Tom Tromey Cc: gdb-patches Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-43.7 required=5.0 tests=BAYES_00, DKIMWL_WL_MED, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, ENV_AND_HDR_SPF_MATCH, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_FILL_THIS_FORM_SHORT, USER_IN_DEF_DKIM_WL, USER_IN_DEF_SPF_WL autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 23 Mar 2020 17:52:12 -0000 On Fri, Mar 20, 2020 at 3:49 PM Tom Tromey wrote: > > lookup_name_info always copies the name that is passed in. However, > normally a copy is not needed. This patch changes this class to avoid > copying. This required changing the "name" method to return something > else; I chose a gdb::string_view, to avoid excessive calls to strlen > in the code using the lookup_name_info. However, as this class does > not allow an arbitrary string_view, I've also added a c_str method > that guarantees a \0-terminated result -- a pedantic difference but > one that respects the string_view contract, IMO. > > gdb/ChangeLog > 2020-03-20 Tom Tromey > > * symtab.h (class lookup_name_info) : Change > "name" parameter to rvalue reference. Initialize m_name_holder. > : New overloads. > : Return gdb::string_view. > : New method. > : Update. > : Update. > : Return const char *. > : Rename from m_name. > : New member. > * symtab.c (demangle_for_lookup_info::demangle_for_lookup_info) > (demangle_for_lookup_info::demangle_for_lookup_info): Update. > (lookup_name_info::match_any): Update. > * psymtab.c (match_partial_symbol, lookup_partial_symbol): > Update. > * minsyms.c (linkage_name_str): Update. > * language.c (default_symbol_name_matcher): Update. > * dwarf2/read.c (mapped_index_base::find_name_components_bounds): > Update. > * ada-lang.c (ada_fold_name): Change parameter to string_view. > (ada_lookup_name_info::ada_lookup_name_info): Update. > (literal_symbol_name_matcher): Update. > --- > gdb/ChangeLog | 25 ++++++++++++++++++++ > gdb/ada-lang.c | 29 ++++++++++++----------- > gdb/dwarf2/read.c | 2 +- > gdb/language.c | 4 ++-- > gdb/minsyms.c | 2 +- > gdb/psymtab.c | 5 ++-- > gdb/symtab.c | 8 +++---- > gdb/symtab.h | 59 +++++++++++++++++++++++++++++++++++++---------- > 8 files changed, 98 insertions(+), 36 deletions(-) > > diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c > index 2822d40c8cd..c38ed1fca0e 100644 > --- a/gdb/ada-lang.c > +++ b/gdb/ada-lang.c > @@ -1016,17 +1016,17 @@ ada_encode (const char *decoded) > to next call. */ > > static char * > -ada_fold_name (const char *name) > +ada_fold_name (gdb::string_view name) > { > static char *fold_buffer = NULL; > static size_t fold_buffer_size = 0; > > - int len = strlen (name); > + int len = name.size (); > GROW_VECT (fold_buffer, fold_buffer_size, len + 1); > > if (name[0] == '\'') > { > - strncpy (fold_buffer, name + 1, len - 2); > + strncpy (fold_buffer, name.data () + 1, len - 2); > fold_buffer[len - 2] = '\000'; > } > else > @@ -13953,14 +13953,16 @@ do_exact_match (const char *symbol_search_name, > > ada_lookup_name_info::ada_lookup_name_info (const lookup_name_info &lookup_name) > { > - const std::string &user_name = lookup_name.name (); > + gdb::string_view user_name = lookup_name.name (); > > if (user_name[0] == '<') > { > if (user_name.back () == '>') > - m_encoded_name = user_name.substr (1, user_name.size () - 2); > + m_encoded_name > + = user_name.substr (1, user_name.size () - 2).to_string (); > else > - m_encoded_name = user_name.substr (1, user_name.size () - 1); > + m_encoded_name > + = user_name.substr (1, user_name.size () - 1).to_string (); > m_encoded_p = true; > m_verbatim_p = true; > m_wild_match_p = false; > @@ -13970,19 +13972,19 @@ ada_lookup_name_info::ada_lookup_name_info (const lookup_name_info &lookup_name) > { > m_verbatim_p = false; > > - m_encoded_p = user_name.find ("__") != std::string::npos; > + m_encoded_p = user_name.find ("__") != gdb::string_view::npos; > > if (!m_encoded_p) > { > - const char *folded = ada_fold_name (user_name.c_str ()); > + const char *folded = ada_fold_name (user_name); > const char *encoded = ada_encode_1 (folded, false); > if (encoded != NULL) > m_encoded_name = encoded; > else > - m_encoded_name = user_name; > + m_encoded_name = user_name.to_string (); > } > else > - m_encoded_name = user_name; > + m_encoded_name = user_name.to_string (); > > /* Handle the 'package Standard' special case. See description > of m_standard_p. */ > @@ -14029,11 +14031,12 @@ literal_symbol_name_matcher (const char *symbol_search_name, > const lookup_name_info &lookup_name, > completion_match_result *comp_match_res) > { > - const std::string &name = lookup_name.name (); > + gdb::string_view name_view = lookup_name.name (); > + const char *name = lookup_name.c_str (); > > int cmp = (lookup_name.completion_mode () > - ? strncmp (symbol_search_name, name.c_str (), name.size ()) > - : strcmp (symbol_search_name, name.c_str ())); > + ? strncmp (symbol_search_name, name, name_view.size ()) > + : strcmp (symbol_search_name, name)); You can just use symbol_search_name == name_view, since gdb::string_view has an overloaded operator==. That way you don't need to call c_str() here. > if (cmp == 0) > { > if (comp_match_res != NULL) > diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c > index 0e879e08a0c..cdccd04c54a 100644 > --- a/gdb/dwarf2/read.c > +++ b/gdb/dwarf2/read.c > @@ -3774,7 +3774,7 @@ mapped_index_base::find_name_components_bounds > = this->name_components_casing == case_sensitive_on ? strcmp : strcasecmp; > > const char *lang_name > - = lookup_name_without_params.language_lookup_name (lang).c_str (); > + = lookup_name_without_params.language_lookup_name (lang); > > /* Comparison function object for lower_bound that matches against a > given symbol name. */ > diff --git a/gdb/language.c b/gdb/language.c > index 454c6dc45a7..c13fd1a406a 100644 > --- a/gdb/language.c > +++ b/gdb/language.c > @@ -699,14 +699,14 @@ default_symbol_name_matcher (const char *symbol_search_name, > const lookup_name_info &lookup_name, > completion_match_result *comp_match_res) > { > - const std::string &name = lookup_name.name (); > + gdb::string_view name = lookup_name.name (); > completion_match_for_lcd *match_for_lcd > = (comp_match_res != NULL ? &comp_match_res->match_for_lcd : NULL); > strncmp_iw_mode mode = (lookup_name.completion_mode () > ? strncmp_iw_mode::NORMAL > : strncmp_iw_mode::MATCH_PARAMS); > > - if (strncmp_iw_with_mode (symbol_search_name, name.c_str (), name.size (), > + if (strncmp_iw_with_mode (symbol_search_name, name.data (), name.size (), > mode, language_minimal, match_for_lcd) == 0) > { > if (comp_match_res != NULL) > diff --git a/gdb/minsyms.c b/gdb/minsyms.c > index e238355dc11..d2ac8172eea 100644 > --- a/gdb/minsyms.c > +++ b/gdb/minsyms.c > @@ -467,7 +467,7 @@ linkage_name_str (const lookup_name_info &lookup_name) > if (current_language->la_language == language_ada) > return lookup_name.ada ().lookup_name ().c_str (); > > - return lookup_name.name ().c_str (); > + return lookup_name.c_str (); > } > > /* See minsyms.h. */ > diff --git a/gdb/psymtab.c b/gdb/psymtab.c > index 34b9741e966..7ab861632eb 100644 > --- a/gdb/psymtab.c > +++ b/gdb/psymtab.c > @@ -612,8 +612,7 @@ match_partial_symbol (struct objfile *objfile, > gdb_assert (center < top); > > enum language lang = (*center)->ginfo.language (); > - const char *lang_ln > - = name.language_lookup_name (lang).c_str (); > + const char *lang_ln = name.language_lookup_name (lang); > > if (ordered_compare ((*center)->ginfo.search_name (), > lang_ln) >= 0) > @@ -692,7 +691,7 @@ lookup_partial_symbol (struct objfile *objfile, > internal_error (__FILE__, __LINE__, > _("failed internal consistency check")); > if (strcmp_iw_ordered ((*center)->ginfo.search_name (), > - lookup_name.name ().c_str ()) >= 0) > + lookup_name.c_str ()) >= 0) > { > top = center; > } > diff --git a/gdb/symtab.c b/gdb/symtab.c > index f300d759e03..680280105f5 100644 > --- a/gdb/symtab.c > +++ b/gdb/symtab.c > @@ -1790,7 +1790,7 @@ demangle_for_lookup_info::demangle_for_lookup_info > if (lookup_name.ignore_parameters () && lang == language_cplus) > { > gdb::unique_xmalloc_ptr without_params > - = cp_remove_params_if_any (lookup_name.name ().c_str (), > + = cp_remove_params_if_any (lookup_name.c_str (), > lookup_name.completion_mode ()); > > if (without_params != NULL) > @@ -1803,9 +1803,9 @@ demangle_for_lookup_info::demangle_for_lookup_info > } > > if (lookup_name.match_type () == symbol_name_match_type::SEARCH_NAME) > - m_demangled_name = lookup_name.name (); > + m_demangled_name = lookup_name.c_str (); > else > - m_demangled_name = demangle_for_lookup (lookup_name.name ().c_str (), > + m_demangled_name = demangle_for_lookup (lookup_name.c_str (), > lang, storage); > } > > @@ -1816,7 +1816,7 @@ lookup_name_info::match_any () > { > /* Lookup any symbol that "" would complete. I.e., this matches all > symbol names. */ > - static const lookup_name_info lookup_name ({}, symbol_name_match_type::FULL, > + static const lookup_name_info lookup_name ("", symbol_name_match_type::FULL, > true); > > return lookup_name; > diff --git a/gdb/symtab.h b/gdb/symtab.h > index 771b5ec5bf7..c80acc3a68e 100644 > --- a/gdb/symtab.h > +++ b/gdb/symtab.h > @@ -186,28 +186,62 @@ class lookup_name_info final > { > public: > /* Create a new object. */ > - lookup_name_info (std::string name, > + lookup_name_info (std::string &&name, > symbol_name_match_type match_type, > 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)) > + m_name_holder (std::move (name)), > + m_name (m_name_holder.c_str ()) > + {} > + > + /* This overload requires that NAME have a lifetime at least as long > + as the lifetime of this object. */ > + lookup_name_info (const std::string &name, > + symbol_name_match_type match_type, > + 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 (name) > + {} > + > + /* This overload requires that NAME have a lifetime at least as long > + as the lifetime of this object. */ > + lookup_name_info (const char *name, > + symbol_name_match_type match_type, > + 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 (name) > {} > > /* Getters. See description of each corresponding field. */ > 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; } > + gdb::string_view name () const { return m_name; } > const bool ignore_parameters () const { return m_ignore_parameters; } > > + /* Like the "name" method but guarantees that the returned string is > + \0-terminated. */ > + const char *c_str () const > + { > + /* Actually this is always guaranteed due to how the class is > + constructed. */ > + return m_name.data (); > + } > + > /* 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, > + return lookup_name_info (c_str (), m_match_type, m_completion_mode, > true /* ignore params */); > } > > @@ -218,27 +252,27 @@ class lookup_name_info final > if (!m_demangled_hashes_p[lang]) > { > m_demangled_hashes[lang] > - = ::search_name_hash (lang, language_lookup_name (lang).c_str ()); > + = ::search_name_hash (lang, language_lookup_name (lang)); > m_demangled_hashes_p[lang] = true; > } > return m_demangled_hashes[lang]; > } > > /* Get the search name for searches in language LANG. */ > - const std::string &language_lookup_name (language lang) const > + const char *language_lookup_name (language lang) const > { > switch (lang) > { > case language_ada: > - return ada ().lookup_name (); > + return ada ().lookup_name ().c_str (); > case language_cplus: > - return cplus ().lookup_name (); > + return cplus ().lookup_name ().c_str (); > case language_d: > - return d ().lookup_name (); > + return d ().lookup_name ().c_str (); > case language_go: > - return go ().lookup_name (); > + return go ().lookup_name ().c_str (); > default: > - return m_name; > + return m_name.data (); > } > } > > @@ -287,7 +321,8 @@ class lookup_name_info final > symbol_name_match_type m_match_type; > bool m_completion_mode; > bool m_ignore_parameters; > - std::string m_name; > + std::string m_name_holder; > + gdb::string_view m_name; > > /* Language-specific info. These fields are filled lazily the first > time a lookup is done in the corresponding language. They're > -- > 2.21.1 >