From 17d6299cae3161132c6be278be2e9d8b48eaaab4 Mon Sep 17 00:00:00 2001 From: Hsin-Yi Chen Date: Fri, 3 Feb 2023 18:26:55 +0800 Subject: [PATCH] Return DiffStatus from FilterOutRenamedRecordFields The status of the filtered out fields is either kNoDiff or kIndirectDiff. It does not affect the diff report, but makes ModuleMerger determine redefined types more accurately. Test: ./test.py ; update the prebuilt clang tools and build Bug: 255702405 Change-Id: Ia94225d25e00597affa8b1e01a7c9de592bf5b05 --- .../src/repr/abi_diff_helpers.cpp | 59 ++++++++++--------- .../src/repr/abi_diff_helpers.h | 2 +- 2 files changed, 32 insertions(+), 29 deletions(-) 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 90b53f7c0..03cff4313 100644 --- a/vndk/tools/header-checker/src/repr/abi_diff_helpers.cpp +++ b/vndk/tools/header-checker/src/repr/abi_diff_helpers.cpp @@ -372,7 +372,10 @@ DiffStatus AbiDiffHelper::CompareCommonRecordFields( DiffStatus field_diff_status = CompareAndDumpTypeDiff( old_field->GetReferencedType(), new_field->GetReferencedType(), type_queue, diff_kind); - if (old_field->GetOffset() != new_field->GetOffset() || + // CompareAndDumpTypeDiff should not return kDirectExt. + // In case it happens, report an incompatible diff for review. + if (field_diff_status.IsExtension() || + old_field->GetOffset() != new_field->GetOffset() || // TODO: Should this be an inquality check instead ? Some compilers can // make signatures dependant on absolute values of access specifiers. IsAccessDowngraded(old_field->GetAccess(), new_field->GetAccess())) { @@ -386,10 +389,14 @@ DiffStatus AbiDiffHelper::CompareCommonRecordFields( // 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( +// +// This function returns either kNoDiff or kIndirectDiff. It is the status of +// the field pairs that are filtered out. +DiffStatus AbiDiffHelper::FilterOutRenamedRecordFields( std::deque *type_queue, DiffMessageIR::DiffKind diff_kind, std::vector &old_fields, std::vector &new_fields) { + DiffStatus diff_status = DiffStatus::kNoDiff; const auto old_end = old_fields.end(); const auto new_end = new_fields.end(); auto is_less = [](const RecordFieldIR *first, const RecordFieldIR *second) { @@ -426,10 +433,13 @@ void AbiDiffHelper::FilterOutRenamedRecordFields( continue; } - if (CompareCommonRecordFields(*old_it, *new_it, type_queue, diff_kind) - .IsDirectDiff()) { + DiffStatus field_diff_status = + CompareCommonRecordFields(*old_it, *new_it, type_queue, diff_kind); + if (field_diff_status.IsDirectDiff()) { out_old_fields.emplace_back(*old_it); out_new_fields.emplace_back(*new_it); + } else { + diff_status.CombineWith(field_diff_status); } old_it = next_old_it; new_it = next_new_it; @@ -439,6 +449,7 @@ void AbiDiffHelper::FilterOutRenamedRecordFields( old_fields = std::move(out_old_fields); new_fields = std::move(out_new_fields); + return diff_status; } RecordFieldDiffResult AbiDiffHelper::CompareRecordFields( @@ -446,6 +457,8 @@ RecordFieldDiffResult AbiDiffHelper::CompareRecordFields( const std::vector &new_fields, std::deque *type_queue, DiffMessageIR::DiffKind diff_kind) { RecordFieldDiffResult result; + DiffStatus &diff_status = result.status; + diff_status = DiffStatus::kNoDiff; // Map names to RecordFieldIR. AbiElementMap old_fields_map; AbiElementMap new_fields_map; @@ -462,35 +475,25 @@ RecordFieldDiffResult AbiDiffHelper::CompareRecordFields( utils::FindRemovedElements(old_fields_map, new_fields_map); result.added_fields = utils::FindRemovedElements(new_fields_map, old_fields_map); - 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); - bool common_field_diff_exists = false; - for (auto &&common_fields : cf) { - DiffStatus field_diff_status = CompareCommonRecordFields( - common_fields.first, common_fields.second, type_queue, diff_kind); - if (field_diff_status.HasDiff()) { - common_field_diff_exists = true; - } - if (field_diff_status.IsDirectDiff()) { - result.diffed_fields.emplace_back(common_fields.first, - common_fields.second); - } - } - // Determine DiffStatus. - DiffStatus &diff_status = result.status; - diff_status = DiffStatus::kNoDiff; - if (result.diffed_fields.size() != 0 || result.removed_fields.size() != 0) { + diff_status.CombineWith(FilterOutRenamedRecordFields( + type_queue, diff_kind, result.removed_fields, result.added_fields)); + if (result.removed_fields.size() != 0) { diff_status.CombineWith(DiffStatus::kDirectDiff); } if (result.added_fields.size() != 0) { diff_status.CombineWith(DiffStatus::kDirectExt); } - if (common_field_diff_exists) { - diff_status.CombineWith(DiffStatus::kIndirectDiff); + // Compare the fields whose names are present in both records. + std::vector> cf = + utils::FindCommonElements(old_fields_map, new_fields_map); + for (auto &&common_fields : cf) { + DiffStatus field_diff_status = CompareCommonRecordFields( + common_fields.first, common_fields.second, type_queue, diff_kind); + diff_status.CombineWith(field_diff_status); + if (field_diff_status.IsDirectDiff()) { + result.diffed_fields.emplace_back(common_fields.first, + common_fields.second); + } } return result; } 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 65adeddfb..6a63fbc38 100644 --- a/vndk/tools/header-checker/src/repr/abi_diff_helpers.h +++ b/vndk/tools/header-checker/src/repr/abi_diff_helpers.h @@ -183,7 +183,7 @@ class AbiDiffHelper { std::deque *type_queue, IRDiffDumper::DiffKind diff_kind); - void FilterOutRenamedRecordFields( + DiffStatus FilterOutRenamedRecordFields( std::deque *type_queue, DiffMessageIR::DiffKind diff_kind, std::vector &old_fields, std::vector &new_fields);