comparison: Fix erroneous detection of deleted anonymous data members

It has been reported over IRC by Robin Jarry that in some cases,
abidiff would wrongly consider anonymous data members as being
deleted.  These are cases where the anonymous data members were moved
into other anonymous data members without incurring any size or offset
change.

This patch reduces (and hopefully eliminates) false positives in
detecting anonymous data members deletion by considering the leaf data
members of the anonymous data members.

It considers that if all leaf data members contained in the previous
anonymous data members are still present in the new version of the
type, then no data member deletion occurred, basically.

The non-regression test added to this patch contains two binaries that
Robin Jarry sent me.  They were built for the source code before and
after the patch he submitted at
http://patches.dpdk.org/project/dpdk/patch/20240327091440.1166119-2-rjarry@redhat.com/.

Note that I have also added a synthetic reproducer example I came up
with after understanding the issue at hand.

	* src/abg-comparison.cc
	(class_or_union_diff::ensure_lookup_tables_populated): If leaf
	data members of an anonymous data member are still present in the
	new version of the class, then the anonymous data member cannot be
	considered as being removed from the second version of the class.
	* tests/data/test-abidiff-exit/non-del-anon-dm/non-regr/report0.txt:
	Reference test output.
	* tests/data/test-abidiff-exit/non-del-anon-dm/non-regr/test0-v{0,1}.c:
	Source code of the binary input below.
	* tests/data/test-abidiff-exit/non-del-anon-dm/non-regr/test0-v{0,1}.o:
	New binary test input.
	* tests/data/test-abidiff-exit/non-del-anon-dm/reported/binaries-origin.txt:
	File mentioning the origin of the reported binary.
	* tests/data/test-abidiff-exit/non-del-anon-dm/reported/librte_graph.so.24.{0,1}:
	Binary test input.
	* tests/data/test-abidiff-exit/non-del-anon-dm/reported/report0.txt:
	Reference test output.
	* tests/data/Makefile.am: Add the test material above to source
	distribution.
	* tests/test-abidiff-exit.cc (in_out_specs): Add test material
	above to this harness.

Signed-off-by: Dodji Seketeli <dodji@redhat.com>

diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc
index 3dc3d28da5eab654ab00f2953c3beafa4cac975d..062988687d14be6d5f3bfd27db39d5c84fea6d8f 100644 (file)
--- a/src/abg-comparison.cc
+++ b/src/abg-comparison.cc
@@ -5370,6 +5370,59 @@ class_or_union_diff::ensure_lookup_tables_populated(void) const
        priv_->inserted_data_members_.erase
          (i->second->second_var()->get_anon_dm_reliable_name());
       }
+
+    // Now detect anonymous data members that might appear as deleted
+    // even though all their data members are still present.  Consider
+    // these as being non-deleted.
+    string_decl_base_sptr_map non_anonymous_dms_in_second_class;
+    collect_non_anonymous_data_members(second_class_or_union(),
+                                      non_anonymous_dms_in_second_class);
+    vector<string> deleted_data_members_to_delete;
+    // Walk data members considered deleted ...
+    for (auto& entry : priv_->deleted_data_members_)
+      {
+       var_decl_sptr data_member = is_var_decl(entry.second);
+       ABG_ASSERT(data_member);
+       if (is_anonymous_data_member(data_member))
+         {
+           // Let's look at this anonymous data member that is
+           // considered deleted because it's moved from where it was
+           // initially, at very least.  If its leaf data members are
+           // still present in the second class then, we won't
+           // consider it as deleted.
+           class_or_union_sptr cou = anonymous_data_member_to_class_or_union(data_member);
+           ABG_ASSERT(cou);
+           string_decl_base_sptr_map non_anonymous_data_members;
+           // Lets collect the leaf data members of the anonymous
+           // data member.
+           collect_non_anonymous_data_members(cou, non_anonymous_data_members);
+           bool anonymous_dm_members_present = true;
+           // Let's see if at least one of the leaf members of the
+           // anonymous data member is NOT present in the second
+           // version of the class.
+           for (auto& e : non_anonymous_data_members)
+             {
+               if (non_anonymous_dms_in_second_class.find(e.first)
+                   == non_anonymous_dms_in_second_class.end())
+                 // Grrr, OK, it looks like at least one leaf data
+                 // member of the original anonymous data member was
+                 // removed from the class, so yeah, the anonymous
+                 // data member might have been removed after all.
+                 anonymous_dm_members_present = false;
+             }
+           if (anonymous_dm_members_present)
+             // All leaf data members of the anonymous data member
+             // are still present in the second version of the class.
+             // So let's mark that anonymous data member as NOT being
+             // deleted.
+             deleted_data_members_to_delete.push_back(data_member->get_anon_dm_reliable_name());
+         }
+      }
+    // All anonymous data members that were recognized as being NOT
+    // deleted should be removed from the set of deleted data members
+    // now.
+    for (string& name_of_dm_to_delete: deleted_data_members_to_delete)
+      priv_->deleted_data_members_.erase(name_of_dm_to_delete);
   }
   sort_string_data_member_diff_sptr_map(priv_->subtype_changed_dm_,
                                        priv_->sorted_subtype_changed_dm_);

diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am
index 5be64c4c628016f0a4fb39ff5db5f443596deb29..d12754963502d87417b171c83097ae13125e48d0 100644 (file)
--- a/tests/data/Makefile.am
+++ b/tests/data/Makefile.am
@@ -444,6 +444,15 @@ test-abidiff-exit/PR31513/non-regr/test3-v0.cc  \
 test-abidiff-exit/PR31513/non-regr/test3-v1.cc  \
 test-abidiff-exit/PR31513/non-regr/test4-v0.cc  \
 test-abidiff-exit/PR31513/non-regr/test4-v1.cc  \
+test-abidiff-exit/non-del-anon-dm/non-regr/report0.txt \
+test-abidiff-exit/non-del-anon-dm/non-regr/test0-v0.c \
+test-abidiff-exit/non-del-anon-dm/non-regr/test0-v0.o \
+test-abidiff-exit/non-del-anon-dm/non-regr/test0-v1.c \
+test-abidiff-exit/non-del-anon-dm/non-regr/test0-v1.o \
+test-abidiff-exit/non-del-anon-dm/reported/binaries-origin.txt \
+test-abidiff-exit/non-del-anon-dm/reported/librte_graph.so.24.0 \
+test-abidiff-exit/non-del-anon-dm/reported/librte_graph.so.24.1 \
+test-abidiff-exit/non-del-anon-dm/reported/report0.txt \
 \
 test-diff-dwarf/test0-v0.cc            \
 test-diff-dwarf/test0-v0.o                     \

diff --git a/tests/data/test-abidiff-exit/non-del-anon-dm/non-regr/report0.txt b/tests/data/test-abidiff-exit/non-del-anon-dm/non-regr/report0.txt
new file mode 100644 (file)
index 0000000..9666a8f
--- /dev/null
+++ b/tests/data/test-abidiff-exit/non-del-anon-dm/non-regr/report0.txt
@@ -0,0 +1,3 @@
+Functions changes summary: 0 Removed, 0 Changed (1 filtered out), 0 Added function
+Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
+

diff --git a/tests/data/test-abidiff-exit/non-del-anon-dm/non-regr/test0-v0.c b/tests/data/test-abidiff-exit/non-del-anon-dm/non-regr/test0-v0.c
new file mode 100644 (file)
index 0000000..65d7df8
--- /dev/null
+++ b/tests/data/test-abidiff-exit/non-del-anon-dm/non-regr/test0-v0.c
@@ -0,0 +1,16 @@
+struct type
+{
+  int m0;
+  struct
+  {
+    union {int um00; char* um01;};
+    int* m1;
+    union {int* um10; char um11;};
+  };
+  float m2;
+};
+
+void
+foo(struct type* __attribute__((unused)))
+{
+}

diff --git a/tests/data/test-abidiff-exit/non-del-anon-dm/non-regr/test0-v0.o b/tests/data/test-abidiff-exit/non-del-anon-dm/non-regr/test0-v0.o
new file mode 100644 (file)
index 0000000..73d997e
Binary files /dev/null and b/tests/data/test-abidiff-exit/non-del-anon-dm/non-regr/test0-v0.o differ

diff --git a/tests/data/test-abidiff-exit/non-del-anon-dm/non-regr/test0-v1.c b/tests/data/test-abidiff-exit/non-del-anon-dm/non-regr/test0-v1.c
new file mode 100644 (file)
index 0000000..1544f6c
--- /dev/null
+++ b/tests/data/test-abidiff-exit/non-del-anon-dm/non-regr/test0-v1.c
@@ -0,0 +1,20 @@
+struct type
+{
+  struct /* wrapping up all data members into an anonymous data
+           member */
+  {
+    int m0;
+    struct
+    {
+      union {int um00; char* um01;};
+      int* m1;
+      union {int* um10; char um11;};
+    };
+    float m2;
+  };
+};
+
+void
+foo(struct type* __attribute__((unused)))
+{
+}

