* [RFC PATCH 0/2] Incomplete enum support
@ 2020-04-23 13:03 Giuliano Procida
2020-04-23 13:03 ` [RFC PATCH 1/2] Support declaration-only enums Giuliano Procida
2020-04-23 13:03 ` [RFC PATCH 2/2] Add tests for " Giuliano Procida
0 siblings, 2 replies; 8+ messages in thread
From: Giuliano Procida @ 2020-04-23 13:03 UTC (permalink / raw)
To: libabigail; +Cc: dodji, kernel-team, gprocida, maennich
Incomplete enums are heavily used in the Linux kernel source as part
of interface specifications.
This code appears to work, but it is lightly tested and unreviewed.
It also duplicates a lot of code that is used for incomplete class and
union types. There may be a significant opportunity for refactoring
and shared code and data.
Giuliano Procida (2):
Support declaration-only enums.
Add tests for declaration-only enums.
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 ++++++++++++-
tests/data/Makefile.am | 5 +
.../test-decl-enum-report.txt | 17 +
.../test-abidiff-exit/test-decl-enum-v0.c | 5 +
.../test-abidiff-exit/test-decl-enum-v0.o | Bin 0 -> 3048 bytes
.../test-abidiff-exit/test-decl-enum-v1.c | 5 +
.../test-abidiff-exit/test-decl-enum-v1.o | Bin 0 -> 3048 bytes
tests/test-abidiff-exit.cc | 9 +
16 files changed, 754 insertions(+), 26 deletions(-)
create mode 100644 tests/data/test-abidiff-exit/test-decl-enum-report.txt
create mode 100644 tests/data/test-abidiff-exit/test-decl-enum-v0.c
create mode 100644 tests/data/test-abidiff-exit/test-decl-enum-v0.o
create mode 100644 tests/data/test-abidiff-exit/test-decl-enum-v1.c
create mode 100644 tests/data/test-abidiff-exit/test-decl-enum-v1.o
--
2.26.1.301.g55bc3eb7cb9-goog
^ permalink raw reply [flat|nested] 8+ messages in thread
* [RFC PATCH 1/2] Support declaration-only enums.
2020-04-23 13:03 [RFC PATCH 0/2] Incomplete enum support Giuliano Procida
@ 2020-04-23 13:03 ` Giuliano Procida
2020-05-12 14:02 ` Matthias Maennich
2020-05-22 10:03 ` Dodji Seketeli
2020-04-23 13:03 ` [RFC PATCH 2/2] Add tests for " Giuliano Procida
1 sibling, 2 replies; 8+ messages in thread
From: Giuliano Procida @ 2020-04-23 13:03 UTC (permalink / raw)
To: libabigail; +Cc: dodji, kernel-team, gprocida, maennich
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);
}
/// Getter of a bitmap made of the set of change categories that are
@@ -3113,7 +3114,7 @@ operator<<(ostream& o, diff_category c)
emitted_a_category |= true;
}
- if (c & FN_RETURN_TYPE_CV_CHANGE_CATEGORY)
+ if (c & FN_RETURN_TYPE_CV_CHANGE_CATEGORY)
{
if (emitted_a_category)
o << "|";
@@ -3121,7 +3122,7 @@ operator<<(ostream& o, diff_category c)
emitted_a_category |= true;
}
- if (c & VAR_TYPE_CV_CHANGE_CATEGORY)
+ if (c & VAR_TYPE_CV_CHANGE_CATEGORY)
{
if (emitted_a_category)
o << "|";
@@ -3129,7 +3130,7 @@ operator<<(ostream& o, diff_category c)
emitted_a_category |= true;
}
- if (c & VOID_PTR_TO_PTR_CHANGE_CATEGORY)
+ if (c & VOID_PTR_TO_PTR_CHANGE_CATEGORY)
{
if (emitted_a_category)
o << "|";
@@ -3137,7 +3138,7 @@ operator<<(ostream& o, diff_category c)
emitted_a_category |= true;
}
- if (c & BENIGN_INFINITE_ARRAY_CHANGE_CATEGORY)
+ if (c & BENIGN_INFINITE_ARRAY_CHANGE_CATEGORY)
{
if (emitted_a_category)
o << "|";
@@ -3145,6 +3146,14 @@ operator<<(ostream& o, diff_category c)
emitted_a_category |= true;
}
+ if (c & ENUM_DECL_ONLY_DEF_CHANGE_CATEGORY)
+ {
+ if (emitted_a_category)
+ o << "|";
+ o << "ENUM_DECL_ONLY_DEF_CHANGE_CATEGORY";
+ emitted_a_category |= true;
+ }
+
return o;
}
@@ -10707,6 +10716,8 @@ struct leaf_diff_node_marker_visitor : public diff_node_visitor
&& !is_anonymous_class_or_union_diff(d)
// Don't show decl-only-ness changes of classes either.
&& !filtering::has_class_decl_only_def_change(d)
+ // Same for enums.
+ && !filtering::has_enum_decl_only_def_change(d)
// Sometime, we can encounter artifacts of bogus DWARF that
// yield a diff node for a decl-only class (and empty class
// with the is_declaration flag set) that carries a non-zero
diff --git a/src/abg-default-reporter.cc b/src/abg-default-reporter.cc
index 04e2bb76..19dec4cb 100644
--- a/src/abg-default-reporter.cc
+++ b/src/abg-default-reporter.cc
@@ -94,13 +94,33 @@ default_reporter::report(const enum_diff& d, ostream& out,
d.second_subject(),
"enum type");
- string name = d.first_enum()->get_pretty_representation();
-
enum_type_decl_sptr first = d.first_enum(), second = d.second_enum();
- report_name_size_and_alignment_changes(first, second, d.context(),
+ const diff_context_sptr& ctxt = d.context();
+
+ string name = first->get_pretty_representation();
+
+ // Report enum decl-only <-> definition changes.
+ if (ctxt->get_allowed_category() & ENUM_DECL_ONLY_DEF_CHANGE_CATEGORY)
+ if (filtering::has_enum_decl_only_def_change(first, second))
+ {
+ string was =
+ first->get_is_declaration_only()
+ ? " was a declaration-only enum type"
+ : " was a defined enum type";
+
+ string is_now =
+ second->get_is_declaration_only()
+ ? " and is now a declaration-only enum type"
+ : " and is now a defined enum type";
+
+ out << indent << "enum type " << name << was << is_now << "\n";
+ return;
+ }
+
+ report_name_size_and_alignment_changes(first, second, ctxt,
out, indent);
- maybe_report_diff_for_member(first, second, d.context(), out, indent);
+ maybe_report_diff_for_member(first, second, ctxt, out, indent);
//underlying type
d.underlying_type_diff()->report(out, indent);
@@ -165,12 +185,12 @@ default_reporter::report(const enum_diff& d, ostream& out,
<< "' from value '"
<< i->first.get_value() << "' to '"
<< i->second.get_value() << "'";
- report_loc_info(second, *d.context(), out);
+ report_loc_info(second, *ctxt, out);
out << "\n";
}
}
- if (d.context()->show_leaf_changes_only())
+ if (ctxt->show_leaf_changes_only())
maybe_report_interfaces_impacted_by_diff(&d, out, indent);
d.reported_once(true);
@@ -844,7 +864,7 @@ default_reporter::report(const class_or_union_diff& d,
const diff_context_sptr& ctxt = d.context();
- // Report class decl-only -> definition change.
+ // Report class decl-only <-> definition change.
if (ctxt->get_allowed_category() & CLASS_DECL_ONLY_DEF_CHANGE_CATEGORY)
if (filtering::has_class_decl_only_def_change(first, second))
{
diff --git a/src/abg-dwarf-reader.cc b/src/abg-dwarf-reader.cc
index 850281ad..5748460e 100644
--- a/src/abg-dwarf-reader.cc
+++ b/src/abg-dwarf-reader.cc
@@ -139,6 +139,11 @@ typedef unordered_map<Dwarf_Off, class_decl_sptr> die_class_map_type;
/// corresponding class_or_union_sptr.
typedef unordered_map<Dwarf_Off, class_or_union_sptr> die_class_or_union_map_type;
+/// Convenience typedef for a map which key is the offset of a dwarf
+/// die, (given by dwarf_dieoffset()) and which value is the
+/// corresponding enum_type_decl_sptr.
+typedef unordered_map<Dwarf_Off, enum_type_decl_sptr> die_enum_map_type;
+
/// Convenience typedef for a map which key the offset of a dwarf die
/// and which value is the corresponding function_decl.
typedef unordered_map<Dwarf_Off, function_decl_sptr> die_function_decl_map_type;
@@ -198,6 +203,10 @@ typedef unordered_map<Dwarf_Off, Dwarf_Off> offset_offset_map_type;
/// value is a vector of smart pointer to a class.
typedef unordered_map<string, classes_type> string_classes_map;
+/// Convenience typedef for a map which key is a string and which
+/// value is a vector of smart pointer to a enum.
+typedef unordered_map<string, enums_type> string_enums_map;
+
/// The abstraction of the place where a partial unit has been
/// imported. This is what the DW_TAG_imported_unit DIE expresses.
///
@@ -2273,6 +2282,9 @@ public:
die_class_or_union_map_type die_wip_classes_map_;
die_class_or_union_map_type alternate_die_wip_classes_map_;
die_class_or_union_map_type type_unit_die_wip_classes_map_;
+ die_enum_map_type die_wip_enums_map_;
+ die_enum_map_type alternate_die_wip_enums_map_;
+ die_enum_map_type type_unit_die_wip_enums_map_;
die_function_type_map_type die_wip_function_types_map_;
die_function_type_map_type alternate_die_wip_function_types_map_;
die_function_type_map_type type_unit_die_wip_function_types_map_;
@@ -2282,6 +2294,7 @@ public:
vector<Dwarf_Off> type_unit_types_to_canonicalize_;
vector<type_base_sptr> extra_types_to_canonicalize_;
string_classes_map decl_only_classes_map_;
+ string_enums_map decl_only_enums_map_;
die_tu_map_type die_tu_map_;
corpus_group_sptr cur_corpus_group_;
corpus_sptr cur_corpus_;
@@ -4340,6 +4353,44 @@ public:
return die_wip_classes_map_;
}
+ /// Getter of a map that associates a die that represents a
+ /// enum with the declaration of the enum, while the enum
+ /// is being constructed.
+ ///
+ /// @param source where the DIE is from.
+ ///
+ /// @return the map that associates a DIE to the enum that is being
+ /// built.
+ const die_enum_map_type&
+ die_wip_enums_map(die_source source) const
+ {return const_cast<read_context*>(this)->die_wip_enums_map(source);}
+
+ /// Getter of a map that associates a die that represents a
+ /// enum with the declaration of the enum, while the enum
+ /// is being constructed.
+ ///
+ /// @param source where the DIE comes from.
+ ///
+ /// @return the map that associates a DIE to the enum that is being
+ /// built.
+ die_enum_map_type&
+ die_wip_enums_map(die_source source)
+ {
+ switch (source)
+ {
+ case PRIMARY_DEBUG_INFO_DIE_SOURCE:
+ break;
+ case ALT_DEBUG_INFO_DIE_SOURCE:
+ return alternate_die_wip_enums_map_;
+ case TYPE_UNIT_DIE_SOURCE:
+ return type_unit_die_wip_enums_map_;
+ case NO_DEBUG_INFO_DIE_SOURCE:
+ case NUMBER_OF_DIE_SOURCES:
+ ABG_ASSERT_NOT_REACHED;
+ }
+ return die_wip_enums_map_;
+ }
+
/// Getter for a map that associates a die (that represents a
/// function type) whith a function type, while the function type is
/// being constructed (WIP == work in progress).
@@ -4620,6 +4671,203 @@ public:
}
}
+ /// Getter for the map of declaration-only enums that are to be
+ /// resolved to their definition enums by the end of the corpus
+ /// loading.
+ ///
+ /// @return a map of string -> vector of enums where the key is
+ /// the fully qualified name of the enum and the value is the
+ /// vector of declaration-only enum.
+ const string_enums_map&
+ declaration_only_enums() const
+ {return decl_only_enums_map_;}
+
+ /// Getter for the map of declaration-only enums that are to be
+ /// resolved to their definition enums by the end of the corpus
+ /// loading.
+ ///
+ /// @return a map of string -> vector of enums where the key is
+ /// the fully qualified name of the enum and the value is the
+ /// vector of declaration-only enum.
+ string_enums_map&
+ declaration_only_enums()
+ {return decl_only_enums_map_;}
+
+ /// If a given enum is a declaration-only enum then stash it on
+ /// the side so that at the end of the corpus reading we can resolve
+ /// it to its definition.
+ ///
+ /// @param enom the enum to consider.
+ void
+ maybe_schedule_declaration_only_enum_for_resolution(enum_type_decl_sptr& enom)
+ {
+ if (enom->get_is_declaration_only()
+ && enom->get_definition_of_declaration() == 0)
+ {
+ string qn = enom->get_qualified_name();
+ string_enums_map::iterator record =
+ declaration_only_enums().find(qn);
+ if (record == declaration_only_enums().end())
+ declaration_only_enums()[qn].push_back(enom);
+ else
+ record->second.push_back(enom);
+ }
+ }
+
+ /// Test if a given declaration-only enum has been scheduled for
+ /// resolution to a defined enum.
+ ///
+ /// @param enom the enum to consider for the test.
+ ///
+ /// @return true iff @p enom is a declaration-only enum and if
+ /// it's been scheduled for resolution to a defined enum.
+ bool
+ is_decl_only_enum_scheduled_for_resolution(enum_type_decl_sptr& enom)
+ {
+ if (enom->get_is_declaration_only())
+ return (declaration_only_enums().find(enom->get_qualified_name())
+ != declaration_only_enums().end());
+
+ return false;
+ }
+
+ /// Walk the declaration-only enums that have been found during
+ /// the building of the corpus and resolve them to their definitions.
+ void
+ resolve_declaration_only_enums()
+ {
+ vector<string> resolved_enums;
+
+ for (string_enums_map::iterator i =
+ declaration_only_enums().begin();
+ i != declaration_only_enums().end();
+ ++i)
+ {
+ bool to_resolve = false;
+ for (enums_type::iterator j = i->second.begin();
+ j != i->second.end();
+ ++j)
+ if ((*j)->get_is_declaration_only()
+ && ((*j)->get_definition_of_declaration() == 0))
+ to_resolve = true;
+
+ if (!to_resolve)
+ {
+ resolved_enums.push_back(i->first);
+ continue;
+ }
+
+ // Now, for each decl-only enum that have the current name
+ // 'i->first', let's try to poke at the fully defined enum
+ // that is defined in the same translation unit as the
+ // declaration.
+ //
+ // If we find one enum (defined in the TU of the declaration)
+ // that defines the declaration, then the declaration can be
+ // resolved to that enum.
+ //
+ // If no defining enum is found in the TU of the declaration,
+ // then there are possibly three cases to consider:
+ //
+ // 1/ There is exactly one enum that defines the
+ // declaration and that enum is defined in another TU. In
+ // this case, the declaration is resolved to that
+ // definition.
+ //
+ // 2/ There are more than one enum that define that
+ // declaration and none of them is defined in the TU of the
+ // declaration. In this case, the declaration is left
+ // unresolved.
+ //
+ // 3/ No enum defines the declaration. In this case, the
+ // declaration is left unresoved.
+
+ // So get the enums that might define the current
+ // declarations which name is i->first.
+ const type_base_wptrs_type *enums =
+ lookup_enum_types(i->first, *current_corpus());
+ if (!enums)
+ continue;
+
+ unordered_map<string, enum_type_decl_sptr> per_tu_enum_map;
+ for (type_base_wptrs_type::const_iterator c = enums->begin();
+ c != enums->end();
+ ++c)
+ {
+ enum_type_decl_sptr enom = is_enum_type(type_base_sptr(*c));
+ ABG_ASSERT(enom);
+
+ enom = is_enum_type(look_through_decl_only_enum(enom));
+ if (enom->get_is_declaration_only())
+ continue;
+
+ string tu_path = enom->get_translation_unit()->get_absolute_path();
+ if (tu_path.empty())
+ continue;
+
+ // Build a map that associates the translation unit path
+ // to the enum (that potentially defines the declarations
+ // that we consider) that are defined in that translation unit.
+ per_tu_enum_map[tu_path] = enom;
+ }
+
+ if (!per_tu_enum_map.empty())
+ {
+ // Walk the declarations to resolve and resolve them
+ // either to the definitions that are in the same TU as
+ // the declaration, or to the definition found elsewhere,
+ // if there is only one such definition.
+ for (enums_type::iterator j = i->second.begin();
+ j != i->second.end();
+ ++j)
+ {
+ if ((*j)->get_is_declaration_only()
+ && ((*j)->get_definition_of_declaration() == 0))
+ {
+ string tu_path =
+ (*j)->get_translation_unit()->get_absolute_path();
+ unordered_map<string, enum_type_decl_sptr>::const_iterator e =
+ per_tu_enum_map.find(tu_path);
+ if (e != per_tu_enum_map.end())
+ (*j)->set_definition_of_declaration(e->second);
+ else if (per_tu_enum_map.size() == 1)
+ (*j)->set_definition_of_declaration
+ (per_tu_enum_map.begin()->second);
+ }
+ }
+ resolved_enums.push_back(i->first);
+ }
+ }
+
+ size_t num_decl_only_enums = declaration_only_enums().size(),
+ num_resolved = resolved_enums.size();
+ if (show_stats())
+ cerr << "resolved " << num_resolved
+ << " enum declarations out of "
+ << num_decl_only_enums
+ << "\n";
+
+ for (vector<string>::const_iterator i = resolved_enums.begin();
+ i != resolved_enums.end();
+ ++i)
+ declaration_only_enums().erase(*i);
+
+ for (string_enums_map::iterator i = declaration_only_enums().begin();
+ i != declaration_only_enums().end();
+ ++i)
+ {
+ if (show_stats())
+ {
+ if (i == declaration_only_enums().begin())
+ cerr << "Here are the "
+ << num_decl_only_enums - num_resolved
+ << " unresolved enum declarations:\n";
+ else
+ cerr << " " << i->first << "\n";
+ }
+ }
+ }
+
/// Some functions described by DWARF may have their linkage name
/// set, but no link to their actual underlying elf symbol. When
/// these are virtual member functions, comparing the enclosing type
@@ -13386,18 +13634,32 @@ build_enum_type(read_context& ctxt,
if (tag != DW_TAG_enumeration_type)
return result;
+ die_source source;
+ ABG_ASSERT(ctxt.get_die_source(die, source));
+ {
+ die_enum_map_type::const_iterator i =
+ ctxt.die_wip_enums_map(source).find(dwarf_dieoffset(die));
+ if (i != ctxt.die_wip_enums_map(source).end())
+ {
+ enum_type_decl_sptr u = is_enum_type(i->second);
+ ABG_ASSERT(u);
+ return u;
+ }
+ }
+
string name, linkage_name;
location loc;
die_loc_and_name(ctxt, die, loc, name, linkage_name);
+ bool is_declaration_only = die_is_declaration_only(die);
- bool enum_is_anonymous = false;
+ bool is_anonymous = false;
// If the enum is anonymous, let's give it a name.
if (name.empty())
{
name = get_internal_anonymous_die_prefix_name(die);
ABG_ASSERT(!name.empty());
// But we remember that the type is anonymous.
- enum_is_anonymous = true;
+ is_anonymous = true;
if (size_t s = scope->get_num_anonymous_member_enums())
name = build_internal_anonymous_die_name(name, s);
@@ -13409,7 +13671,7 @@ build_enum_type(read_context& ctxt,
// representation (name) and location can be later detected as being
// for the same type.
- if (!enum_is_anonymous)
+ if (!is_anonymous)
{
if (use_odr)
{
@@ -13442,6 +13704,7 @@ build_enum_type(read_context& ctxt,
uint64_t size = 0;
if (die_unsigned_constant_attribute(die, DW_AT_byte_size, size))
size *= 8;
+ bool is_artificial = die_is_artificial(die);
// for now we consider that underlying types of enums are all anonymous
bool enum_underlying_type_is_anonymous= true;
@@ -13474,8 +13737,6 @@ build_enum_type(read_context& ctxt,
while (dwarf_siblingof(&child, &child) == 0);
}
- bool is_artificial = die_is_artificial(die);
-
// DWARF up to version 4 (at least) doesn't seem to carry the
// underlying type, so let's create an artificial one here, which
// sole purpose is to be passed to the constructor of the
@@ -13491,9 +13752,13 @@ build_enum_type(read_context& ctxt,
t = dynamic_pointer_cast<type_decl>(d);
ABG_ASSERT(t);
result.reset(new enum_type_decl(name, loc, t, enms, linkage_name));
- result->set_is_anonymous(enum_is_anonymous);
+ result->set_is_anonymous(is_anonymous);
+ result->set_is_declaration_only(is_declaration_only);
result->set_is_artificial(is_artificial);
ctxt.associate_die_to_type(die, result, where_offset);
+
+ ctxt.maybe_schedule_declaration_only_enum_for_resolution(result);
+
return result;
}
@@ -16226,6 +16491,24 @@ read_debug_info_into_corpus(read_context& ctxt)
}
}
+ {
+ tools_utils::timer t;
+ if (ctxt.do_log())
+ {
+ cerr << "resolving declaration only enums ...";
+ t.start();
+ }
+ ctxt.resolve_declaration_only_enums();
+ if (ctxt.do_log())
+ {
+ t.stop();
+ cerr << " DONE@" << ctxt.current_corpus()->get_path()
+ << ":"
+ << t
+ <<"\n";
+ }
+ }
+
{
tools_utils::timer t;
if (ctxt.do_log())
@@ -16660,7 +16943,18 @@ build_ir_node_from_die(read_context& ctxt,
case DW_TAG_enumeration_type:
{
- if (!type_is_suppressed(ctxt, scope, die))
+ bool type_is_private = false;
+ bool type_suppressed =
+ type_is_suppressed(ctxt, scope, die, type_is_private);
+ if (type_suppressed && type_is_private)
+ // The type is suppressed because it's private. If other
+ // non-suppressed and declaration-only instances of this
+ // type exist in the current corpus, then it means those
+ // non-suppressed instances are opaque versions of the
+ // suppressed private type. Lets return one of these opaque
+ // types then.
+ result = get_opaque_version_of_type(ctxt, scope, die, where_offset);
+ else if (!type_suppressed)
{
enum_type_decl_sptr e = build_enum_type(ctxt, die, scope,
where_offset);
diff --git a/src/abg-ir.cc b/src/abg-ir.cc
index 27831352..bfdc5679 100644
--- a/src/abg-ir.cc
+++ b/src/abg-ir.cc
@@ -7674,6 +7674,38 @@ typedef_decl*
is_typedef(type_base* t)
{return dynamic_cast<typedef_decl*>(t);}
+/// Test if a type is a enum. This function looks through typedefs.
+///
+/// @parm t the type to consider.
+///
+/// @return the enum_decl if @p t is a enum_decl or null otherwise.
+enum_type_decl_sptr
+is_compatible_with_enum_type(const type_base_sptr& t)
+{
+ if (!t)
+ return enum_type_decl_sptr();
+
+ // Normally we should strip typedefs entirely, but this is
+ // potentially costly, especially on binaries with huge changesets
+ // like the Linux Kernel. So we just get the leaf types for now.
+ //
+ // Maybe there should be an option by which users accepts to pay the
+ // CPU usage toll in exchange for finer filtering?
+
+ // type_base_sptr ty = strip_typedef(t);
+ type_base_sptr ty = peel_typedef_type(t);;
+ return is_enum_type(ty);
+}
+
+/// Test if a type is a enum. This function looks through typedefs.
+///
+/// @parm t the type to consider.
+///
+/// @return the enum_decl if @p t is a enum_decl or null otherwise.
+enum_type_decl_sptr
+is_compatible_with_enum_type(const decl_base_sptr& t)
+{return is_compatible_with_enum_type(is_type(t));}
+
/// Test if a decl is an enum_type_decl
///
/// @param d the decl to test for.
@@ -8057,6 +8089,50 @@ look_through_decl_only_class(class_or_union_sptr klass)
return result;
}
+/// If a enum is a decl-only enum, get its definition.
+/// Otherwise, just return the initial enum.
+///
+/// @param the_enum the enum to consider.
+///
+/// @return either the definition of the enum, or the enum itself.
+enum_type_decl_sptr
+look_through_decl_only_enum(const enum_type_decl& the_enum)
+{
+ enum_type_decl_sptr enom;
+ if (the_enum.get_is_declaration_only())
+ enom = the_enum.get_definition_of_declaration();
+
+ if (!enom)
+ return enom;
+
+ while (enom
+ && enom->get_is_declaration_only()
+ && enom->get_definition_of_declaration())
+ enom = enom->get_definition_of_declaration();
+
+ ABG_ASSERT(enom);
+ return enom;
+}
+
+/// If a enum is a decl-only enum, get its definition.
+/// Otherwise, just return the initial enum.
+///
+/// @param enom the enum to consider.
+///
+/// @return either the definition of the enum, or the enum itself.
+enum_type_decl_sptr
+look_through_decl_only_enum(enum_type_decl_sptr enom)
+{
+ if (!enom)
+ return enom;
+
+ enum_type_decl_sptr result = look_through_decl_only_enum(*enom);
+ if (!result)
+ result = enom;
+
+ return result;
+}
+
/// Tests if a declaration is a variable declaration.
///
/// @param decl the decl to test.
@@ -9915,6 +9991,37 @@ lookup_enum_type(const interned_string& qualified_name, const corpus& corp)
return result;
}
+/// Look into a given corpus to find the enum type*s* that have a
+/// given qualified name.
+///
+/// @param qualified_name the qualified name of the type to look for.
+///
+/// @param corp the corpus to look into.
+///
+/// @return the vector of enum types that which name is @p qualified_name.
+const type_base_wptrs_type *
+lookup_enum_types(const interned_string& qualified_name, const corpus& corp)
+{
+ const istring_type_base_wptrs_map_type& m = corp.get_types().enum_types();
+
+ return lookup_types_in_map(qualified_name, m);
+}
+
+/// Look into a given corpus to find the enum type*s* that have a
+/// given qualified name.
+///
+/// @param qualified_name the qualified name of the type to look for.
+///
+/// @param corp the corpus to look into.
+///
+/// @return the vector of enum types that which name is @p qualified_name.
+const type_base_wptrs_type*
+lookup_enum_types(const string& qualified_name, const corpus& corp)
+{
+ interned_string s = corp.get_environment()->intern(qualified_name);
+ return lookup_enum_types(s, corp);
+}
+
/// Look up an @ref enum_type_decl from a given corpus, by its location.
///
/// @param loc the location to consider.
@@ -14724,17 +14831,26 @@ class enum_type_decl::priv
{
type_base_sptr underlying_type_;
enumerators enumerators_;
+ decl_base_sptr declaration_;
+ enum_type_decl_wptr definition_of_declaration_;
+ enum_type_decl* naked_definition_of_declaration_;
+ bool is_declaration_only_;
friend class enum_type_decl;
- priv();
-
public:
+ priv()
+ : naked_definition_of_declaration_(),
+ is_declaration_only_(true)
+ {}
+
priv(type_base_sptr underlying_type,
enumerators& enumerators)
: underlying_type_(underlying_type),
- enumerators_(enumerators)
+ enumerators_(enumerators),
+ naked_definition_of_declaration_(),
+ is_declaration_only_(false)
{}
}; // end class enum_type_decl::priv
@@ -14775,6 +14891,25 @@ enum_type_decl::enum_type_decl(const string& name,
e->set_enum_type(this);
}
+/// Constructor for instances of enum_type_decl that represent a
+/// declaration without definition.
+///
+/// @param env the environment we are operating from.
+///
+/// @param name the name of the enum.
+enum_type_decl::enum_type_decl(const environment* env,
+ const string& name)
+ : type_or_decl_base(env,
+ ENUM_TYPE
+ | ABSTRACT_TYPE_BASE
+ | ABSTRACT_DECL_BASE),
+ type_base(env, 0, 0),
+ decl_base(env, name, location(), name),
+ priv_(new priv)
+{
+ runtime_type_instance(this);
+}
+
/// Return the underlying type of the enum.
type_base_sptr
enum_type_decl::get_underlying_type() const
@@ -14790,6 +14925,100 @@ enum_type_decl::enumerators&
enum_type_decl::get_enumerators()
{return priv_->enumerators_;}
+/// Set the definition of this declaration-only @ref enum_type_decl.
+///
+/// @param d the new definition to set.
+void
+enum_type_decl::set_definition_of_declaration(enum_type_decl_sptr d)
+{
+ ABG_ASSERT(get_is_declaration_only());
+ priv_->definition_of_declaration_ = d;
+ if (d->get_canonical_type())
+ type_base::priv_->canonical_type = d->get_canonical_type();
+
+ priv_->naked_definition_of_declaration_ = d.get();
+}
+
+/// If this @ref enum_type_decl_sptr is declaration-only, get its
+/// definition, if any.
+///
+/// @return the definition of this decl-only enum.
+const enum_type_decl_sptr
+enum_type_decl::get_definition_of_declaration() const
+{
+ if (priv_->definition_of_declaration_.expired())
+ return enum_type_decl_sptr();
+ return enum_type_decl_sptr(priv_->definition_of_declaration_);
+}
+
+/// If this @ref enum_type_decl is declaration-only, get its
+/// definition, if any.
+///
+/// Note that this function doesn't return a smart pointer, but rather
+/// the underlying pointer managed by the smart pointer. So it's as
+/// fast as possible. This getter is to be used in code paths that are
+/// proven to be performance hot spots; especially, when comparing
+/// sensitive types like enums. Those are compared extremely frequently
+/// and thus, their access to the definition of declaration must be
+/// fast.
+///
+/// @return the definition of the enum.
+const enum_type_decl*
+enum_type_decl::get_naked_definition_of_declaration() const
+{return priv_->naked_definition_of_declaration_;}
+
+/// If this @ref enum_type_decl_sptr is a definition, get its earlier
+/// declaration.
+///
+/// @return the earlier declaration of the enum, if any.
+decl_base_sptr
+enum_type_decl::get_earlier_declaration() const
+{return priv_->declaration_;}
+
+/// set the earlier declaration of this @ref enum_type_decl definition.
+///
+/// @param declaration the earlier declaration to set. Note that it's
+/// set only if it's a pure declaration.
+void
+enum_type_decl::set_earlier_declaration(decl_base_sptr declaration)
+{
+ enum_type_decl_sptr cl = dynamic_pointer_cast<enum_type_decl>(declaration);
+ if (cl && cl->get_is_declaration_only())
+ priv_->declaration_ = declaration;
+}
+
+/// Test if a @ref enum_type_decl is a declaration-only @ref
+/// enum_type_decl.
+///
+/// @return true iff the current @ref enum_type_decl is a
+/// declaration-only @ref enum_type_decl.
+bool
+enum_type_decl::get_is_declaration_only() const
+{return priv_->is_declaration_only_;}
+
+/// Set a flag saying if the @ref enum_type_decl is a declaration-only
+/// @ref enum_type_decl.
+///
+/// @param f true if the @ref enum_type_decl is a decalaration-only
+/// @ref enum_type_decl.
+void
+enum_type_decl::set_is_declaration_only(bool f)
+{
+ bool update_types_lookup_map = !f && priv_->is_declaration_only_;
+
+ priv_->is_declaration_only_ = f;
+
+ if (update_types_lookup_map)
+ if (scope_decl* s = get_scope())
+ {
+ scope_decl::declarations::iterator i;
+ if (s->find_iterator_for_member(this, i))
+ maybe_update_types_lookup_map(*i);
+ else
+ ABG_ASSERT_NOT_REACHED;
+ }
+}
+
/// Get the pretty representation of the current instance of @ref
/// enum_type_decl.
///
--
2.26.1.301.g55bc3eb7cb9-goog
^ permalink raw reply [flat|nested] 8+ messages in thread
* [RFC PATCH 2/2] Add tests for declaration-only enums.
2020-04-23 13:03 [RFC PATCH 0/2] Incomplete enum support Giuliano Procida
2020-04-23 13:03 ` [RFC PATCH 1/2] Support declaration-only enums Giuliano Procida
@ 2020-04-23 13:03 ` Giuliano Procida
2020-05-12 14:05 ` Matthias Maennich
1 sibling, 1 reply; 8+ messages in thread
From: Giuliano Procida @ 2020-04-23 13:03 UTC (permalink / raw)
To: libabigail; +Cc: dodji, kernel-team, gprocida, maennich
---
tests/data/Makefile.am | 5 +++++
.../test-decl-enum-report.txt | 17 +++++++++++++++++
.../data/test-abidiff-exit/test-decl-enum-v0.c | 5 +++++
.../data/test-abidiff-exit/test-decl-enum-v0.o | Bin 0 -> 3048 bytes
.../data/test-abidiff-exit/test-decl-enum-v1.c | 5 +++++
.../data/test-abidiff-exit/test-decl-enum-v1.o | Bin 0 -> 3048 bytes
tests/test-abidiff-exit.cc | 9 +++++++++
7 files changed, 41 insertions(+)
create mode 100644 tests/data/test-abidiff-exit/test-decl-enum-report.txt
create mode 100644 tests/data/test-abidiff-exit/test-decl-enum-v0.c
create mode 100644 tests/data/test-abidiff-exit/test-decl-enum-v0.o
create mode 100644 tests/data/test-abidiff-exit/test-decl-enum-v1.c
create mode 100644 tests/data/test-abidiff-exit/test-decl-enum-v1.o
diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am
index a1b9bf64..c036f349 100644
--- a/tests/data/Makefile.am
+++ b/tests/data/Makefile.am
@@ -151,6 +151,11 @@ test-abidiff-exit/test-decl-struct-v0.o \
test-abidiff-exit/test-decl-struct-v1.c \
test-abidiff-exit/test-decl-struct-v1.o \
test-abidiff-exit/test-decl-struct-report.txt \
+test-abidiff-exit/test-decl-enum-v0.c \
+test-abidiff-exit/test-decl-enum-v0.o \
+test-abidiff-exit/test-decl-enum-v1.c \
+test-abidiff-exit/test-decl-enum-v1.o \
+test-abidiff-exit/test-decl-enum-report.txt \
\
test-diff-dwarf/test0-v0.cc \
test-diff-dwarf/test0-v0.o \
diff --git a/tests/data/test-abidiff-exit/test-decl-enum-report.txt b/tests/data/test-abidiff-exit/test-decl-enum-report.txt
new file mode 100644
index 00000000..e46ebfa6
--- /dev/null
+++ b/tests/data/test-abidiff-exit/test-decl-enum-report.txt
@@ -0,0 +1,17 @@
+Functions changes summary: 0 Removed, 2 Changed, 0 Added functions
+Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
+
+2 functions with some indirect sub-type change:
+
+ [C] 'function void reg1(const embodied_enum*)' at test-decl-enum-v1.c:4:1 has some indirect sub-type changes:
+ parameter 1 of type 'const embodied_enum*' has sub-type changes:
+ in pointed to type 'const embodied_enum':
+ in unqualified underlying type 'enum embodied_enum' at test-decl-enum-v1.c:1:1:
+ enum type enum embodied_enum was a declaration-only enum type and is now a defined enum type
+
+ [C] 'function void reg2(const disembodied_enum*)' at test-decl-enum-v1.c:5:1 has some indirect sub-type changes:
+ parameter 1 of type 'const disembodied_enum*' has sub-type changes:
+ in pointed to type 'const disembodied_enum':
+ in unqualified underlying type 'enum disembodied_enum':
+ enum type enum disembodied_enum was a defined enum type and is now a declaration-only enum type
+
diff --git a/tests/data/test-abidiff-exit/test-decl-enum-v0.c b/tests/data/test-abidiff-exit/test-decl-enum-v0.c
new file mode 100644
index 00000000..d5672618
--- /dev/null
+++ b/tests/data/test-abidiff-exit/test-decl-enum-v0.c
@@ -0,0 +1,5 @@
+enum embodied_enum;
+enum disembodied_enum { X };
+
+void reg1(const enum embodied_enum * foo) { (void)foo; }
+void reg2(const enum disembodied_enum * foo) { (void)foo; }
diff --git a/tests/data/test-abidiff-exit/test-decl-enum-v0.o b/tests/data/test-abidiff-exit/test-decl-enum-v0.o
new file mode 100644
index 0000000000000000000000000000000000000000..b4c0b06a089ec6adbfa81b1a619ae30087e54f9d
GIT binary patch
literal 3048
zcmbtWUuzsy6hE^wo1Hk#Hp$xBG=+{(+eDplH%QIaCa$Ko2^OhB#TN;)J2$(7+1WBP
zTT?|XqC&xk3W5lt4}JmB7hejZzU!Cp!8d&qe9&`d?%CYjOcC_J+<VUN{GEH}oZWYy
zzk0c17{Fw}I_yh~0xZ?``A*JupaJu6ee=$*n|I#+^X_XuD(v@YVdYkK9_&cYE6Z_(
zuMl6ews18nQ<rcvU&Y1paH(2(-YI0ksOB574lPK==kMTJ+Zl~eyG(rL;{n(<1u1;2
z5}$7-KGxZL(q|fXEP_<F{T&I$obgD#;h6Qs`kXOen|%m_TZUO*cN}M{alv`oX*)&(
zi(LZKK7|!oXJHHF>KSmHb@am$eB+q~vtHPqTc9wV6Q4e}KyI}vx)qwOM&$&iJ&Jx5
zVA=x8a&ZBRcpRVne88rfCZwL<1VrX4J|;Yht6}_ApTRZ1Tf77VGLdFpD0;h|h{r?k
zX4CKGuGPn2y2ydzvFh>ns{R_T6%)P3aT@hw5xP;Fff#m2VI;zB@`o@=4+2+SyzXwa
zo^sp%ir;cqnyq%L)oi=oFdN5WtuJDcL_HLPq&HaGTRrccf712(uD27Ux8mL)8O5V<
z>W$+!qB!)jpt~#5(wY;|ZvieV@I*Ge0dQqw<AS?%NpzziE|Pn#_Oez+{-(G*C>i6;
zm%-SZH5QN5?7Mh8rx2%yDF3>it<Qc~*_fHSj0T92?jk9X%JhZi@JgS*1%Lc~SVI?;
zH^(8BDX&~tnCMYPrtCFcXBIu6!x%+rW<_g4-USL$u;!%S4ysI;IS5F}1pM|($b>_4
z0FI&EymAiQ*(W8=GVlNLMNHoK0ra1=KJuKIaC%E>ETXw$1IBMlnd%~9{EI>aYDyxy
zxk=L?zf2$!*{iMU=jeW%MJZFG5^?-O0(eO>jDIF&nutW4m(Sya0-nVAM9Lq@o$aLv
z-)i=JgM3z$x5~!2&q`j8)4mVxN}SF{J)e$-bN%3_w}#m4xMxYR517luUIu;`WC8fy
zGzC8qyMZ5y?zq1l#XBR^<$TcXCgSD+b&v#cALobavKz&MT`&dsz0q(eaIX3BC=))8
zFE7o4-s{py47PWYU?^na|8t{zr!(Yp=5QU!=mVW>ReBf&!t@F7ifW|ry8k)U(RcY@
z{afg($E0+e|0fa@(ru~V=pR8xu_5*s0+Cjw1AT7<aS!1SV@_+h>M==iQi9wExp9f@
zwGxbMzm)z>5){&Ny@0y1`fYik>`(R6bk*w*5z}KL1V2lV>QhO#-9}|u{#BWu&x!KW
zeADyaK#XEG@loltY|^h(5P8ne`&$yz{eO`D^nIzse!RbrO6FC&-_#+YD0EwzgM{hk
zze|AUBZuJkXPM?=+4)^7xj*{#Vt+o@vfLDVQ#RB<7EHe<G*fkdx)Z(k5Q49%D5TTH
UHd*!er?_SQw`KkJyFU*24@%k9)Bpeg
literal 0
HcmV?d00001
diff --git a/tests/data/test-abidiff-exit/test-decl-enum-v1.c b/tests/data/test-abidiff-exit/test-decl-enum-v1.c
new file mode 100644
index 00000000..046a55d3
--- /dev/null
+++ b/tests/data/test-abidiff-exit/test-decl-enum-v1.c
@@ -0,0 +1,5 @@
+enum embodied_enum { Y };
+enum disembodied_enum;
+
+void reg1(const enum embodied_enum * foo) { (void)foo; }
+void reg2(const enum disembodied_enum * foo) { (void)foo; }
diff --git a/tests/data/test-abidiff-exit/test-decl-enum-v1.o b/tests/data/test-abidiff-exit/test-decl-enum-v1.o
new file mode 100644
index 0000000000000000000000000000000000000000..e9c8d081a21e6d3806fb79661e4fb3c333cc442d
GIT binary patch
literal 3048
zcmbtVPj4GV6o2Dg#}3=LNlZ)ARO(hLZ35jTj!=n96AUSBA}Umnir|2vwRd8#h}YHb
zx}+f53YAK^1qrFDRKbBSz!yM5z#YB>2X1gf;sEc>&e)U5_5x4ZH}C!4-}$@u-n@Rb
z#26@Ium)pIp#Te&vD^}33+gZnH#hcv+t~Z;ul)~y(!?Lo!xmmuk?e?&HFjK+OC(q9
zP2`MCT*1Nm0GaI}E8C*(1d71QVj$5WhQyxSMPAt+4A8o`hjQu59#qLxvB@-9eSka<
zw+q6*Cr=S>pCg&VC#%#0uo?Dzt?pR0`PvMdtxP|MR-0M1HOFx_>zABYou<R;*z5vW
z)l=A!eGWEJFP{a+S;IK2>$BJ9tXl4SevZ<#PCR;Jj>0Ns49hK>_0kD|stuI<@*H-3
z6jM+xxIjHoH~FN5L`+}55s1Qd{496@Ib(m<rjU!h#Z6$4@ig;7-r4bZJnVb78-7QG
zmS2R)yauYr%Ev!1`>V)H7Df-_H0s7YbfY*cOb|vX@3#kG#KSF03)kMh>8>|kahv|K
z-*A_g8qG#ysp)$CY#8&^E{}N<bx;kGPH%O0<)U}-W!LMv-gc1Qi95Yy5D$i_H;g}y
z;?T>2_6|?sV1<Nt8-Q#XUdsBn0IsdCUvd|&@OBi$d0DT~Tr?UGGe!2S6xb*40^6Nt
z^G7PxeO#VXh|@<TPS@+T>4&BDsfnxTfEd{>lM$)No?8xcI{r5N`OjesZIrkihfJ2-
za?@a8L=jo?u9-F~Klmn$S+vWnSuL2<w6SQ7?%Nn6$%3hafYdC&vtK|K9C`=f82Zg>
z>!5D-Nr@|6jfIDBd?H3Oeg@-5_Z?+bnQ)rrGJZre`2Y%6GorqT6#jLt0^UwRv~#0N
zgYsey3xDzcNZr?Ps{2stbWw>^{#*liM+ypmEcN&0sOqZqxM%pEk@~)Ye<k%ta#VSw
zI$A5LTUH>B(^5A5n@_=AiPPHX>(erD)j#;@ojzW39J3@JdrHdqZU%lBWC8f?GzCB5
zJAoha_OQDZ#oGh42{~xD6Mp-EIY@%Ii!X;BvJ=Hzg<uKrJA;0o<Gbd^gN*z5aCvDK
zbUu<nytlQT1br?W|DVG=#?u<=XOdF3h#{E|I{E5!1{K207vMeJNt4a^^Jt@U`CsE3
z7;EOFa;pB%q-bRNUO=0TUrX~L@mB(oR%8I3H-gk1!XKlcRpe$)GMtp)7s6}IOy3)5
zGBNW0MuJP@P$8MVZy{RLe^qX%il_eRbv64B5i@fl1iwg-`qRnu-9lqg{S{eXtrOL!
zcgC!L3o**Mgr82Q<t6=A2T@k_)%dBz%=jN=Je@C{RGb<gpi#-X-|sSB$4EAP=^Z4@
zbpJyF>V2eGA~pY91EBY>X#H*!>>vHTRJ>Z(qS};uLtdz!Y?%Hh^baxPX-~|%hY);6
YO(C7mhY>gDPkD>t@5}z5wtp(&zdst)dH?_b
literal 0
HcmV?d00001
diff --git a/tests/test-abidiff-exit.cc b/tests/test-abidiff-exit.cc
index 5afc15bc..e7bf8362 100644
--- a/tests/test-abidiff-exit.cc
+++ b/tests/test-abidiff-exit.cc
@@ -203,6 +203,15 @@ InOutSpec in_out_specs[] =
"data/test-abidiff-exit/test-decl-struct-report.txt",
"output/test-abidiff-exit/test-decl-struct-report.txt"
},
+ {
+ "data/test-abidiff-exit/test-decl-enum-v0.o",
+ "data/test-abidiff-exit/test-decl-enum-v1.o",
+ "",
+ "--harmless",
+ abigail::tools_utils::ABIDIFF_ABI_CHANGE,
+ "data/test-abidiff-exit/test-decl-enum-report.txt",
+ "output/test-abidiff-exit/test-decl-enum-report.txt"
+ },
{0, 0, 0 ,0, abigail::tools_utils::ABIDIFF_OK, 0, 0}
};
--
2.26.1.301.g55bc3eb7cb9-goog
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH 1/2] Support declaration-only enums.
2020-04-23 13:03 ` [RFC PATCH 1/2] Support declaration-only enums Giuliano Procida
@ 2020-05-12 14:02 ` Matthias Maennich
2020-05-15 14:42 ` Giuliano Procida
2020-05-22 10:03 ` Dodji Seketeli
1 sibling, 1 reply; 8+ messages in thread
From: Matthias Maennich @ 2020-05-12 14:02 UTC (permalink / raw)
To: Giuliano Procida; +Cc: libabigail, dodji, kernel-team
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
> }
>
> /// 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.
> {
> 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.
>+ && !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
>+
>+ // 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.
>+
>+ /// 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
>+ {
>+ 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); ?
>+ }
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.
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
>+
>+ /// 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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH 2/2] Add tests for declaration-only enums.
2020-04-23 13:03 ` [RFC PATCH 2/2] Add tests for " Giuliano Procida
@ 2020-05-12 14:05 ` Matthias Maennich
0 siblings, 0 replies; 8+ messages in thread
From: Matthias Maennich @ 2020-05-12 14:05 UTC (permalink / raw)
To: Giuliano Procida; +Cc: libabigail, dodji, kernel-team
On Thu, Apr 23, 2020 at 02:03:56PM +0100, Giuliano Procida wrote:
>---
Please sign off your work.
Reviewed-by: Matthias Maennich <maennich@google.com>
Cheers,
Matthias
> tests/data/Makefile.am | 5 +++++
> .../test-decl-enum-report.txt | 17 +++++++++++++++++
> .../data/test-abidiff-exit/test-decl-enum-v0.c | 5 +++++
> .../data/test-abidiff-exit/test-decl-enum-v0.o | Bin 0 -> 3048 bytes
> .../data/test-abidiff-exit/test-decl-enum-v1.c | 5 +++++
> .../data/test-abidiff-exit/test-decl-enum-v1.o | Bin 0 -> 3048 bytes
> tests/test-abidiff-exit.cc | 9 +++++++++
> 7 files changed, 41 insertions(+)
> create mode 100644 tests/data/test-abidiff-exit/test-decl-enum-report.txt
> create mode 100644 tests/data/test-abidiff-exit/test-decl-enum-v0.c
> create mode 100644 tests/data/test-abidiff-exit/test-decl-enum-v0.o
> create mode 100644 tests/data/test-abidiff-exit/test-decl-enum-v1.c
> create mode 100644 tests/data/test-abidiff-exit/test-decl-enum-v1.o
>
>diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am
>index a1b9bf64..c036f349 100644
>--- a/tests/data/Makefile.am
>+++ b/tests/data/Makefile.am
>@@ -151,6 +151,11 @@ test-abidiff-exit/test-decl-struct-v0.o \
> test-abidiff-exit/test-decl-struct-v1.c \
> test-abidiff-exit/test-decl-struct-v1.o \
> test-abidiff-exit/test-decl-struct-report.txt \
>+test-abidiff-exit/test-decl-enum-v0.c \
>+test-abidiff-exit/test-decl-enum-v0.o \
>+test-abidiff-exit/test-decl-enum-v1.c \
>+test-abidiff-exit/test-decl-enum-v1.o \
>+test-abidiff-exit/test-decl-enum-report.txt \
> \
> test-diff-dwarf/test0-v0.cc \
> test-diff-dwarf/test0-v0.o \
>diff --git a/tests/data/test-abidiff-exit/test-decl-enum-report.txt b/tests/data/test-abidiff-exit/test-decl-enum-report.txt
>new file mode 100644
>index 00000000..e46ebfa6
>--- /dev/null
>+++ b/tests/data/test-abidiff-exit/test-decl-enum-report.txt
>@@ -0,0 +1,17 @@
>+Functions changes summary: 0 Removed, 2 Changed, 0 Added functions
>+Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
>+
>+2 functions with some indirect sub-type change:
>+
>+ [C] 'function void reg1(const embodied_enum*)' at test-decl-enum-v1.c:4:1 has some indirect sub-type changes:
>+ parameter 1 of type 'const embodied_enum*' has sub-type changes:
>+ in pointed to type 'const embodied_enum':
>+ in unqualified underlying type 'enum embodied_enum' at test-decl-enum-v1.c:1:1:
>+ enum type enum embodied_enum was a declaration-only enum type and is now a defined enum type
>+
>+ [C] 'function void reg2(const disembodied_enum*)' at test-decl-enum-v1.c:5:1 has some indirect sub-type changes:
>+ parameter 1 of type 'const disembodied_enum*' has sub-type changes:
>+ in pointed to type 'const disembodied_enum':
>+ in unqualified underlying type 'enum disembodied_enum':
>+ enum type enum disembodied_enum was a defined enum type and is now a declaration-only enum type
>+
>diff --git a/tests/data/test-abidiff-exit/test-decl-enum-v0.c b/tests/data/test-abidiff-exit/test-decl-enum-v0.c
>new file mode 100644
>index 00000000..d5672618
>--- /dev/null
>+++ b/tests/data/test-abidiff-exit/test-decl-enum-v0.c
>@@ -0,0 +1,5 @@
>+enum embodied_enum;
>+enum disembodied_enum { X };
>+
>+void reg1(const enum embodied_enum * foo) { (void)foo; }
>+void reg2(const enum disembodied_enum * foo) { (void)foo; }
>diff --git a/tests/data/test-abidiff-exit/test-decl-enum-v0.o b/tests/data/test-abidiff-exit/test-decl-enum-v0.o
>new file mode 100644
>index 0000000000000000000000000000000000000000..b4c0b06a089ec6adbfa81b1a619ae30087e54f9d
>GIT binary patch
>literal 3048
>zcmbtWUuzsy6hE^wo1Hk#Hp$xBG=+{(+eDplH%QIaCa$Ko2^OhB#TN;)J2$(7+1WBP
>zTT?|XqC&xk3W5lt4}JmB7hejZzU!Cp!8d&qe9&`d?%CYjOcC_J+<VUN{GEH}oZWYy
>zzk0c17{Fw}I_yh~0xZ?``A*JupaJu6ee=$*n|I#+^X_XuD(v@YVdYkK9_&cYE6Z_(
>zuMl6ews18nQ<rcvU&Y1paH(2(-YI0ksOB574lPK==kMTJ+Zl~eyG(rL;{n(<1u1;2
>z5}$7-KGxZL(q|fXEP_<F{T&I$obgD#;h6Qs`kXOen|%m_TZUO*cN}M{alv`oX*)&(
>zi(LZKK7|!oXJHHF>KSmHb@am$eB+q~vtHPqTc9wV6Q4e}KyI}vx)qwOM&$&iJ&Jx5
>zVA=x8a&ZBRcpRVne88rfCZwL<1VrX4J|;Yht6}_ApTRZ1Tf77VGLdFpD0;h|h{r?k
>zX4CKGuGPn2y2ydzvFh>ns{R_T6%)P3aT@hw5xP;Fff#m2VI;zB@`o@=4+2+SyzXwa
>zo^sp%ir;cqnyq%L)oi=oFdN5WtuJDcL_HLPq&HaGTRrccf712(uD27Ux8mL)8O5V<
>z>W$+!qB!)jpt~#5(wY;|ZvieV@I*Ge0dQqw<AS?%NpzziE|Pn#_Oez+{-(G*C>i6;
>zm%-SZH5QN5?7Mh8rx2%yDF3>it<Qc~*_fHSj0T92?jk9X%JhZi@JgS*1%Lc~SVI?;
>zH^(8BDX&~tnCMYPrtCFcXBIu6!x%+rW<_g4-USL$u;!%S4ysI;IS5F}1pM|($b>_4
>z0FI&EymAiQ*(W8=GVlNLMNHoK0ra1=KJuKIaC%E>ETXw$1IBMlnd%~9{EI>aYDyxy
>zxk=L?zf2$!*{iMU=jeW%MJZFG5^?-O0(eO>jDIF&nutW4m(Sya0-nVAM9Lq@o$aLv
>z-)i=JgM3z$x5~!2&q`j8)4mVxN}SF{J)e$-bN%3_w}#m4xMxYR517luUIu;`WC8fy
>zGzC8qyMZ5y?zq1l#XBR^<$TcXCgSD+b&v#cALobavKz&MT`&dsz0q(eaIX3BC=))8
>zFE7o4-s{py47PWYU?^na|8t{zr!(Yp=5QU!=mVW>ReBf&!t@F7ifW|ry8k)U(RcY@
>z{afg($E0+e|0fa@(ru~V=pR8xu_5*s0+Cjw1AT7<aS!1SV@_+h>M==iQi9wExp9f@
>zwGxbMzm)z>5){&Ny@0y1`fYik>`(R6bk*w*5z}KL1V2lV>QhO#-9}|u{#BWu&x!KW
>zeADyaK#XEG@loltY|^h(5P8ne`&$yz{eO`D^nIzse!RbrO6FC&-_#+YD0EwzgM{hk
>zze|AUBZuJkXPM?=+4)^7xj*{#Vt+o@vfLDVQ#RB<7EHe<G*fkdx)Z(k5Q49%D5TTH
>UHd*!er?_SQw`KkJyFU*24@%k9)Bpeg
>
>literal 0
>HcmV?d00001
>
>diff --git a/tests/data/test-abidiff-exit/test-decl-enum-v1.c b/tests/data/test-abidiff-exit/test-decl-enum-v1.c
>new file mode 100644
>index 00000000..046a55d3
>--- /dev/null
>+++ b/tests/data/test-abidiff-exit/test-decl-enum-v1.c
>@@ -0,0 +1,5 @@
>+enum embodied_enum { Y };
>+enum disembodied_enum;
>+
>+void reg1(const enum embodied_enum * foo) { (void)foo; }
>+void reg2(const enum disembodied_enum * foo) { (void)foo; }
>diff --git a/tests/data/test-abidiff-exit/test-decl-enum-v1.o b/tests/data/test-abidiff-exit/test-decl-enum-v1.o
>new file mode 100644
>index 0000000000000000000000000000000000000000..e9c8d081a21e6d3806fb79661e4fb3c333cc442d
>GIT binary patch
>literal 3048
>zcmbtVPj4GV6o2Dg#}3=LNlZ)ARO(hLZ35jTj!=n96AUSBA}Umnir|2vwRd8#h}YHb
>zx}+f53YAK^1qrFDRKbBSz!yM5z#YB>2X1gf;sEc>&e)U5_5x4ZH}C!4-}$@u-n@Rb
>z#26@Ium)pIp#Te&vD^}33+gZnH#hcv+t~Z;ul)~y(!?Lo!xmmuk?e?&HFjK+OC(q9
>zP2`MCT*1Nm0GaI}E8C*(1d71QVj$5WhQyxSMPAt+4A8o`hjQu59#qLxvB@-9eSka<
>zw+q6*Cr=S>pCg&VC#%#0uo?Dzt?pR0`PvMdtxP|MR-0M1HOFx_>zABYou<R;*z5vW
>z)l=A!eGWEJFP{a+S;IK2>$BJ9tXl4SevZ<#PCR;Jj>0Ns49hK>_0kD|stuI<@*H-3
>z6jM+xxIjHoH~FN5L`+}55s1Qd{496@Ib(m<rjU!h#Z6$4@ig;7-r4bZJnVb78-7QG
>zmS2R)yauYr%Ev!1`>V)H7Df-_H0s7YbfY*cOb|vX@3#kG#KSF03)kMh>8>|kahv|K
>z-*A_g8qG#ysp)$CY#8&^E{}N<bx;kGPH%O0<)U}-W!LMv-gc1Qi95Yy5D$i_H;g}y
>z;?T>2_6|?sV1<Nt8-Q#XUdsBn0IsdCUvd|&@OBi$d0DT~Tr?UGGe!2S6xb*40^6Nt
>z^G7PxeO#VXh|@<TPS@+T>4&BDsfnxTfEd{>lM$)No?8xcI{r5N`OjesZIrkihfJ2-
>za?@a8L=jo?u9-F~Klmn$S+vWnSuL2<w6SQ7?%Nn6$%3hafYdC&vtK|K9C`=f82Zg>
>z>!5D-Nr@|6jfIDBd?H3Oeg@-5_Z?+bnQ)rrGJZre`2Y%6GorqT6#jLt0^UwRv~#0N
>zgYsey3xDzcNZr?Ps{2stbWw>^{#*liM+ypmEcN&0sOqZqxM%pEk@~)Ye<k%ta#VSw
>zI$A5LTUH>B(^5A5n@_=AiPPHX>(erD)j#;@ojzW39J3@JdrHdqZU%lBWC8f?GzCB5
>zJAoha_OQDZ#oGh42{~xD6Mp-EIY@%Ii!X;BvJ=Hzg<uKrJA;0o<Gbd^gN*z5aCvDK
>zbUu<nytlQT1br?W|DVG=#?u<=XOdF3h#{E|I{E5!1{K207vMeJNt4a^^Jt@U`CsE3
>z7;EOFa;pB%q-bRNUO=0TUrX~L@mB(oR%8I3H-gk1!XKlcRpe$)GMtp)7s6}IOy3)5
>zGBNW0MuJP@P$8MVZy{RLe^qX%il_eRbv64B5i@fl1iwg-`qRnu-9lqg{S{eXtrOL!
>zcgC!L3o**Mgr82Q<t6=A2T@k_)%dBz%=jN=Je@C{RGb<gpi#-X-|sSB$4EAP=^Z4@
>zbpJyF>V2eGA~pY91EBY>X#H*!>>vHTRJ>Z(qS};uLtdz!Y?%Hh^baxPX-~|%hY);6
>YO(C7mhY>gDPkD>t@5}z5wtp(&zdst)dH?_b
>
>literal 0
>HcmV?d00001
>
>diff --git a/tests/test-abidiff-exit.cc b/tests/test-abidiff-exit.cc
>index 5afc15bc..e7bf8362 100644
>--- a/tests/test-abidiff-exit.cc
>+++ b/tests/test-abidiff-exit.cc
>@@ -203,6 +203,15 @@ InOutSpec in_out_specs[] =
> "data/test-abidiff-exit/test-decl-struct-report.txt",
> "output/test-abidiff-exit/test-decl-struct-report.txt"
> },
>+ {
>+ "data/test-abidiff-exit/test-decl-enum-v0.o",
>+ "data/test-abidiff-exit/test-decl-enum-v1.o",
>+ "",
>+ "--harmless",
>+ abigail::tools_utils::ABIDIFF_ABI_CHANGE,
>+ "data/test-abidiff-exit/test-decl-enum-report.txt",
>+ "output/test-abidiff-exit/test-decl-enum-report.txt"
>+ },
> {0, 0, 0 ,0, abigail::tools_utils::ABIDIFF_OK, 0, 0}
> };
>
>--
>2.26.1.301.g55bc3eb7cb9-goog
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH 1/2] Support declaration-only enums.
2020-05-12 14:02 ` Matthias Maennich
@ 2020-05-15 14:42 ` Giuliano Procida
0 siblings, 0 replies; 8+ messages in thread
From: Giuliano Procida @ 2020-05-15 14:42 UTC (permalink / raw)
To: Matthias Maennich; +Cc: libabigail, Dodji Seketeli, kernel-team
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
> >
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH 1/2] Support declaration-only enums.
2020-04-23 13:03 ` [RFC PATCH 1/2] Support declaration-only enums Giuliano Procida
2020-05-12 14:02 ` Matthias Maennich
@ 2020-05-22 10:03 ` Dodji Seketeli
2020-06-04 17:23 ` Giuliano Procida
1 sibling, 1 reply; 8+ messages in thread
From: Dodji Seketeli @ 2020-05-22 10:03 UTC (permalink / raw)
To: Giuliano Procida; +Cc: libabigail, kernel-team, maennich
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC PATCH 1/2] Support declaration-only enums.
2020-05-22 10:03 ` Dodji Seketeli
@ 2020-06-04 17:23 ` Giuliano Procida
0 siblings, 0 replies; 8+ messages in thread
From: Giuliano Procida @ 2020-06-04 17:23 UTC (permalink / raw)
To: Dodji Seketeli; +Cc: libabigail, kernel-team, Matthias Männich
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-06-04 17:23 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-23 13:03 [RFC PATCH 0/2] Incomplete enum support Giuliano Procida
2020-04-23 13:03 ` [RFC PATCH 1/2] Support declaration-only enums Giuliano Procida
2020-05-12 14:02 ` Matthias Maennich
2020-05-15 14:42 ` Giuliano Procida
2020-05-22 10:03 ` Dodji Seketeli
2020-06-04 17:23 ` Giuliano Procida
2020-04-23 13:03 ` [RFC PATCH 2/2] Add tests for " Giuliano Procida
2020-05-12 14:05 ` Matthias Maennich
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).