diff mbox series

[2/3] Allow offset changes to be considered harmless.

Message ID 20200504162439.74028-3-gprocida@google.com
State Rejected
Headers
Series Add an option to give finer-grained control of offset reporting. |

Commit Message

Giuliano Procida May 4, 2020, 4:24 p.m. UTC 
  This changes lets the user flip offset changes from harmful to
harmless. This is a simple way of shrinking very verbose diffs where
most changes within structs are offset changes caused by much rarer
member addition, removal and size changes.

	* include/abg-comparison.h (diff_category): Mechanically split
	SIZE_OR_OFFSET_CHANGE_CATEGORY into SIZE_CHANGE_CATEGORY and
	OFFSET_CHANGE_CATEGORY.
	* src/abg-comp-filter.cc (categorize_harmful_diff_node): Ditto.
	* src/abg-comparison.cc
	(get_default_harmless_categories_bitmap): Ditto.
	(operator<<): In the diff_category overload, ditto.
	* src/abg-reporter-priv.cc (represent): Split
	show_size_offset_changes into show_size_changes and
	show_offset_changes and split conditional to match.
	(report_size_and_alignment_changes): Test SIZE_CHANGE_CATEGORY
	instead of old SIZE_OR_OFFSET_CHANGE_CATEGORY. Add a TODO to
	check an unexpected test.
	* tools/abidiff.cc: (options): Add offset_changes_are_harmless
	and plumb this through.

Signed-off-by: Giuliano Procida <gprocida@google.com>
---
 include/abg-comparison.h | 35 +++++++++++++++++++++--------------
 src/abg-comp-filter.cc   | 14 +++++++++-----
 src/abg-comparison.cc    | 15 ++++++++++++---
 src/abg-reporter-priv.cc | 17 +++++++++++------
 tools/abidiff.cc         | 20 +++++++++++++++++---
 5 files changed, 70 insertions(+), 31 deletions(-)

Comments

Dodji Seketeli May 13, 2020, 11:46 a.m. UTC | #1 
Giuliano Procida <gprocida@google.com> a écrit:

> This changes lets the user flip offset changes from harmful to
> harmless. This is a simple way of shrinking very verbose diffs where
> most changes within structs are offset changes caused by much rarer
> member addition, removal and size changes.

Hmmh.

I think this is probably too simple.

Offset changes can really signal a problem on their own.  So I am not
for categorizing them as harmless.

I understand that the reporting of offset changes that are a consequence
of an addition, removal or change of a data member can be verbose, but
I'd rather keep them as is for now, rather than risking some false
negative because we want to go the easy route.

In other words, if we really want to filter out those /consequential/
offset changes, then we need to properly detect them and flag them as
being filtered.

I understand that that is a bigger undertaking, but I think that would
the right way to do it.

So I'd rather not apply this patch, have an enhancement request filed
for that task and discuss/work on it properly instead.

Cheers,
Giuliano Procida May 14, 2020, 1:21 p.m. UTC | #2 
Hi.

On Wed, 13 May 2020 at 12:46, Dodji Seketeli <dodji@seketeli.org> wrote:

> Giuliano Procida <gprocida@google.com> a écrit:
>
> > This changes lets the user flip offset changes from harmful to
> > harmless. This is a simple way of shrinking very verbose diffs where
> > most changes within structs are offset changes caused by much rarer
> > member addition, removal and size changes.
>
> Hmmh.
>
> I think this is probably too simple.
>
> Offset changes can really signal a problem on their own.  So I am not
> for categorizing them as harmless.
>
> I understand that the reporting of offset changes that are a consequence
> of an addition, removal or change of a data member can be verbose, but
> I'd rather keep them as is for now, rather than risking some false
> negative because we want to go the easy route.
>
> In other words, if we really want to filter out those /consequential/
> offset changes, then we need to properly detect them and flag them as
> being filtered.
>

The tests do show them being filtered out. Did you mean in some other way?

+        1 data member changes (2 filtered):
+          type of 'int S::a[4]' changed:
+            type name changed from 'int[4]' to 'int[8]'
+            array type size changed from 128 to 256
+            array type subrange 1 changed length from 4 to 8


> I understand that that is a bigger undertaking, but I think that would
> the right way to do it.
>
> So I'd rather not apply this patch, have an enhancement request filed
> for that task and discuss/work on it properly instead.
>

I think the wider discussion is mostly covered by the other email thread.

Regards,
Giuliano.


> Cheers,
>
> --
>                 Dodji
>
> --
> To unsubscribe from this group and stop receiving emails from it, send an
> email to kernel-team+unsubscribe@android.com.
>
>
diff mbox series

