[05/20] Refactor ELF symbol table reading by adding a new symtab reader

Message ID 20210127125853.886677-6-maennich@google.com
State Committed
Headers
Series Refactor (k)symtab reader |

Commit Message

Matthias Männich Jan. 27, 2021, 12:58 p.m. UTC
  Based on existing functionality, implement the reading of ELF symbol
tables as a separate component. This reduces the complexity of
abg-dwarf-reader's read_context by separating and delegating the
functionality. This also allows dedicated testing.

The new namespace symtab_reader contains a couple of new components that
work loosely coupled together. Together they allow for a consistent view
on a symbol table. With filter criteria those views can be restricted,
iterated and consistent lookup maps can be built on top of them. While
this implementation tries to address some shortcomings of the previous
model, it still provides the high level interfaces to the symbol table
contents through sorted iterating and name/address mapped access.

symtab_reader::symtab

  While the other classes in the same namespace are merely helpers, this
  is the main implementation of symtab reading and storage.
  Symtab objects are factory created to ensure a consistent construction
  and valid invariants. Thus a symtab will be loaded by either passing
  an ELF handle (when reading from binary) or by passing a set of
  function/variable symbol maps (when reading from XML).
  When constructed they are considered const and are not writable
  anymore. As such, all public methods are const.

  The load reuses the existing implementation for loading symtab
  sections, but since the new implementation does not distinguish
  between functions and variables, the code could be simplified. The
  support for ppc64 function entry addresses has been deferred to a
  later commit.

  Linux Kernel symbol tables are now directly loaded by name when
  encountering symbols prefixed with the __ksymtab_ as per convention.
  This has been tricky in the past due to various different binary
  layouts (relocations, position relative relocations, symbol
  namespaces, CFI indirections, differences between vmlinux and kernel
  modules). Thus the new implementation is much simpler and is less
  vulnerable to future ksymtab changes. As we are also not looking up
  the Kernel symbols by addresses, we could resolve shortcomings with
  symbol aliasing: Previously a symbol and its alias were
  indistinguishable as they are having the same symbol address. We could
  not identify the one that is actually exported via ksymtab.

  One major architectural difference of this implementation is that we
  do not early discard suppressed symbols. While we keep them out of the
  vector of exported symbols, we still make them available for lookup.
  That helps addressing issues when looking up a symbol by address (e.g.
  from the ksymtab read implementation) that is suppressed. That would
  fail in the existing implementation.

  Still, we intend to only instantiate each symbol once and pass around
  shared_ptr instances to refer to it from the vector as well as from
  the lookup maps.

  For reading, there are two access paths that serve the existing
  patterns:
	1) lookup_symbol: either via a name or an address
	2) filtered iteration with begin(), end()

  The former is used for direct access with a clue in hand (like a name
  or an address), the latter is used for iteration (e.g. when emitting
  the XML).

symtab_reader::symtab_iterator

  The symtab_iterator is an STL compatible iterator that is returned
  from begin() and end() of the symtab. It allows usual forward iterator
  operations and can optionally take a filter predicate to skip non
  matching elements.

symtab_reader::symtab_filter

  The symtab_filter serves as a predicate for the symtab_iterator by
  providing a matches(const elf_symbol_sptr&) function.  The predicate
  is built by ANDing together several conditions on attributes a symbol
  can have. The filter conditions are implemented in terms of
  std::optional<bool> members to allow a tristate: "needs to have the
  condition set", "must not have it set" and "don't care".

symtab_reader::filtered_symtab

  The filtered_symtab is a convenience zero cost abstraction that allows
  prepopulating the symtab_filter (call it a capture) such that begin()
  and end() are now accessible without the need to pass the filter
  again. Argumentless begin() and end() are a requirement for range-for
  loops and other STL based algorithms.

	* src/abg-symtab-reader.h (symtab_filter): New class.
	(symtab_iterator): Likewise.
	(symtab): Likewise.
	(filtered_symtab): Likewise.
	* src/abg-symtab-reader.cc (symtab_filter::matches): New.
	(symtab::make_filter): Likewise.
	(symtab::lookup_symbol): Likewise.
	(symbol_sort): Likewise.
	(symtab::load): Likewise.
	(symtab::load_): Likewise.

Reviewed-by: Giuliano Procida <gprocida@google.com>
Signed-off-by: Matthias Maennich <maennich@google.com>
---
 src/abg-symtab-reader.cc | 347 +++++++++++++++++++++++++++++++++++++++
 src/abg-symtab-reader.h  | 277 ++++++++++++++++++++++++++++++-
 2 files changed, 623 insertions(+), 1 deletion(-)
  

Comments

Dodji Seketeli March 12, 2021, 11:18 a.m. UTC | #1
Hello,

Matthias Maennich <maennich@google.com> a écrit:

> Based on existing functionality, implement the reading of ELF symbol
> tables as a separate component. This reduces the complexity of
> abg-dwarf-reader's read_context by separating and delegating the
> functionality. This also allows dedicated testing.
>
> The new namespace symtab_reader contains a couple of new components that
> work loosely coupled together. Together they allow for a consistent view
> on a symbol table. With filter criteria those views can be restricted,
> iterated and consistent lookup maps can be built on top of them. While
> this implementation tries to address some shortcomings of the previous
> model, it still provides the high level interfaces to the symbol table
> contents through sorted iterating and name/address mapped access.
>
> symtab_reader::symtab
>
>   While the other classes in the same namespace are merely helpers, this
>   is the main implementation of symtab reading and storage.
>   Symtab objects are factory created to ensure a consistent construction
>   and valid invariants. Thus a symtab will be loaded by either passing
>   an ELF handle (when reading from binary) or by passing a set of
>   function/variable symbol maps (when reading from XML).
>   When constructed they are considered const and are not writable
>   anymore. As such, all public methods are const.
>
>   The load reuses the existing implementation for loading symtab
>   sections, but since the new implementation does not distinguish
>   between functions and variables, the code could be simplified. The
>   support for ppc64 function entry addresses has been deferred to a
>   later commit.
>
>   Linux Kernel symbol tables are now directly loaded by name when
>   encountering symbols prefixed with the __ksymtab_ as per convention.

