From: Dodji Seketeli Date: Wed, 26 Jun 2019 08:43:22 +0000 (+0200) Subject: Bug 24731 - Wrongly reporting union members order change X-Git-Tag: upstream/1.7~78 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=a11a0068ea333a5149aeed6bd92cb2c6b9523afa;p=platform%2Fupstream%2Flibabigail.git 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 --- diff --git a/include/abg-comp-filter.h b/include/abg-comp-filter.h index 966f6c2e..2f7c15c4 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 cf2fa4e1..378ce034 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 ed099c16..3134f5df 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 2cc3c698..feae5f96 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 3ad2bc7c..982d32e5 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 00000000..9666a8fd --- /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 00000000..d33e5653 --- /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 00000000..c1f57619 --- /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 00000000..c5cd811e Binary files /dev/null and b/tests/data/test-diff-filter/test-PR24731-v0.o differ 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 00000000..9a73f998 --- /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 00000000..3eeea29a Binary files /dev/null and b/tests/data/test-diff-filter/test-PR24731-v1.o differ diff --git a/tests/test-diff-filter.cc b/tests/test-diff-filter.cc index c05928f4..74b8c06c 100644 --- a/tests/test-diff-filter.cc +++ b/tests/test-diff-filter.cc @@ -549,6 +549,13 @@ InOutSpec in_out_specs[] = "data/test-diff-filter/PR24430-fold-qualified-array-report-0.txt", "output/test-diff-filter/PR24430-fold-qualified-array-report-0.txt", }, + { + "data/test-diff-filter/test-PR24731-v0.o ", + "data/test-diff-filter/test-PR24731-v1.o ", + "--no-default-suppression", + "data/test-diff-filter/test-PR24731-report-0.txt", + "output/test-diff-filter/test-PR24731-report-0.txt", + }, // This should be the last entry {NULL, NULL, NULL, NULL, NULL} };