* [PATCH] abg-ir.cc: Improve types_have_similar_structure. @ 2020-03-24 13:29 Giuliano Procida 2020-03-25 14:13 ` Giuliano Procida 2020-03-25 14:18 ` [PATCH v2] " Giuliano Procida 0 siblings, 2 replies; 7+ messages in thread From: Giuliano Procida @ 2020-03-24 13:29 UTC (permalink / raw) To: libabigail; +Cc: dodji, kernel-team, gprocida This function is used to determine whether or not type differences are "local" and is primarily used in --leaf-changes-only mode. The logic has some issues which are addressed by this patch: - Any number of points-to (*) and refers-to (& and &&) components are peeled off the types being compared, rather than checking these match in number and kind. - This peeling is done with peel_typedef_pointer_or_reference_type which also peels any number of CV qualifiers (OK) and array-of ([N]) type components (not OK). - The function sets a state variable (was_indirect_type) to modify the behaviour of downstream comparisons, but this cannot be passed through recursive calls when it perhaps should be. The effect of the indirect_type flag is to switch to comparisons by name: identically named structs don't get introspected. Arguably, a more useful behaviour for --leaf-changes-only mode would be to treat any change to a named type as non-local, except in the context of the definition of that type itself. This would be a more significant change in behaviour. * include/abg-fwd.h (types_have_similar_structure): In both overloads, add an indirect_type argument, defaulting to false. * src/abg-ir.cc (reference_type_def constructor): Tabify. (types_have_similar_structure): In both overloads, add an indirect_type argument and update documentation text. In the type_base_sptr overload, pass indirect_type in the tail call. In the type_base* overload, replace was_indirect_type with indirect_type; peel CV qualifiers and typedefs without testing as the peel function does this; replace the indiscriminate peeling of qualifier/pointer/reference/array type components with code that checks the same pointer/reference/array type component is used on each side and makes recursive calls with indirect_type set to true; move the typeid check earlier, document its purpose and remove unneccessary checks after later dynamic casts; pass the indirect_type argument recursively when comparing array subranges; add a TODO for comparing array types more accurately. * tests/data/Makefile.am: Add new test case files. * tests/data/test-abidiff-exit/test-leaf-peeling-v0.cc: New test case. * tests/data/test-abidiff-exit/test-leaf-peeling-v1.cc: Ditto. * tests/data/test-abidiff-exit/test-leaf-peeling-report.txt: Ditto. * tests/test-abidiff-exit.cc: Run new test case. Signed-off-by: Giuliano Procida <gprocida@google.com> --- include/abg-fwd.h | 6 +- src/abg-ir.cc | 130 +++++++++--------- tests/data/Makefile.am | 5 + .../test-abidiff-exit/test-leaf-peeling-v0.cc | 24 ++++ .../test-abidiff-exit/test-leaf-peeling-v0.o | Bin 0 -> 3704 bytes .../test-abidiff-exit/test-leaf-peeling-v1.cc | 24 ++++ .../test-abidiff-exit/test-leaf-peeling-v1.o | Bin 0 -> 3752 bytes tests/test-abidiff-exit.cc | 9 ++ 8 files changed, 134 insertions(+), 64 deletions(-) create mode 100644 tests/data/test-abidiff-exit/test-leaf-peeling-v0.cc create mode 100644 tests/data/test-abidiff-exit/test-leaf-peeling-v0.o create mode 100644 tests/data/test-abidiff-exit/test-leaf-peeling-v1.cc create mode 100644 tests/data/test-abidiff-exit/test-leaf-peeling-v1.o diff --git a/include/abg-fwd.h b/include/abg-fwd.h index 6f2a862c..1aab70a6 100644 --- a/include/abg-fwd.h +++ b/include/abg-fwd.h @@ -1293,11 +1293,13 @@ function_decl_is_less_than(const function_decl&f, const function_decl &s); bool types_have_similar_structure(const type_base_sptr& first, - const type_base_sptr& second); + const type_base_sptr& second, + bool indirect_type = false); bool types_have_similar_structure(const type_base* first, - const type_base* second); + const type_base* second, + bool indirect_type = false); } // end namespace ir diff --git a/src/abg-ir.cc b/src/abg-ir.cc index 2853fe6c..5eacafc0 100644 --- a/src/abg-ir.cc +++ b/src/abg-ir.cc @@ -13573,10 +13573,10 @@ reference_type_def::reference_type_def(const type_base_sptr pointed_to, decl_base_sptr pto = dynamic_pointer_cast<decl_base>(pointed_to); string name; if (pto) - { - set_visibility(pto->get_visibility()); - name = string(pto->get_name()) + "&"; - } + { + set_visibility(pto->get_visibility()); + name = string(pto->get_name()) + "&"; + } else name = string(get_type_name(is_function_type(pointed_to), /*qualified_name=*/true)) + "&"; @@ -22563,33 +22563,44 @@ function_decl_is_less_than(const function_decl &f, const function_decl &s) /// Test if two types have similar structures, even though they are /// (or can be) different. /// -/// Two indirect types (pointers, references, qualified, typedefs) -/// have similar structure if their underlying types are of the same -/// kind and have the same name. In this indirect types case, the -/// size of the underlying type does not matter. +/// const and volatile qualifiers are completely ignored. +/// +/// typedef are resolved to their definitions; their names are ignored. +/// +/// Two indirect types (pointers or references) have similar structure +/// if their underlying types are of the same kind and have the same +/// name. In the indirect types case, the size of the underlying type +/// does not matter. /// /// Two direct types (i.e, non indirect) have a similar structure if /// they have the same kind, name and size. Two class types have -/// similar structure if they have the same name, size, and if their -/// data members have similar types. +/// similar structure if they have the same name, size, and if the +/// types of their data members have similar types. /// /// @param first the first type to consider. /// /// @param second the second type to consider. /// +/// @param indirect_type whether to do an indirect comparison +/// /// @return true iff @p first and @p second have similar structures. bool types_have_similar_structure(const type_base_sptr& first, - const type_base_sptr& second) -{return types_have_similar_structure(first.get(), second.get());} + const type_base_sptr& second, + bool indirect_type) +{return types_have_similar_structure(first.get(), second.get(), indirect_type);} /// Test if two types have similar structures, even though they are /// (or can be) different. /// -/// Two indirect types (pointers, references, qualified, typedefs) -/// have similar structure if their underlying types are of the same -/// kind and have the same name. In this indirect types case, the -/// size of the underlying type does not matter. +/// const and volatile qualifiers are completely ignored. +/// +/// typedef are resolved to their definitions; their names are ignored. +/// +/// Two indirect types (pointers or references) have similar structure +/// if their underlying types are of the same kind and have the same +/// name. In the indirect types case, the size of the underlying type +/// does not matter. /// /// Two direct types (i.e, non indirect) have a similar structure if /// they have the same kind, name and size. Two class types have @@ -22600,9 +22611,13 @@ types_have_similar_structure(const type_base_sptr& first, /// /// @param second the second type to consider. /// +/// @param indirect_type whether to do an indirect comparison +/// /// @return true iff @p first and @p second have similar structures. bool -types_have_similar_structure(const type_base* first, const type_base* second) +types_have_similar_structure(const type_base* first, + const type_base* second, + bool indirect_type) { if (!!first != !!second) return false; @@ -22610,33 +22625,38 @@ types_have_similar_structure(const type_base* first, const type_base* second) if (!first) return false; - if (is_typedef(first) || is_qualified_type(first)) - first = peel_qualified_or_typedef_type(first); - - if (is_typedef(second) || is_qualified_type(second)) - second = peel_qualified_or_typedef_type(second); + // Treat typedefs purely as type aliases and ignore CV-qualifiers. + first = peel_qualified_or_typedef_type(first); + second = peel_qualified_or_typedef_type(second); - bool was_indirect_type = (is_pointer_type(first) - || is_pointer_type(second) - || is_reference_type(first) - || is_reference_type(second)); + // This check simplifies all the dynamic casting below. + if (typeid(*first) != typeid(*second)) + return false; - if (was_indirect_type) + // Peel off matching pointers. + if (const pointer_type_def* ty1 = is_pointer_type(first)) { - first = peel_typedef_pointer_or_reference_type(first); - second = peel_typedef_pointer_or_reference_type(second); + const pointer_type_def* ty2 = is_pointer_type(second); + return types_have_similar_structure(ty1->get_pointed_to_type(), + ty2->get_pointed_to_type(), + true); } - if (typeid(*first) != typeid(*second)) - return false; + // Peel off matching references. + if (const reference_type_def* ty1 = is_reference_type(first)) + { + const reference_type_def* ty2 = is_reference_type(second); + if (ty1->is_lvalue() != ty2->is_lvalue()) + return false; + return types_have_similar_structure(ty1->get_pointed_to_type(), + ty2->get_pointed_to_type(), + true); + } if (const type_decl* ty1 = is_type_decl(first)) { const type_decl* ty2 = is_type_decl(second); - if (ty2 == 0) - return false; - - if (!was_indirect_type) + if (!indirect_type) if (ty1->get_size_in_bits() != ty2->get_size_in_bits()) return false; @@ -22646,10 +22666,7 @@ types_have_similar_structure(const type_base* first, const type_base* second) if (const enum_type_decl* ty1 = is_enum_type(first)) { const enum_type_decl* ty2 = is_enum_type(second); - if (ty2 == 0) - return false; - - if (!was_indirect_type) + if (!indirect_type) if (ty1->get_size_in_bits() != ty2->get_size_in_bits()) return false; @@ -22660,16 +22677,13 @@ types_have_similar_structure(const type_base* first, const type_base* second) if (const class_decl* ty1 = is_class_type(first)) { const class_decl* ty2 = is_class_type(second); - if (ty2 == 0) - return false; - if (!ty1->get_is_anonymous() && !ty2->get_is_anonymous() && ty1->get_name() != ty2->get_name()) return false; - if (!was_indirect_type) + if (!indirect_type) { - if (!was_indirect_type) + if (!indirect_type) if ((ty1->get_size_in_bits() != ty2->get_size_in_bits()) || (ty1->get_non_static_data_members().size() != ty2->get_non_static_data_members().size())) @@ -22696,14 +22710,11 @@ types_have_similar_structure(const type_base* first, const type_base* second) if (const union_decl* ty1 = is_union_type(first)) { const union_decl* ty2 = is_union_type(second); - if (ty2 == 0) - return false; - if (!ty1->get_is_anonymous() && !ty2->get_is_anonymous() && ty1->get_name() != ty2->get_name()) return false; - if (!was_indirect_type) + if (!indirect_type) return ty1->get_size_in_bits() == ty2->get_size_in_bits(); return true; @@ -22712,13 +22723,13 @@ types_have_similar_structure(const type_base* first, const type_base* second) if (const array_type_def* ty1 = is_array_type(first)) { const array_type_def* ty2 = is_array_type(second); - - if (!was_indirect_type) - if (ty1->get_size_in_bits() != ty2->get_size_in_bits() - || ty1->get_dimension_count() != ty2->get_dimension_count() - || !types_have_similar_structure(ty1->get_element_type(), - ty2->get_element_type())) - return false; + // TODO: Handle uint32_t[10] vs uint64_t[5] better. + if (ty1->get_size_in_bits() != ty2->get_size_in_bits() + || ty1->get_dimension_count() != ty2->get_dimension_count() + || !types_have_similar_structure(ty1->get_element_type(), + ty2->get_element_type(), + true)) + return false; return true; } @@ -22726,14 +22737,12 @@ types_have_similar_structure(const type_base* first, const type_base* second) if (const array_type_def::subrange_type *ty1 = is_subrange_type(first)) { const array_type_def::subrange_type *ty2 = is_subrange_type(second); - if (!ty2) - return false; - if (ty1->get_upper_bound() != ty2->get_upper_bound() || ty1->get_lower_bound() != ty2->get_lower_bound() || ty1->get_language() != ty2->get_language() || !types_have_similar_structure(ty1->get_underlying_type(), - ty2->get_underlying_type())) + ty2->get_underlying_type(), + indirect_type)) return false; return true; @@ -22742,9 +22751,6 @@ types_have_similar_structure(const type_base* first, const type_base* second) if (const function_type* ty1 = is_function_type(first)) { const function_type* ty2 = is_function_type(second); - if (!ty2) - return false; - if (!types_have_similar_structure(ty1->get_return_type(), ty2->get_return_type())) return false; diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am index cf7792f1..6d4e2faa 100644 --- a/tests/data/Makefile.am +++ b/tests/data/Makefile.am @@ -130,6 +130,11 @@ test-abidiff-exit/test-leaf3-v0.o \ test-abidiff-exit/test-leaf3-v1.c \ test-abidiff-exit/test-leaf3-v1.o \ test-abidiff-exit/test-leaf3-report.txt \ +test-abidiff-exit/test-leaf-peeling-v0.cc \ +test-abidiff-exit/test-leaf-peeling-v0.o \ +test-abidiff-exit/test-leaf-peeling-v1.cc \ +test-abidiff-exit/test-leaf-peeling-v1.o \ +test-abidiff-exit/test-leaf-peeling-report.txt \ \ test-diff-dwarf/test0-v0.cc \ test-diff-dwarf/test0-v0.o \ diff --git a/tests/data/test-abidiff-exit/test-leaf-peeling-v0.cc b/tests/data/test-abidiff-exit/test-leaf-peeling-v0.cc new file mode 100644 index 00000000..b927d15e --- /dev/null +++ b/tests/data/test-abidiff-exit/test-leaf-peeling-v0.cc @@ -0,0 +1,24 @@ +struct foo { + int z; +}; + +struct ops1 { + int * x; +}; + +struct ops2 { + foo y[10]; +}; + +struct ops3 { + void (*spong)(int & wibble); +}; + +void register_ops1(ops1*) { +} + +void register_ops2(ops2*) { +} + +void register_ops3(ops3*) { +} diff --git a/tests/data/test-abidiff-exit/test-leaf-peeling-v0.o b/tests/data/test-abidiff-exit/test-leaf-peeling-v0.o new file mode 100644 index 0000000000000000000000000000000000000000..a556a63560122a5e926ab708bb13f0fd40714638 GIT binary patch literal 3704 zcmbtXO>7%g5T56a?R7SGoVcZpf{-JD=nuP%ov0+G4TjJ(5v8h-Dx?-j*Us9pu-B1y z14$7g5<jB1B7p#@NJ!jz;zo~%3aK}495`{|iiEg;IKa&A8|V4izQ9QPX6Bop_ujm> zv-Z0eFRvwp044z|aHuf~a6fY>S0b?j1(=3w>$iVhzx~dicV2riTznm~jYR=wlsA?S zlfj%KC+eLJj0s_`A!ytJkw}w-8OD-LKZGR7rbwD~<i;L|RE8vgR1^`JPl$~90h#wf zjHgKgNZUAkijPS{1~O(EIj75--=Z>3h{%mUBo5a1I84w0*(i4^a>@}Pj-`k(SyB*p z|3Yb!hQ!C)xTL!%#Q{e~1f0T>Owtesw0Z$kBLZ$v01a`#OBeuRN<5J(SVnF(HzlUi z`D17;2*b3jbJjD~vLy;wj4T*ttZo#RaK&SfgJrE)hKW@`=bY-Cnnc?MMx}+=C{VaL z2OEVm7?zP?xUF$anNw3*M+Ny#B+(bEmX}F#j(B#4J^g5jmE)g1KSREg<eNB-n|KPJ z1nmZJMGDO+vMRoUiUD-2Lj0Y};uu|Gtc-xr3qrf)xpjNT^IA>6;mkX=S`?j}g-kpR z#pgv*yqBC^>5T~k6SX>iqqyq_&4%x773tb-UN0?ly+$(#z3ygbCn#NAq!EH0bcH@k zpH}YE79a}3p-j@ksC8bta;>;}`gCcrxa^djQn5T=S}v95%SF2#?)u(^hUa_TW(`%h zTid>{w{+G%yI8avMZ4|>`+jY^+wnWQfxYX$)%3UQ(5<$-U|<jBXb)vL6Sj8-j0=%* z0pQZ=>bc^H7rbiI_2cE(rKeA73!};&iOP%Bzx6VRy}X#6NSk-?G#*8q9-`<)c`ld# zFtM6Vt)T;AWIInrq*3-)WAjkEGz~DE8r#nA{|p!qz(XDeBi_3jeRcya!^n`0O}81) z_xAS821H$_r?X&SM;b;(yyF3r0oehJ03+@;12zLD1~7W(rj<HGwMT4T;;b`Ym-r*d zi5MSsiATLMI?mTn$N2_wC8;~JO!M=O2&d{D;6pSOv#6UB-jw=L4gXE*Gm_JdP*4A) z;;(k3D+*5euS>nH;hR$bT*IXbUuyW4)bDBdDXIUc;g_ZUlZO8x^`9l@eet>Rk#pTv zq+eRT5{z?ROX8e|&%LDi)4d^D((vgRhVxn;3HQ#F=8Znz(vX*&=VgEH6*^v&4s?d< zIaV~B*MC#Pd-ZXG{Wi8taDuQK^KF(wZ!ZLA%MD#{szCrw*K4`XmRH?vY&QLR2W^q; zR;yj_%|5f+b^Qi*VKoH5Odf~e6yVf4?Y4)X8^`a2o`atOI|$v{8#2h--mG`swkH?< zKiMqCQw`O3ic(f+K$?qLZzNxp$WEBv8C}&$k@ff|(MIppe~m9;te%sm<N2$aQB$Me z8}w7OLO}|BB@6P444`{Re%u47ZxUU=N6$(40}1{>ibA^YRkZ0Cxql=tFTYQmLic?U z4a3%dAQ#H<w0`>P=<BC`qvu2j?#uaakfM<8OZ{!w{NKolxK1=b^+|pH9mFUnuM@?H z3i3j5-(j5RXZ|)ChQ&Wdi$bFNr4q+6e;<u3t9}<{yo!;m`+gxYS+1V@x{TlJA+f;w zXZRBo$hE?N<PW9ey40zD^dExbxvsPxeQpZ*PR@TI7fjzn`n90P)0yb+9zsx&_x~m- V3hDc&yu;%2lo;vjnDs_I{$J;aGn)Va literal 0 HcmV?d00001 diff --git a/tests/data/test-abidiff-exit/test-leaf-peeling-v1.cc b/tests/data/test-abidiff-exit/test-leaf-peeling-v1.cc new file mode 100644 index 00000000..7c88df9f --- /dev/null +++ b/tests/data/test-abidiff-exit/test-leaf-peeling-v1.cc @@ -0,0 +1,24 @@ +struct foo { + long z; +}; + +struct ops1 { + int ** x; +}; + +struct ops2 { + foo y[10]; +}; + +struct ops3 { + void (*spong)(int && wibble); +}; + +void register_ops1(ops1*) { +} + +void register_ops2(ops2*) { +} + +void register_ops3(ops3*) { +} diff --git a/tests/data/test-abidiff-exit/test-leaf-peeling-v1.o b/tests/data/test-abidiff-exit/test-leaf-peeling-v1.o new file mode 100644 index 0000000000000000000000000000000000000000..e71ade4431f80a78ee73292d60808a2ee5404640 GIT binary patch literal 3752 zcmbtXO=x3P6h1evNnV>K{Yl4Zsf!f+8^*k8(lRq0ZH=wdX;6nE6lF$fU(%N}O!89l zUbU@=f+Ear6h#DAiVI!25#8%7MK^*A7lMjz-4zt{ocqpAZ*Ni%J&=3O`Oe?H=l;C* z)vHgf#)JSx0+!%dQz*digU517iY3UxIBcxl{&ns4D@S);_^Q8n3$qQ#09)pb<l|&8 zqsg&ayA8Gw_9}wbA&8iiFoPW@H#yiMZr8}Z4<a!{5<tRk%CUoEav<LD!^n4C4BBra zW*vYSN|FR%=W+NDe}i(s+)08tciR3Ml_5ezX6Sq3kop#fVHzN9Q;CFh$`ByVCWt9Y zu^>K3;BrT3h`qIg0iU2D3Ya)0;1UJU5Ct4`1IA7X*r5O#qJXEVv=|fjXR;|PGo2X| z<H^ypXw3@CPNlA-9!V{vL>8No2Fs4ra`rOLc;G%rrIs)VyMWFa-8nIWwlYQ~*~!KZ z3Q^8drokB~XD?ueAxxRkQ>M-cau|-I19okhlVlw6*p%}0<rGVk?=4S}?+E$ECUFrD z<1a?50W7A`j3(>iv#3}=+bYDLnKX{_L}O<JgkBKlo1R<C?|NRd;n$slQ?1JA_%sYg zWhj1B#Kr6J>80)%voKM!?bmaAe$c4<-gb`8l|)Fm5Vm$9*hL5Ec=bjQdY#SoZcsR( z6;Eh$ARTZhk~9a`u5aX4&Yv&L=N6ozQ^*x(3k!w9Y%!N_g?qlYSoeId)2O2AcB(sz z`<E}~FV5%k^<2K@1_yq1r_=V^dqIBBf2rYb=R>#B^a9wtQJB-GR9v5zB~R&v>0XfT zg`PBX5}yOOwz6_1_s|nwrQ!O~#Pfwq=Zq!D7Q;luKpZ{;Vt-Uj4=3$AxH0!4PCp`f zXCBXtz8PCdCsxq`F|s{MMx@j1&qwCIb~g1eERE06AAk245WxNJ2P57+B`53_TKbX2 z51whW;KW7lnk|qIP*+!i1=^#lK;%}mVhxEz3(`Fp0ao<z^w=yI?!oB#jca|7y+>@8 zaiuH1#`rzRi4=a?z5BOkO&wX&U!6hKNqVT2O!d`@2&e9C5{NVwG5N3){tfGA41AjP zDduz{^xZ$F`Rk8US;Hy+IP2F9e4X_}1K(i%V*_UuJ~i+gtbbwPIo5wP@MYG2Ht@e# z|Bbm?uiDQvUkTOEdG?zy@Xr~aWL)J@=V8h4e}Mh38u%}amyJ9OclVm+Bfr>e7-g>N zDu4Bcn0SsIXm9oXEg86)|CWKP{(uu4wD8$Q9(E$WqomN=55d`XLl>M%5P;M1ny$0$ zRrcze4Zqe#o0Q#3rQ^ML!t8cjzm6}t9)ce)PlaF!aH{QA%fk<m<F`Z4!B0s(2;J%} z4)S(3YaO@c@x=cpqkBO;)bARlEYW~86S3|{zB-YeF!RN9btg?W;~zvD-M9Z5U%*&1 zCzVt6+lJ9l<)0+<r)!CVH2RVoa>N024#`iw4fLK8E#lA2N%(gR-X%pN)0f^26C?L` z`S8wau_T(lPa@WD{-5zgRXoj~zDDN!xBKKo2<~$Io1|!D)~9#4U;UrBkm?iFr}xvW zzl#{<RP#hpQjQOVdXE&Y>MQ;-8v4c0)1;8-eCed(6n_JaO4j{ua=ea_Z2G>>7`LnI zzQytTT_h6J`W5^R3glW+0P`buTw|U3NB?H1c-2>$k6D{Se&G7A@r3C+Nq<F{@w6xA e-9reT<MY2oibm%8DQ~~{5+z3ZC}Q2wjQ<DlLOH<z literal 0 HcmV?d00001 diff --git a/tests/test-abidiff-exit.cc b/tests/test-abidiff-exit.cc index 6ecbfd24..ebac7a00 100644 --- a/tests/test-abidiff-exit.cc +++ b/tests/test-abidiff-exit.cc @@ -158,6 +158,15 @@ InOutSpec in_out_specs[] = "data/test-abidiff-exit/test-leaf3-report.txt", "output/test-abidiff-exit/test-leaf3-report.txt" }, + { + "data/test-abidiff-exit/test-leaf-peeling-v0.o", + "data/test-abidiff-exit/test-leaf-peeling-v1.o", + "", + "--leaf-changes-only", + abigail::tools_utils::ABIDIFF_ABI_CHANGE, + "data/test-abidiff-exit/test-leaf-peeling-report.txt", + "output/test-abidiff-exit/test-leaf-peeling-report.txt" + }, {0, 0, 0 ,0, abigail::tools_utils::ABIDIFF_OK, 0, 0} }; -- 2.25.1.696.g5e7596f4ac-goog ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] abg-ir.cc: Improve types_have_similar_structure. 2020-03-24 13:29 [PATCH] abg-ir.cc: Improve types_have_similar_structure Giuliano Procida @ 2020-03-25 14:13 ` Giuliano Procida 2020-03-25 14:18 ` [PATCH v2] " Giuliano Procida 1 sibling, 0 replies; 7+ messages in thread From: Giuliano Procida @ 2020-03-25 14:13 UTC (permalink / raw) To: libabigail; +Cc: Dodji Seketeli, kernel-team Please ignore this. The report.txt file is missing and there are some possible issues around passing indirect_type in the recursive calls. I'll post a new version shortly. Giuliano. On Tue, 24 Mar 2020 at 13:29, Giuliano Procida <gprocida@google.com> wrote: > > This function is used to determine whether or not type differences are > "local" and is primarily used in --leaf-changes-only mode. The logic > has some issues which are addressed by this patch: > > - Any number of points-to (*) and refers-to (& and &&) components > are peeled off the types being compared, rather than checking > these match in number and kind. > - This peeling is done with peel_typedef_pointer_or_reference_type > which also peels any number of CV qualifiers (OK) and > array-of ([N]) type components (not OK). > - The function sets a state variable (was_indirect_type) to modify > the behaviour of downstream comparisons, but this cannot be > passed through recursive calls when it perhaps should be. > > The effect of the indirect_type flag is to switch to comparisons by > name: identically named structs don't get introspected. Arguably, a > more useful behaviour for --leaf-changes-only mode would be to treat > any change to a named type as non-local, except in the context of the > definition of that type itself. This would be a more significant > change in behaviour. > > * include/abg-fwd.h (types_have_similar_structure): In both > overloads, add an indirect_type argument, defaulting to > false. > * src/abg-ir.cc (reference_type_def constructor): Tabify. > (types_have_similar_structure): In both overloads, add an > indirect_type argument and update documentation text. In the > type_base_sptr overload, pass indirect_type in the tail > call. In the type_base* overload, replace was_indirect_type > with indirect_type; peel CV qualifiers and typedefs without > testing as the peel function does this; replace the > indiscriminate peeling of qualifier/pointer/reference/array > type components with code that checks the same > pointer/reference/array type component is used on each side > and makes recursive calls with indirect_type set to true; move > the typeid check earlier, document its purpose and remove > unneccessary checks after later dynamic casts; pass the > indirect_type argument recursively when comparing array > subranges; add a TODO for comparing array types more > accurately. > * tests/data/Makefile.am: Add new test case files. > * tests/data/test-abidiff-exit/test-leaf-peeling-v0.cc: New > test case. > * tests/data/test-abidiff-exit/test-leaf-peeling-v1.cc: Ditto. > * tests/data/test-abidiff-exit/test-leaf-peeling-report.txt: > Ditto. > * tests/test-abidiff-exit.cc: Run new test case. > > Signed-off-by: Giuliano Procida <gprocida@google.com> > --- > include/abg-fwd.h | 6 +- > src/abg-ir.cc | 130 +++++++++--------- > tests/data/Makefile.am | 5 + > .../test-abidiff-exit/test-leaf-peeling-v0.cc | 24 ++++ > .../test-abidiff-exit/test-leaf-peeling-v0.o | Bin 0 -> 3704 bytes > .../test-abidiff-exit/test-leaf-peeling-v1.cc | 24 ++++ > .../test-abidiff-exit/test-leaf-peeling-v1.o | Bin 0 -> 3752 bytes > tests/test-abidiff-exit.cc | 9 ++ > 8 files changed, 134 insertions(+), 64 deletions(-) > create mode 100644 tests/data/test-abidiff-exit/test-leaf-peeling-v0.cc > create mode 100644 tests/data/test-abidiff-exit/test-leaf-peeling-v0.o > create mode 100644 tests/data/test-abidiff-exit/test-leaf-peeling-v1.cc > create mode 100644 tests/data/test-abidiff-exit/test-leaf-peeling-v1.o > > diff --git a/include/abg-fwd.h b/include/abg-fwd.h > index 6f2a862c..1aab70a6 100644 > --- a/include/abg-fwd.h > +++ b/include/abg-fwd.h > @@ -1293,11 +1293,13 @@ function_decl_is_less_than(const function_decl&f, const function_decl &s); > > bool > types_have_similar_structure(const type_base_sptr& first, > - const type_base_sptr& second); > + const type_base_sptr& second, > + bool indirect_type = false); > > bool > types_have_similar_structure(const type_base* first, > - const type_base* second); > + const type_base* second, > + bool indirect_type = false); > > } // end namespace ir > > diff --git a/src/abg-ir.cc b/src/abg-ir.cc > index 2853fe6c..5eacafc0 100644 > --- a/src/abg-ir.cc > +++ b/src/abg-ir.cc > @@ -13573,10 +13573,10 @@ reference_type_def::reference_type_def(const type_base_sptr pointed_to, > decl_base_sptr pto = dynamic_pointer_cast<decl_base>(pointed_to); > string name; > if (pto) > - { > - set_visibility(pto->get_visibility()); > - name = string(pto->get_name()) + "&"; > - } > + { > + set_visibility(pto->get_visibility()); > + name = string(pto->get_name()) + "&"; > + } > else > name = string(get_type_name(is_function_type(pointed_to), > /*qualified_name=*/true)) + "&"; > @@ -22563,33 +22563,44 @@ function_decl_is_less_than(const function_decl &f, const function_decl &s) > /// Test if two types have similar structures, even though they are > /// (or can be) different. > /// > -/// Two indirect types (pointers, references, qualified, typedefs) > -/// have similar structure if their underlying types are of the same > -/// kind and have the same name. In this indirect types case, the > -/// size of the underlying type does not matter. > +/// const and volatile qualifiers are completely ignored. > +/// > +/// typedef are resolved to their definitions; their names are ignored. > +/// > +/// Two indirect types (pointers or references) have similar structure > +/// if their underlying types are of the same kind and have the same > +/// name. In the indirect types case, the size of the underlying type > +/// does not matter. > /// > /// Two direct types (i.e, non indirect) have a similar structure if > /// they have the same kind, name and size. Two class types have > -/// similar structure if they have the same name, size, and if their > -/// data members have similar types. > +/// similar structure if they have the same name, size, and if the > +/// types of their data members have similar types. > /// > /// @param first the first type to consider. > /// > /// @param second the second type to consider. > /// > +/// @param indirect_type whether to do an indirect comparison > +/// > /// @return true iff @p first and @p second have similar structures. > bool > types_have_similar_structure(const type_base_sptr& first, > - const type_base_sptr& second) > -{return types_have_similar_structure(first.get(), second.get());} > + const type_base_sptr& second, > + bool indirect_type) > +{return types_have_similar_structure(first.get(), second.get(), indirect_type);} > > /// Test if two types have similar structures, even though they are > /// (or can be) different. > /// > -/// Two indirect types (pointers, references, qualified, typedefs) > -/// have similar structure if their underlying types are of the same > -/// kind and have the same name. In this indirect types case, the > -/// size of the underlying type does not matter. > +/// const and volatile qualifiers are completely ignored. > +/// > +/// typedef are resolved to their definitions; their names are ignored. > +/// > +/// Two indirect types (pointers or references) have similar structure > +/// if their underlying types are of the same kind and have the same > +/// name. In the indirect types case, the size of the underlying type > +/// does not matter. > /// > /// Two direct types (i.e, non indirect) have a similar structure if > /// they have the same kind, name and size. Two class types have > @@ -22600,9 +22611,13 @@ types_have_similar_structure(const type_base_sptr& first, > /// > /// @param second the second type to consider. > /// > +/// @param indirect_type whether to do an indirect comparison > +/// > /// @return true iff @p first and @p second have similar structures. > bool > -types_have_similar_structure(const type_base* first, const type_base* second) > +types_have_similar_structure(const type_base* first, > + const type_base* second, > + bool indirect_type) > { > if (!!first != !!second) > return false; > @@ -22610,33 +22625,38 @@ types_have_similar_structure(const type_base* first, const type_base* second) > if (!first) > return false; > > - if (is_typedef(first) || is_qualified_type(first)) > - first = peel_qualified_or_typedef_type(first); > - > - if (is_typedef(second) || is_qualified_type(second)) > - second = peel_qualified_or_typedef_type(second); > + // Treat typedefs purely as type aliases and ignore CV-qualifiers. > + first = peel_qualified_or_typedef_type(first); > + second = peel_qualified_or_typedef_type(second); > > - bool was_indirect_type = (is_pointer_type(first) > - || is_pointer_type(second) > - || is_reference_type(first) > - || is_reference_type(second)); > + // This check simplifies all the dynamic casting below. > + if (typeid(*first) != typeid(*second)) > + return false; > > - if (was_indirect_type) > + // Peel off matching pointers. > + if (const pointer_type_def* ty1 = is_pointer_type(first)) > { > - first = peel_typedef_pointer_or_reference_type(first); > - second = peel_typedef_pointer_or_reference_type(second); > + const pointer_type_def* ty2 = is_pointer_type(second); > + return types_have_similar_structure(ty1->get_pointed_to_type(), > + ty2->get_pointed_to_type(), > + true); > } > > - if (typeid(*first) != typeid(*second)) > - return false; > + // Peel off matching references. > + if (const reference_type_def* ty1 = is_reference_type(first)) > + { > + const reference_type_def* ty2 = is_reference_type(second); > + if (ty1->is_lvalue() != ty2->is_lvalue()) > + return false; > + return types_have_similar_structure(ty1->get_pointed_to_type(), > + ty2->get_pointed_to_type(), > + true); > + } > > if (const type_decl* ty1 = is_type_decl(first)) > { > const type_decl* ty2 = is_type_decl(second); > - if (ty2 == 0) > - return false; > - > - if (!was_indirect_type) > + if (!indirect_type) > if (ty1->get_size_in_bits() != ty2->get_size_in_bits()) > return false; > > @@ -22646,10 +22666,7 @@ types_have_similar_structure(const type_base* first, const type_base* second) > if (const enum_type_decl* ty1 = is_enum_type(first)) > { > const enum_type_decl* ty2 = is_enum_type(second); > - if (ty2 == 0) > - return false; > - > - if (!was_indirect_type) > + if (!indirect_type) > if (ty1->get_size_in_bits() != ty2->get_size_in_bits()) > return false; > > @@ -22660,16 +22677,13 @@ types_have_similar_structure(const type_base* first, const type_base* second) > if (const class_decl* ty1 = is_class_type(first)) > { > const class_decl* ty2 = is_class_type(second); > - if (ty2 == 0) > - return false; > - > if (!ty1->get_is_anonymous() && !ty2->get_is_anonymous() > && ty1->get_name() != ty2->get_name()) > return false; > > - if (!was_indirect_type) > + if (!indirect_type) > { > - if (!was_indirect_type) > + if (!indirect_type) > if ((ty1->get_size_in_bits() != ty2->get_size_in_bits()) > || (ty1->get_non_static_data_members().size() > != ty2->get_non_static_data_members().size())) > @@ -22696,14 +22710,11 @@ types_have_similar_structure(const type_base* first, const type_base* second) > if (const union_decl* ty1 = is_union_type(first)) > { > const union_decl* ty2 = is_union_type(second); > - if (ty2 == 0) > - return false; > - > if (!ty1->get_is_anonymous() && !ty2->get_is_anonymous() > && ty1->get_name() != ty2->get_name()) > return false; > > - if (!was_indirect_type) > + if (!indirect_type) > return ty1->get_size_in_bits() == ty2->get_size_in_bits(); > > return true; > @@ -22712,13 +22723,13 @@ types_have_similar_structure(const type_base* first, const type_base* second) > if (const array_type_def* ty1 = is_array_type(first)) > { > const array_type_def* ty2 = is_array_type(second); > - > - if (!was_indirect_type) > - if (ty1->get_size_in_bits() != ty2->get_size_in_bits() > - || ty1->get_dimension_count() != ty2->get_dimension_count() > - || !types_have_similar_structure(ty1->get_element_type(), > - ty2->get_element_type())) > - return false; > + // TODO: Handle uint32_t[10] vs uint64_t[5] better. > + if (ty1->get_size_in_bits() != ty2->get_size_in_bits() > + || ty1->get_dimension_count() != ty2->get_dimension_count() > + || !types_have_similar_structure(ty1->get_element_type(), > + ty2->get_element_type(), > + true)) > + return false; > > return true; > } > @@ -22726,14 +22737,12 @@ types_have_similar_structure(const type_base* first, const type_base* second) > if (const array_type_def::subrange_type *ty1 = is_subrange_type(first)) > { > const array_type_def::subrange_type *ty2 = is_subrange_type(second); > - if (!ty2) > - return false; > - > if (ty1->get_upper_bound() != ty2->get_upper_bound() > || ty1->get_lower_bound() != ty2->get_lower_bound() > || ty1->get_language() != ty2->get_language() > || !types_have_similar_structure(ty1->get_underlying_type(), > - ty2->get_underlying_type())) > + ty2->get_underlying_type(), > + indirect_type)) > return false; > > return true; > @@ -22742,9 +22751,6 @@ types_have_similar_structure(const type_base* first, const type_base* second) > if (const function_type* ty1 = is_function_type(first)) > { > const function_type* ty2 = is_function_type(second); > - if (!ty2) > - return false; > - > if (!types_have_similar_structure(ty1->get_return_type(), > ty2->get_return_type())) > return false; > diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am > index cf7792f1..6d4e2faa 100644 > --- a/tests/data/Makefile.am > +++ b/tests/data/Makefile.am > @@ -130,6 +130,11 @@ test-abidiff-exit/test-leaf3-v0.o \ > test-abidiff-exit/test-leaf3-v1.c \ > test-abidiff-exit/test-leaf3-v1.o \ > test-abidiff-exit/test-leaf3-report.txt \ > +test-abidiff-exit/test-leaf-peeling-v0.cc \ > +test-abidiff-exit/test-leaf-peeling-v0.o \ > +test-abidiff-exit/test-leaf-peeling-v1.cc \ > +test-abidiff-exit/test-leaf-peeling-v1.o \ > +test-abidiff-exit/test-leaf-peeling-report.txt \ > \ > test-diff-dwarf/test0-v0.cc \ > test-diff-dwarf/test0-v0.o \ > diff --git a/tests/data/test-abidiff-exit/test-leaf-peeling-v0.cc b/tests/data/test-abidiff-exit/test-leaf-peeling-v0.cc > new file mode 100644 > index 00000000..b927d15e > --- /dev/null > +++ b/tests/data/test-abidiff-exit/test-leaf-peeling-v0.cc > @@ -0,0 +1,24 @@ > +struct foo { > + int z; > +}; > + > +struct ops1 { > + int * x; > +}; > + > +struct ops2 { > + foo y[10]; > +}; > + > +struct ops3 { > + void (*spong)(int & wibble); > +}; > + > +void register_ops1(ops1*) { > +} > + > +void register_ops2(ops2*) { > +} > + > +void register_ops3(ops3*) { > +} > diff --git a/tests/data/test-abidiff-exit/test-leaf-peeling-v0.o b/tests/data/test-abidiff-exit/test-leaf-peeling-v0.o > new file mode 100644 > index 0000000000000000000000000000000000000000..a556a63560122a5e926ab708bb13f0fd40714638 > GIT binary patch > literal 3704 > zcmbtXO>7%g5T56a?R7SGoVcZpf{-JD=nuP%ov0+G4TjJ(5v8h-Dx?-j*Us9pu-B1y > z14$7g5<jB1B7p#@NJ!jz;zo~%3aK}495`{|iiEg;IKa&A8|V4izQ9QPX6Bop_ujm> > zv-Z0eFRvwp044z|aHuf~a6fY>S0b?j1(=3w>$iVhzx~dicV2riTznm~jYR=wlsA?S > zlfj%KC+eLJj0s_`A!ytJkw}w-8OD-LKZGR7rbwD~<i;L|RE8vgR1^`JPl$~90h#wf > zjHgKgNZUAkijPS{1~O(EIj75--=Z>3h{%mUBo5a1I84w0*(i4^a>@}Pj-`k(SyB*p > z|3Yb!hQ!C)xTL!%#Q{e~1f0T>Owtesw0Z$kBLZ$v01a`#OBeuRN<5J(SVnF(HzlUi > z`D17;2*b3jbJjD~vLy;wj4T*ttZo#RaK&SfgJrE)hKW@`=bY-Cnnc?MMx}+=C{VaL > z2OEVm7?zP?xUF$anNw3*M+Ny#B+(bEmX}F#j(B#4J^g5jmE)g1KSREg<eNB-n|KPJ > z1nmZJMGDO+vMRoUiUD-2Lj0Y};uu|Gtc-xr3qrf)xpjNT^IA>6;mkX=S`?j}g-kpR > z#pgv*yqBC^>5T~k6SX>iqqyq_&4%x773tb-UN0?ly+$(#z3ygbCn#NAq!EH0bcH@k > zpH}YE79a}3p-j@ksC8bta;>;}`gCcrxa^djQn5T=S}v95%SF2#?)u(^hUa_TW(`%h > zTid>{w{+G%yI8avMZ4|>`+jY^+wnWQfxYX$)%3UQ(5<$-U|<jBXb)vL6Sj8-j0=%* > z0pQZ=>bc^H7rbiI_2cE(rKeA73!};&iOP%Bzx6VRy}X#6NSk-?G#*8q9-`<)c`ld# > zFtM6Vt)T;AWIInrq*3-)WAjkEGz~DE8r#nA{|p!qz(XDeBi_3jeRcya!^n`0O}81) > z_xAS821H$_r?X&SM;b;(yyF3r0oehJ03+@;12zLD1~7W(rj<HGwMT4T;;b`Ym-r*d > zi5MSsiATLMI?mTn$N2_wC8;~JO!M=O2&d{D;6pSOv#6UB-jw=L4gXE*Gm_JdP*4A) > z;;(k3D+*5euS>nH;hR$bT*IXbUuyW4)bDBdDXIUc;g_ZUlZO8x^`9l@eet>Rk#pTv > zq+eRT5{z?ROX8e|&%LDi)4d^D((vgRhVxn;3HQ#F=8Znz(vX*&=VgEH6*^v&4s?d< > zIaV~B*MC#Pd-ZXG{Wi8taDuQK^KF(wZ!ZLA%MD#{szCrw*K4`XmRH?vY&QLR2W^q; > zR;yj_%|5f+b^Qi*VKoH5Odf~e6yVf4?Y4)X8^`a2o`atOI|$v{8#2h--mG`swkH?< > zKiMqCQw`O3ic(f+K$?qLZzNxp$WEBv8C}&$k@ff|(MIppe~m9;te%sm<N2$aQB$Me > z8}w7OLO}|BB@6P444`{Re%u47ZxUU=N6$(40}1{>ibA^YRkZ0Cxql=tFTYQmLic?U > z4a3%dAQ#H<w0`>P=<BC`qvu2j?#uaakfM<8OZ{!w{NKolxK1=b^+|pH9mFUnuM@?H > z3i3j5-(j5RXZ|)ChQ&Wdi$bFNr4q+6e;<u3t9}<{yo!;m`+gxYS+1V@x{TlJA+f;w > zXZRBo$hE?N<PW9ey40zD^dExbxvsPxeQpZ*PR@TI7fjzn`n90P)0yb+9zsx&_x~m- > V3hDc&yu;%2lo;vjnDs_I{$J;aGn)Va > > literal 0 > HcmV?d00001 > > diff --git a/tests/data/test-abidiff-exit/test-leaf-peeling-v1.cc b/tests/data/test-abidiff-exit/test-leaf-peeling-v1.cc > new file mode 100644 > index 00000000..7c88df9f > --- /dev/null > +++ b/tests/data/test-abidiff-exit/test-leaf-peeling-v1.cc > @@ -0,0 +1,24 @@ > +struct foo { > + long z; > +}; > + > +struct ops1 { > + int ** x; > +}; > + > +struct ops2 { > + foo y[10]; > +}; > + > +struct ops3 { > + void (*spong)(int && wibble); > +}; > + > +void register_ops1(ops1*) { > +} > + > +void register_ops2(ops2*) { > +} > + > +void register_ops3(ops3*) { > +} > diff --git a/tests/data/test-abidiff-exit/test-leaf-peeling-v1.o b/tests/data/test-abidiff-exit/test-leaf-peeling-v1.o > new file mode 100644 > index 0000000000000000000000000000000000000000..e71ade4431f80a78ee73292d60808a2ee5404640 > GIT binary patch > literal 3752 > zcmbtXO=x3P6h1evNnV>K{Yl4Zsf!f+8^*k8(lRq0ZH=wdX;6nE6lF$fU(%N}O!89l > zUbU@=f+Ear6h#DAiVI!25#8%7MK^*A7lMjz-4zt{ocqpAZ*Ni%J&=3O`Oe?H=l;C* > z)vHgf#)JSx0+!%dQz*digU517iY3UxIBcxl{&ns4D@S);_^Q8n3$qQ#09)pb<l|&8 > zqsg&ayA8Gw_9}wbA&8iiFoPW@H#yiMZr8}Z4<a!{5<tRk%CUoEav<LD!^n4C4BBra > zW*vYSN|FR%=W+NDe}i(s+)08tciR3Ml_5ezX6Sq3kop#fVHzN9Q;CFh$`ByVCWt9Y > zu^>K3;BrT3h`qIg0iU2D3Ya)0;1UJU5Ct4`1IA7X*r5O#qJXEVv=|fjXR;|PGo2X| > z<H^ypXw3@CPNlA-9!V{vL>8No2Fs4ra`rOLc;G%rrIs)VyMWFa-8nIWwlYQ~*~!KZ > z3Q^8drokB~XD?ueAxxRkQ>M-cau|-I19okhlVlw6*p%}0<rGVk?=4S}?+E$ECUFrD > z<1a?50W7A`j3(>iv#3}=+bYDLnKX{_L}O<JgkBKlo1R<C?|NRd;n$slQ?1JA_%sYg > zWhj1B#Kr6J>80)%voKM!?bmaAe$c4<-gb`8l|)Fm5Vm$9*hL5Ec=bjQdY#SoZcsR( > z6;Eh$ARTZhk~9a`u5aX4&Yv&L=N6ozQ^*x(3k!w9Y%!N_g?qlYSoeId)2O2AcB(sz > z`<E}~FV5%k^<2K@1_yq1r_=V^dqIBBf2rYb=R>#B^a9wtQJB-GR9v5zB~R&v>0XfT > zg`PBX5}yOOwz6_1_s|nwrQ!O~#Pfwq=Zq!D7Q;luKpZ{;Vt-Uj4=3$AxH0!4PCp`f > zXCBXtz8PCdCsxq`F|s{MMx@j1&qwCIb~g1eERE06AAk245WxNJ2P57+B`53_TKbX2 > z51whW;KW7lnk|qIP*+!i1=^#lK;%}mVhxEz3(`Fp0ao<z^w=yI?!oB#jca|7y+>@8 > zaiuH1#`rzRi4=a?z5BOkO&wX&U!6hKNqVT2O!d`@2&e9C5{NVwG5N3){tfGA41AjP > zDduz{^xZ$F`Rk8US;Hy+IP2F9e4X_}1K(i%V*_UuJ~i+gtbbwPIo5wP@MYG2Ht@e# > z|Bbm?uiDQvUkTOEdG?zy@Xr~aWL)J@=V8h4e}Mh38u%}amyJ9OclVm+Bfr>e7-g>N > zDu4Bcn0SsIXm9oXEg86)|CWKP{(uu4wD8$Q9(E$WqomN=55d`XLl>M%5P;M1ny$0$ > zRrcze4Zqe#o0Q#3rQ^ML!t8cjzm6}t9)ce)PlaF!aH{QA%fk<m<F`Z4!B0s(2;J%} > z4)S(3YaO@c@x=cpqkBO;)bARlEYW~86S3|{zB-YeF!RN9btg?W;~zvD-M9Z5U%*&1 > zCzVt6+lJ9l<)0+<r)!CVH2RVoa>N024#`iw4fLK8E#lA2N%(gR-X%pN)0f^26C?L` > z`S8wau_T(lPa@WD{-5zgRXoj~zDDN!xBKKo2<~$Io1|!D)~9#4U;UrBkm?iFr}xvW > zzl#{<RP#hpQjQOVdXE&Y>MQ;-8v4c0)1;8-eCed(6n_JaO4j{ua=ea_Z2G>>7`LnI > zzQytTT_h6J`W5^R3glW+0P`buTw|U3NB?H1c-2>$k6D{Se&G7A@r3C+Nq<F{@w6xA > e-9reT<MY2oibm%8DQ~~{5+z3ZC}Q2wjQ<DlLOH<z > > literal 0 > HcmV?d00001 > > diff --git a/tests/test-abidiff-exit.cc b/tests/test-abidiff-exit.cc > index 6ecbfd24..ebac7a00 100644 > --- a/tests/test-abidiff-exit.cc > +++ b/tests/test-abidiff-exit.cc > @@ -158,6 +158,15 @@ InOutSpec in_out_specs[] = > "data/test-abidiff-exit/test-leaf3-report.txt", > "output/test-abidiff-exit/test-leaf3-report.txt" > }, > + { > + "data/test-abidiff-exit/test-leaf-peeling-v0.o", > + "data/test-abidiff-exit/test-leaf-peeling-v1.o", > + "", > + "--leaf-changes-only", > + abigail::tools_utils::ABIDIFF_ABI_CHANGE, > + "data/test-abidiff-exit/test-leaf-peeling-report.txt", > + "output/test-abidiff-exit/test-leaf-peeling-report.txt" > + }, > {0, 0, 0 ,0, abigail::tools_utils::ABIDIFF_OK, 0, 0} > }; > > -- > 2.25.1.696.g5e7596f4ac-goog > ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] abg-ir.cc: Improve types_have_similar_structure. 2020-03-24 13:29 [PATCH] abg-ir.cc: Improve types_have_similar_structure Giuliano Procida 2020-03-25 14:13 ` Giuliano Procida @ 2020-03-25 14:18 ` Giuliano Procida 2020-03-26 11:38 ` Matthias Maennich 2020-03-26 13:36 ` Dodji Seketeli 1 sibling, 2 replies; 7+ messages in thread From: Giuliano Procida @ 2020-03-25 14:18 UTC (permalink / raw) To: libabigail; +Cc: dodji, kernel-team, gprocida This function is used to determine whether or not type differences are "local" and is primarily used in --leaf-changes-only mode. The logic has some issues which are addressed by this patch: - Any number of points-to (*) and refers-to (& and &&) components are peeled off the types being compared, rather than checking these match in number and kind. - This peeling is done with peel_typedef_pointer_or_reference_type which also peels any number of CV qualifiers (OK) and array-of ([N]) type components (not OK). - The function sets a state variable (was_indirect_type) to modify the behaviour of downstream comparisons, but this cannot be passed through recursive calls. The effect of the indirect_type flag is to switch to comparisons by name: identically named structs don't get introspected. Arguably, a more useful behaviour for --leaf-changes-only mode would be to treat any change to a named type as non-local, except in the context of the definition of that type itself. This would be a more significant change in behaviour. * include/abg-fwd.h (types_have_similar_structure): In both overloads, add an indirect_type argument, defaulting to false. * src/abg-ir.cc (reference_type_def constructor): Tabify. (types_have_similar_structure): In both overloads, add an indirect_type argument and update documentation text. In the type_base_sptr overload, pass indirect_type in the tail call. In the type_base* overload, replace was_indirect_type with indirect_type; peel CV qualifiers and typedefs without testing as the peel function does this; replace the indiscriminate peeling of qualifier/pointer/reference/array type components with code that checks the same pointer/reference/array type component is used on each side and makes recursive calls with indirect_type set to true; pass the indirect_type argument recursively when comparing other subtypes; move the typeid check earlier, document its purpose and remove unneccessary checks after later dynamic casts; remove an always-true conditional; add a TODO for comparing array types more accurately. * tests/data/Makefile.am: Add new test case files. * tests/data/test-abidiff-exit/test-leaf-peeling-v0.cc: New test case. * tests/data/test-abidiff-exit/test-leaf-peeling-v1.cc: Ditto. * tests/data/test-abidiff-exit/test-leaf-peeling-report.txt: Ditto. * tests/test-abidiff-exit.cc: Run new test case. Signed-off-by: Giuliano Procida <gprocida@google.com> --- include/abg-fwd.h | 6 +- src/abg-ir.cc | 147 ++++++++++-------- tests/data/Makefile.am | 5 + .../test-leaf-peeling-report.txt | 38 +++++ .../test-abidiff-exit/test-leaf-peeling-v0.cc | 24 +++ .../test-abidiff-exit/test-leaf-peeling-v0.o | Bin 0 -> 3704 bytes .../test-abidiff-exit/test-leaf-peeling-v1.cc | 24 +++ .../test-abidiff-exit/test-leaf-peeling-v1.o | Bin 0 -> 3752 bytes tests/test-abidiff-exit.cc | 9 ++ 9 files changed, 182 insertions(+), 71 deletions(-) create mode 100644 tests/data/test-abidiff-exit/test-leaf-peeling-report.txt create mode 100644 tests/data/test-abidiff-exit/test-leaf-peeling-v0.cc create mode 100644 tests/data/test-abidiff-exit/test-leaf-peeling-v0.o create mode 100644 tests/data/test-abidiff-exit/test-leaf-peeling-v1.cc create mode 100644 tests/data/test-abidiff-exit/test-leaf-peeling-v1.o diff --git a/include/abg-fwd.h b/include/abg-fwd.h index 6f2a862c..1aab70a6 100644 --- a/include/abg-fwd.h +++ b/include/abg-fwd.h @@ -1293,11 +1293,13 @@ function_decl_is_less_than(const function_decl&f, const function_decl &s); bool types_have_similar_structure(const type_base_sptr& first, - const type_base_sptr& second); + const type_base_sptr& second, + bool indirect_type = false); bool types_have_similar_structure(const type_base* first, - const type_base* second); + const type_base* second, + bool indirect_type = false); } // end namespace ir diff --git a/src/abg-ir.cc b/src/abg-ir.cc index 2853fe6c..a10b0bb7 100644 --- a/src/abg-ir.cc +++ b/src/abg-ir.cc @@ -13573,10 +13573,10 @@ reference_type_def::reference_type_def(const type_base_sptr pointed_to, decl_base_sptr pto = dynamic_pointer_cast<decl_base>(pointed_to); string name; if (pto) - { - set_visibility(pto->get_visibility()); - name = string(pto->get_name()) + "&"; - } + { + set_visibility(pto->get_visibility()); + name = string(pto->get_name()) + "&"; + } else name = string(get_type_name(is_function_type(pointed_to), /*qualified_name=*/true)) + "&"; @@ -22563,33 +22563,44 @@ function_decl_is_less_than(const function_decl &f, const function_decl &s) /// Test if two types have similar structures, even though they are /// (or can be) different. /// -/// Two indirect types (pointers, references, qualified, typedefs) -/// have similar structure if their underlying types are of the same -/// kind and have the same name. In this indirect types case, the -/// size of the underlying type does not matter. +/// const and volatile qualifiers are completely ignored. +/// +/// typedef are resolved to their definitions; their names are ignored. +/// +/// Two indirect types (pointers or references) have similar structure +/// if their underlying types are of the same kind and have the same +/// name. In the indirect types case, the size of the underlying type +/// does not matter. /// /// Two direct types (i.e, non indirect) have a similar structure if /// they have the same kind, name and size. Two class types have -/// similar structure if they have the same name, size, and if their -/// data members have similar types. +/// similar structure if they have the same name, size, and if the +/// types of their data members have similar types. /// /// @param first the first type to consider. /// /// @param second the second type to consider. /// +/// @param indirect_type whether to do an indirect comparison +/// /// @return true iff @p first and @p second have similar structures. bool types_have_similar_structure(const type_base_sptr& first, - const type_base_sptr& second) -{return types_have_similar_structure(first.get(), second.get());} + const type_base_sptr& second, + bool indirect_type) +{return types_have_similar_structure(first.get(), second.get(), indirect_type);} /// Test if two types have similar structures, even though they are /// (or can be) different. /// -/// Two indirect types (pointers, references, qualified, typedefs) -/// have similar structure if their underlying types are of the same -/// kind and have the same name. In this indirect types case, the -/// size of the underlying type does not matter. +/// const and volatile qualifiers are completely ignored. +/// +/// typedef are resolved to their definitions; their names are ignored. +/// +/// Two indirect types (pointers or references) have similar structure +/// if their underlying types are of the same kind and have the same +/// name. In the indirect types case, the size of the underlying type +/// does not matter. /// /// Two direct types (i.e, non indirect) have a similar structure if /// they have the same kind, name and size. Two class types have @@ -22600,9 +22611,13 @@ types_have_similar_structure(const type_base_sptr& first, /// /// @param second the second type to consider. /// +/// @param indirect_type whether to do an indirect comparison +/// /// @return true iff @p first and @p second have similar structures. bool -types_have_similar_structure(const type_base* first, const type_base* second) +types_have_similar_structure(const type_base* first, + const type_base* second, + bool indirect_type) { if (!!first != !!second) return false; @@ -22610,33 +22625,39 @@ types_have_similar_structure(const type_base* first, const type_base* second) if (!first) return false; - if (is_typedef(first) || is_qualified_type(first)) - first = peel_qualified_or_typedef_type(first); - - if (is_typedef(second) || is_qualified_type(second)) - second = peel_qualified_or_typedef_type(second); + // Treat typedefs purely as type aliases and ignore CV-qualifiers. + first = peel_qualified_or_typedef_type(first); + second = peel_qualified_or_typedef_type(second); - bool was_indirect_type = (is_pointer_type(first) - || is_pointer_type(second) - || is_reference_type(first) - || is_reference_type(second)); + // Eliminate all but N of the N^2 comparison cases. This also guarantees the + // various ty2 below cannot be null. + if (typeid(*first) != typeid(*second)) + return false; - if (was_indirect_type) + // Peel off matching pointers. + if (const pointer_type_def* ty1 = is_pointer_type(first)) { - first = peel_typedef_pointer_or_reference_type(first); - second = peel_typedef_pointer_or_reference_type(second); + const pointer_type_def* ty2 = is_pointer_type(second); + return types_have_similar_structure(ty1->get_pointed_to_type(), + ty2->get_pointed_to_type(), + true); } - if (typeid(*first) != typeid(*second)) - return false; + // Peel off matching references. + if (const reference_type_def* ty1 = is_reference_type(first)) + { + const reference_type_def* ty2 = is_reference_type(second); + if (ty1->is_lvalue() != ty2->is_lvalue()) + return false; + return types_have_similar_structure(ty1->get_pointed_to_type(), + ty2->get_pointed_to_type(), + true); + } if (const type_decl* ty1 = is_type_decl(first)) { const type_decl* ty2 = is_type_decl(second); - if (ty2 == 0) - return false; - - if (!was_indirect_type) + if (!indirect_type) if (ty1->get_size_in_bits() != ty2->get_size_in_bits()) return false; @@ -22646,10 +22667,7 @@ types_have_similar_structure(const type_base* first, const type_base* second) if (const enum_type_decl* ty1 = is_enum_type(first)) { const enum_type_decl* ty2 = is_enum_type(second); - if (ty2 == 0) - return false; - - if (!was_indirect_type) + if (!indirect_type) if (ty1->get_size_in_bits() != ty2->get_size_in_bits()) return false; @@ -22660,20 +22678,16 @@ types_have_similar_structure(const type_base* first, const type_base* second) if (const class_decl* ty1 = is_class_type(first)) { const class_decl* ty2 = is_class_type(second); - if (ty2 == 0) - return false; - if (!ty1->get_is_anonymous() && !ty2->get_is_anonymous() && ty1->get_name() != ty2->get_name()) return false; - if (!was_indirect_type) + if (!indirect_type) { - if (!was_indirect_type) - if ((ty1->get_size_in_bits() != ty2->get_size_in_bits()) - || (ty1->get_non_static_data_members().size() - != ty2->get_non_static_data_members().size())) - return false; + if ((ty1->get_size_in_bits() != ty2->get_size_in_bits()) + || (ty1->get_non_static_data_members().size() + != ty2->get_non_static_data_members().size())) + return false; for (class_or_union::data_members::const_iterator i = ty1->get_non_static_data_members().begin(), @@ -22685,7 +22699,8 @@ types_have_similar_structure(const type_base* first, const type_base* second) var_decl_sptr dm1 = *i; var_decl_sptr dm2 = *j; if (!types_have_similar_structure(dm1->get_type().get(), - dm2->get_type().get())) + dm2->get_type().get(), + indirect_type)) return false; } } @@ -22696,14 +22711,11 @@ types_have_similar_structure(const type_base* first, const type_base* second) if (const union_decl* ty1 = is_union_type(first)) { const union_decl* ty2 = is_union_type(second); - if (ty2 == 0) - return false; - if (!ty1->get_is_anonymous() && !ty2->get_is_anonymous() && ty1->get_name() != ty2->get_name()) return false; - if (!was_indirect_type) + if (!indirect_type) return ty1->get_size_in_bits() == ty2->get_size_in_bits(); return true; @@ -22712,13 +22724,13 @@ types_have_similar_structure(const type_base* first, const type_base* second) if (const array_type_def* ty1 = is_array_type(first)) { const array_type_def* ty2 = is_array_type(second); - - if (!was_indirect_type) - if (ty1->get_size_in_bits() != ty2->get_size_in_bits() - || ty1->get_dimension_count() != ty2->get_dimension_count() - || !types_have_similar_structure(ty1->get_element_type(), - ty2->get_element_type())) - return false; + // TODO: Handle uint32_t[10] vs uint64_t[5] better. + if (ty1->get_size_in_bits() != ty2->get_size_in_bits() + || ty1->get_dimension_count() != ty2->get_dimension_count() + || !types_have_similar_structure(ty1->get_element_type(), + ty2->get_element_type(), + indirect_type)) + return false; return true; } @@ -22726,14 +22738,12 @@ types_have_similar_structure(const type_base* first, const type_base* second) if (const array_type_def::subrange_type *ty1 = is_subrange_type(first)) { const array_type_def::subrange_type *ty2 = is_subrange_type(second); - if (!ty2) - return false; - if (ty1->get_upper_bound() != ty2->get_upper_bound() || ty1->get_lower_bound() != ty2->get_lower_bound() || ty1->get_language() != ty2->get_language() || !types_have_similar_structure(ty1->get_underlying_type(), - ty2->get_underlying_type())) + ty2->get_underlying_type(), + indirect_type)) return false; return true; @@ -22742,11 +22752,9 @@ types_have_similar_structure(const type_base* first, const type_base* second) if (const function_type* ty1 = is_function_type(first)) { const function_type* ty2 = is_function_type(second); - if (!ty2) - return false; - if (!types_have_similar_structure(ty1->get_return_type(), - ty2->get_return_type())) + ty2->get_return_type(), + indirect_type)) return false; if (ty1->get_parameters().size() != ty2->get_parameters().size()) @@ -22759,7 +22767,8 @@ types_have_similar_structure(const type_base* first, const type_base* second) && j != ty2->get_parameters().end()); ++i, ++j) if (!types_have_similar_structure((*i)->get_type(), - (*j)->get_type())) + (*j)->get_type(), + indirect_type)) return false; return true; diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am index cf7792f1..6d4e2faa 100644 --- a/tests/data/Makefile.am +++ b/tests/data/Makefile.am @@ -130,6 +130,11 @@ test-abidiff-exit/test-leaf3-v0.o \ test-abidiff-exit/test-leaf3-v1.c \ test-abidiff-exit/test-leaf3-v1.o \ test-abidiff-exit/test-leaf3-report.txt \ +test-abidiff-exit/test-leaf-peeling-v0.cc \ +test-abidiff-exit/test-leaf-peeling-v0.o \ +test-abidiff-exit/test-leaf-peeling-v1.cc \ +test-abidiff-exit/test-leaf-peeling-v1.o \ +test-abidiff-exit/test-leaf-peeling-report.txt \ \ test-diff-dwarf/test0-v0.cc \ test-diff-dwarf/test0-v0.o \ diff --git a/tests/data/test-abidiff-exit/test-leaf-peeling-report.txt b/tests/data/test-abidiff-exit/test-leaf-peeling-report.txt new file mode 100644 index 00000000..54a26869 --- /dev/null +++ b/tests/data/test-abidiff-exit/test-leaf-peeling-report.txt @@ -0,0 +1,38 @@ +Leaf changes summary: 4 artifacts changed +Changed leaf types summary: 4 leaf types 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 foo at test-leaf-peeling.0.cc:1:1' changed: + type size changed from 32 to 64 (in bits) + there are data member changes: + type 'int' of 'foo::z' changed: + type name changed from 'int' to 'long int' + type size changed from 32 to 64 (in bits) + and size changed from 32 to 64 (in bits) (by +32 bits) + + + +'struct ops1 at test-leaf-peeling.0.cc:5:1' changed: + type size hasn't changed + there are data member changes: + type 'int*' of 'ops1::x' changed: + pointer type changed from: 'int*' to: 'int**' + + + + +'struct ops2 at test-leaf-peeling.0.cc:9:1' changed: + type size changed from 320 to 640 (in bits) + there are data member changes: + 'foo ops2::y[10]' size changed from 320 to 640 (in bits) (by +320 bits) + + + +'struct ops3 at test-leaf-peeling.0.cc:13:1' changed: + type size hasn't changed + there are data member changes: + type 'void (int&)*' of 'ops3::spong' changed: + pointer type changed from: 'void (int&)*' to: 'void (int&&)*' + + diff --git a/tests/data/test-abidiff-exit/test-leaf-peeling-v0.cc b/tests/data/test-abidiff-exit/test-leaf-peeling-v0.cc new file mode 100644 index 00000000..b927d15e --- /dev/null +++ b/tests/data/test-abidiff-exit/test-leaf-peeling-v0.cc @@ -0,0 +1,24 @@ +struct foo { + int z; +}; + +struct ops1 { + int * x; +}; + +struct ops2 { + foo y[10]; +}; + +struct ops3 { + void (*spong)(int & wibble); +}; + +void register_ops1(ops1*) { +} + +void register_ops2(ops2*) { +} + +void register_ops3(ops3*) { +} diff --git a/tests/data/test-abidiff-exit/test-leaf-peeling-v0.o b/tests/data/test-abidiff-exit/test-leaf-peeling-v0.o new file mode 100644 index 0000000000000000000000000000000000000000..a556a63560122a5e926ab708bb13f0fd40714638 GIT binary patch literal 3704 zcmbtXO>7%g5T56a?R7SGoVcZpf{-JD=nuP%ov0+G4TjJ(5v8h-Dx?-j*Us9pu-B1y z14$7g5<jB1B7p#@NJ!jz;zo~%3aK}495`{|iiEg;IKa&A8|V4izQ9QPX6Bop_ujm> zv-Z0eFRvwp044z|aHuf~a6fY>S0b?j1(=3w>$iVhzx~dicV2riTznm~jYR=wlsA?S zlfj%KC+eLJj0s_`A!ytJkw}w-8OD-LKZGR7rbwD~<i;L|RE8vgR1^`JPl$~90h#wf zjHgKgNZUAkijPS{1~O(EIj75--=Z>3h{%mUBo5a1I84w0*(i4^a>@}Pj-`k(SyB*p z|3Yb!hQ!C)xTL!%#Q{e~1f0T>Owtesw0Z$kBLZ$v01a`#OBeuRN<5J(SVnF(HzlUi z`D17;2*b3jbJjD~vLy;wj4T*ttZo#RaK&SfgJrE)hKW@`=bY-Cnnc?MMx}+=C{VaL z2OEVm7?zP?xUF$anNw3*M+Ny#B+(bEmX}F#j(B#4J^g5jmE)g1KSREg<eNB-n|KPJ z1nmZJMGDO+vMRoUiUD-2Lj0Y};uu|Gtc-xr3qrf)xpjNT^IA>6;mkX=S`?j}g-kpR z#pgv*yqBC^>5T~k6SX>iqqyq_&4%x773tb-UN0?ly+$(#z3ygbCn#NAq!EH0bcH@k zpH}YE79a}3p-j@ksC8bta;>;}`gCcrxa^djQn5T=S}v95%SF2#?)u(^hUa_TW(`%h zTid>{w{+G%yI8avMZ4|>`+jY^+wnWQfxYX$)%3UQ(5<$-U|<jBXb)vL6Sj8-j0=%* z0pQZ=>bc^H7rbiI_2cE(rKeA73!};&iOP%Bzx6VRy}X#6NSk-?G#*8q9-`<)c`ld# zFtM6Vt)T;AWIInrq*3-)WAjkEGz~DE8r#nA{|p!qz(XDeBi_3jeRcya!^n`0O}81) z_xAS821H$_r?X&SM;b;(yyF3r0oehJ03+@;12zLD1~7W(rj<HGwMT4T;;b`Ym-r*d zi5MSsiATLMI?mTn$N2_wC8;~JO!M=O2&d{D;6pSOv#6UB-jw=L4gXE*Gm_JdP*4A) z;;(k3D+*5euS>nH;hR$bT*IXbUuyW4)bDBdDXIUc;g_ZUlZO8x^`9l@eet>Rk#pTv zq+eRT5{z?ROX8e|&%LDi)4d^D((vgRhVxn;3HQ#F=8Znz(vX*&=VgEH6*^v&4s?d< zIaV~B*MC#Pd-ZXG{Wi8taDuQK^KF(wZ!ZLA%MD#{szCrw*K4`XmRH?vY&QLR2W^q; zR;yj_%|5f+b^Qi*VKoH5Odf~e6yVf4?Y4)X8^`a2o`atOI|$v{8#2h--mG`swkH?< zKiMqCQw`O3ic(f+K$?qLZzNxp$WEBv8C}&$k@ff|(MIppe~m9;te%sm<N2$aQB$Me z8}w7OLO}|BB@6P444`{Re%u47ZxUU=N6$(40}1{>ibA^YRkZ0Cxql=tFTYQmLic?U z4a3%dAQ#H<w0`>P=<BC`qvu2j?#uaakfM<8OZ{!w{NKolxK1=b^+|pH9mFUnuM@?H z3i3j5-(j5RXZ|)ChQ&Wdi$bFNr4q+6e;<u3t9}<{yo!;m`+gxYS+1V@x{TlJA+f;w zXZRBo$hE?N<PW9ey40zD^dExbxvsPxeQpZ*PR@TI7fjzn`n90P)0yb+9zsx&_x~m- V3hDc&yu;%2lo;vjnDs_I{$J;aGn)Va literal 0 HcmV?d00001 diff --git a/tests/data/test-abidiff-exit/test-leaf-peeling-v1.cc b/tests/data/test-abidiff-exit/test-leaf-peeling-v1.cc new file mode 100644 index 00000000..7c88df9f --- /dev/null +++ b/tests/data/test-abidiff-exit/test-leaf-peeling-v1.cc @@ -0,0 +1,24 @@ +struct foo { + long z; +}; + +struct ops1 { + int ** x; +}; + +struct ops2 { + foo y[10]; +}; + +struct ops3 { + void (*spong)(int && wibble); +}; + +void register_ops1(ops1*) { +} + +void register_ops2(ops2*) { +} + +void register_ops3(ops3*) { +} diff --git a/tests/data/test-abidiff-exit/test-leaf-peeling-v1.o b/tests/data/test-abidiff-exit/test-leaf-peeling-v1.o new file mode 100644 index 0000000000000000000000000000000000000000..e71ade4431f80a78ee73292d60808a2ee5404640 GIT binary patch literal 3752 zcmbtXO=x3P6h1evNnV>K{Yl4Zsf!f+8^*k8(lRq0ZH=wdX;6nE6lF$fU(%N}O!89l zUbU@=f+Ear6h#DAiVI!25#8%7MK^*A7lMjz-4zt{ocqpAZ*Ni%J&=3O`Oe?H=l;C* z)vHgf#)JSx0+!%dQz*digU517iY3UxIBcxl{&ns4D@S);_^Q8n3$qQ#09)pb<l|&8 zqsg&ayA8Gw_9}wbA&8iiFoPW@H#yiMZr8}Z4<a!{5<tRk%CUoEav<LD!^n4C4BBra zW*vYSN|FR%=W+NDe}i(s+)08tciR3Ml_5ezX6Sq3kop#fVHzN9Q;CFh$`ByVCWt9Y zu^>K3;BrT3h`qIg0iU2D3Ya)0;1UJU5Ct4`1IA7X*r5O#qJXEVv=|fjXR;|PGo2X| z<H^ypXw3@CPNlA-9!V{vL>8No2Fs4ra`rOLc;G%rrIs)VyMWFa-8nIWwlYQ~*~!KZ z3Q^8drokB~XD?ueAxxRkQ>M-cau|-I19okhlVlw6*p%}0<rGVk?=4S}?+E$ECUFrD z<1a?50W7A`j3(>iv#3}=+bYDLnKX{_L}O<JgkBKlo1R<C?|NRd;n$slQ?1JA_%sYg zWhj1B#Kr6J>80)%voKM!?bmaAe$c4<-gb`8l|)Fm5Vm$9*hL5Ec=bjQdY#SoZcsR( z6;Eh$ARTZhk~9a`u5aX4&Yv&L=N6ozQ^*x(3k!w9Y%!N_g?qlYSoeId)2O2AcB(sz z`<E}~FV5%k^<2K@1_yq1r_=V^dqIBBf2rYb=R>#B^a9wtQJB-GR9v5zB~R&v>0XfT zg`PBX5}yOOwz6_1_s|nwrQ!O~#Pfwq=Zq!D7Q;luKpZ{;Vt-Uj4=3$AxH0!4PCp`f zXCBXtz8PCdCsxq`F|s{MMx@j1&qwCIb~g1eERE06AAk245WxNJ2P57+B`53_TKbX2 z51whW;KW7lnk|qIP*+!i1=^#lK;%}mVhxEz3(`Fp0ao<z^w=yI?!oB#jca|7y+>@8 zaiuH1#`rzRi4=a?z5BOkO&wX&U!6hKNqVT2O!d`@2&e9C5{NVwG5N3){tfGA41AjP zDduz{^xZ$F`Rk8US;Hy+IP2F9e4X_}1K(i%V*_UuJ~i+gtbbwPIo5wP@MYG2Ht@e# z|Bbm?uiDQvUkTOEdG?zy@Xr~aWL)J@=V8h4e}Mh38u%}amyJ9OclVm+Bfr>e7-g>N zDu4Bcn0SsIXm9oXEg86)|CWKP{(uu4wD8$Q9(E$WqomN=55d`XLl>M%5P;M1ny$0$ zRrcze4Zqe#o0Q#3rQ^ML!t8cjzm6}t9)ce)PlaF!aH{QA%fk<m<F`Z4!B0s(2;J%} z4)S(3YaO@c@x=cpqkBO;)bARlEYW~86S3|{zB-YeF!RN9btg?W;~zvD-M9Z5U%*&1 zCzVt6+lJ9l<)0+<r)!CVH2RVoa>N024#`iw4fLK8E#lA2N%(gR-X%pN)0f^26C?L` z`S8wau_T(lPa@WD{-5zgRXoj~zDDN!xBKKo2<~$Io1|!D)~9#4U;UrBkm?iFr}xvW zzl#{<RP#hpQjQOVdXE&Y>MQ;-8v4c0)1;8-eCed(6n_JaO4j{ua=ea_Z2G>>7`LnI zzQytTT_h6J`W5^R3glW+0P`buTw|U3NB?H1c-2>$k6D{Se&G7A@r3C+Nq<F{@w6xA e-9reT<MY2oibm%8DQ~~{5+z3ZC}Q2wjQ<DlLOH<z literal 0 HcmV?d00001 diff --git a/tests/test-abidiff-exit.cc b/tests/test-abidiff-exit.cc index 6ecbfd24..ebac7a00 100644 --- a/tests/test-abidiff-exit.cc +++ b/tests/test-abidiff-exit.cc @@ -158,6 +158,15 @@ InOutSpec in_out_specs[] = "data/test-abidiff-exit/test-leaf3-report.txt", "output/test-abidiff-exit/test-leaf3-report.txt" }, + { + "data/test-abidiff-exit/test-leaf-peeling-v0.o", + "data/test-abidiff-exit/test-leaf-peeling-v1.o", + "", + "--leaf-changes-only", + abigail::tools_utils::ABIDIFF_ABI_CHANGE, + "data/test-abidiff-exit/test-leaf-peeling-report.txt", + "output/test-abidiff-exit/test-leaf-peeling-report.txt" + }, {0, 0, 0 ,0, abigail::tools_utils::ABIDIFF_OK, 0, 0} }; -- 2.25.1.696.g5e7596f4ac-goog ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] abg-ir.cc: Improve types_have_similar_structure. 2020-03-25 14:18 ` [PATCH v2] " Giuliano Procida @ 2020-03-26 11:38 ` Matthias Maennich 2020-03-26 12:14 ` Giuliano Procida 2020-03-26 13:36 ` Dodji Seketeli 1 sibling, 1 reply; 7+ messages in thread From: Matthias Maennich @ 2020-03-26 11:38 UTC (permalink / raw) To: Giuliano Procida; +Cc: libabigail, dodji, kernel-team Hi! On Wed, Mar 25, 2020 at 02:18:59PM +0000, Android Kernel Team wrote: >This function is used to determine whether or not type differences are >"local" and is primarily used in --leaf-changes-only mode. The logic >has some issues which are addressed by this patch: > > - Any number of points-to (*) and refers-to (& and &&) components > are peeled off the types being compared, rather than checking > these match in number and kind. > - This peeling is done with peel_typedef_pointer_or_reference_type > which also peels any number of CV qualifiers (OK) and > array-of ([N]) type components (not OK). > - The function sets a state variable (was_indirect_type) to modify > the behaviour of downstream comparisons, but this cannot be > passed through recursive calls. > >The effect of the indirect_type flag is to switch to comparisons by >name: identically named structs don't get introspected. Arguably, a >more useful behaviour for --leaf-changes-only mode would be to treat >any change to a named type as non-local, except in the context of the >definition of that type itself. This would be a more significant >change in behaviour. > > * include/abg-fwd.h (types_have_similar_structure): In both > overloads, add an indirect_type argument, defaulting to > false. > * src/abg-ir.cc (reference_type_def constructor): Tabify. > (types_have_similar_structure): In both overloads, add an > indirect_type argument and update documentation text. In the > type_base_sptr overload, pass indirect_type in the tail > call. In the type_base* overload, replace was_indirect_type > with indirect_type; peel CV qualifiers and typedefs without > testing as the peel function does this; replace the > indiscriminate peeling of qualifier/pointer/reference/array > type components with code that checks the same > pointer/reference/array type component is used on each side > and makes recursive calls with indirect_type set to true; pass > the indirect_type argument recursively when comparing other > subtypes; move the typeid check earlier, document its purpose > and remove unneccessary checks after later dynamic casts; > remove an always-true conditional; add a TODO for comparing > array types more accurately. > * tests/data/Makefile.am: Add new test case files. > * tests/data/test-abidiff-exit/test-leaf-peeling-v0.cc: New > test case. > * tests/data/test-abidiff-exit/test-leaf-peeling-v1.cc: Ditto. > * tests/data/test-abidiff-exit/test-leaf-peeling-report.txt: > Ditto. > * tests/test-abidiff-exit.cc: Run new test case. > >Signed-off-by: Giuliano Procida <gprocida@google.com> >--- > include/abg-fwd.h | 6 +- > src/abg-ir.cc | 147 ++++++++++-------- > tests/data/Makefile.am | 5 + > .../test-leaf-peeling-report.txt | 38 +++++ > .../test-abidiff-exit/test-leaf-peeling-v0.cc | 24 +++ > .../test-abidiff-exit/test-leaf-peeling-v0.o | Bin 0 -> 3704 bytes > .../test-abidiff-exit/test-leaf-peeling-v1.cc | 24 +++ > .../test-abidiff-exit/test-leaf-peeling-v1.o | Bin 0 -> 3752 bytes > tests/test-abidiff-exit.cc | 9 ++ > 9 files changed, 182 insertions(+), 71 deletions(-) > create mode 100644 tests/data/test-abidiff-exit/test-leaf-peeling-report.txt > create mode 100644 tests/data/test-abidiff-exit/test-leaf-peeling-v0.cc > create mode 100644 tests/data/test-abidiff-exit/test-leaf-peeling-v0.o > create mode 100644 tests/data/test-abidiff-exit/test-leaf-peeling-v1.cc > create mode 100644 tests/data/test-abidiff-exit/test-leaf-peeling-v1.o > >diff --git a/include/abg-fwd.h b/include/abg-fwd.h >index 6f2a862c..1aab70a6 100644 >--- a/include/abg-fwd.h >+++ b/include/abg-fwd.h >@@ -1293,11 +1293,13 @@ function_decl_is_less_than(const function_decl&f, const function_decl &s); > > bool > types_have_similar_structure(const type_base_sptr& first, >- const type_base_sptr& second); >+ const type_base_sptr& second, >+ bool indirect_type = false); > > bool > types_have_similar_structure(const type_base* first, >- const type_base* second); >+ const type_base* second, >+ bool indirect_type = false); > > } // end namespace ir > >diff --git a/src/abg-ir.cc b/src/abg-ir.cc >index 2853fe6c..a10b0bb7 100644 >--- a/src/abg-ir.cc >+++ b/src/abg-ir.cc >@@ -13573,10 +13573,10 @@ reference_type_def::reference_type_def(const type_base_sptr pointed_to, > decl_base_sptr pto = dynamic_pointer_cast<decl_base>(pointed_to); > string name; > if (pto) >- { >- set_visibility(pto->get_visibility()); >- name = string(pto->get_name()) + "&"; >- } >+ { >+ set_visibility(pto->get_visibility()); >+ name = string(pto->get_name()) + "&"; >+ } Correct, but unrelated. No complaints. :-) > else > name = string(get_type_name(is_function_type(pointed_to), > /*qualified_name=*/true)) + "&"; >@@ -22563,33 +22563,44 @@ function_decl_is_less_than(const function_decl &f, const function_decl &s) > /// Test if two types have similar structures, even though they are > /// (or can be) different. > /// >-/// Two indirect types (pointers, references, qualified, typedefs) >-/// have similar structure if their underlying types are of the same >-/// kind and have the same name. In this indirect types case, the >-/// size of the underlying type does not matter. >+/// const and volatile qualifiers are completely ignored. >+/// >+/// typedef are resolved to their definitions; their names are ignored. >+/// >+/// Two indirect types (pointers or references) have similar structure >+/// if their underlying types are of the same kind and have the same >+/// name. In the indirect types case, the size of the underlying type >+/// does not matter. > /// > /// Two direct types (i.e, non indirect) have a similar structure if > /// they have the same kind, name and size. Two class types have >-/// similar structure if they have the same name, size, and if their >-/// data members have similar types. >+/// similar structure if they have the same name, size, and if the >+/// types of their data members have similar types. > /// > /// @param first the first type to consider. > /// > /// @param second the second type to consider. > /// >+/// @param indirect_type whether to do an indirect comparison >+/// > /// @return true iff @p first and @p second have similar structures. > bool > types_have_similar_structure(const type_base_sptr& first, >- const type_base_sptr& second) >-{return types_have_similar_structure(first.get(), second.get());} >+ const type_base_sptr& second, >+ bool indirect_type) >+{return types_have_similar_structure(first.get(), second.get(), indirect_type);} > > /// Test if two types have similar structures, even though they are > /// (or can be) different. > /// >-/// Two indirect types (pointers, references, qualified, typedefs) >-/// have similar structure if their underlying types are of the same >-/// kind and have the same name. In this indirect types case, the >-/// size of the underlying type does not matter. >+/// const and volatile qualifiers are completely ignored. >+/// >+/// typedef are resolved to their definitions; their names are ignored. >+/// >+/// Two indirect types (pointers or references) have similar structure >+/// if their underlying types are of the same kind and have the same >+/// name. In the indirect types case, the size of the underlying type >+/// does not matter. > /// > /// Two direct types (i.e, non indirect) have a similar structure if > /// they have the same kind, name and size. Two class types have >@@ -22600,9 +22611,13 @@ types_have_similar_structure(const type_base_sptr& first, > /// > /// @param second the second type to consider. > /// >+/// @param indirect_type whether to do an indirect comparison >+/// > /// @return true iff @p first and @p second have similar structures. > bool >-types_have_similar_structure(const type_base* first, const type_base* second) >+types_have_similar_structure(const type_base* first, >+ const type_base* second, >+ bool indirect_type) > { > if (!!first != !!second) > return false; >@@ -22610,33 +22625,39 @@ types_have_similar_structure(const type_base* first, const type_base* second) > if (!first) > return false; > >- if (is_typedef(first) || is_qualified_type(first)) >- first = peel_qualified_or_typedef_type(first); >- >- if (is_typedef(second) || is_qualified_type(second)) >- second = peel_qualified_or_typedef_type(second); >+ // Treat typedefs purely as type aliases and ignore CV-qualifiers. >+ first = peel_qualified_or_typedef_type(first); >+ second = peel_qualified_or_typedef_type(second); > >- bool was_indirect_type = (is_pointer_type(first) >- || is_pointer_type(second) >- || is_reference_type(first) >- || is_reference_type(second)); >+ // Eliminate all but N of the N^2 comparison cases. This also guarantees the >+ // various ty2 below cannot be null. >+ if (typeid(*first) != typeid(*second)) >+ return false; Just curious (because I think it is correct): Does it make a measurable difference? > >- if (was_indirect_type) >+ // Peel off matching pointers. >+ if (const pointer_type_def* ty1 = is_pointer_type(first)) A picky compiler might ask you to add some parentheses here. [-Wparentheses] > { >- first = peel_typedef_pointer_or_reference_type(first); >- second = peel_typedef_pointer_or_reference_type(second); >+ const pointer_type_def* ty2 = is_pointer_type(second); >+ return types_have_similar_structure(ty1->get_pointed_to_type(), >+ ty2->get_pointed_to_type(), >+ true); > } > >- if (typeid(*first) != typeid(*second)) >- return false; >+ // Peel off matching references. >+ if (const reference_type_def* ty1 = is_reference_type(first)) As above. >+ { >+ const reference_type_def* ty2 = is_reference_type(second); >+ if (ty1->is_lvalue() != ty2->is_lvalue()) >+ return false; >+ return types_have_similar_structure(ty1->get_pointed_to_type(), >+ ty2->get_pointed_to_type(), >+ true); >+ } > > if (const type_decl* ty1 = is_type_decl(first)) > { > const type_decl* ty2 = is_type_decl(second); >- if (ty2 == 0) >- return false; >- >- if (!was_indirect_type) >+ if (!indirect_type) > if (ty1->get_size_in_bits() != ty2->get_size_in_bits()) > return false; > >@@ -22646,10 +22667,7 @@ types_have_similar_structure(const type_base* first, const type_base* second) > if (const enum_type_decl* ty1 = is_enum_type(first)) > { > const enum_type_decl* ty2 = is_enum_type(second); >- if (ty2 == 0) >- return false; >- >- if (!was_indirect_type) >+ if (!indirect_type) > if (ty1->get_size_in_bits() != ty2->get_size_in_bits()) > return false; > >@@ -22660,20 +22678,16 @@ types_have_similar_structure(const type_base* first, const type_base* second) > if (const class_decl* ty1 = is_class_type(first)) > { > const class_decl* ty2 = is_class_type(second); >- if (ty2 == 0) >- return false; >- > if (!ty1->get_is_anonymous() && !ty2->get_is_anonymous() > && ty1->get_name() != ty2->get_name()) > return false; > >- if (!was_indirect_type) >+ if (!indirect_type) > { >- if (!was_indirect_type) >- if ((ty1->get_size_in_bits() != ty2->get_size_in_bits()) >- || (ty1->get_non_static_data_members().size() >- != ty2->get_non_static_data_members().size())) >- return false; >+ if ((ty1->get_size_in_bits() != ty2->get_size_in_bits()) >+ || (ty1->get_non_static_data_members().size() >+ != ty2->get_non_static_data_members().size())) >+ return false; > > for (class_or_union::data_members::const_iterator > i = ty1->get_non_static_data_members().begin(), >@@ -22685,7 +22699,8 @@ types_have_similar_structure(const type_base* first, const type_base* second) > var_decl_sptr dm1 = *i; > var_decl_sptr dm2 = *j; > if (!types_have_similar_structure(dm1->get_type().get(), >- dm2->get_type().get())) >+ dm2->get_type().get(), >+ indirect_type)) > return false; > } > } >@@ -22696,14 +22711,11 @@ types_have_similar_structure(const type_base* first, const type_base* second) > if (const union_decl* ty1 = is_union_type(first)) > { > const union_decl* ty2 = is_union_type(second); >- if (ty2 == 0) >- return false; >- > if (!ty1->get_is_anonymous() && !ty2->get_is_anonymous() > && ty1->get_name() != ty2->get_name()) > return false; > >- if (!was_indirect_type) >+ if (!indirect_type) > return ty1->get_size_in_bits() == ty2->get_size_in_bits(); > > return true; >@@ -22712,13 +22724,13 @@ types_have_similar_structure(const type_base* first, const type_base* second) > if (const array_type_def* ty1 = is_array_type(first)) > { > const array_type_def* ty2 = is_array_type(second); >- >- if (!was_indirect_type) >- if (ty1->get_size_in_bits() != ty2->get_size_in_bits() >- || ty1->get_dimension_count() != ty2->get_dimension_count() >- || !types_have_similar_structure(ty1->get_element_type(), >- ty2->get_element_type())) >- return false; >+ // TODO: Handle uint32_t[10] vs uint64_t[5] better. >+ if (ty1->get_size_in_bits() != ty2->get_size_in_bits() >+ || ty1->get_dimension_count() != ty2->get_dimension_count() >+ || !types_have_similar_structure(ty1->get_element_type(), >+ ty2->get_element_type(), >+ indirect_type)) >+ return false; > > return true; > } >@@ -22726,14 +22738,12 @@ types_have_similar_structure(const type_base* first, const type_base* second) > if (const array_type_def::subrange_type *ty1 = is_subrange_type(first)) > { > const array_type_def::subrange_type *ty2 = is_subrange_type(second); >- if (!ty2) >- return false; >- > if (ty1->get_upper_bound() != ty2->get_upper_bound() > || ty1->get_lower_bound() != ty2->get_lower_bound() > || ty1->get_language() != ty2->get_language() > || !types_have_similar_structure(ty1->get_underlying_type(), >- ty2->get_underlying_type())) >+ ty2->get_underlying_type(), >+ indirect_type)) > return false; > > return true; >@@ -22742,11 +22752,9 @@ types_have_similar_structure(const type_base* first, const type_base* second) > if (const function_type* ty1 = is_function_type(first)) > { > const function_type* ty2 = is_function_type(second); >- if (!ty2) >- return false; >- > if (!types_have_similar_structure(ty1->get_return_type(), >- ty2->get_return_type())) >+ ty2->get_return_type(), >+ indirect_type)) > return false; > > if (ty1->get_parameters().size() != ty2->get_parameters().size()) >@@ -22759,7 +22767,8 @@ types_have_similar_structure(const type_base* first, const type_base* second) > && j != ty2->get_parameters().end()); > ++i, ++j) > if (!types_have_similar_structure((*i)->get_type(), >- (*j)->get_type())) >+ (*j)->get_type(), >+ indirect_type)) > return false; > > return true; >diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am >index cf7792f1..6d4e2faa 100644 >--- a/tests/data/Makefile.am >+++ b/tests/data/Makefile.am >@@ -130,6 +130,11 @@ test-abidiff-exit/test-leaf3-v0.o \ > test-abidiff-exit/test-leaf3-v1.c \ > test-abidiff-exit/test-leaf3-v1.o \ > test-abidiff-exit/test-leaf3-report.txt \ >+test-abidiff-exit/test-leaf-peeling-v0.cc \ >+test-abidiff-exit/test-leaf-peeling-v0.o \ >+test-abidiff-exit/test-leaf-peeling-v1.cc \ >+test-abidiff-exit/test-leaf-peeling-v1.o \ >+test-abidiff-exit/test-leaf-peeling-report.txt \ > \ > test-diff-dwarf/test0-v0.cc \ > test-diff-dwarf/test0-v0.o \ >diff --git a/tests/data/test-abidiff-exit/test-leaf-peeling-report.txt b/tests/data/test-abidiff-exit/test-leaf-peeling-report.txt >new file mode 100644 >index 00000000..54a26869 >--- /dev/null >+++ b/tests/data/test-abidiff-exit/test-leaf-peeling-report.txt >@@ -0,0 +1,38 @@ >+Leaf changes summary: 4 artifacts changed >+Changed leaf types summary: 4 leaf types 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 foo at test-leaf-peeling.0.cc:1:1' changed: >+ type size changed from 32 to 64 (in bits) >+ there are data member changes: >+ type 'int' of 'foo::z' changed: >+ type name changed from 'int' to 'long int' >+ type size changed from 32 to 64 (in bits) >+ and size changed from 32 to 64 (in bits) (by +32 bits) >+ >+ >+ >+'struct ops1 at test-leaf-peeling.0.cc:5:1' changed: >+ type size hasn't changed >+ there are data member changes: >+ type 'int*' of 'ops1::x' changed: >+ pointer type changed from: 'int*' to: 'int**' >+ >+ >+ >+ >+'struct ops2 at test-leaf-peeling.0.cc:9:1' changed: >+ type size changed from 320 to 640 (in bits) >+ there are data member changes: >+ 'foo ops2::y[10]' size changed from 320 to 640 (in bits) (by +320 bits) >+ >+ >+ >+'struct ops3 at test-leaf-peeling.0.cc:13:1' changed: >+ type size hasn't changed >+ there are data member changes: >+ type 'void (int&)*' of 'ops3::spong' changed: >+ pointer type changed from: 'void (int&)*' to: 'void (int&&)*' >+ >+ >diff --git a/tests/data/test-abidiff-exit/test-leaf-peeling-v0.cc b/tests/data/test-abidiff-exit/test-leaf-peeling-v0.cc >new file mode 100644 >index 00000000..b927d15e >--- /dev/null >+++ b/tests/data/test-abidiff-exit/test-leaf-peeling-v0.cc >@@ -0,0 +1,24 @@ >+struct foo { >+ int z; >+}; >+ >+struct ops1 { >+ int * x; >+}; >+ >+struct ops2 { >+ foo y[10]; >+}; >+ >+struct ops3 { >+ void (*spong)(int & wibble); >+}; >+ >+void register_ops1(ops1*) { >+} >+ >+void register_ops2(ops2*) { >+} >+ >+void register_ops3(ops3*) { >+} >diff --git a/tests/data/test-abidiff-exit/test-leaf-peeling-v0.o b/tests/data/test-abidiff-exit/test-leaf-peeling-v0.o >new file mode 100644 >index 0000000000000000000000000000000000000000..a556a63560122a5e926ab708bb13f0fd40714638 >GIT binary patch >literal 3704 >zcmbtXO>7%g5T56a?R7SGoVcZpf{-JD=nuP%ov0+G4TjJ(5v8h-Dx?-j*Us9pu-B1y >z14$7g5<jB1B7p#@NJ!jz;zo~%3aK}495`{|iiEg;IKa&A8|V4izQ9QPX6Bop_ujm> >zv-Z0eFRvwp044z|aHuf~a6fY>S0b?j1(=3w>$iVhzx~dicV2riTznm~jYR=wlsA?S >zlfj%KC+eLJj0s_`A!ytJkw}w-8OD-LKZGR7rbwD~<i;L|RE8vgR1^`JPl$~90h#wf >zjHgKgNZUAkijPS{1~O(EIj75--=Z>3h{%mUBo5a1I84w0*(i4^a>@}Pj-`k(SyB*p >z|3Yb!hQ!C)xTL!%#Q{e~1f0T>Owtesw0Z$kBLZ$v01a`#OBeuRN<5J(SVnF(HzlUi >z`D17;2*b3jbJjD~vLy;wj4T*ttZo#RaK&SfgJrE)hKW@`=bY-Cnnc?MMx}+=C{VaL >z2OEVm7?zP?xUF$anNw3*M+Ny#B+(bEmX}F#j(B#4J^g5jmE)g1KSREg<eNB-n|KPJ >z1nmZJMGDO+vMRoUiUD-2Lj0Y};uu|Gtc-xr3qrf)xpjNT^IA>6;mkX=S`?j}g-kpR >z#pgv*yqBC^>5T~k6SX>iqqyq_&4%x773tb-UN0?ly+$(#z3ygbCn#NAq!EH0bcH@k >zpH}YE79a}3p-j@ksC8bta;>;}`gCcrxa^djQn5T=S}v95%SF2#?)u(^hUa_TW(`%h >zTid>{w{+G%yI8avMZ4|>`+jY^+wnWQfxYX$)%3UQ(5<$-U|<jBXb)vL6Sj8-j0=%* >z0pQZ=>bc^H7rbiI_2cE(rKeA73!};&iOP%Bzx6VRy}X#6NSk-?G#*8q9-`<)c`ld# >zFtM6Vt)T;AWIInrq*3-)WAjkEGz~DE8r#nA{|p!qz(XDeBi_3jeRcya!^n`0O}81) >z_xAS821H$_r?X&SM;b;(yyF3r0oehJ03+@;12zLD1~7W(rj<HGwMT4T;;b`Ym-r*d >zi5MSsiATLMI?mTn$N2_wC8;~JO!M=O2&d{D;6pSOv#6UB-jw=L4gXE*Gm_JdP*4A) >z;;(k3D+*5euS>nH;hR$bT*IXbUuyW4)bDBdDXIUc;g_ZUlZO8x^`9l@eet>Rk#pTv >zq+eRT5{z?ROX8e|&%LDi)4d^D((vgRhVxn;3HQ#F=8Znz(vX*&=VgEH6*^v&4s?d< >zIaV~B*MC#Pd-ZXG{Wi8taDuQK^KF(wZ!ZLA%MD#{szCrw*K4`XmRH?vY&QLR2W^q; >zR;yj_%|5f+b^Qi*VKoH5Odf~e6yVf4?Y4)X8^`a2o`atOI|$v{8#2h--mG`swkH?< >zKiMqCQw`O3ic(f+K$?qLZzNxp$WEBv8C}&$k@ff|(MIppe~m9;te%sm<N2$aQB$Me >z8}w7OLO}|BB@6P444`{Re%u47ZxUU=N6$(40}1{>ibA^YRkZ0Cxql=tFTYQmLic?U >z4a3%dAQ#H<w0`>P=<BC`qvu2j?#uaakfM<8OZ{!w{NKolxK1=b^+|pH9mFUnuM@?H >z3i3j5-(j5RXZ|)ChQ&Wdi$bFNr4q+6e;<u3t9}<{yo!;m`+gxYS+1V@x{TlJA+f;w >zXZRBo$hE?N<PW9ey40zD^dExbxvsPxeQpZ*PR@TI7fjzn`n90P)0yb+9zsx&_x~m- >V3hDc&yu;%2lo;vjnDs_I{$J;aGn)Va > >literal 0 >HcmV?d00001 > >diff --git a/tests/data/test-abidiff-exit/test-leaf-peeling-v1.cc b/tests/data/test-abidiff-exit/test-leaf-peeling-v1.cc >new file mode 100644 >index 00000000..7c88df9f >--- /dev/null >+++ b/tests/data/test-abidiff-exit/test-leaf-peeling-v1.cc >@@ -0,0 +1,24 @@ >+struct foo { >+ long z; >+}; >+ >+struct ops1 { >+ int ** x; >+}; >+ >+struct ops2 { >+ foo y[10]; >+}; There is no apparent difference in ops2 between the test files. What are we testing? >+ >+struct ops3 { >+ void (*spong)(int && wibble); >+}; >+ >+void register_ops1(ops1*) { >+} >+ >+void register_ops2(ops2*) { >+} >+ >+void register_ops3(ops3*) { >+} Maybe we can add some more test cases: - compare reference types and pointer types - compare int*** and int* (two levels of peeling) - compare uint32_t[10] and uint64_t[5] (with the same TODO as in the code) - nested types, e.g. struct ops1_v0* vs. struct ops1_v1* Nice work. I like it! Cheers, Matthias >diff --git a/tests/data/test-abidiff-exit/test-leaf-peeling-v1.o b/tests/data/test-abidiff-exit/test-leaf-peeling-v1.o >new file mode 100644 >index 0000000000000000000000000000000000000000..e71ade4431f80a78ee73292d60808a2ee5404640 >GIT binary patch >literal 3752 >zcmbtXO=x3P6h1evNnV>K{Yl4Zsf!f+8^*k8(lRq0ZH=wdX;6nE6lF$fU(%N}O!89l >zUbU@=f+Ear6h#DAiVI!25#8%7MK^*A7lMjz-4zt{ocqpAZ*Ni%J&=3O`Oe?H=l;C* >z)vHgf#)JSx0+!%dQz*digU517iY3UxIBcxl{&ns4D@S);_^Q8n3$qQ#09)pb<l|&8 >zqsg&ayA8Gw_9}wbA&8iiFoPW@H#yiMZr8}Z4<a!{5<tRk%CUoEav<LD!^n4C4BBra >zW*vYSN|FR%=W+NDe}i(s+)08tciR3Ml_5ezX6Sq3kop#fVHzN9Q;CFh$`ByVCWt9Y >zu^>K3;BrT3h`qIg0iU2D3Ya)0;1UJU5Ct4`1IA7X*r5O#qJXEVv=|fjXR;|PGo2X| >z<H^ypXw3@CPNlA-9!V{vL>8No2Fs4ra`rOLc;G%rrIs)VyMWFa-8nIWwlYQ~*~!KZ >z3Q^8drokB~XD?ueAxxRkQ>M-cau|-I19okhlVlw6*p%}0<rGVk?=4S}?+E$ECUFrD >z<1a?50W7A`j3(>iv#3}=+bYDLnKX{_L}O<JgkBKlo1R<C?|NRd;n$slQ?1JA_%sYg >zWhj1B#Kr6J>80)%voKM!?bmaAe$c4<-gb`8l|)Fm5Vm$9*hL5Ec=bjQdY#SoZcsR( >z6;Eh$ARTZhk~9a`u5aX4&Yv&L=N6ozQ^*x(3k!w9Y%!N_g?qlYSoeId)2O2AcB(sz >z`<E}~FV5%k^<2K@1_yq1r_=V^dqIBBf2rYb=R>#B^a9wtQJB-GR9v5zB~R&v>0XfT >zg`PBX5}yOOwz6_1_s|nwrQ!O~#Pfwq=Zq!D7Q;luKpZ{;Vt-Uj4=3$AxH0!4PCp`f >zXCBXtz8PCdCsxq`F|s{MMx@j1&qwCIb~g1eERE06AAk245WxNJ2P57+B`53_TKbX2 >z51whW;KW7lnk|qIP*+!i1=^#lK;%}mVhxEz3(`Fp0ao<z^w=yI?!oB#jca|7y+>@8 >zaiuH1#`rzRi4=a?z5BOkO&wX&U!6hKNqVT2O!d`@2&e9C5{NVwG5N3){tfGA41AjP >zDduz{^xZ$F`Rk8US;Hy+IP2F9e4X_}1K(i%V*_UuJ~i+gtbbwPIo5wP@MYG2Ht@e# >z|Bbm?uiDQvUkTOEdG?zy@Xr~aWL)J@=V8h4e}Mh38u%}amyJ9OclVm+Bfr>e7-g>N >zDu4Bcn0SsIXm9oXEg86)|CWKP{(uu4wD8$Q9(E$WqomN=55d`XLl>M%5P;M1ny$0$ >zRrcze4Zqe#o0Q#3rQ^ML!t8cjzm6}t9)ce)PlaF!aH{QA%fk<m<F`Z4!B0s(2;J%} >z4)S(3YaO@c@x=cpqkBO;)bARlEYW~86S3|{zB-YeF!RN9btg?W;~zvD-M9Z5U%*&1 >zCzVt6+lJ9l<)0+<r)!CVH2RVoa>N024#`iw4fLK8E#lA2N%(gR-X%pN)0f^26C?L` >z`S8wau_T(lPa@WD{-5zgRXoj~zDDN!xBKKo2<~$Io1|!D)~9#4U;UrBkm?iFr}xvW >zzl#{<RP#hpQjQOVdXE&Y>MQ;-8v4c0)1;8-eCed(6n_JaO4j{ua=ea_Z2G>>7`LnI >zzQytTT_h6J`W5^R3glW+0P`buTw|U3NB?H1c-2>$k6D{Se&G7A@r3C+Nq<F{@w6xA >e-9reT<MY2oibm%8DQ~~{5+z3ZC}Q2wjQ<DlLOH<z > >literal 0 >HcmV?d00001 > >diff --git a/tests/test-abidiff-exit.cc b/tests/test-abidiff-exit.cc >index 6ecbfd24..ebac7a00 100644 >--- a/tests/test-abidiff-exit.cc >+++ b/tests/test-abidiff-exit.cc >@@ -158,6 +158,15 @@ InOutSpec in_out_specs[] = > "data/test-abidiff-exit/test-leaf3-report.txt", > "output/test-abidiff-exit/test-leaf3-report.txt" > }, >+ { >+ "data/test-abidiff-exit/test-leaf-peeling-v0.o", >+ "data/test-abidiff-exit/test-leaf-peeling-v1.o", >+ "", >+ "--leaf-changes-only", >+ abigail::tools_utils::ABIDIFF_ABI_CHANGE, >+ "data/test-abidiff-exit/test-leaf-peeling-report.txt", >+ "output/test-abidiff-exit/test-leaf-peeling-report.txt" >+ }, > {0, 0, 0 ,0, abigail::tools_utils::ABIDIFF_OK, 0, 0} > }; > >-- >2.25.1.696.g5e7596f4ac-goog > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] abg-ir.cc: Improve types_have_similar_structure. 2020-03-26 11:38 ` Matthias Maennich @ 2020-03-26 12:14 ` Giuliano Procida 2020-03-26 12:46 ` Matthias Maennich 0 siblings, 1 reply; 7+ messages in thread From: Giuliano Procida @ 2020-03-26 12:14 UTC (permalink / raw) To: Matthias Maennich; +Cc: libabigail, Dodji Seketeli, kernel-team Hi. On Thu, 26 Mar 2020 at 11:38, Matthias Maennich <maennich@google.com> wrote: > > Hi! > > On Wed, Mar 25, 2020 at 02:18:59PM +0000, Android Kernel Team wrote: > >This function is used to determine whether or not type differences are > >"local" and is primarily used in --leaf-changes-only mode. The logic > >has some issues which are addressed by this patch: > > > > - Any number of points-to (*) and refers-to (& and &&) components > > are peeled off the types being compared, rather than checking > > these match in number and kind. > > - This peeling is done with peel_typedef_pointer_or_reference_type > > which also peels any number of CV qualifiers (OK) and > > array-of ([N]) type components (not OK). > > - The function sets a state variable (was_indirect_type) to modify > > the behaviour of downstream comparisons, but this cannot be > > passed through recursive calls. > > > >The effect of the indirect_type flag is to switch to comparisons by > >name: identically named structs don't get introspected. Arguably, a > >more useful behaviour for --leaf-changes-only mode would be to treat > >any change to a named type as non-local, except in the context of the > >definition of that type itself. This would be a more significant > >change in behaviour. > > > > * include/abg-fwd.h (types_have_similar_structure): In both > > overloads, add an indirect_type argument, defaulting to > > false. > > * src/abg-ir.cc (reference_type_def constructor): Tabify. > > (types_have_similar_structure): In both overloads, add an > > indirect_type argument and update documentation text. In the > > type_base_sptr overload, pass indirect_type in the tail > > call. In the type_base* overload, replace was_indirect_type > > with indirect_type; peel CV qualifiers and typedefs without > > testing as the peel function does this; replace the > > indiscriminate peeling of qualifier/pointer/reference/array > > type components with code that checks the same > > pointer/reference/array type component is used on each side > > and makes recursive calls with indirect_type set to true; pass > > the indirect_type argument recursively when comparing other > > subtypes; move the typeid check earlier, document its purpose > > and remove unneccessary checks after later dynamic casts; > > remove an always-true conditional; add a TODO for comparing > > array types more accurately. > > * tests/data/Makefile.am: Add new test case files. > > * tests/data/test-abidiff-exit/test-leaf-peeling-v0.cc: New > > test case. > > * tests/data/test-abidiff-exit/test-leaf-peeling-v1.cc: Ditto. > > * tests/data/test-abidiff-exit/test-leaf-peeling-report.txt: > > Ditto. > > * tests/test-abidiff-exit.cc: Run new test case. > > > >Signed-off-by: Giuliano Procida <gprocida@google.com> > >--- > > include/abg-fwd.h | 6 +- > > src/abg-ir.cc | 147 ++++++++++-------- > > tests/data/Makefile.am | 5 + > > .../test-leaf-peeling-report.txt | 38 +++++ > > .../test-abidiff-exit/test-leaf-peeling-v0.cc | 24 +++ > > .../test-abidiff-exit/test-leaf-peeling-v0.o | Bin 0 -> 3704 bytes > > .../test-abidiff-exit/test-leaf-peeling-v1.cc | 24 +++ > > .../test-abidiff-exit/test-leaf-peeling-v1.o | Bin 0 -> 3752 bytes > > tests/test-abidiff-exit.cc | 9 ++ > > 9 files changed, 182 insertions(+), 71 deletions(-) > > create mode 100644 tests/data/test-abidiff-exit/test-leaf-peeling-report.txt > > create mode 100644 tests/data/test-abidiff-exit/test-leaf-peeling-v0.cc > > create mode 100644 tests/data/test-abidiff-exit/test-leaf-peeling-v0.o > > create mode 100644 tests/data/test-abidiff-exit/test-leaf-peeling-v1.cc > > create mode 100644 tests/data/test-abidiff-exit/test-leaf-peeling-v1.o > > > >diff --git a/include/abg-fwd.h b/include/abg-fwd.h > >index 6f2a862c..1aab70a6 100644 > >--- a/include/abg-fwd.h > >+++ b/include/abg-fwd.h > >@@ -1293,11 +1293,13 @@ function_decl_is_less_than(const function_decl&f, const function_decl &s); > > > > bool > > types_have_similar_structure(const type_base_sptr& first, > >- const type_base_sptr& second); > >+ const type_base_sptr& second, > >+ bool indirect_type = false); > > > > bool > > types_have_similar_structure(const type_base* first, > >- const type_base* second); > >+ const type_base* second, > >+ bool indirect_type = false); > > > > } // end namespace ir > > > >diff --git a/src/abg-ir.cc b/src/abg-ir.cc > >index 2853fe6c..a10b0bb7 100644 > >--- a/src/abg-ir.cc > >+++ b/src/abg-ir.cc > >@@ -13573,10 +13573,10 @@ reference_type_def::reference_type_def(const type_base_sptr pointed_to, > > decl_base_sptr pto = dynamic_pointer_cast<decl_base>(pointed_to); > > string name; > > if (pto) > >- { > >- set_visibility(pto->get_visibility()); > >- name = string(pto->get_name()) + "&"; > >- } > >+ { > >+ set_visibility(pto->get_visibility()); > >+ name = string(pto->get_name()) + "&"; > >+ } > > Correct, but unrelated. No complaints. :-) > > > else > > name = string(get_type_name(is_function_type(pointed_to), > > /*qualified_name=*/true)) + "&"; > >@@ -22563,33 +22563,44 @@ function_decl_is_less_than(const function_decl &f, const function_decl &s) > > /// Test if two types have similar structures, even though they are > > /// (or can be) different. > > /// > >-/// Two indirect types (pointers, references, qualified, typedefs) > >-/// have similar structure if their underlying types are of the same > >-/// kind and have the same name. In this indirect types case, the > >-/// size of the underlying type does not matter. > >+/// const and volatile qualifiers are completely ignored. > >+/// > >+/// typedef are resolved to their definitions; their names are ignored. > >+/// > >+/// Two indirect types (pointers or references) have similar structure > >+/// if their underlying types are of the same kind and have the same > >+/// name. In the indirect types case, the size of the underlying type > >+/// does not matter. > > /// > > /// Two direct types (i.e, non indirect) have a similar structure if > > /// they have the same kind, name and size. Two class types have > >-/// similar structure if they have the same name, size, and if their > >-/// data members have similar types. > >+/// similar structure if they have the same name, size, and if the > >+/// types of their data members have similar types. > > /// > > /// @param first the first type to consider. > > /// > > /// @param second the second type to consider. > > /// > >+/// @param indirect_type whether to do an indirect comparison > >+/// > > /// @return true iff @p first and @p second have similar structures. > > bool > > types_have_similar_structure(const type_base_sptr& first, > >- const type_base_sptr& second) > >-{return types_have_similar_structure(first.get(), second.get());} > >+ const type_base_sptr& second, > >+ bool indirect_type) > >+{return types_have_similar_structure(first.get(), second.get(), indirect_type);} > > > > /// Test if two types have similar structures, even though they are > > /// (or can be) different. > > /// > >-/// Two indirect types (pointers, references, qualified, typedefs) > >-/// have similar structure if their underlying types are of the same > >-/// kind and have the same name. In this indirect types case, the > >-/// size of the underlying type does not matter. > >+/// const and volatile qualifiers are completely ignored. > >+/// > >+/// typedef are resolved to their definitions; their names are ignored. > >+/// > >+/// Two indirect types (pointers or references) have similar structure > >+/// if their underlying types are of the same kind and have the same > >+/// name. In the indirect types case, the size of the underlying type > >+/// does not matter. > > /// > > /// Two direct types (i.e, non indirect) have a similar structure if > > /// they have the same kind, name and size. Two class types have > >@@ -22600,9 +22611,13 @@ types_have_similar_structure(const type_base_sptr& first, > > /// > > /// @param second the second type to consider. > > /// > >+/// @param indirect_type whether to do an indirect comparison > >+/// > > /// @return true iff @p first and @p second have similar structures. > > bool > >-types_have_similar_structure(const type_base* first, const type_base* second) > >+types_have_similar_structure(const type_base* first, > >+ const type_base* second, > >+ bool indirect_type) > > { > > if (!!first != !!second) > > return false; > >@@ -22610,33 +22625,39 @@ types_have_similar_structure(const type_base* first, const type_base* second) > > if (!first) > > return false; > > > >- if (is_typedef(first) || is_qualified_type(first)) > >- first = peel_qualified_or_typedef_type(first); > >- > >- if (is_typedef(second) || is_qualified_type(second)) > >- second = peel_qualified_or_typedef_type(second); > >+ // Treat typedefs purely as type aliases and ignore CV-qualifiers. > >+ first = peel_qualified_or_typedef_type(first); > >+ second = peel_qualified_or_typedef_type(second); > > > >- bool was_indirect_type = (is_pointer_type(first) > >- || is_pointer_type(second) > >- || is_reference_type(first) > >- || is_reference_type(second)); > >+ // Eliminate all but N of the N^2 comparison cases. This also guarantees the > >+ // various ty2 below cannot be null. > >+ if (typeid(*first) != typeid(*second)) > >+ return false; > > Just curious (because I think it is correct): Does it make a measurable > difference? The code was there before, after the peeling. However, various ty2 checks were still there, including ones I'd added. Once I went and reminded myself what typeid meant, the linear code made a lot more sense. > > > >- if (was_indirect_type) > >+ // Peel off matching pointers. > >+ if (const pointer_type_def* ty1 = is_pointer_type(first)) > > A picky compiler might ask you to add some parentheses here. [-Wparentheses] -Wparentheses is already enabled by -Wall which is enabled by ABIGAIL_DEVEL=1. In any case, there is no ambiguity here. > > { > >- first = peel_typedef_pointer_or_reference_type(first); > >- second = peel_typedef_pointer_or_reference_type(second); > >+ const pointer_type_def* ty2 = is_pointer_type(second); > >+ return types_have_similar_structure(ty1->get_pointed_to_type(), > >+ ty2->get_pointed_to_type(), > >+ true); > > } > > > >- if (typeid(*first) != typeid(*second)) > >- return false; > >+ // Peel off matching references. > >+ if (const reference_type_def* ty1 = is_reference_type(first)) > > As above. > > >+ { > >+ const reference_type_def* ty2 = is_reference_type(second); > >+ if (ty1->is_lvalue() != ty2->is_lvalue()) > >+ return false; > >+ return types_have_similar_structure(ty1->get_pointed_to_type(), > >+ ty2->get_pointed_to_type(), > >+ true); > >+ } > > > > if (const type_decl* ty1 = is_type_decl(first)) > > { > > const type_decl* ty2 = is_type_decl(second); > >- if (ty2 == 0) > >- return false; > >- > >- if (!was_indirect_type) > >+ if (!indirect_type) > > if (ty1->get_size_in_bits() != ty2->get_size_in_bits()) > > return false; > > > >@@ -22646,10 +22667,7 @@ types_have_similar_structure(const type_base* first, const type_base* second) > > if (const enum_type_decl* ty1 = is_enum_type(first)) > > { > > const enum_type_decl* ty2 = is_enum_type(second); > >- if (ty2 == 0) > >- return false; > >- > >- if (!was_indirect_type) > >+ if (!indirect_type) > > if (ty1->get_size_in_bits() != ty2->get_size_in_bits()) > > return false; > > > >@@ -22660,20 +22678,16 @@ types_have_similar_structure(const type_base* first, const type_base* second) > > if (const class_decl* ty1 = is_class_type(first)) > > { > > const class_decl* ty2 = is_class_type(second); > >- if (ty2 == 0) > >- return false; > >- > > if (!ty1->get_is_anonymous() && !ty2->get_is_anonymous() > > && ty1->get_name() != ty2->get_name()) > > return false; > > > >- if (!was_indirect_type) > >+ if (!indirect_type) > > { > >- if (!was_indirect_type) > >- if ((ty1->get_size_in_bits() != ty2->get_size_in_bits()) > >- || (ty1->get_non_static_data_members().size() > >- != ty2->get_non_static_data_members().size())) > >- return false; > >+ if ((ty1->get_size_in_bits() != ty2->get_size_in_bits()) > >+ || (ty1->get_non_static_data_members().size() > >+ != ty2->get_non_static_data_members().size())) > >+ return false; > > > > for (class_or_union::data_members::const_iterator > > i = ty1->get_non_static_data_members().begin(), > >@@ -22685,7 +22699,8 @@ types_have_similar_structure(const type_base* first, const type_base* second) > > var_decl_sptr dm1 = *i; > > var_decl_sptr dm2 = *j; > > if (!types_have_similar_structure(dm1->get_type().get(), > >- dm2->get_type().get())) > >+ dm2->get_type().get(), > >+ indirect_type)) > > return false; > > } > > } > >@@ -22696,14 +22711,11 @@ types_have_similar_structure(const type_base* first, const type_base* second) > > if (const union_decl* ty1 = is_union_type(first)) > > { > > const union_decl* ty2 = is_union_type(second); > >- if (ty2 == 0) > >- return false; > >- > > if (!ty1->get_is_anonymous() && !ty2->get_is_anonymous() > > && ty1->get_name() != ty2->get_name()) > > return false; > > > >- if (!was_indirect_type) > >+ if (!indirect_type) > > return ty1->get_size_in_bits() == ty2->get_size_in_bits(); > > > > return true; > >@@ -22712,13 +22724,13 @@ types_have_similar_structure(const type_base* first, const type_base* second) > > if (const array_type_def* ty1 = is_array_type(first)) > > { > > const array_type_def* ty2 = is_array_type(second); > >- > >- if (!was_indirect_type) > >- if (ty1->get_size_in_bits() != ty2->get_size_in_bits() > >- || ty1->get_dimension_count() != ty2->get_dimension_count() > >- || !types_have_similar_structure(ty1->get_element_type(), > >- ty2->get_element_type())) > >- return false; > >+ // TODO: Handle uint32_t[10] vs uint64_t[5] better. > >+ if (ty1->get_size_in_bits() != ty2->get_size_in_bits() > >+ || ty1->get_dimension_count() != ty2->get_dimension_count() > >+ || !types_have_similar_structure(ty1->get_element_type(), > >+ ty2->get_element_type(), > >+ indirect_type)) > >+ return false; > > > > return true; > > } > >@@ -22726,14 +22738,12 @@ types_have_similar_structure(const type_base* first, const type_base* second) > > if (const array_type_def::subrange_type *ty1 = is_subrange_type(first)) > > { > > const array_type_def::subrange_type *ty2 = is_subrange_type(second); > >- if (!ty2) > >- return false; > >- > > if (ty1->get_upper_bound() != ty2->get_upper_bound() > > || ty1->get_lower_bound() != ty2->get_lower_bound() > > || ty1->get_language() != ty2->get_language() > > || !types_have_similar_structure(ty1->get_underlying_type(), > >- ty2->get_underlying_type())) > >+ ty2->get_underlying_type(), > >+ indirect_type)) > > return false; > > > > return true; > >@@ -22742,11 +22752,9 @@ types_have_similar_structure(const type_base* first, const type_base* second) > > if (const function_type* ty1 = is_function_type(first)) > > { > > const function_type* ty2 = is_function_type(second); > >- if (!ty2) > >- return false; > >- > > if (!types_have_similar_structure(ty1->get_return_type(), > >- ty2->get_return_type())) > >+ ty2->get_return_type(), > >+ indirect_type)) > > return false; > > > > if (ty1->get_parameters().size() != ty2->get_parameters().size()) > >@@ -22759,7 +22767,8 @@ types_have_similar_structure(const type_base* first, const type_base* second) > > && j != ty2->get_parameters().end()); > > ++i, ++j) > > if (!types_have_similar_structure((*i)->get_type(), > >- (*j)->get_type())) > >+ (*j)->get_type(), > >+ indirect_type)) > > return false; > > > > return true; > >diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am > >index cf7792f1..6d4e2faa 100644 > >--- a/tests/data/Makefile.am > >+++ b/tests/data/Makefile.am > >@@ -130,6 +130,11 @@ test-abidiff-exit/test-leaf3-v0.o \ > > test-abidiff-exit/test-leaf3-v1.c \ > > test-abidiff-exit/test-leaf3-v1.o \ > > test-abidiff-exit/test-leaf3-report.txt \ > >+test-abidiff-exit/test-leaf-peeling-v0.cc \ > >+test-abidiff-exit/test-leaf-peeling-v0.o \ > >+test-abidiff-exit/test-leaf-peeling-v1.cc \ > >+test-abidiff-exit/test-leaf-peeling-v1.o \ > >+test-abidiff-exit/test-leaf-peeling-report.txt \ > > \ > > test-diff-dwarf/test0-v0.cc \ > > test-diff-dwarf/test0-v0.o \ > >diff --git a/tests/data/test-abidiff-exit/test-leaf-peeling-report.txt b/tests/data/test-abidiff-exit/test-leaf-peeling-report.txt > >new file mode 100644 > >index 00000000..54a26869 > >--- /dev/null > >+++ b/tests/data/test-abidiff-exit/test-leaf-peeling-report.txt > >@@ -0,0 +1,38 @@ > >+Leaf changes summary: 4 artifacts changed > >+Changed leaf types summary: 4 leaf types 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 foo at test-leaf-peeling.0.cc:1:1' changed: > >+ type size changed from 32 to 64 (in bits) > >+ there are data member changes: > >+ type 'int' of 'foo::z' changed: > >+ type name changed from 'int' to 'long int' > >+ type size changed from 32 to 64 (in bits) > >+ and size changed from 32 to 64 (in bits) (by +32 bits) > >+ > >+ > >+ > >+'struct ops1 at test-leaf-peeling.0.cc:5:1' changed: > >+ type size hasn't changed > >+ there are data member changes: > >+ type 'int*' of 'ops1::x' changed: > >+ pointer type changed from: 'int*' to: 'int**' > >+ > >+ > >+ > >+ > >+'struct ops2 at test-leaf-peeling.0.cc:9:1' changed: > >+ type size changed from 320 to 640 (in bits) > >+ there are data member changes: > >+ 'foo ops2::y[10]' size changed from 320 to 640 (in bits) (by +320 bits) > >+ > >+ > >+ > >+'struct ops3 at test-leaf-peeling.0.cc:13:1' changed: > >+ type size hasn't changed > >+ there are data member changes: > >+ type 'void (int&)*' of 'ops3::spong' changed: > >+ pointer type changed from: 'void (int&)*' to: 'void (int&&)*' > >+ > >+ > >diff --git a/tests/data/test-abidiff-exit/test-leaf-peeling-v0.cc b/tests/data/test-abidiff-exit/test-leaf-peeling-v0.cc > >new file mode 100644 > >index 00000000..b927d15e > >--- /dev/null > >+++ b/tests/data/test-abidiff-exit/test-leaf-peeling-v0.cc > >@@ -0,0 +1,24 @@ > >+struct foo { > >+ int z; > >+}; > >+ > >+struct ops1 { > >+ int * x; > >+}; > >+ > >+struct ops2 { > >+ foo y[10]; > >+}; > >+ > >+struct ops3 { > >+ void (*spong)(int & wibble); > >+}; > >+ > >+void register_ops1(ops1*) { > >+} > >+ > >+void register_ops2(ops2*) { > >+} > >+ > >+void register_ops3(ops3*) { > >+} > >diff --git a/tests/data/test-abidiff-exit/test-leaf-peeling-v0.o b/tests/data/test-abidiff-exit/test-leaf-peeling-v0.o > >new file mode 100644 > >index 0000000000000000000000000000000000000000..a556a63560122a5e926ab708bb13f0fd40714638 > >GIT binary patch > >literal 3704 > >zcmbtXO>7%g5T56a?R7SGoVcZpf{-JD=nuP%ov0+G4TjJ(5v8h-Dx?-j*Us9pu-B1y > >z14$7g5<jB1B7p#@NJ!jz;zo~%3aK}495`{|iiEg;IKa&A8|V4izQ9QPX6Bop_ujm> > >zv-Z0eFRvwp044z|aHuf~a6fY>S0b?j1(=3w>$iVhzx~dicV2riTznm~jYR=wlsA?S > >zlfj%KC+eLJj0s_`A!ytJkw}w-8OD-LKZGR7rbwD~<i;L|RE8vgR1^`JPl$~90h#wf > >zjHgKgNZUAkijPS{1~O(EIj75--=Z>3h{%mUBo5a1I84w0*(i4^a>@}Pj-`k(SyB*p > >z|3Yb!hQ!C)xTL!%#Q{e~1f0T>Owtesw0Z$kBLZ$v01a`#OBeuRN<5J(SVnF(HzlUi > >z`D17;2*b3jbJjD~vLy;wj4T*ttZo#RaK&SfgJrE)hKW@`=bY-Cnnc?MMx}+=C{VaL > >z2OEVm7?zP?xUF$anNw3*M+Ny#B+(bEmX}F#j(B#4J^g5jmE)g1KSREg<eNB-n|KPJ > >z1nmZJMGDO+vMRoUiUD-2Lj0Y};uu|Gtc-xr3qrf)xpjNT^IA>6;mkX=S`?j}g-kpR > >z#pgv*yqBC^>5T~k6SX>iqqyq_&4%x773tb-UN0?ly+$(#z3ygbCn#NAq!EH0bcH@k > >zpH}YE79a}3p-j@ksC8bta;>;}`gCcrxa^djQn5T=S}v95%SF2#?)u(^hUa_TW(`%h > >zTid>{w{+G%yI8avMZ4|>`+jY^+wnWQfxYX$)%3UQ(5<$-U|<jBXb)vL6Sj8-j0=%* > >z0pQZ=>bc^H7rbiI_2cE(rKeA73!};&iOP%Bzx6VRy}X#6NSk-?G#*8q9-`<)c`ld# > >zFtM6Vt)T;AWIInrq*3-)WAjkEGz~DE8r#nA{|p!qz(XDeBi_3jeRcya!^n`0O}81) > >z_xAS821H$_r?X&SM;b;(yyF3r0oehJ03+@;12zLD1~7W(rj<HGwMT4T;;b`Ym-r*d > >zi5MSsiATLMI?mTn$N2_wC8;~JO!M=O2&d{D;6pSOv#6UB-jw=L4gXE*Gm_JdP*4A) > >z;;(k3D+*5euS>nH;hR$bT*IXbUuyW4)bDBdDXIUc;g_ZUlZO8x^`9l@eet>Rk#pTv > >zq+eRT5{z?ROX8e|&%LDi)4d^D((vgRhVxn;3HQ#F=8Znz(vX*&=VgEH6*^v&4s?d< > >zIaV~B*MC#Pd-ZXG{Wi8taDuQK^KF(wZ!ZLA%MD#{szCrw*K4`XmRH?vY&QLR2W^q; > >zR;yj_%|5f+b^Qi*VKoH5Odf~e6yVf4?Y4)X8^`a2o`atOI|$v{8#2h--mG`swkH?< > >zKiMqCQw`O3ic(f+K$?qLZzNxp$WEBv8C}&$k@ff|(MIppe~m9;te%sm<N2$aQB$Me > >z8}w7OLO}|BB@6P444`{Re%u47ZxUU=N6$(40}1{>ibA^YRkZ0Cxql=tFTYQmLic?U > >z4a3%dAQ#H<w0`>P=<BC`qvu2j?#uaakfM<8OZ{!w{NKolxK1=b^+|pH9mFUnuM@?H > >z3i3j5-(j5RXZ|)ChQ&Wdi$bFNr4q+6e;<u3t9}<{yo!;m`+gxYS+1V@x{TlJA+f;w > >zXZRBo$hE?N<PW9ey40zD^dExbxvsPxeQpZ*PR@TI7fjzn`n90P)0yb+9zsx&_x~m- > >V3hDc&yu;%2lo;vjnDs_I{$J;aGn)Va > > > >literal 0 > >HcmV?d00001 > > > >diff --git a/tests/data/test-abidiff-exit/test-leaf-peeling-v1.cc b/tests/data/test-abidiff-exit/test-leaf-peeling-v1.cc > >new file mode 100644 > >index 00000000..7c88df9f > >--- /dev/null > >+++ b/tests/data/test-abidiff-exit/test-leaf-peeling-v1.cc > >@@ -0,0 +1,24 @@ > >+struct foo { > >+ long z; > >+}; > >+ > >+struct ops1 { > >+ int ** x; > >+}; > >+ > >+struct ops2 { > >+ foo y[10]; > >+}; > > There is no apparent difference in ops2 between the test files. What are > we testing? We are testing the reporting of an indirect change via its dependency on struct foo (which is reported already). We could (and I would) argue that the change to ops2 is not local and people doing kernel ABI diffs might agree as well. However, at the moment, size changes are considered as local. If y had been a pointer something something foo, the size would not have changed and this would not have been reported. I'll add a comment to the test code. > >+ > >+struct ops3 { > >+ void (*spong)(int && wibble); > >+}; > >+ > >+void register_ops1(ops1*) { > >+} > >+ > >+void register_ops2(ops2*) { > >+} > >+ > >+void register_ops3(ops3*) { > >+} > > Maybe we can add some more test cases: > > - compare reference types and pointer types > - compare int*** and int* (two levels of peeling) > - compare uint32_t[10] and uint64_t[5] (with the same TODO as in the code) > - nested types, e.g. struct ops1_v0* vs. struct ops1_v1* Agreed. Will do. > Nice work. I like it! > > Cheers, > Matthias Thank you, Giuliano. > >diff --git a/tests/data/test-abidiff-exit/test-leaf-peeling-v1.o b/tests/data/test-abidiff-exit/test-leaf-peeling-v1.o > >new file mode 100644 > >index 0000000000000000000000000000000000000000..e71ade4431f80a78ee73292d60808a2ee5404640 > >GIT binary patch > >literal 3752 > >zcmbtXO=x3P6h1evNnV>K{Yl4Zsf!f+8^*k8(lRq0ZH=wdX;6nE6lF$fU(%N}O!89l > >zUbU@=f+Ear6h#DAiVI!25#8%7MK^*A7lMjz-4zt{ocqpAZ*Ni%J&=3O`Oe?H=l;C* > >z)vHgf#)JSx0+!%dQz*digU517iY3UxIBcxl{&ns4D@S);_^Q8n3$qQ#09)pb<l|&8 > >zqsg&ayA8Gw_9}wbA&8iiFoPW@H#yiMZr8}Z4<a!{5<tRk%CUoEav<LD!^n4C4BBra > >zW*vYSN|FR%=W+NDe}i(s+)08tciR3Ml_5ezX6Sq3kop#fVHzN9Q;CFh$`ByVCWt9Y > >zu^>K3;BrT3h`qIg0iU2D3Ya)0;1UJU5Ct4`1IA7X*r5O#qJXEVv=|fjXR;|PGo2X| > >z<H^ypXw3@CPNlA-9!V{vL>8No2Fs4ra`rOLc;G%rrIs)VyMWFa-8nIWwlYQ~*~!KZ > >z3Q^8drokB~XD?ueAxxRkQ>M-cau|-I19okhlVlw6*p%}0<rGVk?=4S}?+E$ECUFrD > >z<1a?50W7A`j3(>iv#3}=+bYDLnKX{_L}O<JgkBKlo1R<C?|NRd;n$slQ?1JA_%sYg > >zWhj1B#Kr6J>80)%voKM!?bmaAe$c4<-gb`8l|)Fm5Vm$9*hL5Ec=bjQdY#SoZcsR( > >z6;Eh$ARTZhk~9a`u5aX4&Yv&L=N6ozQ^*x(3k!w9Y%!N_g?qlYSoeId)2O2AcB(sz > >z`<E}~FV5%k^<2K@1_yq1r_=V^dqIBBf2rYb=R>#B^a9wtQJB-GR9v5zB~R&v>0XfT > >zg`PBX5}yOOwz6_1_s|nwrQ!O~#Pfwq=Zq!D7Q;luKpZ{;Vt-Uj4=3$AxH0!4PCp`f > >zXCBXtz8PCdCsxq`F|s{MMx@j1&qwCIb~g1eERE06AAk245WxNJ2P57+B`53_TKbX2 > >z51whW;KW7lnk|qIP*+!i1=^#lK;%}mVhxEz3(`Fp0ao<z^w=yI?!oB#jca|7y+>@8 > >zaiuH1#`rzRi4=a?z5BOkO&wX&U!6hKNqVT2O!d`@2&e9C5{NVwG5N3){tfGA41AjP > >zDduz{^xZ$F`Rk8US;Hy+IP2F9e4X_}1K(i%V*_UuJ~i+gtbbwPIo5wP@MYG2Ht@e# > >z|Bbm?uiDQvUkTOEdG?zy@Xr~aWL)J@=V8h4e}Mh38u%}amyJ9OclVm+Bfr>e7-g>N > >zDu4Bcn0SsIXm9oXEg86)|CWKP{(uu4wD8$Q9(E$WqomN=55d`XLl>M%5P;M1ny$0$ > >zRrcze4Zqe#o0Q#3rQ^ML!t8cjzm6}t9)ce)PlaF!aH{QA%fk<m<F`Z4!B0s(2;J%} > >z4)S(3YaO@c@x=cpqkBO;)bARlEYW~86S3|{zB-YeF!RN9btg?W;~zvD-M9Z5U%*&1 > >zCzVt6+lJ9l<)0+<r)!CVH2RVoa>N024#`iw4fLK8E#lA2N%(gR-X%pN)0f^26C?L` > >z`S8wau_T(lPa@WD{-5zgRXoj~zDDN!xBKKo2<~$Io1|!D)~9#4U;UrBkm?iFr}xvW > >zzl#{<RP#hpQjQOVdXE&Y>MQ;-8v4c0)1;8-eCed(6n_JaO4j{ua=ea_Z2G>>7`LnI > >zzQytTT_h6J`W5^R3glW+0P`buTw|U3NB?H1c-2>$k6D{Se&G7A@r3C+Nq<F{@w6xA > >e-9reT<MY2oibm%8DQ~~{5+z3ZC}Q2wjQ<DlLOH<z > > > >literal 0 > >HcmV?d00001 > > > >diff --git a/tests/test-abidiff-exit.cc b/tests/test-abidiff-exit.cc > >index 6ecbfd24..ebac7a00 100644 > >--- a/tests/test-abidiff-exit.cc > >+++ b/tests/test-abidiff-exit.cc > >@@ -158,6 +158,15 @@ InOutSpec in_out_specs[] = > > "data/test-abidiff-exit/test-leaf3-report.txt", > > "output/test-abidiff-exit/test-leaf3-report.txt" > > }, > >+ { > >+ "data/test-abidiff-exit/test-leaf-peeling-v0.o", > >+ "data/test-abidiff-exit/test-leaf-peeling-v1.o", > >+ "", > >+ "--leaf-changes-only", > >+ abigail::tools_utils::ABIDIFF_ABI_CHANGE, > >+ "data/test-abidiff-exit/test-leaf-peeling-report.txt", > >+ "output/test-abidiff-exit/test-leaf-peeling-report.txt" > >+ }, > > {0, 0, 0 ,0, abigail::tools_utils::ABIDIFF_OK, 0, 0} > > }; > > > >-- > >2.25.1.696.g5e7596f4ac-goog > > > > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] abg-ir.cc: Improve types_have_similar_structure. 2020-03-26 12:14 ` Giuliano Procida @ 2020-03-26 12:46 ` Matthias Maennich 0 siblings, 0 replies; 7+ messages in thread From: Matthias Maennich @ 2020-03-26 12:46 UTC (permalink / raw) To: Giuliano Procida; +Cc: libabigail, Dodji Seketeli, kernel-team On Thu, Mar 26, 2020 at 12:14:20PM +0000, Giuliano Procida wrote: >Hi. > >On Thu, 26 Mar 2020 at 11:38, Matthias Maennich <maennich@google.com> wrote: >> >> Hi! >> >> On Wed, Mar 25, 2020 at 02:18:59PM +0000, Android Kernel Team wrote: >> >This function is used to determine whether or not type differences are >> >"local" and is primarily used in --leaf-changes-only mode. The logic >> >has some issues which are addressed by this patch: >> > >> > - Any number of points-to (*) and refers-to (& and &&) components >> > are peeled off the types being compared, rather than checking >> > these match in number and kind. >> > - This peeling is done with peel_typedef_pointer_or_reference_type >> > which also peels any number of CV qualifiers (OK) and >> > array-of ([N]) type components (not OK). >> > - The function sets a state variable (was_indirect_type) to modify >> > the behaviour of downstream comparisons, but this cannot be >> > passed through recursive calls. >> > >> >The effect of the indirect_type flag is to switch to comparisons by >> >name: identically named structs don't get introspected. Arguably, a >> >more useful behaviour for --leaf-changes-only mode would be to treat >> >any change to a named type as non-local, except in the context of the >> >definition of that type itself. This would be a more significant >> >change in behaviour. >> > >> > * include/abg-fwd.h (types_have_similar_structure): In both >> > overloads, add an indirect_type argument, defaulting to >> > false. >> > * src/abg-ir.cc (reference_type_def constructor): Tabify. >> > (types_have_similar_structure): In both overloads, add an >> > indirect_type argument and update documentation text. In the >> > type_base_sptr overload, pass indirect_type in the tail >> > call. In the type_base* overload, replace was_indirect_type >> > with indirect_type; peel CV qualifiers and typedefs without >> > testing as the peel function does this; replace the >> > indiscriminate peeling of qualifier/pointer/reference/array >> > type components with code that checks the same >> > pointer/reference/array type component is used on each side >> > and makes recursive calls with indirect_type set to true; pass >> > the indirect_type argument recursively when comparing other >> > subtypes; move the typeid check earlier, document its purpose >> > and remove unneccessary checks after later dynamic casts; >> > remove an always-true conditional; add a TODO for comparing >> > array types more accurately. >> > * tests/data/Makefile.am: Add new test case files. >> > * tests/data/test-abidiff-exit/test-leaf-peeling-v0.cc: New >> > test case. >> > * tests/data/test-abidiff-exit/test-leaf-peeling-v1.cc: Ditto. >> > * tests/data/test-abidiff-exit/test-leaf-peeling-report.txt: >> > Ditto. >> > * tests/test-abidiff-exit.cc: Run new test case. >> > >> >Signed-off-by: Giuliano Procida <gprocida@google.com> >> >--- >> > include/abg-fwd.h | 6 +- >> > src/abg-ir.cc | 147 ++++++++++-------- >> > tests/data/Makefile.am | 5 + >> > .../test-leaf-peeling-report.txt | 38 +++++ >> > .../test-abidiff-exit/test-leaf-peeling-v0.cc | 24 +++ >> > .../test-abidiff-exit/test-leaf-peeling-v0.o | Bin 0 -> 3704 bytes >> > .../test-abidiff-exit/test-leaf-peeling-v1.cc | 24 +++ >> > .../test-abidiff-exit/test-leaf-peeling-v1.o | Bin 0 -> 3752 bytes >> > tests/test-abidiff-exit.cc | 9 ++ >> > 9 files changed, 182 insertions(+), 71 deletions(-) >> > create mode 100644 tests/data/test-abidiff-exit/test-leaf-peeling-report.txt >> > create mode 100644 tests/data/test-abidiff-exit/test-leaf-peeling-v0.cc >> > create mode 100644 tests/data/test-abidiff-exit/test-leaf-peeling-v0.o >> > create mode 100644 tests/data/test-abidiff-exit/test-leaf-peeling-v1.cc >> > create mode 100644 tests/data/test-abidiff-exit/test-leaf-peeling-v1.o >> > >> >diff --git a/include/abg-fwd.h b/include/abg-fwd.h >> >index 6f2a862c..1aab70a6 100644 >> >--- a/include/abg-fwd.h >> >+++ b/include/abg-fwd.h >> >@@ -1293,11 +1293,13 @@ function_decl_is_less_than(const function_decl&f, const function_decl &s); >> > >> > bool >> > types_have_similar_structure(const type_base_sptr& first, >> >- const type_base_sptr& second); >> >+ const type_base_sptr& second, >> >+ bool indirect_type = false); >> > >> > bool >> > types_have_similar_structure(const type_base* first, >> >- const type_base* second); >> >+ const type_base* second, >> >+ bool indirect_type = false); >> > >> > } // end namespace ir >> > >> >diff --git a/src/abg-ir.cc b/src/abg-ir.cc >> >index 2853fe6c..a10b0bb7 100644 >> >--- a/src/abg-ir.cc >> >+++ b/src/abg-ir.cc >> >@@ -13573,10 +13573,10 @@ reference_type_def::reference_type_def(const type_base_sptr pointed_to, >> > decl_base_sptr pto = dynamic_pointer_cast<decl_base>(pointed_to); >> > string name; >> > if (pto) >> >- { >> >- set_visibility(pto->get_visibility()); >> >- name = string(pto->get_name()) + "&"; >> >- } >> >+ { >> >+ set_visibility(pto->get_visibility()); >> >+ name = string(pto->get_name()) + "&"; >> >+ } >> >> Correct, but unrelated. No complaints. :-) >> >> > else >> > name = string(get_type_name(is_function_type(pointed_to), >> > /*qualified_name=*/true)) + "&"; >> >@@ -22563,33 +22563,44 @@ function_decl_is_less_than(const function_decl &f, const function_decl &s) >> > /// Test if two types have similar structures, even though they are >> > /// (or can be) different. >> > /// >> >-/// Two indirect types (pointers, references, qualified, typedefs) >> >-/// have similar structure if their underlying types are of the same >> >-/// kind and have the same name. In this indirect types case, the >> >-/// size of the underlying type does not matter. >> >+/// const and volatile qualifiers are completely ignored. >> >+/// >> >+/// typedef are resolved to their definitions; their names are ignored. >> >+/// >> >+/// Two indirect types (pointers or references) have similar structure >> >+/// if their underlying types are of the same kind and have the same >> >+/// name. In the indirect types case, the size of the underlying type >> >+/// does not matter. >> > /// >> > /// Two direct types (i.e, non indirect) have a similar structure if >> > /// they have the same kind, name and size. Two class types have >> >-/// similar structure if they have the same name, size, and if their >> >-/// data members have similar types. >> >+/// similar structure if they have the same name, size, and if the >> >+/// types of their data members have similar types. >> > /// >> > /// @param first the first type to consider. >> > /// >> > /// @param second the second type to consider. >> > /// >> >+/// @param indirect_type whether to do an indirect comparison >> >+/// >> > /// @return true iff @p first and @p second have similar structures. >> > bool >> > types_have_similar_structure(const type_base_sptr& first, >> >- const type_base_sptr& second) >> >-{return types_have_similar_structure(first.get(), second.get());} >> >+ const type_base_sptr& second, >> >+ bool indirect_type) >> >+{return types_have_similar_structure(first.get(), second.get(), indirect_type);} >> > >> > /// Test if two types have similar structures, even though they are >> > /// (or can be) different. >> > /// >> >-/// Two indirect types (pointers, references, qualified, typedefs) >> >-/// have similar structure if their underlying types are of the same >> >-/// kind and have the same name. In this indirect types case, the >> >-/// size of the underlying type does not matter. >> >+/// const and volatile qualifiers are completely ignored. >> >+/// >> >+/// typedef are resolved to their definitions; their names are ignored. >> >+/// >> >+/// Two indirect types (pointers or references) have similar structure >> >+/// if their underlying types are of the same kind and have the same >> >+/// name. In the indirect types case, the size of the underlying type >> >+/// does not matter. >> > /// >> > /// Two direct types (i.e, non indirect) have a similar structure if >> > /// they have the same kind, name and size. Two class types have >> >@@ -22600,9 +22611,13 @@ types_have_similar_structure(const type_base_sptr& first, >> > /// >> > /// @param second the second type to consider. >> > /// >> >+/// @param indirect_type whether to do an indirect comparison >> >+/// >> > /// @return true iff @p first and @p second have similar structures. >> > bool >> >-types_have_similar_structure(const type_base* first, const type_base* second) >> >+types_have_similar_structure(const type_base* first, >> >+ const type_base* second, >> >+ bool indirect_type) >> > { >> > if (!!first != !!second) >> > return false; >> >@@ -22610,33 +22625,39 @@ types_have_similar_structure(const type_base* first, const type_base* second) >> > if (!first) >> > return false; >> > >> >- if (is_typedef(first) || is_qualified_type(first)) >> >- first = peel_qualified_or_typedef_type(first); >> >- >> >- if (is_typedef(second) || is_qualified_type(second)) >> >- second = peel_qualified_or_typedef_type(second); >> >+ // Treat typedefs purely as type aliases and ignore CV-qualifiers. >> >+ first = peel_qualified_or_typedef_type(first); >> >+ second = peel_qualified_or_typedef_type(second); >> > >> >- bool was_indirect_type = (is_pointer_type(first) >> >- || is_pointer_type(second) >> >- || is_reference_type(first) >> >- || is_reference_type(second)); >> >+ // Eliminate all but N of the N^2 comparison cases. This also guarantees the >> >+ // various ty2 below cannot be null. >> >+ if (typeid(*first) != typeid(*second)) >> >+ return false; >> >> Just curious (because I think it is correct): Does it make a measurable >> difference? > >The code was there before, after the peeling. However, various ty2 >checks were still there, including ones I'd added. Once I went and >reminded myself what typeid meant, the linear code made a lot more >sense. > >> > >> >- if (was_indirect_type) >> >+ // Peel off matching pointers. >> >+ if (const pointer_type_def* ty1 = is_pointer_type(first)) >> >> A picky compiler might ask you to add some parentheses here. [-Wparentheses] > >-Wparentheses is already enabled by -Wall which is enabled by ABIGAIL_DEVEL=1. > >In any case, there is no ambiguity here. > >> > { >> >- first = peel_typedef_pointer_or_reference_type(first); >> >- second = peel_typedef_pointer_or_reference_type(second); >> >+ const pointer_type_def* ty2 = is_pointer_type(second); >> >+ return types_have_similar_structure(ty1->get_pointed_to_type(), >> >+ ty2->get_pointed_to_type(), >> >+ true); >> > } >> > >> >- if (typeid(*first) != typeid(*second)) >> >- return false; >> >+ // Peel off matching references. >> >+ if (const reference_type_def* ty1 = is_reference_type(first)) >> >> As above. >> >> >+ { >> >+ const reference_type_def* ty2 = is_reference_type(second); >> >+ if (ty1->is_lvalue() != ty2->is_lvalue()) >> >+ return false; >> >+ return types_have_similar_structure(ty1->get_pointed_to_type(), >> >+ ty2->get_pointed_to_type(), >> >+ true); >> >+ } >> > >> > if (const type_decl* ty1 = is_type_decl(first)) >> > { >> > const type_decl* ty2 = is_type_decl(second); >> >- if (ty2 == 0) >> >- return false; >> >- >> >- if (!was_indirect_type) >> >+ if (!indirect_type) >> > if (ty1->get_size_in_bits() != ty2->get_size_in_bits()) >> > return false; >> > >> >@@ -22646,10 +22667,7 @@ types_have_similar_structure(const type_base* first, const type_base* second) >> > if (const enum_type_decl* ty1 = is_enum_type(first)) >> > { >> > const enum_type_decl* ty2 = is_enum_type(second); >> >- if (ty2 == 0) >> >- return false; >> >- >> >- if (!was_indirect_type) >> >+ if (!indirect_type) >> > if (ty1->get_size_in_bits() != ty2->get_size_in_bits()) >> > return false; >> > >> >@@ -22660,20 +22678,16 @@ types_have_similar_structure(const type_base* first, const type_base* second) >> > if (const class_decl* ty1 = is_class_type(first)) >> > { >> > const class_decl* ty2 = is_class_type(second); >> >- if (ty2 == 0) >> >- return false; >> >- >> > if (!ty1->get_is_anonymous() && !ty2->get_is_anonymous() >> > && ty1->get_name() != ty2->get_name()) >> > return false; >> > >> >- if (!was_indirect_type) >> >+ if (!indirect_type) >> > { >> >- if (!was_indirect_type) >> >- if ((ty1->get_size_in_bits() != ty2->get_size_in_bits()) >> >- || (ty1->get_non_static_data_members().size() >> >- != ty2->get_non_static_data_members().size())) >> >- return false; >> >+ if ((ty1->get_size_in_bits() != ty2->get_size_in_bits()) >> >+ || (ty1->get_non_static_data_members().size() >> >+ != ty2->get_non_static_data_members().size())) >> >+ return false; >> > >> > for (class_or_union::data_members::const_iterator >> > i = ty1->get_non_static_data_members().begin(), >> >@@ -22685,7 +22699,8 @@ types_have_similar_structure(const type_base* first, const type_base* second) >> > var_decl_sptr dm1 = *i; >> > var_decl_sptr dm2 = *j; >> > if (!types_have_similar_structure(dm1->get_type().get(), >> >- dm2->get_type().get())) >> >+ dm2->get_type().get(), >> >+ indirect_type)) >> > return false; >> > } >> > } >> >@@ -22696,14 +22711,11 @@ types_have_similar_structure(const type_base* first, const type_base* second) >> > if (const union_decl* ty1 = is_union_type(first)) >> > { >> > const union_decl* ty2 = is_union_type(second); >> >- if (ty2 == 0) >> >- return false; >> >- >> > if (!ty1->get_is_anonymous() && !ty2->get_is_anonymous() >> > && ty1->get_name() != ty2->get_name()) >> > return false; >> > >> >- if (!was_indirect_type) >> >+ if (!indirect_type) >> > return ty1->get_size_in_bits() == ty2->get_size_in_bits(); >> > >> > return true; >> >@@ -22712,13 +22724,13 @@ types_have_similar_structure(const type_base* first, const type_base* second) >> > if (const array_type_def* ty1 = is_array_type(first)) >> > { >> > const array_type_def* ty2 = is_array_type(second); >> >- >> >- if (!was_indirect_type) >> >- if (ty1->get_size_in_bits() != ty2->get_size_in_bits() >> >- || ty1->get_dimension_count() != ty2->get_dimension_count() >> >- || !types_have_similar_structure(ty1->get_element_type(), >> >- ty2->get_element_type())) >> >- return false; >> >+ // TODO: Handle uint32_t[10] vs uint64_t[5] better. >> >+ if (ty1->get_size_in_bits() != ty2->get_size_in_bits() >> >+ || ty1->get_dimension_count() != ty2->get_dimension_count() >> >+ || !types_have_similar_structure(ty1->get_element_type(), >> >+ ty2->get_element_type(), >> >+ indirect_type)) >> >+ return false; >> > >> > return true; >> > } >> >@@ -22726,14 +22738,12 @@ types_have_similar_structure(const type_base* first, const type_base* second) >> > if (const array_type_def::subrange_type *ty1 = is_subrange_type(first)) >> > { >> > const array_type_def::subrange_type *ty2 = is_subrange_type(second); >> >- if (!ty2) >> >- return false; >> >- >> > if (ty1->get_upper_bound() != ty2->get_upper_bound() >> > || ty1->get_lower_bound() != ty2->get_lower_bound() >> > || ty1->get_language() != ty2->get_language() >> > || !types_have_similar_structure(ty1->get_underlying_type(), >> >- ty2->get_underlying_type())) >> >+ ty2->get_underlying_type(), >> >+ indirect_type)) >> > return false; >> > >> > return true; >> >@@ -22742,11 +22752,9 @@ types_have_similar_structure(const type_base* first, const type_base* second) >> > if (const function_type* ty1 = is_function_type(first)) >> > { >> > const function_type* ty2 = is_function_type(second); >> >- if (!ty2) >> >- return false; >> >- >> > if (!types_have_similar_structure(ty1->get_return_type(), >> >- ty2->get_return_type())) >> >+ ty2->get_return_type(), >> >+ indirect_type)) >> > return false; >> > >> > if (ty1->get_parameters().size() != ty2->get_parameters().size()) >> >@@ -22759,7 +22767,8 @@ types_have_similar_structure(const type_base* first, const type_base* second) >> > && j != ty2->get_parameters().end()); >> > ++i, ++j) >> > if (!types_have_similar_structure((*i)->get_type(), >> >- (*j)->get_type())) >> >+ (*j)->get_type(), >> >+ indirect_type)) >> > return false; >> > >> > return true; >> >diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am >> >index cf7792f1..6d4e2faa 100644 >> >--- a/tests/data/Makefile.am >> >+++ b/tests/data/Makefile.am >> >@@ -130,6 +130,11 @@ test-abidiff-exit/test-leaf3-v0.o \ >> > test-abidiff-exit/test-leaf3-v1.c \ >> > test-abidiff-exit/test-leaf3-v1.o \ >> > test-abidiff-exit/test-leaf3-report.txt \ >> >+test-abidiff-exit/test-leaf-peeling-v0.cc \ >> >+test-abidiff-exit/test-leaf-peeling-v0.o \ >> >+test-abidiff-exit/test-leaf-peeling-v1.cc \ >> >+test-abidiff-exit/test-leaf-peeling-v1.o \ >> >+test-abidiff-exit/test-leaf-peeling-report.txt \ >> > \ >> > test-diff-dwarf/test0-v0.cc \ >> > test-diff-dwarf/test0-v0.o \ >> >diff --git a/tests/data/test-abidiff-exit/test-leaf-peeling-report.txt b/tests/data/test-abidiff-exit/test-leaf-peeling-report.txt >> >new file mode 100644 >> >index 00000000..54a26869 >> >--- /dev/null >> >+++ b/tests/data/test-abidiff-exit/test-leaf-peeling-report.txt >> >@@ -0,0 +1,38 @@ >> >+Leaf changes summary: 4 artifacts changed >> >+Changed leaf types summary: 4 leaf types 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 foo at test-leaf-peeling.0.cc:1:1' changed: >> >+ type size changed from 32 to 64 (in bits) >> >+ there are data member changes: >> >+ type 'int' of 'foo::z' changed: >> >+ type name changed from 'int' to 'long int' >> >+ type size changed from 32 to 64 (in bits) >> >+ and size changed from 32 to 64 (in bits) (by +32 bits) >> >+ >> >+ >> >+ >> >+'struct ops1 at test-leaf-peeling.0.cc:5:1' changed: >> >+ type size hasn't changed >> >+ there are data member changes: >> >+ type 'int*' of 'ops1::x' changed: >> >+ pointer type changed from: 'int*' to: 'int**' >> >+ >> >+ >> >+ >> >+ >> >+'struct ops2 at test-leaf-peeling.0.cc:9:1' changed: >> >+ type size changed from 320 to 640 (in bits) >> >+ there are data member changes: >> >+ 'foo ops2::y[10]' size changed from 320 to 640 (in bits) (by +320 bits) >> >+ >> >+ >> >+ >> >+'struct ops3 at test-leaf-peeling.0.cc:13:1' changed: >> >+ type size hasn't changed >> >+ there are data member changes: >> >+ type 'void (int&)*' of 'ops3::spong' changed: >> >+ pointer type changed from: 'void (int&)*' to: 'void (int&&)*' >> >+ >> >+ >> >diff --git a/tests/data/test-abidiff-exit/test-leaf-peeling-v0.cc b/tests/data/test-abidiff-exit/test-leaf-peeling-v0.cc >> >new file mode 100644 >> >index 00000000..b927d15e >> >--- /dev/null >> >+++ b/tests/data/test-abidiff-exit/test-leaf-peeling-v0.cc >> >@@ -0,0 +1,24 @@ >> >+struct foo { >> >+ int z; >> >+}; >> >+ >> >+struct ops1 { >> >+ int * x; >> >+}; >> >+ >> >+struct ops2 { >> >+ foo y[10]; >> >+}; >> >+ >> >+struct ops3 { >> >+ void (*spong)(int & wibble); >> >+}; >> >+ >> >+void register_ops1(ops1*) { >> >+} >> >+ >> >+void register_ops2(ops2*) { >> >+} >> >+ >> >+void register_ops3(ops3*) { >> >+} >> >diff --git a/tests/data/test-abidiff-exit/test-leaf-peeling-v0.o b/tests/data/test-abidiff-exit/test-leaf-peeling-v0.o >> >new file mode 100644 >> >index 0000000000000000000000000000000000000000..a556a63560122a5e926ab708bb13f0fd40714638 >> >GIT binary patch >> >literal 3704 >> >zcmbtXO>7%g5T56a?R7SGoVcZpf{-JD=nuP%ov0+G4TjJ(5v8h-Dx?-j*Us9pu-B1y >> >z14$7g5<jB1B7p#@NJ!jz;zo~%3aK}495`{|iiEg;IKa&A8|V4izQ9QPX6Bop_ujm> >> >zv-Z0eFRvwp044z|aHuf~a6fY>S0b?j1(=3w>$iVhzx~dicV2riTznm~jYR=wlsA?S >> >zlfj%KC+eLJj0s_`A!ytJkw}w-8OD-LKZGR7rbwD~<i;L|RE8vgR1^`JPl$~90h#wf >> >zjHgKgNZUAkijPS{1~O(EIj75--=Z>3h{%mUBo5a1I84w0*(i4^a>@}Pj-`k(SyB*p >> >z|3Yb!hQ!C)xTL!%#Q{e~1f0T>Owtesw0Z$kBLZ$v01a`#OBeuRN<5J(SVnF(HzlUi >> >z`D17;2*b3jbJjD~vLy;wj4T*ttZo#RaK&SfgJrE)hKW@`=bY-Cnnc?MMx}+=C{VaL >> >z2OEVm7?zP?xUF$anNw3*M+Ny#B+(bEmX}F#j(B#4J^g5jmE)g1KSREg<eNB-n|KPJ >> >z1nmZJMGDO+vMRoUiUD-2Lj0Y};uu|Gtc-xr3qrf)xpjNT^IA>6;mkX=S`?j}g-kpR >> >z#pgv*yqBC^>5T~k6SX>iqqyq_&4%x773tb-UN0?ly+$(#z3ygbCn#NAq!EH0bcH@k >> >zpH}YE79a}3p-j@ksC8bta;>;}`gCcrxa^djQn5T=S}v95%SF2#?)u(^hUa_TW(`%h >> >zTid>{w{+G%yI8avMZ4|>`+jY^+wnWQfxYX$)%3UQ(5<$-U|<jBXb)vL6Sj8-j0=%* >> >z0pQZ=>bc^H7rbiI_2cE(rKeA73!};&iOP%Bzx6VRy}X#6NSk-?G#*8q9-`<)c`ld# >> >zFtM6Vt)T;AWIInrq*3-)WAjkEGz~DE8r#nA{|p!qz(XDeBi_3jeRcya!^n`0O}81) >> >z_xAS821H$_r?X&SM;b;(yyF3r0oehJ03+@;12zLD1~7W(rj<HGwMT4T;;b`Ym-r*d >> >zi5MSsiATLMI?mTn$N2_wC8;~JO!M=O2&d{D;6pSOv#6UB-jw=L4gXE*Gm_JdP*4A) >> >z;;(k3D+*5euS>nH;hR$bT*IXbUuyW4)bDBdDXIUc;g_ZUlZO8x^`9l@eet>Rk#pTv >> >zq+eRT5{z?ROX8e|&%LDi)4d^D((vgRhVxn;3HQ#F=8Znz(vX*&=VgEH6*^v&4s?d< >> >zIaV~B*MC#Pd-ZXG{Wi8taDuQK^KF(wZ!ZLA%MD#{szCrw*K4`XmRH?vY&QLR2W^q; >> >zR;yj_%|5f+b^Qi*VKoH5Odf~e6yVf4?Y4)X8^`a2o`atOI|$v{8#2h--mG`swkH?< >> >zKiMqCQw`O3ic(f+K$?qLZzNxp$WEBv8C}&$k@ff|(MIppe~m9;te%sm<N2$aQB$Me >> >z8}w7OLO}|BB@6P444`{Re%u47ZxUU=N6$(40}1{>ibA^YRkZ0Cxql=tFTYQmLic?U >> >z4a3%dAQ#H<w0`>P=<BC`qvu2j?#uaakfM<8OZ{!w{NKolxK1=b^+|pH9mFUnuM@?H >> >z3i3j5-(j5RXZ|)ChQ&Wdi$bFNr4q+6e;<u3t9}<{yo!;m`+gxYS+1V@x{TlJA+f;w >> >zXZRBo$hE?N<PW9ey40zD^dExbxvsPxeQpZ*PR@TI7fjzn`n90P)0yb+9zsx&_x~m- >> >V3hDc&yu;%2lo;vjnDs_I{$J;aGn)Va >> > >> >literal 0 >> >HcmV?d00001 >> > >> >diff --git a/tests/data/test-abidiff-exit/test-leaf-peeling-v1.cc b/tests/data/test-abidiff-exit/test-leaf-peeling-v1.cc >> >new file mode 100644 >> >index 00000000..7c88df9f >> >--- /dev/null >> >+++ b/tests/data/test-abidiff-exit/test-leaf-peeling-v1.cc >> >@@ -0,0 +1,24 @@ >> >+struct foo { >> >+ long z; >> >+}; >> >+ >> >+struct ops1 { >> >+ int ** x; >> >+}; >> >+ >> >+struct ops2 { >> >+ foo y[10]; >> >+}; >> >> There is no apparent difference in ops2 between the test files. What are >> we testing? > >We are testing the reporting of an indirect change via its dependency >on struct foo (which is reported already). > >We could (and I would) argue that the change to ops2 is not local and >people doing kernel ABI diffs might agree as well. However, at the >moment, size changes are considered as local. If y had been a pointer >something something foo, the size would not have changed and this >would not have been reported. I'll add a comment to the test code. I agree that this should not be considered local then. Dodji, what do you think? Cheers, Matthias > >> >+ >> >+struct ops3 { >> >+ void (*spong)(int && wibble); >> >+}; >> >+ >> >+void register_ops1(ops1*) { >> >+} >> >+ >> >+void register_ops2(ops2*) { >> >+} >> >+ >> >+void register_ops3(ops3*) { >> >+} >> >> Maybe we can add some more test cases: >> >> - compare reference types and pointer types >> - compare int*** and int* (two levels of peeling) >> - compare uint32_t[10] and uint64_t[5] (with the same TODO as in the code) >> - nested types, e.g. struct ops1_v0* vs. struct ops1_v1* > >Agreed. Will do. > >> Nice work. I like it! >> >> Cheers, >> Matthias > >Thank you, >Giuliano. > >> >diff --git a/tests/data/test-abidiff-exit/test-leaf-peeling-v1.o b/tests/data/test-abidiff-exit/test-leaf-peeling-v1.o >> >new file mode 100644 >> >index 0000000000000000000000000000000000000000..e71ade4431f80a78ee73292d60808a2ee5404640 >> >GIT binary patch >> >literal 3752 >> >zcmbtXO=x3P6h1evNnV>K{Yl4Zsf!f+8^*k8(lRq0ZH=wdX;6nE6lF$fU(%N}O!89l >> >zUbU@=f+Ear6h#DAiVI!25#8%7MK^*A7lMjz-4zt{ocqpAZ*Ni%J&=3O`Oe?H=l;C* >> >z)vHgf#)JSx0+!%dQz*digU517iY3UxIBcxl{&ns4D@S);_^Q8n3$qQ#09)pb<l|&8 >> >zqsg&ayA8Gw_9}wbA&8iiFoPW@H#yiMZr8}Z4<a!{5<tRk%CUoEav<LD!^n4C4BBra >> >zW*vYSN|FR%=W+NDe}i(s+)08tciR3Ml_5ezX6Sq3kop#fVHzN9Q;CFh$`ByVCWt9Y >> >zu^>K3;BrT3h`qIg0iU2D3Ya)0;1UJU5Ct4`1IA7X*r5O#qJXEVv=|fjXR;|PGo2X| >> >z<H^ypXw3@CPNlA-9!V{vL>8No2Fs4ra`rOLc;G%rrIs)VyMWFa-8nIWwlYQ~*~!KZ >> >z3Q^8drokB~XD?ueAxxRkQ>M-cau|-I19okhlVlw6*p%}0<rGVk?=4S}?+E$ECUFrD >> >z<1a?50W7A`j3(>iv#3}=+bYDLnKX{_L}O<JgkBKlo1R<C?|NRd;n$slQ?1JA_%sYg >> >zWhj1B#Kr6J>80)%voKM!?bmaAe$c4<-gb`8l|)Fm5Vm$9*hL5Ec=bjQdY#SoZcsR( >> >z6;Eh$ARTZhk~9a`u5aX4&Yv&L=N6ozQ^*x(3k!w9Y%!N_g?qlYSoeId)2O2AcB(sz >> >z`<E}~FV5%k^<2K@1_yq1r_=V^dqIBBf2rYb=R>#B^a9wtQJB-GR9v5zB~R&v>0XfT >> >zg`PBX5}yOOwz6_1_s|nwrQ!O~#Pfwq=Zq!D7Q;luKpZ{;Vt-Uj4=3$AxH0!4PCp`f >> >zXCBXtz8PCdCsxq`F|s{MMx@j1&qwCIb~g1eERE06AAk245WxNJ2P57+B`53_TKbX2 >> >z51whW;KW7lnk|qIP*+!i1=^#lK;%}mVhxEz3(`Fp0ao<z^w=yI?!oB#jca|7y+>@8 >> >zaiuH1#`rzRi4=a?z5BOkO&wX&U!6hKNqVT2O!d`@2&e9C5{NVwG5N3){tfGA41AjP >> >zDduz{^xZ$F`Rk8US;Hy+IP2F9e4X_}1K(i%V*_UuJ~i+gtbbwPIo5wP@MYG2Ht@e# >> >z|Bbm?uiDQvUkTOEdG?zy@Xr~aWL)J@=V8h4e}Mh38u%}amyJ9OclVm+Bfr>e7-g>N >> >zDu4Bcn0SsIXm9oXEg86)|CWKP{(uu4wD8$Q9(E$WqomN=55d`XLl>M%5P;M1ny$0$ >> >zRrcze4Zqe#o0Q#3rQ^ML!t8cjzm6}t9)ce)PlaF!aH{QA%fk<m<F`Z4!B0s(2;J%} >> >z4)S(3YaO@c@x=cpqkBO;)bARlEYW~86S3|{zB-YeF!RN9btg?W;~zvD-M9Z5U%*&1 >> >zCzVt6+lJ9l<)0+<r)!CVH2RVoa>N024#`iw4fLK8E#lA2N%(gR-X%pN)0f^26C?L` >> >z`S8wau_T(lPa@WD{-5zgRXoj~zDDN!xBKKo2<~$Io1|!D)~9#4U;UrBkm?iFr}xvW >> >zzl#{<RP#hpQjQOVdXE&Y>MQ;-8v4c0)1;8-eCed(6n_JaO4j{ua=ea_Z2G>>7`LnI >> >zzQytTT_h6J`W5^R3glW+0P`buTw|U3NB?H1c-2>$k6D{Se&G7A@r3C+Nq<F{@w6xA >> >e-9reT<MY2oibm%8DQ~~{5+z3ZC}Q2wjQ<DlLOH<z >> > >> >literal 0 >> >HcmV?d00001 >> > >> >diff --git a/tests/test-abidiff-exit.cc b/tests/test-abidiff-exit.cc >> >index 6ecbfd24..ebac7a00 100644 >> >--- a/tests/test-abidiff-exit.cc >> >+++ b/tests/test-abidiff-exit.cc >> >@@ -158,6 +158,15 @@ InOutSpec in_out_specs[] = >> > "data/test-abidiff-exit/test-leaf3-report.txt", >> > "output/test-abidiff-exit/test-leaf3-report.txt" >> > }, >> >+ { >> >+ "data/test-abidiff-exit/test-leaf-peeling-v0.o", >> >+ "data/test-abidiff-exit/test-leaf-peeling-v1.o", >> >+ "", >> >+ "--leaf-changes-only", >> >+ abigail::tools_utils::ABIDIFF_ABI_CHANGE, >> >+ "data/test-abidiff-exit/test-leaf-peeling-report.txt", >> >+ "output/test-abidiff-exit/test-leaf-peeling-report.txt" >> >+ }, >> > {0, 0, 0 ,0, abigail::tools_utils::ABIDIFF_OK, 0, 0} >> > }; >> > >> >-- >> >2.25.1.696.g5e7596f4ac-goog >> > >> > ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] abg-ir.cc: Improve types_have_similar_structure. 2020-03-25 14:18 ` [PATCH v2] " Giuliano Procida 2020-03-26 11:38 ` Matthias Maennich @ 2020-03-26 13:36 ` Dodji Seketeli 1 sibling, 0 replies; 7+ messages in thread From: Dodji Seketeli @ 2020-03-26 13:36 UTC (permalink / raw) To: Giuliano Procida; +Cc: libabigail, kernel-team Hello Giuliano, Giuliano Procida <gprocida@google.com> a écrit: > This function is used to determine whether or not type differences are > "local" and is primarily used in --leaf-changes-only mode. The logic > has some issues which are addressed by this patch: > > - Any number of points-to (*) and refers-to (& and &&) components > are peeled off the types being compared, rather than checking > these match in number and kind. > - This peeling is done with peel_typedef_pointer_or_reference_type > which also peels any number of CV qualifiers (OK) and > array-of ([N]) type components (not OK). > - The function sets a state variable (was_indirect_type) to modify > the behaviour of downstream comparisons, but this cannot be > passed through recursive calls. > > The effect of the indirect_type flag is to switch to comparisons by > name: identically named structs don't get introspected. Arguably, a > more useful behaviour for --leaf-changes-only mode would be to treat > any change to a named type as non-local, except in the context of the > definition of that type itself. This would be a more significant > change in behaviour. > > * include/abg-fwd.h (types_have_similar_structure): In both > overloads, add an indirect_type argument, defaulting to > false. > * src/abg-ir.cc (reference_type_def constructor): Tabify. > (types_have_similar_structure): In both overloads, add an > indirect_type argument and update documentation text. In the > type_base_sptr overload, pass indirect_type in the tail > call. In the type_base* overload, replace was_indirect_type > with indirect_type; peel CV qualifiers and typedefs without > testing as the peel function does this; replace the > indiscriminate peeling of qualifier/pointer/reference/array > type components with code that checks the same > pointer/reference/array type component is used on each side > and makes recursive calls with indirect_type set to true; pass > the indirect_type argument recursively when comparing other > subtypes; move the typeid check earlier, document its purpose > and remove unneccessary checks after later dynamic casts; > remove an always-true conditional; add a TODO for comparing > array types more accurately. > * tests/data/Makefile.am: Add new test case files. > * tests/data/test-abidiff-exit/test-leaf-peeling-v0.cc: New > test case. > * tests/data/test-abidiff-exit/test-leaf-peeling-v1.cc: Ditto. > * tests/data/test-abidiff-exit/test-leaf-peeling-report.txt: > Ditto. > * tests/test-abidiff-exit.cc: Run new test case. Applied to master. Thanks! -- Dodji ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-03-26 13:36 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-03-24 13:29 [PATCH] abg-ir.cc: Improve types_have_similar_structure Giuliano Procida 2020-03-25 14:13 ` Giuliano Procida 2020-03-25 14:18 ` [PATCH v2] " Giuliano Procida 2020-03-26 11:38 ` Matthias Maennich 2020-03-26 12:14 ` Giuliano Procida 2020-03-26 12:46 ` Matthias Maennich 2020-03-26 13:36 ` Dodji Seketeli
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).