A suppressed diff node implies suppressing all equivalent nodes too

When a diff node N is suppressed (for instance, using the
--headers-dir{1,2} option of abidiff}, it's only that diff node that
is suppressed.  We forget to suppress diff nodes that are equivalent
to N.

Here is why we forget to suppress diff ndoes that are equivalent.

apply_suppressions walks the diff node graph to mark diff nodes as
suppressed.  But it does the walking by making sure each diff node's
*class of equivalence* is visited once.  This is not only a way to
prevent infinite loops while visiting the graph, but also an
optimization as it avoids walking two equivalent diff nodes.

But then it can happen that we forget to categorize some diff nodes
inside a given class of equivalence, even though we categorized some
others.

This patch makes it so that when a diff node inside a class of
equivalence is categorized as SUPPRESSED, the canonical diff node of
that class of equivalence is categorized as SUPPRESSED too.  That way,
to know if a diff node is suppressed, we just need to look at its
canonical diff node.

While doing this, I noticed that abidiff and abipkgdiff are not
dropping private types from libabigail's internal representation, even
though the Library now has that capability.  The patch fixes that.
But then the patch adds a --dont-drop-private-types option to abidiff
to avoid dropping those private types from the IR, so that regression
tests can make sure that a suppressed diff node implies suppression
all equivalent nodes too.

	* doc/manuals/abidiff.rst b/doc/manuals/abidiff.rst: Document the
	new --dont-drop-private-types option.
	* src/abg-comparison.cc (diff::is_filtered_out): If the canonical
	type was suppressed then the current diff node is filtered out.
	(suppression_categorization_visitor::visit_{begin,end}):
	Categorized the canonical node as SUPPRESSED if the current node
	is suppressed.
	* tools/abidiff.cc (options::drop_private_types): New data member.
	(options::options): Initialize it.
	(display_usage): Add new help string for the new
	--dont-drop-private-types option.
	(parse_command_line): Parse the new --dont-drop-private-types
	option.
	(set_suppressions): Generate suppression specification from header
	directories given in parameter and stick them to the read context.
	* tools/abipkgdiff.cc (compare): Likewise.
	* tests/data/test-diff-suppr/libtest34-v0.so: New test input.
	* tests/data/test-diff-suppr/libtest34-v1.so: Likewise.
	* tests/data/test-diff-suppr/test34-report-0.txt: New reference
	report.
	* tests/data/test-diff-suppr/test34-v0.c: Source code for the new
	test input.
	* tests/data/test-diff-suppr/test34-v1.c: Likewise.
	* tests/data/test-diff-suppr/test34-priv-include-dir-v0/test34-priv-include-v0.h:
	Likewise.
	* tests/data/test-diff-suppr/test34-priv-include-dir-v1/test34-priv-include-v1.h:
	Likewise.
	* tests/data/test-diff-suppr/test34-pub-include-dir-v0/test34-pub-include-v0.h:
	Likewise.
	* tests/data/test-diff-suppr/test34-pub-include-dir-v1/test34-pub-include-v1.h:
	Likewise.
	* tests/data/Makefile.am: Add new test input material above to
	source distribution.
	* tests/test-diff-suppr.cc (in_out_spec): Compare the two new test
	library provided.  Add --dont-drop-private-types to test30*.

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

doc/manuals/abidiff.rst

@@ -96,6 +96,23 @@ Options
     library that the tool has to consider.  The tool will thus filter
     out ABI changes on types that are not defined in public headers.
 
+  * ``--dont-drop-private-types``
+
+    This option is to be used with the ``--headers-dir1`` and
+    ``--headers-dir2`` options.  Without this option, types that are
+    *NOT* defined in the headers are entirely dropped from the
+    internal representation build by Libabigail to represent the ABI.
+    They thus don't have to be filtered out from the final ABI change
+    report because they are not even present in Libabigail's
+    representation.
+
+    With this option however, those private types are kept in the
+    internal representation and later filtered out from the report.
+
+    This options thus potentially makes Libabigail to potentially
+    consume more memory.  It's meant to be mainly used for debugging
+    purposes.
+
   * ``--stat``
 
     Rather than displaying the detailed ABI differences between