Whoah.  No more messing with __ksymtab sections then.  How cool is that!
:-)

>   This has been tricky in the past due to various different binary
>   layouts (relocations, position relative relocations, symbol
>   namespaces, CFI indirections, differences between vmlinux and kernel
>   modules). Thus the new implementation is much simpler and is less
>   vulnerable to future ksymtab changes.

Let's just hope the "__ksymtab_" prefix convention stays that, I guess
;-)

>   As we are also not looking up the Kernel symbols by addresses, we
>   could resolve shortcomings with symbol aliasing: Previously a symbol
>   and its alias were indistinguishable as they are having the same
>   symbol address. We could not identify the one that is actually
>   exported via ksymtab.

I see.

>   One major architectural difference of this implementation is that we
>   do not early discard suppressed symbols. While we keep them out of the
>   vector of exported symbols, we still make them available for lookup.
>   That helps addressing issues when looking up a symbol by address (e.g.
>   from the ksymtab read implementation) that is suppressed. That would
>   fail in the existing implementation.
>
>   Still, we intend to only instantiate each symbol once and pass around
>   shared_ptr instances to refer to it from the vector as well as from
>   the lookup maps.
>
>   For reading, there are two access paths that serve the existing
>   patterns:
> 	1) lookup_symbol: either via a name or an address
> 	2) filtered iteration with begin(), end()
>
>   The former is used for direct access with a clue in hand (like a name
>   or an address), the latter is used for iteration (e.g. when emitting
>   the XML).
>
> symtab_reader::symtab_iterator
>
>   The symtab_iterator is an STL compatible iterator that is returned
>   from begin() and end() of the symtab. It allows usual forward iterator
>   operations and can optionally take a filter predicate to skip non
>   matching elements.
>
> symtab_reader::symtab_filter
>
>   The symtab_filter serves as a predicate for the symtab_iterator by
>   providing a matches(const elf_symbol_sptr&) function.  The predicate
>   is built by ANDing together several conditions on attributes a symbol
>   can have. The filter conditions are implemented in terms of
>   std::optional<bool> members to allow a tristate: "needs to have the
>   condition set", "must not have it set" and "don't care".
>
> symtab_reader::filtered_symtab
>
>   The filtered_symtab is a convenience zero cost abstraction that allows
>   prepopulating the symtab_filter (call it a capture) such that begin()
>   and end() are now accessible without the need to pass the filter
>   again. Argumentless begin() and end() are a requirement for range-for
>   loops and other STL based algorithms.

Neat design.  I like it.  Thanks for making this is so "down to the
point" and yet with the a nice level of abstraction.


Now I've just picked some superficial nits below.

>
> 	* src/abg-symtab-reader.h (symtab_filter): New class.
> 	(symtab_iterator): Likewise.
> 	(symtab): Likewise.
> 	(filtered_symtab): Likewise.
> 	* src/abg-symtab-reader.cc (symtab_filter::matches): New.
> 	(symtab::make_filter): Likewise.
> 	(symtab::lookup_symbol): Likewise.
> 	(symbol_sort): Likewise.
> 	(symtab::load): Likewise.
> 	(symtab::load_): Likewise.
>
> Reviewed-by: Giuliano Procida <gprocida@google.com>
> Signed-off-by: Matthias Maennich <maennich@google.com>
> ---
>  src/abg-symtab-reader.cc | 347 +++++++++++++++++++++++++++++++++++++++
>  src/abg-symtab-reader.h  | 277 ++++++++++++++++++++++++++++++-
>  2 files changed, 623 insertions(+), 1 deletion(-)
>
> diff --git a/src/abg-symtab-reader.cc b/src/abg-symtab-reader.cc
> index a6c8ca0ef548..4576be2a0b42 100644
> --- a/src/abg-symtab-reader.cc
> +++ b/src/abg-symtab-reader.cc
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
>  // -*- Mode: C++ -*-
>  //
> +// Copyright (C) 2013-2020 Red Hat, Inc.
>  // Copyright (C) 2020 Google, Inc.
>  //
>  // Author: Matthias Maennich
> @@ -9,7 +10,20 @@
>  ///
>  /// This contains the definition of the symtab reader
>  
> +#include <algorithm>
> +#include <iostream>
> +#include <unordered_set>
> +
> +#include "abg-elf-helpers.h"
> +#include "abg-fwd.h"
> +#include "abg-internal.h"
> +#include "abg-tools-utils.h"
> +
> +// Though this is an internal header, we need to export the symbols to be able
> +// to test this code.  TODO: find a way to export symbols just for unit tests.
> +ABG_BEGIN_EXPORT_DECLARATIONS
>  #include "abg-symtab-reader.h"
> +ABG_END_EXPORT_DECLARATIONS

Ah, right.  Now that we have unitary tests this may become a real thing
to care about.  I think a way forward at some point might be to use ELF
versioning (or something similar) to put assign a dedicated ELF version
(e.g, _ABG_INTERNAL_) to all these symbols that are needed just for unit
testing.  Then a linker script would hide all these symbols in the final
shared library.

Another way to go would be to use the unit testing smartly, rather than
like a gold hammer that considers that everything is a nail.  Contrary
to popular belief, I am unimpressed by the parroting of the fable that
unit testing would be the end-all-be-all of software engineering.  I
think that reductionist view might be more harmful than what people
might think.  I tend to lean towards the "it depends" point of view.  If
we have a good enough API, I think unit testing on an envelope that is
smaller than the exposed API is often counterproductive in practise.  If
we /need/ to do that, then it probably means that the API might need to
be more granular, more "test-able", so to speak.  So we might
address/fix the problem at that level instead.

In any case, I guess this can be saved as an undertaking for another day
:-)

[...]


> +/// Construct a symtab object and instantiate from an ELF handle. Also pass
> +/// in an ir::environment handle to interact with the context we are living
> +/// in. If specified, the symbol_predicate will be respected when creating
> +/// the full vector of symbols.
> +symtab_ptr
> +symtab::load(Elf*	      elf_handle,
> +	     ir::environment* env,
> +	     symbol_predicate is_suppressed)

This function lacks descriptive comments for its parameters and return
value.

