diff mbox series

Fix maybe_report_data_members_replaced_by_anon_dm

Message ID 20200722105538.2543928-1-gprocida@google.com
State New
Headers show
Series Fix maybe_report_data_members_replaced_by_anon_dm | expand

Commit Message

Giuliano Procida July 22, 2020, 10:55 a.m. UTC
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 <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
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}
 };