Re: [PATCH v1 08/16] corpus: make get_(undefined_)?_(var|fun)_symbols use the new symtab

[Giuliano Procida <gprocida@google.com>]

Hi.

On Fri, 19 Jun 2020 at 22:43, Matthias Maennich <maennich@google.com> wrote:
>
> Make the corresponding members an implementation detail of corpus::priv.
> They get computed based on the new symtab whenever they are accessed
> first with an atomic instantiation. That simplifies the implementation
> and homogenizes the access to functions and variables. Sorting does not
> need to be done as the symtab already gives a guarantee for that.
>
> Due to improved alias detection in the new symtab reader, ensure we only
> write symbol aliases to ksymtab symbols if having a ksymtab main symbol.
>
> Test data needed to be adjusted as the new symtab reader is stricter in
> regards to symbols listed in ksymtab. I.e. init_module is not an
> exported symbol in the ksymtab of a kernel module.
>
>         * src/abg-corpus-priv.h (corpus::priv::sorted_var_symbols): make
>           private, mutable and optional.
>           (corpus::sorted_undefined_var_symbols): Likewise.
>           (corpus::sorted_fun_symbols): Likewise.
>           (corpus::sorted_undefined_fun_symbols): Likewise.
>           (corpus::priv::get_sorted_fun_symbols): New method declaration.
>           (corpus::priv::get_sorted_undefined_fun_symbols): Likewise.
>           (corpus::priv::get_sorted_var_symbols): Likewise.
>           (corpus::priv::get_sorted_undefined_var_symbols): Likewise.
>         * src/abg-corpus.cc
>           (corpus::elf_symbol_comp_functor): Delete struct.
>           (corpus::priv::get_sorted_fun_symbols): New method implementation.
>           (corpus::priv::get_sorted_undefined_fun_symbols): Likewise.
>           (corpus::priv::get_sorted_var_symbols): Likewise.
>           (corpus::priv::get_sorted_undefined_var_symbols): Likewise.
>           (corpus::get_sorted_fun_symbols): Proxy call to corpus::priv.
>           (corpus::get_sorted_undefined_fun_symbols): Likewise.
>           (corpus::get_sorted_var_symbols): Likewise.
>           (corpus::get_sorted_undefined_var_symbols): Likewise.
>         * src/abg-writer.cc (write_elf_symbol_aliases): When emitting
>           aliases for a kernel symbol, ensure to only emit exported aliases.
>         * tests/data/test-read-dwarf/PR25007-sdhci.ko.abi: update test
>           data.
>

Reviewed-by: Giuliano Procida <gprocida@google.com>