I agree the meaning parameters are obvious in the overall context of the
code, but we need the comment this to have a complete API doc generated
with all the descriptions :-(

> +{
> +  ABG_ASSERT(elf_handle);
> +  ABG_ASSERT(env);
> +
> +  symtab_ptr result(new symtab);
> +  if (!result->load_(elf_handle, env, is_suppressed))
> +    return {};
> +
> +  return result;
> +}
> +
> +/// Construct a symtab object from existing name->symbol lookup maps.
> +/// They were possibly read from a different representation (XML maybe).
> +symtab_ptr
> +symtab::load(string_elf_symbols_map_sptr function_symbol_map,
> +	     string_elf_symbols_map_sptr variables_symbol_map)
>

Likewise.

> +{
> +  symtab_ptr result(new symtab);
> +  if (!result->load_(function_symbol_map, variables_symbol_map))
> +    return {};
> +
> +  return result;
> +}
> +
> +symtab::symtab() : is_kernel_binary_(false), has_ksymtab_entries_(false) {}

Just for the sake of consistency with the rest of the code, I'd say this
might be written as: 

symtab::symtab()
  : is_kernel_binary_(false), has_ksymtab_entries_(false)
{}

> +
> +/// Load the symtab representation from an Elf binary presented to us by an
> +/// Elf* handle.
> +///
> +/// This method iterates over the entries of .symtab and collects all
> +/// interesting symbols (functions and variables).
> +///
> +/// In case of a Linux Kernel binary, it also collects information about the
> +/// symbols exported via EXPORT_SYMBOL in the Kernel that would then end up
> +/// having a corresponding __ksymtab entry.
> +///
> +/// Symbols that are suppressed will be omitted from the symbols_ vector, but
> +/// still be discoverable through the name->symbol and addr->symbol lookup
> +/// maps.
> +bool
> +symtab::load_(Elf*	       elf_handle,
> +	      ir::environment* env,
> +	      symbol_predicate is_suppressed)
> +{

This function lacks descriptive comments for its parameters and return
value.

[...]


> +      const elf_symbol_sptr& symbol_sptr = elf_symbol::create(
> +	  env, i, sym->st_size, name,
> +	  elf_helpers::stt_to_elf_symbol_type(GELF_ST_TYPE(sym->st_info)),
> +	  elf_helpers::stb_to_elf_symbol_binding(GELF_ST_BIND(sym->st_info)),
> +	  sym_is_defined, sym_is_common, ver,
> +	  elf_helpers::stv_to_elf_symbol_visibility(
> +	      GELF_ST_VISIBILITY(sym->st_other)),
> +	  false); // TODO: is_linux_strings_cstr

Do we still need the is_linux_strings_cstr parameter?  I'd say no, as we
don't mess with __ksymtab* sections anymore.  So maybe the TODO comment
should be more explicit in saying that we need to get rid of it.

Besides, I'd say that to comply with the rest of the code, the "new
line" should come before the opening parenthesis of the function call, e.g:

  const elf_symbol_sptr& symbol_sptr =
    elf_symbol::create
    (env, i, sym->st_size, name,
     elf_helpers::stt_to_elf_symbol_type(GELF_ST_TYPE(sym->st_info)),
     elf_helpers::stb_to_elf_symbol_binding(GELF_ST_BIND(sym->st_info)),
     sym_is_defined, sym_is_common, ver,
     elf_helpers::stv_to_elf_symbol_visibility
     (GELF_ST_VISIBILITY(sym->st_other)),
     /*is_linux_strings_cstr=*/false); // TODO: The
				       // is_linux_strings_cstr
				       // parameter should be removed
				       // as it's not needed anymore

[...]

> +
> +/// Load the symtab representation from a function/variable lookup map pair.
> +///
> +/// This method assumes the lookup maps are correct and sets up the data
> +/// vector as well as the name->symbol lookup map. The addr->symbol lookup
> +/// map cannot be set up in this case.
> +bool
> +symtab::load_(string_elf_symbols_map_sptr function_symbol_map,
> +	     string_elf_symbols_map_sptr variables_symbol_map)
> +
> +{

This function lacks descriptive comments for its parameters and return
value.

> +  if (function_symbol_map)
> +    for (const auto& symbol_map_entry : *function_symbol_map)
> +      {
> +	symbols_.insert(symbols_.end(), symbol_map_entry.second.begin(),
> +			symbol_map_entry.second.end());
> +	ABG_ASSERT(name_symbol_map_.insert(symbol_map_entry).second);
> +      }
> +
> +  if (variables_symbol_map)
> +    for (const auto& symbol_map_entry : *variables_symbol_map)
> +      {
> +	symbols_.insert(symbols_.end(), symbol_map_entry.second.begin(),
> +			symbol_map_entry.second.end());
> +	ABG_ASSERT(name_symbol_map_.insert(symbol_map_entry).second);
> +      }
> +
> +  // sort the symbols for deterministic output
> +  std::sort(symbols_.begin(), symbols_.end(), symbol_sort);
> +
> +  return true;
> +}
> +
>  } // end namespace symtab_reader
>  } // end namespace abigail
> diff --git a/src/abg-symtab-reader.h b/src/abg-symtab-reader.h
> index a929166b83ef..4c5e3b85c22d 100644
> --- a/src/abg-symtab-reader.h
> +++ b/src/abg-symtab-reader.h

[...]


