From patchwork Wed Jul 22 10:55:38 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Giuliano Procida X-Patchwork-Id: 40144 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 962993857C6A; Wed, 22 Jul 2020 10:55:47 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 962993857C6A DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1595415347; bh=s8SyU4+rPCiesYj6meRbHSjpZxrns675cYPfQSqfA9E=; h=Date:Subject:To:List-Id:List-Unsubscribe:List-Archive:List-Help: List-Subscribe:From:Reply-To:Cc:From; b=DX76gR+UaqCnzI7L7zALARHkYsdKbfSX4KA5FYTSW4ov9j7BdZi/XeEforYpXlmE0 s1BwzLi847b+9uZMux6RqDdn2wKz8tJZ7cHCa939w9pIOL+RLQlYfZf7r+ITiD2FP8 Z/dRzVbuqko8xoBdI26Zn5W6EgbazI7oPtuhOcFM= X-Original-To: libabigail@sourceware.org Delivered-To: libabigail@sourceware.org Received: from mail-wr1-x449.google.com (mail-wr1-x449.google.com [IPv6:2a00:1450:4864:20::449]) by sourceware.org (Postfix) with ESMTPS id 556A03857C6A for ; Wed, 22 Jul 2020 10:55:44 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org 556A03857C6A Received: by mail-wr1-x449.google.com with SMTP id a18so476515wrm.14 for ; Wed, 22 Jul 2020 03:55:44 -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:message-id:mime-version:subject:from:to:cc; bh=s8SyU4+rPCiesYj6meRbHSjpZxrns675cYPfQSqfA9E=; b=SVdal2wR00rVVuzaMXP6jXhCrv0v/OXFwTEIo4PgQKOUpWykh+0Jo3dSeTNpmrTIcT +hOVip8AzhWXEjaZ9sKPqMiMS9yQD8HgdUAq37ZhF9I5YLLRvIo/IPYdNnwfH7eWDqqd vqf47egKQ4HqP+xiqMWgBku8aK96fT1A+PqZi/Ft+J00YgHLJ7b5N8EmgX68jwKrGBSq wfI9LOmEeegi2OrY3eovXMc+UAim3VTN51oT1KxzDccGIgKRT/jeLkc1qGOPgYthG9Aq BFumGKY4MCcKUrj1DjPw9Nb8hY63ylwb5H57W2mHIG1ZntyRGCc/d4/3oGF8mU1DpQV6 19IQ== X-Gm-Message-State: AOAM532jCN84F3PBhfKHjzjbEE/QSO1mbZp9HYTEW/h8r3dqlgwLQkD6 u+qA6wxX8ElzF1+bTEqk9tLUttpeigcABjX6kVhvuEuFNKR/TlyHDZ2m6BXbd/pBry9nGxZiUMH PBKb6ILjmJV+FiW2kucphPne4bQ+ioLbis1eKJRHHvr1ajWBh5QM27KR13P7KX3tEmYJwFF4= X-Google-Smtp-Source: ABdhPJz1IjNegEBpB273D56mgMeAWVn16V7ja+lMZ1cnpL+6xdyeTP0rh/cKDnDrMsu+CQ8QVGYDGhIknVmxUg== X-Received: by 2002:a7b:cd93:: with SMTP id y19mr704673wmj.0.1595415342697; Wed, 22 Jul 2020 03:55:42 -0700 (PDT) Date: Wed, 22 Jul 2020 11:55:38 +0100 Message-Id: <20200722105538.2543928-1-gprocida@google.com> Mime-Version: 1.0 X-Mailer: git-send-email 2.28.0.rc0.105.gf9edc3c819-goog Subject: [PATCH] Fix maybe_report_data_members_replaced_by_anon_dm To: libabigail@sourceware.org X-Spam-Status: No, score=-23.2 required=5.0 tests=BAYES_00, DKIMWL_WL_MED, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, USER_IN_DEF_DKIM_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: Mailing list of the Libabigail project List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-Patchwork-Original-From: Giuliano Procida via Libabigail From: Giuliano Procida Reply-To: Giuliano Procida Cc: maennich@google.com, kernel-team@android.com Errors-To: libabigail-bounces@sourceware.org Sender: "Libabigail" The function maybe_report_data_members_replaced_by_anon_dm has some minor issues. This commit tidies these up. The function came to my attention when a broken equality test triggered an infinite loop. The issues included: - anonymous_data_member, assigned but not used - two iterators i, j used, when one would suffice - dms_replaced_by_same_anon_dm declared outside loop, gets reused - self-comparison of first decl, potential infinite loop The third of these would result incorrect diff reports under the right circumstances. * src/abg-reporter-priv.cc (maybe_report_data_members_replaced_by_anon_dm): Move declarations of anonymous_data_member and dms_replaced_by_same_anon_dm into inner loop. Use anonymous_data_member for testing and reporting, allowing iterators i and j to be replaced by just iterator i. Push first decl onto dms_replaced_by_same_anon_dm unconditionally and move control flow logic into loop condition. Signed-off-by: Giuliano Procida --- src/abg-reporter-priv.cc | 39 +++++++----------- tests/data/Makefile.am | 8 ++++ .../test-PR25661-7-report-1.txt | 3 ++ .../test-PR25661-7-report-2.txt | 5 +++ .../test-PR25661-7-report-3.txt | 14 +++++++ .../test-PR25661-7-report-4.txt | 11 +++++ .../data/test-diff-filter/test-PR25661-7-v0.c | 12 ++++++ .../data/test-diff-filter/test-PR25661-7-v0.o | Bin 0 -> 2680 bytes .../data/test-diff-filter/test-PR25661-7-v1.c | 26 ++++++++++++ .../data/test-diff-filter/test-PR25661-7-v1.o | Bin 0 -> 2976 bytes tests/test-diff-filter.cc | 28 +++++++++++++ 11 files changed, 123 insertions(+), 23 deletions(-) create mode 100644 tests/data/test-diff-filter/test-PR25661-7-report-1.txt create mode 100644 tests/data/test-diff-filter/test-PR25661-7-report-2.txt create mode 100644 tests/data/test-diff-filter/test-PR25661-7-report-3.txt create mode 100644 tests/data/test-diff-filter/test-PR25661-7-report-4.txt create mode 100644 tests/data/test-diff-filter/test-PR25661-7-v0.c create mode 100644 tests/data/test-diff-filter/test-PR25661-7-v0.o create mode 100644 tests/data/test-diff-filter/test-PR25661-7-v1.c create mode 100644 tests/data/test-diff-filter/test-PR25661-7-v1.o diff --git a/src/abg-reporter-priv.cc b/src/abg-reporter-priv.cc index ef925608..78491143 100644 --- a/src/abg-reporter-priv.cc +++ b/src/abg-reporter-priv.cc @@ -1437,30 +1437,25 @@ maybe_report_data_members_replaced_by_anon_dm(const class_or_union_diff &d, // Let's detect all the data members that are replaced by // members of the same anonymous data member and report them // in one go. - changed_var_sptrs_type::const_iterator i, j; - i = d.ordered_data_members_replaced_by_adms().begin(); - // This contains the data members replaced by the same - // anonymous data member. - vector dms_replaced_by_same_anon_dm; - // This contains the anonymous data member that replaced the - // data members in the variable above. - var_decl_sptr anonymous_data_member; - - while (i != d.ordered_data_members_replaced_by_adms().end()) + for (changed_var_sptrs_type::const_iterator i = + d.ordered_data_members_replaced_by_adms().begin(); + i != d.ordered_data_members_replaced_by_adms().end();) { - anonymous_data_member = i->second; + // This contains the data members replaced by the same + // anonymous data member. + vector dms_replaced_by_same_anon_dm; + dms_replaced_by_same_anon_dm.push_back(i->first); + // This contains the anonymous data member that replaced the + // data members in the variable above. + var_decl_sptr anonymous_data_member = i->second; // Let's look forward to see if the subsequent data // members were replaced by members of // anonymous_data_member. - for (j = i; - j != d.ordered_data_members_replaced_by_adms().end(); - ++j) - { - if (*i->second == *j->second) - dms_replaced_by_same_anon_dm.push_back(j->first); - else - break; - } + for (++i; + i != d.ordered_data_members_replaced_by_adms().end() + && *i->second == *anonymous_data_member; + ++i) + dms_replaced_by_same_anon_dm.push_back(i->first); bool several_data_members_replaced = dms_replaced_by_same_anon_dm.size() > 1; @@ -1490,10 +1485,8 @@ maybe_report_data_members_replaced_by_anon_dm(const class_or_union_diff &d, out << "replaced by anonymous data member:\n" << indent + " " << "'" - << i->second->get_pretty_representation() + << anonymous_data_member->get_pretty_representation() << "'\n"; - - i = j; } } } diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am index ea94f0bd..ff8005ed 100644 --- a/tests/data/Makefile.am +++ b/tests/data/Makefile.am @@ -851,6 +851,14 @@ test-diff-filter/test-PR25661-6-report-1.txt \ test-diff-filter/test-PR25661-6-report-2.txt \ test-diff-filter/test-PR25661-6-report-3.txt \ test-diff-filter/test-PR25661-6-report-4.txt \ +test-diff-filter/test-PR25661-7-v0.c \ +test-diff-filter/test-PR25661-7-v1.c \ +test-diff-filter/test-PR25661-7-v0.o \ +test-diff-filter/test-PR25661-7-v1.o \ +test-diff-filter/test-PR25661-7-report-1.txt \ +test-diff-filter/test-PR25661-7-report-2.txt \ +test-diff-filter/test-PR25661-7-report-3.txt \ +test-diff-filter/test-PR25661-7-report-4.txt \ \ test-diff-suppr/test0-type-suppr-v0.cc \ test-diff-suppr/test0-type-suppr-v1.cc \ diff --git a/tests/data/test-diff-filter/test-PR25661-7-report-1.txt b/tests/data/test-diff-filter/test-PR25661-7-report-1.txt new file mode 100644 index 00000000..9666a8fd --- /dev/null +++ b/tests/data/test-diff-filter/test-PR25661-7-report-1.txt @@ -0,0 +1,3 @@ +Functions changes summary: 0 Removed, 0 Changed (1 filtered out), 0 Added function +Variables changes summary: 0 Removed, 0 Changed, 0 Added variable + diff --git a/tests/data/test-diff-filter/test-PR25661-7-report-2.txt b/tests/data/test-diff-filter/test-PR25661-7-report-2.txt new file mode 100644 index 00000000..0927b7ab --- /dev/null +++ b/tests/data/test-diff-filter/test-PR25661-7-report-2.txt @@ -0,0 +1,5 @@ +Leaf changes summary: 0 artifact changed (1 filtered out) +Changed leaf types summary: 0 (1 filtered out) leaf type changed +Removed/Changed/Added functions summary: 0 Removed, 0 Changed, 0 Added function +Removed/Changed/Added variables summary: 0 Removed, 0 Changed, 0 Added variable + diff --git a/tests/data/test-diff-filter/test-PR25661-7-report-3.txt b/tests/data/test-diff-filter/test-PR25661-7-report-3.txt new file mode 100644 index 00000000..8fc7c736 --- /dev/null +++ b/tests/data/test-diff-filter/test-PR25661-7-report-3.txt @@ -0,0 +1,14 @@ +Functions changes summary: 0 Removed, 1 Changed, 0 Added function +Variables changes summary: 0 Removed, 0 Changed, 0 Added variable + +1 function with some indirect sub-type change: + + [C] 'function void foo(S*)' at test-PR25661-7-v1.c:24:1 has some indirect sub-type changes: + parameter 1 of type 'S*' has sub-type changes: + in pointed to type 'struct S' at test-PR25661-7-v1.c:1:1: + type size hasn't changed + data members 'S::a', 'S::b' were replaced by anonymous data member: + 'union {int tag1[2]; struct {int a; int b;};}' + data members 'S::c', 'S::d' were replaced by anonymous data member: + 'union {int tag2[2]; struct {int c; int d;};}' + diff --git a/tests/data/test-diff-filter/test-PR25661-7-report-4.txt b/tests/data/test-diff-filter/test-PR25661-7-report-4.txt new file mode 100644 index 00000000..be901b50 --- /dev/null +++ b/tests/data/test-diff-filter/test-PR25661-7-report-4.txt @@ -0,0 +1,11 @@ +Leaf changes summary: 1 artifact changed +Changed leaf types summary: 1 leaf type changed +Removed/Changed/Added functions summary: 0 Removed, 0 Changed, 0 Added function +Removed/Changed/Added variables summary: 0 Removed, 0 Changed, 0 Added variable + +'struct S at test-PR25661-7-v0.c:1:1' changed: + type size hasn't changed + data members 'S::a', 'S::b' were replaced by anonymous data member: + 'union {int tag1[2]; struct {int a; int b;};}' + data members 'S::c', 'S::d' were replaced by anonymous data member: + 'union {int tag2[2]; struct {int c; int d;};}' diff --git a/tests/data/test-diff-filter/test-PR25661-7-v0.c b/tests/data/test-diff-filter/test-PR25661-7-v0.c new file mode 100644 index 00000000..9c672bae --- /dev/null +++ b/tests/data/test-diff-filter/test-PR25661-7-v0.c @@ -0,0 +1,12 @@ +struct S +{ + int a; + int b; + int c; + int d; +}; + +void +foo(struct S* s __attribute__((unused))) +{ +} diff --git a/tests/data/test-diff-filter/test-PR25661-7-v0.o b/tests/data/test-diff-filter/test-PR25661-7-v0.o new file mode 100644 index 0000000000000000000000000000000000000000..3395c00bbc5d696f6cade9acec32c1c9e0710d08 GIT binary patch literal 2680 zcmbtV&1)1(5U<|Zk4Yw3H(5nP6!yWBs5oQdM@*FH`l9g@35qu*&hAdKi`iM(nP^nR zgCclS5LA5d4)5a8qj&!dFYhAAyLk(E(5jxUq|?q`v|zide^tM#>h9^b4A;&*`+V-%!?ou(KE6dbD^0eHFd%hpa5O!06-q+b0Ya7! z!YYskRt-c}s#&WJA}3YbYJkW~bpW#2Tq}$rx7g``=wGjtLyNy(W0-t|wk@P(UNoU# zyA)dtiQUDrV--h>Lt?m4+K$$;ux!UU<{WpX9Z|;ogJ9Xo@_cy;gJ%B($2s9xHbBl3 zCr8*iHbOC~J6`=YLcDxV<7IXL6t*DmJX&BsRYCayV`#z^YmijuaI|0_jzWAZ4&o?p z2X`tU4x-qdzjE-%nvrZICv=^l7R;Z%!W4|vhNHh$bV$K7OZvK9xcF?bC>_Q9(~ z5qRBT+4mYjZDnbp6)twrCUw77>jrl=nfAQfowEX|6 z{CrN7pZbQG|2AS2lj}rP$|seG>eGeu^ZWpfY5bGwG4ImJ%9kMKKhbo;S*~wt28?d- z7x3Zza^T0IKmBy1D)BOXK$XD3p{D}&3rT8%$wQw-rj!Y zwe6e`poW0+Fx3QIC0m+t;{>F$Sr-Fy4KfxLWdA_<@r%c+-{l5>Wb zE4+??^gtLUNg!Jwim{l7e7?{RA}AO_$|2g~^rBECdEOy!gE+VGDbh>8f-#RXVBkvO zTNGp)#3BdofT(d`j98N2qdkdeB@yos)w~8(A&j3JVQQ z5VJzGRY=Nx)`N;cu`#oF!m69nTCo;IZN7R8t#x6V=gb$(4O7%{d1a7BqVCii7&8Aj znC5v?8UTf3M_lL7)l#llei?Q!fFZ|BiM$YclGtaNDuQ_L(n`>^HD1bdICOJ@%OX8E9MBc$L=-T5T>~{mdW7DR{;zwTh z44jSzH{jxx>-N^#S$o5M)?KxoK{O8h&8{E#qkbDzZ`AH>9yZpU_2+D-Ydia1cq?f4 zM#Eq@4xMrEUO(tKk=Hu#L%^VG02jBm&eiw%A4y$6N zG;iF)Djz|d9vX3vUbL!ra$DuOZFETB%r{c;5dHa2mJz#=1!JElT{~izXvrFqkEw2x z$(hh$j1r%$47@ml5g?O_XKWJ6Gg#t}J2qp;*((GdM!#CB50m|uwJGS_@`z76$_w}? znM~@EBAlvOB@oAAGR6Lc*I1|gG?f3-i3-a+(-%sAjZ=)1fBL(>lfo(f2dsar;jF@U z8vZ2fzw)f-RW(xH0{hTZWm)&%_!rz}oN7wf1#Wn2fX4*qXq3!7B}M*W1a8NRJaAiK z2=2%~@Z65y8h7{l!Tu0!vFx>4Bmd?Rvp4dBE}pzJ<)9z&SKY=#d zH~*E7_gCt)OcSa6B`u+*#`iw`)SFXkEd8GR>ERE?X zYx>fCchRI{J#OsdqB^B12KxJ)`?;~zUKTNv92bSU(Ix{Wbq&44fUuZ@~n7j{*?Ke8(h#g zq{dhNUoxMUt6*p{zcE1)LDjF|&r!(gpPkV^`im&O>MPZu=cd?)oPU=KrTd=#PCB2y gNmi*p(4ugtdq2@;_X@?$;x~Bx-zXx_s_zPZ081>tR{#J2 literal 0 HcmV?d00001 diff --git a/tests/test-diff-filter.cc b/tests/test-diff-filter.cc index 3ddbdbc5..bdf7d41f 100644 --- a/tests/test-diff-filter.cc +++ b/tests/test-diff-filter.cc @@ -738,6 +738,34 @@ InOutSpec in_out_specs[] = "data/test-diff-filter/test-PR25661-6-report-4.txt", "output/test-diff-filter/test-PR25661-6-report-4.txt", }, + { + "data/test-diff-filter/test-PR25661-7-v0.o", + "data/test-diff-filter/test-PR25661-7-v1.o", + "--no-default-suppression", + "data/test-diff-filter/test-PR25661-7-report-1.txt", + "output/test-diff-filter/test-PR25661-7-report-1.txt", + }, + { + "data/test-diff-filter/test-PR25661-7-v0.o", + "data/test-diff-filter/test-PR25661-7-v1.o", + "--no-default-suppression --leaf-changes-only", + "data/test-diff-filter/test-PR25661-7-report-2.txt", + "output/test-diff-filter/test-PR25661-7-report-2.txt", + }, + { + "data/test-diff-filter/test-PR25661-7-v0.o", + "data/test-diff-filter/test-PR25661-7-v1.o", + "--no-default-suppression --harmless", + "data/test-diff-filter/test-PR25661-7-report-3.txt", + "output/test-diff-filter/test-PR25661-7-report-3.txt", + }, + { + "data/test-diff-filter/test-PR25661-7-v0.o", + "data/test-diff-filter/test-PR25661-7-v1.o", + "--no-default-suppression --harmless --leaf-changes-only", + "data/test-diff-filter/test-PR25661-7-report-4.txt", + "output/test-diff-filter/test-PR25661-7-report-4.txt", + }, // This should be the last entry {NULL, NULL, NULL, NULL, NULL} };