[RFC PATCH 0/2] Incomplete enum support

[RFC PATCH 0/2] Incomplete enum support

[Giuliano Procida]

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

[RFC PATCH 1/2] Support declaration-only enums.

[Giuliano Procida]

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

Re: [RFC PATCH 1/2] Support declaration-only enums.

[Matthias Maennich]

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
>

Re: [RFC PATCH 1/2] Support declaration-only enums.

[Giuliano Procida]

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
> >
>

Re: [RFC PATCH 1/2] Support declaration-only enums.

[Dodji Seketeli]

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

Re: [RFC PATCH 1/2] Support declaration-only enums.

[Giuliano Procida]

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

[RFC PATCH 2/2] Add tests for declaration-only enums.

[Giuliano Procida]

---
 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

Re: [RFC PATCH 2/2] Add tests for declaration-only enums.

[Matthias Maennich]

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
>