> +/// The symtab filter is the object passed to the symtab object in order to
> +/// iterate over the symbols in the symtab while applying filters.
> +///
> +/// The general idea is that it consists of a set of optionally enforced flags,
> +/// such as 'functions' or 'variables'. If not set, those are not filtered for,
> +/// neither inclusive nor exclusive. If set they are all ANDed together.
> +class symtab_filter
> +{
> +public:
> +  // Default constructor disabling all features.
> +  symtab_filter() {}
> +
> +  bool
> +  matches(const elf_symbol& symbol) const;
> +
> +  void
> +  set_functions(bool new_value = true)

This function which is defined lacks comments for its parameter.

> +  { functions_ = new_value; };

To comply with the rest of the code, one liner function implementation
don't have any leading/trailing space, e.g, it should be:

    void
    set_functions(bool new_value = true)
    {functions_ = new_value;}

> +
> +  void
> +  set_variables(bool new_value = true)
> +  { variables_ = new_value; };

Likewise.

> +
> +  void
> +  set_public_symbols(bool new_value = true)
> +  { public_symbols_ = new_value; };

Likewise.

> +
> +  void
> +  set_undefined_symbols(bool new_value = true)
> +  { undefined_symbols_ = new_value; };

Likewise.

> +
> +  void
> +  set_kernel_symbols(bool new_value = true)
> +  { kernel_symbols_ = new_value; };

Likewise.

> +
> +private:
> +  // The symbol is a function (FUNC)
> +  abg_compat::optional<bool> functions_;
> +
> +  // The symbol is a variables (OBJECT)
> +  abg_compat::optional<bool> variables_;
> +
> +  // The symbol is publicly accessible (global/weak with default/protected
> +  // visibility)
> +  abg_compat::optional<bool> public_symbols_;
> +
> +  // The symbols is not defined (declared)
> +  abg_compat::optional<bool> undefined_symbols_;
> +
> +  // The symbol is listed in the ksymtab (for Linux Kernel binaries).
> +  abg_compat::optional<bool> kernel_symbols_;
> +};
> +
> +/// Base iterator for our custom iterator based on whatever the const_iterator
> +/// is for a vector of symbols.
> +/// As of writing this, std::vector<elf_symbol_sptr>::const_iterator.
> +typedef elf_symbols::const_iterator base_iterator;
> +
> +/// An iterator to walk a vector of elf_symbols filtered by symtab_filter.
> +///
> +/// The implementation inherits all properties from the vector's
> +/// const_iterator, but intercepts where necessary to allow effective
> +/// filtering. This makes it a STL compatible iterator for general purpose
> +/// usage.
> +class symtab_iterator : public base_iterator
> +{
> +public:
> +  typedef base_iterator::value_type	 value_type;
> +  typedef base_iterator::reference	 reference;
> +  typedef base_iterator::pointer	 pointer;
> +  typedef base_iterator::difference_type difference_type;
> +  typedef std::forward_iterator_tag	 iterator_category;
> +
> +  /// Construct the iterator based on a pair of underlying iterators and a
> +  /// symtab_filter object. Immediately fast forward to the next element that
> +  /// matches the criteria (if any).
> +  symtab_iterator(base_iterator	       begin,
> +		  base_iterator	       end,
> +		  const symtab_filter& filter = symtab_filter())
> +    : base_iterator(begin), end_(end), filter_(filter)

This function lacks description for its parameters.

> +  { skip_to_next(); }

There should be no trailing/leading space.

> +
> +  /// Pre-increment operator to advance to the next matching element.
> +  symtab_iterator&
> +  operator++()
> +  {

This function lacks a description for its return value.

> +    base_iterator::operator++();
> +    skip_to_next();
> +    return *this;
> +  }
> +
> +  /// Post-increment operator to advance to the next matching element.
> +  symtab_iterator
> +  operator++(int)
> +  {

This function lacks a description for its return value.

> +    symtab_iterator result(*this);
> +    ++(*this);
> +    return result;
> +  }

[...]


> +/// symtab is the actual data container of the symtab_reader implementation.
> +///
> +/// The symtab is instantiated either via an Elf handle (from binary) or from a
> +/// set of existing symbol maps (usually when instantiated from XML). It will
> +/// then discover the symtab, possibly the ksymtab (for Linux Kernel binaries)
> +/// and setup the data containers and lookup maps for later perusal.
> +///
> +/// The symtab is supposed to be used in a const context as all information is
> +/// already computed at construction time. Symbols are stored sorted to allow
> +/// deterministic reading of the entries.
> +///
> +/// An example use of the symtab class is
> +///
> +/// const auto symtab    = symtab::load(elf_handle, env);
> +/// symtab_filter filter = symtab->make_filter();
> +/// filter.set_public_symbols();
> +/// filter.set_functions();
> +///
> +/// for (const auto& symbol : filtered_symtab(*symtab, filter))
> +///   {
> +///     std::cout << symbol->get_name() << "\n";
> +///   }
> +///

I find this a great API design.  Simple enough to understand, even for a
simple mind like me, not over-engineered.  Thank you for that.

> +/// This uses the filtered_symtab proxy object to capture the filter.
> +class symtab
> +{
> +public:
> +  typedef std::function<bool(const elf_symbol_sptr&)> symbol_predicate;
> +
> +  /// Indicate whether any (kernel) symbols have been seen at construction.
> +  ///
> +  /// @return true if there are symbols detected earlier.
> +  bool
> +  has_symbols() const
> +  { return is_kernel_binary_ ? has_ksymtab_entries_ : !symbols_.empty(); }

Leading/Trailing space.

> +
> +  symtab_filter
> +  make_filter() const;
> +
> +  /// The (only) iterator type we offer is a const_iterator implemented by the
> +  /// symtab_iterator.
> +  typedef symtab_iterator const_iterator;
> +
> +  /// Obtain an iterator to the beginning of the symtab according to the filter
> +  /// criteria. Whenever this iterator advances, it skips elements that do not
> +  /// match the filter criteria.
> +  ///
> +  /// @param filter the symtab_filter to match symbols against
> +  ///
> +  /// @return a filtering const_iterator of the underlying type
> +  const_iterator
> +  begin(const symtab_filter& filter) const
> +  { return symtab_iterator(symbols_.begin(), symbols_.end(), filter); }

Likewise.

> +  /// Obtain an iterator to the end of the symtab.
> +  ///
> +  /// @return an end iterator
> +  const_iterator
> +  end() const
> +  { return symtab_iterator(symbols_.end(), symbols_.end()); }

Likewise.

[...]


> +/// Helper class to allow range-for loops on symtabs for C++11 and later code.
> +/// It serves as a proxy for the symtab iterator and provides a begin() method
> +/// without arguments, as required for range-for loops (and possibly other
> +/// iterator based transformations).
> +///
> +/// Example usage:
> +///
> +///   for (const auto& symbol : filtered_symtab(tab, filter))
> +///     {
> +///       std::cout << symbol->get_name() << "\n";
> +///     }
> +///
> +class filtered_symtab
> +{
> +  const symtab&	      tab_;
> +  const symtab_filter filter_;
> +
> +public:
> +  /// Construct the proxy object keeping references to the underlying symtab
> +  /// and the filter object.
> +  filtered_symtab(const symtab& tab, const symtab_filter& filter)
> +    : tab_(tab), filter_(filter) { }

I'd put the '{}' on the next line, with no space between the two braces.

> +  /// Pass through symtab.begin(), but also pass on the filter.
> +  symtab::const_iterator
> +  begin() const
> +  { return tab_.begin(filter_); }

No trailing/leading space.

> +
> +  /// Pass through symtab.end().
> +  symtab::const_iterator
> +  end() const
> +  { return tab_.end(); }

No trailing/leading space.

> +};
> +
>  } // end namespace symtab_reader
>  } // end namespace abigail

