public inbox for libabigail@sourceware.org
 help / color / mirror / Atom feed
From: Giuliano Procida <gprocida@google.com>
To: libabigail@sourceware.org
Cc: dodji@seketeli.org, kernel-team@android.com, gprocida@google.com,
	 maennich@google.com
Subject: [RFC PATCH 1/2] Support declaration-only enums.
Date: Thu, 23 Apr 2020 14:03:55 +0100	[thread overview]
Message-ID: <20200423130356.93136-2-gprocida@google.com> (raw)
In-Reply-To: <20200423130356.93136-1-gprocida@google.com>

This is an almost blind attempt at addition of support for incomplete,
also known as forward-declared, enum types. I've not made any attempt
to refactor or share logic with the struct/union code and I've
probably got other things wrong.

Signed-off-by: Giuliano Procida <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


  reply	other threads:[~2020-04-23 13:04 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-23 13:03 [RFC PATCH 0/2] Incomplete enum support Giuliano Procida
2020-04-23 13:03 ` Giuliano Procida [this message]
2020-05-12 14:02   ` [RFC PATCH 1/2] Support declaration-only enums Matthias Maennich
2020-05-15 14:42     ` Giuliano Procida
2020-05-22 10:03   ` Dodji Seketeli
2020-06-04 17:23     ` Giuliano Procida
2020-04-23 13:03 ` [RFC PATCH 2/2] Add tests for " Giuliano Procida
2020-05-12 14:05   ` Matthias Maennich

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200423130356.93136-2-gprocida@google.com \
    --to=gprocida@google.com \
    --cc=dodji@seketeli.org \
    --cc=kernel-team@android.com \
    --cc=libabigail@sourceware.org \
    --cc=maennich@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).