[PATCH, applied] Fix spurious deleted/added virtual destructor change report

[PATCH, applied] Fix spurious deleted/added virtual destructor change report

[Dodji Seketeli]

Hello,

While looking at something else, I noticed this spurious change
report:

    1 member function deletion:
      'method virtual tbb::internal::concurrent_queue_base_v3::~concurrent_queue_base_v3(int)' at concurrent_queue.cpp:361:1
    1 member function insertion:
		'method virtual tbb::internal::concurrent_queue_base_v3::~concurrent_queue_base_v3(int)' at concurrent_queue.cpp:370:1

This is when running tests/runtestdiffpkg and the result is in
tests/data/test-diff-pkg/tbb-4.1-9.20130314.fc22.x86_64--tbb-4.3-3.20141204.fc23.x86_64-report-0.txt.

To understand what is going on, one need to refer to the definitions
of the C++ ABI at
https://itanium-cxx-abi.github.io/cxx-abi/abi.html#definitions.

When a class has a virtual destructor, historically, there might be 3
different and co-existing actual implementations of the destructors:
the D0, D1 and D2 destructors.  There might even be a D3 destructor,
actually.

In https://github.com/itanium-cxx-abi/cxx-abi/issues/73, one can see
that there might be new D4 and D5 destructors.  Each one of them might
replace the set of the D0,D1,D2 destructors.

So, in a new version of a binary, the virtual D4 destructor might
replace the previous ones, without it being an ABI issue.

The switch to the D4 virtual destructor is what libabigail is naively
reporting in the change report above.

This patch detects this kind of changes in the pipeline when the edit
script from the diff2 algorithm is interpreted for virtual member
functions and drops it on the floor.

	* src/abg-comparison.cc (find_virtual_dtor_in_map): Define new
	static function.
	(class_diff::ensure_lookup_tables_populated): If a virtual
	destructor is removed from the old binary version but is added to
	the new one (but through a different name), let's assume the
	virtual destructor is still there so there is no ABI issue
	from that point of view.
	* tests/data/test-diff-pkg/tbb-4.1-9.20130314.fc22.x86_64--tbb-4.3-3.20141204.fc23.x86_64-report-0.txt: Adjust.

Signed-off-by: Dodji Seketeli <dodji@redhat.com>
Applied to the master branch.
---
 src/abg-comparison.cc                         | 92 ++++++++++++++++++-
 ...bb-4.3-3.20141204.fc23.x86_64-report-0.txt |  8 --
 2 files changed, 91 insertions(+), 9 deletions(-)

diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc
index 0868d384..0ecd0f27 100644
--- a/src/abg-comparison.cc
+++ b/src/abg-comparison.cc
@@ -5146,6 +5146,27 @@ class_diff::lookup_tables_empty(void) const
 	  && priv_->changed_bases_.empty());
 }
 
+/// Find a virtual destructor in a map of member functions
+///
+/// @param map the map of member functions.  Note that the key of the
+/// map is the member function name.  The key is the member function.
+///
+/// @return an iterator to the destructor found or, if no virtual destructor
+/// was found, return map.end()
+static string_member_function_sptr_map::const_iterator
+find_virtual_dtor_in_map(const string_member_function_sptr_map& map)
+{
+  for (string_member_function_sptr_map::const_iterator i = map.begin();
+       i !=map.end();
+       ++i)
+    {
+      if (get_member_function_is_dtor(i->second)
+	  && get_member_function_is_virtual(i->second))
+	return i;
+    }
+  return map.end();
+}
+
 /// If the lookup tables are not yet built, walk the differences and
 /// fill them.
 void
@@ -5277,6 +5298,45 @@ class_diff::ensure_lookup_tables_populated(void) const
     // underlying symbols are deleted as well; otherwise, consider
     // that the member function in question hasn't been deleted.
 
+    // Also, while walking the deleted member functions, we attend at
+    // a particular cleanup business related to (virtual) C++
+    // destructors:
+    //
+    // In the binary, there can be at least three types of
+    // destructors, defined in the document
+    // https://itanium-cxx-abi.github.io/cxx-abi/abi.html#definitions:
+    //
+    //   1/ Base object destructor (aka D2 destructor):
+    //
+    //      "A function that runs the destructors for non-static data
+    //      members of T and non-virtual direct base classes of T. "
+    //
+    //   2/ Complete object destructor (aka D1 destructor):
+    //
+    //      "A function that, in addition to the actions required of a
+    //      base object destructor, runs the destructors for the
+    //      virtual base classes of T."
+    //
+    //   3/ Deleting destructor (aka D0 destructor):
+    //
+    //      "A function that, in addition to the actions required of a
+    //      complete object destructor, calls the appropriate
+    //      deallocation function (i.e,. operator delete) for T."
+    //
+    // With binaries generated by GCC, these destructors might be ELF
+    // clones of each others, meaning, their ELF symbols can be
+    // aliases.
+    //
+    // Also, note that because the actual destructor invoked by user
+    // code is virtual, it's invoked through the vtable.  So the
+    // presence of the underlying D0, D1, D2 in the binary might vary
+    // without that variation being an ABI issue, provided that the
+    // destructor invoked through the vtable is present.
+    //
+    // So, a particular virtual destructor implementation for a class
+    // might disapear and be replaced by another one in a subsequent
+    // version of the binary.  If all versions of the binary have an
+    // actual virtual destructor, things might be considered fine.
     vector<string> to_delete;
     corpus_sptr f = context()->get_first_corpus(),
       s = context()->get_second_corpus();