Patch

diff --git a/include/abg-comparison.h b/include/abg-comparison.h
index 4f60ff99..8df84404 100644
--- a/include/abg-comparison.h
+++ b/include/abg-comparison.h
@@ -388,48 +388,54 @@  enum diff_category
   PRIVATE_TYPE_CATEGORY = 1 << 9,
 
   /// This means the diff node (or at least one of its descendant
-  /// nodes) carries a change that modifies the size of a type or an
-  /// offset of a type member.  Removal or changes of enumerators in a
-  /// enum fall in this category too.
-  SIZE_OR_OFFSET_CHANGE_CATEGORY = 1 << 10,
+  /// nodes) carries a change that modifies the size of a type.
+  /// Removal or changes of enumerators in a enum fall in this
+  /// category too.
+  SIZE_CHANGE_CATEGORY = 1 << 10,
+
+  /// This means the diff node (or at least one of its descendant
+  /// nodes) carries a change that modifies the offset of a type
+  ///  member.
+  OFFSET_CHANGE_CATEGORY = 1 << 11,
 
   /// This means that a diff node in the sub-tree carries an
   /// incompatible change to a vtable.
-  VIRTUAL_MEMBER_CHANGE_CATEGORY = 1 << 11,
+  VIRTUAL_MEMBER_CHANGE_CATEGORY = 1 << 12,
 
   /// A diff node in this category is redundant.  That means it's
   /// present as a child of a other nodes in the diff tree.
-  REDUNDANT_CATEGORY = 1 << 12,
+  REDUNDANT_CATEGORY = 1 << 13,
 
   /// This means that a diff node in the sub-tree carries a class type
   /// that was declaration-only and that is now defined, or vice
   /// versa.
-  CLASS_DECL_ONLY_DEF_CHANGE_CATEGORY = 1 << 13,
+  CLASS_DECL_ONLY_DEF_CHANGE_CATEGORY = 1 << 14,
 
   /// A diff node in this category is a function parameter type which
   /// top cv-qualifiers change.
-  FN_PARM_TYPE_TOP_CV_CHANGE_CATEGORY = 1 << 14,
+  FN_PARM_TYPE_TOP_CV_CHANGE_CATEGORY = 1 << 15,
 
   /// A diff node in this category has a function parameter type with a
   /// cv-qualifiers change.
-  FN_PARM_TYPE_CV_CHANGE_CATEGORY = 1 << 15,
+  FN_PARM_TYPE_CV_CHANGE_CATEGORY = 1 << 16,
 
   /// A diff node in this category is a function return type with a
   /// cv-qualifier change.
-  FN_RETURN_TYPE_CV_CHANGE_CATEGORY = 1 << 16,
+  FN_RETURN_TYPE_CV_CHANGE_CATEGORY = 1 << 17,
 
   /// A diff node in this category is for a variable which type holds
   /// a cv-qualifier change.
-  VAR_TYPE_CV_CHANGE_CATEGORY = 1 << 17,
+  VAR_TYPE_CV_CHANGE_CATEGORY = 1 << 18,
 
   /// A diff node in this category carries a change from void pointer
   /// to non-void pointer.
-  VOID_PTR_TO_PTR_CHANGE_CATEGORY = 1 << 18,
+  VOID_PTR_TO_PTR_CHANGE_CATEGORY = 1 << 19,
 
   /// A diff node in this category carries a change in the size of the
   /// array type of a global variable, but the ELF size of the
   /// variable didn't change.
-  BENIGN_INFINITE_ARRAY_CHANGE_CATEGORY = 1 << 19,
+  BENIGN_INFINITE_ARRAY_CHANGE_CATEGORY = 1 << 20,
+
   /// A special enumerator that is the logical 'or' all the
   /// enumerators above.
   ///
@@ -446,7 +452,8 @@  enum diff_category
   | HARMLESS_UNION_CHANGE_CATEGORY
   | SUPPRESSED_CATEGORY
   | PRIVATE_TYPE_CATEGORY
-  | SIZE_OR_OFFSET_CHANGE_CATEGORY
+  | SIZE_CHANGE_CATEGORY
+  | OFFSET_CHANGE_CATEGORY
   | VIRTUAL_MEMBER_CHANGE_CATEGORY
   | REDUNDANT_CATEGORY
   | CLASS_DECL_ONLY_DEF_CHANGE_CATEGORY
diff --git a/src/abg-comp-filter.cc b/src/abg-comp-filter.cc
index 75df901c..4e66e8b3 100644
--- a/src/abg-comp-filter.cc
+++ b/src/abg-comp-filter.cc
@@ -1585,14 +1585,18 @@  categorize_harmful_diff_node(diff *d, bool pre)
       // or removal.
       //
       // TODO: be more specific -- not all size changes are harmful.
