From 4e07799f42537c1d1aba02ce8c85216e4e4749c7 Mon Sep 17 00:00:00 2001 From: Dodji Seketeli Date: Mon, 13 Oct 2014 16:27:28 +0200 Subject: [PATCH] Properly propagate {REDUNDANT, SUPPRESSED}_CATEGORY wrt local changes * src/abg-comparison.cc (suppression_categorization_visitor::visit_end): If a diff node carries local changes, then, even if all of its children node have been suppressed, this diff node shall not be categorized as suppressed by way of propagation. (redundancy_marking_visitor::visit_end): If a diff node carries local changes, then, even if all of its children nodes are redundant, this diff node shall not be categorized as being redundant by way of propagation. * tests/data/test-diff-suppr/libtest4-local-suppr-v{0,1}.so: New test inputs. * tests/data/test-diff-suppr/test4-local-suppr-0.suppr: Likewise. * tests/data/test-diff-suppr/test4-local-suppr-report-{0,1}.txt: Likewise. * tests/data/test-diff-suppr/test4-local-suppr-v{0,1}.{c,h}: Source code of the new tests inputs. * tests/Makefile.am: Add the new test material to the source distribution. * tests/test-diff-suppr.cc (in_out_spec): Run this test harness over the new test input above. Signed-off-by: Dodji Seketeli --- src/abg-comparison.cc | 50 ++++++++++-------- tests/Makefile.am | 9 ++++ .../libtest4-local-suppr-v0.so | Bin 0 -> 9062 bytes .../libtest4-local-suppr-v1.so | Bin 0 -> 9158 bytes .../test-diff-suppr/test4-local-suppr-0.suppr | 3 ++ .../test4-local-suppr-report-0.txt | 25 +++++++++ .../test4-local-suppr-report-1.txt | 20 +++++++ .../test-diff-suppr/test4-local-suppr-v0.c | 16 ++++++ .../test-diff-suppr/test4-local-suppr-v0.h | 13 +++++ .../test-diff-suppr/test4-local-suppr-v1.c | 18 +++++++ .../test-diff-suppr/test4-local-suppr-v1.h | 16 ++++++ tests/test-diff-suppr.cc | 16 ++++++ 12 files changed, 163 insertions(+), 23 deletions(-) create mode 100755 tests/data/test-diff-suppr/libtest4-local-suppr-v0.so create mode 100755 tests/data/test-diff-suppr/libtest4-local-suppr-v1.so create mode 100644 tests/data/test-diff-suppr/test4-local-suppr-0.suppr create mode 100644 tests/data/test-diff-suppr/test4-local-suppr-report-0.txt create mode 100644 tests/data/test-diff-suppr/test4-local-suppr-report-1.txt create mode 100644 tests/data/test-diff-suppr/test4-local-suppr-v0.c create mode 100644 tests/data/test-diff-suppr/test4-local-suppr-v0.h create mode 100644 tests/data/test-diff-suppr/test4-local-suppr-v1.c create mode 100644 tests/data/test-diff-suppr/test4-local-suppr-v1.h diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc index 0183a6a9..b00147ab 100644 --- a/src/abg-comparison.cc +++ b/src/abg-comparison.cc @@ -8983,7 +8983,8 @@ struct suppression_categorization_visitor : public diff_node_visitor bool has_non_empty_child = false; bool has_suppressed_child = false; - if (!(d->get_category() & SUPPRESSED_CATEGORY)) + if (!(d->get_category() & SUPPRESSED_CATEGORY) + && !d->has_local_changes()) { for (vector::const_iterator i = d->children_nodes().begin(); i != d->children_nodes().end(); @@ -9205,32 +9206,35 @@ struct redundancy_marking_visitor : public diff_node_visitor virtual void visit_end(diff* d) { - bool has_non_redundant_child = false; - bool has_non_empty_child = false; - for (vector::const_iterator i = d->children_nodes().begin(); - i != d->children_nodes().end(); - ++i) + // Propagate the redundancy categorization of the children nodes + // to this node. But if this node has local changes, then it + // doesn't inherit redundancy from its children nodes. + if (!(d->get_category() & REDUNDANT_CATEGORY) + && !d->has_local_changes()) { - if ((*i)->length()) + bool has_non_redundant_child = false; + bool has_non_empty_child = false; + for (vector::const_iterator i = d->children_nodes().begin(); + i != d->children_nodes().end(); + ++i) { - has_non_empty_child = true; - if (((*i)->get_category() & REDUNDANT_CATEGORY) == 0) - has_non_redundant_child = true; + if ((*i)->length()) + { + has_non_empty_child = true; + if (((*i)->get_category() & REDUNDANT_CATEGORY) == 0) + has_non_redundant_child = true; + } + if (has_non_redundant_child) + break; } - if (has_non_redundant_child) - break; - } - // A diff node for which at least child node carries a change, and - // for which all the children are redundant is deemed redundant too. - // - // TODO: xxx when, for a given node, we can tell the difference - // between local changes and changes coming from children nodes, - // we'll have to alter the condition above: if the current node - // has local changes, even if all its children are redundant, - // won't inherit redundancy from its children. - if (has_non_empty_child && !has_non_redundant_child) - d->add_to_category(REDUNDANT_CATEGORY); + // A diff node for which at least a child node carries a + // change, and for which all the children are redundant is + // deemed redundant too, unless it has local changes. + if (has_non_empty_child + && !has_non_redundant_child) + d->add_to_category(REDUNDANT_CATEGORY); + } } virtual void diff --git a/tests/Makefile.am b/tests/Makefile.am index 9a070356..3929f503 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -388,6 +388,15 @@ tests/data/test-diff-suppr/test3-struct-suppr-v0.cc \ tests/data/test-diff-suppr/test3-struct-suppr-v0.o \ tests/data/test-diff-suppr/test3-struct-suppr-v1.cc \ tests/data/test-diff-suppr/test3-struct-suppr-v1.o \ +tests/data/test-diff-suppr/libtest4-local-suppr-v0.so \ +tests/data/test-diff-suppr/libtest4-local-suppr-v1.so \ +tests/data/test-diff-suppr/test4-local-suppr-0.suppr \ +tests/data/test-diff-suppr/test4-local-suppr-report-0.txt \ +tests/data/test-diff-suppr/test4-local-suppr-report-1.txt \ +tests/data/test-diff-suppr/test4-local-suppr-v0.c \ +tests/data/test-diff-suppr/test4-local-suppr-v0.h \ +tests/data/test-diff-suppr/test4-local-suppr-v1.c \ +tests/data/test-diff-suppr/test4-local-suppr-v1.h \ \ data/test-lookup-syms/test0.cc \ data/test-lookup-syms/test0.o \ diff --git a/tests/data/test-diff-suppr/libtest4-local-suppr-v0.so b/tests/data/test-diff-suppr/libtest4-local-suppr-v0.so new file mode 100755 index 0000000000000000000000000000000000000000..d82cd6148295e1db1756c75f6412e6c75c9e134e GIT binary patch literal 9062 zcmeHMU2Ggz6~43UpLNz=Z|o#EKPZzbp-F|uPMRhr!HFHm&N_0^;5sTrSku|vv3INW zuGyV+?Wlx6N=XWl0unSppDIKv9(Zn{0yjY+4@d-Pr3eWrq68ACL{da$1d932oqN_Z zv+D*4i5GlVGv}V~{M~!!?*0A3;K)!&(*&omI3S3-SnnczO3<@LWkC8wk7&eO7ax_n zN_9!WlWT5t8KweOlqg06o`n|Ti0VGt9g?i*QIze7CA*H8EANQuGQ)J993wG}xU8rH zrRSQG2kBO&$CPv_ow((sDgQMJT7zzKVZoH`J_S2vqBK67a5+a+yye;@qS}M0s*bE1 zeWEa)BgapQ-Ysow4y^j)SMHrVy>I=E@4tB}{PUX-NjZHIZvS(abxqXuH4(N7PcxoF zgYUd|u5>Q?<9EOD+>OtDdea*h?#2G{o4=|Shr%)`ESf}Lpfgar=r!~yel6Tu2Jcx0 z9|ispo?to)ATHL5?Qz{FDuZhiQ8BGP4p-`1Qh%N34(PwA@SvZ#^iy5mbrNqDXM!K! zG$*GDd9&nLMaMLSnayV%VNQSub9n5SnYN4eWVYnk#j#@pxkBC^vywU6mDQG*sacC0 zSh?&eC?1_LN0oB{5Mbh+H@prh;XNsYUb6Ia1;o!GOZBHz-jHn)A|$@+vX%ca_vb!t2lYQ?Tqryqz?r|%s!NZg zRv7}2zv;oLuXDQX!TCQ(vb!GK+cg(FI2V!hJJk25|AW`R?!mqKiL>uT5?`(V<|L-`)tr#Mz%ku8=n&79V>BqQ!0B!@G5JA9BQJNOb4sBGR_+5Kn)HJJ-P`F4SKF zkhswMBFM?aj)lbeTZyv^v1dc$R}g3`2@&Z1@x`0u5Bip?{~3?|TMOaD`OAs7{(LZ@ zT~D09n)ucIvb10OuO_npHQE23?9cRWft@HnoH*P2C&&=;{4J*$@!xOM#qDtv>2?OM z@vHR<5Nm(CGC|2`-TXX?EKt?nc_<2JGYa>TbUyF2pM?Dn0K?bf_3=AT!oi(Uye2M^ z`^5RrqgLg+<5wmw%Jx&urELuF?|@JG-f!PuT-*)%2UvX;l&2r<)Tq#Ahpq z+E&PDjJX8;Zcq`655+o;wnm?goDl~%?0@9Zt#WuEox^y}z+a5?`eX4ILIW)g!*E3W zN?K`wl@?fOft40mX@UQP7O1Styo(vso7Z2oAfZHSJxa7Mpv3EYwQZ}cbJx4#%KDF% zL6mrI-QcpApLLm|`Ir*R|9*e5K>S@5nAd6 z+jtEr*9egFdVXZI)c?W1;DPgstN zclPWMABQuyz1>y|?zMukE(36zC8B*1d959#M-oPTHD+^Q?Y9y>aD?&}Ku%WI#n|DMbZI!gf^H0NbCvtTW8*ibl*0#mkqv6=b*lMj!i=#({+O=>j8r>J&AEi2lTSPcU z%y4@T`fFXQh$4&P0FNx;4O@X9h=$R4jfQq`y(@coJ$dcxca7vL2yLx#e(Ul^&v)Rc`5Z=~&Hc@jRX{3K2a6r0UYki9EP zCX4nAbGdBZ<~$n4?IQoho8|`^V|iR&?mUh|s9}>?VIcPb#!$BBd4uUEl_rm`?nqQI z#|fs8a|YYN3cE53ru;we}sp2s!O2~QbpH}k}Q;yFDag||uI$+P|-AwPYVZe^z=L7a< zR7IHb`hfL=<)?E+DnEa};Q8_ z@;`)HtH++#bE-zr@IRTvySuk5dSoz{re zyo~+P)vh7^zr86KYr``3*Oa|n(s>*N?LVgMgZ1oebB*ZxdK23-r7-;Myn2mT+fmxd ziY{B#RwU-JL#as8qXv#?H6CsT+J%Sv1CQS{KGll<+7{_X{Nw@gU9H;hFiOkwxmP2M z67&4*$HRi>T|ZtYcs}*x^@8U~Ki(jC{`2FFmHEw&M=JY0KVE&WL-H_YGM-=j`q9dL zsUNR#4#xN@K`s4tRrhUV7q0BCy&lJz zw$!ig53d8SRWFKD3;(6`6B7|W=m1aYcX(>G1IcRnxg+(f`_%^$ukKgW@LG*~*(eH1 zc(7k>0j|R)n0kTNy5BtvybY&Sybfc#qs#btO6phN2aikt)%U?kh4XyN{`1TD|I#w} z%fKIEV-I~Dcw3Eobdf4LC8vy=w-j!0j}DBQBg0RO;UanI-q>`e%oN=mmjquJdmz(H z7tG0AA!+5zv{NXSOshOAupOMv*^Zqy_UwFgFFe;!l9$x(ZWH&#PH|4S`_puJYHAK9 z9!}nB`&5@XdoazR(f(tD=HT(ebQvxy#QAioU}mg*nr^=jKXttS*zf=}a4XK*M@Ak$ z)IVZAJ~Z^i;Fvkqe`sV7K0fRcUEO@Ux9$Fmcslrz*WkW;@fw%uG5|NM|Bl{w6W`b8 Ky-;`W`Tq?_4<3;K literal 0 HcmV?d00001 diff --git a/tests/data/test-diff-suppr/libtest4-local-suppr-v1.so b/tests/data/test-diff-suppr/libtest4-local-suppr-v1.so new file mode 100755 index 0000000000000000000000000000000000000000..9a81027df1ba6f0824ec7da944870921c8fe176f GIT binary patch literal 9158 zcmeGiZEPGz_3ho+KKGpSC2oTAfwGAbnjpNk(=;&+ZtOTt&XJP_*HH;zZTIfhcN?AW zj=MeAj-W(1RB<6vK!O_RuL>xt0EvI)L&a@S#SbJxp;Cl|6j34)NYykzc^hwBigpyuo+~Ie|RZ(HMNQSX6j5;Vp z5YqEY$@jw@QjQ_XQaO3u&QSf!#v5|!~_D7z#2 zMxQ8*&(Ra7U?!LU)ZDgPTYk{-^41^Sdh5l%FKmCET0ngIBpm+>uc`_}23m=BA3kmP z92$P()`iN2*iYa5>SwQh<`Y|feDR(5Uw`|yj5rE3C}C)Y0pDPtxTpd0S^-4q{M}ABQ{jbs@h2dVKQFNV?xnLfEPA?*>6{gLD3m zuj|>VVnMIiM%mVN(5-@HgFcA>^pWx7dd4i9S*v23YZW75J<-l$Z}3QQJ@khfB4ty0wXfo^6D+W=ipRVq$9kW|RP(b17ZL;4?4N>=_H2H~?Wb-fb7%Jv5hGbZ#QacgpWizX!K{)kSy9C+BWOlV583+I=b{ zzqn{O-PnwZ$+=%dFOxNZ<%ga}*7DA8e`P70oPRa>%3ls9 zl`F~l*OI@vTNV22|J6$Ruao{CN&j5`Hq?Xa1IfAmKO+xa&R@6N(Eptl6>dymlWyej zHSt>060()QU7n<(wQoI-O$O|0{~`*4y%ihxtT4W4cbr1~7mjqj9K+Y- zB{H9!{~Y$JdVAvX6ml1oJ_*FnY=Hgy$Y2V9Do_UQz(C+Qp??B z-o`nk$Ptjw`2^*8R6gJUNJz&_KPi(*Zs($u!=zUf_J`?*Xn&8C$23FW9LL)dPe}fI z60fyW+WTJv-Z%UGPRlqvE#c9jp@-F7xU<}??$!2dd(=HWy?c6ldp@e}8Z$F$(y$$R zSKn^<2pV(Z?L^JL*Yf+ig9IlmQRTZ>S5$nR!G{%};;)ai&|COQghI6O3-2dFgsKpJ zkLt}&;X4YEi1>b?LBbS444=Td68#RoAxs&i()xXTN1!Qs8exbAbTrgT{ALS6F(KG| z210F)SWBeID%cQFnm_ANEz-F$2aGa2ljy@dUksT0=J%S2oq8+Ov zDAJ4~(G`&vDS~m2YRbWl4)1}D|ZCq%>7z$XMpsY%K|<7@CO&Vz8| zDqx!xdv901m^SiVm1?O}?wan^(kL3t$q@%Vgg7wacI9Uw+(#|xzVkB8L@-MQkF*_|n7&RE@9%kIVvu#vK| zhL!IYJ?N%BRl3QJsCH$n$w{YkbaJ8&-E)AeT~l_oVD@Ltf?2lGh#KW|u79R)U)R39 zYF8FY)l}X}I~}8@>Hu10cFHzV2<@^1b6mq%m|C__)lyX}pXstPAh4WK$$^%cEuf|Y z?Xts}Hp>;OSnwb^^2%o3AO%?~qv6_Od7=~T4Y}<7e!l)DJzZsi)a;Jns9~33ZSKnQ&T2>Vl88)s#&yQ6tZ{{AyKO^ zN%{^el`5OljODF@$#o2jGmid?H?2Q32k^Y#=)8|z$Z45T(6II{&e5#T>k>nn1gP-* z&Fh0h1w0`Iu_m!TuUicJ`iYNRCAXbob0UUwMs zdd%%-IfkcDpVl9{&^3Cm+WXNEJ`$n{rgt*|M!0O}2{VyQjtOYU~Xk0}M^{7z<9Z-Aw@t-5dt4`Ze|!d@V+)k z{(j(j%`3kiw|GJF8|v=!NH1L5y}PXtokyde&ZiZAx<4y$_+jAF48Od86?NvE2)yI} zd5ZHB1?uBQEeT_DCxDtMlv2psK82m_~$h#==I<6qO>z0*L7(I zz9Ho6$HPlV59$~B38Md6*oi|_Y{U_f%~1-%7x^qQENw_r5V~ zDAE1nY8%p3)bW>oqzBy}A4d8*{FjyyJw7u75G(z-pRr|D+80(>Fu zBI$ajsAu!Vl#$mncClR1jp_{GcCeH;Z8M|o>wR!PS`H8rm*CEB6Yrz#@+>&}(@b@0 zY8F*on7B3fh&DK&(Dfr@gU5&U;S-1HvR!nD>zPVX&l!ab-L4-#ePZzV$Ph~4tvgE} z9ewoB;HduSkt2@{kL%-uhen6d#`|5;%bRuQHs5>EPX|Tfn%#5n9&lMN9B^R0{{!H; O=^q$yU)VeUNAPb205rJ( literal 0 HcmV?d00001 diff --git a/tests/data/test-diff-suppr/test4-local-suppr-0.suppr b/tests/data/test-diff-suppr/test4-local-suppr-0.suppr new file mode 100644 index 00000000..b7c6271b --- /dev/null +++ b/tests/data/test-diff-suppr/test4-local-suppr-0.suppr @@ -0,0 +1,3 @@ +[suppress_type] + # Types whose name start with "private" should not be flagged + name_regexp = ^private.* diff --git a/tests/data/test-diff-suppr/test4-local-suppr-report-0.txt b/tests/data/test-diff-suppr/test4-local-suppr-report-0.txt new file mode 100644 index 00000000..b707701a --- /dev/null +++ b/tests/data/test-diff-suppr/test4-local-suppr-report-0.txt @@ -0,0 +1,25 @@ +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(public_type*, a_not_private_type*)' has some indirect sub-type changes: + parameter 0 of type 'public_type*' has sub-type changes: + in pointed to type 'struct public_type': + size changed from 64 to 128 bits + 1 data member insertion: + 'unsigned int public_type::oops', at offset 0 (in bits) + 1 data member change: + 'private_data* public_type::priv_' offset changed from 0 to 64 + and its type 'private_data*' changed: + in pointed to type 'struct private_data': + size changed from 32 to 64 bits + 1 data member insertion: + 'char private_data::private_data1', at offset 32 (in bits) + + parameter 1 of type 'a_not_private_type*' has sub-type changes: + in pointed to type 'struct a_not_private_type': + size changed from 32 to 64 bits + 1 data member insertion: + 'char a_not_private_type::j', at offset 32 (in bits) + diff --git a/tests/data/test-diff-suppr/test4-local-suppr-report-1.txt b/tests/data/test-diff-suppr/test4-local-suppr-report-1.txt new file mode 100644 index 00000000..6a6ccd3d --- /dev/null +++ b/tests/data/test-diff-suppr/test4-local-suppr-report-1.txt @@ -0,0 +1,20 @@ +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(public_type*, a_not_private_type*)' has some indirect sub-type changes: + parameter 0 of type 'public_type*' has sub-type changes: + in pointed to type 'struct public_type': + size changed from 64 to 128 bits + 1 data member insertion: + 'unsigned int public_type::oops', at offset 0 (in bits) + 1 data member change: + 'private_data* public_type::priv_' offset changed from 0 to 64 + + parameter 1 of type 'a_not_private_type*' has sub-type changes: + in pointed to type 'struct a_not_private_type': + size changed from 32 to 64 bits + 1 data member insertion: + 'char a_not_private_type::j', at offset 32 (in bits) + diff --git a/tests/data/test-diff-suppr/test4-local-suppr-v0.c b/tests/data/test-diff-suppr/test4-local-suppr-v0.c new file mode 100644 index 00000000..005ac397 --- /dev/null +++ b/tests/data/test-diff-suppr/test4-local-suppr-v0.c @@ -0,0 +1,16 @@ +// To compile this, type: +// gcc -shared -g -Wall -o libtest4-local-suppr-v0.so test4-local-suppr-v0.c + +#include "test4-local-suppr-v0.h" + +struct private_data +{ + int private_data0; +}; + +void +foo(struct public_type* p __attribute__((unused)), + struct a_not_private_type* t __attribute__((unused))) +{ + /* Do something with p */ +} diff --git a/tests/data/test-diff-suppr/test4-local-suppr-v0.h b/tests/data/test-diff-suppr/test4-local-suppr-v0.h new file mode 100644 index 00000000..f2efbe57 --- /dev/null +++ b/tests/data/test-diff-suppr/test4-local-suppr-v0.h @@ -0,0 +1,13 @@ +struct private_opaque_data; +struct public_type +{ + struct private_data* priv_; +}; + +struct a_not_private_type +{ + int i; +}; + +void +foo(struct public_type* p, struct a_not_private_type* t); diff --git a/tests/data/test-diff-suppr/test4-local-suppr-v1.c b/tests/data/test-diff-suppr/test4-local-suppr-v1.c new file mode 100644 index 00000000..f096fe2e --- /dev/null +++ b/tests/data/test-diff-suppr/test4-local-suppr-v1.c @@ -0,0 +1,18 @@ +// To compile this, type: +// gcc -shared -g -Wall -o libtest4-local-suppr-v0.so test4-local-suppr-v0.c + +#include "test4-local-suppr-v1.h" + +struct private_data +{ + int private_data0; + char private_data1; // This new member should not be flagged when + // using the suppression list. +}; + +void +foo(struct public_type* p __attribute__((unused)), + struct a_not_private_type* t __attribute__((unused))) +{ + /* Do something with p */ +} diff --git a/tests/data/test-diff-suppr/test4-local-suppr-v1.h b/tests/data/test-diff-suppr/test4-local-suppr-v1.h new file mode 100644 index 00000000..88ea90e1 --- /dev/null +++ b/tests/data/test-diff-suppr/test4-local-suppr-v1.h @@ -0,0 +1,16 @@ +struct private_opaque_data; +struct public_type +{ + unsigned oops; // <--- we accidentally added a member here. This + // breaks ABI. + struct private_data* priv_; +}; + +struct a_not_private_type +{ + int i; + char j; // <-- This added member should be flagged too. +}; + +void +foo(struct public_type* p, struct a_not_private_type* t); diff --git a/tests/test-diff-suppr.cc b/tests/test-diff-suppr.cc index ed4d31e8..5b8b8af1 100644 --- a/tests/test-diff-suppr.cc +++ b/tests/test-diff-suppr.cc @@ -151,6 +151,22 @@ InOutSpec in_out_specs[] = "data/test-diff-suppr/test3-struct-suppr-report-2.txt", "output/test-diff-suppr/test3-struct-suppr-report-2.txt", }, + { + "data/test-diff-suppr/libtest4-local-suppr-v0.so", + "data/test-diff-suppr/libtest4-local-suppr-v1.so", + "data/test-diff-suppr/test4-local-suppr-0.suppr", + "", + "data/test-diff-suppr/test4-local-suppr-report-1.txt", + "output/test-diff-suppr/test4-local-suppr-report-1.txt", + }, + { + "data/test-diff-suppr/libtest4-local-suppr-v0.so", + "data/test-diff-suppr/libtest4-local-suppr-v1.so", + "", + "", + "data/test-diff-suppr/test4-local-suppr-report-0.txt", + "output/test-diff-suppr/test4-local-suppr-report-0.txt", + }, // This should be the last entry {NULL, NULL, NULL, NULL, NULL, NULL} };