From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from relay4-d.mail.gandi.net (relay4-d.mail.gandi.net [IPv6:2001:4b98:dc4:8::224]) by sourceware.org (Postfix) with ESMTPS id CC3A53847700; Wed, 3 Apr 2024 09:58:25 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org CC3A53847700 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=seketeli.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=seketeli.org ARC-Filter: OpenARC Filter v1.0.0 sourceware.org CC3A53847700 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2001:4b98:dc4:8::224 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1712138309; cv=none; b=fej5LgtUwOwZNgYaoW5M6Xx3ZWTRWo8XQlh54RuTUfyvIRWz8XECI5hbD2NypvFD5qQ3406p6X+yRw+a95qGhakjRG1307L/KejhxGE6l6y6TwSLzqxUlgrnWGNT7pvQz6uPLVg/Wd0Hsg25QQTdRPq+m5Zvo/VYnxNOgn3mxeI= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1712138309; c=relaxed/simple; bh=yCgYbzQuqC1Au/a3P4ncugUb6dr11weqv3SnhHu4eOY=; h=From:To:Subject:Date:Message-ID:MIME-Version; b=VjrkfgyIpFzNjJjoinqVJKv7QXRJUc5g2QlNiWYhq6SiiGLxhGmwXuY9MnzHimw4aTK/3t8HgG0Og1TKQ6YZN+u7vuqvs1ccL/FKYbPrqM4/MYyXEOwZMQnEZPpTUJwLp3K7sKQKC5H0mZMkWyfW6lJtYdSkeYpGeF4beppTtaE= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail.gandi.net (Postfix) with ESMTPSA id 68D52E000E; Wed, 3 Apr 2024 09:58:23 +0000 (UTC) Received: by localhost (Postfix, from userid 1000) id A6E6AC1B73C3; Wed, 3 Apr 2024 11:58:22 +0200 (CEST) From: Dodji Seketeli To: "quic_jiafan at quicinc dot com" Cc: libabigail@sourceware.org, quic_jiafan@quicinc.com Subject: Re: [Bug default/31513] abidiff wrongly considers data members moved to base class as harmful Organization: Me, myself and I References: X-Operating-System: AlmaLinux 9.3 X-URL: http://www.seketeli.net/~dodji Date: Wed, 03 Apr 2024 11:58:22 +0200 In-Reply-To: (quic jiafan at quicinc dot com's message of "Tue, 02 Apr 2024 13:16:32 +0000") Message-ID: <87ttkilqdd.fsf@seketeli.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.2 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-GND-Sasl: dodj@seketeli.org X-Spam-Status: No, score=-9.7 required=5.0 tests=BAYES_00,GIT_PATCH_0,JMQ_SPF_NEUTRAL,KAM_DMARC_STATUS,RCVD_IN_DNSWL_LOW,SPF_HELO_PASS,SPF_PASS,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 List-Id: 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" }, -- 2.39.3 -- Dodji