[RFC,1/2] Support declaration-only enums.
Commit Message
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(-)
Comments
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
@@ -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 *);
@@ -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
@@ -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&);
@@ -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;
@@ -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)
@@ -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
@@ -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))
{
@@ -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);
@@ -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.
///