src/abg-comparison.cc

@@ -1944,7 +1944,15 @@ diff::set_local_category(diff_category c)
 /// @return true iff the current diff node should NOT be reported.
 bool
 diff::is_filtered_out() const
-{return priv_->is_filtered_out(get_category());}
+{
+  if (diff * canonical = get_canonical_diff())
+    if (canonical->get_category() & SUPPRESSED_CATEGORY)
+      // The canonical type was suppressed.  This means all the class
+      // of equivalence of that canonical type was suppressed.  So
+      // this node should be suppressed too.
+      return true;
+  return priv_->is_filtered_out(get_category());
+}
 
 /// Test if this diff tree node is to be filtered out for reporting
 /// purposes, but by considering only the categories that were *NOT*
@@ -13116,7 +13124,15 @@ struct suppression_categorization_visitor : public diff_node_visitor
   visit_begin(diff* d)
   {
     if (d->is_suppressed())
-      d->add_to_local_and_inherited_categories(SUPPRESSED_CATEGORY);
+      {
+	d->add_to_local_and_inherited_categories(SUPPRESSED_CATEGORY);
+
+	// If a node was suppressed, all the other nodes of its class
+	// of equivalence are suppressed too.
+	diff *canonical_diff = d->get_canonical_diff();
+	if (canonical_diff != d)
+	  canonical_diff->add_to_category(SUPPRESSED_CATEGORY);
+      }
   }
 
   /// After visiting the children nodes of a given diff node,
@@ -13161,7 +13177,14 @@ struct suppression_categorization_visitor : public diff_node_visitor
 	if (has_non_empty_child
 	    && has_suppressed_child
 	    && !has_non_suppressed_child)
-	  d->add_to_category(SUPPRESSED_CATEGORY);
+	  {
+	    d->add_to_category(SUPPRESSED_CATEGORY);
+	    // If a node was suppressed, all the other nodes of its class
+	    // of equivalence are suppressed too.
+	    diff *canonical_diff = d->get_canonical_diff();
+	    if (canonical_diff != d)
+	      canonical_diff->add_to_category(SUPPRESSED_CATEGORY);
+	  }
       }
   }
 }; //end struct suppression_categorization_visitor

tests/data/Makefile.am

@@ -999,6 +999,15 @@ test-diff-suppr/test33-v0.cc \
 test-diff-suppr/test33-v0.h \
 test-diff-suppr/test33-v1.cc \
 test-diff-suppr/test33-v1.h \
+test-diff-suppr/libtest34-v0.so \
+test-diff-suppr/libtest34-v1.so \
+test-diff-suppr/test34-priv-include-dir-v0/test34-priv-include-v0.h \
+test-diff-suppr/test34-priv-include-dir-v1/test34-priv-include-v1.h \
+test-diff-suppr/test34-pub-include-dir-v0/test34-pub-include-v0.h \
+test-diff-suppr/test34-pub-include-dir-v1/test34-pub-include-v1.h \
+test-diff-suppr/test34-report-0.txt \
+test-diff-suppr/test34-v0.c \
+test-diff-suppr/test34-v1.c \
 \
 test-diff-dwarf-abixml/test0-pr19026-libvtkIOSQL-6.1.so.1 \
 test-diff-dwarf-abixml/test0-pr19026-libvtkIOSQL-6.1.so.1.abi \

tests/data/test-diff-pkg/tbb-4.1-9.20130314.fc22.x86_64--tbb-4.3-3.20141204.fc23.x86_64-report-1.txt

@@ -1,5 +1,5 @@
 ================ changes of 'libtbb.so.2'===============
-  Functions changes summary: 0 Removed, 7 Changed (17 filtered out), 17 Added functions
+  Functions changes summary: 0 Removed, 7 Changed (9 filtered out), 17 Added functions
   Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
   Function symbols changes summary: 0 Removed, 0 Added function symbol not referenced by debug info
   Variable symbols changes summary: 3 Removed, 0 Added variable symbols not referenced by debug info
