From patchwork Thu Apr 23 15:44:26 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Giuliano Procida X-Patchwork-Id: 39133 From: gprocida@google.com (Giuliano Procida) Date: Thu, 23 Apr 2020 16:44:26 +0100 Subject: [PATCH 06/21] diff suppression: Fix handling of change kinds. In-Reply-To: <20200423154441.170531-1-gprocida@google.com> References: <20200423154441.170531-1-gprocida@google.com> Message-ID: <20200423154441.170531-7-gprocida@google.com> 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 --- 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 --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