diff mbox series

[3/4] abg-writer: allow debugging of equality / hash issues

Message ID 20210910112325.806676-4-gprocida@google.com
State New
Headers
Series Looking at equality and hashing |

Commit Message

Giuliano Procida Sept. 10, 2021, 11:23 a.m. UTC 
  Inconsistenencies between equality and hash values can and have caused
hard-to-debug issues. There are quite a few unordered sets and maps in
the XML writer that use deep_ptr_eq_functor and type_hasher.

New ./configure option: --enable-debug-equality-hash-consistency

A simple way of tracking down potential issues is to check for
inconsistencies any time a type is looked up or inserted into a
container.

	* src/abg-writer.cc (get_id_for_type): Add a debugging stanza
	that, before a type is looked up or inserted, asserts that if
	it is equal to an existing mapped type then their hash values
	must be the same.

Signed-off-by: Giuliano Procida <gprocida@google.com>
---
 configure.ac      |  16 ++++++++
 src/abg-writer.cc | 102 +++++++++++++++++++++++++++++++++++++---------
 2 files changed, 98 insertions(+), 20 deletions(-)
diff mbox series

Patch

diff --git a/configure.ac b/configure.ac
index 735cc9de..8870a7e7 100644
--- a/configure.ac
+++ b/configure.ac
@@ -72,6 +72,12 @@  AC_ARG_ENABLE(debug-self-comparison,
 	      ENABLE_DEBUG_SELF_COMPARISON=$enableval,
 	      ENABLE_DEBUG_SELF_COMPARISON=no)
 
+AC_ARG_ENABLE(debug-equality-hash-consistency,
+	      AS_HELP_STRING([--enable-debug-equality-hash-consistency=yes|no],
+			     [enable debugging of equality / hash value consistency in unordered maps (default is no)]),
+	      ENABLE_DEBUG_EQUALITY_HASH_CONSISTENCY=$enableval,
+	      ENABLE_DEBUG_EQUALITY_HASH_CONSISTENCY=no)
+
 AC_ARG_ENABLE(deb,
 	      AS_HELP_STRING([--enable-deb=yes|no|auto],
 			     [enable the support of deb in abipkgdiff (default is auto)]),
@@ -313,6 +319,16 @@  fi
 
 AM_CONDITIONAL(ENABLE_DEBUG_SELF_COMPARISON, test x$ENABLE_DEBUG_SELF_COMPARISON = xyes)
 
+dnl enable the debugging of equality / hash value consistency in the abidw XML writer
+if test x$ENABLE_DEBUG_EQUALITY_HASH_CONSISTENCY = xyes; then
+  AC_DEFINE([WITH_DEBUG_EQUALITY_HASH_CONSISTENCY], 1, [compile equality / hash value consistency checks])
+  AC_MSG_NOTICE([support of debugging equality / hash value consistency is enabled])
+else
+  AC_MSG_NOTICE([support of debugging equality / hash value consistency is disabled])
+fi
+
+AM_CONDITIONAL(ENABLE_DEBUG_EQUALITY_HASH_CONSISTENCY, test x$ENABLE_DEBUG_EQUALITY_HASH_CONSISTENCY = xyes)
+
 dnl Check for the dpkg program
 if test x$ENABLE_DEB = xauto -o x$ENABLE_DEB = xyes; then
    AC_CHECK_PROG(HAS_DPKG, dpkg, yes, no)
diff --git a/src/abg-writer.cc b/src/abg-writer.cc
index 795ca685..cf664720 100644
--- a/src/abg-writer.cc
+++ b/src/abg-writer.cc
@@ -115,6 +115,41 @@  struct type_hasher
   {return hash_type(t);}
 }; // end struct type_hasher
 
+template <typename T>
+static void check_set_equality_hash_consistency(
+    const T& haystack, const typename T::key_type& needle)
+{
+#ifdef WITH_DEBUG_EQUALITY_HASH_CONSISTENCY
+  abigail::diff_utils::deep_ptr_eq_functor eq;
+  type_hasher hasher;
+  const auto needle_hash = hasher(needle);
+  for (const auto& key : haystack)
+    ABG_ASSERT(!eq(needle, key) || needle_hash == hasher(key));
+#else
+  (void) haystack;
+  (void) needle;
+#endif
+}
+
+template <typename T>
+static void check_map_equality_hash_consistency(
+    const T& haystack, const typename T::key_type& needle)
+{
+#ifdef WITH_DEBUG_EQUALITY_HASH_CONSISTENCY
+  abigail::diff_utils::deep_ptr_eq_functor eq;
+  type_hasher hasher;
+  const auto needle_hash = hasher(needle);
+  for (const auto& key_value : haystack)
+    {
+      const auto& key = key_value.first;
+      ABG_ASSERT(!eq(key, needle) || needle_hash == hasher(key));
+    }
+#else
+  (void) haystack;
+  (void) needle;
+#endif
+}
+
 /// A convenience typedef for a map that associates a pointer to type
 /// to a string.  The pointer to type is hashed as fast as possible.
 typedef unordered_map<type_base*,
