From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <3_x0YXwgKCpoAJLI6C74AIIAF8.6IGFC545CA4CFMIOL68Q4L8.ILA@flex--gprocida.bounces.google.com> 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 From: Giuliano Procida To: libabigail@sourceware.org Cc: dodji@seketeli.org, kernel-team@android.com, gprocida@google.com, maennich@google.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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-List-Received-Date: Wed, 22 Jul 2020 11:07:49 -0000 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-v= ers-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, chan= ge_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 =3D (ln !=3D rn); - if (decls_are_different + bool decls_are_same =3D (ln =3D=3D 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 =3D 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 =3D true; + } =20 - 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 =3D tools_utils::decl_names_equal(ln, rn); + decls_are_same =3D tools_utils::decl_names_equal(ln, rn); =20 - if (decls_are_different) + if (!decls_are_same) { result =3D 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 Add= ed variables type of 'union {lttng_event_perf_counter_ctx perf_counter; struc= t {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 '__anonym= ous_union__5::app_ctx' - name of '__anonymous_union__4::padding' changed to '__anonym= ous_union__5::padding' - name of '__anonymous_union__4::perf_counter' changed to '__a= nonymous_union__5::perf_counter' type changed from: union {lttng_event_perf_counter_ctx perf_counter; struct {ch= ar* 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/t= est-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-mult= iple-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-sy= m-vers-report-0.txt +++ b/tests/data/test-diff-pkg/elfutils-libs-0.170-4.el7.x86_64-multiple-sy= m-vers-report-0.txt @@ -1,5 +1,5 @@ =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D changes of 'libdw-0.170.s= o'=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D - Functions changes summary: 0 Removed, 0 Changed (178 filtered out), 4 Ad= ded functions + Functions changes summary: 0 Removed, 0 Changed (175 filtered out), 4 Ad= ded functions Variables changes summary: 0 Removed, 0 Changed, 0 Added variable =20 4 Added functions: diff --git a/tests/data/test-diff-pkg/spice-server-0.12.4-19.el7.x86_64-0.1= 2.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.e= l7.x86_64-report-2.txt +++ b/tests/data/test-diff-pkg/spice-server-0.12.4-19.el7.x86_64-0.12.8-1.e= l7.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 chang= ed: 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 @@ - + - + --=20 2.28.0.rc0.105.gf9edc3c819-goog