Re: [PATCH v2 06/21] Refactor ELF symbol table reading by adding a new symtab reader

[Dodji Seketeli <dodji@seketeli.org>]

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

[...]

> +++ b/include/abg-symtab-reader.h
> @@ -27,12 +27,381 @@
>  #ifndef __ABG_SYMTAB_READER_H__
>  #define __ABG_SYMTAB_READER_H__
>  
> +#include <gelf.h>
> +
> +#include <iterator>
> +#include <vector>
> +
> +#include "abg-cxx-compat.h"
> +#include "abg-ir.h"
> +
>  namespace abigail
>  {
> -
>  namespace symtab_reader
>  {
>  
> +class symtab_filter_builder;
> +
> +/// 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:
> +  // The symtab_filter_builder helps us to build filters efficiently, hence
> +  // let's be nice and grant access to our internals.
> +  friend class symtab_filter_builder;
> +
> +  // Default constructor disabling all features.
> +  symtab_filter() {}
> +
> +  /// 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
> +  matches(const elf_symbol_sptr& symbol) const;
> +
> +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_;
> +};

As a trend, I've been moving slowly away from exposing details of (new)
types that are meant to be public interface and are thus declared in
include/*.h.

In the future, we'll be likely moving to proposing some level of ABI
compatibility for the library.  I know there many things to change
before reaching that goal, but for new classes at least, I try to not
make the road to that goal longer.

So can we please stick to a pimpl[1] pattern like what is done elsewhere
in the source code? For instance, in include/abg-suppression.h, or even
in the majority of abg-ir.h?
[1]: https://en.cppreference.com/w/cpp/language/pimpl.

Also, throughout the existing code base, doxygen code comments are put
close to where the actual code (definition) is, as opposed to being in
the header files near the declaration.  This is a conscious choice to
keep those comment close to where the actual code is.  So that whenever
the code is changed, the comment can be readily adapted without having
to go hunt the declaration elsewhere to adapt it.  Of course, because
classes are defined in header files, their doxygen comment is in the
header file there.

For the sake of consistency, can we please stick that exsiting scheme
and avoid putting API documentation in the header files near the
declarations and rather put them in the *.cc files near the definitions
of functions instead?

> +
> +/// Helper class to provide an attractive interface to build symtab_filters.
>

For all intends and purposes, I tend to think that attractiveness is in
the eye of the beholder :-)

> +///
> +/// When constructed, the helper instantiates a default symtab_filter and
> +/// allows modifications to it via builder pattern / fluent interface.
> +///
> +/// When assigned to a symtab_filter instance, it converts by returning the
> +/// locally build symtab_filter instance.
> +///
> +/// Example usage:
> +///
> +///   const symtab_filter filter =
> +///                symtab_filter_builder().functions().kernel_symbols();


Ahh fluent interface design patterns ...

Quite frankly, I perceive this as overkill in this particular context.

I mean, if the domain-specific language you are creating with this
'fluent interface' was supposed to have super tight semantic constraints
that are to be enforced in order to avoid some sort of corruption or
whatnot, then I'd maybe say OK.

My point is that I think that something like libabigail should be
hackable by folks coming from various horizons, especially low level
tools-oriented people.  My belief is that it does have value to stay
"simple" as far as the abstractions we use are concerned.  I deliberatly
chose the "less abstraction" path whenever possible.  Why?

Because someone who usually hacks on ELF or DWARF should be able to get
into the code without having to dig through loads of abstraction layers
and design patters, if that is possible.  We shouldn't require that
people be design pattern wizzards to understand the code.  That, at
least, has been one of my design philosophies since the beginning.  I
believe however that having the "right level" of abstraction is key to
handle the problem at hand in a maintainable way, so I am not against
abstractions and design patterns, far from it.

But in this case though, I think that just having:

    symtab_filter f;
    f.enable_function_symbols();
    f.enable_linux_kernel_symbols();

takes more typing but is less surprising /in the context/ that I laid
down above.  Surely less attractive to a fluent interface practionner
but less surprising to a low level tools hacker.  And at this point both
ways to write the code get the job done.

Sorry to be picky on this, but I think that these are design
philosophies that are important to convey and try to adhere to
otherwise, over time, the code base is going to be quite hard to
maintain, in part due to lack of consistency in our vision.

So I'd rather remove this layer, sorry.  And from what I read in how
this is used, I don't think it'd be removing any functionality.

Would it?

Cheers,


-- 
		Dodji