diff --git a/tests/data/test-abidiff-exit/non-del-anon-dm/non-regr/test0-v1.o b/tests/data/test-abidiff-exit/non-del-anon-dm/non-regr/test0-v1.o
new file mode 100644 (file)
index 0000000..5582130
Binary files /dev/null and b/tests/data/test-abidiff-exit/non-del-anon-dm/non-regr/test0-v1.o differ

diff --git a/tests/data/test-abidiff-exit/non-del-anon-dm/reported/binaries-origin.txt b/tests/data/test-abidiff-exit/non-del-anon-dm/reported/binaries-origin.txt
new file mode 100644 (file)
index 0000000..b9742c0
--- /dev/null
+++ b/tests/data/test-abidiff-exit/non-del-anon-dm/reported/binaries-origin.txt
@@ -0,0 +1,2 @@
+The binaries have been generated from before & after this patch:
+http://patches.dpdk.org/project/dpdk/patch/20240327091440.1166119-2-rjarry@redhat.com/

diff --git a/tests/data/test-abidiff-exit/non-del-anon-dm/reported/librte_graph.so.24.0 b/tests/data/test-abidiff-exit/non-del-anon-dm/reported/librte_graph.so.24.0
new file mode 100755 (executable)
index 0000000..0ecfd4f
Binary files /dev/null and b/tests/data/test-abidiff-exit/non-del-anon-dm/reported/librte_graph.so.24.0 differ

diff --git a/tests/data/test-abidiff-exit/non-del-anon-dm/reported/librte_graph.so.24.1 b/tests/data/test-abidiff-exit/non-del-anon-dm/reported/librte_graph.so.24.1
new file mode 100755 (executable)
index 0000000..af801f8
Binary files /dev/null and b/tests/data/test-abidiff-exit/non-del-anon-dm/reported/librte_graph.so.24.1 differ

diff --git a/tests/data/test-abidiff-exit/non-del-anon-dm/reported/report0.txt b/tests/data/test-abidiff-exit/non-del-anon-dm/reported/report0.txt
new file mode 100644 (file)
index 0000000..cbae006
--- /dev/null
+++ b/tests/data/test-abidiff-exit/non-del-anon-dm/reported/report0.txt
@@ -0,0 +1,3 @@
+Functions changes summary: 0 Removed, 0 Changed (10 filtered out), 0 Added functions
+Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
+

diff --git a/tests/test-abidiff-exit.cc b/tests/test-abidiff-exit.cc
index 3b0c13fca64aef1b1f6d7674e2fee0d3cd4f6431..104d058062efa201fb3fe368bd0c0f1914b8d7a5 100644 (file)
--- a/tests/test-abidiff-exit.cc
+++ b/tests/test-abidiff-exit.cc
@@ -1380,6 +1380,36 @@ InOutSpec in_out_specs[] =
     "data/test-abidiff-exit/PR31513/non-regr/report4.txt",
     "output/test-abidiff-exit/PR31513/non-regr/report4.txt"
   },
+  {
+    "data/test-abidiff-exit/non-del-anon-dm/reported/librte_graph.so.24.0",
+    "data/test-abidiff-exit/non-del-anon-dm/reported/librte_graph.so.24.1",
+    "",
+    "",
+    "",
+    "",
+    "",
+    "",
+    "",
+    "--no-default-suppression",
+    abigail::tools_utils::ABIDIFF_OK,
+    "data/test-abidiff-exit/non-del-anon-dm/reported/report0.txt",
+    "output/test-abidiff-exit/non-del-anon-dm/reported/report0.txt"
+  },
+  {
+    "data/test-abidiff-exit/non-del-anon-dm/non-regr/test0-v0.o",
+    "data/test-abidiff-exit/non-del-anon-dm/non-regr/test0-v1.o",
+    "",
+    "",
+    "",
+    "",
+    "",
+    "",
+    "",
+    "--no-default-suppression",
+    abigail::tools_utils::ABIDIFF_OK,
+    "data/test-abidiff-exit/non-del-anon-dm/non-regr/report0.txt",
+    "output/test-abidiff-exit/non-del-anon-dm/non-regr/report0.txt"
+  },
 #ifdef WITH_BTF
   {
     "data/test-abidiff-exit/btf/test0-v0.o",