@@ -46,7 +46,7 @@
         in referenced type 'class tbb::task_group_context' at task.h:302:1:
           1 data member insertion:
             'tbb::internal::cpu_ctl_env_space tbb::task_group_context::my_cpu_ctl_env', at offset 896 (in bits) at task.h:380:1
-          1 data member changes (2 filtered):
+          1 data member changes (1 filtered):
            type of 'char tbb::task_group_context::_leading_padding[80]' changed:
              type name changed from 'char[80]' to 'char[72]'
              array type size changed from 640 to 576 bits:

tests/data/test-diff-suppr/test34-priv-include-dir-v0/test34-priv-include-v0.h

@@ -0,0 +1,17 @@
+struct priv
+{
+  int m0;
+};
+
+void
+init1(struct priv* p)
+{
+  p->m0 = 0;
+}
+
+struct priv*
+init2(struct priv* p)
+{
+  p->m0 = 0xCAFEBABE;
+  return p;
+}

tests/data/test-diff-suppr/test34-priv-include-dir-v1/test34-priv-include-v1.h

@@ -0,0 +1,20 @@
+struct priv
+{
+  unsigned m0;
+  char m1;
+};
+
+void
+init1 (struct priv* p)
+{
+  p->m0 = 0;
+  p->m1 = 0;
+}
+
+struct priv*
+init2(struct priv* p)
+{
+  p->m0 = 0xCAFEBABE;
+  p->m1 = 0xCA;
+  return p;
+}

tests/data/test-diff-suppr/test34-pub-include-dir-v0/test34-pub-include-v0.h

@@ -0,0 +1,20 @@
+struct priv;
+typedef struct priv* priv_ptr;
+
+#define NULL (void*)0
+
+struct S
+{
+  priv_ptr p;
+};
+
+typedef struct S* s_ptr;
+
+void
+foo(s_ptr, int a);
+
+s_ptr
+bar(s_ptr, char a);
+
+void
+baz(s_ptr);

tests/data/test-diff-suppr/test34-pub-include-dir-v1/test34-pub-include-v1.h

@@ -0,0 +1,20 @@
+struct priv;
+typedef struct priv* priv_ptr;
+
+#define NULL (void*)0
+
+struct S
+{
+  priv_ptr p;
+};
+
+typedef struct S* s_ptr;
+
+void
+foo(s_ptr, int a);
+
+s_ptr
+bar(s_ptr, char a);
+
+void
+baz(s_ptr);

tests/data/test-diff-suppr/test34-report-0.txt

@@ -0,0 +1,3 @@
+Functions changes summary: 0 Removed, 0 Changed (5 filtered out), 0 Added function
+Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
+

tests/data/test-diff-suppr/test34-v0.c

@@ -0,0 +1,28 @@
+/*
+ * Compile with:
+ *   gcc -shared -g -Wall -o libtest33-v0.so test33-v0.c
+ */
+
+#include "test33-pub-include-dir-v0/test33-pub-include-v0.h"
+#include "test33-priv-include-dir-v0/test33-priv-include-v0.h"
+
+void
+foo(s_ptr ptr, int a)
+{
+  ptr->p = NULL;
+  ++a;
+}
+
+s_ptr
+bar(s_ptr ptr, char a)
+{
+  ptr->p = NULL;
+  ++a;
+  return ptr;
+}
+
+void
+baz(s_ptr ptr)
+{
+  ptr->p = NULL;
+}

tests/data/test-diff-suppr/test34-v1.c

@@ -0,0 +1,28 @@
+/*
+ * Compile with:
+ *   gcc -shared -g -Wall -o libtest33-v1.so test33-v1.c
+ */
+
+#include "test33-pub-include-dir-v1/test33-pub-include-v1.h"
+#include "test33-priv-include-dir-v1/test33-priv-include-v1.h"
+
+void
+foo(s_ptr ptr, int a)
+{
+  ptr->p = NULL;
+  a++;
+}
+
+s_ptr
+bar(s_ptr ptr, char a)
+{
+  ptr->p = NULL;
+  a++;
+  return ptr;
+}
+
+void
+baz(s_ptr ptr)
+{
+  ptr->p = NULL;
+}

tests/test-diff-suppr.cc

@@ -1604,7 +1604,7 @@ InOutSpec in_out_specs[] =
     "",
     "",
     "",
