Re: [PATCH 2/2] Avoid copying in lookup_name_info

[Christian Biesinger <cbiesinger@google.com>]

On Fri, Mar 20, 2020 at 3:49 PM Tom Tromey <tromey@adacore.com> 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  <tromey@adacore.com>
>
>         * symtab.h (class lookup_name_info) <lookup_name_info>: Change
>         "name" parameter to rvalue reference.  Initialize m_name_holder.
>         <lookup_name_info>: New overloads.
>         <name>: Return gdb::string_view.
>         <c_str>: New method.
>         <make_ignore_params>: Update.
>         <search_name_hash>: Update.
>         <language_lookup_name>: Return const char *.
>         <m_name_holder>: Rename from m_name.
>         <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<char> 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
>