From patchwork Wed Apr 3 09:58:30 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: dodji at seketeli dot org X-Patchwork-Id: 87960 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 079783847718 for ; Wed, 3 Apr 2024 09:58:43 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 079783847718 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1712138323; bh=g7FLElov5WhuQcKSq2RKhzb64Fh4Q8nprGMtad5kVh0=; h=From:To:Subject:Date:In-Reply-To:References:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=X950mp0pduYSEwb3Lf049d71fwDgkI1Y4zMjfUKcHImuKXff33Kf9JZJVUcliXPfa vDzk4nru6lzBk/IQNtcPb55fQ7k6J6nBQN4YawcBLKNMWYdNJNMx0t4rmQ93SH7jNy mfiFfD9uhH5/sWgF/BGVOngGO3lk9radoqj7ealE= X-Original-To: libabigail@sourceware.org Delivered-To: libabigail@sourceware.org Received: by sourceware.org (Postfix, from userid 48) id 20AE43847725; Wed, 3 Apr 2024 09:58:36 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 20AE43847725 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1712138316; bh=g7FLElov5WhuQcKSq2RKhzb64Fh4Q8nprGMtad5kVh0=; h=From:To:Subject:Date:In-Reply-To:References:From; b=SDHNFHGqEC11H3Dkc1eI7HzmjhYRjEQiu2GUxIa+6J9HB5FiYvvEifRt/YSMgUX3I u7pCY5a3oO2grKOmX7RI0Y95Hg6Fhd3L2+AbDZ2uamrwCuePUnXNRtVGzmumLta2uJ FfwCy4tLQjpDVTbNI2bqlxwGDob9ZqzEpfCpVyKQ= From: "dodji at seketeli dot org" To: libabigail@sourceware.org Subject: [Bug default/31513] abidiff wrongly considers data members moved to base class as harmful Date: Wed, 03 Apr 2024 09:58:30 +0000 X-Bugzilla-Reason: CC X-Bugzilla-Type: changed X-Bugzilla-Watch-Reason: None X-Bugzilla-Product: libabigail X-Bugzilla-Component: default X-Bugzilla-Version: unspecified X-Bugzilla-Keywords: X-Bugzilla-Severity: normal X-Bugzilla-Who: dodji at seketeli dot org X-Bugzilla-Status: RESOLVED X-Bugzilla-Resolution: FIXED X-Bugzilla-Priority: P2 X-Bugzilla-Assigned-To: dodji at seketeli dot org X-Bugzilla-Target-Milestone: --- X-Bugzilla-Flags: X-Bugzilla-Changed-Fields: Message-ID: In-Reply-To: References: X-Bugzilla-URL: http://sourceware.org/bugzilla/ Auto-Submitted: auto-generated MIME-Version: 1.0 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-Post: List-Help: List-Subscribe: , Errors-To: libabigail-bounces+patchwork=sourceware.org@sourceware.org https://sourceware.org/bugzilla/show_bug.cgi?id=31513 --- Comment #15 from Dodji Seketeli --- Hello Jianfeng, [...] > Added inline comments > static bool > has_subtype_changes(const string_decl_base_sptr_map& f_data_members, > const string_decl_base_sptr_map& s_data_members, > diff_context_sptr ctxt) > { > // Now compare the offsets of the data members collected. > var_decl_sptr s_member; > for (auto entry : f_data_members) > { > var_decl_sptr f_member = is_var_decl(entry.second); > ABG_ASSERT(f_member); > > auto i = s_data_members.find(entry.first); > if (i == s_data_members.end()) > { > unsigned offset = get_data_member_offset(f_member); > s_member = find_data_member_at_offset(s_data_members, offset); > if (!s_member) > // A data member was suppressed; that's bad; let's consider > // that as a sub-type change. > return true; > } > > if (!s_member) <<<<<<<<< we never reset s_member, so it will only be assigned once, each f_member compare to the first s_member. > s_member = is_var_decl(i->second); Ahah! Good catch. You are right! s_member should have been declared in the same scope as f_member. Please find below the patch that I am proposing to fix this, just like what is done in the has_offset_changes function. Note that you can get that patch from the branch https://sourceware.org/git/?p=libabigail.git;a=shortlog;h=refs/heads/users/dodji/try-PR31513. Here is the patch. What do you think about it? From 1d61a563334a4f15cd834c1de7a3041cbf9e3ac8 Mon Sep 17 00:00:00 2001 From: Dodji Seketeli Date: Wed, 3 Apr 2024 11:10:38 +0200 Subject: [PATCH] Bug 31513 - Fix fallout of initial patch As Jianfeng Fan pointed out in a comment at https://sourceware.org/bugzilla/show_bug.cgi?id=31513#c14, there is a thinko in commit https://sourceware.org/git/?p=libabigail.git;a=commit;h=338394f5454990c715b52bb4bc2ed47b39d6528b. has_subtype_changes forgets to reset the s_member when f_member is reset. Both variables should be reset in tandem, just like what is done in has_offset_changes. Fixed thus. * src/abg-comp-filter.cc (has_subtype_changes): Reset s_member in the loop, just like f_member. * tests/data/test-abidiff-exit/PR31513/non-regr/report1.txt: Adjust. * tests/data/test-abidiff-exit/PR31513/non-regr/report2.txt: Adjust. * tests/test-abidiff-exit.cc (in_out_specs): Adjust. Signed-off-by: Dodji Seketeli --- src/abg-comp-filter.cc | 2 +- .../test-abidiff-exit/PR31513/non-regr/report1.txt | 11 +---------- .../test-abidiff-exit/PR31513/non-regr/report2.txt | 11 +---------- tests/test-abidiff-exit.cc | 4 ++-- 4 files changed, 5 insertions(+), 23 deletions(-) diff --git a/src/abg-comp-filter.cc b/src/abg-comp-filter.cc index caea9d0c..2156fad0 100644 --- a/src/abg-comp-filter.cc +++ b/src/abg-comp-filter.cc @@ -331,12 +331,12 @@ has_subtype_changes(const string_decl_base_sptr_map& f_data_members, diff_context_sptr ctxt) { // Now compare the offsets of the data members collected. - var_decl_sptr s_member; for (auto entry : f_data_members) { var_decl_sptr f_member = is_var_decl(entry.second); ABG_ASSERT(f_member); + var_decl_sptr s_member; auto i = s_data_members.find(entry.first); if (i == s_data_members.end()) { diff --git a/tests/data/test-abidiff-exit/PR31513/non-regr/report1.txt b/tests/data/test-abidiff-exit/PR31513/non-regr/report1.txt index dff216f8..9666a8fd 100644 --- a/tests/data/test-abidiff-exit/PR31513/non-regr/report1.txt +++ b/tests/data/test-abidiff-exit/PR31513/non-regr/report1.txt @@ -1,12 +1,3 @@ -Functions changes summary: 0 Removed, 1 Changed, 0 Added function +Functions changes summary: 0 Removed, 0 Changed (1 filtered out), 0 Added function Variables changes summary: 0 Removed, 0 Changed, 0 Added variable -1 function with some indirect sub-type change: - - [C] 'function int foo(type&)' at test1-v0.cc:10:1 has some indirect sub-type changes: - parameter 1 of type 'type&' has sub-type changes: - in referenced type 'struct type' at test1-v1.cc:8:1: - type size hasn't changed - 1 base class insertion: - struct base at test1-v1.cc:2:1 - diff --git a/tests/data/test-abidiff-exit/PR31513/non-regr/report2.txt b/tests/data/test-abidiff-exit/PR31513/non-regr/report2.txt index 25d39e9b..9666a8fd 100644 --- a/tests/data/test-abidiff-exit/PR31513/non-regr/report2.txt +++ b/tests/data/test-abidiff-exit/PR31513/non-regr/report2.txt @@ -1,12 +1,3 @@ -Functions changes summary: 0 Removed, 1 Changed, 0 Added function +Functions changes summary: 0 Removed, 0 Changed (1 filtered out), 0 Added function Variables changes summary: 0 Removed, 0 Changed, 0 Added variable -1 function with some indirect sub-type change: - - [C] 'function int foo(type&)' at test2-v0.cc:10:1 has some indirect sub-type changes: - parameter 1 of type 'type&' has sub-type changes: - in referenced type 'struct type' at test2-v1.cc:9:1: - type size hasn't changed - 1 base class insertion: - struct base at test2-v1.cc:2:1 - diff --git a/tests/test-abidiff-exit.cc b/tests/test-abidiff-exit.cc index 104d0580..29df74ff 100644 --- a/tests/test-abidiff-exit.cc +++ b/tests/test-abidiff-exit.cc @@ -1331,7 +1331,7 @@ InOutSpec in_out_specs[] = "", "", "--no-default-suppression", - abigail::tools_utils::ABIDIFF_ABI_CHANGE, + abigail::tools_utils::ABIDIFF_OK, "data/test-abidiff-exit/PR31513/non-regr/report1.txt", "output/test-abidiff-exit/PR31513/non-regr/report1.txt" }, @@ -1346,7 +1346,7 @@ InOutSpec in_out_specs[] = "", "", "--no-default-suppression", - abigail::tools_utils::ABIDIFF_ABI_CHANGE, + abigail::tools_utils::ABIDIFF_OK, "data/test-abidiff-exit/PR31513/non-regr/report2.txt", "output/test-abidiff-exit/PR31513/non-regr/report2.txt" },