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

[Dodji Seketeli <dodji@seketeli.org>]

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.
-- 
2.33.1


Cheers,


-- 
		Dodji