From ef9d20c97cffb214db3108303d0f44aa203c6672 Mon Sep 17 00:00:00 2001 From: Dodji Seketeli Date: Thu, 28 Jun 2018 10:11:18 +0200 Subject: [PATCH] Fix redundancy detection through fn ptr and typedef paths When analyzing the libfreetype.so binary, it appears that libabigail's diff node redundancy marking pass is failing to detect a redundant diff node in cases were the node is recursively referencing itself through a path that involves function type and typedef diff nodes. So it is only at reporting time that we'd detect that the node is redundant so we emit messages like "this change was reported earlier". But When the earlier change in question is suppressed due to, e.g, a suppression specification resulting from the user providing abidiff with the --headers-dir{1,2} command line option, then the change report becomes confusing, at best. The right behaviour is to detect the node is redundant and mark it as such, so that the reporting pass can avoid reporting it altogether. This is what this patch does. This patch changes the output of the runtestdiffpkg regression test. To update the reference output, we need an additional patch to handle a separate (but somewhat related) issue. That is going to be done in the subsequent commit which title is: "Filter out changes like type to const type" * include/abg-comparison.h (is_function_type_diff_with_local_changes) (is_reference_or_pointer_diff_to_non_basic_distinct_types) (peel_typedef_diff): Declare new functions. * src/abg-comparison.cc (is_function_type_diff_with_local_changes) (is_reference_or_ptr_diff_to_non_basic_nor_distinct_types) (peel_typedef_diff): Define new functions. (is_reference_or_pointer_diff): Peel typedefs before operating. (redundancy_marking_visitor::visit_begin): Only sibbling parameter diff node that carry basic type changes (or distinct type changes) are *not* marked as redundant. All other kinds of sibbling parameter diff nodes are markes redundant. Also, rather than never marking function type diffs as redundant by fear of missing local changes on these, just avoid marking function type diff nodes with local changes. It's possible to be that precise now that we can detect that a diff node carries local changes. * tests/data/test-diff-suppr/test37-opaque-type-v{0,1}.o: New binary tests input. * tests/data/test-diff-suppr/test37-opaque-type-v{0,1}.c: Source code of the binary tests input above. * tests/data/test-diff-suppr/test37-opaque-type-header-dir/test37-opaque-type-header-v{0,1}.h: Headers of the binary tests input above. * tests/data/test-diff-suppr/test37-opaque-type-report-0.txt: Reference output for this new test. * tests/data/Makefile.am: Add the new test material above to source distribution. * tests/test-diff-suppr.cc (in_out_specs): Add the new test input above to the test harness. Signed-off-by: Dodji Seketeli --- include/abg-comparison.h | 9 ++ src/abg-comparison.cc | 98 ++++++++++++++++-- tests/data/Makefile.am | 7 ++ .../test37-opaque-type-header-v0.h | 24 +++++ .../test37-opaque-type-header-v1.h | 24 +++++ .../test37-opaque-type-report-0.txt | 3 + .../test-diff-suppr/test37-opaque-type-v0.c | 22 ++++ .../test-diff-suppr/test37-opaque-type-v0.o | Bin 0 -> 3376 bytes .../test-diff-suppr/test37-opaque-type-v1.c | 24 +++++ .../test-diff-suppr/test37-opaque-type-v1.o | Bin 0 -> 3432 bytes tests/test-diff-suppr.cc | 10 ++ 11 files changed, 212 insertions(+), 9 deletions(-) create mode 100644 tests/data/test-diff-suppr/test37-opaque-type-header-dir/test37-opaque-type-header-v0.h create mode 100644 tests/data/test-diff-suppr/test37-opaque-type-header-dir/test37-opaque-type-header-v1.h create mode 100644 tests/data/test-diff-suppr/test37-opaque-type-report-0.txt create mode 100644 tests/data/test-diff-suppr/test37-opaque-type-v0.c create mode 100644 tests/data/test-diff-suppr/test37-opaque-type-v0.o create mode 100644 tests/data/test-diff-suppr/test37-opaque-type-v1.c create mode 100644 tests/data/test-diff-suppr/test37-opaque-type-v1.o diff --git a/include/abg-comparison.h b/include/abg-comparison.h index c6a6a1e8..a7b9be9e 100644 --- a/include/abg-comparison.h +++ b/include/abg-comparison.h @@ -2682,6 +2682,9 @@ is_array_diff(const diff* diff); const function_type_diff* is_function_type_diff(const diff* diff); +const function_type_diff* +is_function_type_diff_with_local_changes(const diff* diff); + const typedef_diff* is_typedef_diff(const diff *diff); @@ -2702,6 +2705,9 @@ is_qualified_type_diff(const diff* diff); bool is_reference_or_pointer_diff(const diff* diff); +bool +is_reference_or_pointer_diff_to_non_basic_distinct_types(const diff* diff); + const fn_parm_diff* is_fn_parm_diff(const diff* diff); @@ -2720,6 +2726,9 @@ is_child_node_of_base_diff(const diff* diff); const corpus_diff* is_corpus_diff(const diff* diff); +const diff* +peel_typedef_diff(const diff* dif); + const diff* peel_pointer_diff(const diff* dif); diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc index bd254079..61727585 100644 --- a/src/abg-comparison.cc +++ b/src/abg-comparison.cc @@ -675,6 +675,23 @@ const function_type_diff* is_function_type_diff(const diff* diff) {return dynamic_cast(diff);} +/// Test if a given diff node carries a function type change with +/// local changes. +/// +/// @param diff the diff node to consider. +/// +/// @return a non-nil pointer to a @ref function_type_diff iff @p diff +/// is a function_type_diff node that carries a local change. +const function_type_diff* +is_function_type_diff_with_local_changes(const diff* diff) +{ + if (const function_type_diff* d = is_function_type_diff(diff)) + if (d->has_local_changes()) + return d; + + return 0; +} + /// Test if a diff node is about differences between variables. /// /// @param diff the diff node to test. @@ -740,7 +757,8 @@ is_qualified_type_diff(const diff* diff) {return dynamic_cast(diff);} /// Test if a diff node is either a reference diff node or a pointer -/// diff node. +/// diff node. Note that this function also works on diffs of +/// typedefs of reference or pointer. /// /// @param diff the diff node to test. /// @@ -748,7 +766,42 @@ is_qualified_type_diff(const diff* diff) /// pointer diff node. bool is_reference_or_pointer_diff(const diff* diff) -{return is_reference_diff(diff) || is_pointer_diff(diff);} +{ + diff = peel_typedef_diff(diff); + return is_reference_diff(diff) || is_pointer_diff(diff); +} + +/// Test if a diff node is a reference or pointer diff node to a +/// change that is neither basic type change nor distinct type change. +/// +/// Note that this function also works on diffs of typedefs of +/// reference or pointer. +/// +/// @param diff the diff node to consider. +/// +/// @return true iff @p diff is a eference or pointer diff node to a +/// change that is neither basic type change nor distinct type change. +bool +is_reference_or_ptr_diff_to_non_basic_nor_distinct_types(const diff* diff) +{ + diff = peel_typedef_diff(diff); + if (const reference_diff* d = is_reference_diff(diff)) + { + diff = peel_reference_diff(d); + if (is_diff_of_basic_type(diff) || is_distinct_diff(diff)) + return false; + return true; + } + else if (const pointer_diff *d = is_pointer_diff(diff)) + { + diff = peel_pointer_diff(d); + if (is_diff_of_basic_type(diff) || is_distinct_diff(diff)) + return false; + return true; + } + + return false; +} /// Test if a diff node is about differences between two function /// parameters. @@ -10913,9 +10966,10 @@ struct redundancy_marking_visitor : public diff_node_visitor || d->get_canonical_diff()->is_traversing()) && d->has_changes()) { - // But if two diff nodes are redundant sibbling, do not - // mark them as being redundant. This is to avoid marking - // nodes as redundant in this case: + // But if two diff nodes are redundant sibbling that carry + // changes of base types, do not mark them as being + // redundant. This is to avoid marking nodes as redundant + // in this case: // // int foo(int a, int b); // compared with: @@ -10948,7 +11002,10 @@ struct redundancy_marking_visitor : public diff_node_visitor sib = f->type_diff().get(); if (sib == d) continue; - if (sib->get_canonical_diff() == d->get_canonical_diff()) + if (sib->get_canonical_diff() == d->get_canonical_diff() + // Sibbling diff nodes that carry base type + // changes ar to be marked as redundant. + && (is_base_diff(sib) || is_distinct_diff(sib))) { redundant_with_sibling_node = true; break; @@ -10966,7 +11023,7 @@ struct redundancy_marking_visitor : public diff_node_visitor // redundant because otherwise one could miss important // similar local changes that are applied to different // functions. - && !dynamic_cast(d) + && !is_function_type_diff_with_local_changes(d) // Changes involving variadic parameters of functions // should never be marked redundant because we want to see // them all. @@ -10985,9 +11042,13 @@ struct redundancy_marking_visitor : public diff_node_visitor // that must be marked redundant. && d->context()->diff_has_been_visited(d) != d // If the diff node is a function parameter and is not - // a reference/pointer then do not mark it as + // a reference/pointer (to a non basic or a non + // distinct type diff) then do not mark it as // redundant. - && (is_reference_or_pointer_diff(d) + // + // Children nodes of base class diff nodes are never + // redundant either, we want to see them all. + && (is_reference_or_ptr_diff_to_non_basic_nor_distinct_types(d) || (!is_child_node_of_function_parm_diff(d) && !is_child_node_of_base_diff(d)))) { @@ -11312,6 +11373,25 @@ is_diff_of_basic_type(const diff* diff, bool allow_indirect_type) return is_diff_of_basic_type(diff); } +/// If a diff node is about changes between two typedef types, get the +/// diff node about changes between the underlying types. +/// +/// Note that this function walks the tree of underlying diff nodes +/// returns the first diff node about types that are not typedefs. +/// +/// @param dif the dif node to consider. +/// +/// @return the underlying diff node of @p dif, or just return @p dif +/// if it's not a typedef diff node. +const diff* +peel_typedef_diff(const diff* dif) +{ + const typedef_diff *d = 0; + while ((d = is_typedef_diff(dif))) + dif = d->underlying_type_diff().get(); + return dif; +} + /// If a diff node is about changes between two pointer types, get the /// diff node about changes between the underlying (pointed-to) types. /// diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am index ad296b48..46d1ecd3 100644 --- a/tests/data/Makefile.am +++ b/tests/data/Makefile.am @@ -1135,6 +1135,13 @@ test-diff-suppr/libtest36-leaf-v1.so \ test-diff-suppr/test36-leaf-report-0.txt \ test-diff-suppr/test36-leaf-v0.cc \ test-diff-suppr/test36-leaf-v1.cc \ +test-diff-suppr/test37-opaque-type-header-dir/test37-opaque-type-header-v0.h \ +test-diff-suppr/test37-opaque-type-header-dir/test37-opaque-type-header-v1.h \ +test-diff-suppr/test37-opaque-type-report-0.txt \ +test-diff-suppr/test37-opaque-type-v0.c \ +test-diff-suppr/test37-opaque-type-v0.o \ +test-diff-suppr/test37-opaque-type-v1.c \ +test-diff-suppr/test37-opaque-type-v1.o \ \ test-diff-dwarf-abixml/test0-pr19026-libvtkIOSQL-6.1.so.1 \ test-diff-dwarf-abixml/test0-pr19026-libvtkIOSQL-6.1.so.1.abi \ diff --git a/tests/data/test-diff-suppr/test37-opaque-type-header-dir/test37-opaque-type-header-v0.h b/tests/data/test-diff-suppr/test37-opaque-type-header-dir/test37-opaque-type-header-v0.h new file mode 100644 index 00000000..cb5ecb5d --- /dev/null +++ b/tests/data/test-diff-suppr/test37-opaque-type-header-dir/test37-opaque-type-header-v0.h @@ -0,0 +1,24 @@ +typedef struct opaque_struct * opaque_struct_pointer_type; + +typedef struct public_struct_type *public_struct_pointer_type; +typedef struct public_struct_type2 *public_struct_pointer_type2; + +typedef void (*FuncPointerType0) (public_struct_pointer_type, + public_struct_pointer_type); + +typedef void (*FuncPointerType1) (public_struct_pointer_type, int); + +typedef struct public_struct_type2 +{ + FuncPointerType0 m0; + FuncPointerType1 m1; +} public_struct_type2; + +typedef struct public_struct_type +{ + opaque_struct_pointer_type m0; + public_struct_type2 *m1; +} public_struct_type; + +void +bar0(public_struct_pointer_type *p); diff --git a/tests/data/test-diff-suppr/test37-opaque-type-header-dir/test37-opaque-type-header-v1.h b/tests/data/test-diff-suppr/test37-opaque-type-header-dir/test37-opaque-type-header-v1.h new file mode 100644 index 00000000..cb5ecb5d --- /dev/null +++ b/tests/data/test-diff-suppr/test37-opaque-type-header-dir/test37-opaque-type-header-v1.h @@ -0,0 +1,24 @@ +typedef struct opaque_struct * opaque_struct_pointer_type; + +typedef struct public_struct_type *public_struct_pointer_type; +typedef struct public_struct_type2 *public_struct_pointer_type2; + +typedef void (*FuncPointerType0) (public_struct_pointer_type, + public_struct_pointer_type); + +typedef void (*FuncPointerType1) (public_struct_pointer_type, int); + +typedef struct public_struct_type2 +{ + FuncPointerType0 m0; + FuncPointerType1 m1; +} public_struct_type2; + +typedef struct public_struct_type +{ + opaque_struct_pointer_type m0; + public_struct_type2 *m1; +} public_struct_type; + +void +bar0(public_struct_pointer_type *p); diff --git a/tests/data/test-diff-suppr/test37-opaque-type-report-0.txt b/tests/data/test-diff-suppr/test37-opaque-type-report-0.txt new file mode 100644 index 00000000..9666a8fd --- /dev/null +++ b/tests/data/test-diff-suppr/test37-opaque-type-report-0.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-suppr/test37-opaque-type-v0.c b/tests/data/test-diff-suppr/test37-opaque-type-v0.c new file mode 100644 index 00000000..e1f6d6c2 --- /dev/null +++ b/tests/data/test-diff-suppr/test37-opaque-type-v0.c @@ -0,0 +1,22 @@ +/* + * Compile this test with: + * gcc -c -g test37-opaque-type-v0.c + * + * This test exhibits changes that are redundant in a weird way. + * The redundant path through the diff graph involves typedefs, + * function types and function parameter diff nodes. + * + */ +#include "test37-opaque-type-header-dir/test37-opaque-type-header-v0.h" + +struct opaque_struct +{ + int m0; + int m1; + struct public_struct_type *m2; +}; + +void +bar0(public_struct_pointer_type *p __attribute__((unused))) +{ +} diff --git a/tests/data/test-diff-suppr/test37-opaque-type-v0.o b/tests/data/test-diff-suppr/test37-opaque-type-v0.o new file mode 100644 index 0000000000000000000000000000000000000000..fde596fe6cbc7374327b76dc596786f77b804495 GIT binary patch literal 3376 zcmbtW-D@LN6u)<-Y15{erXTxJDod0~x8O|D>~1Zq?MBOP>nclGb{{oNlAB~Slf=nn zZIx9)5m$Ur5M05B75@SMhxNfHAH+XkVO{V|d=LadJ?GB3>Ew3Gf*#1d=lss+J@;es z-sM-W7@RXjoNci~NuiAO(uaImvSl{U=Ge{MJCAnn-2U^``&YqjOls0Hz>z3ZhY^^8 zyumkMG}Bu!O3xghZUdk4i;P<-43P5I7@t9QEajIN&%Fg;n%qqk%=B#-BX?u=9`NXH z%-g_8cVpocxTlp#=Dsq?A{UuhzW}Ggj{gXPnf@8Zal|LECOJ^v$={^GA83H3MFz6o z^jOO4gk(2!Pufj`$s}-$FW`uzTEV~G0p(N=RLB^gw;(a5V|tg1)%uDl7-wex0W^A6DRwjcu2RxO92$9i72I@P(R~8YUNHKIvFN51w#)RFR zboiykDB#K><|v>3@$vtkKt#q4vd`tWo2QM2MiX- zL(vbJ}& z%2erx!Jrnh-k{p{Y9v)KeTfxXU0)RH-TLcZq3MN%wpVqlUeop3g@(5;`UT9~FVx-8 zl`K@+XxRNhuNP?XdR^Fi5y-gg%Aix*i+~%Dr^t4$-OO+2&pR8=g?y>FcA>alI+s6t zUDWftZW!TqX=62S`{AG?Hk+a&0z&pX@O7xQPK)YZpVio3$pk>+n-XM8`$m!>DupTSN)0XqC4$#e38mHWup&W>LO zg9uI>!4+=m@Bbtz&BT`pHk(eT?qh-`iOlFNIHEGy_z?^NlT93h%WHi^XEL~X31qTt z0*djiWAN!C_#z;4u|5p%V8Z>7TPlwLJOmU8?G=xZ%78eo<`V>gW}`{&9pWc(V}MRX z(-41(WULDb&4#xS_*DdfFn)aB9f?Lb#(zliB@HK$`5OKj$?s~oNp*gs;i|*mYxn`l zk2L%zlCu;~^+&}yqv3y({W9S=#rvRoj&V|Ci&Ktx-Ts|p@I2udJ071o{R2OAt1yQ_ zG`AENiv5r|_<%8|+V3+b5N+3~i|U|R@j8tz=p^q}tAV&RqILtf(-fmFZLcGg3#4F9 zt?T;&zIsll8wv*=54#__wKph`XjK}4>kBIUe@;u_kG)d;RN>0RV1MI8+cAWm1P(F% zWw<)D7-e+-6;R_j{;&RP;H$^Pj4J;%O{t0UTa15p%IFlMU+GZ2PY!sl&`#Ys{Ej1S z!cUKh_+0`&MIlDI?H=fK49%D6FcxFJ$m+H)1C~^Oi=J5JkM$#@*I!SF2@tzM`5#w* z1#FY@-=+MjPnaLyB|U!+FkF+W6Ie+PD8HjEScz)Jd!6KOVNpQpU5uo|{we`Q{%lfAbQCjmQ~G5*%T9;D7-P z(hX6@r)h2AQ%25;>}#lJ+9ePZc_PTPmqFx79m%v;Kuo@kpp|K_f|y!FG|x;X;imOI zJ|$D$n7kvyOJGbps7WT{_$iFBl#BhXa>jSR<6;zbklhe7q>`xSh|gX`AG-%)h7{xD7KTw) ze0T9^ZrV0;^SMbeHI_e#vom1k&e~7fPfsKJ1lUp;nHFZwwh0%AXR^o;JC$uL^MqKK zBYZ3a6lOhwVH>uI>pRYB4$E=Hxw)8oVUE&OW^X+u!|cqE&X~or9>A0H)C(ms z(rJkd=-^X`d$}w=k5!rSvOMY<1gyvC(b35)_+3RNM_} zUM*;P!ESLU*z@~E%G@tDyvUO(R=Ts}^as6OsPXl>xEuUX^1{VIr@kEnFJPV$TzdXW zVY6`BExTt5E2ZT#rL~pS!s2DWQP}dL7#g;@@|v@D zy5KZ*;^nl?$fg~7_3EMGYF=1^ddt(_fI0$jX>;>zVG-+H`uB3N*T-pjP{iw(LG0zl z{IN0XBV6nU5u+c8+$!gD`S*;??C1q_h~d;HhT`%4{qM9%Gxg@e_2vz#$Cx;iCT4s= z9GNmfp4=3C>tEIcdnVuKT}nAH2XgaVPn~?eDiEuZCk3#$!uqk-ry#OLrT%wSFJm(BJjkhF=>r zt3hX{i*r)3zn)dg9jbFxCos%-E z{MWQe&6J;M`e#(3poCc5klP$U`-=S3S)@-q$vXaYPQt%p@D>>f(S5gZPRGdoJa5KQ zB9>I$_gTc!>i?XtUKLOE6QtMQNZ~{Xu5A96Y zb)qWeFV4TJB~+#w>0GD9m#8QdbzjETLH|_zyEvhA%N~d#^ny(uW@{N zh{TF|e+7Sn3b|Gkz~kMN{?Q$(;#FU%9z8cv+~)k-Trho?=qswn)0*gK4