-      if (!has_class_decl_only_def_change(d)
-	  && (type_size_changed(f, s)
-	      || data_member_offset_changed(f, s)
+      if (!has_class_decl_only_def_change(d))
+	{
+	  if (type_size_changed(f, s)
 	      || non_static_data_member_type_size_changed(f, s)
 	      || non_static_data_member_added_or_removed(d)
 	      || base_classes_added_or_removed(d)
-	      || has_harmful_enum_change(d)))
-	category |= SIZE_OR_OFFSET_CHANGE_CATEGORY;
+	      || has_harmful_enum_change(d))
+	    category |= SIZE_CHANGE_CATEGORY;
+
+	  if (data_member_offset_changed(f, s))
+	    category |= OFFSET_CHANGE_CATEGORY;
+	}
 
       if (has_virtual_mem_fn_change(d))
 	category |= VIRTUAL_MEMBER_CHANGE_CATEGORY;
diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc
index 46bf9e30..32a2cf1b 100644
--- a/src/abg-comparison.cc
+++ b/src/abg-comparison.cc
@@ -2963,7 +2963,8 @@  get_default_harmless_categories_bitmap()
 diff_category
 get_default_harmful_categories_bitmap()
 {
-  return (abigail::comparison::SIZE_OR_OFFSET_CHANGE_CATEGORY
+  return (abigail::comparison::SIZE_CHANGE_CATEGORY
+	  | abigail::comparison::OFFSET_CHANGE_CATEGORY
 	  | abigail::comparison::VIRTUAL_MEMBER_CHANGE_CATEGORY);
 }
 
@@ -3065,11 +3066,19 @@  operator<<(ostream& o, diff_category c)
       emitted_a_category |= true;
     }
 
-  if (c & SIZE_OR_OFFSET_CHANGE_CATEGORY)
+  if (c & SIZE_CHANGE_CATEGORY)
     {
       if (emitted_a_category)
 	o << "|";
-      o << "SIZE_OR_OFFSET_CHANGE_CATEGORY";
+      o << "SIZE_CHANGE_CATEGORY";
+      emitted_a_category |= true;
+    }
+
+  if (c & OFFSET_CHANGE_CATEGORY)
+    {
+      if (emitted_a_category)
+	o << "|";
+      o << "OFFSET_CHANGE_CATEGORY";
       emitted_a_category |= true;
     }
 
