From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from relay3-d.mail.gandi.net (relay3-d.mail.gandi.net [217.70.183.195]) by sourceware.org (Postfix) with ESMTPS id 399AF3858C3A for ; Thu, 9 Dec 2021 17:57:53 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 399AF3858C3A Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=seketeli.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=seketeli.org Received: (Authenticated sender: dodji@seketeli.org) by relay3-d.mail.gandi.net (Postfix) with ESMTPSA id 6AD6E60011; Thu, 9 Dec 2021 17:57:51 +0000 (UTC) Received: by localhost (Postfix, from userid 1000) id 9A9A85802BD; Thu, 9 Dec 2021 18:57:50 +0100 (CET) From: Dodji Seketeli To: Matthias Maennich Cc: libabigail@sourceware.org, gprocida@google.com, kernel-team@android.com Subject: Re: [PATCH 1/5] XML writer: use consistent type pointers for type ids and emission tracking Organization: Me, myself and I References: <20211203114622.2944173-1-maennich@google.com> <20211203114622.2944173-2-maennich@google.com> X-Operating-System: Fedora 36 X-URL: http://www.seketeli.net/~dodji Date: Thu, 09 Dec 2021 18:57:50 +0100 In-Reply-To: <20211203114622.2944173-2-maennich@google.com> (Matthias Maennich's message of "Fri, 3 Dec 2021 11:46:19 +0000") Message-ID: <87r1alr78h.fsf@seketeli.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-9.6 required=5.0 tests=BAYES_00, GIT_PATCH_0, JMQ_SPF_NEUTRAL, KAM_DMARC_STATUS, RCVD_IN_DNSWL_LOW, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: libabigail@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Mailing list of the Libabigail project List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 09 Dec 2021 17:57:56 -0000 Hello, Matthias Maennich a =C3=A9crit: > 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 =3D look_through_decl_only(is_decl(type))) > + { > + type =3D is_type(decl); > + ABG_ASSERT(type); > + } > + type_base* canonical =3D type->get_naked_canonical_type(); > + return canonical ? canonical : const_cast(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 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 Signed-off-by: Matthias Maennich Signed-off-by: Dodji Seketeli --- 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&); =20 +/// 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); =20 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= ); } =20 +/// 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 =3D is_decl(type)) + { + // Make sure we get the real definition of a decl-only type. + decl =3D look_through_decl_only(decl); + type =3D is_type(decl); + ABG_ASSERT(type); + } + type_base *exemplar =3D 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 =3D const_cast(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 =20 #include "abg-writer.h" #include "abg-libxml-utils.h" +#include "abg-fwd.h" =20 ABG_END_EXPORT_DECLARATIONS // @@ -394,10 +395,8 @@ public: bool type_has_existing_id(type_base* type) const { - type_base *c =3D type->get_naked_canonical_type(); - if (c =3D=3D 0) - c =3D const_cast(type); - return (m_type_id_map.find(c) !=3D m_type_id_map.end()); + type =3D get_exemplar_type(type); + return m_type_id_map.find(type) !=3D m_type_id_map.end(); } =20 /// 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 =3D t->get_naked_canonical_type(); - if (c =3D=3D 0) - c =3D const_cast(t); + type_base* c =3D get_exemplar_type(type); =20 type_ptr_map::const_iterator it =3D m_type_id_map.find(c); if (it !=3D 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 =3D t->get_naked_canonical_type(); - if (c =3D=3D 0) - c =3D const_cast(t); + type_base* c =3D get_exemplar_type(t); m_emitted_type_set.insert(c); } =20 @@ -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) !=3D m_emitted_type_set.end(); + type_base* c =3D get_exemplar_type(t); + return m_emitted_type_set.find(c) !=3D m_emitted_type_set.end(); } =20 /// Test if a given type has been written out to the XML output. --=20 2.33.1 Cheers, --=20 Dodji