From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from relay7-d.mail.gandi.net (relay7-d.mail.gandi.net [217.70.183.200]) by sourceware.org (Postfix) with ESMTPS id 13000385E012 for ; Thu, 26 Mar 2020 17:12:25 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 13000385E012 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 relay7-d.mail.gandi.net (Postfix) with ESMTPSA id 338202000B; Thu, 26 Mar 2020 17:12:23 +0000 (UTC) Received: by localhost (Postfix, from userid 1000) id 5871B581892; Thu, 26 Mar 2020 18:12:23 +0100 (CET) From: Dodji Seketeli To: Giuliano Procida Cc: libabigail@sourceware.org, kernel-team@android.com, maennich@google.com Subject: Re: [RFC] Fix has_net_changes for leaf-changes-only mode. Organization: Me, myself and I References: <20200324171344.258865-1-gprocida@google.com> X-Operating-System: Fedora 33 X-URL: http://www.seketeli.net/~dodji Date: Thu, 26 Mar 2020 18:12:23 +0100 In-Reply-To: <20200324171344.258865-1-gprocida@google.com> (Giuliano Procida's message of "Tue, 24 Mar 2020 17:13:44 +0000") Message-ID: <87tv2bqbuw.fsf@seketeli.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-20.8 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, 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: Libabigail mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 26 Mar 2020 17:12:27 -0000 Hello Giuliano, Giuliano Procida a =C3=A9crit: > * Which other functions need the same treatment? > * Should something else change instead? > * What about tracking of changed types (inclusion of these in summary > stats and exit code for leaf mode)? > > This patch breaks tests and is not suitable for applying in its > current form. > > The issue we saw is that anonymous struct name changes triggered > hundreds of diffs Sorry, but what do you mean by "anonymous struct name changes" ? [...] > diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc > index 46bf9e30..7756c12b 100644 > --- a/src/abg-comparison.cc > +++ b/src/abg-comparison.cc > @@ -10602,21 +10602,26 @@ corpus_diff::has_net_changes() const > const diff_stats& stats =3D const_cast(this)-> > apply_filters_and_suppressions_before_reporting(); >=20=20 > + bool leaf =3D context()->show_leaf_changes_only(); > return (architecture_changed() > - || soname_changed() > - || stats.net_num_func_changed() > - || stats.net_num_vars_changed() > - || stats.net_num_func_added() > - || stats.net_num_added_func_syms() > - || stats.net_num_func_removed() > - || stats.net_num_removed_func_syms() > - || stats.net_num_vars_added() > - || stats.net_num_added_var_syms() > - || stats.net_num_vars_removed() > - || stats.net_num_removed_var_syms() > - || stats.net_num_added_unreachable_types() > - || stats.net_num_removed_unreachable_types() > - || stats.net_num_changed_unreachable_types()); > + || soname_changed() > + || (leaf > + ? stats.net_num_leaf_func_changes() > + : stats.net_num_func_changed()) > + || (leaf > + ? stats.net_num_leaf_var_changes() > + : stats.net_num_vars_changed()) In essence, this is code which behaviour depends on the kind of "reporter" we are using. That is, if we are using the leaf reporter, then we want a given behaviour, otherwise we want another behaviour. So I think a route to make the whole thing maintainable going forward would be to put that code in the reporters. That is, in include/abg-reporter.h, add a pure virtual bool reporter_base::diff_has_net_changes(const corpus_diff&) const function. That interface would then be implemented in both default_reporter and leaf_reporter class types. Then in corpus_diff::has_net_changes(), we'd do something like: return context()->get_reporter()->diff_has_net_changes(*this); That way, the actual choice of what constitute the set of net changes would be left to the each reporter. Does that make any sense? [...] > (mostly but not all filtered out in non-leaf mode which may be its own > bug). Hopefully with this approach, this issue should go away as well. Thanks a lot for looking into this. Cheers, --=20 Dodji