Re: [Bug default/31513] abidiff wrongly considers data members moved to base class as harmful

[Dodji Seketeli <dodji@seketeli.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"
   },
-- 
2.39.3



-- 
		Dodji