-    "--no-default-suppression",
+    "--no-default-suppression --dont-drop-private-types",
     "data/test-diff-suppr/test30-report-0.txt",
     "output/test-diff-suppr/test30-report-0.txt"
   },
@@ -1614,7 +1614,7 @@ InOutSpec in_out_specs[] =
     "data/test-diff-suppr/test30-include-dir-v0",
     "data/test-diff-suppr/test30-include-dir-v1",
     "",
-    "--no-default-suppression",
+    "--no-default-suppression --dont-drop-private-types",
     "data/test-diff-suppr/test30-report-1.txt",
     "output/test-diff-suppr/test30-report-1.txt"
   },
@@ -1663,11 +1663,21 @@ InOutSpec in_out_specs[] =
     "data/test-diff-suppr/libtest33-v1.so",
     "",
     "",
-    "data/test-diff-suppr/libtest33-1.suppr",
+    "data/test-diff-suppr/test33-suppr-1.txt",
     "--no-default-suppression --no-show-locs --no-redundant",
     "data/test-diff-suppr/test33-report-0.txt",
     "output/test-diff-suppr/test33-report-0.txt"
   },
+  {
+    "data/test-diff-suppr/libtest34-v0.so",
+    "data/test-diff-suppr/libtest34-v1.so",
+    "data/test-diff-suppr/test34-pub-include-dir-v0",
+    "data/test-diff-suppr/test34-pub-include-dir-v1",
+    "",
+    "--no-default-suppression --dont-drop-private-types",
+    "data/test-diff-suppr/test34-report-0.txt",
+    "output/test-diff-suppr/test34-report-0.txt"
+  },
   // This should be the last entry
   {NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL}
 };

tools/abidiff.cc

@@ -75,6 +75,7 @@ struct options
   vector<string>	keep_var_regex_patterns;
   string		headers_dir1;
   string		headers_dir2;
+  bool			drop_private_types;
   bool			no_default_supprs;
   bool			no_arch;
   bool			show_stats_only;
@@ -103,6 +104,7 @@ struct options
     : display_usage(),
       display_version(),
       missing_operand(),
+      drop_private_types(true),
       no_default_supprs(),
       no_arch(),
       show_stats_only(),
@@ -139,6 +141,8 @@ display_usage(const string& prog_name, ostream& out)
     << " --debug-info-dir2|--d2 <path> the root for the debug info of file2\n"
     << " --headers-dir1|--hd1 <path>  the path to headers of file1\n"
     << " --headers-dir2|--hd2 <path>  the path to headers of file2\n"
+    << "  --dont-drop-private-types\n  keep private types in "
+    "internal representation\n"
     << " --stat  only display the diff stats\n"
     << " --symtabs  only display the symbol tables of the corpora\n"
     << " --no-default-suppression  don't load any "
@@ -275,6 +279,8 @@ parse_command_line(int argc, char* argv[], options& opts)
 	  opts.display_usage = true;
 	  return true;
 	}
+      else if (!strcmp(argv[i], "--dont-drop-private-types"))
+	opts.drop_private_types = false;
       else if (!strcmp(argv[i], "--no-default-suppression"))
 	opts.no_default_supprs = true;
       else if (!strcmp(argv[i], "--no-architecture"))
@@ -591,7 +597,39 @@ set_suppressions(ReadContextType& read_ctxt, const options& opts)
        i != opts.suppression_paths.end();
        ++i)
     read_suppressions(*i, supprs);
-  add_read_context_suppressions(read_ctxt, supprs);
+
+  if (opts.drop_private_types)
+    {
+      if (!opts.headers_dir1.empty())
+	{
+	  // Generate suppression specification to avoid showing ABI
+	  // changes on types that are not defined in public headers.
+	  //
+	  // As these suppression specifications are applied during the
+	  // corpus loading, they are going to be dropped from the
+	  // internal representation altogether.
+	  suppression_sptr suppr =
+	    gen_suppr_spec_from_headers(opts.headers_dir1);
+	  if (suppr)
+	    supprs.push_back(suppr);
+	}
+
+      if (!opts.headers_dir2.empty())
+	{
+	  // Generate suppression specification to avoid showing ABI
+	  // changes on types that are not defined in public headers.
+	  //
+	  // As these suppression specifications are applied during the
+	  // corpus loading, they are going to be dropped from the
+	  // internal representation altogether.
+	  suppression_sptr suppr =
+	    gen_suppr_spec_from_headers(opts.headers_dir2);
+	  if (suppr)
+	    supprs.push_back(suppr);
+	}
+    }
+
+    add_read_context_suppressions(read_ctxt, supprs);
 }
 
 /// Set the regex patterns describing the functions to drop from the

