ir: Avoid cancelling a "confirmed" propagated canonical type

When canonicalizing a type T at the IR level, the canonical type of T
(aka C(T)) can be deduced in two ways:

1/ Either T structurally (aka member-wise) compares equal to another
   T' which has a canonical type C(T').  In that case we deduce that
   C(T) equals C(T').

2/ Or, during the canonicalization of another type Y of which T is a
   sub-type, the comparison engine comes to compare T against a type
   T' that is already canonicalized and finds that T structurally
   compares equal to T'.  In that case we deduce that C(T) equals
   C(T'), even though we were canonicalizing Y in the first place.

In 2/ we C(T) is stored and so when comes the time to canonicalize T,
we already know C(T).

What happens in 2/ is called "canonical type propagation". Because the
canonical type of T' is propagated to T while canonicalizing Y.

There can be cases however where the propagated canonical type has to
be "cancelled".  This is described in the source code in abg-ir.cc in
the "@ref OnTheFlyCanonicalization" chapter of the doxygen
documentation.

There are also cases where the propagated canonical type has been
"confirmed", meaning, it cannot be cancelled anymore.

Unfortunately, there seems to be (wrong) cases where confirmed
propagated canonical types are being cancelled.  Oops.  That leads to
having types that are missing canonical types down the road.

This is exhibited by the self-comparison check of the
avr-binutils-2.39-1.fc36.armv7hl.rpm package (more precisely when
analyzing the 'objdump' binary from that package).  This was triggered
by the command:

$ fedabipkgdiff  --self-compare --from fc36 avr-binutils

This patch improves the book keeping around IR canonical type
propagation to avoid cancelling confirmed propagated canonical types.

It introduces a flag to the type_base::priv sub-object to track the
confirmation status of propagated canonical types.  It then updates
the book-keeping routines to make them avoid cancelling confirmed
propagated canonical types.

	* src/abg-ir-priv.h
	(type_base::priv::propagated_canonical_type_confirmed_): Define
	new data member.
	(type_base::priv::priv): Initialize it.
	(type_base::priv::{propagated_canonical_type_confirmed,
	set_propagated_canonical_type_confirmed}): Define new member
	functions.
	(type_base::priv::clear_propagated_canonical_type): Do not clear
	the propagated canonical type if it has been confirmed already.
	(type_base::priv::confirm_ct_propagation_for_types_dependant_on): Rename
	type_base::confirm_ct into this.  Mark the confirmed propagated
	types as being confirmed.
	(type_base::priv::confirm_ct_propagation): This is now a new member
	function that calls
	type_base::confirm_ct_propagation_for_types_dependant_on and that
	does the book-keeping that was being done in
	return_comparison_result.
	(type_base::priv::cancel_ct_propagation_for_types_dependant_on):
	Renamed type_base::priv::cancel_ct_propagation in this.
	(type_base::priv::cancel_ct_propagation): In this new one, call
	type_base::priv::cancel_ct_propagation_for_types_dependant_on. Perform here
	the book-keeping that was being done in return_comparison_result.
	Also, do not cancel a confirmed propagated canonical type.
	(type_base::priv::add_to_types_with_non_confirmed_propagated_ct):
	Define new member function.
	* src/abg-ir.cc (return_comparison_result): Consider only types
	with non-confirmed propagated canonical types for the
	non-confirmed type queue.  Also, only sub-types can be considered
	non-confirmed.  Split out some of the book-keeping into
	type_base::priv::{confirm_ct_propagation, cancel_ct_propagation}
	and call these instead.  Confirm the propagated canonical types of
	all types that remain after the comparison is fully done and is
	successful.
	(canonicalize): Assert the rule "The result of canonicalizing must
	always been a confirmed canonical type".

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

diff --git a/src/abg-ir-priv.h b/src/abg-ir-priv.h
index 5c108dd346113130b1d2713f79362a20ce857463..dd3c627699e65e376fcab548e57085a7ed66462e 100644 (file)
--- a/src/abg-ir-priv.h
+++ b/src/abg-ir-priv.h
@@ -200,12 +200,14 @@ struct type_base::priv
   // The set of canonical recursive types this type depends on.
   unordered_set<uintptr_t> depends_on_recursive_type_;
   bool canonical_type_propagated_;
+  bool propagated_canonical_type_confirmed_;
 
   priv()
     : size_in_bits(),
       alignment_in_bits(),
       naked_canonical_type(),
-      canonical_type_propagated_(false)
+      canonical_type_propagated_(false),
+      propagated_canonical_type_confirmed_(false)
   {}
 
   priv(size_t s,
@@ -215,7 +217,8 @@ struct type_base::priv
       alignment_in_bits(a),
       canonical_type(c),
       naked_canonical_type(c.get()),
-      canonical_type_propagated_(false)
+      canonical_type_propagated_(false),
+      propagated_canonical_type_confirmed_(false)
   {}
 
   /// Test if the current type depends on recursive type comparison.
@@ -306,12 +309,38 @@ struct type_base::priv
   set_canonical_type_propagated(bool f)
   {canonical_type_propagated_ = f;}
 
+  /// Getter of the property propagated-canonical-type-confirmed.
+  ///
+  /// If canonical_type_propagated() returns true, then this property
+  /// says if the propagated canonical type has been confirmed or not.
+  /// If it hasn't been confirmed, then it means it can still
+  /// cancelled.
+  ///
+  /// @return true iff the propagated canonical type has been
+  /// confirmed.
+  bool
+  propagated_canonical_type_confirmed() const
+  {return propagated_canonical_type_confirmed_;}
+
+  /// Setter of the property propagated-canonical-type-confirmed.
+  ///
+  /// If canonical_type_propagated() returns true, then this property
+  /// says if the propagated canonical type has been confirmed or not.
+  /// If it hasn't been confirmed, then it means it can still
+  /// cancelled.
+  ///
+  /// @param f If this is true then the propagated canonical type has
+  /// been confirmed.
+  void
+  set_propagated_canonical_type_confirmed(bool f)
+  {propagated_canonical_type_confirmed_ = f;}
+
   /// If the current canonical type was set as the result of the
   /// "canonical type propagation optimization", then clear it.
   void
   clear_propagated_canonical_type()
   {
-    if (canonical_type_propagated_)
+    if (canonical_type_propagated_ && !propagated_canonical_type_confirmed_)
       {
        canonical_type.reset();
        naked_canonical_type = nullptr;
@@ -810,7 +839,7 @@ struct environment::priv
   /// propagation optimization" at @ref OnTheFlyCanonicalization, in
   /// the src/abg-ir.cc file.
   void
-  confirm_ct_propagation(const type_base* dependant_type)
+  confirm_ct_propagation_for_types_dependant_on(const type_base* dependant_type)
   {
     pointer_set to_remove;
     for (auto i : types_with_non_confirmed_propagated_ct_)
@@ -820,13 +849,65 @@ struct environment::priv
                   || t->priv_->depends_on_recursive_type());
        t->priv_->set_does_not_depend_on_recursive_type(dependant_type);
        if (!t->priv_->depends_on_recursive_type())
-         to_remove.insert(i);
+         {
+           to_remove.insert(i);
+           t->priv_->set_propagated_canonical_type_confirmed(true);
+         }
       }
 
     for (auto i : to_remove)
       types_with_non_confirmed_propagated_ct_.erase(i);
   }
 
+  /// Mark a type that has been the target of canonical type
+  /// propagation as being permanently canonicalized.
+  ///
+  /// This function also marks the set of types that have been the
+  /// target of canonical type propagation and that depend on a
+  /// recursive type as being permanently canonicalized.
+  ///
+  /// To understand the sentence above, please read the description of
+  /// type canonicalization and especially about the "canonical type
+  /// propagation optimization" at @ref OnTheFlyCanonicalization, in
+  /// the src/abg-ir.cc file.
+  void
+  confirm_ct_propagation(const type_base*t)
+  {
+    if (!t || t->priv_->propagated_canonical_type_confirmed())
+      return;
+
+    const environment* env = t->get_environment();
+    ABG_ASSERT(env);
+
+    env->priv_->confirm_ct_propagation_for_types_dependant_on(t);
+    t->priv_->set_does_not_depend_on_recursive_type();
+    env->priv_->remove_from_types_with_non_confirmed_propagated_ct(t);
+    env->priv_->set_is_not_recursive(t);
+    t->priv_->set_propagated_canonical_type_confirmed(true);
+  }
+
+  /// Mark all the types that have been the target of canonical type
+  /// propagation and that are not yet confirmed as being permanently
+  /// canonicalized (aka confirmed).
+  ///
+  /// To understand the sentence above, please read the description of
+  /// type canonicalization and especially about the "canonical type
+  /// propagation optimization" at @ref OnTheFlyCanonicalization, in
+  /// the src/abg-ir.cc file.
+  void
+  confirm_ct_propagation()
+  {
+    for (auto i : types_with_non_confirmed_propagated_ct_)
+      {
+       type_base *t = reinterpret_cast<type_base*>(i);
+       ABG_ASSERT(t->get_environment()->priv_->is_recursive_type(t)
+                  || t->priv_->depends_on_recursive_type());
+       t->priv_->set_does_not_depend_on_recursive_type();
+       t->priv_->set_propagated_canonical_type_confirmed(true);
+      }
+    types_with_non_confirmed_propagated_ct_.clear();
+  }
+
   /// Collect the types that depends on a given "target" type.
   ///
   /// Walk a set of types and if they depend directly or indirectly on
@@ -868,7 +949,7 @@ struct environment::priv
   /// depend on a given recursive type.
   ///
   /// Once the canonical type of a type in that set is reset, the type
-  /// is marked as non being dependant on a recursive type anymore.
+  /// is marked as being non-dependant on a recursive type anymore.
   ///
   /// To understand the sentences above, please read the description
   /// of type canonicalization and especially about the "canonical
@@ -879,7 +960,7 @@ struct environment::priv
   /// type propagation optimizationdepends on a this target type, then
   /// cancel its canonical type.
   void
-  cancel_ct_propagation(const type_base* target)
+  cancel_ct_propagation_for_types_dependant_on(const type_base* target)
   {
     pointer_set to_remove;
     collect_types_that_depends_on(target,
@@ -903,6 +984,58 @@ struct environment::priv
       types_with_non_confirmed_propagated_ct_.erase(i);
   }
 
+  /// Reset the canonical type (set it nullptr) of a type that has
+  /// been the target of canonical type propagation.
+  ///
+  /// This also resets the propagated canonical type of the set of
+  /// types that depends on a given recursive type.
+  ///
+  /// Once the canonical type of a type in that set is reset, the type
+  /// is marked as being non-dependant on a recursive type anymore.
+  ///
+  /// To understand the sentences above, please read the description
+  /// of type canonicalization and especially about the "canonical
+  /// type propagation optimization" at @ref OnTheFlyCanonicalization,
+  /// in the src/abg-ir.cc file.
+  ///
+  /// @param target if a type which has been subject to the canonical
+  /// type propagation optimizationdepends on a this target type, then
+  /// cancel its canonical type.
+  void
+  cancel_ct_propagation(const type_base* t)
+  {
+    if (!t)
+      return;
+
+    const environment *env = t->get_environment();
+    env->priv_->cancel_ct_propagation_for_types_dependant_on(t);
+    if (t->priv_->depends_on_recursive_type()
+       || env->priv_->is_recursive_type(t))
+      {
+       // This cannot carry any tentative canonical type at this
+       // point.
+       if (t->priv_->canonical_type_propagated()
+           && !t->priv_->propagated_canonical_type_confirmed())
+         t->priv_->clear_propagated_canonical_type();
+       // Reset the marking of the type as it no longer carries a
+       // tentative canonical type that might be later cancelled.
+       t->priv_->set_does_not_depend_on_recursive_type();
+       env->priv_->remove_from_types_with_non_confirmed_propagated_ct(t);
+      }
+  }
+
+  /// Add a given type to the set of types that have been
+  /// non-confirmed subjects of the canonical type propagation
+  /// optimization.
+  ///
+  /// @param t the dependant type to consider.
+  void
+  add_to_types_with_non_confirmed_propagated_ct(const type_base *t)
+  {
+    uintptr_t v = reinterpret_cast<uintptr_t>(t);
+    types_with_non_confirmed_propagated_ct_.insert(v);
+  }
+
   /// Remove a given type from the set of types that have been
   /// non-confirmed subjects of the canonical type propagation
   /// optimization.

diff --git a/src/abg-ir.cc b/src/abg-ir.cc
index f739b12686997acea70e8b3597ef1104d960b6af..2563b5b3ed90b25c62beb5c867cfc4594fb91676 100644 (file)
--- a/src/abg-ir.cc
+++ b/src/abg-ir.cc
@@ -1084,13 +1084,14 @@ return_comparison_result(T& l, T& r, bool value,
       if (value == true
          && (is_type(&r)->priv_->depends_on_recursive_type()
              || env->priv_->is_recursive_type(&r))
-         && is_type(&r)->priv_->canonical_type_propagated())
+         && is_type(&r)->priv_->canonical_type_propagated()
+         && !is_type(&r)->priv_->propagated_canonical_type_confirmed()
+         && !env->priv_->right_type_comp_operands_.empty())
        {
          // Track the object 'r' for which the propagated canonical
          // type might be re-initialized if the current comparison
          // eventually fails.
-         env->priv_->types_with_non_confirmed_propagated_ct_.insert
-           (reinterpret_cast<uintptr_t>(is_type(&r)));
+         env->priv_->add_to_types_with_non_confirmed_propagated_ct(is_type(&r));
        }
       else if (value == true && env->priv_->right_type_comp_operands_.empty())
        {
@@ -1102,13 +1103,6 @@ return_comparison_result(T& l, T& r, bool value,
          // sub-types that were compared during the comparison of
          // 'r'.
          env->priv_->confirm_ct_propagation(&r);
-         if (is_type(&r)->priv_->depends_on_recursive_type()
-             || env->priv_->is_recursive_type(&r))
-           {
-             is_type(&r)->priv_->set_does_not_depend_on_recursive_type();
-             env->priv_->remove_from_types_with_non_confirmed_propagated_ct(&r);
-             env->priv_->set_is_not_recursive(&r);
-           }
        }
       else if (value == false)
        {
@@ -1118,20 +1112,25 @@ return_comparison_result(T& l, T& r, bool value,
          // should see their tentatively propagated canonical type
          // cancelled.
          env->priv_->cancel_ct_propagation(&r);
-         if (is_type(&r)->priv_->depends_on_recursive_type()
-             || env->priv_->is_recursive_type(&r))
-           {
-             // The right-hand-side operand cannot carry any tentative
-             // canonical type at this point.
-             is_type(&r)->priv_->clear_propagated_canonical_type();
-             // Reset the marking of the right-hand-side operand as it no
-             // longer carries a tentative canonical type that might be
-             // later cancelled.
-             is_type(&r)->priv_->set_does_not_depend_on_recursive_type();
-             env->priv_->remove_from_types_with_non_confirmed_propagated_ct(&r);
-           }
        }
     }
+
+  // If we reached this point with value == true and the stack of
+  // types being compared is empty, then it means that the type pair
+  // that was at the bottom of the stack is now fully compared.
+  //
+  // It follows that all types that were target of canonical type
+  // propagation can now see their tentative canonical type be
+  // confirmed for real.
+  if (value == true
+      && env->priv_->right_type_comp_operands_.empty()
+      && !env->priv_->types_with_non_confirmed_propagated_ct_.empty())
+    // So the comparison is completely done and there are some
+    // types for which their propagated canonical type is sitll
+    // considered not confirmed.  As the comparison did yield true, we
+    // shall now confirm the propagation for all those types.
+    env->priv_->confirm_ct_propagation();
+
   ABG_RETURN(value);
 }
 
@@ -14666,6 +14665,19 @@ canonicalize(type_base_sptr t)
   t->priv_->canonical_type = canonical;
   t->priv_->naked_canonical_type = canonical.get();
 
+  // So this type is now canonicalized.
+  //
+  // It means that:
+  //
+  //   1/ Either the canonical type was not propagated during the
+  //      comparison of another type that was being canonicalized
+  //
+  //   2/ Or the canonical type has been propagated during the
+  //      comparison of another type was being canonicalized and that
+  //      propagated canonical type has been confirmed.
+  ABG_ASSERT(!t->priv_->canonical_type_propagated()
+            || t->priv_->propagated_canonical_type_confirmed());
+
   if (class_decl_sptr cl = is_class_type(t))
     if (type_base_sptr d = is_type(cl->get_earlier_declaration()))
       if ((canonical = d->get_canonical_type()))