diff mbox series

[06/21] diff suppression: Fix handling of change kinds.

Message ID 20200423154441.170531-7-gprocida@google.com
State Superseded
Headers show
Series Simplify regex and suppression parsing. | expand

Commit Message

Giuliano Procida April 23, 2020, 3:44 p.m. UTC
When parsing suppression specifications, libabigail attempts to detect
and so ignore useless suppressions. This is an optimisation to avoid
the cost of testing against a suppression that will do nothing.

Unfortunately, with the way the parser currently works, these checks
have to be against an exhaustive list of fields that matter, rather
than by listing the fields that don't (label). They are fragile in the
face of changes that add new fields.

Two of the short-cut checks are in fact buggy, missing out the
change_kind field. One of the checks also risks a null pointer
dereference as it doesn't actually trigger a return from the function.

This patch eliminates (rather than fixing up) this short-cutting on
the grounds that it buys very litte and is a maintenance burden. In a
future version, it could be reinstated with logic that has global
knowledge of which fields are present and which fields have no
suppression semantics and perhaps emitting warning message to the user
if they have supplied a useless specification.

The patch also corrects 4 affected test cases to reflect that
suppression is actually happening (according to change_kind).

	* src/abg-suppression.cc (read_type_suppression): Remove
	short-circuiting of useless suppressions.
	(read_function_suppression): Ditto.
	(read_variable_suppression: Ditto.
	(read_file_suppression): Ditto.
	tests/data/test-diff-suppr/test15-suppr-added-fn-report-5.txt:
	Fix test - something is actually suppressed.
	* tests/data/test-diff-suppr/test16-suppr-removed-fn-report-5.txt:
	Ditto.
	* tests/data/test-diff-suppr/test17-suppr-added-var-report-5.txt:
	Ditto.
	* tests/data/test-diff-suppr/test18-suppr-removed-var-report-5.txt:
	Ditto.

Signed-off-by: Giuliano Procida <gprocida@google.com>
---
 src/abg-suppression.cc                        | 89 +++++--------------
 .../test15-suppr-added-fn-report-5.txt        |  6 +-
 .../test16-suppr-removed-fn-report-5.txt      | 15 +---
 .../test17-suppr-added-var-report-5.txt       | 15 +---
 .../test18-suppr-removed-var-report-5.txt     | 15 +---
 5 files changed, 25 insertions(+), 115 deletions(-)
diff mbox series

Patch

diff --git a/src/abg-suppression.cc b/src/abg-suppression.cc
index 2fbbd61b..b600c88c 100644
--- a/src/abg-suppression.cc
+++ b/src/abg-suppression.cc
@@ -1840,19 +1840,6 @@  read_type_suppression(const ini::config::section& section)
 	changed_enumerator_names.push_back(p->get_value()->as_string());
     }
 
-  if (file_name_regex_str.empty()
-      && file_name_not_regex_str.empty()
-      && soname_regex_str.empty()
-      && soname_not_regex_str.empty()
-      && (!name_regex_prop || name_regex_prop->get_value()->as_string().empty())
-      && (!name_not_regex_prop
-	  || name_not_regex_prop->get_value()->as_string().empty())
-      && (!name_prop || name_prop->get_value()->as_string().empty())
-      && !consider_type_kind
-      && srcloc_not_regexp_str.empty()
-      && srcloc_not_in.empty())
-    return result;
-
   result.reset(new type_suppression(label_str, name_regex_str, name_str));
 
   if (consider_type_kind)
@@ -3288,32 +3275,16 @@  read_function_suppression(const ini::config::section& section)
 	  parms.push_back(parm);
       }
 
-  if (!label_str.empty()
-      || !name.empty()
-      || !name_regex_str.empty()
-      || !name_not_regex_str.empty()
-      || !file_name_regex_str.empty()
-      || !file_name_not_regex_str.empty()
-      || !soname_regex_str.empty()
-      || !soname_not_regex_str.empty()
-      || !return_type_name.empty()
-      || !return_type_regex_str.empty()
-      || !sym_name.empty()
-      || !sym_name_regex_str.empty()
-      || !sym_name_not_regex_str.empty()
-      || !sym_version.empty()
-      || !sym_ver_regex_str.empty()
-      || !parms.empty())
-
-    result.reset(new function_suppression(label_str, name,
-					  name_regex_str,
-					  return_type_name,
-					  return_type_regex_str,
-					  parms,
-					  sym_name,
-					  sym_name_regex_str,
-					  sym_version,
-					  sym_ver_regex_str));
+  result.reset(new function_suppression(label_str,
+					name,
+					name_regex_str,
+					return_type_name,
+					return_type_regex_str,
+					parms,
+					sym_name,
+					sym_name_regex_str,
+					sym_version,
+					sym_ver_regex_str));
 
   if ((drop_artifact_str == "yes" || drop_artifact_str == "true")
       && (!name.empty()
@@ -3324,11 +3295,11 @@  read_function_suppression(const ini::config::section& section)
 	  || !sym_name_not_regex_str.empty()))
     result->set_drops_artifact_from_ir(true);
 
-  if (result && !change_kind_str.empty())
+  if (!change_kind_str.empty())
     result->set_change_kind
       (function_suppression::parse_change_kind(change_kind_str));
 
-  if (result && !allow_other_aliases.empty())
+  if (!allow_other_aliases.empty())
     result->set_allow_other_aliases(allow_other_aliases == "yes"
 				    || allow_other_aliases == "true");
 
@@ -4157,27 +4128,15 @@  read_variable_suppression(const ini::config::section& section)
     ? type_name_regex_prop->get_value()->as_string()
      : "";
 