Thank you for this gem!

Cheers,
  

Patch

diff --git a/src/abg-symtab-reader.cc b/src/abg-symtab-reader.cc
index a6c8ca0ef548..4576be2a0b42 100644
--- a/src/abg-symtab-reader.cc
+++ b/src/abg-symtab-reader.cc
@@ -1,6 +1,7 @@ 
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 // -*- Mode: C++ -*-
 //
+// Copyright (C) 2013-2020 Red Hat, Inc.
 // Copyright (C) 2020 Google, Inc.
 //
 // Author: Matthias Maennich
@@ -9,7 +10,20 @@ 
 ///
 /// This contains the definition of the symtab reader
 
+#include <algorithm>
+#include <iostream>
+#include <unordered_set>
+
+#include "abg-elf-helpers.h"
+#include "abg-fwd.h"
+#include "abg-internal.h"
+#include "abg-tools-utils.h"
+
+// Though this is an internal header, we need to export the symbols to be able
+// to test this code.  TODO: find a way to export symbols just for unit tests.
+ABG_BEGIN_EXPORT_DECLARATIONS
 #include "abg-symtab-reader.h"
+ABG_END_EXPORT_DECLARATIONS
 
 namespace abigail
 {
@@ -17,5 +31,338 @@  namespace abigail
 namespace symtab_reader
 {
 
+/// symtab_filter implementations
+
+/// Determine whether a symbol is matching the filter criteria of this filter
+/// object. In terms of a filter functionality, you would _not_ filter out
+/// this symbol if it passes this (i.e. returns true).
+///
+/// @param symbol The Elf symbol under test.
+///
+/// @return whether the symbol matches all relevant / required criteria
+bool
+symtab_filter::matches(const elf_symbol& symbol) const
+{
+  if (functions_ && *functions_ != symbol.is_function())
+    return false;
+  if (variables_ && *variables_ != symbol.is_variable())
+    return false;
+  if (public_symbols_ && *public_symbols_ != symbol.is_public())
+    return false;
+  if (undefined_symbols_ && *undefined_symbols_ == symbol.is_defined())
+    return false;
+  if (kernel_symbols_ && *kernel_symbols_ != symbol.is_in_ksymtab())
+    return false;
+
+  return true;
+}
+
+/// symtab implementations
+
+/// Obtain a suitable default filter for iterating this symtab object.
+///
+/// The symtab_filter obtained is populated with some sensible default
+/// settings, such as public_symbols(true) and kernel_symbols(true) if the
+/// binary has been identified as Linux Kernel binary.
+///
+/// @return a symtab_filter with sensible populated defaults
+symtab_filter
+symtab::make_filter() const
+{
+  symtab_filter filter;
+  filter.set_public_symbols();
+  if (is_kernel_binary_)
+    filter.set_kernel_symbols();
+  return filter;
+}
+
+/// Get a vector of symbols that are associated with a certain name
+///
+/// @param name the name the symbols need to match
+///
+/// @return a vector of symbols, empty if no matching symbols have been found
+const elf_symbols&
+symtab::lookup_symbol(const std::string& name) const
+{
+  static const elf_symbols empty_result;
+  const auto it = name_symbol_map_.find(name);
+  if (it != name_symbol_map_.end())
+      return it->second;
+  return empty_result;
+}
+
+/// Lookup a symbol by its address
+///
+/// @param symbol_addr the starting address of the symbol
+///
+/// @return a symbol if found, else an empty sptr
+const elf_symbol_sptr&
+symtab::lookup_symbol(GElf_Addr symbol_addr) const
+{
+  static const elf_symbol_sptr empty_result;
+  const auto it = addr_symbol_map_.find(symbol_addr);
+  if (it != addr_symbol_map_.end())
+      return it->second;
+  return empty_result;
+}
+
+/// A symbol sorting functor.
+static struct
+{
+  bool
+  operator()(const elf_symbol_sptr& left, const elf_symbol_sptr& right)
+  { return left->get_id_string() < right->get_id_string(); }
+} symbol_sort;
+
+/// Construct a symtab object and instantiate from an ELF handle. Also pass
+/// in an ir::environment handle to interact with the context we are living
+/// in. If specified, the symbol_predicate will be respected when creating
+/// the full vector of symbols.
+symtab_ptr
+symtab::load(Elf*	      elf_handle,
+	     ir::environment* env,
+	     symbol_predicate is_suppressed)
+{
+  ABG_ASSERT(elf_handle);
+  ABG_ASSERT(env);
+
+  symtab_ptr result(new symtab);
+  if (!result->load_(elf_handle, env, is_suppressed))
+    return {};
+
+  return result;
+}
+
+/// Construct a symtab object from existing name->symbol lookup maps.
+/// They were possibly read from a different representation (XML maybe).
+symtab_ptr
+symtab::load(string_elf_symbols_map_sptr function_symbol_map,
+	     string_elf_symbols_map_sptr variables_symbol_map)
+{
+  symtab_ptr result(new symtab);
+  if (!result->load_(function_symbol_map, variables_symbol_map))
+    return {};
+
+  return result;
+}
+
+symtab::symtab() : is_kernel_binary_(false), has_ksymtab_entries_(false) {}
+
+/// Load the symtab representation from an Elf binary presented to us by an
+/// Elf* handle.
+///
+/// This method iterates over the entries of .symtab and collects all
+/// interesting symbols (functions and variables).
+///
+/// In case of a Linux Kernel binary, it also collects information about the
+/// symbols exported via EXPORT_SYMBOL in the Kernel that would then end up
+/// having a corresponding __ksymtab entry.
+///
+/// Symbols that are suppressed will be omitted from the symbols_ vector, but
+/// still be discoverable through the name->symbol and addr->symbol lookup
+/// maps.
+bool
+symtab::load_(Elf*	       elf_handle,
+	      ir::environment* env,
+	      symbol_predicate is_suppressed)
+{
+
+  Elf_Scn* symtab_section = elf_helpers::find_symbol_table_section(elf_handle);
+  if (!symtab_section)
+    {
+      std::cerr << "No symbol table found: Skipping symtab load.\n";
+      return false;
+    }
+
+  GElf_Shdr symtab_sheader;
+  gelf_getshdr(symtab_section, &symtab_sheader);
+
+  // check for bogus section header
+  if (symtab_sheader.sh_entsize == 0)
+    {
+      std::cerr << "Invalid symtab header found: Skipping symtab load.\n";
+      return false;
+    }
+
+  const size_t number_syms =
+      symtab_sheader.sh_size / symtab_sheader.sh_entsize;
+
+  Elf_Data* symtab = elf_getdata(symtab_section, 0);
+  if (!symtab)
+    {
+      std::cerr << "Could not load elf symtab: Skipping symtab load.\n";
+      return false;
+    }
+
+  const bool is_kernel = elf_helpers::is_linux_kernel(elf_handle);
+  std::unordered_set<std::string> exported_kernel_symbols;
+
+  for (size_t i = 0; i < number_syms; ++i)
+    {
+      GElf_Sym *sym, sym_mem;
+      sym = gelf_getsym(symtab, i, &sym_mem);
+      if (!sym)
+	{
+	  std::cerr << "Could not load symbol with index " << i
+		    << ": Skipping symtab load.\n";
+	  return false;
+	}
+
+      const char* const name_str =
+	  elf_strptr(elf_handle, symtab_sheader.sh_link, sym->st_name);
+
+      // no name, no game
+      if (!name_str)
+	continue;
+
+      const std::string name = name_str;
+      if (name.empty())
+	continue;
+
+      // Handle ksymtab entries. Every symbol entry that starts with __ksymtab_
+      // indicates that the symbol in question is exported through ksymtab. We
+      // do not know whether this is ksymtab_gpl or ksymtab, but that is good
+      // enough for now.
+      //
+      // We could follow up with this entry:
+      //
+      // symbol_value -> ksymtab_entry in either ksymtab_gpl or ksymtab
+      //              -> addr/name/namespace (in case of PREL32: offset)
+      //
+      // That way we could also detect ksymtab<>ksymtab_gpl changes or changes
+      // of the symbol namespace.
+      //
+      // As of now this lookup is fragile, as occasionally ksymtabs are empty
+      // (seen so far for kernel modules and LTO builds). Hence we stick to the
+      // fairly safe assumption that ksymtab exported entries are having an
+      // appearence as __ksymtab_<symbol> in the symtab.
+      if (is_kernel && name.rfind("__ksymtab_", 0) == 0)
+	{
+	  ABG_ASSERT(exported_kernel_symbols.insert(name.substr(10)).second);
+	  continue;
+	}
+
+      // filter out uninteresting entries and only keep functions/variables for
+      // now. The rest might be interesting in the future though.
+      const int sym_type = GELF_ST_TYPE(sym->st_info);
+      if (!(sym_type == STT_FUNC
+	    || sym_type == STT_GNU_IFUNC
+	    // If the symbol is for an OBJECT, the index of the
+	    // section it refers to cannot be absolute.
+	    // Otherwise that OBJECT is not a variable.
+	    || (sym_type == STT_OBJECT && sym->st_shndx != SHN_ABS)
+	    || sym_type == STT_TLS))
+	continue;
+
+      const bool sym_is_defined = sym->st_shndx != SHN_UNDEF;
+      // this occurs in relocatable files.
+      const bool sym_is_common = sym->st_shndx == SHN_COMMON;
+
+      elf_symbol::version ver;
+      elf_helpers::get_version_for_symbol(elf_handle, i, sym_is_defined, ver);
+
+      const elf_symbol_sptr& symbol_sptr = elf_symbol::create(
+	  env, i, sym->st_size, name,
+	  elf_helpers::stt_to_elf_symbol_type(GELF_ST_TYPE(sym->st_info)),
+	  elf_helpers::stb_to_elf_symbol_binding(GELF_ST_BIND(sym->st_info)),
+	  sym_is_defined, sym_is_common, ver,
+	  elf_helpers::stv_to_elf_symbol_visibility(
+	      GELF_ST_VISIBILITY(sym->st_other)),
+	  false); // TODO: is_linux_strings_cstr
+
+      // We do not take suppressed symbols into our symbol vector to avoid
+      // accidental leakage. But we ensure supressed symbols are otherwise set
+      // up for lookup.
+      if (!(is_suppressed && is_suppressed(symbol_sptr)))
+	// add to the symbol vector
+	symbols_.push_back(symbol_sptr);
+      else
+	symbol_sptr->set_is_suppressed(true);
+
+      // add to the name->symbol lookup
+      name_symbol_map_[name].push_back(symbol_sptr);
+
+      // add to the addr->symbol lookup
+      if (symbol_sptr->is_common_symbol())
+	{
+	  const auto it = name_symbol_map_.find(name);
+	  ABG_ASSERT(it != name_symbol_map_.end());
+	  const elf_symbols& common_sym_instances = it->second;
+	  ABG_ASSERT(!common_sym_instances.empty());
+	  if (common_sym_instances.size() > 1)
+	    {
+	      elf_symbol_sptr main_common_sym = common_sym_instances[0];
+	      ABG_ASSERT(main_common_sym->get_name() == name);
+	      ABG_ASSERT(main_common_sym->is_common_symbol());
+	      ABG_ASSERT(symbol_sptr.get() != main_common_sym.get());
+	      main_common_sym->add_common_instance(symbol_sptr);
+	    }
+	}
+      else if (symbol_sptr->is_defined())
+	{
+	  const GElf_Addr symbol_value =
+	      elf_helpers::maybe_adjust_et_rel_sym_addr_to_abs_addr(elf_handle,
+								    sym);
+
+	  const auto result =
+	    addr_symbol_map_.emplace(symbol_value, symbol_sptr);
+	  if (!result.second)
+	    result.first->second->get_main_symbol()->add_alias(symbol_sptr);
+	}
+    }
+
+  is_kernel_binary_ = elf_helpers::is_linux_kernel(elf_handle);
+
+  // Now apply the ksymtab_exported attribute to the symbols we collected.
+  for (const auto& symbol : exported_kernel_symbols)
+    {
+      const auto r = name_symbol_map_.find(symbol);
+      if (r == name_symbol_map_.end())
+	continue;
+
+      for (const auto& elf_symbol : r->second)
+	  if (elf_symbol->is_public())
+	    elf_symbol->set_is_in_ksymtab(true);
+      has_ksymtab_entries_ = true;
+    }
+
+  // sort the symbols for deterministic output
+  std::sort(symbols_.begin(), symbols_.end(), symbol_sort);
+
+  return true;
+}
+
+/// Load the symtab representation from a function/variable lookup map pair.
+///
+/// This method assumes the lookup maps are correct and sets up the data
+/// vector as well as the name->symbol lookup map. The addr->symbol lookup
+/// map cannot be set up in this case.
+bool
+symtab::load_(string_elf_symbols_map_sptr function_symbol_map,
+	     string_elf_symbols_map_sptr variables_symbol_map)
+
+{
+  if (function_symbol_map)
+    for (const auto& symbol_map_entry : *function_symbol_map)
+      {
+	symbols_.insert(symbols_.end(), symbol_map_entry.second.begin(),
+			symbol_map_entry.second.end());
+	ABG_ASSERT(name_symbol_map_.insert(symbol_map_entry).second);
+      }
+
+  if (variables_symbol_map)
+    for (const auto& symbol_map_entry : *variables_symbol_map)
+      {
+	symbols_.insert(symbols_.end(), symbol_map_entry.second.begin(),
+			symbol_map_entry.second.end());
+	ABG_ASSERT(name_symbol_map_.insert(symbol_map_entry).second);
+      }
+
+  // sort the symbols for deterministic output
+  std::sort(symbols_.begin(), symbols_.end(), symbol_sort);
+
+  return true;
+}
+
 } // end namespace symtab_reader
 } // end namespace abigail
