[PATCH v4 05/15] diff suppression: Fix handling of change kinds.
Matthias Maennich
maennich@google.com
Mon May 4 13:04:43 GMT 2020
On Mon, May 04, 2020 at 01:34:06PM +0100, Giuliano Procida wrote:
>When parsing suppression specifications, libabigail attempts to detect
>and ignore suppressions that are empty and so would match everything
>in their category by default.
>
>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 (like 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 fixes up) this short-cutting on the
>grounds that it is a maintenance burden and inconsistent behaviour.
>Users should be able to do this:
>
>[suppress_variable]
> label = Suppress all changes to variables
>
>We could reinstate the logic when the code has global knowledge of
>which fields are present and which have no suppression (restriction)
>semantics, or perhaps just emit a warning message to the user if they
>have supplied a completely empty (no label even) 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>
Reviewed-by: Matthias Maennich <maennich@google.com>
Cheers,
Matthias
>---
> src/abg-suppression.cc | 91 +++++--------------
> .../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, 26 insertions(+), 116 deletions(-)
>
>diff --git a/src/abg-suppression.cc b/src/abg-suppression.cc
>index 217ec5e9..51885cf2 100644
>--- a/src/abg-suppression.cc
>+++ b/src/abg-suppression.cc
>@@ -1835,19 +1835,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)
>@@ -3283,32 +3270,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()
>@@ -3319,11 +3290,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");
>
>@@ -4151,27 +4122,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()
>@@ -4188,7 +4147,7 @@ read_variable_suppression(const ini::config::section& section)
> if (!symbol_name_not_regex_str.empty())
> result->set_symbol_name_not_regex_str(symbol_name_not_regex_str);
>
>- if (result && !change_kind_str.empty())
>+ if (!change_kind_str.empty())
> result->set_change_kind
> (variable_suppression::parse_change_kind(change_kind_str));
>
>@@ -4331,12 +4290,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
>
>--
>2.26.2.526.g744177e7f7-goog
>
More information about the Libabigail
mailing list