-  if (label_str.empty()
-      && name_str.empty()
-      && name_regex_str.empty()
-      && name_not_regex_str.empty()
-      && file_name_regex_str.empty()
-      && file_name_not_regex_str.empty()
-      && soname_regex_str.empty()
-      && soname_not_regex_str.empty()
-      && symbol_name.empty()
-      && symbol_name_regex_str.empty()
-      && symbol_name_not_regex_str.empty()
-      && symbol_version.empty()
-      && symbol_version_regex_str.empty()
-      && type_name_str.empty()
-      && type_name_regex_str.empty())
-    return result;
-
-  result.reset(new variable_suppression(label_str, name_str, name_regex_str,
-					symbol_name, symbol_name_regex_str,
-					symbol_version, symbol_version_regex_str,
-					type_name_str, type_name_regex_str));
+  result.reset(new variable_suppression(label_str,
+					name_str,
+					name_regex_str,
+					symbol_name,
+					symbol_name_regex_str,
+					symbol_version,
+					symbol_version_regex_str,
+					type_name_str,
+					type_name_regex_str));
 
   if ((drop_artifact_str == "yes" || drop_artifact_str == "true")
       && (!name_str.empty()
@@ -4337,12 +4296,6 @@  read_file_suppression(const ini::config::section& section)
     ? soname_not_regex_prop->get_value()->as_string()
     : "";
 
-  if (file_name_regex_str.empty()
-      && file_name_not_regex_str.empty()
-      && soname_regex_str.empty()
-      && soname_not_regex_str.empty())
-    return result;
-
   result.reset(new file_suppression(label_str,
 				    file_name_regex_str,
 				    file_name_not_regex_str));
diff --git a/tests/data/test-diff-suppr/test15-suppr-added-fn-report-5.txt b/tests/data/test-diff-suppr/test15-suppr-added-fn-report-5.txt
index 4eaba5b7..83dfe326 100644
--- a/tests/data/test-diff-suppr/test15-suppr-added-fn-report-5.txt
+++ b/tests/data/test-diff-suppr/test15-suppr-added-fn-report-5.txt
@@ -1,10 +1,6 @@ 
-Functions changes summary: 0 Removed, 1 Changed, 1 Added functions
+Functions changes summary: 0 Removed, 1 Changed, 0 Added (1 filtered out) functions
 Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
 
-1 Added function:
-
-  [A] 'function void bar()'    {_Z3barv}
-
 1 function with some indirect sub-type change:
 
   [C] 'function void bar(S&)' has some indirect sub-type changes:
diff --git a/tests/data/test-diff-suppr/test16-suppr-removed-fn-report-5.txt b/tests/data/test-diff-suppr/test16-suppr-removed-fn-report-5.txt
index b28fbd16..851f7728 100644
--- a/tests/data/test-diff-suppr/test16-suppr-removed-fn-report-5.txt
+++ b/tests/data/test-diff-suppr/test16-suppr-removed-fn-report-5.txt
@@ -1,16 +1,3 @@ 
-Functions changes summary: 1 Removed, 1 Changed, 0 Added functions
+Functions changes summary: 0 Removed (1 filtered out), 0 Changed (1 filtered out), 0 Added functions
 Variables changes summary: 0 Removed, 0 Changed, 0 Added variable
 
-1 Removed function:
-
-  [D] 'function void bar()'    {_Z3barv}
-
-1 function with some indirect sub-type change:
-
-  [C] 'function void bar(S*)' has some indirect sub-type changes:
-    parameter 1 of type 'S*' has sub-type changes:
-      in pointed to type 'struct S':
-        type size changed from 32 to 64 (in bits)
-        1 data member insertion:
-          'unsigned int S::bar', at offset 32 (in bits)
-
diff --git a/tests/data/test-diff-suppr/test17-suppr-added-var-report-5.txt b/tests/data/test-diff-suppr/test17-suppr-added-var-report-5.txt
index 6965a151..f4e0aa29 100644
--- a/tests/data/test-diff-suppr/test17-suppr-added-var-report-5.txt
+++ b/tests/data/test-diff-suppr/test17-suppr-added-var-report-5.txt
@@ -1,16 +1,3 @@ 
 Functions changes summary: 0 Removed, 0 Changed, 0 Added function
-Variables changes summary: 0 Removed, 1 Changed, 1 Added variables
-
-1 Added variable:
-
-  [A] 'int var1'    {var1}
-
-1 Changed variable:
-
-  [C] 'S* var0' was changed:
-    type of variable changed:
-      in pointed to type 'struct S':
-        type size changed from 32 to 64 (in bits)
-        1 data member insertion:
-          'char S::m1', at offset 32 (in bits)
+Variables changes summary: 0 Removed, 0 Changed (1 filtered out), 0 Added (1 filtered out) variables
 
diff --git a/tests/data/test-diff-suppr/test18-suppr-removed-var-report-5.txt b/tests/data/test-diff-suppr/test18-suppr-removed-var-report-5.txt
index 3edf2bd1..ac380a4a 100644
--- a/tests/data/test-diff-suppr/test18-suppr-removed-var-report-5.txt
+++ b/tests/data/test-diff-suppr/test18-suppr-removed-var-report-5.txt
@@ -1,16 +1,3 @@ 
 Functions changes summary: 0 Removed, 0 Changed, 0 Added function
-Variables changes summary: 1 Removed, 1 Changed, 0 Added variables
-
-1 Removed variable:
-
-  [D] 'int var1'    {var1}
-
-1 Changed variable:
-
-  [C] 'S* var0' was changed:
-    type of variable changed:
-      in pointed to type 'struct S':
-        type size changed from 32 to 64 (in bits)
-        1 data member insertion:
-          'char S::m1', at offset 32 (in bits)
+Variables changes summary: 0 Removed (1 filtered out), 0 Changed (1 filtered out), 0 Added variables