From patchwork Mon May 4 12:34:06 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Giuliano Procida X-Patchwork-Id: 39200 From: gprocida@google.com (Giuliano Procida) Date: Mon, 4 May 2020 13:34:06 +0100 Subject: [PATCH v4 05/15] diff suppression: Fix handling of change kinds. In-Reply-To: <20200504123416.243214-1-gprocida@google.com> References: <20200424092132.150547-1-gprocida@google.com> <20200504123416.243214-1-gprocida@google.com> Message-ID: <20200504123416.243214-6-gprocida@google.com> 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 Reviewed-by: Matthias Maennich --- 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