PR23641 - Type definition DIE matched by a supprspec but not its decl

Suppose we have two versions of a library which are almost identical.
Suppose the difference between the two binaries is a slight difference of
ordering in how the linker put together what the DWARF describes.

Then, while processing the debug info of the first library, libabigail
first comes across the forward declaration of a type T.  Suppose that
declaration is not matched by any private type suppression
specification.  Libabigail is going to keep the declaration of T and
build an internal representation (IR) for it.  It's also going to
build an IR for types and decls using T like "T*" or "T* var".

Now suppose that while processing the debug info of the second
library, libabigail first comes across the *definition* of T --
because in this second library, that definition comes first.  Suppose
that the definition is matched by a private type suppression
specification, unlike the declaration in the first library.  In this
case, T is going to be dropped, and no IR is going to be built for it.
If other types or decl like 'T*' or 'T *var" refer to this
*definition* of T, they will be dropped too.

We'll end up with those two libraries that are identical (modulo the
order in which libabigail sees type declarations and their
definitions) and are considered different when a suppression
specification makes us drop T: the second library appears to
libabigail as if T was removed from it.

This is the problem addressed by this patch.

When the definition of a type T is suppressed because it's considered
private then we look if there was a forward declaration for it
elsewhere, that is not matched by the private type suppression
specification.  If we encountered such a type declaration then it
means that declaration is in effect an "opaque" version of T.  So
rather than just dropping T altogether, we keep (and build an IR) for
its opaque version only.  And we drop the definition of T.

This seems to fix the issue.

I can't seem to reproduce the slight re-ordering of DIEs descriptions that
uncover the issue so I'll rely on integration tests to catch future
regressions on this issue, rather than on unit tests.  Sigh.

	* include/abg-tools-utils.h (PRIVATE_TYPES_SUPPR_SPEC_NAME):
	Remove this extern constant definition.
	* src/abg-dwarf-reader.cc (type_is_suppressed): Add an overload
	that takes an additional type_is_private output parameter.
	(get_opaque_version_of_type): New static function.
	(build_ir_node_from_die): For class types, get the opaque version
	for suppressed private types rather than dropping them altogether.
	* src/abg-reader.cc (type_is_suppressed): Adjust.
	* src/abg-suppression-priv.h (type_is_suppressed): Add an overload
	that takes a type_is_private output parameter.
	* include/abg-suppression.h (get_private_types_suppr_spec_label)
	(is_private_type_suppr_spec): Declare new functions.
	* src/abg-suppression.cc
	(get_private_types_suppr_spec_label, is_private_type_suppr_spec):
	Define new functions.
	(suppression_matches_type_name_or_location): Use the new
	get_private_types_suppr_spec_label rather than a global extern
	variable.
	* src/abg-tools-utils.cc (handle_fts_entry): Adjust to use the new
	get_private_types_suppr_spec_label.
	(gen_suppr_spec_from_headers): Handle the case or an empty headers
	root dir.

Signed-off-by: Dodji Seketeli <dodji@redhat.com>

include/abg-suppression.h

@@ -796,6 +796,11 @@ file_suppression_sptr
 file_is_suppressed(const string& file_path,
 		   const suppressions_type& suppressions);
 
+const char*
+get_private_types_suppr_spec_label();
+
+bool
+is_private_type_suppr_spec(suppr::type_suppression&);
 } // end namespace suppr
 
 } // end namespace abigail

include/abg-tools-utils.h

@@ -274,8 +274,6 @@ build_corpus_group_from_kernel_dist_under(const string&	root,
 					  suppr::suppressions_type&	supprs,
 					  bool				verbose,
 					  environment_sptr&		env);
-
-extern const char* PRIVATE_TYPES_SUPPR_SPEC_NAME;
 }// end namespace tools_utils
 
 /// A macro that expands to aborting the program when executed.

src/abg-dwarf-reader.cc

