[RFC PATCH 1/2] Support declaration-only enums.
Dodji Seketeli
dodji@seketeli.org
Fri May 22 10:03:00 GMT 2020
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=libabigail.git;a=shortlog;h=refs/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=libabigail.git;a=commit;h=4e361d914e8e6c34d5e00f570551b4b5a545fd82
3/ write tests that exercise
* opaque enum type building from suppression specifications
* resolution of declaration-only enums to their right fully defined counterparts.
* 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 <gprocida@google.com> a écrit:
[...]
> 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> typedef_decl_wptr;
>
> class enum_type_decl;
>
> -/// 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> 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 = "",
> visibility vis = VISIBILITY_DEFAULT);
>
> + 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");
>
> - string name = d.first_enum()->get_pretty_representation();
> -
This change is ...
> enum_type_decl_sptr first = d.first_enum(), second = d.second_enum();
>
> - report_name_size_and_alignment_changes(first, second, d.context(),
> + const diff_context_sptr& ctxt = d.context();
> +
> + string name = 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,
>
> const diff_context_sptr& ctxt = d.context();
>
> - // 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<Dwarf_Off, class_decl_sptr> die_class_map_type;
> /// corresponding class_or_union_sptr.
> typedef unordered_map<Dwarf_Off, class_or_union_sptr> die_class_or_union_map_type;
>
> +/// 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<Dwarf_Off, enum_type_decl_sptr> 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<read_context*>(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,
>
> case DW_TAG_enumeration_type:
> {
> - if (!type_is_suppressed(ctxt, scope, die))
> + bool type_is_private = false;
> + bool type_suppressed =
> + 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 = 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=libabigail.git;a=commit;h=4e361d914e8e6c34d5e00f570551b4b5a545fd82
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 != DW_TAG_enumeration_type)
> return result;
>
> + die_source source;
> + ABG_ASSERT(ctxt.get_die_source(die, source));
> + {
> + die_enum_map_type::const_iterator i =
> + ctxt.die_wip_enums_map(source).find(dwarf_dieoffset(die));
> + if (i != ctxt.die_wip_enums_map(source).end())
> + {
> + enum_type_decl_sptr u = is_enum_type(i->second);
> + ABG_ASSERT(u);
> + return u;
> + }
> + }
As I said earlier, this WIP handling is unnecessary.
[...]
> - bool enum_is_anonymous = false;
> + bool is_anonymous = false;
This change in unnecessary.
> // If the enum is anonymous, let's give it a name.
> if (name.empty())
> {
> name = get_internal_anonymous_die_prefix_name(die);
> ABG_ASSERT(!name.empty());
> // But we remember that the type is anonymous.
> - enum_is_anonymous = true;
> + is_anonymous = true;
Likewise.
>
> if (size_t s = scope->get_num_anonymous_member_enums())
> name = 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.
>
> - if (!enum_is_anonymous)
> + if (!is_anonymous)
Likewise.
> {
> if (use_odr)
> {
> @@ -13442,6 +13704,7 @@ build_enum_type(read_context& ctxt,
> uint64_t size = 0;
> if (die_unsigned_constant_attribute(die, DW_AT_byte_size, size))
> size *= 8;
> + bool is_artificial = 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= true;
> @@ -13474,8 +13737,6 @@ build_enum_type(read_context& ctxt,
> while (dwarf_siblingof(&child, &child) == 0);
> }
>
> - bool is_artificial = 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 = dynamic_pointer_cast<type_decl>(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 = dynamic_pointer_cast<enum_type_decl>(declaration);
> + if (cl && cl->get_is_declaration_only())
> + priv_->declaration_ = declaration;
> +}
Likewise.
[...]
Thanks again for working on this.
Cheers,
--
Dodji
More information about the Libabigail
mailing list