From patchwork Thu Apr 23 13:03:55 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Giuliano Procida X-Patchwork-Id: 39125 From: gprocida@google.com (Giuliano Procida) Date: Thu, 23 Apr 2020 14:03:55 +0100 Subject: [RFC PATCH 1/2] Support declaration-only enums. In-Reply-To: <20200423130356.93136-1-gprocida@google.com> References: <20200423130356.93136-1-gprocida@google.com> Message-ID: <20200423130356.93136-2-gprocida@google.com> 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 --- 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_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_sptr; +/// Convenience typedef for a vector of @ref enum_type_decl_sptr +typedef vector enums_type; + +/// Convenience typedef for a weak pointer to a @ref enum_type_decl. +typedef weak_ptr enum_type_decl_wptr; + class class_or_union; typedef shared_ptr 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(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 die_class_map_type; /// corresponding class_or_union_sptr. typedef unordered_map 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 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 die_function_decl_map_type; @@ -198,6 +203,10 @@ typedef unordered_map offset_offset_map_type; /// value is a vector of smart pointer to a class. typedef unordered_map 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_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 type_unit_types_to_canonicalize_; vector 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(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 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 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::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::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(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(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(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. /// From patchwork Thu Apr 23 13:03:56 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Giuliano Procida X-Patchwork-Id: 39126 From: gprocida@google.com (Giuliano Procida) Date: Thu, 23 Apr 2020 14:03:56 +0100 Subject: [RFC PATCH 2/2] Add tests for declaration-only enums. In-Reply-To: <20200423130356.93136-1-gprocida@google.com> References: <20200423130356.93136-1-gprocida@google.com> Message-ID: <20200423130356.93136-3-gprocida@google.com> --- tests/data/Makefile.am | 5 +++++ .../test-decl-enum-report.txt | 17 +++++++++++++++++ .../data/test-abidiff-exit/test-decl-enum-v0.c | 5 +++++ .../data/test-abidiff-exit/test-decl-enum-v0.o | Bin 0 -> 3048 bytes .../data/test-abidiff-exit/test-decl-enum-v1.c | 5 +++++ .../data/test-abidiff-exit/test-decl-enum-v1.o | Bin 0 -> 3048 bytes tests/test-abidiff-exit.cc | 9 +++++++++ 7 files changed, 41 insertions(+) create mode 100644 tests/data/test-abidiff-exit/test-decl-enum-report.txt create mode 100644 tests/data/test-abidiff-exit/test-decl-enum-v0.c create mode 100644 tests/data/test-abidiff-exit/test-decl-enum-v0.o create mode 100644 tests/data/test-abidiff-exit/test-decl-enum-v1.c create mode 100644 tests/data/test-abidiff-exit/test-decl-enum-v1.o diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am index a1b9bf64..c036f349 100644 --- a/tests/data/Makefile.am +++ b/tests/data/Makefile.am @@ -151,6 +151,11 @@ test-abidiff-exit/test-decl-struct-v0.o \ test-abidiff-exit/test-decl-struct-v1.c \ test-abidiff-exit/test-decl-struct-v1.o \ test-abidiff-exit/test-decl-struct-report.txt \ +test-abidiff-exit/test-decl-enum-v0.c \ +test-abidiff-exit/test-decl-enum-v0.o \ +test-abidiff-exit/test-decl-enum-v1.c \ +test-abidiff-exit/test-decl-enum-v1.o \ +test-abidiff-exit/test-decl-enum-report.txt \ \ test-diff-dwarf/test0-v0.cc \ test-diff-dwarf/test0-v0.o \ diff --git a/tests/data/test-abidiff-exit/test-decl-enum-report.txt b/tests/data/test-abidiff-exit/test-decl-enum-report.txt new file mode 100644 index 00000000..e46ebfa6 --- /dev/null +++ b/tests/data/test-abidiff-exit/test-decl-enum-report.txt @@ -0,0 +1,17 @@ +Functions changes summary: 0 Removed, 2 Changed, 0 Added functions +Variables changes summary: 0 Removed, 0 Changed, 0 Added variable + +2 functions with some indirect sub-type change: + + [C] 'function void reg1(const embodied_enum*)' at test-decl-enum-v1.c:4:1 has some indirect sub-type changes: + parameter 1 of type 'const embodied_enum*' has sub-type changes: + in pointed to type 'const embodied_enum': + in unqualified underlying type 'enum embodied_enum' at test-decl-enum-v1.c:1:1: + enum type enum embodied_enum was a declaration-only enum type and is now a defined enum type + + [C] 'function void reg2(const disembodied_enum*)' at test-decl-enum-v1.c:5:1 has some indirect sub-type changes: + parameter 1 of type 'const disembodied_enum*' has sub-type changes: + in pointed to type 'const disembodied_enum': + in unqualified underlying type 'enum disembodied_enum': + enum type enum disembodied_enum was a defined enum type and is now a declaration-only enum type + diff --git a/tests/data/test-abidiff-exit/test-decl-enum-v0.c b/tests/data/test-abidiff-exit/test-decl-enum-v0.c new file mode 100644 index 00000000..d5672618 --- /dev/null +++ b/tests/data/test-abidiff-exit/test-decl-enum-v0.c @@ -0,0 +1,5 @@ +enum embodied_enum; +enum disembodied_enum { X }; + +void reg1(const enum embodied_enum * foo) { (void)foo; } +void reg2(const enum disembodied_enum * foo) { (void)foo; } diff --git a/tests/data/test-abidiff-exit/test-decl-enum-v0.o b/tests/data/test-abidiff-exit/test-decl-enum-v0.o new file mode 100644 index 0000000000000000000000000000000000000000..b4c0b06a089ec6adbfa81b1a619ae30087e54f9d GIT binary patch literal 3048 zcmbtWUuzsy6hE^wo1Hk#Hp$xBG=+{(+eDplH%QIaCa$Ko2^OhB#TN;)J2$(7+1WBP zTT?|XqC&xk3W5lt4}JmB7hejZzU!Cp!8d&qe9&`d?%CYjOcC_J+KSmHb@am$eB+q~vtHPqTc9wV6Q4e}KyI}vx)qwOM&$&iJ&Jx5 zVA=x8a&ZBRcpRVne88rfCZwL<1VrX4J|;Yht6}_ApTRZ1Tf77VGLdFpD0;h|h{r?k zX4CKGuGPn2y2ydzvFh>ns{R_T6%)P3aT@hw5xP;Fff#m2VI;zB@`o@=4+2+SyzXwa zo^sp%ir;cqnyq%L)oi=oFdN5WtuJDcL_HLPq&HaGTRrccf712(uD27Ux8mL)8O5V< z>W$+!qB!)jpt~#5(wY;|ZvieV@I*Ge0dQqwi6; zm%-SZH5QN5?7Mh8rx2%yDF3>it_4 z0FI&EymAiQ*(W8=GVlNLMNHoK0ra1=KJuKIaC%E>ETXw$1IBMlnd%~9{EI>aYDyxy zxk=L?zf2$!*{iMU=jeW%MJZFG5^?-O0(eO>jDIF&nutW4m(Sya0-nVAM9Lq@o$aLv z-)i=JgM3z$x5~!2&q`j8)4mVxN}SF{J)e$-bN%3_w}#m4xMxYR517luUIu;`WC8fy zGzC8qyMZ5y?zq1l#XBR^<$TcXCgSD+b&v#cALobavKz&MT`&dsz0q(eaIX3BC=))8 zFE7o4-s{py47PWYU?^na|8t{zr!(Yp=5QU!=mVW>ReBf&!t@F7ifW|ry8k)U(RcY@ z{afg($E0+e|0fa@(ru~V=pR8xu_5*s0+Cjw1AT7M==iQi9wExp9f@ zwGxbMzm)z>5){&Ny@0y1`fYik>`(R6bk*w*5z}KL1V2lV>QhO#-9}|u{#BWu&x!KW zeADyaK#XEG@loltY|^h(5P8ne`&$yz{eO`D^nIzse!RbrO6FC&-_#+YD0EwzgM{hk zze|AUBZuJkXPM?=+4)^7xj*{#Vt+o@vfLDVQ#RB<7EHe2X1gf;sEc>&e)U5_5x4ZH}C!4-}$@u-n@Rb z#26@Ium)pIp#Te&vD^}33+gZnH#hcv+t~Z;ul)~y(!?Lo!xmmuk?e?&HFjK+OC(q9 zP2`MCT*1Nm0GaI}E8C*(1d71QVj$5WhQyxSMPAt+4A8o`hjQu59#qLxvB@-9eSka< zw+q6*Cr=S>pCg&VC#%#0uo?Dzt?pR0`PvMdtxP|MR-0M1HOFx_>zABYou$BJ9tXl4SevZ<#PCR;Jj>0Ns49hK>_0kD|stuI<@*H-3 z6jM+xxIjHoH~FN5L`+}55s1Qd{496@Ib(mV)H7Df-_H0s7YbfY*cOb|vX@3#kG#KSF03)kMh>8>|kahv|K z-*A_g8qG#ysp)$CY#8&^E{}N2_6|?sV14&BDsfnxTfEd{>lM$)No?8xcI{r5N`OjesZIrkihfJ2- za?@a8L=jo?u9-F~Klmn$S+vWnSuL2 z>!5D-Nr@|6jfIDBd?H3Oeg@-5_Z?+bnQ)rrGJZre`2Y%6GorqT6#jLt0^UwRv~#0N zgYsey3xDzcNZr?Ps{2stbWw>^{#*liM+ypmEcN&0sOqZqxM%pEk@~)YeB(^5A5n@_=AiPPHX>(erD)j#;@ojzW39J3@JdrHdqZU%lBWC8f?GzCB5 zJAoha_OQDZ#oGh42{~xD6Mp-EIY@%Ii!X;BvJ=HzgV2eGA~pY91EBY>X#H*!>>vHTRJ>Z(qS};uLtdz!Y?%Hh^baxPX-~|%hY);6 YO(C7mhY>gDPkD>t@5}z5wtp(&zdst)dH?_b literal 0 HcmV?d00001 diff --git a/tests/test-abidiff-exit.cc b/tests/test-abidiff-exit.cc index 5afc15bc..e7bf8362 100644 --- a/tests/test-abidiff-exit.cc +++ b/tests/test-abidiff-exit.cc @@ -203,6 +203,15 @@ InOutSpec in_out_specs[] = "data/test-abidiff-exit/test-decl-struct-report.txt", "output/test-abidiff-exit/test-decl-struct-report.txt" }, + { + "data/test-abidiff-exit/test-decl-enum-v0.o", + "data/test-abidiff-exit/test-decl-enum-v1.o", + "", + "--harmless", + abigail::tools_utils::ABIDIFF_ABI_CHANGE, + "data/test-abidiff-exit/test-decl-enum-report.txt", + "output/test-abidiff-exit/test-decl-enum-report.txt" + }, {0, 0, 0 ,0, abigail::tools_utils::ABIDIFF_OK, 0, 0} };