From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from relay1-d.mail.gandi.net (relay1-d.mail.gandi.net [217.70.183.193]) by sourceware.org (Postfix) with ESMTPS id F1F8C385E830 for ; Fri, 22 May 2020 10:03:03 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org F1F8C385E830 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=seketeli.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=dodji@seketeli.org X-Originating-IP: 91.166.131.130 Received: from localhost (91-166-131-130.subs.proxad.net [91.166.131.130]) (Authenticated sender: dodj@seketeli.org) by relay1-d.mail.gandi.net (Postfix) with ESMTPSA id 6AEB624000F; Fri, 22 May 2020 10:03:01 +0000 (UTC) Received: by localhost (Postfix, from userid 1001) id 2E7ED1A0779; Fri, 22 May 2020 12:03:00 +0200 (CEST) From: Dodji Seketeli To: Giuliano Procida Cc: libabigail@sourceware.org, kernel-team@android.com, maennich@google.com Subject: Re: [RFC PATCH 1/2] Support declaration-only enums. Organization: Me, myself and I References: <20200423130356.93136-1-gprocida@google.com> <20200423130356.93136-2-gprocida@google.com> X-Operating-System: Red Hat Enterprise Linux Server 7.8 X-URL: http://www.seketeli.net/~dodji Date: Fri, 22 May 2020 12:03:00 +0200 In-Reply-To: <20200423130356.93136-2-gprocida@google.com> (Giuliano Procida's message of "Thu, 23 Apr 2020 14:03:55 +0100") Message-ID: <86eerci8x7.fsf@seketeli.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-8.8 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, T_FILL_THIS_FORM_SHORT autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) 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: Fri, 22 May 2020 10:03:06 -0000 Hello, I think this is a very interesting and needed task. It's a bit involved, given that it touches many parts of the codebase so I really appreciate you diving into this. Really, thank you for doing this. And I really find that your patch set is a great start. Honest. To help with conveying how I think we should shape this, I have put your patches as well as some changes that I talk about in this review in the dodji/incomp-enums branch at https://sourceware.org/git/?p=3Dlibabigail.git;a=3Dshortlog;h=3Drefs/heads/= dodji/incomp-enums. You can start from that branch to further amend the patch series (following the review round) or not, at your convenience. In any case, I'll sometimes refer to a patch in that branch when I try to convey an idea about a particular topic in your patch set. I think the three main things that are missing from this patch set are 1/ the support for serializing declaration-only enums. I guess this would be like what is done for declaration-only classes in write_class_decl_opening_tag by invoking write_class_or_union_is_declaration_only. 2/ support generating an opaque type from an enum when that enum has been deemed private by a type supression specification. This should be done by get_opaque_version_of_type in abg-dwarf-reader.cc. I have put up a patch for this in dodji/incomp-enums at https://sourceware.org/git/?p=3Dlibabigail.git;a=3Dcommit;h=3D4e361d914e8= e6c34d5e00f570551b4b5a545fd82 3/ write tests that exercise * opaque enum type building from suppression specifications * resolution of declaration-only enums to their right fully defined co= unterparts. * serialization of declaration-only enums There might be other things to add/change (like ensuring that after decl-only enums resolution is performed, an unresolved decl-only enum doesn't equal a fully defined enum) but I think we can get to those later once these fundamental pieces are dealt with first. Giuliano Procida a =C3=A9crit: [...] > diff --git a/include/abg-fwd.h b/include/abg-fwd.h > index 1aab70a6..d1cf0322 100644 > --- a/include/abg-fwd.h > +++ b/include/abg-fwd.h > @@ -154,9 +154,15 @@ typedef weak_ptr typedef_decl_wptr; >=20=20 > class enum_type_decl; >=20=20 > -/// Convenience typedef for shared pointer on enum_type_decl. > +/// Convenience typedef for shared pointer to a @ref enum_type_decl. > typedef shared_ptr enum_type_decl_sptr; Your fix is correct, but I think it should probably go into a separate commit. [...] > --- a/include/abg-ir.h > +++ b/include/abg-ir.h > @@ -2465,6 +2465,8 @@ public: > const string& mangled_name =3D "", > visibility vis =3D VISIBILITY_DEFAULT); >=20=20 > + enum_type_decl(const environment* env, const string& name); Declaring a new constructor for this type here is not necessary. So we should drop this change. Please see later down below where I comment about the changes in build_enum_type. [...] > /// Test if a class_or_union_diff carries a change in which the two > /// classes are different by the fact that one is a decl-only and the > /// other one is defined. > /// > /// @param diff the diff node to consider. > -//// > -//// @return true if the class_or_union_diff carries a change in which > +/// > +/// @return true if the class_or_union_diff carries a change in which > /// the two classes are different by the fact that one is a decl-only > /// and the other one is defined. > bool This change should go in a different "misc style fixes" commit. [...] > +++ b/src/abg-comparison.cc [...] > - if (c & FN_RETURN_TYPE_CV_CHANGE_CATEGORY) > + if (c & FN_RETURN_TYPE_CV_CHANGE_CATEGORY) Likewise. [...] > - if (c & VAR_TYPE_CV_CHANGE_CATEGORY) > + if (c & VAR_TYPE_CV_CHANGE_CATEGORY) Likewise. [...] > - if (c & VOID_PTR_TO_PTR_CHANGE_CATEGORY) > + if (c & VOID_PTR_TO_PTR_CHANGE_CATEGORY) Likewise. [...] > - if (c & BENIGN_INFINITE_ARRAY_CHANGE_CATEGORY) > + if (c & BENIGN_INFINITE_ARRAY_CHANGE_CATEGORY) Likewise. [...] > --- a/src/abg-default-reporter.cc > +++ b/src/abg-default-reporter.cc > @@ -94,13 +94,33 @@ default_reporter::report(const enum_diff& d, ostream&= out, > d.second_subject(), > "enum type"); >=20=20 > - string name =3D d.first_enum()->get_pretty_representation(); > - This change is ... > enum_type_decl_sptr first =3D d.first_enum(), second =3D d.second_enum= (); >=20=20 > - report_name_size_and_alignment_changes(first, second, d.context(), > + const diff_context_sptr& ctxt =3D d.context(); > + > + string name =3D first->get_pretty_representation(); ... unnecessary. The s/d.context()/ctxt/ changes in this function are unnecessary as well. [...] > d.reported_once(true); > @@ -844,7 +864,7 @@ default_reporter::report(const class_or_union_diff& d, >=20=20 > const diff_context_sptr& ctxt =3D d.context(); >=20=20 > - // Report class decl-only -> definition change. > + // Report class decl-only <-> definition change. This change is unrelated to the current patch. [...] > +++ b/src/abg-dwarf-reader.cc [...] > @@ -139,6 +139,11 @@ typedef unordered_map di= e_class_map_type; > /// corresponding class_or_union_sptr. > typedef unordered_map die_class_or_union= _map_type; >=20=20 > +/// Convenience typedef for a map which key is the offset of a dwarf > +/// die, (given by dwarf_dieoffset()) and which value is the > +/// corresponding enum_type_decl_sptr. > +typedef unordered_map die_enum_map_type; > + Adding this typedef is unnecessary because ... [...] > @@ -2273,6 +2282,9 @@ public: > die_class_or_union_map_type die_wip_classes_map_; > die_class_or_union_map_type alternate_die_wip_classes_map_; > die_class_or_union_map_type type_unit_die_wip_classes_map_; > + die_enum_map_type die_wip_enums_map_; > + die_enum_map_type alternate_die_wip_enums_map_; > + die_enum_map_type type_unit_die_wip_enums_map_; ... these new data members are unnecessary. Because enum types are built atomically, they don't have to go through a 'work-in-progress' process. That process is only suited for classes and function types because these are built gradually and thus stay non-fully constructed for a certain time during which other types are built. Hence these transiently non fully built types (a.k.a WIP types) are 'marked' as such. [...] > + /// Getter of a map that associates a die that represents a > + /// enum with the declaration of the enum, while the enum > + /// is being constructed. > + /// > + /// @param source where the DIE is from. > + /// > + /// @return the map that associates a DIE to the enum that is being > + /// built. > + const die_enum_map_type& > + die_wip_enums_map(die_source source) const > + {return const_cast(this)->die_wip_enums_map(source);} This accessor is unnecessary as well. [...] > + > + /// Getter of a map that associates a die that represents a > + /// enum with the declaration of the enum, while the enum > + /// is being constructed. > + /// > + /// @param source where the DIE comes from. > + /// > + /// @return the map that associates a DIE to the enum that is being > + /// built. > + die_enum_map_type& > + die_wip_enums_map(die_source source) Likewise. [...] > @@ -16660,7 +16943,18 @@ build_ir_node_from_die(read_context& ctxt, >=20=20 > case DW_TAG_enumeration_type: > { > - if (!type_is_suppressed(ctxt, scope, die)) > + bool type_is_private =3D false; > + bool type_suppressed =3D > + type_is_suppressed(ctxt, scope, die, type_is_private); > + if (type_suppressed && type_is_private) > + // The type is suppressed because it's private. If other > + // non-suppressed and declaration-only instances of this > + // type exist in the current corpus, then it means those > + // non-suppressed instances are opaque versions of the > + // suppressed private type. Lets return one of these opaque > + // types then. > + result =3D get_opaque_version_of_type(ctxt, scope, die, where_offset); Right. Though, for this to work, get_opaque_version_of_type must be adapted to function with enum types. Note that at the moment, it only functions for classes (not even for unions). I have added a patch to dodji/incomp-enums at https://sourceware.org/git/?p=3Dlibabigail.git;a=3Dcommit;h=3D4e361d914e8e6= c34d5e00f570551b4b5a545fd82 to do that. That patch factorizes a some parts of build_enum_type because it needed it to reuse some them. > + else if (!type_suppressed) > { [...] > @@ -13386,18 +13634,32 @@ build_enum_type(read_context& ctxt, > if (tag !=3D DW_TAG_enumeration_type) > return result; >=20=20 > + die_source source; > + ABG_ASSERT(ctxt.get_die_source(die, source)); > + { > + die_enum_map_type::const_iterator i =3D > + ctxt.die_wip_enums_map(source).find(dwarf_dieoffset(die)); > + if (i !=3D ctxt.die_wip_enums_map(source).end()) > + { > + enum_type_decl_sptr u =3D is_enum_type(i->second); > + ABG_ASSERT(u); > + return u; > + } > + } As I said earlier, this WIP handling is unnecessary. [...] > - bool enum_is_anonymous =3D false; > + bool is_anonymous =3D false; This change in unnecessary. > // If the enum is anonymous, let's give it a name. > if (name.empty()) > { > name =3D get_internal_anonymous_die_prefix_name(die); > ABG_ASSERT(!name.empty()); > // But we remember that the type is anonymous. > - enum_is_anonymous =3D true; > + is_anonymous =3D true; Likewise. >=20=20 > if (size_t s =3D scope->get_num_anonymous_member_enums()) > name =3D build_internal_anonymous_die_name(name, s); > @@ -13409,7 +13671,7 @@ build_enum_type(read_context& ctxt, > // representation (name) and location can be later detected as being > // for the same type. >=20=20 > - if (!enum_is_anonymous) > + if (!is_anonymous) Likewise. > { > if (use_odr) > { > @@ -13442,6 +13704,7 @@ build_enum_type(read_context& ctxt, > uint64_t size =3D 0; > if (die_unsigned_constant_attribute(die, DW_AT_byte_size, size)) > size *=3D 8; > + bool is_artificial =3D die_is_artificial(die); This change is ... [...] > // for now we consider that underlying types of enums are all anonymous > bool enum_underlying_type_is_anonymous=3D true; > @@ -13474,8 +13737,6 @@ build_enum_type(read_context& ctxt, > while (dwarf_siblingof(&child, &child) =3D=3D 0); > } >=20=20 > - bool is_artificial =3D die_is_artificial(die); > - ... unnecessary. > // DWARF up to version 4 (at least) doesn't seem to carry the > // underlying type, so let's create an artificial one here, which > // sole purpose is to be passed to the constructor of the > @@ -13491,9 +13752,13 @@ build_enum_type(read_context& ctxt, > t =3D dynamic_pointer_cast(d); > ABG_ASSERT(t); > result.reset(new enum_type_decl(name, loc, t, enms, linkage_name)); > - result->set_is_anonymous(enum_is_anonymous); > + result->set_is_anonymous(is_anonymous); Likewise. [...] > +++ b/src/abg-ir.cc [...] > +/// Constructor for instances of enum_type_decl that represent a > +/// declaration without definition. > +/// > +/// @param env the environment we are operating from. > +/// > +/// @param name the name of the enum. > +enum_type_decl::enum_type_decl(const environment* env, > + const string& name) > + : type_or_decl_base(env, > + ENUM_TYPE > + | ABSTRACT_TYPE_BASE > + | ABSTRACT_DECL_BASE), > + type_base(env, 0, 0), > + decl_base(env, name, location(), name), > + priv_(new priv) > +{ > + runtime_type_instance(this); > +} > + This constructor is unnecessary. [...] > +/// If this @ref enum_type_decl_sptr is a definition, get its earlier > +/// declaration. > +/// > +/// @return the earlier declaration of the enum, if any. > +decl_base_sptr > +enum_type_decl::get_earlier_declaration() const > +{return priv_->declaration_;} I think this is unnecessary. Classes do have a similar accessor but it's a relique from ancient times when resolution of decl-only classes was done differently. I keep it around to be able to support ancient abixml files that might be out there somewhere. > + > +/// set the earlier declaration of this @ref enum_type_decl definition. > +/// > +/// @param declaration the earlier declaration to set. Note that it's > +/// set only if it's a pure declaration. > +void > +enum_type_decl::set_earlier_declaration(decl_base_sptr declaration) > +{ > + enum_type_decl_sptr cl =3D dynamic_pointer_cast(declar= ation); > + if (cl && cl->get_is_declaration_only()) > + priv_->declaration_ =3D declaration; > +} Likewise. [...] Thanks again for working on this. Cheers, --=20 Dodji