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 <gprocida@google.com>
---
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
@@ -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<var_decl_sptr> 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<var_decl_sptr> 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;
}
}
}
@@ -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 \
new file mode 100644
@@ -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
+
new file mode 100644
@@ -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
+
new file mode 100644
@@ -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;};}'
+
new file mode 100644
@@ -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;};}'
new file mode 100644
@@ -0,0 +1,12 @@
+struct S
+{
+ int a;
+ int b;
+ int c;
+ int d;
+};
+
+void
+foo(struct S* s __attribute__((unused)))
+{
+}
new file mode 100644
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^<M`te0WrP40
z0ViN%#VEjs+=gD2Y8A>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-%<m7~V)V(wA)g>n<pfAZm<bLruV<9ii`#{{j4QDT3tISRutxS7|
zyzz?Lj#t89W+@1RZmW(<zgut4tWHh3lZPwrQpH{LqkCb!+3kd#mB?KQ@3z8*8~e58
zAObi$JA16M=X6kO`C$^!o!Xlyvb3#o#NzK?0kK*VqXPx|Ic{br;`AetM{%lHdYYLX
z%+H}i0;i62<5~FnJ<W)lPJ{6{*AHit-9k&6Sn4x2Z5HHBNUkj?^uYUi;OUod!C(*n
zVh{YMCkETlb67iv^-O7B13e-oyt95>vrZ<?KsfDYgg}&`WCF(3QsyU}alU`%(GBUh
zu~!kvU%&TQ;if;=M*dn$f>ICv=^l7R;Z%!W4|vhNHh$bV$K7OZvK9xcF?bC>_Q9(~
z5qRBT+4mYjZDnbp6)twrCUw77>jrl=nf<OGF5!F8Da)-eU<#%Huik0518i<y*ogxV
zU#T0#e*Kmr1<i%UuHTk@QWgIj(<pN3Z23GnR8=YK-;G+EFY6D)2{T{NMdfc!CVvba
z=)QlIKY?5`CgtS(|Ik7c)0f&Z{aIBtA5!h)9}M0poW3i9_}j$ZE@cKsGbS1SmsWtg
zYUauGokxR-k*}}7aVDzS^rd~K)&E}A&wQ$%x|3Od12HouLhwR?7igi0>AQfowEX|6
z{CrN7pZbQG|2AS2lj}rP$|seG>eGeu^ZWpfY5bGwG4ImJ%9kMKKhbo;S*~wt28?d-
z7<q#V#WLM*DqxBo)y(^6_$4Z7=eL$}e$@S$&*z$!n_|CK`RCO}sZZ0tipi%tG2b3S
d@I`^#uL&dJb4yl9|4-wWR8#(!9o1~|e*g&qr{4eo
literal 0
HcmV?d00001
new file mode 100644
@@ -0,0 +1,26 @@
+struct S
+{
+ union
+ {
+ int tag1[2];
+ struct
+ {
+ int a;
+ int b;
+ };
+ };
+ union
+ {
+ int tag2[2];
+ struct
+ {
+ int c;
+ int d;
+ };
+ };
+};
+
+void
+foo(struct S* s __attribute__((unused)))
+{
+}
new file mode 100644
GIT binary patch
literal 2976
zcmbtVL2nyX5T55}C+n=ec5F}+1*9zk+5)>x3Zza^T0<ahC{$5P1s90c_Qtk|*Ohl&
zQV=QvRH?TjAt5AAAaUj|pkBCggBu)>IKmBy1D)BOXK$XD3p{D}&3rT8%$wQw-rj!Y
zwe6e`poW0+Fx3<a@XOp(ZpLC0>QIC0m+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&j3<EWJTMiQ*Vg-X9J@EKS;?>JVQQ
z5VJzGRY=Nx)`N;cu`#oF!m69nTCo;IZN7R8t#x6V=gb$(4O7%{d1a7BqVCii7&8Aj
znC5v?8UTf3M_lL7)l#llei?Q!fFZ<Aogx*FqkbAAEN}!1OQ{QKq|=p%L}|*dxUj4;
zpID~VHJ844X_+Efh18mIO8}J$u8EStGx+4FGVDiss9vNolDF}Z@D#p<_}ePuJKkj2
zB>|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&e<nl_FH`~NTNE8Q;7iZP`q-i>iw%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<ih?P?Z4U<nAMXJ-7)HK}
z=iLb-ul+8Q{NCRF$Q$^)@c%ijAfI}r`l-S-S(xlU*2y<Lj-x`DeiARIol>&SKY=#d
zH~*E7_gCt)OcSa6B`u+*#`iw`)SFXkEd8GR>ERE?X<ref_Bj4L;<1TuJti66Wbnt>
zYx>fCchRI{<b9Gi<LZ<eOS&)BnYI3(_)sW5t)Ff=ef=H8^q2_2Tb%z@Qc^?r-9uYe
zeuFoZ>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
@@ -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}
};