@@ -5287,7 +5347,37 @@ class_diff::ensure_lookup_tables_populated(void) const
 	   ++i)
 	{
 	  if (get_member_function_is_virtual(i->second))
-	    continue;
+	    {
+	      if (get_member_function_is_dtor(i->second))
+		{
+		  // If a particular virtual destructor is deleted,
+		  // but the new binary still have a virtual
+		  // destructor for that class we consider that things
+		  // are fine.  For instance, in the
+		  // tests/data/test-diff-pkg/tbb-4.1-9.20130314.fc22.x86_64--tbb-4.3-3.20141204.fc23.x86_64-report-0.txt
+		  // test, a new D4 destructor replaces the old ones.
+		  // But because the virtual destructor is still
+		  // there, this is not an ABI issue.  So let's detect
+		  // this case.
+		  auto it =
+		    find_virtual_dtor_in_map(p->inserted_member_functions_);
+		  if (it != p->inserted_member_functions_.end())
+		    {
+		      // So the deleted virtual destructor is not
+		      // really deleted, because a proper virtual
+		      // destructor was added to the new version.
+		      // Let's remove the deleted/added virtual
+		      // destructor then.
+		      string name =
+			(!i->second->get_linkage_name().empty())
+			? i->second->get_linkage_name()
+			: i->second->get_pretty_representation();
+		      to_delete.push_back(name);
+		      p->inserted_member_functions_.erase(it);
+		    }
+		}
+	      continue;
+	    }
 	  // We assume that all non-virtual member functions functions
 	  // we look at here have ELF symbols.
 	  if (!i->second->get_symbol()
diff --git a/tests/data/test-diff-pkg/tbb-4.1-9.20130314.fc22.x86_64--tbb-4.3-3.20141204.fc23.x86_64-report-0.txt b/tests/data/test-diff-pkg/tbb-4.1-9.20130314.fc22.x86_64--tbb-4.3-3.20141204.fc23.x86_64-report-0.txt
index 89e9c5ce..07d2f187 100644
--- a/tests/data/test-diff-pkg/tbb-4.1-9.20130314.fc22.x86_64--tbb-4.3-3.20141204.fc23.x86_64-report-0.txt
+++ b/tests/data/test-diff-pkg/tbb-4.1-9.20130314.fc22.x86_64--tbb-4.3-3.20141204.fc23.x86_64-report-0.txt
@@ -30,10 +30,6 @@
       implicit parameter 0 of type 'tbb::filter*' has sub-type changes:
         in pointed to type 'class tbb::filter' at pipeline.h:65:1:
           type size hasn't changed
-          1 member function deletion:
-            'method virtual tbb::filter::~filter(int)' at pipeline.cpp:698:1
-          1 member function insertion:
-            'method virtual tbb::filter::~filter(int)' at pipeline.cpp:688:1
           no member function changes (4 filtered);
           1 data member changes (4 filtered):
             type of 'tbb::internal::input_buffer* my_input_buffer' changed:
@@ -171,10 +167,6 @@
       implicit parameter 0 of type 'tbb::internal::concurrent_queue_base_v3*' has sub-type changes:
         in pointed to type 'class tbb::internal::concurrent_queue_base_v3' at _concurrent_queue_impl.h:834:1:
           type size hasn't changed
-          1 member function deletion:
-            'method virtual tbb::internal::concurrent_queue_base_v3::~concurrent_queue_base_v3(int)' at concurrent_queue.cpp:361:1
-          1 member function insertion:
-            'method virtual tbb::internal::concurrent_queue_base_v3::~concurrent_queue_base_v3(int)' at concurrent_queue.cpp:370:1
           no member function changes (7 filtered);
           1 data member change:
             type of 'tbb::internal::concurrent_queue_rep* my_rep' changed:
-- 
2.31.1


-- 
		Dodji