From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm1-x342.google.com (mail-wm1-x342.google.com [IPv6:2a00:1450:4864:20::342]) by sourceware.org (Postfix) with ESMTPS id 1DAB3383E804 for ; Mon, 4 May 2020 13:04:46 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 1DAB3383E804 Received: by mail-wm1-x342.google.com with SMTP id r26so8971015wmh.0 for ; Mon, 04 May 2020 06:04:46 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=teOZsfdpXMcJFMPNahXTowqYkFUoF13CctUuzHeBW9A=; b=ERNrR/kusFWmtkmx2CRRqywog8HaY7fYeYnUl9iVsJsCaQRizOvdO9aZFQw3vYwb+d Iq+dxmcqncOHOR/8dgXLpqEjgMGVR4XE2AknMNB8VF8Xw8lxxKwGVOUvq8naubLXM5P3 TId7OWEMFQl+HVa5BfVOv+QFJqbWwEKdsNS5vfG/f/Mdvhl3FhMrQsOnbpHWhwjvPc7U DkX8yVzog9HTGlVSrfCMv27vMhjwHCGJVKB4Bjvtkwp1FeT68pDlJPLThwHButjfObwB PuvZtMeBt6atCpSs2MVtUGG8BQcoJudqPkAMKc/m3u+v+guY/P8z6C/GqQbJeEFH3PFf xSfw== X-Gm-Message-State: AGi0PubLa1hSQ9pwSGNBlIgfTMMltq7Vm16Rsi6wHUAoydxtyYFsJ8zP pIF1RwSTsUW74qw9L9Wn2yDdcqIa1kwfkQ== X-Google-Smtp-Source: APiQypKcb7vfqdKprGIcRpvYJoCkWreHOfALxRzZMx6ZLHFjS7SjUt7eW8ocjyxxEq8Xr/ybPZj5Pw== X-Received: by 2002:a05:600c:1008:: with SMTP id c8mr13969371wmc.14.1588597484786; Mon, 04 May 2020 06:04:44 -0700 (PDT) Received: from google.com ([2a00:79e0:d:210:e8f7:125b:61e9:733d]) by smtp.gmail.com with ESMTPSA id n9sm18625868wrx.61.2020.05.04.06.04.44 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Mon, 04 May 2020 06:04:44 -0700 (PDT) Date: Mon, 4 May 2020 15:04:43 +0200 From: Matthias Maennich To: Giuliano Procida Cc: libabigail@sourceware.org, dodji@seketeli.org, kernel-team@android.com Subject: Re: [PATCH v4 05/15] diff suppression: Fix handling of change kinds. Message-ID: <20200504130443.GA16445@google.com> References: <20200424092132.150547-1-gprocida@google.com> <20200504123416.243214-1-gprocida@google.com> <20200504123416.243214-6-gprocida@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Disposition: inline In-Reply-To: <20200504123416.243214-6-gprocida@google.com> X-Spam-Status: No, score=-41.3 required=5.0 tests=BAYES_00, DKIMWL_WL_MED, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, ENV_AND_HDR_SPF_MATCH, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, USER_IN_DEF_DKIM_WL, USER_IN_DEF_SPF_WL autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: libabigail@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Libabigail mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 04 May 2020 13:04:56 -0000 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 Reviewed-by: Matthias Maennich 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 >