[1/5] XML writer: use consistent type pointers for type ids and emission tracking

  Insertion resolves the canonical type, if available, but look-up did
not. Given that type id insertion and look-up also resolve canonical
types, it makes sense to adjust the remaining code accordingly.

Neither decl_only_type_is_emitted nor record_decl_only_type_as_emitted
do the check, but very few types end up being recorded this way.

The functions write_class_decl and write_union_decl (but not
write_class_decl_opening_tag and write_union_decl_opening_tag which
can be called in other contexts) resolve declaration-only types to a
definition where possible.

To ensure type ids consistently refer to the same resolved type we
should resolve canonical types and definitions-of-declarations more
consistently.

This change introduces get_preferred_type to return the exemplar type
that should be used for type id and emitted checks.

However, it does not also change all the write functions to write out
the exemplar types.

	* src/abg-writer.cc (get_preferred_type): new function.
	(type_has_existing_id): use get_preferred_type for resolution.
	(get_id_for_type): Likewise.
	(record_type_as_emitted): Likewise.
	(type_is_emitted): Likewise.

Reviewed-by: Giuliano Procida <gprocida@google.com>
Signed-off-by: Matthias Maennich <maennich@google.com>
---
 src/abg-writer.cc | 36 ++++++++++++++++++++++--------------
 1 file changed, 22 insertions(+), 14 deletions(-)
  

Hello,

Matthias Maennich <maennich@google.com> a écrit:

> Insertion resolves the canonical type, if available, but look-up did
> not. Given that type id insertion and look-up also resolve canonical
> types, it makes sense to adjust the remaining code accordingly.
>
> Neither decl_only_type_is_emitted nor record_decl_only_type_as_emitted
> do the check, but very few types end up being recorded this way.
>
> The functions write_class_decl and write_union_decl (but not
> write_class_decl_opening_tag and write_union_decl_opening_tag which
> can be called in other contexts) resolve declaration-only types to a
> definition where possible.
>
> To ensure type ids consistently refer to the same resolved type we
> should resolve canonical types and definitions-of-declarations more
> consistently.
>
> This change introduces get_preferred_type to return the exemplar type
> that should be used for type id and emitted checks.
>
> However, it does not also change all the write functions to write out
> the exemplar types.
>
> 	* src/abg-writer.cc (get_preferred_type): new function.

You call this an "exemplar type" in other places, at least in commit
logs.  And, I quite like that naming.  So I went ahead and called this
function "get_exemplar_type".  Also, since I don't think it's specific
to the abixml writer in essence, I moved it to the abigail::ir
namespace in abg-ir.cc a free form function.

> 	(type_has_existing_id): use get_preferred_type for resolution.
> 	(get_id_for_type): Likewise.
> 	(record_type_as_emitted): Likewise.
> 	(type_is_emitted): Likewise.

I obviously updated these.

[...]


> +++ b/src/abg-writer.cc

[...]

> +  type_base*
> +  get_preferred_type(const type_base* type) const

I have added doxygen documentation to this one.

> +  {
> +    // declaration -> definition -> canonical is possible
> +    if (decl_base* decl = look_through_decl_only(is_decl(type)))
> +      {
> +	type = is_type(decl);
> +	ABG_ASSERT(type);
> +      }
> +    type_base* canonical = type->get_naked_canonical_type();
> +    return canonical ? canonical : const_cast<type_base*>(type);

When the type doesn't have any canonical type, I think we should assert
that the type belongs to the small subset of types that are allowed to
NOT have canonical types.  Namely, decl-only classes when we are looking
at C++ code.  In other words, assert that
is_non_canonicalized_type(examplar_type) returns true.

I've done that in the version of the patch that I've committed.

[...]

I am attaching below the version of the patch that I am applying to
master.

Thanks for the patches!

From 74a2866e4f8deec05d6ef08c54a9bf3414f953d2 Mon Sep 17 00:00:00 2001
From: Matthias Maennich <maennich@google.com>
Date: Fri, 3 Dec 2021 11:46:19 +0000
Subject: [PATCH] XML writer: use consistent type pointers for type ids and emission tracking

Insertion uses the canonical type, if available, but look-up did
not. Given that type id insertion and look-up also use canonical
types, it makes sense to adjust the remaining code accordingly.

Neither decl_only_type_is_emitted nor record_decl_only_type_as_emitted
do the check, but very few types end up being recorded this way.

The functions write_class_decl and write_union_decl (but not
write_class_decl_opening_tag and write_union_decl_opening_tag which
can be called in other contexts) resolve declaration-only types to a
definition where possible.

To ensure type ids consistently refer to the same canonical type we
should use canonical types and definitions-of-declarations more
consistently.

This change introduces get_exemplar_type to return the exemplar type
that should be used for type id and emitted checks.  That exemplar
type is the canonical type of a given type, or the canonical type of
the definition-of-declaration-only-type when applicable.

However, it does not also change all the write functions to write out
the exemplar types.

	* include/abg-fwd.h (get_exemplar_type): Declare new function.
	* src/abg-ir.cc (get_exemplar_type): Define new function.
	* src/abg-writer.cc (type_has_existing_id): use get_exemplar_type
	for resolution.
	(get_id_for_type): Likewise.
	(record_type_as_emitted): Likewise.
	(type_is_emitted): Likewise.

Reviewed-by: Giuliano Procida <gprocida@google.com>
Signed-off-by: Matthias Maennich <maennich@google.com>
Signed-off-by: Dodji Seketeli <dodji@redhat.com>
---
 include/abg-fwd.h | 13 +++++++++++++
 src/abg-ir.cc     | 32 ++++++++++++++++++++++++++++++++
 src/abg-writer.cc | 24 ++++++++++--------------
 3 files changed, 55 insertions(+), 14 deletions(-)

diff --git a/include/abg-fwd.h b/include/abg-fwd.h
index 36a99c3b..c5e98afe 100644
--- a/include/abg-fwd.h
+++ b/include/abg-fwd.h
@@ -1393,6 +1393,19 @@ is_non_canonicalized_type(const type_base *);
 bool
 is_non_canonicalized_type(const type_base_sptr&);
 
+/// For a given type, return its exemplar type.
+///
+/// For a given type, its exemplar type is either its canonical type
+/// or the canonical type of the definition type of a given
+/// declaration-only type.  If the neither of those two types exist,
+/// then the exemplar type is the given type itself.
+///
+/// @param type the input to consider.
+///
+/// @return the exemplar type.
+type_base*
+get_exemplar_type(const type_base* type);
+
 bool
 function_decl_is_less_than(const function_decl&f, const function_decl &s);
 
diff --git a/src/abg-ir.cc b/src/abg-ir.cc
index 03b38c3a..8d0c72a9 100644
--- a/src/abg-ir.cc
+++ b/src/abg-ir.cc
@@ -25232,6 +25232,38 @@ is_non_canonicalized_type(const type_base *t)
   return is_declaration_only_class_or_union_type(t) || env->is_void_type(t);
 }
 
