From a11a0068ea333a5149aeed6bd92cb2c6b9523afa Mon Sep 17 00:00:00 2001 From: Dodji Seketeli Date: Wed, 26 Jun 2019 10:43:22 +0200 Subject: [PATCH] Bug 24731 - Wrongly reporting union members order change When union data members are re-ordered, abidiff reports the re-ordering as if it was a meaningful ABI change. This patch teaches Libabigail to categorize that benign type layout change as a HARMLESS_UNION_CHANGE_CATEGORY kind of change and ignore it. * include/abg-comp-filter.h (union_diff_has_harmless_changes): Declare new function and ... * src/abg-comp-filter.cc (union_diff_has_harmless_changes): ... define it here. (categorize_harmless_diff_node): Use the new union_diff_has_harmless_changes here. * include/abg-comparison.h (HARMLESS_UNION_CHANGE_CATEGORY): Add a new enumerator to diff_category enum. Adjust the value of the other enumerators. * src/abg-comparison.cc (get_default_harmless_categories_bitmap): Add the new HARMLESS_UNION_CHANGE_CATEGORY in here. (operator<<(ostream& o, diff_category c)): Support the new HARMLESS_UNION_CHANGE_CATEGORY. * tests/data/test-diff-filter/test-PR24731-report-0.txt: Likewise. * tests/data/test-diff-filter/test-PR24731-report-1.txt: Likewise. * tests/data/test-diff-filter/test-PR24731-v0.c: Likewise. * tests/data/test-diff-filter/test-PR24731-v0.o: Likewise. * tests/data/test-diff-filter/test-PR24731-v1.c: Likewise. * tests/data/test-diff-filter/test-PR24731-v1.o: Likewise. * tests/data/Makefile.am: Add the new test material above to source distribution. * tests/test-diff-filter.cc (in_out_spec): Add the new test input to this test harness. Signed-off-by: Dodji Seketeli --- include/abg-comp-filter.h | 2 ++ include/abg-comparison.h | 27 ++++++++++++--------- src/abg-comp-filter.cc | 22 +++++++++++++++++ src/abg-comparison.cc | 9 +++++++ tests/data/Makefile.am | 6 +++++ .../test-diff-filter/test-PR24731-report-0.txt | 3 +++ .../test-diff-filter/test-PR24731-report-1.txt | 13 ++++++++++ tests/data/test-diff-filter/test-PR24731-v0.c | 5 ++++ tests/data/test-diff-filter/test-PR24731-v0.o | Bin 0 -> 2800 bytes tests/data/test-diff-filter/test-PR24731-v1.c | 5 ++++ tests/data/test-diff-filter/test-PR24731-v1.o | Bin 0 -> 2800 bytes tests/test-diff-filter.cc | 7 ++++++ 12 files changed, 87 insertions(+), 12 deletions(-) create mode 100644 tests/data/test-diff-filter/test-PR24731-report-0.txt create mode 100644 tests/data/test-diff-filter/test-PR24731-report-1.txt create mode 100644 tests/data/test-diff-filter/test-PR24731-v0.c create mode 100644 tests/data/test-diff-filter/test-PR24731-v0.o create mode 100644 tests/data/test-diff-filter/test-PR24731-v1.c create mode 100644 tests/data/test-diff-filter/test-PR24731-v1.o diff --git a/include/abg-comp-filter.h b/include/abg-comp-filter.h index 966f6c2..2f7c15c 100644 --- a/include/abg-comp-filter.h +++ b/include/abg-comp-filter.h @@ -42,6 +42,8 @@ namespace filtering bool has_harmless_name_change(const decl_base_sptr& f, const decl_base_sptr& s); +bool union_diff_has_harmless_changes(const diff *d); + bool has_harmful_name_change(const decl_base_sptr& f, const decl_base_sptr& s); diff --git a/include/abg-comparison.h b/include/abg-comparison.h index cf2fa4e..378ce03 100644 --- a/include/abg-comparison.h +++ b/include/abg-comparison.h @@ -372,59 +372,61 @@ enum diff_category /// alias change that is harmless. HARMLESS_SYMBOL_ALIAS_CHANGE_CATEORY = 1 << 6, + HARMLESS_UNION_CHANGE_CATEGORY = 1 << 7, + /// This means that a diff node was marked as suppressed by a /// user-provided suppression specification. - SUPPRESSED_CATEGORY = 1 << 7, + SUPPRESSED_CATEGORY = 1 << 8, /// This means that a diff node was warked as being for a private /// type. That is, the diff node is meant to be suppressed by a /// suppression specification that was auto-generated to filter out /// changes to private types. - PRIVATE_TYPE_CATEGORY = 1 << 8, + PRIVATE_TYPE_CATEGORY = 1 << 9, /// This means the diff node (or at least one of its descendant /// nodes) carries a change that modifies the size of a type or an /// offset of a type member. Removal or changes of enumerators in a /// enum fall in this category too. - SIZE_OR_OFFSET_CHANGE_CATEGORY = 1 << 9, + SIZE_OR_OFFSET_CHANGE_CATEGORY = 1 << 10, /// This means that a diff node in the sub-tree carries an /// incompatible change to a vtable. - VIRTUAL_MEMBER_CHANGE_CATEGORY = 1 << 10, + VIRTUAL_MEMBER_CHANGE_CATEGORY = 1 << 11, /// A diff node in this category is redundant. That means it's /// present as a child of a other nodes in the diff tree. - REDUNDANT_CATEGORY = 1 << 11, + REDUNDANT_CATEGORY = 1 << 12, /// This means that a diff node in the sub-tree carries a class type /// that was declaration-only and that is now defined, or vice /// versa. - CLASS_DECL_ONLY_DEF_CHANGE_CATEGORY = 1 << 12, + CLASS_DECL_ONLY_DEF_CHANGE_CATEGORY = 1 << 13, /// A diff node in this category is a function parameter type which /// top cv-qualifiers change. - FN_PARM_TYPE_TOP_CV_CHANGE_CATEGORY = 1 << 13, + FN_PARM_TYPE_TOP_CV_CHANGE_CATEGORY = 1 << 14, /// A diff node in this category has a function parameter type with a /// cv-qualifiers change. - FN_PARM_TYPE_CV_CHANGE_CATEGORY = 1 << 14, + FN_PARM_TYPE_CV_CHANGE_CATEGORY = 1 << 15, /// A diff node in this category is a function return type with a /// cv-qualifier change. - FN_RETURN_TYPE_CV_CHANGE_CATEGORY = 1 << 15, + FN_RETURN_TYPE_CV_CHANGE_CATEGORY = 1 << 16, /// A diff node in this category is for a variable which type holds /// a cv-qualifier change. - VAR_TYPE_CV_CHANGE_CATEGORY = 1 << 16, + VAR_TYPE_CV_CHANGE_CATEGORY = 1 << 17, /// A diff node in this category carries a change from void pointer /// to non-void pointer. - VOID_PTR_TO_PTR_CHANGE_CATEGORY = 1 << 17, + VOID_PTR_TO_PTR_CHANGE_CATEGORY = 1 << 18, /// A diff node in this category carries a change in the size of the /// array type of a global variable, but the ELF size of the /// variable didn't change. - BENIGN_INFINITE_ARRAY_CHANGE_CATEGORY = 1 << 18, + BENIGN_INFINITE_ARRAY_CHANGE_CATEGORY = 1 << 19, /// A special enumerator that is the logical 'or' all the /// enumerators above. /// @@ -438,6 +440,7 @@ enum diff_category | STATIC_DATA_MEMBER_CHANGE_CATEGORY | HARMLESS_ENUM_CHANGE_CATEGORY | HARMLESS_SYMBOL_ALIAS_CHANGE_CATEORY + | HARMLESS_UNION_CHANGE_CATEGORY | SUPPRESSED_CATEGORY | PRIVATE_TYPE_CATEGORY | SIZE_OR_OFFSET_CHANGE_CATEGORY diff --git a/src/abg-comp-filter.cc b/src/abg-comp-filter.cc index ed099c1..3134f5d 100644 --- a/src/abg-comp-filter.cc +++ b/src/abg-comp-filter.cc @@ -1319,6 +1319,25 @@ has_benign_infinite_array_change(const diff* dif) } return false; } + +/// Test if a union diff node does have changes that don't impact its +/// size. +/// +/// @param d the union diff node to consider. +/// +/// @return true iff @p d is a diff node which has changes that don't +/// impact its size. +bool +union_diff_has_harmless_changes(const diff *d) +{ + if (is_union_diff(d) + && d->has_changes() + && !has_type_size_change(d)) + return true; + + return false; +} + /// Detect if the changes carried by a given diff node are deemed /// harmless and do categorize the diff node accordingly. /// @@ -1355,6 +1374,9 @@ categorize_harmless_diff_node(diff *d, bool pre) || class_diff_has_harmless_odr_violation_change(d)) category |= HARMLESS_DECL_NAME_CHANGE_CATEGORY; + if (union_diff_has_harmless_changes(d)) + category |= HARMLESS_UNION_CHANGE_CATEGORY; + if (has_non_virtual_mem_fn_change(d)) category |= NON_VIRT_MEM_FUN_CHANGE_CATEGORY; diff --git a/src/abg-comparison.cc b/src/abg-comparison.cc index 2cc3c69..feae5f9 100644 --- a/src/abg-comparison.cc +++ b/src/abg-comparison.cc @@ -2921,6 +2921,7 @@ get_default_harmless_categories_bitmap() | abigail::comparison::STATIC_DATA_MEMBER_CHANGE_CATEGORY | abigail::comparison::HARMLESS_ENUM_CHANGE_CATEGORY | abigail::comparison::HARMLESS_SYMBOL_ALIAS_CHANGE_CATEORY + | abigail::comparison::HARMLESS_UNION_CHANGE_CATEGORY | abigail::comparison::CLASS_DECL_ONLY_DEF_CHANGE_CATEGORY | abigail::comparison::FN_PARM_TYPE_TOP_CV_CHANGE_CATEGORY | abigail::comparison::FN_PARM_TYPE_CV_CHANGE_CATEGORY @@ -3016,6 +3017,14 @@ operator<<(ostream& o, diff_category c) emitted_a_category |= true; } + if (c & HARMLESS_UNION_CHANGE_CATEGORY) + { + if (emitted_a_category) + o << "|"; + o << "HARMLESS_UNION_CHANGE_CATEORY"; + emitted_a_category |= true; + } + if (c & SUPPRESSED_CATEGORY) { if (emitted_a_category) diff --git a/tests/data/Makefile.am b/tests/data/Makefile.am index 3ad2bc7..982d32e 100644 --- a/tests/data/Makefile.am +++ b/tests/data/Makefile.am @@ -698,6 +698,12 @@ test-diff-filter/test47-filter-void-ptr-change-v1.o \ test-diff-filter/PR24430-fold-qualified-array-clang \ test-diff-filter/PR24430-fold-qualified-array-gcc \ test-diff-filter/PR24430-fold-qualified-array-report-0.txt \ +test-diff-filter/test-PR24731-report-0.txt \ +test-diff-filter/test-PR24731-report-1.txt \ +test-diff-filter/test-PR24731-v0.c \ +test-diff-filter/test-PR24731-v0.o \ +test-diff-filter/test-PR24731-v1.c \ +test-diff-filter/test-PR24731-v1.o \ \ test-diff-suppr/test0-type-suppr-v0.cc \ test-diff-suppr/test0-type-suppr-v1.cc \ diff --git a/tests/data/test-diff-filter/test-PR24731-report-0.txt b/tests/data/test-diff-filter/test-PR24731-report-0.txt new file mode 100644 index 0000000..9666a8f --- /dev/null +++ b/tests/data/test-diff-filter/test-PR24731-report-0.txt @@ -0,0 +1,3 @@ +Functions changes summary: 0 Removed, 0 Changed (1 filtered out), 0 Added function +Variables changes summary: 0 Removed, 0 Changed, 0 Added variable + diff --git a/tests/data/test-diff-filter/test-PR24731-report-1.txt b/tests/data/test-diff-filter/test-PR24731-report-1.txt new file mode 100644 index 0000000..d33e565 --- /dev/null +++ b/tests/data/test-diff-filter/test-PR24731-report-1.txt @@ -0,0 +1,13 @@ +Functions changes summary: 0 Removed, 1 Changed, 0 Added function +Variables changes summary: 0 Removed, 0 Changed, 0 Added variable + +1 function with some indirect sub-type change: + + [C]'function void test_func(u)' at test-PR24731-v1.c:3:1 has some indirect sub-type changes: + parameter 1 of type 'union u' has sub-type changes: + type size hasn't changed + type changed from: + union u{int a; char c; short int s;} + to: + union u{int a; short int s; char c;} + diff --git a/tests/data/test-diff-filter/test-PR24731-v0.c b/tests/data/test-diff-filter/test-PR24731-v0.c new file mode 100644 index 0000000..c1f5761 --- /dev/null +++ b/tests/data/test-diff-filter/test-PR24731-v0.c @@ -0,0 +1,5 @@ + union u { int a; char c; short s; }; + + void test_func(union u var) { + (void) var; + } diff --git a/tests/data/test-diff-filter/test-PR24731-v0.o b/tests/data/test-diff-filter/test-PR24731-v0.o new file mode 100644 index 0000000000000000000000000000000000000000..c5cd811ee9fcb579ab3a1b504929389a869f7de6 GIT binary patch literal 2800 zcmbtV-D?v|6hE_@nCRNjFR`TxD=2NjY|>AQQg5_+t0+<`f`}3~$uwDQHf48Pt0MR1 za?uAtK@h<={V#m<<^B!6-*@k`4|>kdOtP775%j>$obx-MJ9B2{ugll}G#CR#49-JO zQYb+AK##A)Yz0PO7;as8@!yMQU+;Z>iC`vH8rU5YXBwR%ya}q4;yk+JAvXU)eHi!K&ZWhRBa-70c#@=2W2gqf+ zuErQ=-sqzv~%vNwTUahnL z6!B3Q-7yUG2wR;XvT23Lqq?!}*J1U>EqleDcb1$5d$v5YP+pv!v!~X1!@lB10x!-j zPTR$HwC(fdCii*Z)lqbVdTV)iX|cFCZx@>>HPl3W-`4R4o+rLBN!2}`nkfYp_iv-T90HU0Cd=s!or&jNe04s3UjO&reUU*WPGL!3UM zcu_AD2LCZu1`b?80|BRw)P@=O|9hI!Ozm{+3%QlLkBOQzGG*iII#V>O4r7$u9*h99 z+s*xI6RdtfN+t~KgJb8WkO_zPpxF6%ifNSubsKhX7xuagP1SX9D8W+%m4R@IrS@}( zlt>D{D?~UVXN8AS#xoWZRn~2Me%Tj^By6gJ-oEMsEF~48&X2jlgYl zS@>^G6X;KSs@7A5t4N0SE=j%46zB<@2-8pE)udB`*Zn6^M}70F{xj&S$E0*B|7#Kw zq}$R%LC<6*VMFYacst>%#Hp_cQaw(O4AC-vdQ8H<(16qyQ+L#D*HEKlWV|ZDa@SKZ zy6rWD)9Rm+heG*N{dCLe^*0dHV93i=%f8E?r&W0s@=p5e=C35MqzvV({$Igw zkV#v=t(5hnS48=%b)|ar+!Xs?39iY4>At7eN%yBc(a#=2Fh)fo9T!t~)c>F2rujdY M^?&Gk3P$(;0Rq6gaR2}S literal 0 HcmV?d00001 diff --git a/tests/data/test-diff-filter/test-PR24731-v1.c b/tests/data/test-diff-filter/test-PR24731-v1.c new file mode 100644 index 0000000..9a73f99 --- /dev/null +++ b/tests/data/test-diff-filter/test-PR24731-v1.c @@ -0,0 +1,5 @@ + union u { int a; short s; char c; }; + + void test_func(union u var) { + (void) var; + } diff --git a/tests/data/test-diff-filter/test-PR24731-v1.o b/tests/data/test-diff-filter/test-PR24731-v1.o new file mode 100644 index 0000000000000000000000000000000000000000..3eeea29a03f92c964c4b73d86e91c6aca1868232 GIT binary patch literal 2800 zcmbtV&1)1%6o1t?w&p9dtFsMok|B`_d!>^_j{jJ^{VU1qwI8L%;uHJ$|yTl z8aaXZ3C7E|ecrxcvoXvx47^OjH53ZpgKb~5c^NY)wgNEZvL7dSg^bQlP?-AoUq4R( zl!~dVHI5n2QC9o_&_6@EybP4EbsR02!jZ90m0=u{Gr&z_5Q`|Tu5ZjO{4hUT-JNw? zi3c2!@liOwYgyGDV{<2H<@m1MyzQ(y3+}SJ=*-Q`F3v2?%{x;YqU~Ju zVu@Ghm!_R+FWw2nN=F1D^jj!;VXM2cx4cwcT5zfzXmxRWYu$cN)Y|>_J-^oR#@$4)%UPNgRd`!Lf65h{MqXD0V*HV%jB1X~Q1u!amk+K^(#D4J*+)+5i&c+8fdeDo#CeCpv=dR{ru@{5e_F@m*W)y)NiY?D=i{?(J z;Rl<2)Fr&vY=&ZYpV|w(pdz`R=?L1_#1J9ek@$PtJNs>TK82T(QRyo zUQejP|K~J`{&c2#KXtgeVzOsP(4aG=!6}8AH}RTO9~e#lvuHtm^I!dE(btSgNp=1g zBxK06rH6u^$$G|y*k$>4!bgQuUlF8xoE{mX75vPYgukZ&sVAp&G;P;WV`5~yrofrh zQ!=LQb%gWkpHi1X`&0e&mow{cBWA`#2!2!GB@!}Z+BQ&^m;V=)U+)v;r(QDiZzD!A zb)Cp2^hPCmtpPRb{Ca+bc%FYlU1s+y{=X}K!wjD7`xy+hIa|JAGLpAI5 zYxpHHdHc7Wvw!r8Xn(!0RF9dPV!u=1x+<9d_w+iM{&Xhh-9re*sVJoH