diff mbox series

[v2] Fix maybe_report_data_members_replaced_by_anon_dm

Message ID 20200723083543.3143442-1-gprocida@google.com
State New
Headers show
Series [v2] Fix maybe_report_data_members_replaced_by_anon_dm | expand

Commit Message

Giuliano Procida July 23, 2020, 8:35 a.m. UTC
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

Comments

Dodji Seketeli Aug. 4, 2020, 4:28 p.m. UTC | #1
Giuliano Procida <gprocida@google.com> a écrit:

[...]

>
> 	* 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>

Applied to master, thanks!

Cheers,
diff mbox series

Patch

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<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;
 	}
     }
 }
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^<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

diff --git a/tests/data/test-diff-filter/test-PR25661-7-v1.c b/tests/data/test-diff-filter/test-PR25661-7-v1.c
new file mode 100644
index 00000000..445abb71
--- /dev/null
+++ b/tests/data/test-diff-filter/test-PR25661-7-v1.c
@@ -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)))
+{
+}
diff --git a/tests/data/test-diff-filter/test-PR25661-7-v1.o b/tests/data/test-diff-filter/test-PR25661-7-v1.o
new file mode 100644
index 0000000000000000000000000000000000000000..24ce1d5bfb381076e0b8726d27d14b62d4d9273d
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

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}
 };