> Signed-off-by: Matthias Maennich <maennich@google.com>
> ---
>  src/abg-corpus-priv.h                         |  21 +-
>  src/abg-corpus.cc                             | 242 +++++++-----------
>  src/abg-writer.cc                             |  38 ++-
>  .../data/test-read-dwarf/PR25007-sdhci.ko.abi |   3 -
>  4 files changed, 135 insertions(+), 169 deletions(-)
>
> diff --git a/src/abg-corpus-priv.h b/src/abg-corpus-priv.h
> index 5c1e915ad2f3..ad96f260aa89 100644
> --- a/src/abg-corpus-priv.h
> +++ b/src/abg-corpus-priv.h
> @@ -699,13 +699,9 @@ struct corpus::priv
>    vector<var_decl*>                            vars;
>    string_elf_symbols_map_sptr                  var_symbol_map;
>    string_elf_symbols_map_sptr                  undefined_var_symbol_map;
> -  elf_symbols                                  sorted_var_symbols;
> -  elf_symbols                                  sorted_undefined_var_symbols;
>    symtab_reader::symtab_sptr                   symtab_;
>    string_elf_symbols_map_sptr                  fun_symbol_map;
>    string_elf_symbols_map_sptr                  undefined_fun_symbol_map;
> -  elf_symbols                                  sorted_fun_symbols;
> -  elf_symbols                                  sorted_undefined_fun_symbols;
>    elf_symbols                                  unrefed_fun_symbols;
>    elf_symbols                                  unrefed_var_symbols;
>    // The type maps contained in this data member are populated if the
> @@ -727,6 +723,11 @@ struct corpus::priv
>  private:
>    priv();
>
> +  mutable abg_compat::optional<elf_symbols> sorted_var_symbols;
> +  mutable abg_compat::optional<elf_symbols> sorted_undefined_var_symbols;
> +  mutable abg_compat::optional<elf_symbols> sorted_fun_symbols;
> +  mutable abg_compat::optional<elf_symbols> sorted_undefined_fun_symbols;
> +
>  public:
>    priv(const string &  p,
>         environment*    e)
> @@ -746,6 +747,18 @@ public:
>    const type_maps&
>    get_types() const;
>
> +  const elf_symbols&
> +  get_sorted_fun_symbols() const;
> +
> +  const elf_symbols&
> +  get_sorted_undefined_fun_symbols() const;
> +
> +  const elf_symbols&
> +  get_sorted_var_symbols() const;
> +
> +  const elf_symbols&
> +  get_sorted_undefined_var_symbols() const;
> +
>    unordered_set<interned_string, hash_interned_string>*
>    get_public_types_pretty_representations();
>
> diff --git a/src/abg-corpus.cc b/src/abg-corpus.cc
> index 94702047dd82..6d4bedfd57b7 100644
> --- a/src/abg-corpus.cc
> +++ b/src/abg-corpus.cc
> @@ -453,6 +453,88 @@ const type_maps&
>  corpus::priv::get_types() const
>  {return types_;}
>
> +/// Return a sorted vector of function symbols for this corpus.
> +///
> +/// Note that the first time this function is called, the symbols are
> +/// sorted and cached.  Subsequent invocations of this function return
> +/// the cached vector that was built previously.
> +///
> +/// @return the sorted list of function symbols.
> +const elf_symbols&
> +corpus::priv::get_sorted_fun_symbols() const
> +{
> +  if (!sorted_fun_symbols)
> +    {
> +      const symtab_reader::symtab_filter filter =
> +         symtab_->make_filter().functions();
> +
> +      sorted_fun_symbols = elf_symbols(symtab_->begin(filter), symtab_->end());
> +    }
> +  return *sorted_fun_symbols;
> +}
> +
> +/// Getter for a sorted vector of the function symbols undefined in
> +/// this corpus.
> +///
> +/// @return a vector of the function symbols undefined in this corpus,
> +/// sorted by name and then version.
> +const elf_symbols&
> +corpus::priv::get_sorted_undefined_fun_symbols() const
> +{
> +  if (!sorted_undefined_fun_symbols)
> +    {
> +      const symtab_reader::symtab_filter filter = symtab_->make_filter()
> +                                                     .functions()
> +                                                     .undefined_symbols()
> +                                                     .public_symbols(false);
> +
> +      sorted_undefined_fun_symbols =
> +         elf_symbols(symtab_->begin(filter), symtab_->end());
> +    }
> +  return *sorted_undefined_fun_symbols;
> +}
> +
> +/// Getter for the sorted vector of variable symbols for this corpus.
> +///
> +/// Note that the first time this function is called, it computes the
> +/// sorted vector, caches the result and returns it.  Subsequent
> +/// invocations of this function just return the cached vector.
> +///
> +/// @return the sorted vector of variable symbols for this corpus.
> +const elf_symbols&
> +corpus::priv::get_sorted_var_symbols() const
> +{
> +  if (!sorted_var_symbols)
> +    {
> +      const symtab_reader::symtab_filter filter =
> +         symtab_->make_filter().variables();
> +
> +      sorted_var_symbols = elf_symbols(symtab_->begin(filter), symtab_->end());
> +    }
> +  return *sorted_var_symbols;
> +}
> +
> +/// Getter for a sorted vector of the variable symbols undefined in
> +/// this corpus.
> +///
> +/// @return a vector of the variable symbols undefined in this corpus,
> +/// sorted by name and then version.
> +const elf_symbols&
> +corpus::priv::get_sorted_undefined_var_symbols() const
> +{
> +  if (!sorted_undefined_var_symbols)
> +    {
> +      const symtab_reader::symtab_filter filter = symtab_->make_filter()
> +                                                     .variables()
> +                                                     .undefined_symbols()
> +                                                     .public_symbols(false);
> +
> +      sorted_undefined_var_symbols =
> +         elf_symbols(symtab_->begin(filter), symtab_->end());
> +    }
> +  return *sorted_undefined_var_symbols;
> +}
> +
>  /// Getter of the set of pretty representation of types that are
>  /// reachable from public interfaces (global functions and variables).
>  ///
> @@ -988,104 +1070,21 @@ const string_elf_symbols_map_type&
>  corpus::get_undefined_fun_symbol_map() const
>  {return *get_undefined_fun_symbol_map_sptr();}
>
> -/// Functor to sort instances of @ref elf_symbol.
> -struct elf_symbol_comp_functor
> -{
> -
> -  /// Return true if the first argument is less than the second one.
> -  ///
> -  /// @param l the first parameter to consider.
> -  ///
> -  /// @param r the second parameter to consider.
> -  ///
> -  /// @return true if @p l is less than @p r
> -  bool
> -  operator()(elf_symbol& l, elf_symbol& r)
> -  {return (l.get_id_string() < r.get_id_string());}
> -
> -  /// Return true if the first argument is less than the second one.
> -  ///
> -  /// @param l the first parameter to consider.
> -  ///
> -  /// @param r the second parameter to consider.
> -  ///
> -  /// @return true if @p l is less than @p r
> -  bool
> -  operator()(elf_symbol* l, elf_symbol* r)
> -  {return operator()(*l, *r);}
> -
> -  /// Return true if the first argument is less than the second one.
> -  ///
> -  /// @param l the first parameter to consider.
> -  ///
> -  /// @param r the second parameter to consider.
> -  ///
> -  /// @return true if @p l is less than @p r
> -  bool
> -  operator()(elf_symbol_sptr l, elf_symbol_sptr r)
> -  {return operator()(*l, *r);}
> -}; // end struct elf_symbol_comp_functor
> -
> -/// Return a sorted vector of function symbols for this corpus.
> -///
> -/// Note that the first time this function is called, the symbols are
> -/// sorted and cached.  Subsequent invocations of this function return
> -/// the cached vector that was built previously.
> -///
> -/// @return the sorted list of function symbols.
>  const elf_symbols&
>  corpus::get_sorted_fun_symbols() const
> -{
> -  if (priv_->sorted_fun_symbols.empty()
> -      && !get_fun_symbol_map().empty())
> -    {
> -      priv_->sorted_fun_symbols.reserve(get_fun_symbol_map().size());
> -      for (string_elf_symbols_map_type::const_iterator i =
> -            get_fun_symbol_map().begin();
> -          i != get_fun_symbol_map().end();
> -          ++i)
> -       for (elf_symbols::const_iterator s = i->second.begin();
> -            s != i->second.end();
> -            ++s)
> -         priv_->sorted_fun_symbols.push_back(*s);
> +{ return priv_->get_sorted_fun_symbols(); }
>
> -      elf_symbol_comp_functor comp;
> -      std::sort(priv_->sorted_fun_symbols.begin(),
> -               priv_->sorted_fun_symbols.end(),
> -               comp);
> -    }
> -  return priv_->sorted_fun_symbols;
> -}
> -
> -/// Getter for a sorted vector of the function symbols undefined in
> -/// this corpus.
> -///
> -/// @return a vector of the function symbols undefined in this corpus,
> -/// sorted by name and then version.
>  const elf_symbols&
>  corpus::get_sorted_undefined_fun_symbols() const
> -{
> -  if (priv_->sorted_undefined_fun_symbols.empty()
> -      && !get_undefined_fun_symbol_map().empty())
> -    {
> -      priv_->sorted_undefined_fun_symbols.reserve
> -       (get_undefined_fun_symbol_map().size());
> -      for (string_elf_symbols_map_type::const_iterator i =
> -            get_undefined_fun_symbol_map().begin();
> -          i != get_undefined_fun_symbol_map().end();
> -          ++i)
> -       for (elf_symbols::const_iterator s = i->second.begin();
> -            s != i->second.end();
> -            ++s)
> -         priv_->sorted_undefined_fun_symbols.push_back(*s);
> +{ return priv_->get_sorted_undefined_fun_symbols(); }
>
> -      elf_symbol_comp_functor comp;
> -      std::sort(priv_->sorted_undefined_fun_symbols.begin(),
> -               priv_->sorted_undefined_fun_symbols.end(),
> -               comp);
> -    }
> -  return priv_->sorted_undefined_fun_symbols;
> -}
> +const elf_symbols&
> +corpus::get_sorted_var_symbols() const
> +{ return priv_->get_sorted_var_symbols(); }
> +
> +const elf_symbols&
> +corpus::get_sorted_undefined_var_symbols() const
> +{ return priv_->get_sorted_undefined_var_symbols(); }
>
>  /// Getter for the variable symbols map.
>  ///
> @@ -1125,65 +1124,6 @@ const string_elf_symbols_map_type&
>  corpus::get_undefined_var_symbol_map() const
>  {return *get_undefined_var_symbol_map_sptr();}
>
> -/// Getter for the sorted vector of variable symbols for this corpus.
> -///
> -/// Note that the first time this function is called, it computes the
> -/// sorted vector, caches the result and returns it.  Subsequent
> -/// invocations of this function just return the cached vector.
> -///
> -/// @return the sorted vector of variable symbols for this corpus.
> -const elf_symbols&
> -corpus::get_sorted_var_symbols() const
> -{
> -  if (priv_->sorted_var_symbols.empty()
> -      && !get_var_symbol_map().empty())
> -    {
> -      priv_->sorted_var_symbols.reserve(get_var_symbol_map().size());
> -      for (string_elf_symbols_map_type::const_iterator i =
> -            get_var_symbol_map().begin();
> -          i != get_var_symbol_map().end();
> -          ++i)
> -       for (elf_symbols::const_iterator s = i->second.begin();
> -            s != i->second.end(); ++s)
> -         priv_->sorted_var_symbols.push_back(*s);
> -
> -      elf_symbol_comp_functor comp;
> -      std::sort(priv_->sorted_var_symbols.begin(),
> -               priv_->sorted_var_symbols.end(),
> -               comp);
> -    }
> -  return priv_->sorted_var_symbols;
> -}
> -
> -/// Getter for a sorted vector of the variable symbols undefined in
> -/// this corpus.
> -///
> -/// @return a vector of the variable symbols undefined in this corpus,
> -/// sorted by name and then version.
> -const elf_symbols&
> -corpus::get_sorted_undefined_var_symbols() const
> -{
> -  if (priv_->sorted_undefined_var_symbols.empty()
> -      && !get_undefined_var_symbol_map().empty())
> -    {
> -      priv_->sorted_undefined_var_symbols.reserve
> -       (get_undefined_var_symbol_map().size());
> -      for (string_elf_symbols_map_type::const_iterator i =
> -            get_undefined_var_symbol_map().begin();
> -          i != get_undefined_var_symbol_map().end();
> -          ++i)
> -       for (elf_symbols::const_iterator s = i->second.begin();
> -            s != i->second.end(); ++s)
> -         priv_->sorted_undefined_var_symbols.push_back(*s);
> -
> -      elf_symbol_comp_functor comp;
> -      std::sort(priv_->sorted_undefined_var_symbols.begin(),
> -               priv_->sorted_undefined_var_symbols.end(),
> -               comp);
> -    }
> -  return priv_->sorted_undefined_var_symbols;
> -}
> -
>  /// Look in the function symbols map for a symbol with a given name.
>  ///
>  /// @param n the name of the symbol to look for.
> diff --git a/src/abg-writer.cc b/src/abg-writer.cc
> index ce0bae2d5cfd..c5be11b26072 100644
> --- a/src/abg-writer.cc
> +++ b/src/abg-writer.cc
> @@ -1693,26 +1693,42 @@ write_elf_symbol_visibility(elf_symbol::visibility v, ostream& o)
>  ///
>  /// @return true upon successful completion.
>  static bool
> -write_elf_symbol_aliases(const elf_symbol& sym, ostream& o)
> +write_elf_symbol_aliases(const elf_symbol& sym, ostream& out)
>  {
>    if (!sym.is_main_symbol() || !sym.has_aliases())
>      return false;
>
> -  bool emitted = false;
> -  o << " alias='";
> -  for (elf_symbol_sptr s = sym.get_next_alias();
> -       !s->is_main_symbol();
> +
> +  std::vector<std::string> aliases;
> +  for (elf_symbol_sptr s = sym.get_next_alias(); s && !s->is_main_symbol();
>         s = s->get_next_alias())
>      {
> -      if (s->get_next_alias()->is_main_symbol())
> -       o << s->get_id_string() << "'";
> -      else
> -       o << s->get_id_string() << ",";
> +      if (s->is_suppressed())
> +       continue;
>
> -      emitted = true;
> +      if (sym.is_in_ksymtab() != s->is_in_ksymtab())
> +       continue;
> +
> +      aliases.push_back(s->get_id_string());
>      }
>
> -  return emitted;
> +  if (!aliases.empty())
> +    {
> +      out << " alias='";
> +      std::string separator;
> +      for (std::vector<std::string>::const_iterator it = aliases.begin(),
> +                                                   end = aliases.end();
> +          it != end; ++it)
> +       {
if (it != aliases.begin()) // you can put begin in a variable as well,
if you like, but it's all compiled away anyway
  out << ',';
> +         out << separator << *it;
> +         separator = ",";
> +       }
> +
> +      out << "'";
> +      return true;
> +    }
> +
> +  return false;
>  }
>
>  /// Write an XML attribute for the reference to a symbol for the
> diff --git a/tests/data/test-read-dwarf/PR25007-sdhci.ko.abi b/tests/data/test-read-dwarf/PR25007-sdhci.ko.abi
> index 755ea6dc433e..d5af7183095f 100644
> --- a/tests/data/test-read-dwarf/PR25007-sdhci.ko.abi
> +++ b/tests/data/test-read-dwarf/PR25007-sdhci.ko.abi
> @@ -2,8 +2,6 @@
>    <elf-function-symbols>
>      <elf-symbol name='__sdhci_add_host' type='func-type' binding='global-binding' visibility='default-visibility' is-defined='yes'/>
>      <elf-symbol name='__sdhci_read_caps' type='func-type' binding='global-binding' visibility='default-visibility' is-defined='yes'/>
> -    <elf-symbol name='cleanup_module' type='func-type' binding='global-binding' visibility='default-visibility' is-defined='yes'/>
> -    <elf-symbol name='init_module' type='func-type' binding='global-binding' visibility='default-visibility' is-defined='yes'/>
>      <elf-symbol name='sdhci_add_host' type='func-type' binding='global-binding' visibility='default-visibility' is-defined='yes'/>
>      <elf-symbol name='sdhci_adma_write_desc' type='func-type' binding='global-binding' visibility='default-visibility' is-defined='yes'/>
>      <elf-symbol name='sdhci_alloc_host' type='func-type' binding='global-binding' visibility='default-visibility' is-defined='yes'/>
> @@ -40,7 +38,6 @@
>      <elf-symbol name='sdhci_suspend_host' type='func-type' binding='global-binding' visibility='default-visibility' is-defined='yes'/>
>    </elf-function-symbols>
>    <elf-variable-symbols>
> -    <elf-symbol name='__this_module' size='896' type='object-type' binding='global-binding' visibility='default-visibility' is-defined='yes'/>
>    </elf-variable-symbols>
>    <abi-instr version='1.0' address-size='64' path='/ws/android/kernel/aosp/common-mainline/common/drivers/mmc/host/sdhci.c' comp-dir-path='/ws/android/kernel/aosp/common-mainline/out/android-mainline/common' language='LANG_C89'>
>
> --
> 2.27.0.111.gc72c7da667-goog
>