diff --git a/src/abg-reporter-priv.cc b/src/abg-reporter-priv.cc
index 4ea591aa..1897b448 100644
--- a/src/abg-reporter-priv.cc
+++ b/src/abg-reporter-priv.cc
@@ -422,8 +422,10 @@  represent(const var_diff_sptr	&diff,
   const uint64_t n_offset = get_data_member_offset(n);
   const string o_pretty_representation = o->get_pretty_representation();
   // no n_pretty_representation here as it's only needed in a couple of places
-  const bool show_size_offset_changes = ctxt->get_allowed_category()
-					& SIZE_OR_OFFSET_CHANGE_CATEGORY;
+  const bool show_size_changes = ctxt->get_allowed_category()
+				 & SIZE_CHANGE_CATEGORY;
+  const bool show_offset_changes = ctxt->get_allowed_category()
+				   & OFFSET_CHANGE_CATEGORY;
 
   // Has the main diff text been output?
   bool emitted = false;
@@ -554,7 +556,7 @@  represent(const var_diff_sptr	&diff,
 	out << "now becomes laid out";
       emitted = true;
     }
-  if (show_size_offset_changes)
+  if (show_offset_changes)
     {
       if (o_offset != n_offset)
 	{
@@ -577,7 +579,9 @@  represent(const var_diff_sptr	&diff,
 	  maybe_show_relative_offset_change(diff, *ctxt, out);
 	  emitted = true;
 	}
-
+    }
+  if (show_size_changes)
+    {
       if (!size_reported && o_size != n_size)
 	{
 	  if (begin_with_and)
@@ -728,7 +732,7 @@  report_size_and_alignment_changes(type_or_decl_base_sptr	first,
   unsigned fdc = first_array ? first_array->get_dimension_count(): 0,
     sdc = second_array ? second_array->get_dimension_count(): 0;
 
-  if (ctxt->get_allowed_category() & SIZE_OR_OFFSET_CHANGE_CATEGORY)
+  if (ctxt->get_allowed_category() & SIZE_CHANGE_CATEGORY)
     {
       if (fs != ss || fdc != sdc)
 	{
@@ -794,12 +798,13 @@  report_size_and_alignment_changes(type_or_decl_base_sptr	first,
 	    }
 	} // end if (fs != ss || fdc != sdc)
       else
+	// TODO: Is this the right check?
 	if (ctxt->show_relative_offset_changes())
 	  {
 	    out << indent << "type size hasn't changed\n";
 	  }
     }
-  if ((ctxt->get_allowed_category() & SIZE_OR_OFFSET_CHANGE_CATEGORY)
+  if ((ctxt->get_allowed_category() & SIZE_CHANGE_CATEGORY)
       && (fa != sa))
     {
       out << indent;
diff --git a/tools/abidiff.cc b/tools/abidiff.cc
index cffe5ab5..0dbd05f3 100644
--- a/tools/abidiff.cc
+++ b/tools/abidiff.cc
@@ -34,9 +34,12 @@ 
 #include "abg-dwarf-reader.h"
 
 using abg_compat::shared_ptr;
+using abigail::comparison::NO_CHANGE_CATEGORY;
+using abigail::comparison::OFFSET_CHANGE_CATEGORY;
 using abigail::comparison::compute_diff;
 using abigail::comparison::corpus_diff;
 using abigail::comparison::corpus_diff_sptr;
+using abigail::comparison::diff_category;
 using abigail::comparison::diff_context;
 using abigail::comparison::diff_context_sptr;
 using abigail::comparison::get_default_harmful_categories_bitmap;
@@ -95,6 +98,7 @@  struct options
   bool			show_hexadecimal_values;
   bool			show_offsets_sizes_in_bits;
   bool			show_relative_offset_changes;
+  bool			offset_changes_are_harmless;
   bool			show_stats_only;
   bool			show_symtabs;
   bool			show_deleted_fns;
@@ -136,6 +140,7 @@  struct options
       show_hexadecimal_values(),
       show_offsets_sizes_in_bits(true),
       show_relative_offset_changes(true),
+      offset_changes_are_harmless(false),
       show_stats_only(),
       show_symtabs(),
       show_deleted_fns(),
@@ -224,6 +229,7 @@  display_usage(const string& prog_name, ostream& out)
     << " --show-bits  show size and offsets in bits\n"
     << " --show-hex  show size and offset in hexadecimal\n"
     << " --show-dec  show size and offset in decimal\n"
+    << " --offset-changes-are-harmless"
     << " --no-show-relative-offset-changes  do not show relative"
     " offset changes\n"
     << " --suppressions|--suppr <path> specify a suppression file\n"
@@ -480,6 +486,8 @@  parse_command_line(int argc, char* argv[], options& opts)
 	opts.show_hexadecimal_values = false;
       else if (!strcmp(argv[i], "--no-show-relative-offset-changes"))
 	opts.show_relative_offset_changes = false;
+      else if (!strcmp(argv[i], "--offset-changes-are-harmless"))
+	opts.offset_changes_are_harmless = true;
       else if (!strcmp(argv[i], "--suppressions")
 	       || !strcmp(argv[i], "--suppr"))
 	{
@@ -690,7 +698,7 @@  set_diff_context_from_opts(diff_context_sptr ctxt,
   // redundancy analysis pass altogether.  That could help save a
   // couple of CPU cycle here and there!
   ctxt->show_redundant_changes(opts.show_redundant_changes
-                               || opts.leaf_changes_only);
+			       || opts.leaf_changes_only);
   ctxt->show_symbols_unreferenced_by_debug_info
     (opts.show_symbols_not_referenced_by_debug_info);
   ctxt->show_added_symbols_unreferenced_by_debug_info
@@ -698,11 +706,17 @@  set_diff_context_from_opts(diff_context_sptr ctxt,
   ctxt->show_unreachable_types(opts.show_all_types);
   ctxt->show_impacted_interfaces(opts.show_impacted_interfaces);
 
+  diff_category extra_harmless = opts.offset_changes_are_harmless
+				 ? OFFSET_CHANGE_CATEGORY
+				 : NO_CHANGE_CATEGORY;
+
   if (!opts.show_harmless_changes)
-      ctxt->switch_categories_off(get_default_harmless_categories_bitmap());
+    ctxt->switch_categories_off(get_default_harmless_categories_bitmap()
+				| extra_harmless);
 
   if (!opts.show_harmful_changes)
-    ctxt->switch_categories_off(get_default_harmful_categories_bitmap());
+    ctxt->switch_categories_off(get_default_harmful_categories_bitmap()
+				&~ extra_harmless);
 
   suppressions_type supprs;
   for (vector<string>::const_iterator i = opts.suppression_paths.begin();