From patchwork Thu Jul 23 08:35:42 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Giuliano Procida X-Patchwork-Id: 40152 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 BC4C0384240C; Thu, 23 Jul 2020 08:35:53 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org BC4C0384240C DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=sourceware.org; s=default; t=1595493353; bh=2F718fqKKfcqAck2roEpc84HaJVktWsnjdUscQ8Vaz4=; h=Date:In-Reply-To:References:Subject:To:List-Id:List-Unsubscribe: List-Archive:List-Help:List-Subscribe:From:Reply-To:Cc:From; b=J8faxK04WNKDyHOI910XdAeZZmFF4MAyXq/AGACAti3u99CRv8gN0+sBCVzV5LPKx mrTTldBQvrI+CNXxjmeBdVjvnMVx2mpnUpVwB5y7pa6ljbfmq2+nYrCbQNB+L0wdQP c/ldztaxtYUsXobilTp1BngOd1fB/k3jh6cKAHGE= X-Original-To: libabigail@sourceware.org Delivered-To: libabigail@sourceware.org Received: from mail-wr1-x44a.google.com (mail-wr1-x44a.google.com [IPv6:2a00:1450:4864:20::44a]) by sourceware.org (Postfix) with ESMTPS id A5E50384240C for ; Thu, 23 Jul 2020 08:35:50 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org A5E50384240C Received: by mail-wr1-x44a.google.com with SMTP id m7so1150459wrb.20 for ; Thu, 23 Jul 2020 01:35:50 -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:in-reply-to:message-id:mime-version :references:subject:from:to:cc; bh=2F718fqKKfcqAck2roEpc84HaJVktWsnjdUscQ8Vaz4=; b=Ahd/181srt0UOJeOjR2H0a/yKL8quT0ZzD5nh5cNDddcbFpit1ZELZdTgmrw8kLXVw mglUeGWnDS0PmdvuqfeQCgOZaYBotVhdkCieAt/Dt0WPxVJCVKJTvnSaGVnQKVALR4NV Cy3KibAuHbwCxq3Kd3qO+E9cTg57qlDck2VL+Hwl+oi/76qGJ7rQSs13tK0iWjkMBZFT VH/Tov+nSCuG0fvVrDinxmbau0PhgNXY3ZGEla6NCdxhFUq5AQQYIvhqrqSEO8d8i1vR WPXjcMeCHsDP1/1oxCzlBbi7+jGfQJDOg2vpBrAwvTc0FNymuLh51kazcHSxngHfKeBw nPnA== X-Gm-Message-State: AOAM530frDfbUq/BWsQneqqQEAIMj7Yt8TLgyYjFXin9Tyv+rDVrf+ZE GnhHJ63SGNShMpNpuMpA7MPCxr9f+P1Jp4KrfJSTW2Lu+ncxwZk3/GGe7s44NsnzeelGkRmZ18t Tko3dW0ET+5OhkC2JIZAQzfR1GOKIUTIl/nPFFjbcerBLoWAp/tMQg2at5ISKAV9q2OGi578= X-Google-Smtp-Source: ABdhPJx9b4Wrr1cq43xElqxML3JGzF2vESNuj2pZsjKZaN7+wsMkBJ0qz6zWMgTfmF8x1hfhxWBzTufDiPbiDA== X-Received: by 2002:a1c:cc09:: with SMTP id h9mr1280310wmb.1.1595493349035; Thu, 23 Jul 2020 01:35:49 -0700 (PDT) Date: Thu, 23 Jul 2020 09:35:42 +0100 In-Reply-To: <20200722105538.2543928-1-gprocida@google.com> Message-Id: <20200723083543.3143442-1-gprocida@google.com> Mime-Version: 1.0 References: <20200722105538.2543928-1-gprocida@google.com> X-Mailer: git-send-email 2.28.0.rc0.105.gf9edc3c819-goog Subject: [PATCH v2] Fix maybe_report_data_members_replaced_by_anon_dm To: libabigail@sourceware.org X-Spam-Status: No, score=-22.7 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 a bug and 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 were: - dms_replaced_by_same_anon_dm declared outside loop, gets reused - self-comparison of first decl, potential infinite loop - anonymous_data_member, assigned but not used - two iterators i, j used, when one would suffice The first issue results in incorrect diff reports if data members in a structure are replaced by more than one anonymous data member. This commit adds additional test cases for this, following the pattern used for the existing PR25661 ones. The second issue only affects behaviour if equality is defined inconsistently with object identity. * 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. * tests/data/Makefile.am: Add new test cases. * tests/data/test-diff-filter/test-PR25661-7-report-1.txt: New test case file. * tests/data/test-diff-filter/test-PR25661-7-report-2.txt: Likewise. * tests/data/test-diff-filter/test-PR25661-7-report-3.txt: Likewise. * tests/data/test-diff-filter/test-PR25661-7-report-4.txt: Likewise. * tests/data/test-diff-filter/test-PR25661-7-v0.c: Likewise. * tests/data/test-diff-filter/test-PR25661-7-v0.o: Likewise. * tests/data/test-diff-filter/test-PR25661-7-v1.c: Likewise. * tests/data/test-diff-filter/test-PR25661-7-v1.o: Likewise. * tests/test-diff-filter.cc: Call new test cases. 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} };