@@ -14638,13 +14638,19 @@ variable_is_suppressed(const read_context& ctxt,
 ///
 /// @param type_die the DIE that designates the type to consider.
 ///
+/// @param type_is_private out parameter.  If this function returns
+/// true (the type @p type_die is suppressed) and if the type was
+/// suppressed because it's private then this parameter is set to
+/// true.
+///
 /// @return true iff the type designated by the DIE @p type_die, in
 /// the scope @p scope is suppressed by at the suppression
 /// specifications associated to the current read context.
 static bool
 type_is_suppressed(const read_context& ctxt,
 		   const scope_decl* scope,
-		   Dwarf_Die *type_die)
+		   Dwarf_Die *type_die,
+		   bool &type_is_private)
 {
   if (type_die == 0
       || (dwarf_tag(type_die) != DW_TAG_enumeration_type
@@ -14660,9 +14666,79 @@ type_is_suppressed(const read_context& ctxt,
 
   return suppr::type_is_suppressed(ctxt, qualified_name,
 				   type_location,
+				   type_is_private,
 				   /*require_drop_property=*/true);
 }
 
+/// Test if a type (designated by a given DIE) in a given scope is
+/// suppressed by the suppression specifications that are associated
+/// to a given read context.
+///
+/// @param ctxt the read context to consider.
+///
+/// @param scope of the scope of the type DIE to consider.
+///
+/// @param type_die the DIE that designates the type to consider.
+///
+/// @return true iff the type designated by the DIE @p type_die, in
+/// the scope @p scope is suppressed by at the suppression
+/// specifications associated to the current read context.
+static bool
+type_is_suppressed(const read_context& ctxt,
+		   const scope_decl* scope,
+		   Dwarf_Die *type_die)
+{
+  bool type_is_private = false;
+  return type_is_suppressed(ctxt, scope, type_die, type_is_private);
+}
+
+/// Get the opaque version of a type that was suppressed because it's
+/// a private type.
+///
+/// The opaque version version of the type is just a declared-only
+/// version of the type (class or union type) denoted by @p type_die.
+///
+/// @param ctxt the read context in use.
+///
+/// @param scope the scope of the type die we are looking at.
+///
+/// @param type_die the type DIE we are looking at.
+///
+/// @return the opaque version of the type denoted by @p type_die or
+/// nil if no opaque version was found.
+static class_or_union_sptr
+get_opaque_version_of_type(const read_context	&ctxt,
+			   scope_decl		*scope,
+			   Dwarf_Die		*type_die)
+{
+  class_or_union_sptr result;
+
+    if (type_die == 0
+      || (dwarf_tag(type_die) != DW_TAG_enumeration_type
+	  && dwarf_tag(type_die) != DW_TAG_class_type
+	  && dwarf_tag(type_die) != DW_TAG_structure_type
+	  && dwarf_tag(type_die) != DW_TAG_union_type))
+    return result;
+
+  string type_name, linkage_name;
+  location type_location;
+  die_loc_and_name(ctxt, type_die, type_location, type_name, linkage_name);
+  if (!type_location)
+    return result;
+
+  string qualified_name = build_qualified_name(scope, type_name);
+
+  // TODO: also handle declaration-only unions.  To do that, we mostly
+  // need to adapt add_or_update_union_type to make it schedule
+  // declaration-only unions for resolution too.
+  string_classes_map::const_iterator i =
+    ctxt.declaration_only_classes().find(qualified_name);
+  if (i != ctxt.declaration_only_classes().end())
+    result = i->second.back();
+
+  return result;
+}
+
 /// Create a function symbol with a given name.
 ///
 /// @param sym_name the name of the symbol to create.
@@ -15301,7 +15377,19 @@ build_ir_node_from_die(read_context&	ctxt,
     case DW_TAG_class_type:
     case DW_TAG_structure_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);
+	else if (!type_suppressed)
 	  {
 	    Dwarf_Die spec_die;
 	    scope_decl_sptr scop;

src/abg-reader.cc

@@ -3087,7 +3087,9 @@ type_is_suppressed(const read_context& ctxt, xmlNodePtr node)
 
   string qualified_name = build_qualified_name(scope, type_name);
 
+  bool type_is_private = false;
   return suppr::type_is_suppressed(ctxt, qualified_name, type_location,
+				   type_is_private,
 				   /*require_drop_property=*/true);
 }
 

src/abg-suppression-priv.h

@@ -808,6 +808,38 @@ type_is_suppressed(const ReadContextType&	ctxt,
 		   const string&		type_name,
 		   const location&		type_location,
 		   bool require_drop_property = false)
+{
+  bool type_is_private = false;
+  return type_is_suppressed(ctxt, type_name, type_location, type_is_private);
+}
+
+/// Test if a type (designated by its name and location) is suppressed
+/// by at least one suppression specification associated with a given
+/// read context.
+///
+/// @param ctxt the read context to consider.
+///
+/// @param type_name the name of the type to consider.
+///
+/// @param type_location the location of the type to consider.
+///
+/// @param type_is_private out parameter. If the type is suppressed
+/// because it's private then this out parameter is set to true.
+///
+/// @param require_drop_property if set to "true", tests if the type
+/// is suppressed and if its representation is dropped from the ABI
+/// corpus being built.  Otherwise, if set to "false", only test if
+/// the type is suppressed.
+///
+/// @return true iff at least one type specification matches a type
+/// with name @p type_name and with location @p type_location.
+template <typename ReadContextType>
+bool
+type_is_suppressed(const ReadContextType&	ctxt,
+		   const string&		type_name,
+		   const location&		type_location,
+		   bool&			type_is_private,
+		   bool require_drop_property = false)
 {
   for (suppr::suppressions_type::const_iterator i =
 	 ctxt.get_suppressions().begin();
@@ -819,11 +851,18 @@ type_is_suppressed(const ReadContextType&	ctxt,
 	  continue;
 	if (ctxt.suppression_matches_type_name_or_location(*suppr, type_name,
 							   type_location))
-	  return true;
+	  {
+	    if (is_private_type_suppr_spec(*suppr))
+	      type_is_private = true;
+
+	    return true;
+	  }
       }
 
+  type_is_private = false;
   return false;
 }
+
 // </type_suppression stuff>
 
 }// end namespace suppr

src/abg-suppression.cc

@@ -980,7 +980,7 @@ suppression_matches_type_location(const type_suppression&	s,
 		// the declaration.  If we reach this place, it
 		// means the class has no definition at this point.
 		assert(!cl->get_definition_of_declaration());
-	      if (s.get_label() == tools_utils::PRIVATE_TYPES_SUPPR_SPEC_NAME)
+	      if (s.get_label() == get_private_types_suppr_spec_label())
 		// So this looks like what really amounts to an
 		// opaque type.  So it's not defined in the public
 		// headers.  So we want to filter it out.
@@ -4051,6 +4051,30 @@ file_is_suppressed(const string& file_path,
 
   return file_suppression_sptr();
 }
+
+/// @return the name of the artificial private type suppression
+/// specification that is auto-generated by libabigail to suppress
+/// change reports about types that are not defined in public headers.
+const char*
+get_private_types_suppr_spec_label()
+{
+  static const char *PRIVATE_TYPES_SUPPR_SPEC_NAME =
+    "Artificial private types suppression specification";
+
+  return PRIVATE_TYPES_SUPPR_SPEC_NAME;
+}
+
+/// Test if a type suppression specification represents a private type
+/// suppression automatically generated by libabigail from the user
+/// telling us where public headers are.
+///
+/// @param s the suppression specification we are looking at.
+///
+/// @return true iff @p s is a private type suppr spec.
+bool
+is_private_type_suppr_spec(suppr::type_suppression& s)
+{return s.get_label() == get_private_types_suppr_spec_label();}
+
 // </file_suppression stuff>
 }// end namespace suppr
 } // end namespace abigail

src/abg-tools-utils.cc

@@ -1445,11 +1445,6 @@ make_path_absolute(const char*p)
   return result;
 }
 
-/// The name of the artificial private type suppression specification
-/// that is auto-generated by libabigail to suppress change reports
-/// about types that are not defined in public headers.
-const char* PRIVATE_TYPES_SUPPR_SPEC_NAME =
-  "Artificial private types suppression specification";
 
 /// This is a sub-routine of gen_suppr_spec_from_headers.
 ///
@@ -1482,7 +1477,7 @@ handle_fts_entry(const FTSENT *entry,
 	{
 	  if (!suppr)
 	    {
-	      suppr.reset(new type_suppression(PRIVATE_TYPES_SUPPR_SPEC_NAME,
+	      suppr.reset(new type_suppression(get_private_types_suppr_spec_label(),
 					       /*type_name_regexp=*/"",
 					       /*type_name=*/""));
 
@@ -1511,9 +1506,15 @@ handle_fts_entry(const FTSENT *entry,
 type_suppression_sptr
 gen_suppr_spec_from_headers(const string& headers_root_dir)
 {
-  char* paths[] = {const_cast<char*>(headers_root_dir.c_str()), 0};
   type_suppression_sptr result;
 
+  if (headers_root_dir.empty())
+    // We were given no headers root dir so the resulting suppression
+    // specification shall be empty.
+    return result;
+
+  char* paths[] = {const_cast<char*>(headers_root_dir.c_str()), 0};
+
   FTS *file_hierarchy = fts_open(paths, FTS_LOGICAL|FTS_NOCHDIR, NULL);
   if (!file_hierarchy)
     return result;