Message ID | 20200423130356.93136-2-gprocida@google.com |
---|---|
State | Superseded |
Headers | show |
Series | Incomplete enum support | expand |
Hi. On Tue, 12 May 2020 at 15:03, Matthias Maennich <maennich@google.com> wrote: > On Thu, Apr 23, 2020 at 02:03:55PM +0100, Giuliano Procida wrote: > >This is an almost blind attempt at addition of support for incomplete, > >also known as forward-declared, enum types. I've not made any attempt > >to refactor or share logic with the struct/union code and I've > >probably got other things wrong. > > > >Signed-off-by: Giuliano Procida <gprocida@google.com> > >--- > > include/abg-comp-filter.h | 7 + > > include/abg-comparison.h | 7 + > > include/abg-fwd.h | 26 ++- > > include/abg-ir.h | 23 +++ > > src/abg-comp-filter.cc | 78 ++++++++- > > src/abg-comparison.cc | 21 ++- > > src/abg-default-reporter.cc | 34 +++- > > src/abg-dwarf-reader.cc | 308 +++++++++++++++++++++++++++++++++++- > > src/abg-ir.cc | 235 ++++++++++++++++++++++++++- > > 9 files changed, 713 insertions(+), 26 deletions(-) > > > >diff --git a/include/abg-comp-filter.h b/include/abg-comp-filter.h > >index b8da3156..c486fec5 100644 > >--- a/include/abg-comp-filter.h > >+++ b/include/abg-comp-filter.h > >@@ -68,9 +68,16 @@ bool > > has_class_decl_only_def_change(const class_or_union_sptr& first, > > const class_or_union_sptr& second); > > > >+bool > >+has_enum_decl_only_def_change(const enum_type_decl_sptr& first, > >+ const enum_type_decl_sptr& second); > >+ > > bool > > has_class_decl_only_def_change(const diff *diff); > > > >+bool > >+has_enum_decl_only_def_change(const diff *diff); > >+ > > bool > > has_basic_type_name_change(const diff *); > > > >diff --git a/include/abg-comparison.h b/include/abg-comparison.h > >index 4f60ff99..bb654cef 100644 > >--- a/include/abg-comparison.h > >+++ b/include/abg-comparison.h > >@@ -430,6 +430,12 @@ enum diff_category > > /// array type of a global variable, but the ELF size of the > > /// variable didn't change. > > BENIGN_INFINITE_ARRAY_CHANGE_CATEGORY = 1 << 19, > >+ > >+ /// This means that a diff node in the sub-tree carries a enum type > >+ /// that was declaration-only and that is now defined, or vice > >+ /// versa. > >+ ENUM_DECL_ONLY_DEF_CHANGE_CATEGORY = 1 << 20, > >+ > > /// A special enumerator that is the logical 'or' all the > > /// enumerators above. > > /// > >@@ -456,6 +462,7 @@ enum diff_category > > | VAR_TYPE_CV_CHANGE_CATEGORY > > | VOID_PTR_TO_PTR_CHANGE_CATEGORY > > | BENIGN_INFINITE_ARRAY_CHANGE_CATEGORY > >+ | ENUM_DECL_ONLY_DEF_CHANGE_CATEGORY > > }; // enum diff_category > > > > diff_category > >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; > > > >+/// Convenience typedef for a vector of @ref enum_type_decl_sptr > >+typedef vector<enum_type_decl_sptr> enums_type; > >+ > >+/// Convenience typedef for a weak pointer to a @ref enum_type_decl. > >+typedef weak_ptr<enum_type_decl> enum_type_decl_wptr; > >+ > > class class_or_union; > > > > typedef shared_ptr<class_or_union> class_or_union_sptr; > >@@ -422,6 +428,12 @@ is_typedef(const type_base*); > > typedef_decl* > > is_typedef(type_base*); > > > >+enum_type_decl_sptr > >+is_compatible_with_enum_type(const type_base_sptr&); > >+ > >+enum_type_decl_sptr > >+is_compatible_with_enum_type(const decl_base_sptr&); > >+ > > enum_type_decl_sptr > > is_enum_type(const type_or_decl_base_sptr&); > > > >@@ -512,6 +524,12 @@ look_through_decl_only_class(const class_or_union&); > > class_or_union_sptr > > look_through_decl_only_class(class_or_union_sptr); > > > >+enum_type_decl_sptr > >+look_through_decl_only_enum(const enum_type_decl&); > >+ > >+enum_type_decl_sptr > >+look_through_decl_only_enum(enum_type_decl_sptr); > >+ > > var_decl* > > is_var_decl(const type_or_decl_base*); > > > >@@ -1071,6 +1089,12 @@ lookup_enum_type(const string&, const corpus&); > > enum_type_decl_sptr > > lookup_enum_type(const interned_string&, const corpus&); > > > >+const type_base_wptrs_type* > >+lookup_enum_types(const interned_string&, const corpus&); > >+ > >+const type_base_wptrs_type* > >+lookup_enum_types(const string&, const corpus&); > >+ > > enum_type_decl_sptr > > lookup_enum_type_per_location(const interned_string&, const corpus&); > > > >diff --git a/include/abg-ir.h b/include/abg-ir.h > >index fda10de5..88ce39e0 100644 > >--- 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); > >+ > > type_base_sptr > > get_underlying_type() const; > > > >@@ -2474,6 +2476,27 @@ public: > > enumerators& > > get_enumerators(); > > > >+ bool > >+ get_is_declaration_only() const; > >+ > >+ void > >+ set_is_declaration_only(bool f); > >+ > >+ void > >+ set_definition_of_declaration(enum_type_decl_sptr); > >+ > >+ const enum_type_decl_sptr > >+ get_definition_of_declaration() const; > >+ > >+ const enum_type_decl* > >+ get_naked_definition_of_declaration() const; > >+ > >+ decl_base_sptr > >+ get_earlier_declaration() const; > >+ > >+ void > >+ set_earlier_declaration(decl_base_sptr declaration); > >+ > > virtual string > > get_pretty_representation(bool internal = false, > > bool qualified_name = true) const; > >diff --git a/src/abg-comp-filter.cc b/src/abg-comp-filter.cc > >index 75df901c..c8a031d3 100644 > >--- a/src/abg-comp-filter.cc > >+++ b/src/abg-comp-filter.cc > >@@ -118,6 +118,25 @@ there_is_a_decl_only_class(const class_decl_sptr& > class1, > > return false; > > } > > > >+/// Test if there is a enum that is declaration-only among the two > >+/// enumes in parameter. > >+/// > >+/// @param enum1 the first enum to consider. > >+/// > >+/// @param enum2 the second enum to consider. > >+/// > >+/// @return true if either enumes are declaration-only, false > >+/// otherwise. > >+static bool > >+there_is_a_decl_only_enum(const enum_type_decl_sptr& enum1, > >+ const enum_type_decl_sptr& enum2) > >+{ > >+ if ((enum1 && enum1->get_is_declaration_only()) > >+ || (enum2 && enum2->get_is_declaration_only())) > >+ return true; > >+ return false; > >+} > >+ > > /// Test if the diff involves a declaration-only class. > > /// > > /// @param diff the class diff to consider. > >@@ -146,7 +165,9 @@ type_size_changed(const type_base_sptr f, const > type_base_sptr s) > > || f->get_size_in_bits() == 0 > > || s->get_size_in_bits() == 0 > > || there_is_a_decl_only_class(is_compatible_with_class_type(f), > >- is_compatible_with_class_type(s))) > >+ is_compatible_with_class_type(s)) > >+ || there_is_a_decl_only_enum(is_compatible_with_enum_type(f), > >+ is_compatible_with_enum_type(s))) > > return false; > > > > return f->get_size_in_bits() != s->get_size_in_bits(); > >@@ -937,13 +958,38 @@ has_class_decl_only_def_change(const > class_or_union_sptr& first, > > return (f->get_is_declaration_only() != s->get_is_declaration_only()); > > } > > > >+/// Test if two @ref enum_sptr are different just by the > >+/// fact that one is decl-only and the other one is defined. > >+/// > >+/// @param first the first enum to consider. > >+/// > >+/// @param second the second enum to consider. > >+/// > >+/// @return true iff the two arguments are different just by the fact > >+/// that one is decl-only and the other one is defined. > >+bool > >+has_enum_decl_only_def_change(const enum_type_decl_sptr& first, > >+ const enum_type_decl_sptr& second) > >+{ > >+ if (!first || !second) > >+ return false; > >+ > >+ enum_type_decl_sptr f = look_through_decl_only_enum(first); > >+ enum_type_decl_sptr s = look_through_decl_only_enum(second); > >+ > >+ if (f->get_qualified_name() != s->get_qualified_name()) > >+ return false; > >+ > >+ return (f->get_is_declaration_only() != s->get_is_declaration_only()); > >+} > >+ > > /// 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 > >@@ -961,6 +1007,28 @@ has_class_decl_only_def_change(const diff *diff) > > return has_class_decl_only_def_change(f, s); > > } > > > >+/// Test if a enum_diff carries a change in which the two > >+/// enumes 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 enum_diff carries a change in which > >+/// the two enumes are different by the fact that one is a decl-only > >+/// and the other one is defined. > >+bool > >+has_enum_decl_only_def_change(const diff *diff) > >+{ > >+ const enum_diff *d = dynamic_cast<const enum_diff*>(diff); > >+ if (!d) > >+ return false; > >+ > >+ enum_type_decl_sptr f = look_through_decl_only_enum(d->first_enum()); > >+ enum_type_decl_sptr s = look_through_decl_only_enum(d->second_enum()); > >+ > >+ return has_enum_decl_only_def_change(f, s); > >+} > >+ > > /// Test if a diff node carries a basic type name change. > > /// > > /// @param d the diff node to consider. > >@@ -1501,6 +1569,9 @@ categorize_harmless_diff_node(diff *d, bool pre) > > if (has_class_decl_only_def_change(d)) > > category |= CLASS_DECL_ONLY_DEF_CHANGE_CATEGORY; > > > >+ if (has_enum_decl_only_def_change(d)) > >+ category |= ENUM_DECL_ONLY_DEF_CHANGE_CATEGORY; > >+ > > if (access_changed(f, s)) > > category |= ACCESS_CHANGE_CATEGORY; > > > >@@ -1586,6 +1657,7 @@ categorize_harmful_diff_node(diff *d, bool pre) > > // > > // TODO: be more specific -- not all size changes are harmful. > > if (!has_class_decl_only_def_change(d) > >+ && !has_enum_decl_only_def_change(d) > > && (type_size_changed(f, s) > > || data_member_offset_changed(f, s) > > || non_static_data_member_type_size_changed(f, s) > >diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc > >index 46bf9e30..40d80073 100644 > >--- a/src/abg-comparison.cc > >+++ b/src/abg-comparison.cc > >@@ -2952,7 +2952,8 @@ get_default_harmless_categories_bitmap() > > | abigail::comparison::FN_RETURN_TYPE_CV_CHANGE_CATEGORY > > | abigail::comparison::VAR_TYPE_CV_CHANGE_CATEGORY > > | abigail::comparison::VOID_PTR_TO_PTR_CHANGE_CATEGORY > >- | abigail::comparison::BENIGN_INFINITE_ARRAY_CHANGE_CATEGORY); > >+ | abigail::comparison::BENIGN_INFINITE_ARRAY_CHANGE_CATEGORY > >+ | abigail::comparison::ENUM_DECL_ONLY_DEF_CHANGE_CATEGORY); > > I would move that up to the line with > > | abigail::comparison::CLASS_DECL_ONLY_DEF_CHANGE_CATEGORY > Actually, I think it should just be a single TYPE_DECL_ONLY... or even DECL_ONLY... CLASS already includes structs and (possibly, the support may be incomplete) unions. > } > > > > /// Getter of a bitmap made of the set of change categories that are > >@@ -3113,7 +3114,7 @@ operator<<(ostream& o, diff_category c) > > emitted_a_category |= true; > > } > > > >- if (c & FN_RETURN_TYPE_CV_CHANGE_CATEGORY) > >+ if (c & FN_RETURN_TYPE_CV_CHANGE_CATEGORY) > > Correct, but unrelated. > Ack, moved into another commit. > > { > > if (emitted_a_category) > > o << "|"; > >@@ -3121,7 +3122,7 @@ operator<<(ostream& o, diff_category c) > > emitted_a_category |= true; > > } > > > >- if (c & VAR_TYPE_CV_CHANGE_CATEGORY) > >+ if (c & VAR_TYPE_CV_CHANGE_CATEGORY) > > { > > if (emitted_a_category) > > o << "|"; > >@@ -3129,7 +3130,7 @@ operator<<(ostream& o, diff_category c) > > emitted_a_category |= true; > > } > > > >- if (c & VOID_PTR_TO_PTR_CHANGE_CATEGORY) > >+ if (c & VOID_PTR_TO_PTR_CHANGE_CATEGORY) > > { > > if (emitted_a_category) > > o << "|"; > >@@ -3137,7 +3138,7 @@ operator<<(ostream& o, diff_category c) > > emitted_a_category |= true; > > } > > > >- if (c & BENIGN_INFINITE_ARRAY_CHANGE_CATEGORY) > >+ if (c & BENIGN_INFINITE_ARRAY_CHANGE_CATEGORY) > > { > > if (emitted_a_category) > > o << "|"; > >@@ -3145,6 +3146,14 @@ operator<<(ostream& o, diff_category c) > > emitted_a_category |= true; > > } > > > >+ if (c & ENUM_DECL_ONLY_DEF_CHANGE_CATEGORY) > >+ { > >+ if (emitted_a_category) > >+ o << "|"; > >+ o << "ENUM_DECL_ONLY_DEF_CHANGE_CATEGORY"; > >+ emitted_a_category |= true; > >+ } > >+ > > return o; > > } > > > >@@ -10707,6 +10716,8 @@ struct leaf_diff_node_marker_visitor : public > diff_node_visitor > > && !is_anonymous_class_or_union_diff(d) > > // Don't show decl-only-ness changes of classes either. > > && !filtering::has_class_decl_only_def_change(d) > >+ // Same for enums. > > The comment should be self-containing. > Similar to the comment about a single category, this should really just be "has_decl_only_def_change". Here things get a little harder as all the declaration-only pieces are at the wrong level of the inheritance hierarchy to allow this to work in the obvious way. We can side-step this for the moment and hide the complexity in one place until such time as the hierarchy is reworked. More words: class_or_union, enum_type_decl, base_decl, base_type, scoped_type_decl. These are all part of the exposed API. > >+ && !filtering::has_enum_decl_only_def_change(d) > > // Sometime, we can encounter artifacts of bogus DWARF that > > // yield a diff node for a decl-only class (and empty class > > // with the is_declaration flag set) that carries a non-zero > >diff --git a/src/abg-default-reporter.cc b/src/abg-default-reporter.cc > >index 04e2bb76..19dec4cb 100644 > >--- 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(); > >- > > 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(); > > const > Ack. > >+ > >+ // Report enum decl-only <-> definition changes. > >+ if (ctxt->get_allowed_category() & ENUM_DECL_ONLY_DEF_CHANGE_CATEGORY) > >+ if (filtering::has_enum_decl_only_def_change(first, second)) > >+ { > >+ string was = > >+ first->get_is_declaration_only() > >+ ? " was a declaration-only enum type" > >+ : " was a defined enum type"; > >+ > >+ string is_now = > >+ second->get_is_declaration_only() > >+ ? " and is now a declaration-only enum type" > >+ : " and is now a defined enum type"; > >+ > >+ out << indent << "enum type " << name << was << is_now << "\n"; > >+ return; > >+ } > >+ > >+ report_name_size_and_alignment_changes(first, second, ctxt, > > out, indent); > >- maybe_report_diff_for_member(first, second, d.context(), out, indent); > >+ maybe_report_diff_for_member(first, second, ctxt, out, indent); > > > > //underlying type > > d.underlying_type_diff()->report(out, indent); > >@@ -165,12 +185,12 @@ default_reporter::report(const enum_diff& d, > ostream& out, > > << "' from value '" > > << i->first.get_value() << "' to '" > > << i->second.get_value() << "'"; > >- report_loc_info(second, *d.context(), out); > >+ report_loc_info(second, *ctxt, out); > > out << "\n"; > > } > > } > > > >- if (d.context()->show_leaf_changes_only()) > >+ if (ctxt->show_leaf_changes_only()) > > maybe_report_interfaces_impacted_by_diff(&d, out, indent); > > > > 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. > > if (ctxt->get_allowed_category() & CLASS_DECL_ONLY_DEF_CHANGE_CATEGORY) > > if (filtering::has_class_decl_only_def_change(first, second)) > > { > >diff --git a/src/abg-dwarf-reader.cc b/src/abg-dwarf-reader.cc > >index 850281ad..5748460e 100644 > >--- a/src/abg-dwarf-reader.cc > >+++ 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; > >+ > > /// Convenience typedef for a map which key the offset of a dwarf die > > /// and which value is the corresponding function_decl. > > typedef unordered_map<Dwarf_Off, function_decl_sptr> > die_function_decl_map_type; > >@@ -198,6 +203,10 @@ typedef unordered_map<Dwarf_Off, Dwarf_Off> > offset_offset_map_type; > > /// value is a vector of smart pointer to a class. > > typedef unordered_map<string, classes_type> string_classes_map; > > > >+/// Convenience typedef for a map which key is a string and which > >+/// value is a vector of smart pointer to a enum. > >+typedef unordered_map<string, enums_type> string_enums_map; > >+ > > /// The abstraction of the place where a partial unit has been > > /// imported. This is what the DW_TAG_imported_unit DIE expresses. > > /// > >@@ -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_; > > die_function_type_map_type die_wip_function_types_map_; > > die_function_type_map_type alternate_die_wip_function_types_map_; > > die_function_type_map_type type_unit_die_wip_function_types_map_; > >@@ -2282,6 +2294,7 @@ public: > > vector<Dwarf_Off> type_unit_types_to_canonicalize_; > > vector<type_base_sptr> extra_types_to_canonicalize_; > > string_classes_map decl_only_classes_map_; > >+ string_enums_map decl_only_enums_map_; > > die_tu_map_type die_tu_map_; > > corpus_group_sptr cur_corpus_group_; > > corpus_sptr cur_corpus_; > >@@ -4340,6 +4353,44 @@ public: > > return die_wip_classes_map_; > > } > > > >+ /// 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);} > > Give the comment further down, this can also be templated. > The WIP stuff *can* actually be simplified by going up the existing inheritance hierarchy. > >+ > >+ /// 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. > > This should be a template over the map type to consolidate with > die_wip_classes_map. > > >+ die_enum_map_type& > >+ die_wip_enums_map(die_source source) > >+ { > >+ switch (source) > >+ { > >+ case PRIMARY_DEBUG_INFO_DIE_SOURCE: > >+ break; > >+ case ALT_DEBUG_INFO_DIE_SOURCE: > >+ return alternate_die_wip_enums_map_; > >+ case TYPE_UNIT_DIE_SOURCE: > >+ return type_unit_die_wip_enums_map_; > >+ case NO_DEBUG_INFO_DIE_SOURCE: > >+ case NUMBER_OF_DIE_SOURCES: > >+ ABG_ASSERT_NOT_REACHED; > >+ } > >+ return die_wip_enums_map_; > >+ } > >+ > > /// Getter for a map that associates a die (that represents a > > /// function type) whith a function type, while the function type is > > /// being constructed (WIP == work in progress). > >@@ -4620,6 +4671,203 @@ public: > > } > > } > > > >+ /// Getter for the map of declaration-only enums that are to be > >+ /// resolved to their definition enums by the end of the corpus > >+ /// loading. > >+ /// > >+ /// @return a map of string -> vector of enums where the key is > >+ /// the fully qualified name of the enum and the value is the > >+ /// vector of declaration-only enum. > >+ const string_enums_map& > >+ declaration_only_enums() const > >+ {return decl_only_enums_map_;} > >+ > >+ /// Getter for the map of declaration-only enums that are to be > >+ /// resolved to their definition enums by the end of the corpus > >+ /// loading. > >+ /// > >+ /// @return a map of string -> vector of enums where the key is > >+ /// the fully qualified name of the enum and the value is the > >+ /// vector of declaration-only enum. > >+ string_enums_map& > >+ declaration_only_enums() > >+ {return decl_only_enums_map_;} > >+ > >+ /// If a given enum is a declaration-only enum then stash it on > >+ /// the side so that at the end of the corpus reading we can resolve > >+ /// it to its definition. > >+ /// > >+ /// @param enom the enum to consider. > >+ void > >+ > maybe_schedule_declaration_only_enum_for_resolution(enum_type_decl_sptr& > enom) > >+ { > >+ if (enom->get_is_declaration_only() > >+ && enom->get_definition_of_declaration() == 0) > > == NULL > It's copy/paste so the right thing to do is adjust this stylistic issue in another commit (or decide not to adjust it). > >+ { > >+ string qn = enom->get_qualified_name(); > >+ string_enums_map::iterator record = > >+ declaration_only_enums().find(qn); > >+ if (record == declaration_only_enums().end()) > >+ declaration_only_enums()[qn].push_back(enom); > >+ else > >+ record->second.push_back(enom); > >+ } > > Isn't that just > declaration_only_enums()[qn].push_back(enom); ? > Yes. Similar comment to previous. > >+ } > > What comes now is a lot of code duplication. There must be a way to > reuse the existing code and I once more suggest using templates with > maybe intermediate functions to get the correct helper functions and > maps. > The code duplication would be eliminable if there were a part of the inheritance hierarchy that represented "type declarations, potentially forward-declared". We'd want a common root for struct, union, class and enum declarations and for the is_declaration_only (et al) machinery to exist at that level. This could be a fairly intrusive change. > I think I would like to see clarified whether we can reduce the > duplication or what I am missing why we need this to be separate. > > I think it would also make it easier to review and judge the patch. > > Cheers, > Matthias > I'll simplify what I can, without touching the hierarchy, for further review. Regards, Giuliano. > >+ > >+ /// Test if a given declaration-only enum has been scheduled for > >+ /// resolution to a defined enum. > >+ /// > >+ /// @param enom the enum to consider for the test. > >+ /// > >+ /// @return true iff @p enom is a declaration-only enum and if > >+ /// it's been scheduled for resolution to a defined enum. > >+ bool > >+ is_decl_only_enum_scheduled_for_resolution(enum_type_decl_sptr& enom) > >+ { > >+ if (enom->get_is_declaration_only()) > >+ return (declaration_only_enums().find(enom->get_qualified_name()) > >+ != declaration_only_enums().end()); > >+ > >+ return false; > >+ } > >+ > >+ /// Walk the declaration-only enums that have been found during > >+ /// the building of the corpus and resolve them to their definitions. > >+ void > >+ resolve_declaration_only_enums() > >+ { > >+ vector<string> resolved_enums; > >+ > >+ for (string_enums_map::iterator i = > >+ declaration_only_enums().begin(); > >+ i != declaration_only_enums().end(); > >+ ++i) > >+ { > >+ bool to_resolve = false; > >+ for (enums_type::iterator j = i->second.begin(); > >+ j != i->second.end(); > >+ ++j) > >+ if ((*j)->get_is_declaration_only() > >+ && ((*j)->get_definition_of_declaration() == 0)) > >+ to_resolve = true; > >+ > >+ if (!to_resolve) > >+ { > >+ resolved_enums.push_back(i->first); > >+ continue; > >+ } > >+ > >+ // Now, for each decl-only enum that have the current name > >+ // 'i->first', let's try to poke at the fully defined enum > >+ // that is defined in the same translation unit as the > >+ // declaration. > >+ // > >+ // If we find one enum (defined in the TU of the declaration) > >+ // that defines the declaration, then the declaration can be > >+ // resolved to that enum. > >+ // > >+ // If no defining enum is found in the TU of the declaration, > >+ // then there are possibly three cases to consider: > >+ // > >+ // 1/ There is exactly one enum that defines the > >+ // declaration and that enum is defined in another TU. In > >+ // this case, the declaration is resolved to that > >+ // definition. > >+ // > >+ // 2/ There are more than one enum that define that > >+ // declaration and none of them is defined in the TU of the > >+ // declaration. In this case, the declaration is left > >+ // unresolved. > >+ // > >+ // 3/ No enum defines the declaration. In this case, the > >+ // declaration is left unresoved. > >+ > >+ // So get the enums that might define the current > >+ // declarations which name is i->first. > >+ const type_base_wptrs_type *enums = > >+ lookup_enum_types(i->first, *current_corpus()); > >+ if (!enums) > >+ continue; > >+ > >+ unordered_map<string, enum_type_decl_sptr> per_tu_enum_map; > >+ for (type_base_wptrs_type::const_iterator c = enums->begin(); > >+ c != enums->end(); > >+ ++c) > >+ { > >+ enum_type_decl_sptr enom = is_enum_type(type_base_sptr(*c)); > >+ ABG_ASSERT(enom); > >+ > >+ enom = is_enum_type(look_through_decl_only_enum(enom)); > >+ if (enom->get_is_declaration_only()) > >+ continue; > >+ > >+ string tu_path = > enom->get_translation_unit()->get_absolute_path(); > >+ if (tu_path.empty()) > >+ continue; > >+ > >+ // Build a map that associates the translation unit path > >+ // to the enum (that potentially defines the declarations > >+ // that we consider) that are defined in that translation unit. > >+ per_tu_enum_map[tu_path] = enom; > >+ } > >+ > >+ if (!per_tu_enum_map.empty()) > >+ { > >+ // Walk the declarations to resolve and resolve them > >+ // either to the definitions that are in the same TU as > >+ // the declaration, or to the definition found elsewhere, > >+ // if there is only one such definition. > >+ for (enums_type::iterator j = i->second.begin(); > >+ j != i->second.end(); > >+ ++j) > >+ { > >+ if ((*j)->get_is_declaration_only() > >+ && ((*j)->get_definition_of_declaration() == 0)) > >+ { > >+ string tu_path = > >+ (*j)->get_translation_unit()->get_absolute_path(); > >+ unordered_map<string, > enum_type_decl_sptr>::const_iterator e = > >+ per_tu_enum_map.find(tu_path); > >+ if (e != per_tu_enum_map.end()) > >+ (*j)->set_definition_of_declaration(e->second); > >+ else if (per_tu_enum_map.size() == 1) > >+ (*j)->set_definition_of_declaration > >+ (per_tu_enum_map.begin()->second); > >+ } > >+ } > >+ resolved_enums.push_back(i->first); > >+ } > >+ } > >+ > >+ size_t num_decl_only_enums = declaration_only_enums().size(), > >+ num_resolved = resolved_enums.size(); > >+ if (show_stats()) > >+ cerr << "resolved " << num_resolved > >+ << " enum declarations out of " > >+ << num_decl_only_enums > >+ << "\n"; > >+ > >+ for (vector<string>::const_iterator i = resolved_enums.begin(); > >+ i != resolved_enums.end(); > >+ ++i) > >+ declaration_only_enums().erase(*i); > >+ > >+ for (string_enums_map::iterator i = declaration_only_enums().begin(); > >+ i != declaration_only_enums().end(); > >+ ++i) > >+ { > >+ if (show_stats()) > >+ { > >+ if (i == declaration_only_enums().begin()) > >+ cerr << "Here are the " > >+ << num_decl_only_enums - num_resolved > >+ << " unresolved enum declarations:\n"; > >+ else > >+ cerr << " " << i->first << "\n"; > >+ } > >+ } > >+ } > >+ > > /// Some functions described by DWARF may have their linkage name > > /// set, but no link to their actual underlying elf symbol. When > > /// these are virtual member functions, comparing the enclosing type > >@@ -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; > >+ } > >+ } > >+ > > string name, linkage_name; > > location loc; > > die_loc_and_name(ctxt, die, loc, name, linkage_name); > >+ bool is_declaration_only = die_is_declaration_only(die); > > > >- bool enum_is_anonymous = false; > >+ bool is_anonymous = false; > > // 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; > > > > 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) > > { > > 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); > > > > // 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); > >- > > // 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); > >+ result->set_is_declaration_only(is_declaration_only); > > result->set_is_artificial(is_artificial); > > ctxt.associate_die_to_type(die, result, where_offset); > >+ > >+ ctxt.maybe_schedule_declaration_only_enum_for_resolution(result); > >+ > > return result; > > } > > > >@@ -16226,6 +16491,24 @@ read_debug_info_into_corpus(read_context& ctxt) > > } > > } > > > >+ { > >+ tools_utils::timer t; > >+ if (ctxt.do_log()) > >+ { > >+ cerr << "resolving declaration only enums ..."; > >+ t.start(); > >+ } > >+ ctxt.resolve_declaration_only_enums(); > >+ if (ctxt.do_log()) > >+ { > >+ t.stop(); > >+ cerr << " DONE@" << ctxt.current_corpus()->get_path() > >+ << ":" > >+ << t > >+ <<"\n"; > >+ } > >+ } > >+ > > { > > tools_utils::timer t; > > if (ctxt.do_log()) > >@@ -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); > >+ else if (!type_suppressed) > > { > > enum_type_decl_sptr e = build_enum_type(ctxt, die, scope, > > where_offset); > >diff --git a/src/abg-ir.cc b/src/abg-ir.cc > >index 27831352..bfdc5679 100644 > >--- a/src/abg-ir.cc > >+++ b/src/abg-ir.cc > >@@ -7674,6 +7674,38 @@ typedef_decl* > > is_typedef(type_base* t) > > {return dynamic_cast<typedef_decl*>(t);} > > > >+/// Test if a type is a enum. This function looks through typedefs. > >+/// > >+/// @parm t the type to consider. > >+/// > >+/// @return the enum_decl if @p t is a enum_decl or null otherwise. > >+enum_type_decl_sptr > >+is_compatible_with_enum_type(const type_base_sptr& t) > >+{ > >+ if (!t) > >+ return enum_type_decl_sptr(); > >+ > >+ // Normally we should strip typedefs entirely, but this is > >+ // potentially costly, especially on binaries with huge changesets > >+ // like the Linux Kernel. So we just get the leaf types for now. > >+ // > >+ // Maybe there should be an option by which users accepts to pay the > >+ // CPU usage toll in exchange for finer filtering? > >+ > >+ // type_base_sptr ty = strip_typedef(t); > >+ type_base_sptr ty = peel_typedef_type(t);; > >+ return is_enum_type(ty); > >+} > >+ > >+/// Test if a type is a enum. This function looks through typedefs. > >+/// > >+/// @parm t the type to consider. > >+/// > >+/// @return the enum_decl if @p t is a enum_decl or null otherwise. > >+enum_type_decl_sptr > >+is_compatible_with_enum_type(const decl_base_sptr& t) > >+{return is_compatible_with_enum_type(is_type(t));} > >+ > > /// Test if a decl is an enum_type_decl > > /// > > /// @param d the decl to test for. > >@@ -8057,6 +8089,50 @@ look_through_decl_only_class(class_or_union_sptr > klass) > > return result; > > } > > > >+/// If a enum is a decl-only enum, get its definition. > >+/// Otherwise, just return the initial enum. > >+/// > >+/// @param the_enum the enum to consider. > >+/// > >+/// @return either the definition of the enum, or the enum itself. > >+enum_type_decl_sptr > >+look_through_decl_only_enum(const enum_type_decl& the_enum) > >+{ > >+ enum_type_decl_sptr enom; > >+ if (the_enum.get_is_declaration_only()) > >+ enom = the_enum.get_definition_of_declaration(); > >+ > >+ if (!enom) > >+ return enom; > >+ > >+ while (enom > >+ && enom->get_is_declaration_only() > >+ && enom->get_definition_of_declaration()) > >+ enom = enom->get_definition_of_declaration(); > >+ > >+ ABG_ASSERT(enom); > >+ return enom; > >+} > >+ > >+/// If a enum is a decl-only enum, get its definition. > >+/// Otherwise, just return the initial enum. > >+/// > >+/// @param enom the enum to consider. > >+/// > >+/// @return either the definition of the enum, or the enum itself. > >+enum_type_decl_sptr > >+look_through_decl_only_enum(enum_type_decl_sptr enom) > >+{ > >+ if (!enom) > >+ return enom; > >+ > >+ enum_type_decl_sptr result = look_through_decl_only_enum(*enom); > >+ if (!result) > >+ result = enom; > >+ > >+ return result; > >+} > >+ > > /// Tests if a declaration is a variable declaration. > > /// > > /// @param decl the decl to test. > >@@ -9915,6 +9991,37 @@ lookup_enum_type(const interned_string& > qualified_name, const corpus& corp) > > return result; > > } > > > >+/// Look into a given corpus to find the enum type*s* that have a > >+/// given qualified name. > >+/// > >+/// @param qualified_name the qualified name of the type to look for. > >+/// > >+/// @param corp the corpus to look into. > >+/// > >+/// @return the vector of enum types that which name is @p > qualified_name. > >+const type_base_wptrs_type * > >+lookup_enum_types(const interned_string& qualified_name, const corpus& > corp) > >+{ > >+ const istring_type_base_wptrs_map_type& m = > corp.get_types().enum_types(); > >+ > >+ return lookup_types_in_map(qualified_name, m); > >+} > >+ > >+/// Look into a given corpus to find the enum type*s* that have a > >+/// given qualified name. > >+/// > >+/// @param qualified_name the qualified name of the type to look for. > >+/// > >+/// @param corp the corpus to look into. > >+/// > >+/// @return the vector of enum types that which name is @p > qualified_name. > >+const type_base_wptrs_type* > >+lookup_enum_types(const string& qualified_name, const corpus& corp) > >+{ > >+ interned_string s = corp.get_environment()->intern(qualified_name); > >+ return lookup_enum_types(s, corp); > >+} > >+ > > /// Look up an @ref enum_type_decl from a given corpus, by its location. > > /// > > /// @param loc the location to consider. > >@@ -14724,17 +14831,26 @@ class enum_type_decl::priv > > { > > type_base_sptr underlying_type_; > > enumerators enumerators_; > >+ decl_base_sptr declaration_; > >+ enum_type_decl_wptr definition_of_declaration_; > >+ enum_type_decl* naked_definition_of_declaration_; > >+ bool is_declaration_only_; > > > > friend class enum_type_decl; > > > >- priv(); > >- > > public: > > > >+ priv() > >+ : naked_definition_of_declaration_(), > >+ is_declaration_only_(true) > >+ {} > >+ > > priv(type_base_sptr underlying_type, > > enumerators& enumerators) > > : underlying_type_(underlying_type), > >- enumerators_(enumerators) > >+ enumerators_(enumerators), > >+ naked_definition_of_declaration_(), > >+ is_declaration_only_(false) > > {} > > }; // end class enum_type_decl::priv > > > >@@ -14775,6 +14891,25 @@ enum_type_decl::enum_type_decl(const string& > name, > > e->set_enum_type(this); > > } > > > >+/// 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); > >+} > >+ > > /// Return the underlying type of the enum. > > type_base_sptr > > enum_type_decl::get_underlying_type() const > >@@ -14790,6 +14925,100 @@ enum_type_decl::enumerators& > > enum_type_decl::get_enumerators() > > {return priv_->enumerators_;} > > > >+/// Set the definition of this declaration-only @ref enum_type_decl. > >+/// > >+/// @param d the new definition to set. > >+void > >+enum_type_decl::set_definition_of_declaration(enum_type_decl_sptr d) > >+{ > >+ ABG_ASSERT(get_is_declaration_only()); > >+ priv_->definition_of_declaration_ = d; > >+ if (d->get_canonical_type()) > >+ type_base::priv_->canonical_type = d->get_canonical_type(); > >+ > >+ priv_->naked_definition_of_declaration_ = d.get(); > >+} > >+ > >+/// If this @ref enum_type_decl_sptr is declaration-only, get its > >+/// definition, if any. > >+/// > >+/// @return the definition of this decl-only enum. > >+const enum_type_decl_sptr > >+enum_type_decl::get_definition_of_declaration() const > >+{ > >+ if (priv_->definition_of_declaration_.expired()) > >+ return enum_type_decl_sptr(); > >+ return enum_type_decl_sptr(priv_->definition_of_declaration_); > >+} > >+ > >+/// If this @ref enum_type_decl is declaration-only, get its > >+/// definition, if any. > >+/// > >+/// Note that this function doesn't return a smart pointer, but rather > >+/// the underlying pointer managed by the smart pointer. So it's as > >+/// fast as possible. This getter is to be used in code paths that are > >+/// proven to be performance hot spots; especially, when comparing > >+/// sensitive types like enums. Those are compared extremely frequently > >+/// and thus, their access to the definition of declaration must be > >+/// fast. > >+/// > >+/// @return the definition of the enum. > >+const enum_type_decl* > >+enum_type_decl::get_naked_definition_of_declaration() const > >+{return priv_->naked_definition_of_declaration_;} > >+ > >+/// 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_;} > >+ > >+/// 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; > >+} > >+ > >+/// Test if a @ref enum_type_decl is a declaration-only @ref > >+/// enum_type_decl. > >+/// > >+/// @return true iff the current @ref enum_type_decl is a > >+/// declaration-only @ref enum_type_decl. > >+bool > >+enum_type_decl::get_is_declaration_only() const > >+{return priv_->is_declaration_only_;} > >+ > >+/// Set a flag saying if the @ref enum_type_decl is a declaration-only > >+/// @ref enum_type_decl. > >+/// > >+/// @param f true if the @ref enum_type_decl is a decalaration-only > >+/// @ref enum_type_decl. > >+void > >+enum_type_decl::set_is_declaration_only(bool f) > >+{ > >+ bool update_types_lookup_map = !f && priv_->is_declaration_only_; > >+ > >+ priv_->is_declaration_only_ = f; > >+ > >+ if (update_types_lookup_map) > >+ if (scope_decl* s = get_scope()) > >+ { > >+ scope_decl::declarations::iterator i; > >+ if (s->find_iterator_for_member(this, i)) > >+ maybe_update_types_lookup_map(*i); > >+ else > >+ ABG_ASSERT_NOT_REACHED; > >+ } > >+} > >+ > > /// Get the pretty representation of the current instance of @ref > > /// enum_type_decl. > > /// > >-- > >2.26.1.301.g55bc3eb7cb9-goog > > >
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,
Hi there. Thanks for the review comments I've done a little more work on this and added a couple of your commits essentially as-is to my series. Also been playing with github mirroring. I can just as easily repost to the list, though. https://github.com/myxoid/libabigail/commits/incomplete-enums https://github.com/myxoid/libabigail/pull/2 - to allow review commenting in situ. Regards, Giuliano. On Fri, 22 May 2020 at 11:03, Dodji Seketeli <dodji@seketeli.org> wrote: > > 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
diff --git a/include/abg-comp-filter.h b/include/abg-comp-filter.h index b8da3156..c486fec5 100644 --- a/include/abg-comp-filter.h +++ b/include/abg-comp-filter.h @@ -68,9 +68,16 @@ bool has_class_decl_only_def_change(const class_or_union_sptr& first, const class_or_union_sptr& second); +bool +has_enum_decl_only_def_change(const enum_type_decl_sptr& first, + const enum_type_decl_sptr& second); + bool has_class_decl_only_def_change(const diff *diff); +bool +has_enum_decl_only_def_change(const diff *diff); + bool has_basic_type_name_change(const diff *); diff --git a/include/abg-comparison.h b/include/abg-comparison.h index 4f60ff99..bb654cef 100644 --- a/include/abg-comparison.h +++ b/include/abg-comparison.h @@ -430,6 +430,12 @@ enum diff_category /// array type of a global variable, but the ELF size of the /// variable didn't change. BENIGN_INFINITE_ARRAY_CHANGE_CATEGORY = 1 << 19, + + /// This means that a diff node in the sub-tree carries a enum type + /// that was declaration-only and that is now defined, or vice + /// versa. + ENUM_DECL_ONLY_DEF_CHANGE_CATEGORY = 1 << 20, + /// A special enumerator that is the logical 'or' all the /// enumerators above. /// @@ -456,6 +462,7 @@ enum diff_category | VAR_TYPE_CV_CHANGE_CATEGORY | VOID_PTR_TO_PTR_CHANGE_CATEGORY | BENIGN_INFINITE_ARRAY_CHANGE_CATEGORY + | ENUM_DECL_ONLY_DEF_CHANGE_CATEGORY }; // enum diff_category diff_category 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; +/// Convenience typedef for a vector of @ref enum_type_decl_sptr +typedef vector<enum_type_decl_sptr> enums_type; + +/// Convenience typedef for a weak pointer to a @ref enum_type_decl. +typedef weak_ptr<enum_type_decl> enum_type_decl_wptr; + class class_or_union; typedef shared_ptr<class_or_union> class_or_union_sptr; @@ -422,6 +428,12 @@ is_typedef(const type_base*); typedef_decl* is_typedef(type_base*); +enum_type_decl_sptr +is_compatible_with_enum_type(const type_base_sptr&); + +enum_type_decl_sptr +is_compatible_with_enum_type(const decl_base_sptr&); + enum_type_decl_sptr is_enum_type(const type_or_decl_base_sptr&); @@ -512,6 +524,12 @@ look_through_decl_only_class(const class_or_union&); class_or_union_sptr look_through_decl_only_class(class_or_union_sptr); +enum_type_decl_sptr +look_through_decl_only_enum(const enum_type_decl&); + +enum_type_decl_sptr +look_through_decl_only_enum(enum_type_decl_sptr); + var_decl* is_var_decl(const type_or_decl_base*); @@ -1071,6 +1089,12 @@ lookup_enum_type(const string&, const corpus&); enum_type_decl_sptr lookup_enum_type(const interned_string&, const corpus&); +const type_base_wptrs_type* +lookup_enum_types(const interned_string&, const corpus&); + +const type_base_wptrs_type* +lookup_enum_types(const string&, const corpus&); + enum_type_decl_sptr lookup_enum_type_per_location(const interned_string&, const corpus&); diff --git a/include/abg-ir.h b/include/abg-ir.h index fda10de5..88ce39e0 100644 --- 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); + type_base_sptr get_underlying_type() const; @@ -2474,6 +2476,27 @@ public: enumerators& get_enumerators(); + bool + get_is_declaration_only() const; + + void + set_is_declaration_only(bool f); + + void + set_definition_of_declaration(enum_type_decl_sptr); + + const enum_type_decl_sptr + get_definition_of_declaration() const; + + const enum_type_decl* + get_naked_definition_of_declaration() const; + + decl_base_sptr + get_earlier_declaration() const; + + void + set_earlier_declaration(decl_base_sptr declaration); + virtual string get_pretty_representation(bool internal = false, bool qualified_name = true) const; diff --git a/src/abg-comp-filter.cc b/src/abg-comp-filter.cc index 75df901c..c8a031d3 100644 --- a/src/abg-comp-filter.cc +++ b/src/abg-comp-filter.cc @@ -118,6 +118,25 @@ there_is_a_decl_only_class(const class_decl_sptr& class1, return false; } +/// Test if there is a enum that is declaration-only among the two +/// enumes in parameter. +/// +/// @param enum1 the first enum to consider. +/// +/// @param enum2 the second enum to consider. +/// +/// @return true if either enumes are declaration-only, false +/// otherwise. +static bool +there_is_a_decl_only_enum(const enum_type_decl_sptr& enum1, + const enum_type_decl_sptr& enum2) +{ + if ((enum1 && enum1->get_is_declaration_only()) + || (enum2 && enum2->get_is_declaration_only())) + return true; + return false; +} + /// Test if the diff involves a declaration-only class. /// /// @param diff the class diff to consider. @@ -146,7 +165,9 @@ type_size_changed(const type_base_sptr f, const type_base_sptr s) || f->get_size_in_bits() == 0 || s->get_size_in_bits() == 0 || there_is_a_decl_only_class(is_compatible_with_class_type(f), - is_compatible_with_class_type(s))) + is_compatible_with_class_type(s)) + || there_is_a_decl_only_enum(is_compatible_with_enum_type(f), + is_compatible_with_enum_type(s))) return false; return f->get_size_in_bits() != s->get_size_in_bits(); @@ -937,13 +958,38 @@ has_class_decl_only_def_change(const class_or_union_sptr& first, return (f->get_is_declaration_only() != s->get_is_declaration_only()); } +/// Test if two @ref enum_sptr are different just by the +/// fact that one is decl-only and the other one is defined. +/// +/// @param first the first enum to consider. +/// +/// @param second the second enum to consider. +/// +/// @return true iff the two arguments are different just by the fact +/// that one is decl-only and the other one is defined. +bool +has_enum_decl_only_def_change(const enum_type_decl_sptr& first, + const enum_type_decl_sptr& second) +{ + if (!first || !second) + return false; + + enum_type_decl_sptr f = look_through_decl_only_enum(first); + enum_type_decl_sptr s = look_through_decl_only_enum(second); + + if (f->get_qualified_name() != s->get_qualified_name()) + return false; + + return (f->get_is_declaration_only() != s->get_is_declaration_only()); +} + /// 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 @@ -961,6 +1007,28 @@ has_class_decl_only_def_change(const diff *diff) return has_class_decl_only_def_change(f, s); } +/// Test if a enum_diff carries a change in which the two +/// enumes 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 enum_diff carries a change in which +/// the two enumes are different by the fact that one is a decl-only +/// and the other one is defined. +bool +has_enum_decl_only_def_change(const diff *diff) +{ + const enum_diff *d = dynamic_cast<const enum_diff*>(diff); + if (!d) + return false; + + enum_type_decl_sptr f = look_through_decl_only_enum(d->first_enum()); + enum_type_decl_sptr s = look_through_decl_only_enum(d->second_enum()); + + return has_enum_decl_only_def_change(f, s); +} + /// Test if a diff node carries a basic type name change. /// /// @param d the diff node to consider. @@ -1501,6 +1569,9 @@ categorize_harmless_diff_node(diff *d, bool pre) if (has_class_decl_only_def_change(d)) category |= CLASS_DECL_ONLY_DEF_CHANGE_CATEGORY; + if (has_enum_decl_only_def_change(d)) + category |= ENUM_DECL_ONLY_DEF_CHANGE_CATEGORY; + if (access_changed(f, s)) category |= ACCESS_CHANGE_CATEGORY; @@ -1586,6 +1657,7 @@ categorize_harmful_diff_node(diff *d, bool pre) // // TODO: be more specific -- not all size changes are harmful. if (!has_class_decl_only_def_change(d) + && !has_enum_decl_only_def_change(d) && (type_size_changed(f, s) || data_member_offset_changed(f, s) || non_static_data_member_type_size_changed(f, s) diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc index 46bf9e30..40d80073 100644 --- a/src/abg-comparison.cc +++ b/src/abg-comparison.cc @@ -2952,7 +2952,8 @@ get_default_harmless_categories_bitmap() | abigail::comparison::FN_RETURN_TYPE_CV_CHANGE_CATEGORY | abigail::comparison::VAR_TYPE_CV_CHANGE_CATEGORY | abigail::comparison::VOID_PTR_TO_PTR_CHANGE_CATEGORY - | abigail::comparison::BENIGN_INFINITE_ARRAY_CHANGE_CATEGORY); + | abigail::comparison::BENIGN_INFINITE_ARRAY_CHANGE_CATEGORY + | abigail::comparison::ENUM_DECL_ONLY_DEF_CHANGE_CATEGORY); } /// Getter of a bitmap made of the set of change categories that are @@ -3113,7 +3114,7 @@ operator<<(ostream& o, diff_category c) emitted_a_category |= true; } - if (c & FN_RETURN_TYPE_CV_CHANGE_CATEGORY) + if (c & FN_RETURN_TYPE_CV_CHANGE_CATEGORY) { if (emitted_a_category) o << "|"; @@ -3121,7 +3122,7 @@ operator<<(ostream& o, diff_category c) emitted_a_category |= true; } - if (c & VAR_TYPE_CV_CHANGE_CATEGORY) + if (c & VAR_TYPE_CV_CHANGE_CATEGORY) { if (emitted_a_category) o << "|"; @@ -3129,7 +3130,7 @@ operator<<(ostream& o, diff_category c) emitted_a_category |= true; } - if (c & VOID_PTR_TO_PTR_CHANGE_CATEGORY) + if (c & VOID_PTR_TO_PTR_CHANGE_CATEGORY) { if (emitted_a_category) o << "|"; @@ -3137,7 +3138,7 @@ operator<<(ostream& o, diff_category c) emitted_a_category |= true; } - if (c & BENIGN_INFINITE_ARRAY_CHANGE_CATEGORY) + if (c & BENIGN_INFINITE_ARRAY_CHANGE_CATEGORY) { if (emitted_a_category) o << "|"; @@ -3145,6 +3146,14 @@ operator<<(ostream& o, diff_category c) emitted_a_category |= true; } + if (c & ENUM_DECL_ONLY_DEF_CHANGE_CATEGORY) + { + if (emitted_a_category) + o << "|"; + o << "ENUM_DECL_ONLY_DEF_CHANGE_CATEGORY"; + emitted_a_category |= true; + } + return o; } @@ -10707,6 +10716,8 @@ struct leaf_diff_node_marker_visitor : public diff_node_visitor && !is_anonymous_class_or_union_diff(d) // Don't show decl-only-ness changes of classes either. && !filtering::has_class_decl_only_def_change(d) + // Same for enums. + && !filtering::has_enum_decl_only_def_change(d) // Sometime, we can encounter artifacts of bogus DWARF that // yield a diff node for a decl-only class (and empty class // with the is_declaration flag set) that carries a non-zero diff --git a/src/abg-default-reporter.cc b/src/abg-default-reporter.cc index 04e2bb76..19dec4cb 100644 --- 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(); - 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(); + + // Report enum decl-only <-> definition changes. + if (ctxt->get_allowed_category() & ENUM_DECL_ONLY_DEF_CHANGE_CATEGORY) + if (filtering::has_enum_decl_only_def_change(first, second)) + { + string was = + first->get_is_declaration_only() + ? " was a declaration-only enum type" + : " was a defined enum type"; + + string is_now = + second->get_is_declaration_only() + ? " and is now a declaration-only enum type" + : " and is now a defined enum type"; + + out << indent << "enum type " << name << was << is_now << "\n"; + return; + } + + report_name_size_and_alignment_changes(first, second, ctxt, out, indent); - maybe_report_diff_for_member(first, second, d.context(), out, indent); + maybe_report_diff_for_member(first, second, ctxt, out, indent); //underlying type d.underlying_type_diff()->report(out, indent); @@ -165,12 +185,12 @@ default_reporter::report(const enum_diff& d, ostream& out, << "' from value '" << i->first.get_value() << "' to '" << i->second.get_value() << "'"; - report_loc_info(second, *d.context(), out); + report_loc_info(second, *ctxt, out); out << "\n"; } } - if (d.context()->show_leaf_changes_only()) + if (ctxt->show_leaf_changes_only()) maybe_report_interfaces_impacted_by_diff(&d, out, indent); 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. if (ctxt->get_allowed_category() & CLASS_DECL_ONLY_DEF_CHANGE_CATEGORY) if (filtering::has_class_decl_only_def_change(first, second)) { diff --git a/src/abg-dwarf-reader.cc b/src/abg-dwarf-reader.cc index 850281ad..5748460e 100644 --- a/src/abg-dwarf-reader.cc +++ 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; + /// Convenience typedef for a map which key the offset of a dwarf die /// and which value is the corresponding function_decl. typedef unordered_map<Dwarf_Off, function_decl_sptr> die_function_decl_map_type; @@ -198,6 +203,10 @@ typedef unordered_map<Dwarf_Off, Dwarf_Off> offset_offset_map_type; /// value is a vector of smart pointer to a class. typedef unordered_map<string, classes_type> string_classes_map; +/// Convenience typedef for a map which key is a string and which +/// value is a vector of smart pointer to a enum. +typedef unordered_map<string, enums_type> string_enums_map; + /// The abstraction of the place where a partial unit has been /// imported. This is what the DW_TAG_imported_unit DIE expresses. /// @@ -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_; die_function_type_map_type die_wip_function_types_map_; die_function_type_map_type alternate_die_wip_function_types_map_; die_function_type_map_type type_unit_die_wip_function_types_map_; @@ -2282,6 +2294,7 @@ public: vector<Dwarf_Off> type_unit_types_to_canonicalize_; vector<type_base_sptr> extra_types_to_canonicalize_; string_classes_map decl_only_classes_map_; + string_enums_map decl_only_enums_map_; die_tu_map_type die_tu_map_; corpus_group_sptr cur_corpus_group_; corpus_sptr cur_corpus_; @@ -4340,6 +4353,44 @@ public: return die_wip_classes_map_; } + /// 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);} + + /// 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) + { + switch (source) + { + case PRIMARY_DEBUG_INFO_DIE_SOURCE: + break; + case ALT_DEBUG_INFO_DIE_SOURCE: + return alternate_die_wip_enums_map_; + case TYPE_UNIT_DIE_SOURCE: + return type_unit_die_wip_enums_map_; + case NO_DEBUG_INFO_DIE_SOURCE: + case NUMBER_OF_DIE_SOURCES: + ABG_ASSERT_NOT_REACHED; + } + return die_wip_enums_map_; + } + /// Getter for a map that associates a die (that represents a /// function type) whith a function type, while the function type is /// being constructed (WIP == work in progress). @@ -4620,6 +4671,203 @@ public: } } + /// Getter for the map of declaration-only enums that are to be + /// resolved to their definition enums by the end of the corpus + /// loading. + /// + /// @return a map of string -> vector of enums where the key is + /// the fully qualified name of the enum and the value is the + /// vector of declaration-only enum. + const string_enums_map& + declaration_only_enums() const + {return decl_only_enums_map_;} + + /// Getter for the map of declaration-only enums that are to be + /// resolved to their definition enums by the end of the corpus + /// loading. + /// + /// @return a map of string -> vector of enums where the key is + /// the fully qualified name of the enum and the value is the + /// vector of declaration-only enum. + string_enums_map& + declaration_only_enums() + {return decl_only_enums_map_;} + + /// If a given enum is a declaration-only enum then stash it on + /// the side so that at the end of the corpus reading we can resolve + /// it to its definition. + /// + /// @param enom the enum to consider. + void + maybe_schedule_declaration_only_enum_for_resolution(enum_type_decl_sptr& enom) + { + if (enom->get_is_declaration_only() + && enom->get_definition_of_declaration() == 0) + { + string qn = enom->get_qualified_name(); + string_enums_map::iterator record = + declaration_only_enums().find(qn); + if (record == declaration_only_enums().end()) + declaration_only_enums()[qn].push_back(enom); + else + record->second.push_back(enom); + } + } + + /// Test if a given declaration-only enum has been scheduled for + /// resolution to a defined enum. + /// + /// @param enom the enum to consider for the test. + /// + /// @return true iff @p enom is a declaration-only enum and if + /// it's been scheduled for resolution to a defined enum. + bool + is_decl_only_enum_scheduled_for_resolution(enum_type_decl_sptr& enom) + { + if (enom->get_is_declaration_only()) + return (declaration_only_enums().find(enom->get_qualified_name()) + != declaration_only_enums().end()); + + return false; + } + + /// Walk the declaration-only enums that have been found during + /// the building of the corpus and resolve them to their definitions. + void + resolve_declaration_only_enums() + { + vector<string> resolved_enums; + + for (string_enums_map::iterator i = + declaration_only_enums().begin(); + i != declaration_only_enums().end(); + ++i) + { + bool to_resolve = false; + for (enums_type::iterator j = i->second.begin(); + j != i->second.end(); + ++j) + if ((*j)->get_is_declaration_only() + && ((*j)->get_definition_of_declaration() == 0)) + to_resolve = true; + + if (!to_resolve) + { + resolved_enums.push_back(i->first); + continue; + } + + // Now, for each decl-only enum that have the current name + // 'i->first', let's try to poke at the fully defined enum + // that is defined in the same translation unit as the + // declaration. + // + // If we find one enum (defined in the TU of the declaration) + // that defines the declaration, then the declaration can be + // resolved to that enum. + // + // If no defining enum is found in the TU of the declaration, + // then there are possibly three cases to consider: + // + // 1/ There is exactly one enum that defines the + // declaration and that enum is defined in another TU. In + // this case, the declaration is resolved to that + // definition. + // + // 2/ There are more than one enum that define that + // declaration and none of them is defined in the TU of the + // declaration. In this case, the declaration is left + // unresolved. + // + // 3/ No enum defines the declaration. In this case, the + // declaration is left unresoved. + + // So get the enums that might define the current + // declarations which name is i->first. + const type_base_wptrs_type *enums = + lookup_enum_types(i->first, *current_corpus()); + if (!enums) + continue; + + unordered_map<string, enum_type_decl_sptr> per_tu_enum_map; + for (type_base_wptrs_type::const_iterator c = enums->begin(); + c != enums->end(); + ++c) + { + enum_type_decl_sptr enom = is_enum_type(type_base_sptr(*c)); + ABG_ASSERT(enom); + + enom = is_enum_type(look_through_decl_only_enum(enom)); + if (enom->get_is_declaration_only()) + continue; + + string tu_path = enom->get_translation_unit()->get_absolute_path(); + if (tu_path.empty()) + continue; + + // Build a map that associates the translation unit path + // to the enum (that potentially defines the declarations + // that we consider) that are defined in that translation unit. + per_tu_enum_map[tu_path] = enom; + } + + if (!per_tu_enum_map.empty()) + { + // Walk the declarations to resolve and resolve them + // either to the definitions that are in the same TU as + // the declaration, or to the definition found elsewhere, + // if there is only one such definition. + for (enums_type::iterator j = i->second.begin(); + j != i->second.end(); + ++j) + { + if ((*j)->get_is_declaration_only() + && ((*j)->get_definition_of_declaration() == 0)) + { + string tu_path = + (*j)->get_translation_unit()->get_absolute_path(); + unordered_map<string, enum_type_decl_sptr>::const_iterator e = + per_tu_enum_map.find(tu_path); + if (e != per_tu_enum_map.end()) + (*j)->set_definition_of_declaration(e->second); + else if (per_tu_enum_map.size() == 1) + (*j)->set_definition_of_declaration + (per_tu_enum_map.begin()->second); + } + } + resolved_enums.push_back(i->first); + } + } + + size_t num_decl_only_enums = declaration_only_enums().size(), + num_resolved = resolved_enums.size(); + if (show_stats()) + cerr << "resolved " << num_resolved + << " enum declarations out of " + << num_decl_only_enums + << "\n"; + + for (vector<string>::const_iterator i = resolved_enums.begin(); + i != resolved_enums.end(); + ++i) + declaration_only_enums().erase(*i); + + for (string_enums_map::iterator i = declaration_only_enums().begin(); + i != declaration_only_enums().end(); + ++i) + { + if (show_stats()) + { + if (i == declaration_only_enums().begin()) + cerr << "Here are the " + << num_decl_only_enums - num_resolved + << " unresolved enum declarations:\n"; + else + cerr << " " << i->first << "\n"; + } + } + } + /// Some functions described by DWARF may have their linkage name /// set, but no link to their actual underlying elf symbol. When /// these are virtual member functions, comparing the enclosing type @@ -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; + } + } + string name, linkage_name; location loc; die_loc_and_name(ctxt, die, loc, name, linkage_name); + bool is_declaration_only = die_is_declaration_only(die); - bool enum_is_anonymous = false; + bool is_anonymous = false; // 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; 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) { 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); // 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); - // 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); + result->set_is_declaration_only(is_declaration_only); result->set_is_artificial(is_artificial); ctxt.associate_die_to_type(die, result, where_offset); + + ctxt.maybe_schedule_declaration_only_enum_for_resolution(result); + return result; } @@ -16226,6 +16491,24 @@ read_debug_info_into_corpus(read_context& ctxt) } } + { + tools_utils::timer t; + if (ctxt.do_log()) + { + cerr << "resolving declaration only enums ..."; + t.start(); + } + ctxt.resolve_declaration_only_enums(); + if (ctxt.do_log()) + { + t.stop(); + cerr << " DONE@" << ctxt.current_corpus()->get_path() + << ":" + << t + <<"\n"; + } + } + { tools_utils::timer t; if (ctxt.do_log()) @@ -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); + else if (!type_suppressed) { enum_type_decl_sptr e = build_enum_type(ctxt, die, scope, where_offset); diff --git a/src/abg-ir.cc b/src/abg-ir.cc index 27831352..bfdc5679 100644 --- a/src/abg-ir.cc +++ b/src/abg-ir.cc @@ -7674,6 +7674,38 @@ typedef_decl* is_typedef(type_base* t) {return dynamic_cast<typedef_decl*>(t);} +/// Test if a type is a enum. This function looks through typedefs. +/// +/// @parm t the type to consider. +/// +/// @return the enum_decl if @p t is a enum_decl or null otherwise. +enum_type_decl_sptr +is_compatible_with_enum_type(const type_base_sptr& t) +{ + if (!t) + return enum_type_decl_sptr(); + + // Normally we should strip typedefs entirely, but this is + // potentially costly, especially on binaries with huge changesets + // like the Linux Kernel. So we just get the leaf types for now. + // + // Maybe there should be an option by which users accepts to pay the + // CPU usage toll in exchange for finer filtering? + + // type_base_sptr ty = strip_typedef(t); + type_base_sptr ty = peel_typedef_type(t);; + return is_enum_type(ty); +} + +/// Test if a type is a enum. This function looks through typedefs. +/// +/// @parm t the type to consider. +/// +/// @return the enum_decl if @p t is a enum_decl or null otherwise. +enum_type_decl_sptr +is_compatible_with_enum_type(const decl_base_sptr& t) +{return is_compatible_with_enum_type(is_type(t));} + /// Test if a decl is an enum_type_decl /// /// @param d the decl to test for. @@ -8057,6 +8089,50 @@ look_through_decl_only_class(class_or_union_sptr klass) return result; } +/// If a enum is a decl-only enum, get its definition. +/// Otherwise, just return the initial enum. +/// +/// @param the_enum the enum to consider. +/// +/// @return either the definition of the enum, or the enum itself. +enum_type_decl_sptr +look_through_decl_only_enum(const enum_type_decl& the_enum) +{ + enum_type_decl_sptr enom; + if (the_enum.get_is_declaration_only()) + enom = the_enum.get_definition_of_declaration(); + + if (!enom) + return enom; + + while (enom + && enom->get_is_declaration_only() + && enom->get_definition_of_declaration()) + enom = enom->get_definition_of_declaration(); + + ABG_ASSERT(enom); + return enom; +} + +/// If a enum is a decl-only enum, get its definition. +/// Otherwise, just return the initial enum. +/// +/// @param enom the enum to consider. +/// +/// @return either the definition of the enum, or the enum itself. +enum_type_decl_sptr +look_through_decl_only_enum(enum_type_decl_sptr enom) +{ + if (!enom) + return enom; + + enum_type_decl_sptr result = look_through_decl_only_enum(*enom); + if (!result) + result = enom; + + return result; +} + /// Tests if a declaration is a variable declaration. /// /// @param decl the decl to test. @@ -9915,6 +9991,37 @@ lookup_enum_type(const interned_string& qualified_name, const corpus& corp) return result; } +/// Look into a given corpus to find the enum type*s* that have a +/// given qualified name. +/// +/// @param qualified_name the qualified name of the type to look for. +/// +/// @param corp the corpus to look into. +/// +/// @return the vector of enum types that which name is @p qualified_name. +const type_base_wptrs_type * +lookup_enum_types(const interned_string& qualified_name, const corpus& corp) +{ + const istring_type_base_wptrs_map_type& m = corp.get_types().enum_types(); + + return lookup_types_in_map(qualified_name, m); +} + +/// Look into a given corpus to find the enum type*s* that have a +/// given qualified name. +/// +/// @param qualified_name the qualified name of the type to look for. +/// +/// @param corp the corpus to look into. +/// +/// @return the vector of enum types that which name is @p qualified_name. +const type_base_wptrs_type* +lookup_enum_types(const string& qualified_name, const corpus& corp) +{ + interned_string s = corp.get_environment()->intern(qualified_name); + return lookup_enum_types(s, corp); +} + /// Look up an @ref enum_type_decl from a given corpus, by its location. /// /// @param loc the location to consider. @@ -14724,17 +14831,26 @@ class enum_type_decl::priv { type_base_sptr underlying_type_; enumerators enumerators_; + decl_base_sptr declaration_; + enum_type_decl_wptr definition_of_declaration_; + enum_type_decl* naked_definition_of_declaration_; + bool is_declaration_only_; friend class enum_type_decl; - priv(); - public: + priv() + : naked_definition_of_declaration_(), + is_declaration_only_(true) + {} + priv(type_base_sptr underlying_type, enumerators& enumerators) : underlying_type_(underlying_type), - enumerators_(enumerators) + enumerators_(enumerators), + naked_definition_of_declaration_(), + is_declaration_only_(false) {} }; // end class enum_type_decl::priv @@ -14775,6 +14891,25 @@ enum_type_decl::enum_type_decl(const string& name, e->set_enum_type(this); } +/// 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); +} + /// Return the underlying type of the enum. type_base_sptr enum_type_decl::get_underlying_type() const @@ -14790,6 +14925,100 @@ enum_type_decl::enumerators& enum_type_decl::get_enumerators() {return priv_->enumerators_;} +/// Set the definition of this declaration-only @ref enum_type_decl. +/// +/// @param d the new definition to set. +void +enum_type_decl::set_definition_of_declaration(enum_type_decl_sptr d) +{ + ABG_ASSERT(get_is_declaration_only()); + priv_->definition_of_declaration_ = d; + if (d->get_canonical_type()) + type_base::priv_->canonical_type = d->get_canonical_type(); + + priv_->naked_definition_of_declaration_ = d.get(); +} + +/// If this @ref enum_type_decl_sptr is declaration-only, get its +/// definition, if any. +/// +/// @return the definition of this decl-only enum. +const enum_type_decl_sptr +enum_type_decl::get_definition_of_declaration() const +{ + if (priv_->definition_of_declaration_.expired()) + return enum_type_decl_sptr(); + return enum_type_decl_sptr(priv_->definition_of_declaration_); +} + +/// If this @ref enum_type_decl is declaration-only, get its +/// definition, if any. +/// +/// Note that this function doesn't return a smart pointer, but rather +/// the underlying pointer managed by the smart pointer. So it's as +/// fast as possible. This getter is to be used in code paths that are +/// proven to be performance hot spots; especially, when comparing +/// sensitive types like enums. Those are compared extremely frequently +/// and thus, their access to the definition of declaration must be +/// fast. +/// +/// @return the definition of the enum. +const enum_type_decl* +enum_type_decl::get_naked_definition_of_declaration() const +{return priv_->naked_definition_of_declaration_;} + +/// 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_;} + +/// 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; +} + +/// Test if a @ref enum_type_decl is a declaration-only @ref +/// enum_type_decl. +/// +/// @return true iff the current @ref enum_type_decl is a +/// declaration-only @ref enum_type_decl. +bool +enum_type_decl::get_is_declaration_only() const +{return priv_->is_declaration_only_;} + +/// Set a flag saying if the @ref enum_type_decl is a declaration-only +/// @ref enum_type_decl. +/// +/// @param f true if the @ref enum_type_decl is a decalaration-only +/// @ref enum_type_decl. +void +enum_type_decl::set_is_declaration_only(bool f) +{ + bool update_types_lookup_map = !f && priv_->is_declaration_only_; + + priv_->is_declaration_only_ = f; + + if (update_types_lookup_map) + if (scope_decl* s = get_scope()) + { + scope_decl::declarations::iterator i; + if (s->find_iterator_for_member(this, i)) + maybe_update_types_lookup_map(*i); + else + ABG_ASSERT_NOT_REACHED; + } +} + /// Get the pretty representation of the current instance of @ref /// enum_type_decl. ///
This is an almost blind attempt at addition of support for incomplete, also known as forward-declared, enum types. I've not made any attempt to refactor or share logic with the struct/union code and I've probably got other things wrong. Signed-off-by: Giuliano Procida <gprocida@google.com> --- include/abg-comp-filter.h | 7 + include/abg-comparison.h | 7 + include/abg-fwd.h | 26 ++- include/abg-ir.h | 23 +++ src/abg-comp-filter.cc | 78 ++++++++- src/abg-comparison.cc | 21 ++- src/abg-default-reporter.cc | 34 +++- src/abg-dwarf-reader.cc | 308 +++++++++++++++++++++++++++++++++++- src/abg-ir.cc | 235 ++++++++++++++++++++++++++- 9 files changed, 713 insertions(+), 26 deletions(-)