From e90b33a9d9cc0a1599e9dd750bd3de4ad6217080 Mon Sep 17 00:00:00 2001 From: Jayant Chowdhary Date: Thu, 31 May 2018 15:31:32 -0700 Subject: [PATCH] header-abi-diff: Add a flag to consider opaque types with different names structurally unequal. Bug: 79576032 Test: tests/test.py Change-Id: I28cb7a2676a1e85e4ef1d2faaf6684d7c1824370 --- .../header-abi-diff/src/abi_diff.cpp | 4 +++- .../header-abi-diff/src/abi_diff.h | 7 +++++- .../header-abi-diff/src/abi_diff_wrappers.h | 5 +++-- .../header-abi-diff/src/header_abi_diff.cpp | 14 ++++++++++-- .../include/abi_diff_helpers.h | 13 ++++++++++- .../header-abi-util/src/abi_diff_helpers.cpp | 8 +++++++ .../header-abi-util/src/ir_representation.cpp | 4 +++- .../integration/c_and_cpp/include/c_include.h | 8 +++++++ vndk/tools/header-checker/tests/module.py | 22 +++++++++++++++++++ vndk/tools/header-checker/tests/test.py | 7 ++++++ 10 files changed, 84 insertions(+), 8 deletions(-) diff --git a/vndk/tools/header-checker/header-abi-diff/src/abi_diff.cpp b/vndk/tools/header-checker/header-abi-diff/src/abi_diff.cpp index 936d905c0..9d8acf5e4 100644 --- a/vndk/tools/header-checker/header-abi-diff/src/abi_diff.cpp +++ b/vndk/tools/header-checker/header-abi-diff/src/abi_diff.cpp @@ -360,7 +360,9 @@ bool HeaderAbiDiff::DumpDiffElements( } abi_diff_wrappers::DiffWrapper diff_wrapper(old_element, new_element, ir_diff_dumper, old_types, - new_types, &type_cache_); + new_types, + diff_policy_options_, + &type_cache_); if (!diff_wrapper.DumpDiff(diff_kind)) { llvm::errs() << "Failed to diff elements\n"; return false; diff --git a/vndk/tools/header-checker/header-abi-diff/src/abi_diff.h b/vndk/tools/header-checker/header-abi-diff/src/abi_diff.h index 18e785dac..9dcedc07e 100644 --- a/vndk/tools/header-checker/header-abi-diff/src/abi_diff.h +++ b/vndk/tools/header-checker/header-abi-diff/src/abi_diff.h @@ -20,6 +20,7 @@ #include using abi_util::AbiElementMap; +using abi_util::DiffPolicyOptions; class HeaderAbiDiff { public: @@ -27,12 +28,15 @@ class HeaderAbiDiff { const std::string &old_dump, const std::string &new_dump, const std::string &compatibility_report, const std::set &ignored_symbols, + const DiffPolicyOptions &diff_policy_options, bool check_all_apis, abi_util::TextFormatIR text_format_old, abi_util::TextFormatIR text_format_new, abi_util::TextFormatIR text_format_diff) : lib_name_(lib_name), arch_(arch), old_dump_(old_dump), new_dump_(new_dump), cr_(compatibility_report), - ignored_symbols_(ignored_symbols), check_all_apis_(check_all_apis), + ignored_symbols_(ignored_symbols), + diff_policy_options_(diff_policy_options), + check_all_apis_(check_all_apis), text_format_old_(text_format_old), text_format_new_(text_format_new), text_format_diff_(text_format_diff) { } @@ -134,6 +138,7 @@ class HeaderAbiDiff { const std::string &new_dump_; const std::string &cr_; const std::set &ignored_symbols_; + const DiffPolicyOptions &diff_policy_options_; bool check_all_apis_; std::set type_cache_; abi_util::TextFormatIR text_format_old_; diff --git a/vndk/tools/header-checker/header-abi-diff/src/abi_diff_wrappers.h b/vndk/tools/header-checker/header-abi-diff/src/abi_diff_wrappers.h index 5376d2ffd..92c04758e 100644 --- a/vndk/tools/header-checker/header-abi-diff/src/abi_diff_wrappers.h +++ b/vndk/tools/header-checker/header-abi-diff/src/abi_diff_wrappers.h @@ -42,9 +42,10 @@ class DiffWrapper : public AbiDiffHelper { abi_util::IRDiffDumper *ir_diff_dumper, const AbiElementMap &old_types, const AbiElementMap &new_types, + const abi_util::DiffPolicyOptions &diff_policy_options, std::set *type_cache) - : AbiDiffHelper(old_types, new_types, type_cache, ir_diff_dumper), - oldp_(oldp), newp_(newp) { } + : AbiDiffHelper(old_types, new_types, diff_policy_options, type_cache, + ir_diff_dumper), oldp_(oldp), newp_(newp) { } bool DumpDiff(abi_util::IRDiffDumper::DiffKind diff_kind); diff --git a/vndk/tools/header-checker/header-abi-diff/src/header_abi_diff.cpp b/vndk/tools/header-checker/header-abi-diff/src/header_abi_diff.cpp index f5764707d..9cb95c809 100644 --- a/vndk/tools/header-checker/header-abi-diff/src/header_abi_diff.cpp +++ b/vndk/tools/header-checker/header-abi-diff/src/header_abi_diff.cpp @@ -85,6 +85,14 @@ static llvm::cl::opt allow_unreferenced_changes( " APIs."), llvm::cl::Optional, llvm::cl::cat(header_checker_category)); +static llvm::cl::opt consider_opaque_types_different( + "consider-opaque-types-different", + llvm::cl::desc("Consider opaque types with different names as different" + " .This should not be used while comparing C++ library" + " ABIs"), + llvm::cl::Optional, llvm::cl::cat(header_checker_category)); + + static llvm::cl::opt text_format_old( "text-format-old", llvm::cl::desc("Specify text format of old abi dump"), llvm::cl::values(clEnumValN(abi_util::TextFormatIR::ProtobufTextFormat, @@ -142,9 +150,11 @@ int main(int argc, const char **argv) { if (llvm::sys::fs::exists(ignore_symbol_list)) { ignored_symbols = LoadIgnoredSymbols(ignore_symbol_list); } + abi_util::DiffPolicyOptions diff_policy_options( + consider_opaque_types_different); HeaderAbiDiff judge(lib_name, arch, old_dump, new_dump, compatibility_report, - ignored_symbols, check_all_apis, text_format_old, - text_format_new, text_format_diff); + ignored_symbols, diff_policy_options, check_all_apis, + text_format_old, text_format_new, text_format_diff); abi_util::CompatibilityStatusIR status = judge.GenerateCompatibilityReport(); diff --git a/vndk/tools/header-checker/header-abi-util/include/abi_diff_helpers.h b/vndk/tools/header-checker/header-abi-util/include/abi_diff_helpers.h index 64033325c..839bac038 100644 --- a/vndk/tools/header-checker/header-abi-util/include/abi_diff_helpers.h +++ b/vndk/tools/header-checker/header-abi-util/include/abi_diff_helpers.h @@ -19,6 +19,8 @@ enum DiffStatus { // There was a diff found, however it need not be added as a part of a diff // message, since it would have already been noted elsewhere. indirect_diff = 2, + + opaque_diff = 3, }; static inline DiffStatus operator| (DiffStatus f,DiffStatus s) { @@ -46,16 +48,24 @@ struct GenericFieldDiffInfo { std::string Unwind(const std::deque *type_queue); +struct DiffPolicyOptions { + DiffPolicyOptions(bool consider_opaque_types_different) : + consider_opaque_types_different_(consider_opaque_types_different) { } + bool consider_opaque_types_different_ = false; +}; + class AbiDiffHelper { public: AbiDiffHelper( const AbiElementMap &old_types, const AbiElementMap &new_types, + const DiffPolicyOptions &diff_policy_options, std::set *type_cache, abi_util::IRDiffDumper *ir_diff_dumper = nullptr, AbiElementMap *local_to_global_type_id_map = nullptr) : old_types_(old_types), new_types_(new_types), - type_cache_(type_cache), ir_diff_dumper_(ir_diff_dumper), + diff_policy_options_(diff_policy_options), type_cache_(type_cache), + ir_diff_dumper_(ir_diff_dumper), local_to_global_type_id_map_(local_to_global_type_id_map) { } DiffStatus CompareAndDumpTypeDiff( @@ -175,6 +185,7 @@ class AbiDiffHelper { protected: const AbiElementMap &old_types_; const AbiElementMap &new_types_; + const DiffPolicyOptions &diff_policy_options_; std::set *type_cache_ = nullptr; abi_util::IRDiffDumper *ir_diff_dumper_ = nullptr; AbiElementMap *local_to_global_type_id_map_ = nullptr; diff --git a/vndk/tools/header-checker/header-abi-util/src/abi_diff_helpers.cpp b/vndk/tools/header-checker/header-abi-util/src/abi_diff_helpers.cpp index 3d19d71c7..616f28f18 100644 --- a/vndk/tools/header-checker/header-abi-util/src/abi_diff_helpers.cpp +++ b/vndk/tools/header-checker/header-abi-util/src/abi_diff_helpers.cpp @@ -786,6 +786,9 @@ DiffStatus AbiDiffHelper::CompareAndDumpTypeDiff( if (old_it == old_types_.end() || new_it == new_types_.end()) { TypeQueueCheckAndPop(type_queue); // One of the types were hidden, we cannot compare further. + if (diff_policy_options_.consider_opaque_types_different_) { + return DiffStatus::opaque_diff; + } return DiffStatus::no_diff; } abi_util::LinkableMessageKind old_kind = @@ -800,6 +803,11 @@ DiffStatus AbiDiffHelper::CompareAndDumpTypeDiff( old_kind, type_queue, diff_kind); } TypeQueueCheckAndPop(type_queue); + if (diff_policy_options_.consider_opaque_types_different_ && + diff_status == DiffStatus::opaque_diff && + (old_it->second->GetName() != new_it->second->GetName())) { + return DiffStatus::direct_diff; + } return diff_status; } diff --git a/vndk/tools/header-checker/header-abi-util/src/ir_representation.cpp b/vndk/tools/header-checker/header-abi-util/src/ir_representation.cpp index 8ccb5bef6..cf1e2fb0f 100644 --- a/vndk/tools/header-checker/header-abi-util/src/ir_representation.cpp +++ b/vndk/tools/header-checker/header-abi-util/src/ir_representation.cpp @@ -123,7 +123,9 @@ MergeStatus TextFormatToIRReader::DoesUDTypeODRViolationExist( return MergeStatus(true, ""); } std::set type_cache; - AbiDiffHelper diff_helper(type_graph_, addend.type_graph_, &type_cache, + DiffPolicyOptions diff_policy_options(false) ; + AbiDiffHelper diff_helper(type_graph_, addend.type_graph_, + diff_policy_options, &type_cache, nullptr, local_to_global_type_id_map_); for (auto &contender_ud : it->second) { if (diff_helper.CompareAndDumpTypeDiff(contender_ud->GetSelfType(), diff --git a/vndk/tools/header-checker/tests/integration/c_and_cpp/include/c_include.h b/vndk/tools/header-checker/tests/integration/c_and_cpp/include/c_include.h index 42fc6e07e..d19ce8dc9 100644 --- a/vndk/tools/header-checker/tests/integration/c_and_cpp/include/c_include.h +++ b/vndk/tools/header-checker/tests/integration/c_and_cpp/include/c_include.h @@ -6,9 +6,17 @@ struct Cinner { int c; }; +struct opaque_a; +struct opaque_b; + struct Cstruct { int a; struct Cinner *b; +#ifdef OPAQUE_STRUCT_A + struct opaque_a *op; +#elif OPAQUE_STRUCT_B + struct opaque_b *op; +#endif }; void CFunction(struct Cstruct **cstruct); diff --git a/vndk/tools/header-checker/tests/module.py b/vndk/tools/header-checker/tests/module.py index 0df8b2adf..b7f9cde4b 100755 --- a/vndk/tools/header-checker/tests/module.py +++ b/vndk/tools/header-checker/tests/module.py @@ -125,6 +125,28 @@ TEST_MODULES = [ arch = '', api = 'current', ), + Module( + name = 'libc_and_cpp_with_opaque_ptr_a', + srcs = ['integration/c_and_cpp/source1.cpp', + 'integration/c_and_cpp/source2.c', + ], + version_script = 'integration/c_and_cpp/map.txt', + export_include_dirs = ['integration/c_and_cpp/include'], + cflags = ['-DOPAQUE_STRUCT_A=1'], + arch = '', + api = 'current', + ), + Module( + name = 'libc_and_cpp_with_opaque_ptr_b', + srcs = ['integration/c_and_cpp/source1.cpp', + 'integration/c_and_cpp/source2.c', + ], + version_script = 'integration/c_and_cpp/map.txt', + export_include_dirs = ['integration/c_and_cpp/include'], + cflags = ['-DOPAQUE_STRUCT_B=1'], + arch = '', + api = 'current', + ), Module( name = 'libc_and_cpp_with_unused_struct', srcs = ['integration/c_and_cpp/source1.cpp', diff --git a/vndk/tools/header-checker/tests/test.py b/vndk/tools/header-checker/tests/test.py index ed186ad93..8196a33c4 100755 --- a/vndk/tools/header-checker/tests/test.py +++ b/vndk/tools/header-checker/tests/test.py @@ -146,6 +146,13 @@ class MyTest(unittest.TestCase): "libc_and_cpp", "libc_and_cpp_with_unused_struct", 0, ['-check-all-apis', '-advice-only']) + def test_libc_and_cpp_opaque_pointer_diff(self): + self.prepare_and_run_abi_diff_all_archs( + "libc_and_cpp_with_opaque_ptr_a", + "libc_and_cpp_with_opaque_ptr_b", 8, + ['-consider-opaque-types-different'], True, + True) + def test_libgolden_cpp_return_type_diff(self): self.prepare_and_run_abi_diff_all_archs( "libgolden_cpp", "libgolden_cpp_return_type_diff", 8)