From patchwork Wed Mar 25 14:18:59 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Giuliano Procida X-Patchwork-Id: 39054 From: gprocida@google.com (Giuliano Procida) Date: Wed, 25 Mar 2020 14:18:59 +0000 Subject: [PATCH v2] abg-ir.cc: Improve types_have_similar_structure. In-Reply-To: <20200324132926.139360-1-gprocida@google.com> References: <20200324132926.139360-1-gprocida@google.com> Message-ID: <20200325141859.93896-1-gprocida@google.com> 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 --- 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(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 zv-Z0eFRvwp044z|aHuf~a6fY>S0b?j1(=3w>$iVhzx~dicV2riTznm~jYR=wlsA?S zlfj%KC+eLJj0s_`A!ytJkw}w-8OD-LKZGR7rbwD~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)g1KSREg6;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|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(rjnH;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%smibA^YRkZ0Cxql=tFTYQmLic?U z4a3%dAQ#HP=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)pbK3;BrT3h`qIg0iU2D3Ya)0;1UJU5Ct4`1IA7X*r5O#qJXEVv=|fjXR;|PGo2X| z8No2Fs4ra`rOLc;G%rrIs)VyMWFa-8nIWwlYQ~*~!KZ z3Q^8drokB~XD?ueAxxRkQ>M-cau|-I19okhlVlw6*p%}0iv#3}=+bYDLnKX{_L}O80)%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`#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@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%fkN024#`iw4fLK8E#lA2N%(gR-X%pN)0f^26C?L` z`S8wau_T(lPa@WD{-5zgRXoj~zDDN!xBKKo2<~$Io1|!D)~9#4U;UrBkm?iFr}xvW zzl#{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