From patchwork Wed Jul 22 11:07:36 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Giuliano Procida X-Patchwork-Id: 40147 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id D896F387090D; Wed, 22 Jul 2020 11:07:50 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org D896F387090D DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1595416070; bh=AHgZ64TqggUtpz2rrUl192uTtB3ZnR1q7luixRrshyQ=; h=Date:In-Reply-To:References:Subject:To:List-Id:List-Unsubscribe: List-Archive:List-Help:List-Subscribe:From:Reply-To:Cc:From; b=aebD/gcRxzfNCiO/tEjoL6qo+AF7ZH2AAev8U8obn6/VJxDu90QXXsfykxQr7/veE 1rYBnnNk4fEQ/UOrKKSe4HlGnrig8jFnC/McUQKtPk7n4ieAJtFCMSBoIL5utnYHaq vBCVgL9+36bYy/OCQka9n4QlKIO4ZQym3K4lIGGk= X-Original-To: libabigail@sourceware.org Delivered-To: libabigail@sourceware.org Received: from mail-wm1-x349.google.com (mail-wm1-x349.google.com [IPv6:2a00:1450:4864:20::349]) by sourceware.org (Postfix) with ESMTPS id CB8F6387606B for ; Wed, 22 Jul 2020 11:07:44 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org CB8F6387606B Received: by mail-wm1-x349.google.com with SMTP id 65so922041wmd.8 for ; Wed, 22 Jul 2020 04:07:44 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:in-reply-to:message-id:mime-version :references:subject:from:to:cc:content-transfer-encoding; bh=AHgZ64TqggUtpz2rrUl192uTtB3ZnR1q7luixRrshyQ=; b=cHEY0vsjgHe3z4yyn0Ghr7V7/kZxThHc37TDgDnvWj5VUhjz8tO5T9JIToEzOsTQgs fCOCoXadskGbzbpzBoFvn3xeNkdggqrYu9NHRlXV+FX8CeVW/dtc98tmUf2M9JdyuuHL DG2/qIQzgxyR3ir2tpymozBkMOqX3pQxFbwa8YxrTVI8PaYojLJhmrKZMT2akSeUeleT RF5nWC4WpkR4WBJa0MLz1efCnI0nHCupMlTq6Oo6pcQl00rAFTt3kVlKG2v+DVbYovFd /zQ3ZMe+ea7+IAfJIzdb87mVxJ+lzaoDmjyrzY8dwuLRpF1TVSyQf0CydUxvejRQ9hxD AVDw== X-Gm-Message-State: AOAM533GRUFaqaWcXYj1lwVcP9nwqZWFl5W5+eVfITAext8tU/5mTyWG /WLjipR/yNDoZj07JrCcd7M2kBtG5MImv8fGbltpWjwnpQynUXtAlHaX0O0gqI2jzjbk94TZI3a CGCMCubO29lAz/RaEeczv2wqETTSzZFYV2Hyob0dIGhXWSYVezhSi9bbRoqiFbUPj2txxjxc= X-Google-Smtp-Source: ABdhPJyGEalDfGxMpcCSVpz8KdHvGRR7zNNYcFMIPUHGbj13GPuA4A4weyswE1qsKZfML0AyAf/N+Gr6NL444A== X-Received: by 2002:adf:bb14:: with SMTP id r20mr16004699wrg.366.1595416063851; Wed, 22 Jul 2020 04:07:43 -0700 (PDT) Date: Wed, 22 Jul 2020 12:07:36 +0100 In-Reply-To: <20200722110736.2550361-1-gprocida@google.com> Message-Id: <20200722110736.2550361-2-gprocida@google.com> Mime-Version: 1.0 References: <20200722110736.2550361-1-gprocida@google.com> X-Mailer: git-send-email 2.28.0.rc0.105.gf9edc3c819-goog Subject: [RFC PATCH 1/1] Fix decl_base comparison function To: libabigail@sourceware.org X-Spam-Status: No, score=-23.0 required=5.0 tests=BAYES_00, DKIMWL_WL_MED, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, USER_IN_DEF_DKIM_WL 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-Patchwork-Original-From: Giuliano Procida via Libabigail From: Giuliano Procida Reply-To: Giuliano Procida Cc: maennich@google.com, kernel-team@android.com Errors-To: libabigail-bounces@sourceware.org Sender: "Libabigail" When two decl_base ojects are compared, there are both fast and slow paths to the name comparison. The latter is roughly equivalent to comparing names after applying the regex s/(__anonymous_(?:struct|union|enum)__)\d+/\1/g to the names before comparing them while the former is a straight string comparison with some tweaks for detecting anonymous types. The slow path is taken care of by the helper function tools_utils::decl_names_equal but unfortunately, there is a missing negation of the returned bool. This commit fixes this and updates the few affected tests. Rather than just adding a '!', this commit replaces the negative decls_are_different with a positive decls_are_same. I spent far too long staring at the code before I spotted the mistake and having positively-named things improves readability. This commit also adds some TODOs for review around the fast path logic. The same helper function is also called by has_harmless_name_change and that should be reviewed as well. * src/abg-ir.cc (equals): In the decl_base overload, note that the value returned by decl_names_equal should be negated and replace decls_are_different with decls_are_same, negating all occurrences. * tests/data/test-diff-dwarf/PR25058-liblttng-ctl-report-1.txt: Update tests, removing some spurious anonymous union name change. * tests/data/test-diff-filter/test33-report-0.txt: Diff now completely empty. * tests/data/test-diff-pkg/elfutils-libs-0.170-4.el7.x86_64-multiple-sym-vers-report-0.txt: 3 functions previously considered to have harmless changes are now deemed to have no changes. * tests/data/test-diff-pkg/spice-server-0.12.4-19.el7.x86_64-0.12.8-1.el7.x86_64-report-2.txt: 1 struct RedStore data member previously considered to have harmless changes is now deemed to have no changes. * tests/data/test-read-dwarf/test9-pr18818-clang.so.abi: One instance of an anonymous struct removed and a typedef repointed at another existing instance; many type ids renumbered. Signed-off-by: Giuliano Procida --- src/abg-ir.cc | 25 +- .../PR25058-liblttng-ctl-report-1.txt | 4 - .../data/test-diff-filter/test33-report-0.txt | 3 - ....el7.x86_64-multiple-sym-vers-report-0.txt | 2 +- ...l7.x86_64-0.12.8-1.el7.x86_64-report-2.txt | 2 +- .../test9-pr18818-clang.so.abi | 236 ++++++++---------- 6 files changed, 127 insertions(+), 145 deletions(-) diff --git a/src/abg-ir.cc b/src/abg-ir.cc index 1fe6f499..aa2a56fa 100644 --- a/src/abg-ir.cc +++ b/src/abg-ir.cc @@ -4067,26 +4067,33 @@ equals(const decl_base& l, const decl_base& r, change_kind* k) /// Otherwise, let's just compare their name, the obvious way. /// That's the fast path because in that case the names are /// interned_string and comparing them is much faster. - bool decls_are_different = (ln != rn); - if (decls_are_different + bool decls_are_same = (ln == rn); + if (!decls_are_same && l.get_is_anonymous() && !l.get_has_anonymous_parent() && r.get_is_anonymous() && !r.get_has_anonymous_parent()) - // Both decls are anonymous and their scope are *NOT* anonymous. - // So we consider the decls to have equivalent names (both - // anonymous, remember). We are still in the fast path here. - decls_are_different = false; + { + // Both decls are anonymous and their scope are *NOT* anonymous. + // So we consider the decls to have equivalent names (both + // anonymous, remember). We are still in the fast path here. + // + // TODO: Don't conflate anonymous structs, unions and enums? + // + // TODO: Should we really be conflating all foo1::..::fooM::anon + // with all bar1::..::barN::anon? + decls_are_same = true; + } - if (decls_are_different + if (!decls_are_same && l.get_has_anonymous_parent() && r.get_has_anonymous_parent()) // This is the slow path as we are comparing the decl qualified // names component by component, properly handling anonymous // scopes. - decls_are_different = tools_utils::decl_names_equal(ln, rn); + decls_are_same = tools_utils::decl_names_equal(ln, rn); - if (decls_are_different) + if (!decls_are_same) { result = false; if (k) diff --git a/tests/data/test-diff-dwarf/PR25058-liblttng-ctl-report-1.txt b/tests/data/test-diff-dwarf/PR25058-liblttng-ctl-report-1.txt index dc1dff32..71df361e 100644 --- a/tests/data/test-diff-dwarf/PR25058-liblttng-ctl-report-1.txt +++ b/tests/data/test-diff-dwarf/PR25058-liblttng-ctl-report-1.txt @@ -130,10 +130,6 @@ Variables changes summary: 0 Removed, 0 Changed, 3 Added variables type of 'union {lttng_event_perf_counter_ctx perf_counter; struct {char* provider_name; char* ctx_name;} app_ctx; char padding[288];} lttng_event_context::u' changed: type name changed from '__anonymous_union__4' to '__anonymous_union__5' type size hasn't changed - 3 data member changes: - name of '__anonymous_union__4::app_ctx' changed to '__anonymous_union__5::app_ctx' - name of '__anonymous_union__4::padding' changed to '__anonymous_union__5::padding' - name of '__anonymous_union__4::perf_counter' changed to '__anonymous_union__5::perf_counter' type changed from: union {lttng_event_perf_counter_ctx perf_counter; struct {char* provider_name; char* ctx_name;} app_ctx; char padding[288];} to: diff --git a/tests/data/test-diff-filter/test33-report-0.txt b/tests/data/test-diff-filter/test33-report-0.txt index 433f9508..e69de29b 100644 --- a/tests/data/test-diff-filter/test33-report-0.txt +++ b/tests/data/test-diff-filter/test33-report-0.txt @@ -1,3 +0,0 @@ -Functions changes summary: 0 Removed, 0 Changed (16 filtered out), 0 Added functions -Variables changes summary: 0 Removed, 0 Changed, 0 Added variable - diff --git a/tests/data/test-diff-pkg/elfutils-libs-0.170-4.el7.x86_64-multiple-sym-vers-report-0.txt b/tests/data/test-diff-pkg/elfutils-libs-0.170-4.el7.x86_64-multiple-sym-vers-report-0.txt index 8ce5aa3c..9d1f078d 100644 --- a/tests/data/test-diff-pkg/elfutils-libs-0.170-4.el7.x86_64-multiple-sym-vers-report-0.txt +++ b/tests/data/test-diff-pkg/elfutils-libs-0.170-4.el7.x86_64-multiple-sym-vers-report-0.txt @@ -1,5 +1,5 @@ ================ changes of 'libdw-0.170.so'=============== - Functions changes summary: 0 Removed, 0 Changed (178 filtered out), 4 Added functions + Functions changes summary: 0 Removed, 0 Changed (175 filtered out), 4 Added functions Variables changes summary: 0 Removed, 0 Changed, 0 Added variable 4 Added functions: diff --git a/tests/data/test-diff-pkg/spice-server-0.12.4-19.el7.x86_64-0.12.8-1.el7.x86_64-report-2.txt b/tests/data/test-diff-pkg/spice-server-0.12.4-19.el7.x86_64-0.12.8-1.el7.x86_64-report-2.txt index c1aff1c7..a0bfc761 100644 --- a/tests/data/test-diff-pkg/spice-server-0.12.4-19.el7.x86_64-0.12.8-1.el7.x86_64-report-2.txt +++ b/tests/data/test-diff-pkg/spice-server-0.12.4-19.el7.x86_64-0.12.8-1.el7.x86_64-report-2.txt @@ -630,7 +630,7 @@ in pointed to type 'typedef SpiceServer' at spice-server.h:38:1: underlying type 'struct RedsState' at reds-private.h:127:1 changed: type size hasn't changed - 5 data member changes (2 filtered): + 5 data member changes (1 filtered): type of 'SpiceWatch* RedsState::listen_watch' changed: pointed to type 'typedef SpiceWatch' changed at spice.h:61:1, as reported earlier type of 'SpiceWatch* RedsState::secure_listen_watch' changed: diff --git a/tests/data/test-read-dwarf/test9-pr18818-clang.so.abi b/tests/data/test-read-dwarf/test9-pr18818-clang.so.abi index 8254692e..6ad6cc9c 100644 --- a/tests/data/test-read-dwarf/test9-pr18818-clang.so.abi +++ b/tests/data/test-read-dwarf/test9-pr18818-clang.so.abi @@ -6313,7 +6313,7 @@ - + @@ -6348,7 +6348,7 @@ - + @@ -6469,24 +6469,6 @@ - - - - - - - - - - - - - - - - - - @@ -6526,17 +6508,17 @@ - - - - - - - - - - - + + + + + + + + + + + @@ -6548,151 +6530,151 @@ - + - + - + - + - + - + - + - + - + - + - + - + - + - - + + - - + + - - + + - - + + - + - + - - + + - + - + - - - + + + - - - + + + - - - + + + - - - + + + - - - + + + - - + + - - + + - + @@ -6705,117 +6687,117 @@ - + - - - - - - + + + + + + + - - - - + + + - + - + - - + + - - + + - + - + - + - - + + - + - + - - + + - + - - + + - + - + - + - + - - + + @@ -6824,7 +6806,7 @@ - + @@ -6833,18 +6815,18 @@ - + - + - + - + @@ -6856,19 +6838,19 @@ - + - + - + - + - + @@ -6880,7 +6862,7 @@ - + @@ -6901,7 +6883,7 @@ - + @@ -6949,13 +6931,13 @@ - + - +