From: "dodji at seketeli dot org" <sourceware-bugzilla@sourceware.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 [thread overview]
Message-ID: <bug-31513-9487-ME81w4ivGW@http.sourceware.org/bugzilla/> (raw)
In-Reply-To: <bug-31513-9487@http.sourceware.org/bugzilla/>
https://sourceware.org/bugzilla/show_bug.cgi?id=31513
--- Comment #15 from Dodji Seketeli <dodji at seketeli dot org> ---
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 <dodji@redhat.com>
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 <dodji@redhat.com>
---
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"
},
--
You are receiving this mail because:
You are on the CC list for the bug.
next prev parent reply other threads:[~2024-04-03 9:58 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-20 5:57 [Bug default/31513] New: abidiff differences due to change in compiler version quic_ashudas at quicinc dot com
2024-03-20 10:22 ` [Bug default/31513] " dodji at redhat dot com
2024-03-20 10:53 ` quic_ashudas at quicinc dot com
2024-03-20 10:54 ` quic_ashudas at quicinc dot com
2024-03-20 10:54 ` quic_ashudas at quicinc dot com
2024-03-21 10:19 ` Dodji Seketeli
2024-03-20 19:14 ` woodard at redhat dot com
2024-03-21 6:51 ` quic_ashudas at quicinc dot com
2024-03-21 10:19 ` dodji at seketeli dot org
2024-03-22 6:36 ` quic_ashudas at quicinc dot com
2024-03-22 6:36 ` quic_ashudas at quicinc dot com
2024-03-22 9:20 ` dodji at redhat dot com
2024-03-22 9:20 ` dodji at redhat dot com
2024-03-28 16:41 ` [Bug default/31513] abidiff wrongly considers data members moved to base class as harmful dodji at seketeli dot org
2024-03-29 17:40 ` dodji at seketeli dot org
2024-04-01 14:28 ` quic_ashudas at quicinc dot com
2024-04-02 9:54 ` dodji at seketeli dot org
2024-04-02 13:16 ` quic_jiafan at quicinc dot com
2024-04-03 9:58 ` Dodji Seketeli
2024-04-03 9:58 ` dodji at seketeli dot org [this message]
2024-04-03 10:22 ` quic_jiafan at quicinc dot com
2024-04-03 16:26 ` dodji at seketeli dot org
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=bug-31513-9487-ME81w4ivGW@http.sourceware.org/bugzilla/ \
--to=sourceware-bugzilla@sourceware.org \
--cc=libabigail@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).