diff --git a/src/abg-symtab-reader.h b/src/abg-symtab-reader.h
index a929166b83ef..4c5e3b85c22d 100644
--- a/src/abg-symtab-reader.h
+++ b/src/abg-symtab-reader.h
@@ -12,12 +12,287 @@ 
 #ifndef __ABG_SYMTAB_READER_H__
 #define __ABG_SYMTAB_READER_H__
 
+#include <gelf.h>
+
+#include <functional>
+#include <iterator>
+#include <memory>
+#include <unordered_map>
+#include <vector>
+
+#include "abg-cxx-compat.h"  // for abg_compat::optional
+#include "abg-ir.h"
+
 namespace abigail
 {
-
 namespace symtab_reader
 {
 
+/// The symtab filter is the object passed to the symtab object in order to
+/// iterate over the symbols in the symtab while applying filters.
+///
+/// The general idea is that it consists of a set of optionally enforced flags,
+/// such as 'functions' or 'variables'. If not set, those are not filtered for,
+/// neither inclusive nor exclusive. If set they are all ANDed together.
+class symtab_filter
+{
+public:
+  // Default constructor disabling all features.
+  symtab_filter() {}
+
+  bool
+  matches(const elf_symbol& symbol) const;
+
+  void
+  set_functions(bool new_value = true)
+  { functions_ = new_value; };
+
+  void
+  set_variables(bool new_value = true)
+  { variables_ = new_value; };
+
+  void
+  set_public_symbols(bool new_value = true)
+  { public_symbols_ = new_value; };
+
+  void
+  set_undefined_symbols(bool new_value = true)
+  { undefined_symbols_ = new_value; };
+
+  void
+  set_kernel_symbols(bool new_value = true)
+  { kernel_symbols_ = new_value; };
+
+private:
+  // The symbol is a function (FUNC)
+  abg_compat::optional<bool> functions_;
+
+  // The symbol is a variables (OBJECT)
+  abg_compat::optional<bool> variables_;
+
+  // The symbol is publicly accessible (global/weak with default/protected
+  // visibility)
+  abg_compat::optional<bool> public_symbols_;
+
+  // The symbols is not defined (declared)
+  abg_compat::optional<bool> undefined_symbols_;
+
+  // The symbol is listed in the ksymtab (for Linux Kernel binaries).
+  abg_compat::optional<bool> kernel_symbols_;
+};
+
+/// Base iterator for our custom iterator based on whatever the const_iterator
+/// is for a vector of symbols.
+/// As of writing this, std::vector<elf_symbol_sptr>::const_iterator.
+typedef elf_symbols::const_iterator base_iterator;
+
+/// An iterator to walk a vector of elf_symbols filtered by symtab_filter.
+///
+/// The implementation inherits all properties from the vector's
+/// const_iterator, but intercepts where necessary to allow effective
+/// filtering. This makes it a STL compatible iterator for general purpose
+/// usage.
+class symtab_iterator : public base_iterator
+{
+public:
+  typedef base_iterator::value_type	 value_type;
+  typedef base_iterator::reference	 reference;
+  typedef base_iterator::pointer	 pointer;
+  typedef base_iterator::difference_type difference_type;
+  typedef std::forward_iterator_tag	 iterator_category;
+
+  /// Construct the iterator based on a pair of underlying iterators and a
+  /// symtab_filter object. Immediately fast forward to the next element that
+  /// matches the criteria (if any).
+  symtab_iterator(base_iterator	       begin,
+		  base_iterator	       end,
+		  const symtab_filter& filter = symtab_filter())
+    : base_iterator(begin), end_(end), filter_(filter)
+  { skip_to_next(); }
+
+  /// Pre-increment operator to advance to the next matching element.
+  symtab_iterator&
+  operator++()
+  {
+    base_iterator::operator++();
+    skip_to_next();
+    return *this;
+  }
+
+  /// Post-increment operator to advance to the next matching element.
+  symtab_iterator
+  operator++(int)
+  {
+    symtab_iterator result(*this);
+    ++(*this);
+    return result;
+  }
+
+private:
+  /// The end of the underlying iterator.
+  const base_iterator end_;
+
+  /// The symtab_filter used to determine when to advance.
+  const symtab_filter& filter_;
+
+  /// Skip to the next element that matches the filter criteria (if any). Hold
+  /// off when reaching the end of the underlying iterator.
+  void
+  skip_to_next()
+  {
+    while (*this != end_ && !filter_.matches(***this))
+      ++(*this);
+  }
+};
+
+/// Convenience declaration of a unique_ptr<symtab>
+class symtab;
+typedef std::unique_ptr<symtab> symtab_ptr;
+
+/// symtab is the actual data container of the symtab_reader implementation.
+///
+/// The symtab is instantiated either via an Elf handle (from binary) or from a
+/// set of existing symbol maps (usually when instantiated from XML). It will
+/// then discover the symtab, possibly the ksymtab (for Linux Kernel binaries)
+/// and setup the data containers and lookup maps for later perusal.
+///
+/// The symtab is supposed to be used in a const context as all information is
+/// already computed at construction time. Symbols are stored sorted to allow
+/// deterministic reading of the entries.
+///
+/// An example use of the symtab class is
+///
+/// const auto symtab    = symtab::load(elf_handle, env);
+/// symtab_filter filter = symtab->make_filter();
+/// filter.set_public_symbols();
+/// filter.set_functions();
+///
+/// for (const auto& symbol : filtered_symtab(*symtab, filter))
+///   {
+///     std::cout << symbol->get_name() << "\n";
+///   }
+///
+/// This uses the filtered_symtab proxy object to capture the filter.
+class symtab
+{
+public:
+  typedef std::function<bool(const elf_symbol_sptr&)> symbol_predicate;
+
+  /// Indicate whether any (kernel) symbols have been seen at construction.
+  ///
+  /// @return true if there are symbols detected earlier.
+  bool
+  has_symbols() const
+  { return is_kernel_binary_ ? has_ksymtab_entries_ : !symbols_.empty(); }
+
+  symtab_filter
+  make_filter() const;
+
+  /// The (only) iterator type we offer is a const_iterator implemented by the
+  /// symtab_iterator.
+  typedef symtab_iterator const_iterator;
+
+  /// Obtain an iterator to the beginning of the symtab according to the filter
+  /// criteria. Whenever this iterator advances, it skips elements that do not
+  /// match the filter criteria.
+  ///
+  /// @param filter the symtab_filter to match symbols against
+  ///
+  /// @return a filtering const_iterator of the underlying type
+  const_iterator
+  begin(const symtab_filter& filter) const
+  { return symtab_iterator(symbols_.begin(), symbols_.end(), filter); }
+
+  /// Obtain an iterator to the end of the symtab.
+  ///
+  /// @return an end iterator
+  const_iterator
+  end() const
+  { return symtab_iterator(symbols_.end(), symbols_.end()); }
+
+  const elf_symbols&
+  lookup_symbol(const std::string& name) const;
+
+  const elf_symbol_sptr&
+  lookup_symbol(GElf_Addr symbol_addr) const;
+
+  static symtab_ptr
+  load(Elf*		elf_handle,
+       ir::environment* env,
+       symbol_predicate is_suppressed = NULL);
+
+  static symtab_ptr
+  load(string_elf_symbols_map_sptr function_symbol_map,
+       string_elf_symbols_map_sptr variables_symbol_map);
+
+private:
+  /// Default constructor. Private to enforce creation by factory methods.
+  symtab();
+
+  /// The vector of symbols we discovered.
+  elf_symbols symbols_;
+
+  /// Whether this is a Linux Kernel binary
+  bool is_kernel_binary_;
+
+  /// Whether this kernel_binary has ksymtab entries
+  ///
+  /// A kernel module might not have a ksymtab if it does not export any
+  /// symbols. In order to quickly decide whether the symbol table is empty, we
+  /// remember whether we ever saw ksymtab entries.
+  bool has_ksymtab_entries_;
+
+  /// Lookup map name->symbol(s)
+  typedef std::unordered_map<std::string, std::vector<elf_symbol_sptr>>
+		       name_symbol_map_type;
+  name_symbol_map_type name_symbol_map_;
+
+  /// Lookup map addr->symbol
+  typedef std::unordered_map<GElf_Addr, elf_symbol_sptr> addr_symbol_map_type;
+  addr_symbol_map_type addr_symbol_map_;
+
+  bool
+  load_(Elf* elf_handle, ir::environment* env, symbol_predicate is_suppressed);
+
+  bool
+  load_(string_elf_symbols_map_sptr function_symbol_map,
+       string_elf_symbols_map_sptr variables_symbol_map);
+};
+
+/// Helper class to allow range-for loops on symtabs for C++11 and later code.
+/// It serves as a proxy for the symtab iterator and provides a begin() method
+/// without arguments, as required for range-for loops (and possibly other
+/// iterator based transformations).
+///
+/// Example usage:
+///
+///   for (const auto& symbol : filtered_symtab(tab, filter))
+///     {
+///       std::cout << symbol->get_name() << "\n";
+///     }
+///
+class filtered_symtab
+{
+  const symtab&	      tab_;
+  const symtab_filter filter_;
+
+public:
+  /// Construct the proxy object keeping references to the underlying symtab
+  /// and the filter object.
+  filtered_symtab(const symtab& tab, const symtab_filter& filter)
+    : tab_(tab), filter_(filter) { }
+
+  /// Pass through symtab.begin(), but also pass on the filter.
+  symtab::const_iterator
+  begin() const
+  { return tab_.begin(filter_); }
+
+  /// Pass through symtab.end().
+  symtab::const_iterator
+  end() const
+  { return tab_.end(); }
+};
+
 } // end namespace symtab_reader
 } // end namespace abigail