From 08e0ce10667bd67c32f1af53d3821ef93c5aac8a Mon Sep 17 00:00:00 2001 From: Hsin-Yi Chen Date: Sat, 14 Jan 2023 17:50:33 +0800 Subject: [PATCH] Diff the fields located at the same offset in a class - Rewrite the diff algorithm to handle multiple fields located at the same offset in a class, struct or union. - Allow renaming a field if its (offset, type) pair is unique. Bug: 255702405 Test: ./test.py Change-Id: I79ebe9a827822b0f89d95952fa7d8c30499ca680 --- .../src/repr/abi_diff_helpers.cpp | 120 ++++++----- .../src/repr/abi_diff_helpers.h | 5 + .../tests/integration/union/include/base.h | 26 +++ .../tests/integration/union/include/diff.h | 26 +++ .../tests/integration/union/map.txt | 5 + vndk/tools/header-checker/tests/module.py | 17 ++ .../reference_dumps/arm64/libunion.so.lsdump | 188 ++++++++++++++++++ vndk/tools/header-checker/tests/test.py | 11 + 8 files changed, 347 insertions(+), 51 deletions(-) create mode 100644 vndk/tools/header-checker/tests/integration/union/include/base.h create mode 100644 vndk/tools/header-checker/tests/integration/union/include/diff.h create mode 100644 vndk/tools/header-checker/tests/integration/union/map.txt create mode 100644 vndk/tools/header-checker/tests/reference_dumps/arm64/libunion.so.lsdump diff --git a/vndk/tools/header-checker/src/repr/abi_diff_helpers.cpp b/vndk/tools/header-checker/src/repr/abi_diff_helpers.cpp index ed728ab1f..fefccdc8a 100644 --- a/vndk/tools/header-checker/src/repr/abi_diff_helpers.cpp +++ b/vndk/tools/header-checker/src/repr/abi_diff_helpers.cpp @@ -390,16 +390,75 @@ AbiDiffHelper::CompareCommonRecordFields( return std::make_pair(field_diff_status, nullptr); } +// This function filters out the pairs of old and new fields that meet the +// following conditions: +// The old field's (offset, type) is unique in old_fields. +// The new field's (offset, type) is unique in new_fields. +// The two fields have compatible attributes except the name. +void AbiDiffHelper::FilterOutRenamedRecordFields( + std::deque *type_queue, DiffMessageIR::DiffKind diff_kind, + std::vector &old_fields, + std::vector &new_fields) { + const auto old_end = old_fields.end(); + const auto new_end = new_fields.end(); + auto is_less = [](const RecordFieldIR *first, const RecordFieldIR *second) { + if (first->GetOffset() != second->GetOffset()) { + return first->GetOffset() < second->GetOffset(); + } + return first->GetReferencedType() < second->GetReferencedType(); + }; + std::sort(old_fields.begin(), old_end, is_less); + std::sort(new_fields.begin(), new_end, is_less); + + std::vector out_old_fields; + std::vector out_new_fields; + auto old_it = old_fields.begin(); + auto new_it = new_fields.begin(); + while (old_it != old_end && new_it != new_end) { + auto next_old_it = std::next(old_it); + while (next_old_it != old_end && !is_less(*old_it, *next_old_it)) { + next_old_it++; + } + if (is_less(*old_it, *new_it) || next_old_it - old_it > 1) { + out_old_fields.insert(out_old_fields.end(), old_it, next_old_it); + old_it = next_old_it; + continue; + } + + auto next_new_it = std::next(new_it); + while (next_new_it != new_end && !is_less(*new_it, *next_new_it)) { + next_new_it++; + } + if (is_less(*new_it, *old_it) || next_new_it - new_it > 1) { + out_new_fields.insert(out_new_fields.end(), new_it, next_new_it); + new_it = next_new_it; + continue; + } + + auto comparison_result = + CompareCommonRecordFields(*old_it, *new_it, type_queue, diff_kind); + if (comparison_result.second != nullptr) { + out_old_fields.emplace_back(*old_it); + out_new_fields.emplace_back(*new_it); + } + old_it = next_old_it; + new_it = next_new_it; + } + out_old_fields.insert(out_old_fields.end(), old_it, old_end); + out_new_fields.insert(out_new_fields.end(), new_it, new_end); + + old_fields = std::move(out_old_fields); + new_fields = std::move(out_new_fields); +} + RecordFieldDiffResult AbiDiffHelper::CompareRecordFields( const std::vector &old_fields, const std::vector &new_fields, std::deque *type_queue, DiffMessageIR::DiffKind diff_kind) { RecordFieldDiffResult result; + // Map names to RecordFieldIR. AbiElementMap old_fields_map; AbiElementMap new_fields_map; - std::map old_fields_offset_map; - std::map new_fields_offset_map; - utils::AddToMap( &old_fields_map, old_fields, [](const RecordFieldIR *f) {return f->GetName();}, @@ -408,55 +467,14 @@ RecordFieldDiffResult AbiDiffHelper::CompareRecordFields( &new_fields_map, new_fields, [](const RecordFieldIR *f) {return f->GetName();}, [](const RecordFieldIR *f) {return f;}); - utils::AddToMap( - &old_fields_offset_map, old_fields, - [](const RecordFieldIR *f) {return f->GetOffset();}, - [](const RecordFieldIR *f) {return f;}); - utils::AddToMap( - &new_fields_offset_map, new_fields, - [](const RecordFieldIR *f) {return f->GetOffset();}, - [](const RecordFieldIR *f) {return f;}); - // If a field is removed from the map field_name -> offset see if another - // field is present at the same offset and compare the size and type etc, - // remove it from the removed fields if they're compatible. - std::vector removed_fields = + // Compare the fields whose names are not present in both records. + result.removed_fields = utils::FindRemovedElements(old_fields_map, new_fields_map); - - std::vector added_fields = + result.added_fields = utils::FindRemovedElements(new_fields_map, old_fields_map); - - auto predicate = - [&](const RecordFieldIR *removed_field, - std::map &field_off_map) { - uint64_t old_field_offset = removed_field->GetOffset(); - auto corresponding_field_at_same_offset = - field_off_map.find(old_field_offset); - // Correctly reported as removed, so do not remove. - if (corresponding_field_at_same_offset == field_off_map.end()) { - return false; - } - - auto comparison_result = CompareCommonRecordFields( - removed_field, corresponding_field_at_same_offset->second, - type_queue, diff_kind); - // No actual diff, so remove it. - return (comparison_result.second == nullptr); - }; - - removed_fields.erase( - std::remove_if( - removed_fields.begin(), removed_fields.end(), - std::bind(predicate, std::placeholders::_1, new_fields_offset_map)), - removed_fields.end()); - added_fields.erase( - std::remove_if( - added_fields.begin(), added_fields.end(), - std::bind(predicate, std::placeholders::_1, old_fields_offset_map)), - added_fields.end()); - - result.removed_fields = std::move(removed_fields); - result.added_fields = std::move(added_fields); - + FilterOutRenamedRecordFields(type_queue, diff_kind, result.removed_fields, + result.added_fields); + // Compare the fields whose names are present in both records. std::vector> cf = utils::FindCommonElements(old_fields_map, new_fields_map); @@ -472,7 +490,7 @@ RecordFieldDiffResult AbiDiffHelper::CompareRecordFields( std::move(*(diffed_field_ptr.second.release()))); } } - + // Determine DiffStatus. DiffStatus &diff_status = result.status; diff_status = DiffStatus::kNoDiff; if (result.diffed_fields.size() != 0 || result.removed_fields.size() != 0) { diff --git a/vndk/tools/header-checker/src/repr/abi_diff_helpers.h b/vndk/tools/header-checker/src/repr/abi_diff_helpers.h index f20a58896..dd386fdef 100644 --- a/vndk/tools/header-checker/src/repr/abi_diff_helpers.h +++ b/vndk/tools/header-checker/src/repr/abi_diff_helpers.h @@ -188,6 +188,11 @@ class AbiDiffHelper { std::deque *type_queue, IRDiffDumper::DiffKind diff_kind); + void FilterOutRenamedRecordFields( + std::deque *type_queue, DiffMessageIR::DiffKind diff_kind, + std::vector &old_fields, + std::vector &new_fields); + RecordFieldDiffResult CompareRecordFields( const std::vector &old_fields, const std::vector &new_fields, diff --git a/vndk/tools/header-checker/tests/integration/union/include/base.h b/vndk/tools/header-checker/tests/integration/union/include/base.h new file mode 100644 index 000000000..d68209e70 --- /dev/null +++ b/vndk/tools/header-checker/tests/integration/union/include/base.h @@ -0,0 +1,26 @@ +union ChangeType { + char member_1; + char member_2; + int member_3; +}; + +union Rename { + int member_1; + char member_2; +}; + +union Swap { + int member_1; + char member_2; +}; + +struct ChangeTypeInStruct { + int member_1; + char member_2[0]; + char member_3[0]; + int member_4[0]; +}; + +extern "C" { +void function(ChangeType, Rename, Swap, ChangeTypeInStruct); +} diff --git a/vndk/tools/header-checker/tests/integration/union/include/diff.h b/vndk/tools/header-checker/tests/integration/union/include/diff.h new file mode 100644 index 000000000..2f5d62ed9 --- /dev/null +++ b/vndk/tools/header-checker/tests/integration/union/include/diff.h @@ -0,0 +1,26 @@ +union ChangeType { + char member_1; + int member_2; + int member_3; +}; + +union Rename { + int rename_1; + char rename_2; +}; + +union Swap { + char member_2; + int rename_1; +}; + +struct ChangeTypeInStruct { + int member_1; + char member_2[0]; + int member_3[0]; + int member_4[0]; +}; + +extern "C" { +void function(ChangeType, Rename, Swap, ChangeTypeInStruct); +} diff --git a/vndk/tools/header-checker/tests/integration/union/map.txt b/vndk/tools/header-checker/tests/integration/union/map.txt new file mode 100644 index 000000000..d4a87a0b1 --- /dev/null +++ b/vndk/tools/header-checker/tests/integration/union/map.txt @@ -0,0 +1,5 @@ +libunion { + global: + function; +}; + diff --git a/vndk/tools/header-checker/tests/module.py b/vndk/tools/header-checker/tests/module.py index a33c69b01..565e97e23 100755 --- a/vndk/tools/header-checker/tests/module.py +++ b/vndk/tools/header-checker/tests/module.py @@ -772,6 +772,23 @@ TEST_MODULES = [ linker_flags=['-output-format', 'Json'], has_reference_dump=True, ), + LsdumpModule( + name='libunion', + arch='arm64', + srcs=['integration/union/include/base.h'], + version_script='integration/union/map.txt', + export_include_dirs=['integration/union/include'], + linker_flags=['-output-format', 'Json'], + has_reference_dump=True, + ), + LsdumpModule( + name='libunion_diff', + arch='arm64', + srcs=['integration/union/include/diff.h'], + version_script='integration/union/map.txt', + export_include_dirs=['integration/union/include'], + linker_flags=['-output-format', 'Json'], + ), ] TEST_MODULES = {m.name: m for m in TEST_MODULES} diff --git a/vndk/tools/header-checker/tests/reference_dumps/arm64/libunion.so.lsdump b/vndk/tools/header-checker/tests/reference_dumps/arm64/libunion.so.lsdump new file mode 100644 index 000000000..3bb451a2a --- /dev/null +++ b/vndk/tools/header-checker/tests/reference_dumps/arm64/libunion.so.lsdump @@ -0,0 +1,188 @@ +{ + "array_types" : + [ + { + "alignment" : 1, + "linker_set_key" : "_ZTIA0_c", + "name" : "char[0]", + "referenced_type" : "_ZTIc", + "self_type" : "_ZTIA0_c", + "source_file" : "development/vndk/tools/header-checker/tests/integration/union/include/base.h" + }, + { + "alignment" : 4, + "linker_set_key" : "_ZTIA0_i", + "name" : "int[0]", + "referenced_type" : "_ZTIi", + "self_type" : "_ZTIA0_i", + "source_file" : "development/vndk/tools/header-checker/tests/integration/union/include/base.h" + } + ], + "builtin_types" : + [ + { + "alignment" : 1, + "is_integral" : true, + "is_unsigned" : true, + "linker_set_key" : "_ZTIc", + "name" : "char", + "referenced_type" : "_ZTIc", + "self_type" : "_ZTIc", + "size" : 1 + }, + { + "alignment" : 4, + "is_integral" : true, + "linker_set_key" : "_ZTIi", + "name" : "int", + "referenced_type" : "_ZTIi", + "self_type" : "_ZTIi", + "size" : 4 + }, + { + "linker_set_key" : "_ZTIv", + "name" : "void", + "referenced_type" : "_ZTIv", + "self_type" : "_ZTIv" + } + ], + "elf_functions" : + [ + { + "name" : "function" + } + ], + "elf_objects" : [], + "enum_types" : [], + "function_types" : [], + "functions" : + [ + { + "function_name" : "function", + "linker_set_key" : "function", + "parameters" : + [ + { + "referenced_type" : "_ZTI10ChangeType" + }, + { + "referenced_type" : "_ZTI6Rename" + }, + { + "referenced_type" : "_ZTI4Swap" + }, + { + "referenced_type" : "_ZTI18ChangeTypeInStruct" + } + ], + "return_type" : "_ZTIv", + "source_file" : "development/vndk/tools/header-checker/tests/integration/union/include/base.h" + } + ], + "global_vars" : [], + "lvalue_reference_types" : [], + "pointer_types" : [], + "qualified_types" : [], + "record_types" : + [ + { + "alignment" : 4, + "fields" : + [ + { + "field_name" : "member_1", + "referenced_type" : "_ZTIc" + }, + { + "field_name" : "member_2", + "referenced_type" : "_ZTIc" + }, + { + "field_name" : "member_3", + "referenced_type" : "_ZTIi" + } + ], + "linker_set_key" : "_ZTI10ChangeType", + "name" : "ChangeType", + "record_kind" : "union", + "referenced_type" : "_ZTI10ChangeType", + "self_type" : "_ZTI10ChangeType", + "size" : 4, + "source_file" : "development/vndk/tools/header-checker/tests/integration/union/include/base.h" + }, + { + "alignment" : 4, + "fields" : + [ + { + "field_name" : "member_1", + "referenced_type" : "_ZTIi" + }, + { + "field_name" : "member_2", + "field_offset" : 32, + "referenced_type" : "_ZTIA0_c" + }, + { + "field_name" : "member_3", + "field_offset" : 32, + "referenced_type" : "_ZTIA0_c" + }, + { + "field_name" : "member_4", + "field_offset" : 32, + "referenced_type" : "_ZTIA0_i" + } + ], + "linker_set_key" : "_ZTI18ChangeTypeInStruct", + "name" : "ChangeTypeInStruct", + "referenced_type" : "_ZTI18ChangeTypeInStruct", + "self_type" : "_ZTI18ChangeTypeInStruct", + "size" : 4, + "source_file" : "development/vndk/tools/header-checker/tests/integration/union/include/base.h" + }, + { + "alignment" : 4, + "fields" : + [ + { + "field_name" : "member_1", + "referenced_type" : "_ZTIi" + }, + { + "field_name" : "member_2", + "referenced_type" : "_ZTIc" + } + ], + "linker_set_key" : "_ZTI4Swap", + "name" : "Swap", + "record_kind" : "union", + "referenced_type" : "_ZTI4Swap", + "self_type" : "_ZTI4Swap", + "size" : 4, + "source_file" : "development/vndk/tools/header-checker/tests/integration/union/include/base.h" + }, + { + "alignment" : 4, + "fields" : + [ + { + "field_name" : "member_1", + "referenced_type" : "_ZTIi" + }, + { + "field_name" : "member_2", + "referenced_type" : "_ZTIc" + } + ], + "linker_set_key" : "_ZTI6Rename", + "name" : "Rename", + "record_kind" : "union", + "referenced_type" : "_ZTI6Rename", + "self_type" : "_ZTI6Rename", + "size" : 4, + "source_file" : "development/vndk/tools/header-checker/tests/integration/union/include/base.h" + } + ], + "rvalue_reference_types" : [] +} diff --git a/vndk/tools/header-checker/tests/test.py b/vndk/tools/header-checker/tests/test.py index 7c6d9f0de..4217d62dc 100755 --- a/vndk/tools/header-checker/tests/test.py +++ b/vndk/tools/header-checker/tests/test.py @@ -474,6 +474,17 @@ class HeaderCheckerTest(unittest.TestCase): self.assertIn(f'"{type_id}"', diff, f'"{type_id}" should be in the diff report.') + def test_union_diff(self): + diff = self.prepare_and_run_abi_diff_all_archs( + "libunion", "libunion_diff", 8, + flags=["-input-format-new", "Json", "-input-format-old", "Json"], + create_old=False, create_new=True) + self.assertIn('"ChangeType"', diff) + self.assertIn('"ChangeTypeInStruct"', diff) + self.assertEqual(2, diff.count("fields_diff")) + self.assertNotIn("fields_added", diff) + self.assertNotIn("fields_removed", diff) + if __name__ == '__main__': unittest.main()