[applied] Bug 33090 - Implement abidw --force-early-suppression

Message ID 875xgdrtqw.fsf@redhat.com
State New
Headers
Series [applied] Bug 33090 - Implement abidw --force-early-suppression |

Commit Message

Dodji Seketeli June 30, 2025, 6:04 p.m. UTC
  Hello,

The new --force-early-suppression makes abidw consider that all
suppression specifications provided by the option

    --suppression </path/to/suppr>

are to be applied in the "early suppression specification" mode, even
if the suppression specification directives don't contain the "drop =
yes" property.

This makes the internal representation nodes that are matched by the
suppression specification be dropped from memory before the ABI is
serialized to ABIXML.

	* doc/manuals/abidw.rst: Add documentation for the new
	--force-early-suppression.  Adjust the docs for the
	--drop-private-types, --headers-dir, --headers-file and
	--suppressions.
	* doc/manuals/suppression-specifications.rst:  Adjust
	documentation for early suppression mode.
	* tools/abidw.cc (options::force_early_suppression): Define new
	data member.
	(options::options): Initialize the new
	options::force_early_suppression.
	(display_usage): Document the new --force-early-suppression.
	(parse_command_line): Support the new --force-early-suppression.
	Also, if --header-dirs or --header-file is set, then set
	--drop-private-types.
	(set_suppressions): If --force-early-suppression is provided, then
	force suppressed IR node to be dropped from memory.
	* tests/test-read-common.h
	(test_task::set_in_public_headers_path): Reset the
	in_public_headers_path by default.
	(test_task::set_in_options): Define new member function.
	* tests/test-read-dwarf.cc (in_out_specs): Add the new test input
	binaries below to the test harness.
	(test_task_dwarf::perform): Take the additional command line
	options into account.
	* tests/data/test-read-dwarf/PR33090/bug[12].c: Source code
	for new test input binary.
	* tests/data/test-read-dwarf/PR33090/impl{1,2}.so: Add new binary
	test input.
	* tests/data/test-read-dwarf/PR33090/impl{1,2}.so.abi: Add new
	expected ABIXML output.
	* tests/data/test-read-dwarf/PR33090/interface.h: Source code for
	new test input binary.
	* tests/data/test-read-dwarf/PR33090/private.suppr: Suppression
	specification for new input test.

Signed-off-by: Dodji Seketeli <dodji@redhat.com>
Applied to the mainline.
---
 doc/manuals/abidw.rst                      | 50 +++++++++++++++++++---
 doc/manuals/suppression-specifications.rst | 18 +++++++-
 tests/data/Makefile.am                     |  8 ++++
 tests/test-read-common.h                   | 14 ++++++
 tests/test-read-dwarf.cc                   | 25 ++++++++++-
 tools/abidw.cc                             | 22 +++++++++-
 6 files changed, 126 insertions(+), 11 deletions(-)
  

Comments

Mark Wielaard June 30, 2025, 9:17 p.m. UTC | #1
Hi Dodji,

On Mon, Jun 30, 2025 at 08:04:23PM +0200, Dodji Seketeli wrote:
> 	* tests/data/test-read-dwarf/PR33090/bug[12].c: Source code
> 	for new test input binary.
> 	* tests/data/test-read-dwarf/PR33090/impl{1,2}.so: Add new binary
> 	test input.
> 	* tests/data/test-read-dwarf/PR33090/impl{1,2}.so.abi: Add new
> 	expected ABIXML output.
> 	* tests/data/test-read-dwarf/PR33090/interface.h: Source code for
> 	new test input binary.
> 	* tests/data/test-read-dwarf/PR33090/private.suppr: Suppression
> 	specification for new input test.

This tests data files haven't been added.
Which makes the buildbots a little sad.

Cheers,

Mark
  

Patch

diff --git a/doc/manuals/abidw.rst b/doc/manuals/abidw.rst
index 0293a3c0..bcb7af60 100644
--- a/doc/manuals/abidw.rst
+++ b/doc/manuals/abidw.rst
@@ -1,3 +1,4 @@ 
+.. _abidw_label:
 
 ======
 abidw
@@ -187,13 +188,20 @@  Options
     the --enable-debug-type-canonicalization option.
 
 
+    .. _abidw_drop_private_types_option_label:
   * ``--drop-private-types``
 