+/// For a given type, return its exemplar type.
+///
+/// For a given type, its exemplar type is either its canonical type
+/// or the canonical type of the definition type of a given
+/// declaration-only type.  If neither of those two types exist,
+/// then the exemplar type is the given type itself.
+///
+/// @param type the input to consider.
+///
+/// @return the exemplar type.
+type_base*
+get_exemplar_type(const type_base* type)
+{
+  if (decl_base * decl = is_decl(type))
+    {
+      // Make sure we get the real definition of a decl-only type.
+      decl = look_through_decl_only(decl);
+      type = is_type(decl);
+      ABG_ASSERT(type);
+    }
+  type_base *exemplar = type->get_naked_canonical_type();
+  if (!exemplar)
+    {
+      // The type has no canonical type.  Let's be sure that it's one
+      // of those rare types that are allowed to be non canonicalized
+      // in the system.
+      exemplar = const_cast<type_base*>(type);
+      ABG_ASSERT(is_non_canonicalized_type(exemplar));
+    }
+  return exemplar;
+}
+
 /// Test if a given type is allowed to be non canonicalized
 ///
 /// This is a subroutine of hash_as_canonical_type_or_constant.
diff --git a/src/abg-writer.cc b/src/abg-writer.cc
index 21b79e88..c1935e01 100644
--- a/src/abg-writer.cc
+++ b/src/abg-writer.cc
@@ -37,6 +37,7 @@ ABG_BEGIN_EXPORT_DECLARATIONS
 
 #include "abg-writer.h"
 #include "abg-libxml-utils.h"
+#include "abg-fwd.h"
 
 ABG_END_EXPORT_DECLARATIONS
 // </headers defining libabigail's API>
@@ -394,10 +395,8 @@ public:
   bool
   type_has_existing_id(type_base* type) const
   {
-    type_base *c = type->get_naked_canonical_type();
-    if (c == 0)
-      c = const_cast<type_base*>(type);
-    return (m_type_id_map.find(c) != m_type_id_map.end());
+    type = get_exemplar_type(type);
+    return m_type_id_map.find(type) != m_type_id_map.end();
   }
 
   /// Associate a unique id to a given type.  For that, put the type
@@ -413,11 +412,9 @@ public:
   /// associated to it, create a new one and return it.  Otherwise,
   /// return the existing id for that type.
   interned_string
-  get_id_for_type(const type_base* t) const
+  get_id_for_type(type_base* type) const
   {
-    type_base *c = t->get_naked_canonical_type();
-    if (c == 0)
-      c = const_cast<type_base*>(t);
+    type_base* c = get_exemplar_type(type);
 
     type_ptr_map::const_iterator it = m_type_id_map.find(c);
     if (it != m_type_id_map.end())
@@ -715,11 +712,9 @@ public:
   ///
   /// @param t the type to flag.
   void
-  record_type_as_emitted(const type_base *t)
+  record_type_as_emitted(const type_base* t)
   {
-    type_base *c = t->get_naked_canonical_type();
-    if (c == 0)
-      c = const_cast<type_base*>(t);
+    type_base* c = get_exemplar_type(t);
     m_emitted_type_set.insert(c);
   }
 
@@ -730,9 +725,10 @@ public:
   /// @return true if the type has already been emitted, false
   /// otherwise.
   bool
-  type_is_emitted(const type_base *t) const
+  type_is_emitted(const type_base* t) const
   {
-    return m_emitted_type_set.find(t) != m_emitted_type_set.end();
+    type_base* c = get_exemplar_type(t);
+    return m_emitted_type_set.find(c) != m_emitted_type_set.end();
   }
 
   /// Test if a given type has been written out to the XML output.