@@ -421,6 +456,7 @@  public:
     if (c == 0)
       c = const_cast<type_base*>(t);
 
+    check_map_equality_hash_consistency(m_type_id_map, c);
     auto insertion = m_type_id_map.emplace(c, interned_string());
     interned_string& id = insertion.first->second;
 
@@ -544,14 +580,25 @@  public:
   {
     // If the type is a function type, record it in a dedicated data
     // structure.
-    if (function_type* f = is_function_type(t.get()))
-      m_referenced_fn_types_set.insert(f);
-    else if (!t->get_naked_canonical_type())
-      // If the type doesn't have a canonical type, record it in a
-      // dedicated data structure.
-      m_referenced_non_canonical_types_set.insert(t.get());
+    type_base* t_ptr = t.get();
+    if (function_type* f = is_function_type(t_ptr))
+      {
+	check_set_equality_hash_consistency(m_referenced_fn_types_set, f);
+	m_referenced_fn_types_set.insert(f);
+      }
+    else if (!t_ptr->get_naked_canonical_type())
+      {
+	// If the type doesn't have a canonical type, record it in a
+	// dedicated data structure.
+	check_set_equality_hash_consistency(
+	    m_referenced_non_canonical_types_set, t_ptr);
+	m_referenced_non_canonical_types_set.insert(t_ptr);
+      }
     else
-      m_referenced_types_set.insert(t.get());
+      {
+	check_set_equality_hash_consistency(m_referenced_types_set, t_ptr);
+	m_referenced_types_set.insert(t_ptr);
+      }
   }
 
   /// Test if a given type has been referenced by a pointer, a
@@ -564,15 +611,26 @@  public:
   bool
   type_is_referenced(const type_base_sptr& t)
   {
-    if (function_type *f = is_function_type(t.get()))
-      return (m_referenced_fn_types_set.find(f)
-	      != m_referenced_fn_types_set.end());
-    else if (!t->get_naked_canonical_type())
-      return (m_referenced_non_canonical_types_set.find(t.get())
-	      != m_referenced_non_canonical_types_set.end());
+    type_base* t_ptr = t.get();
+    if (function_type *f = is_function_type(t_ptr))
+      {
+	check_set_equality_hash_consistency(m_referenced_fn_types_set, f);
+	return (m_referenced_fn_types_set.find(f)
+		!= m_referenced_fn_types_set.end());
+      }
+    else if (!t_ptr->get_naked_canonical_type())
+      {
+	check_set_equality_hash_consistency(
+	    m_referenced_non_canonical_types_set, t_ptr);
+	return (m_referenced_non_canonical_types_set.find(t_ptr)
+		!= m_referenced_non_canonical_types_set.end());
+      }
     else
-      return m_referenced_types_set.find
-	(t.get()) != m_referenced_types_set.end();
+      {
+	check_set_equality_hash_consistency(m_referenced_types_set, t_ptr);
+	return (m_referenced_types_set.find(t_ptr)
+		!= m_referenced_types_set.end());
+      }
   }
 
   /// A comparison functor to compare pointers to @ref type_base.
@@ -727,6 +785,7 @@  public:
     type_base *c = t->get_naked_canonical_type();
     if (c == 0)
       c = const_cast<type_base*>(t);
+    check_set_equality_hash_consistency(m_emitted_type_set, c);
     m_emitted_type_set.insert(c);
   }
 
@@ -739,6 +798,7 @@  public:
   bool
   type_is_emitted(const type_base *t) const
   {
+    check_set_equality_hash_consistency(m_emitted_type_set, t);
     return m_emitted_type_set.find(t) != m_emitted_type_set.end();
   }
 
@@ -792,6 +852,7 @@  public:
   {
     class_or_union* cl = is_class_or_union_type(t);
     ABG_ASSERT(cl && cl->get_is_declaration_only());
+    check_set_equality_hash_consistency(m_emitted_decl_only_set, t);
     m_emitted_decl_only_set.insert(t);
   }
 
@@ -814,10 +875,8 @@  public:
   bool
   decl_only_type_is_emitted(const type_base* t) const
   {
-    type_ptr_set_type::const_iterator i = m_emitted_decl_only_set.find(t);
-    if (i == m_emitted_decl_only_set.end())
-      return false;
-    return true;
+    check_set_equality_hash_consistency(m_emitted_decl_only_set, t);
+    return m_emitted_decl_only_set.find(t) != m_emitted_decl_only_set.end();
   }
 
   /// Test if a declaration-only class has been emitted.
@@ -2273,7 +2332,10 @@  record_unemitted_type(type_ptr_set_type& types,
        == tu.get_absolute_path())
       && !ctxt.type_is_emitted(t)
       && !ctxt.decl_only_type_is_emitted(t))
-    types.insert(t);
+    {
+      check_set_equality_hash_consistency(types, t);
+      types.insert(t);
+    }
 }
 
 /// Serialize a translation unit to an output stream.