-    This option is to be used with the ``--headers-dir`` and/or
-    ``header-file`` options.  With this option, types that are *NOT*
-    defined in the headers are entirely dropped from the internal
-    representation build by Libabigail to represent the ABI and will
-    not end up in the abi XML file.
+    This option is implicitly set by option :ref:`--headers-dir
+    <abidw_header_dir_option_label>` and :ref:`--header-file
+    <abidw_header_file_option_label>` options.
+
+    With this option, types that are *NOT* defined in the headers are
+    entirely dropped from the internal representation build by
+    Libabigail to represent the ABI and will not end up in the abi XML
+    file.
+
+    This option is provided or compatibility reasons only and should
+    not be explicitly used anymore.
 
 
   * ``--drop-undefined-syms``
@@ -240,14 +248,32 @@  Options
     Group is then serialized out.
 
 
+    .. _abidw_force_early_suppression_option_label:
+  * ``--force-early-suppression``
+
+    This option must be used alongside option :ref:`--suppression
+    <abidw_suppressions_option_label>`.
 
+    It forces :ref:`suppression specifications <suppr_spec_label>` to
+    be applied in :ref:`early suppression mode
+    <early_suppression_mode_label>`.  Artifacts of the internal
+    representation that are matched by suppression specifications are
+    thus suppressed from memory.
 
+    If this option is not used then only suppression specifications
+    with the :ref:`drop property set to 'yes' <drop_property_label>`
+    are effective.  All other suppression specification directives
+    will appear to be ignored.
 
+
+    .. _abidw_header_dir_option_label:
   * ``--headers-dir | --hd`` <headers-directory-path-1>
 
     Specifies where to find the public headers of the binary that the
     tool has to consider.  The tool will thus filter out types that
-    are not defined in public headers.
+    are not defined in public headers.  This option implicitly sets
+    option :ref:`--drop-private-types
+    <abidw_drop_private_types_option_label>`.
 
     Note that several public header directories can be specified for
     the binary to consider.  In that case the ``--header-dir`` option
@@ -267,6 +293,9 @@  Options
     that the tool has to consider.  The tool will thus filter out
     types that are not defined in public headers.
 
+    This option implicitly sets option :ref:`--drop-private-types
+    <abidw_drop_private_types_option_label>`.
+
 
   * ``--help | -h``
 
@@ -433,6 +462,8 @@  Options
     Emit statistics about various internal things.
 
 
+    .. _abidw_suppressions_option_label:
+
   * ``--suppressions | suppr`` <*path-to-suppression-specifications-file*>
 
     Use a :ref:`suppression specification <suppr_spec_label>` file
@@ -442,6 +473,13 @@  Options
     taken into account.  ABI artifacts matched by the suppression
     specifications are suppressed from the output of this tool.
 
+    Only suppression specifications that have the :ref:`drop property
+    set to 'yes' <drop_property_label>` are going to be effective.
+    All other suppression specification directives will appear as
+    being ignored, unless the command line option
+    :ref:`--force-early-suppression
+    <abidw_force_early_suppression_option_label>` is provided.
+
 
   * ``--type-id-style`` <``sequence``|``hash``>
 
diff --git a/doc/manuals/suppression-specifications.rst b/doc/manuals/suppression-specifications.rst
index 5262d7fd..aa5408e2 100644
--- a/doc/manuals/suppression-specifications.rst
+++ b/doc/manuals/suppression-specifications.rst
@@ -118,6 +118,10 @@  is just not mentioned in the ABI change report.  The change report can
 still mention statistics about the number of changed ABI artifacts
 that were suppressed.
 
+This is the default mode of interpretation of suppression
+specifications for most tools including :ref:`abidiff <abidiff_label>`
+and :ref:`abidw <abidw_label>`.
+
 .. _early_suppression_mode_label:
 
 There is another operating mode called the "early suppression mode"
@@ -129,10 +133,20 @@  change about the matched ABI artifact is going to be mentioned in the
 ABI change report and no statistic about the number of suppressed ABI
 changes is available.  Also, please note that because suppressed ABI
 artifacts are removed from the in-memory internal representation in
-this mode, the amount memory used by the internal representation is
+this mode, the amount of memory used by the internal representation is
 potentially smaller than the memory consumption in the late
 suppression mode.
 
+Please note that this mode suppresses data from the internal
+representation constructed by libabigail and thus can lead to
+unintended and surprising consequences.  Consequently this mode has to
+be set explicitly by users either by using the :ref:`drop
+property<drop_property_label>` in a suppression specification or by
+using a special tool command line option like
+:ref:`abidw --force-early-suppression
+<abidw_force_early_suppression_option_label>`.
+
+
 Sections
 ^^^^^^^^
 
