From a062a6cf35c9a30c4a41ff6b7d13fbf49f409ee8 Mon Sep 17 00:00:00 2001 From: Hsin-Yi Chen Date: Sat, 10 Dec 2022 01:28:15 +0800 Subject: [PATCH] Determine struct extensions before calling IRDiffDumper This commit moves the functions that determine struct extensions from ir_diff_representation.cpp to abi_diff_helpers.cpp. The functions are called before IRDiffDumper so that the IR and the dumper do not include the diff logic. Test: ./test.py Bug: 259148872 Change-Id: I93aaa3028bf6a30312f0b0e5b1ef5f6ae1041f21 --- vndk/tools/header-checker/Android.bp | 1 - .../src/repr/abi_diff_helpers.cpp | 157 +++++++++++++++--- .../src/repr/abi_diff_helpers.h | 7 - .../src/repr/ir_diff_representation.cpp | 157 ------------------ .../src/repr/ir_diff_representation.h | 14 +- 5 files changed, 135 insertions(+), 201 deletions(-) delete mode 100644 vndk/tools/header-checker/src/repr/ir_diff_representation.cpp diff --git a/vndk/tools/header-checker/Android.bp b/vndk/tools/header-checker/Android.bp index 251f6ae99..e5b7e1d8b 100644 --- a/vndk/tools/header-checker/Android.bp +++ b/vndk/tools/header-checker/Android.bp @@ -143,7 +143,6 @@ cc_library_host_static { srcs: [ "src/repr/abi_diff_helpers.cpp", "src/repr/ir_diff_dumper.cpp", - "src/repr/ir_diff_representation.cpp", "src/repr/ir_dumper.cpp", "src/repr/ir_reader.cpp", "src/repr/ir_representation.cpp", 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 53f539516..328c30807 100644 --- a/vndk/tools/header-checker/src/repr/abi_diff_helpers.cpp +++ b/vndk/tools/header-checker/src/repr/abi_diff_helpers.cpp @@ -220,9 +220,8 @@ static std::string RemoveThunkInfoFromMangledName(const std::string &name) { return "_Z" + name.substr(base_name_pos); } -bool AbiDiffHelper::CompareVTableComponents( - const VTableComponentIR &old_component, - const VTableComponentIR &new_component) { +static bool CompareVTableComponents(const VTableComponentIR &old_component, + const VTableComponentIR &new_component) { // Vtable components in prebuilts/abi-dumps/vndk/28 don't have thunk info. if (old_component.GetName() != new_component.GetName()) { if (RemoveThunkInfoFromMangledName(old_component.GetName()) == @@ -238,14 +237,9 @@ bool AbiDiffHelper::CompareVTableComponents( old_component.GetKind() == new_component.GetKind(); } -bool AbiDiffHelper::CompareVTables( - const RecordTypeIR *old_record, - const RecordTypeIR *new_record) { - - const std::vector &old_components = - old_record->GetVTableLayout().GetVTableComponents(); - const std::vector &new_components = - new_record->GetVTableLayout().GetVTableComponents(); +static bool CompareVTables( + const std::vector &old_components, + const std::vector &new_components) { if (old_components.size() != new_components.size()) { return false; } @@ -257,6 +251,97 @@ bool AbiDiffHelper::CompareVTables( return true; } +static inline bool IsVOffset(VTableComponentIR::Kind kind) { + return kind == VTableComponentIR::VBaseOffset || + kind == VTableComponentIR::VCallOffset; +} + +static inline bool IsFunctionPointer(VTableComponentIR::Kind kind) { + return kind == VTableComponentIR::FunctionPointer || + kind == VTableComponentIR::CompleteDtorPointer || + kind == VTableComponentIR::DeletingDtorPointer; +} + +// A Vtable consists of one or more sub-vtables. Each sub-vtable is a sequence +// of components in the following order: +// Zero or more VCallOffset or VBaseOffset. +// One OffsetToTop. +// One RTTI. +// Zero or more FunctionPointer, CompleteDtorPointer, or DeletingDtorPointer. +// +// An object's vtable pointer points to the next component of the RTTI +// component. Hence, new components can be appended or prepended to sub-vtables +// without breaking compatibility. +static bool IsVTableExtended( + const std::vector &old_components, + const std::vector &new_components) { + const auto old_end = old_components.end(); + const auto new_end = new_components.end(); + auto old_it = old_components.begin(); + auto new_it = new_components.begin(); + bool is_extended = false; + while (old_it != old_end) { + const auto old_begin = old_it; + const auto new_begin = new_it; + // Iterate VCallOffset and VBaseOffset. + while (old_it != old_end && IsVOffset(old_it->GetKind())) { + old_it++; + } + while (new_it != new_end && IsVOffset(new_it->GetKind())) { + new_it++; + } + // Compare VCallOffset and VBaseOffset. + auto old_back_it = old_it; + auto new_back_it = new_it; + while (old_back_it != old_begin) { + if (new_back_it == new_begin) { + return false; + } + old_back_it--; + new_back_it--; + if (old_back_it->GetKind() != new_back_it->GetKind()) { + return false; + } + } + // The new sub-vtable has additional VOffsets at the beginning. + if (new_back_it != new_begin) { + is_extended = true; + } + // Compare OffsetToTop. + if (old_it == old_end || new_it == new_end || + old_it->GetKind() != VTableComponentIR::OffsetToTop || + new_it->GetKind() != VTableComponentIR::OffsetToTop) { + return false; + } + old_it++; + new_it++; + // Compare RTTI. + if (old_it == old_end || new_it == new_end || + old_it->GetKind() != VTableComponentIR::RTTI || + new_it->GetKind() != VTableComponentIR::RTTI || + old_it->GetName() != new_it->GetName()) { + return false; + } + old_it++; + new_it++; + // Compare function pointers. + while (old_it != old_end && IsFunctionPointer(old_it->GetKind())) { + if (new_it == new_end || old_it->GetKind() != new_it->GetKind() || + old_it->GetName() != new_it->GetName()) { + return false; + } + old_it++; + new_it++; + } + // The new sub-vtable has additional function pointers at the end. + while (new_it != new_end && IsFunctionPointer(new_it->GetKind())) { + is_extended = true; + new_it++; + } + } + return new_it == new_end ? is_extended : false; +} + bool AbiDiffHelper::AreOpaqueTypesEqual(const std::string &old_type_id, const std::string &new_type_id) const { // b/253095767: In T, some dump files contain opaque types whose IDs end with @@ -391,13 +476,18 @@ AbiDiffHelper::CompareRecordFields( std::move(*(diffed_field_ptr.second.release()))); } } + + DiffStatus &diff_status = diffed_removed_added_fields.diff_status_; + diff_status = DiffStatus::kNoDiff; if (diffed_removed_added_fields.diffed_fields_.size() != 0 || diffed_removed_added_fields.removed_fields_.size() != 0) { - diffed_removed_added_fields.diff_status_ = DiffStatus::kDirectDiff; - } else if (common_field_diff_exists) { - diffed_removed_added_fields.diff_status_ = DiffStatus::kIndirectDiff; - } else { - diffed_removed_added_fields.diff_status_ = DiffStatus::kNoDiff; + diff_status.CombineWith(DiffStatus::kDirectDiff); + } + if (diffed_removed_added_fields.added_fields_.size() != 0) { + diff_status.CombineWith(DiffStatus::kDirectExt); + } + if (common_field_diff_exists) { + diff_status.CombineWith(DiffStatus::kIndirectDiff); } return diffed_removed_added_fields; } @@ -533,44 +623,58 @@ DiffStatus AbiDiffHelper::CompareRecordTypes( DiffStatus final_diff_status = DiffStatus::kNoDiff; record_type_diff_ir->SetName(old_type->GetName()); record_type_diff_ir->SetLinkerSetKey(old_type->GetLinkerSetKey()); + if (IsAccessDowngraded(old_type->GetAccess(), new_type->GetAccess())) { - final_diff_status.CombineWith(DiffStatus::kIndirectDiff); + final_diff_status.CombineWith(DiffStatus::kDirectDiff); record_type_diff_ir->SetAccessDiff( std::make_unique( old_type->GetAccess(), new_type->GetAccess())); } if (!CompareSizeAndAlignment(old_type, new_type)) { - final_diff_status.CombineWith(DiffStatus::kIndirectDiff); + if (old_type->GetSize() < new_type->GetSize() && + old_type->GetAlignment() == new_type->GetAlignment()) { + final_diff_status.CombineWith(DiffStatus::kDirectExt); + } else { + final_diff_status.CombineWith(DiffStatus::kDirectDiff); + } record_type_diff_ir->SetTypeDiff( std::make_unique( std::make_pair(old_type->GetSize(), new_type->GetSize()), std::make_pair(old_type->GetAlignment(), new_type->GetAlignment()))); } - if (!CompareVTables(old_type, new_type)) { - final_diff_status.CombineWith(DiffStatus::kIndirectDiff); + + const std::vector &old_vtable = + old_type->GetVTableLayout().GetVTableComponents(); + const std::vector &new_vtable = + new_type->GetVTableLayout().GetVTableComponents(); + if (!CompareVTables(old_vtable, new_vtable)) { + if (IsVTableExtended(old_vtable, new_vtable)) { + final_diff_status.CombineWith(DiffStatus::kDirectExt); + } else { + final_diff_status.CombineWith(DiffStatus::kDirectDiff); + } record_type_diff_ir->SetVTableLayoutDiff( std::make_unique( old_type->GetVTableLayout(), new_type->GetVTableLayout())); } + auto &old_fields_dup = old_type->GetFields(); auto &new_fields_dup = new_type->GetFields(); auto field_status_and_diffs = CompareRecordFields( old_fields_dup, new_fields_dup, type_queue, diff_kind); - // TODO: Combine this with base class diffs as well. final_diff_status.CombineWith(field_status_and_diffs.diff_status_); std::vector old_bases = old_type->GetBases(); std::vector new_bases = new_type->GetBases(); - if (!CompareBaseSpecifiers(old_bases, new_bases, type_queue, diff_kind) && ir_diff_dumper_) { + final_diff_status.CombineWith(DiffStatus::kDirectDiff); ReplaceReferencesOtherTypeIdWithName(old_types_, old_bases); ReplaceReferencesOtherTypeIdWithName(new_types_, new_bases); - record_type_diff_ir->SetBaseSpecifierDiffs ( - std::make_unique(old_bases, - new_bases)); + record_type_diff_ir->SetBaseSpecifierDiffs( + std::make_unique(old_bases, new_bases)); } if (ir_diff_dumper_) { // Make copies of the fields removed and diffed, since we have to change @@ -596,8 +700,9 @@ DiffStatus AbiDiffHelper::CompareRecordTypes( record_type_diff_ir->SetFieldDiffs(std::move(field_diffs_fixed)); record_type_diff_ir->SetFieldsRemoved(std::move(fields_removed_fixed)); record_type_diff_ir->SetFieldsAdded(std::move(fields_added_fixed)); + record_type_diff_ir->SetExtended(final_diff_status.IsExtension()); - if (record_type_diff_ir->DiffExists() && + if (final_diff_status.IsDirectDiff() && !ir_diff_dumper_->AddDiffMessageIR(record_type_diff_ir.get(), Unwind(type_queue), diff_kind)) { llvm::errs() << "AddDiffMessage on record type failed\n"; 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 f421b6ab9..5483f1dcb 100644 --- a/vndk/tools/header-checker/src/repr/abi_diff_helpers.h +++ b/vndk/tools/header-checker/src/repr/abi_diff_helpers.h @@ -197,13 +197,6 @@ class AbiDiffHelper { std::deque *type_queue, IRDiffDumper::DiffKind diff_kind); - bool CompareVTables(const RecordTypeIR *old_record, - const RecordTypeIR *new_record); - - bool CompareVTableComponents( - const VTableComponentIR &old_component, - const VTableComponentIR &new_component); - DiffStatus CompareFunctionParameters( const std::vector &old_parameters, const std::vector &new_parameters, diff --git a/vndk/tools/header-checker/src/repr/ir_diff_representation.cpp b/vndk/tools/header-checker/src/repr/ir_diff_representation.cpp deleted file mode 100644 index a33c60863..000000000 --- a/vndk/tools/header-checker/src/repr/ir_diff_representation.cpp +++ /dev/null @@ -1,157 +0,0 @@ -// Copyright (C) 2022 The Android Open Source Project -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -#include "repr/ir_diff_representation.h" - -namespace header_checker { -namespace repr { - -static inline bool IsVOffset(VTableComponentIR::Kind kind) { - return kind == VTableComponentIR::VBaseOffset || - kind == VTableComponentIR::VCallOffset; -} - -static inline bool IsFunctionPointer(VTableComponentIR::Kind kind) { - return kind == VTableComponentIR::FunctionPointer || - kind == VTableComponentIR::CompleteDtorPointer || - kind == VTableComponentIR::DeletingDtorPointer; -} - -// A Vtable consists of one or more sub-vtables. Each sub-vtable is a sequence -// of components in the following order: -// Zero or more VCallOffset or VBaseOffset. -// One OffsetToTop. -// One RTTI. -// Zero or more FunctionPointer, CompleteDtorPointer, or DeletingDtorPointer. -// -// An object's vtable pointer points to the next component of the RTTI -// component. Hence, new components can be appended or prepended to sub-vtables -// without breaking compatibility. -bool VTableLayoutDiffIR::IsExtended() const { - const std::vector &old_components = - old_layout_.GetVTableComponents(); - const std::vector &new_components = - new_layout_.GetVTableComponents(); - const auto old_end = old_components.end(); - const auto new_end = new_components.end(); - auto old_it = old_components.begin(); - auto new_it = new_components.begin(); - bool is_extended = false; - while (old_it != old_end) { - const auto old_begin = old_it; - const auto new_begin = new_it; - // Iterate VCallOffset and VBaseOffset. - while (old_it != old_end && IsVOffset(old_it->GetKind())) { - old_it++; - } - while (new_it != new_end && IsVOffset(new_it->GetKind())) { - new_it++; - } - // Compare VCallOffset and VBaseOffset. - auto old_back_it = old_it; - auto new_back_it = new_it; - while (old_back_it != old_begin) { - if (new_back_it == new_begin) { - return false; - } - old_back_it--; - new_back_it--; - if (old_back_it->GetKind() != new_back_it->GetKind()) { - return false; - } - } - // The new sub-vtable has additional VOffsets at the beginning. - if (new_back_it != new_begin) { - is_extended = true; - } - // Compare OffsetToTop. - if (old_it == old_end || new_it == new_end || - old_it->GetKind() != VTableComponentIR::OffsetToTop || - new_it->GetKind() != VTableComponentIR::OffsetToTop) { - return false; - } - old_it++; - new_it++; - // Compare RTTI. - if (old_it == old_end || new_it == new_end || - old_it->GetKind() != VTableComponentIR::RTTI || - new_it->GetKind() != VTableComponentIR::RTTI || - old_it->GetName() != new_it->GetName()) { - return false; - } - old_it++; - new_it++; - // Compare function pointers. - while (old_it != old_end && IsFunctionPointer(old_it->GetKind())) { - if (new_it == new_end || old_it->GetKind() != new_it->GetKind() || - old_it->GetName() != new_it->GetName()) { - return false; - } - old_it++; - new_it++; - } - // The new sub-vtable has additional function pointers at the end. - while (new_it != new_end && IsFunctionPointer(new_it->GetKind())) { - is_extended = true; - new_it++; - } - } - return new_it == new_end ? is_extended : false; -} - -bool RecordTypeDiffIR::IsExtended() const { - bool is_extended = false; - if (type_diff_ != nullptr) { - auto sizes = type_diff_->GetSizes(); - if (sizes.first < sizes.second) { - is_extended = true; - } - if (sizes.first > sizes.second) { - return false; - } - auto alignments = type_diff_->GetAlignments(); - if (alignments.first != alignments.second) { - return false; - } - } - if (access_diff_ != nullptr) { - if (IsAccessDowngraded(access_diff_->GetOldAccess(), - access_diff_->GetNewAccess())) { - return false; - } - is_extended = true; - } - if (base_specifier_diffs_ != nullptr) { - return false; - } - if (vtable_diffs_ != nullptr) { - if (!vtable_diffs_->IsExtended()) { - return false; - } - is_extended = true; - } - // This function skips comparing the access specifiers of field_diffs_ - // because AbiDiffHelper::CompareCommonRecordFields does not report - // upgraded access specifiers as ABI difference. - if (field_diffs_.size() != 0 || fields_removed_.size() != 0) { - return false; - } - if (fields_added_.size() != 0) { - is_extended = true; - } - return is_extended; -} - -} // namespace repr -} // namespace header_checker diff --git a/vndk/tools/header-checker/src/repr/ir_diff_representation.h b/vndk/tools/header-checker/src/repr/ir_diff_representation.h index 81f8aff61..4c1ccbf76 100644 --- a/vndk/tools/header-checker/src/repr/ir_diff_representation.h +++ b/vndk/tools/header-checker/src/repr/ir_diff_representation.h @@ -102,8 +102,6 @@ class VTableLayoutDiffIR { return new_layout_; } - bool IsExtended() const; - protected: const VTableLayoutIR &old_layout_; const VTableLayoutIR &new_layout_; @@ -199,14 +197,7 @@ class RecordTypeDiffIR : public DiffMessageIR { linker_set_key_ = std::move(linker_set_key); } - bool DiffExists() const { - return (type_diff_ != nullptr) || (vtable_diffs_ != nullptr) || - (field_diffs_.size() != 0) || (fields_removed_.size() != 0) || - (fields_added_.size() != 0) || (access_diff_ != nullptr) || - (base_specifier_diffs_ != nullptr); - } - - bool IsExtended() const; + void SetExtended(bool is_extended) { is_extended_ = is_extended; } const TypeDiffIR *GetTypeDiff() const { return type_diff_.get(); @@ -222,6 +213,8 @@ class RecordTypeDiffIR : public DiffMessageIR { const std::string &GetLinkerSetKey() const { return linker_set_key_; } + bool IsExtended() const { return is_extended_; } + protected: // optional implemented with vector / std::unique_ptr. std::unique_ptr type_diff_; @@ -232,6 +225,7 @@ class RecordTypeDiffIR : public DiffMessageIR { std::unique_ptr access_diff_; std::unique_ptr base_specifier_diffs_; std::string linker_set_key_; + bool is_extended_ = false; }; class EnumFieldDiffIR {