tools/abipkgdiff.cc

@@ -113,6 +113,8 @@ using abigail::comparison::corpus_diff_sptr;
 using abigail::suppr::suppression_sptr;
 using abigail::suppr::suppressions_type;
 using abigail::suppr::read_suppressions;
+using abigail::dwarf_reader::read_context_sptr;
+using abigail::dwarf_reader::create_read_context;
 using abigail::dwarf_reader::get_soname_of_elf_file;
 using abigail::dwarf_reader::get_type_of_elf_file;
 using abigail::dwarf_reader::read_corpus_from_elf;
@@ -1083,18 +1085,23 @@ compare(const elf_file& elf1,
       << elf1.path
       << " ...\n";
 
-  corpus_sptr corpus1 = read_corpus_from_elf(elf1.path, &di_dir1, env.get(),
-					     /*load_all_types=*/false,
-					     c1_status);
-  if (!(c1_status & abigail::dwarf_reader::STATUS_OK))
-    {
-      if (verbose)
-	emit_prefix("abipkgdiff", cerr)
-	  << "Could not read file '"
-	  << elf1.path
-	  << "' properly\n";
-      return abigail::tools_utils::ABIDIFF_ERROR;
-    }
+  corpus_sptr corpus1;
+  {
+    read_context_sptr c = create_read_context(elf1.path, &di_dir1, env.get(),
+					      /*load_all_types=*/false);
+    add_read_context_suppressions(*c, priv_types_supprs1);
+    corpus1 = read_corpus_from_elf(*c, c1_status);
+
+    if (!(c1_status & abigail::dwarf_reader::STATUS_OK))
+      {
+	if (verbose)
+	  emit_prefix("abipkgdiff", cerr)
+	    << "Could not read file '"
+	    << elf1.path
+	    << "' properly\n";
+	return abigail::tools_utils::ABIDIFF_ERROR;
+      }
+  }
 
   if (opts.fail_if_no_debug_info
       && (c1_status & abigail::dwarf_reader::STATUS_DEBUG_INFO_NOT_FOUND))
@@ -1120,18 +1127,23 @@ compare(const elf_file& elf1,
       << elf2.path
       << " ...\n";
 
-  corpus_sptr corpus2 = read_corpus_from_elf(elf2.path, &di_dir2, env.get(),
-					     /*load_all_types=*/false,
-					     c2_status);
-  if (!(c2_status & abigail::dwarf_reader::STATUS_OK))
-    {
-      if (verbose)
-	emit_prefix("abipkgdiff", cerr)
-	  << "Could not find the read file '"
-	  << elf2.path
-	  << "' properly\n";
-      return abigail::tools_utils::ABIDIFF_ERROR;
-    }
+  corpus_sptr corpus2;
+  {
+    read_context_sptr c = create_read_context(elf2.path, &di_dir2, env.get(),
+					      /*load_all_types=*/false);
+    add_read_context_suppressions(*c, priv_types_supprs2);
+    corpus2 = read_corpus_from_elf(*c, c2_status);
+
+    if (!(c2_status & abigail::dwarf_reader::STATUS_OK))
+      {
+	if (verbose)
+	  emit_prefix("abipkgdiff", cerr)
+	    << "Could not find the read file '"
+	    << elf2.path
+	    << "' properly\n";
+	return abigail::tools_utils::ABIDIFF_ERROR;
+      }
+  }
 
   if (opts.fail_if_no_debug_info
       && (c2_status & abigail::dwarf_reader::STATUS_DEBUG_INFO_NOT_FOUND))