@@ -341,7 +355,7 @@  The potential properties of this sections are listed below:
   Note that for this property to be applied to changes to an enum
   type, the size of the enum type must *NOT* have changed.
 
-
+ .. _drop_property_label:
 * ``drop``
 
  Usage:
diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am
index f6b77077..2ffb958f 100644
--- a/tests/data/Makefile.am
+++ b/tests/data/Makefile.am
@@ -893,6 +893,14 @@  test-read-dwarf/PR29692-kdelibs3-libkjava.so.1.0.0.abi \
 test-read-dwarf/test-pointer-to-member-1.cc \
 test-read-dwarf/test-pointer-to-member-1.o \
 test-read-dwarf/test-pointer-to-member-1.o.abi \
+test-read-dwarf/PR33090/bug1.c \
+test-read-dwarf/PR33090/bug2.c \
+test-read-dwarf/PR33090/impl1.so \
+test-read-dwarf/PR33090/impl1.so.abi \
+test-read-dwarf/PR33090/impl2.so \
+test-read-dwarf/PR33090/impl2.so.abi \
+test-read-dwarf/PR33090/interface.h \
+test-read-dwarf/PR33090/private.suppr \
 \
 test-read-ctf/test0		\
 test-read-ctf/test0.abi		\
diff --git a/tests/test-read-common.h b/tests/test-read-common.h
index 3b2884a0..f0d877b7 100644
--- a/tests/test-read-common.h
+++ b/tests/test-read-common.h
@@ -58,6 +58,7 @@  struct test_task : public abigail::workers::task
   string in_abi_path;
   string in_suppr_spec_path;
   string in_public_headers_path;
+  string in_options;
   string out_abi_path;
 
 
@@ -91,12 +92,25 @@  struct test_task : public abigail::workers::task
   void
   set_in_public_headers_path()
   {
+    in_public_headers_path.clear();
     if (spec.in_public_headers_path)
       in_public_headers_path = spec.in_public_headers_path;
     if (!in_public_headers_path.empty())
       in_public_headers_path = in_elf_base + spec.in_public_headers_path;
   }
 
+  /// A setter for the in_options field.
+  ///
+  /// The in_options is the additional option to be passed to the
+  /// abidw command at run time.
+  void
+  set_in_options()
+  {
+    in_options.clear();
+    if (spec.options)
+      in_options = spec.options;
+  }
+
   /// A setter for `out_abi_path` field.
   /// The `out_abi_path` is the full path for output of abixml file.
   /// @return true if `out_abi_path` is a valid directory.
diff --git a/tests/test-read-dwarf.cc b/tests/test-read-dwarf.cc
index 194b3f88..ec39bbb8 100644
--- a/tests/test-read-dwarf.cc
+++ b/tests/test-read-dwarf.cc
@@ -586,6 +586,24 @@  static InOutSpec in_out_specs[] =
     "output/test-read-dwarf/test-pointer-to-member-1.o.abi",
     NULL,
   },
