From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from relay6-d.mail.gandi.net (relay6-d.mail.gandi.net [217.70.183.198]) by sourceware.org (Postfix) with ESMTPS id 4DD313851C39 for ; Mon, 20 Jul 2020 15:39:06 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 4DD313851C39 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=seketeli.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=dodji@seketeli.org X-Originating-IP: 91.166.131.130 Received: from localhost (91-166-131-130.subs.proxad.net [91.166.131.130]) (Authenticated sender: dodji@seketeli.org) by relay6-d.mail.gandi.net (Postfix) with ESMTPSA id 3796BC000F; Mon, 20 Jul 2020 15:39:04 +0000 (UTC) Received: by localhost (Postfix, from userid 1000) id EA2EA1800938; Mon, 20 Jul 2020 17:39:02 +0200 (CEST) From: Dodji Seketeli To: Matthias Maennich Cc: libabigail@sourceware.org, gprocida@google.com, kernel-team@android.com Subject: Re: [PATCH v2 06/21] Refactor ELF symbol table reading by adding a new symtab reader Organization: Me, myself and I References: <20200619214305.562-1-maennich@google.com> <20200703164651.1510825-1-maennich@google.com> <20200703164651.1510825-7-maennich@google.com> X-Operating-System: Red Hat Enterprise Linux Workstation 7.8 Beta X-URL: http://www.seketeli.net/~dodji Date: Mon, 20 Jul 2020 17:39:02 +0200 In-Reply-To: <20200703164651.1510825-7-maennich@google.com> (Matthias Maennich's message of "Fri, 3 Jul 2020 18:46:36 +0200") Message-ID: <871rl6i4bt.fsf@seketeli.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-3.7 required=5.0 tests=BAYES_00, JMQ_SPF_NEUTRAL, KAM_DMARC_STATUS, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_PASS, TXREP 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: libabigail@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Mailing list of the Libabigail project List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 20 Jul 2020 15:39:08 -0000 Matthias Maennich a =C3=A9crit: [...] > +++ b/include/abg-symtab-reader.h > @@ -27,12 +27,381 @@ > #ifndef __ABG_SYMTAB_READER_H__ > #define __ABG_SYMTAB_READER_H__ >=20=20 > +#include > + > +#include > +#include > + > +#include "abg-cxx-compat.h" > +#include "abg-ir.h" > + > namespace abigail > { > - > namespace symtab_reader > { >=20=20 > +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 filter= ed 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, he= nce > + // 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 functions_; > + > + // The symbol is a variables (OBJECT) > + abg_compat::optional variables_; > + > + // The symbol is publicly accessible (global/weak with default/protect= ed > + // visibility) > + abg_compat::optional public_symbols_; > + > + // The symbols is not defined (declared) > + abg_compat::optional undefined_symbols_; > + > + // The symbol is listed in the ksymtab (for Linux Kernel binaries). > + abg_compat::optional 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_filt= ers. > 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 =3D > +/// 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, --=20 Dodji