From patchwork Wed Oct 4 10:30:18 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Dodji Seketeli X-Patchwork-Id: 77089 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 0713C385800C for ; Wed, 4 Oct 2023 10:30:32 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 0713C385800C DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1696415432; bh=o4dnRMYODLL7KFWYjyv6T3vx/ab7CrYQOGqKaGVBmiE=; h=To:Subject:Date:List-Id:List-Unsubscribe:List-Archive:List-Help: List-Subscribe:From:Reply-To:From; b=PlXhFbgk3x8AQ0YjKR5NLuQFhkMka4sopt8EkDoJlSUGnEWa+9bYCQPDz3URfNLaW GQf58Z/PPAo4/BkLYZZtft95NEo6b4T0s44XnysBNu2QCn3l2OoZxxn97xkZj6cqk9 //5csi735WJEy+muANLYodHzw/JCU0JlLlWaUXuY= X-Original-To: libabigail@sourceware.org Delivered-To: libabigail@sourceware.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTPS id BBEF8385841B for ; Wed, 4 Oct 2023 10:30:24 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org BBEF8385841B Received: from mail-qv1-f72.google.com (mail-qv1-f72.google.com [209.85.219.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-440-uVTirKL7O0KTCZzrxROO2w-1; Wed, 04 Oct 2023 06:30:23 -0400 X-MC-Unique: uVTirKL7O0KTCZzrxROO2w-1 Received: by mail-qv1-f72.google.com with SMTP id 6a1803df08f44-65b08994e15so11552946d6.0 for ; Wed, 04 Oct 2023 03:30:23 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1696415422; x=1697020222; h=mime-version:user-agent:message-id:date:organization:subject:to :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=o4dnRMYODLL7KFWYjyv6T3vx/ab7CrYQOGqKaGVBmiE=; b=SDKJaHQlsUJWQe7L0D39XG86PQhh0DMca7CHcIS7kzcRF99oQgBu8j2DS0djx9mCYo relppGdbofMJdak0TxDYrot6vhHlQE/rvwVvA4/lTxhZPGcNv7S006k7QnyK2i5HiFiJ cV5KHoAvxWsjIHkSowL5KZzxnut1iseNTiwdat4EqRcO6TNTXF7TZhHGv4++Sd7z+eQ8 8MFhs9+XBMI8CTuj726EPVHVOeT2um08I2un/UD8x3EHqM+3GVVIldKyo1I6S+6tM/EU HsSB6+cnqhkwQdDT4xFg/T8STYa7rxIVssHB0n5vJqVY9oYSTSDsTvKNXt4JXHxVeVXv 5Zlw== X-Gm-Message-State: AOJu0YwBCtGLn+6HpwLfrkrS3DCAhcEk2JlsBO9ZNvjpqlUeWUXipZPZ 6xbPqqZ9AVOR1wN30cMX3dcx1ZDuxDKmMBIZxmh3f9Uv/fcWNU39VehTeYzGOipH6zYOq8LD7/M C2HgsoHiYbMSrj9Bd2XFU3Fhzz+xLOomjuzw29rnJ9f2fvR2k+Y3R6YpQAy9+4vGSBiw9a5dLk4 ck X-Received: by 2002:a0c:b395:0:b0:653:589b:ac47 with SMTP id t21-20020a0cb395000000b00653589bac47mr1899786qve.18.1696415422358; Wed, 04 Oct 2023 03:30:22 -0700 (PDT) X-Google-Smtp-Source: AGHT+IENHC+oRHchArBRAATqDrdJdvcCe/0W95DVneW0UdFxv9sag1QwwPtIZEywTEfrb4D+ZxHHnA== X-Received: by 2002:a0c:b395:0:b0:653:589b:ac47 with SMTP id t21-20020a0cb395000000b00653589bac47mr1899757qve.18.1696415421634; Wed, 04 Oct 2023 03:30:21 -0700 (PDT) Received: from localhost ([88.120.130.27]) by smtp.gmail.com with ESMTPSA id n20-20020a0ce554000000b00656e2464719sm1234212qvm.92.2023.10.04.03.30.21 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 04 Oct 2023 03:30:21 -0700 (PDT) Received: by localhost (Postfix, from userid 1000) id A35BDB6EB1; Wed, 4 Oct 2023 12:30:18 +0200 (CEST) To: libabigail@sourceware.org Subject: [PATCH, applied] abg-comparison[-priv]: Better detection of incompatible unreachable type changes Organization: Red Hat / France X-Operating-System: CentOS Stream release 9 X-URL: http://www.redhat.com Date: Wed, 04 Oct 2023 12:30:18 +0200 Message-ID: <87cyxur879.fsf@redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.2 (gnu/linux) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-Spam-Status: No, score=-11.9 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: libabigail@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Mailing list of the Libabigail project List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-Patchwork-Original-From: Dodji Seketeli via Libabigail From: Dodji Seketeli Reply-To: Dodji Seketeli Errors-To: libabigail-bounces+patchwork=sourceware.org@sourceware.org Sender: "Libabigail" Hello, Whenever there is a change in unreachable type, corpus_diff::has_incompatible_changes always reports that the corresponding diff node carries an incompatible change. It does this even if the change is known to be compatible. To be able to say if a diff node for a unreachable type carries a compatible (not incompatible) change, corpus_diff::has_incompatible_changes must look at the change category of that diff node, instead of saying that any change to an unreachable type is incompatible. While looking at this, I noted that corpus_diff::priv::apply_filters_and_compute_diff_stats doesn't categorize the diffs in corpus_diff::priv::changed_unreachable_types_, so it's not possible to look at the categories of the changes held by that data member to see if they are incompatible or not. This patch thus categorizes the diff nodes held by corpus_diff::priv::changed_unreachable_types_ and makes corpus_diff::has_incompatible_changes look at those diff nodes to detect if they are incompatible. Let's see the result of this patch. Consider the change in the input test source code from included in this patch, from test-enumerator-changes1-v0.c to test-enumerator-changes1-v1.c: $ diff -u test-enumerator-changes1-v0.c test-enumerator-changes1-v1.c --- test-enumerator-changes1-v0.c 2023-10-04 11:25:30.722989530 +0200 +++ test-enumerator-changes1-v1.c 2023-10-04 11:25:30.722989530 +0200 @@ -1,13 +1,14 @@ /* * * Compile this with: - * gcc -g -c -fno-eliminate-unused-debug-types test-enumerator-changes1-v0.c + * gcc -g -c -fno-eliminate-unused-debug-types test-enumerator-changes1-v1.c */ enum foo { E1_O, - E1_1 + E1_1, + E1_2 }; void $ The enumerator E1_2 has been added to the 'foo' enum. Now, let's see what abidiff prior to this patch would say about the change between the two result binaries test-enumerator-changes1-v0.o and test-enumerator-changes1-v1.o: $ abidiff --non-reachable-types --harmless test-enumerator-changes1-v0.o test-enumerator-changes1-v1.o || echo "return value: $?" Functions changes summary: 0 Removed, 0 Changed, 0 Added function Variables changes summary: 0 Removed, 0 Changed, 0 Added variable Unreachable types summary: 0 removed, 1 changed, 0 added type 1 changed type unreachable from any public interface: [C] 'enum foo' changed: type size hasn't changed 1 enumerator insertion: 'foo::E1_2' value '2' return value: 12 $ See the return value of 12, that is actually the bits abigail::tools_utils::ABIDIFF_ABI_CHANGE (of value 4) and abigail::tools_utils::ABIDIFF_ABI_INCOMPATIBLE_CHANGE (of value 8) being set. Normally, only the bit abigail::tools_utils::ABIDIFF_ABI_CHANGE (of value 4) should be set. Now, let's look at what abidiff says with this patch: $ abidiff --non-reachable-types --harmless test-enumerator-changes1-v0.o test-enumerator-changes1-v1.o || echo "return value: $?" Functions changes summary: 0 Removed, 0 Changed, 0 Added function Variables changes summary: 0 Removed, 0 Changed, 0 Added variable Unreachable types summary: 0 removed, 1 changed, 0 added type 1 changed type unreachable from any public interface: [C] 'enum foo' changed: type size hasn't changed 1 enumerator insertion: 'foo::E1_2' value '2' return value: 4 $ Now the return value is 4, which is the bit abigail::tools_utils::ABIDIFF_ABI_CHANGE being set, as we would expect because that change is known to be not incompatible. Please note that this patch originates from the patch set on the branch "better-anon-enums" to work on the support for the UAPI checker tool tracked by https://sourceware.org/PR30931. It's being applied to the master branch as it turns out it's quite independent from that task. * Src/abg-comparison-priv.h (corpus_diff::priv::changed_unreachable_types): Declare ... * src/abg-comparison.cc (corpus_diff::priv::changed_unreachable_types): ... new function. (corpus_diff::priv::apply_filters_and_compute_diff_stats): Walk the nodes returned by corpus_diff:priv::changed_unreachable_types and apply the filters (including categorization filters) to them. Also make the loop similarly applying filters to the nodes returned by corpus_diff::priv::changed_unreachable_types_sorted be a ranged-based one, for the sake of consistency. (corpus_diff::has_incompatible_changes): Now that diff nodes returned by corpus_diff::priv::changed_unreachable_types are categorized, look at their change categories to see if they are incompatible or not. * tests/data/test-abidiff-exit/test-enumerator-changes1-report-1.txt: New test output reference. * tests/data/test-abidiff-exit/test-enumerator-changes1-v{0,1}.o: New test input binaries. * tests/data/test-abidiff-exit/test-enumerator-changes1-v{0,1}.c: New source code for the new test input binaries. * tests/data/Makefile.am: Add the new test material above to source distribution. * tests/test-abidiff-exit.cc (in_out_specs): Add the new test input binaries to the test harness. Signed-off-by: Dodji Seketeli Applied to master. --- src/abg-comparison-priv.h | 3 + src/abg-comparison.cc | 54 ++++++++++++++---- tests/data/Makefile.am | 5 ++ .../test-enumerator-changes1-report-1.txt | 11 ++++ .../test-enumerator-changes1-v0.c | 16 ++++++ .../test-enumerator-changes1-v0.o | Bin 0 -> 3144 bytes .../test-enumerator-changes1-v1.c | 17 ++++++ .../test-enumerator-changes1-v1.o | Bin 0 -> 3176 bytes tests/test-abidiff-exit.cc | 15 +++++ 9 files changed, 110 insertions(+), 11 deletions(-) create mode 100644 tests/data/test-abidiff-exit/test-enumerator-changes1-report-1.txt create mode 100644 tests/data/test-abidiff-exit/test-enumerator-changes1-v0.c create mode 100644 tests/data/test-abidiff-exit/test-enumerator-changes1-v0.o create mode 100644 tests/data/test-abidiff-exit/test-enumerator-changes1-v1.c create mode 100644 tests/data/test-abidiff-exit/test-enumerator-changes1-v1.o new file mode 100644 index 00000000..abc1d1b7 index e539242a..fb3d6fff 100644 diff --git a/src/abg-comparison-priv.h b/src/abg-comparison-priv.h index 481fc9c6..9802e78f 100644 --- a/src/abg-comparison-priv.h +++ b/src/abg-comparison-priv.h @@ -1159,6 +1159,9 @@ struct corpus_diff::priv size_t &num_filtered_removed, size_t &num_filtered_changed); + const string_diff_sptr_map& + changed_unreachable_types() const; + const vector& changed_unreachable_types_sorted() const; diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc index 1cfc0952..1207729c 100644 --- a/src/abg-comparison.cc +++ b/src/abg-comparison.cc @@ -10129,6 +10129,14 @@ corpus_diff::priv::count_unreachable_types(size_t &num_added, ++num_filtered_changed; } +/// Get the map of diff nodes representing changed unreachable types. +/// +/// @return the map of diff nodes representing changed unreachable +/// types. +const string_diff_sptr_map& +corpus_diff::priv::changed_unreachable_types() const +{return changed_unreachable_types_;} + /// Get the sorted vector of diff nodes representing changed /// unreachable types. /// @@ -10239,14 +10247,11 @@ corpus_diff::priv::apply_filters_and_compute_diff_stats(diff_stats& stat) // walk the changed unreachable types to apply categorization // filters - for (diff_sptrs_type::const_iterator i = - changed_unreachable_types_sorted().begin(); - i != changed_unreachable_types_sorted().end(); - ++i) - { - diff_sptr diff = *i; - ctxt->maybe_apply_filters(diff); - } + for (auto& diff : changed_unreachable_types_sorted()) + ctxt->maybe_apply_filters(diff); + + for (auto& entry : changed_unreachable_types()) + ctxt->maybe_apply_filters(entry.second); if (get_context()->do_log()) { @@ -11194,7 +11199,8 @@ corpus_diff::has_incompatible_changes() const const diff_stats& stats = const_cast(this)-> apply_filters_and_suppressions_before_reporting(); - return (soname_changed() || architecture_changed() + bool has_incompatible_changes = + (soname_changed() || architecture_changed() || stats.net_num_func_removed() != 0 || (stats.num_func_with_virtual_offset_changes() != 0 // If all reports about functions with sub-type changes @@ -11205,8 +11211,34 @@ corpus_diff::has_incompatible_changes() const || stats.net_num_vars_removed() != 0 || stats.net_num_removed_func_syms() != 0 || stats.net_num_removed_var_syms() != 0 - || stats.net_num_removed_unreachable_types() != 0 - || stats.net_num_changed_unreachable_types() != 0); + || stats.net_num_removed_unreachable_types() != 0); + + // If stats.net_num_changed_unreachable_types() != 0 then walk the + // corpus_diff::priv::changed_unreachable_types_, and see if there + // is one that is harmful by bitwise and-ing their category with + // abigail::comparison::get_default_harmful_categories_bitmap(). + if (!has_incompatible_changes + && stats.net_num_changed_unreachable_types()) + { + // The changed unreachable types can carry harmful changes. + // Let's figure if they actually do. + + diff_context_sptr ctxt = context(); + for (auto &entry : priv_->changed_unreachable_types()) + { + diff_sptr dif = entry.second; + + // Let's see if any of the categories of this diff node + // belong to the "harmful" ones. + if (dif->get_category() & get_default_harmful_categories_bitmap()) + { + has_incompatible_changes |= true; + break; + } + } + } + + return has_incompatible_changes; } /// Test if the current instance of @ref corpus_diff carries subtype diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am index b3434da5..0569af34 100644 --- a/tests/data/Makefile.am +++ b/tests/data/Makefile.am @@ -351,6 +351,11 @@ test-abidiff-exit/test-PR30034/split/lib64/librte_telemetry.so \ test-abidiff-exit/test-PR30034/split/lib64/librte_telemetry.so.23 \ test-abidiff-exit/test-PR30034/split/lib64/librte_telemetry.so.23.2 \ test-abidiff-exit/test-PR30034/test-PR30034-report-1.txt \ +test-abidiff-exit/test-enumerator-changes1-report-1.txt \ +test-abidiff-exit/test-enumerator-changes1-v0.c \ +test-abidiff-exit/test-enumerator-changes1-v0.o \ +test-abidiff-exit/test-enumerator-changes1-v1.c \ +test-abidiff-exit/test-enumerator-changes1-v1.o \ \ test-diff-dwarf/test0-v0.cc \ test-diff-dwarf/test0-v0.o \ diff --git a/tests/data/test-abidiff-exit/test-enumerator-changes1-report-1.txt b/tests/data/test-abidiff-exit/test-enumerator-changes1-report-1.txt new file mode 100644 index 00000000..cf1c3c82 --- /dev/null +++ b/tests/data/test-abidiff-exit/test-enumerator-changes1-report-1.txt @@ -0,0 +1,11 @@ +Functions changes summary: 0 Removed, 0 Changed, 0 Added function +Variables changes summary: 0 Removed, 0 Changed, 0 Added variable +Unreachable types summary: 0 removed, 1 changed, 0 added type + +1 changed type unreachable from any public interface: + + [C] 'enum foo' changed: + type size hasn't changed + 1 enumerator insertion: + 'foo::E1_2' value '2' + diff --git a/tests/data/test-abidiff-exit/test-enumerator-changes1-v0.c b/tests/data/test-abidiff-exit/test-enumerator-changes1-v0.c new file mode 100644 index 00000000..b02c3a07 --- /dev/null +++ b/tests/data/test-abidiff-exit/test-enumerator-changes1-v0.c @@ -0,0 +1,16 @@ +/* + * + * Compile this with: + * gcc -g -c -fno-eliminate-unused-debug-types test-enumerator-changes1-v0.c + */ + +enum foo + { + E1_O, + E1_1 + }; + +void +foo() +{ +} diff --git a/tests/data/test-abidiff-exit/test-enumerator-changes1-v0.o b/tests/data/test-abidiff-exit/test-enumerator-changes1-v0.o new file mode 100644 index 0000000000000000000000000000000000000000..c7cc06e9f50601f5f42e43a63599f8255ec89ab6 GIT binary patch literal 3144 zcmb_d&2Jl35TEC@H%h+=Q++}lOkZGeKYf$nK$#kW_s9BLW`*ef2&btTr}8CZh5dtd&1@bORg5GsI}B}O`)C)% z8O4;VN-Q9sSI!0U5~WEsdIs?gh*hvk=WWcjATE@aY^%IdUJ#4L`R4$vys&1sZQFit zY16)Hzh#Rhfc&dq+uOEP!ZJO0`P5>76kQdN* z4Y?5FNokH4Rf``AWmR}ilyhgt)2g_FfKwCIDv2983!Sgm8Z{V(Nze^t+X=!H5cv>x zKe+4c)ZTPzHSe-lbJnZtuU9v!Z#b*B(QD67+2F2Ua@;{W4CPi=hB6LXsQPiMw>7%5 z;ci@Z_tzb_>$sgTa%Ddl1ficwcNh*6*>>BqIqbUWgZnapR3@n_!{I>2ej3GYtLKMZ znbh3*g0_z_!H}mQ>QeN## zuJgzEzs$H^SC4h-l0+K+eyl=`xyB!}ewDddkHa|a&Gi0&mpmAxeiL~bk9ki^sT`%? zQBMJ{nIz!FvhRDUfi;4#6QPZoS;KEOW4V9C?8mB;9l`oRD0K*?&?3PNrMIHNKw>ZU z!YGv<_6avh{nkC6MD`k;*dIt<$bV!j7*Bho&q+^LXJK-`Sts9VdI=T6%nqRM&$KVe zX8c9O=sy12cshS`PJ}?`=VwQdBGZ@d6?Lh)vK|h9aO?Vwp#|gw>9@1Y_!e?=PBQ(- z_B+fqGkxhjnHc$=X0WOQnVY^h5YMXr7k+Fyp6aKwH0y68X3mKaY%+M0lqoWO8)(bQ zzswiv6ON_))L+c}_YtExb)6_Gdc=vo&;WCtU-Qop&x)Vp$NlS5;=ej3{t3rdo)Z5x z#~W_2W?pD^&~PQQFG8%!@mI7XbG?5W{uYI-{U7oE>w8N3Pk%WbuW_o!%#9HA rIDd}|rvC!{Jj`?LC6y0WZ#;%#{vC{q z;+#^7stQk=$w5A^QVPTs3$vc|IN}=+Yhabn*;sK=JXc<}t?FuZQ7jcJj{#VDVU@OR z+kSodvVFyV*%r$H`Deklw{5G8&3f?4S*tXSQBrf|&RJ~Y1$@*5K+Q$9MK4`;2C%V* zx`5mZsD%)Jl;=rNxA>q@RgLFFHMcMwR^=50oSM1Tg1DZukbJG)s>39VgMKKxP7o%5 z$Xl@c<}GKZ{*qI#dl$XB(^zl3xW2J|$yvLJtUW*BfZI6lxT9ne%B{W(WfXMK^`p*U z>u__!-MHu;G#t0@xV>=f%3&}HLO+r2B%H*u>vm;((sz^F`!c4Y4M=31xH6oKWaKB~ z$n6aLurK4fdrjT;@hLd zZ=+74sUB!6k%tL*bkTsLJHlOz291|hX^(W+{#N^VsqT8#P;wY03Yl#yU5hR}e zrdF80h1wjGLO*f*UDjHe+_y1cV#KX5xULgfo80S&XZ2s=4b}P7KkcR2e-|-xOoX7x z;0>-t)Mf_PT`3V+N5* C@d`Eo literal 0 HcmV?d00001 diff --git a/tests/test-abidiff-exit.cc b/tests/test-abidiff-exit.cc --- a/tests/test-abidiff-exit.cc +++ b/tests/test-abidiff-exit.cc @@ -1017,6 +1017,21 @@ InOutSpec in_out_specs[] = "data/test-abidiff-exit/test-PR30034/test-PR30034-report-1.txt", "output/test-abidiff-exit/test-PR30034/test-PR30034-report-1.txt" }, + { + "data/test-abidiff-exit/test-enumerator-changes1-v0.o", + "data/test-abidiff-exit/test-enumerator-changes1-v1.o", + "", + "", + "", + "", + "", + "", + "", + "--no-default-suppression --non-reachable-types --harmless", + abigail::tools_utils::ABIDIFF_ABI_CHANGE, + "data/test-abidiff-exit/test-enumerator-changes1-report-1.txt", + "output/test-abidiff-exit/test-enumerator-changes1-report-1.txt" + }, #ifdef WITH_BTF { "data/test-abidiff-exit/btf/test0-v0.o",