+  {
+    "data/test-read-dwarf/PR33090/impl1.so",
+    "data/test-read-dwarf/PR33090/private.suppr",
+    "",
+    SEQUENCE_TYPE_ID_STYLE,
+    "data/test-read-dwarf/PR33090/impl1.so.abi",
+    "output/test-read-dwarf/PR33090/impl1.so.abi",
+    "--force-early-suppression",
+  },
+  {
+    "data/test-read-dwarf/PR33090/impl2.so",
+    "data/test-read-dwarf/PR33090/private.suppr",
+    "",
+    SEQUENCE_TYPE_ID_STYLE,
+    "data/test-read-dwarf/PR33090/impl2.so.abi",
+    "output/test-read-dwarf/PR33090/impl2.so.abi",
+    "--force-early-suppression",
+  },
   // DWARF fallback feature.
   {
     "data/test-read-dwarf/test-fallback.o",
@@ -655,18 +673,21 @@  test_task_dwarf::perform()
   set_in_elf_path();
   set_in_suppr_spec_path();
   set_in_public_headers_path();
+  set_in_options();
 
   if (!set_out_abi_path()
       || in_elf_path.empty())
     return;
 
   string abidw = string(get_build_dir()) + "/tools/abidw";
-  string drop_private_types, suppr_specs;
+  string drop_private_types, suppr_specs, options;
   if (!in_public_headers_path.empty())
     drop_private_types += "--headers-dir " + in_public_headers_path +
       " --drop-private-types";
   if (!in_suppr_spec_path.empty())
     suppr_specs = " --suppr " + in_suppr_spec_path + " ";
+  if (!in_options.empty())
+    options = " " + in_options + " ";
   string type_id_style = "sequence";
   if (spec.type_id_style == HASH_TYPE_ID_STYLE)
     type_id_style = "hash";
@@ -674,7 +695,7 @@  test_task_dwarf::perform()
   string cmd = abidw + " --no-parameter-names --no-architecture --no-load-undefined-interfaces"
     + " --type-id-style " + type_id_style
     + " --no-corpus-path "
-    + drop_private_types + suppr_specs + " " + in_elf_path
+    + drop_private_types + suppr_specs + options + " " + in_elf_path
     +" > " + out_abi_path;
 
   if (system(cmd.c_str()))
diff --git a/tools/abidw.cc b/tools/abidw.cc
index 9f3f1e03..ebffb8fd 100644
--- a/tools/abidw.cc
+++ b/tools/abidw.cc
@@ -133,6 +133,7 @@  struct options
   bool			annotate;
   bool			do_log;
   bool			drop_private_types;
+  bool			force_early_suppression;
   bool			drop_undefined_syms;
   bool			assume_odr_for_cplusplus;
   bool			leverage_dwarf_factorization;
@@ -180,6 +181,7 @@  struct options
       annotate(),
       do_log(),
       drop_private_types(false),
+      force_early_suppression(false),
       drop_undefined_syms(false),
       assume_odr_for_cplusplus(true),
       leverage_dwarf_factorization(true),
@@ -222,10 +224,11 @@  display_usage(const string& prog_name, ostream& out)
     << "  --debug-tc  debug the type canonicalization process\n"
     << "  --debug-dc  debug the DIE canonicalization process\n"
 #endif
-    << "  --drop-private-types  drop private types from representation\n"
     << "  --drop-undefined-syms  drop undefined symbols from representation\n"
     << "  --exported-interfaces-only  analyze exported interfaces only\n"
     << "  --follow-dependencies  build a corpus group with the dependencies\n"
+    << "  --follow-dependencies  build a corpus group with the dependencies\n"
+    << "  --force-early-suppression  drop IR nodes that match suppression specifications\n"
     << "  --headers-dir|--hd <path> the path to headers of the elf file\n"
     << "  --header-file|--hf <path> the path one header of the elf file\n"
     << "  --help|-h  display this message\n"
@@ -307,6 +310,10 @@  parse_command_line(int argc, char* argv[], options& opts)
 	  if (j >= argc)
 	    return false;
 	  opts.headers_dirs.push_back(argv[j]);
+	  // The user is indirectly defining private types so she
+	  // really wants those types to be removed from the ABIXML.
+	  // So let's drop them from the IR.
+	  opts.drop_private_types = true;
 	  ++i;
 	}
       else if (!strcmp(argv[i], "--added-binaries-dir")
@@ -325,6 +332,10 @@  parse_command_line(int argc, char* argv[], options& opts)
 	  if (j >= argc)
 	    return false;
 	  opts.header_files.push_back(argv[j]);
+	  // The user is indirectly defining private types so she
+	  // really wants those types to be removed from the ABIXML.
+	  // So let's drop them from the IR.
+	  opts.drop_private_types = true;
 	  ++i;
 	}
       else if (!strcmp(argv[i], "--out-file")
@@ -447,6 +458,8 @@  parse_command_line(int argc, char* argv[], options& opts)
 	opts.load_undefined_interfaces = false;
       else if (!strcmp(argv[i], "--drop-private-types"))
 	opts.drop_private_types = true;
+      else if (!strcmp(argv[i], "--force-early-suppression"))
+	opts.force_early_suppression = true;
       else if (!strcmp(argv[i], "--drop-undefined-syms"))
 	opts.drop_undefined_syms = true;
       else if (!strcmp(argv[i], "--exported-interfaces-only"))
@@ -577,6 +590,13 @@  set_suppressions(abigail::elf_based_reader& rdr, options& opts)
        ++i)
     read_suppressions(*i, supprs);
 
+  if (opts.force_early_suppression)
+    // User asked to unconditionally drop suppressed artifacts from
+    // the IR.  Let's drop all nodes matched by suppression
+    // specifications from the IR.
+    for (auto& s : supprs)
+      s->set_drops_artifact_from_ir(true);
+
   suppression_sptr suppr =
     abigail::tools_utils::gen_suppr_spec_from_headers(opts.headers_dirs,
 						      opts.header_files);