From d875d513f28529e39f2c31fc673214a94ac0f2fb Mon Sep 17 00:00:00 2001 From: Hsin-Yi Chen Date: Wed, 28 Sep 2022 14:23:53 +0800 Subject: [PATCH] Do not allow extending pass-by-value parameters or return types Resizing the parameters or return types changes the stack layout. It is not an allowed extension to functions. AbiDiffHelper::CompareFunctionTypes additionally checks the sizes of the parameters and the return types. The difference cannot be ignored by -allow-extensions. The user who intends to ignore the difference should specify -ignore-symbols or -ignore-linker-set-key. Test: ./test.py Bug: 248418092 Change-Id: Ibef3b9260504afff3fc0260b0565736133b8e0dc --- .../src/diff/abi_diff_wrappers.cpp | 10 +- .../src/repr/abi_diff_helpers.cpp | 50 +++++++--- .../src/repr/abi_diff_helpers.h | 15 ++- .../integration/pass_by_value/include/base.h | 10 ++ .../pass_by_value/include/param_size_diff.h | 11 +++ .../pass_by_value/include/return_size_diff.h | 11 +++ .../tests/integration/pass_by_value/map.txt | 5 + vndk/tools/header-checker/tests/module.py | 27 +++++ .../arm64/libparam_size_diff.so.lsdump | 99 +++++++++++++++++++ .../arm64/libpass_by_value.so.lsdump | 94 ++++++++++++++++++ .../arm64/libreturn_size_diff.so.lsdump | 99 +++++++++++++++++++ vndk/tools/header-checker/tests/test.py | 12 +++ 12 files changed, 416 insertions(+), 27 deletions(-) create mode 100644 vndk/tools/header-checker/tests/integration/pass_by_value/include/base.h create mode 100644 vndk/tools/header-checker/tests/integration/pass_by_value/include/param_size_diff.h create mode 100644 vndk/tools/header-checker/tests/integration/pass_by_value/include/return_size_diff.h create mode 100644 vndk/tools/header-checker/tests/integration/pass_by_value/map.txt create mode 100644 vndk/tools/header-checker/tests/reference_dumps/arm64/libparam_size_diff.so.lsdump create mode 100644 vndk/tools/header-checker/tests/reference_dumps/arm64/libpass_by_value.so.lsdump create mode 100644 vndk/tools/header-checker/tests/reference_dumps/arm64/libreturn_size_diff.so.lsdump diff --git a/vndk/tools/header-checker/src/diff/abi_diff_wrappers.cpp b/vndk/tools/header-checker/src/diff/abi_diff_wrappers.cpp index 72fa69b33..87ac0fe6d 100644 --- a/vndk/tools/header-checker/src/diff/abi_diff_wrappers.cpp +++ b/vndk/tools/header-checker/src/diff/abi_diff_wrappers.cpp @@ -90,18 +90,14 @@ bool DiffWrapper::DumpDiff( std::deque type_queue; type_queue.push_back(oldp_->GetName()); - DiffStatus param_diffs = CompareFunctionParameters( - oldp_->GetParameters(), newp_->GetParameters(), &type_queue, diff_kind); - - DiffStatus return_type_diff = CompareAndDumpTypeDiff( - oldp_->GetReturnType(), newp_->GetReturnType(), &type_queue, diff_kind); + DiffStatus function_type_diff = + CompareFunctionTypes(oldp_, newp_, &type_queue, diff_kind); CompareTemplateInfo(oldp_->GetTemplateElements(), newp_->GetTemplateElements(), &type_queue, diff_kind); - if ((param_diffs == DiffStatus::direct_diff || - return_type_diff == DiffStatus::direct_diff) || + if ((function_type_diff == DiffStatus::direct_diff) || (oldp_->GetAccess() != newp_->GetAccess())) { repr::FunctionIR old_function = *oldp_; repr::FunctionIR new_function = *newp_; 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 2f5c9f9e5..39f418024 100644 --- a/vndk/tools/header-checker/src/repr/abi_diff_helpers.cpp +++ b/vndk/tools/header-checker/src/repr/abi_diff_helpers.cpp @@ -260,13 +260,26 @@ bool AbiDiffHelper::CompareVTables( return true; } -bool AbiDiffHelper::CompareSizeAndAlignment( - const TypeIR *old_type, - const TypeIR *new_type) { +static bool CompareSizeAndAlignment(const TypeIR *old_type, + const TypeIR *new_type) { return old_type->GetSize() == new_type->GetSize() && old_type->GetAlignment() == new_type->GetAlignment(); } +bool AbiDiffHelper::AreTypeSizeAndAlignmentEqual( + const std::string &old_type_id, const std::string &new_type_id) const { + AbiElementMap::const_iterator old_it = + old_types_.find(old_type_id); + AbiElementMap::const_iterator new_it = + new_types_.find(new_type_id); + + if (old_it == old_types_.end() || new_it == new_types_.end()) { + return !diff_policy_options_.consider_opaque_types_different_; + } + + return CompareSizeAndAlignment(old_it->second, new_it->second); +} + DiffStatusPair> AbiDiffHelper::CompareCommonRecordFields( const RecordFieldIR *old_field, @@ -496,17 +509,14 @@ AbiDiffHelper::FixupDiffedFieldTypeIds( } DiffStatus AbiDiffHelper::CompareFunctionTypes( - const FunctionTypeIR *old_type, - const FunctionTypeIR *new_type, - std::deque *type_queue, - DiffMessageIR::DiffKind diff_kind) { + const CFunctionLikeIR *old_type, const CFunctionLikeIR *new_type, + std::deque *type_queue, DiffMessageIR::DiffKind diff_kind) { DiffStatus param_diffs = CompareFunctionParameters(old_type->GetParameters(), new_type->GetParameters(), type_queue, diff_kind); - DiffStatus return_type_diff = - CompareAndDumpTypeDiff(old_type->GetReturnType(), - new_type->GetReturnType(), - type_queue, diff_kind); + DiffStatus return_type_diff = CompareParameterOrReturnType( + old_type->GetReturnType(), new_type->GetReturnType(), type_queue, + diff_kind); if (param_diffs == DiffStatus::direct_diff || return_type_diff == DiffStatus::direct_diff) { @@ -698,10 +708,10 @@ DiffStatus AbiDiffHelper::CompareFunctionParameters( while (i < old_parameters_size) { const ParamIR &old_parameter = old_parameters.at(i); const ParamIR &new_parameter = new_parameters.at(i); - if ((CompareAndDumpTypeDiff(old_parameter.GetReferencedType(), - new_parameter.GetReferencedType(), - type_queue, diff_kind) == - DiffStatus::direct_diff) || + if (CompareParameterOrReturnType(old_parameter.GetReferencedType(), + new_parameter.GetReferencedType(), + type_queue, + diff_kind) == DiffStatus::direct_diff || (old_parameter.GetIsDefault() != new_parameter.GetIsDefault())) { return DiffStatus::direct_diff; } @@ -710,6 +720,16 @@ DiffStatus AbiDiffHelper::CompareFunctionParameters( return DiffStatus::no_diff; } +DiffStatus AbiDiffHelper::CompareParameterOrReturnType( + const std::string &old_type_id, const std::string &new_type_id, + std::deque *type_queue, DiffMessageIR::DiffKind diff_kind) { + if (!AreTypeSizeAndAlignmentEqual(old_type_id, new_type_id)) { + return DiffStatus::direct_diff; + } + return CompareAndDumpTypeDiff(old_type_id, new_type_id, type_queue, + diff_kind); +} + DiffStatus AbiDiffHelper::CompareAndDumpTypeDiff( const TypeIR *old_type, const TypeIR *new_type, LinkableMessageKind kind, std::deque *type_queue, 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 2d0774b28..a1673e583 100644 --- a/vndk/tools/header-checker/src/repr/abi_diff_helpers.h +++ b/vndk/tools/header-checker/src/repr/abi_diff_helpers.h @@ -88,6 +88,9 @@ class AbiDiffHelper { ignored_linker_set_keys_(ignored_linker_set_keys), ir_diff_dumper_(ir_diff_dumper) {} + bool AreTypeSizeAndAlignmentEqual(const std::string &old_type_str, + const std::string &new_type_str) const; + DiffStatus CompareAndDumpTypeDiff( const std::string &old_type_str, const std::string &new_type_str, std::deque *type_queue = nullptr, @@ -110,8 +113,8 @@ class AbiDiffHelper { std::deque *type_queue, IRDiffDumper::DiffKind diff_kind); - DiffStatus CompareFunctionTypes(const FunctionTypeIR *old_type, - const FunctionTypeIR *new_type, + DiffStatus CompareFunctionTypes(const CFunctionLikeIR *old_type, + const CFunctionLikeIR *new_type, std::deque *type_queue, DiffMessageIR::DiffKind diff_kind); @@ -121,6 +124,11 @@ class AbiDiffHelper { std::deque *type_queue, IRDiffDumper::DiffKind diff_kind); + DiffStatus CompareParameterOrReturnType(const std::string &old_type_id, + const std::string &new_type_id, + std::deque *type_queue, + IRDiffDumper::DiffKind diff_kind); + DiffStatus CompareTemplateInfo( const std::vector &old_template_elements, const std::vector &new_template_elements, @@ -198,9 +206,6 @@ class AbiDiffHelper { const VTableComponentIR &old_component, const VTableComponentIR &new_component); - bool CompareSizeAndAlignment(const TypeIR *old_ti, - const TypeIR *new_ti); - template bool AddToDiff(DiffType *mutable_diff, const DiffElement *oldp, const DiffElement *newp, diff --git a/vndk/tools/header-checker/tests/integration/pass_by_value/include/base.h b/vndk/tools/header-checker/tests/integration/pass_by_value/include/base.h new file mode 100644 index 000000000..62ea96e7a --- /dev/null +++ b/vndk/tools/header-checker/tests/integration/pass_by_value/include/base.h @@ -0,0 +1,10 @@ +struct Parameter { + int member; +}; + +struct Return { + int member; +}; + +void PassByValue(Parameter); +Return ReturnByValue(); diff --git a/vndk/tools/header-checker/tests/integration/pass_by_value/include/param_size_diff.h b/vndk/tools/header-checker/tests/integration/pass_by_value/include/param_size_diff.h new file mode 100644 index 000000000..218414bdd --- /dev/null +++ b/vndk/tools/header-checker/tests/integration/pass_by_value/include/param_size_diff.h @@ -0,0 +1,11 @@ +struct Parameter { + int member; + int extra_member; +}; + +struct Return { + int member; +}; + +void PassByValue(Parameter); +Return ReturnByValue(); diff --git a/vndk/tools/header-checker/tests/integration/pass_by_value/include/return_size_diff.h b/vndk/tools/header-checker/tests/integration/pass_by_value/include/return_size_diff.h new file mode 100644 index 000000000..c2ec6553b --- /dev/null +++ b/vndk/tools/header-checker/tests/integration/pass_by_value/include/return_size_diff.h @@ -0,0 +1,11 @@ +struct Parameter { + int member; +}; + +struct Return { + int member; + int extra_member; +}; + +void PassByValue(Parameter); +Return ReturnByValue(); diff --git a/vndk/tools/header-checker/tests/integration/pass_by_value/map.txt b/vndk/tools/header-checker/tests/integration/pass_by_value/map.txt new file mode 100644 index 000000000..6b82211d9 --- /dev/null +++ b/vndk/tools/header-checker/tests/integration/pass_by_value/map.txt @@ -0,0 +1,5 @@ +libpass_by_value { + global: + _Z11PassByValue9Parameter; + _Z13ReturnByValuev; +}; diff --git a/vndk/tools/header-checker/tests/module.py b/vndk/tools/header-checker/tests/module.py index 1849612ad..2824f7cb4 100755 --- a/vndk/tools/header-checker/tests/module.py +++ b/vndk/tools/header-checker/tests/module.py @@ -688,6 +688,33 @@ TEST_MODULES = [ linker_flags=['-output-format', 'Json'], has_reference_dump=True, ), + LsdumpModule( + name='libpass_by_value', + arch='arm64', + srcs=['integration/pass_by_value/include/base.h'], + version_script='integration/pass_by_value/map.txt', + export_include_dirs=['integration/pass_by_value/include'], + linker_flags=['-output-format', 'Json'], + has_reference_dump=True, + ), + LsdumpModule( + name='libparam_size_diff', + arch='arm64', + srcs=['integration/pass_by_value/include/param_size_diff.h'], + version_script='integration/pass_by_value/map.txt', + export_include_dirs=['integration/pass_by_value/include'], + linker_flags=['-output-format', 'Json'], + has_reference_dump=True, + ), + LsdumpModule( + name='libreturn_size_diff', + arch='arm64', + srcs=['integration/pass_by_value/include/return_size_diff.h'], + version_script='integration/pass_by_value/map.txt', + export_include_dirs=['integration/pass_by_value/include'], + linker_flags=['-output-format', 'Json'], + has_reference_dump=True, + ), ] TEST_MODULES = {m.name: m for m in TEST_MODULES} diff --git a/vndk/tools/header-checker/tests/reference_dumps/arm64/libparam_size_diff.so.lsdump b/vndk/tools/header-checker/tests/reference_dumps/arm64/libparam_size_diff.so.lsdump new file mode 100644 index 000000000..331ec285e --- /dev/null +++ b/vndk/tools/header-checker/tests/reference_dumps/arm64/libparam_size_diff.so.lsdump @@ -0,0 +1,99 @@ +{ + "array_types" : [], + "builtin_types" : + [ + { + "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" : "_Z11PassByValue9Parameter" + }, + { + "name" : "_Z13ReturnByValuev" + } + ], + "elf_objects" : [], + "enum_types" : [], + "function_types" : [], + "functions" : + [ + { + "function_name" : "PassByValue", + "linker_set_key" : "_Z11PassByValue9Parameter", + "parameters" : + [ + { + "referenced_type" : "_ZTI9Parameter" + } + ], + "return_type" : "_ZTIv", + "source_file" : "development/vndk/tools/header-checker/tests/integration/pass_by_value/include/param_size_diff.h" + }, + { + "function_name" : "ReturnByValue", + "linker_set_key" : "_Z13ReturnByValuev", + "return_type" : "_ZTI6Return", + "source_file" : "development/vndk/tools/header-checker/tests/integration/pass_by_value/include/param_size_diff.h" + } + ], + "global_vars" : [], + "lvalue_reference_types" : [], + "pointer_types" : [], + "qualified_types" : [], + "record_types" : + [ + { + "alignment" : 4, + "fields" : + [ + { + "field_name" : "member", + "referenced_type" : "_ZTIi" + } + ], + "linker_set_key" : "_ZTI6Return", + "name" : "Return", + "referenced_type" : "_ZTI6Return", + "self_type" : "_ZTI6Return", + "size" : 4, + "source_file" : "development/vndk/tools/header-checker/tests/integration/pass_by_value/include/param_size_diff.h" + }, + { + "alignment" : 4, + "fields" : + [ + { + "field_name" : "member", + "referenced_type" : "_ZTIi" + }, + { + "field_name" : "extra_member", + "field_offset" : 32, + "referenced_type" : "_ZTIi" + } + ], + "linker_set_key" : "_ZTI9Parameter", + "name" : "Parameter", + "referenced_type" : "_ZTI9Parameter", + "self_type" : "_ZTI9Parameter", + "size" : 8, + "source_file" : "development/vndk/tools/header-checker/tests/integration/pass_by_value/include/param_size_diff.h" + } + ], + "rvalue_reference_types" : [] +} diff --git a/vndk/tools/header-checker/tests/reference_dumps/arm64/libpass_by_value.so.lsdump b/vndk/tools/header-checker/tests/reference_dumps/arm64/libpass_by_value.so.lsdump new file mode 100644 index 000000000..d6598b9a7 --- /dev/null +++ b/vndk/tools/header-checker/tests/reference_dumps/arm64/libpass_by_value.so.lsdump @@ -0,0 +1,94 @@ +{ + "array_types" : [], + "builtin_types" : + [ + { + "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" : "_Z11PassByValue9Parameter" + }, + { + "name" : "_Z13ReturnByValuev" + } + ], + "elf_objects" : [], + "enum_types" : [], + "function_types" : [], + "functions" : + [ + { + "function_name" : "PassByValue", + "linker_set_key" : "_Z11PassByValue9Parameter", + "parameters" : + [ + { + "referenced_type" : "_ZTI9Parameter" + } + ], + "return_type" : "_ZTIv", + "source_file" : "development/vndk/tools/header-checker/tests/integration/pass_by_value/include/base.h" + }, + { + "function_name" : "ReturnByValue", + "linker_set_key" : "_Z13ReturnByValuev", + "return_type" : "_ZTI6Return", + "source_file" : "development/vndk/tools/header-checker/tests/integration/pass_by_value/include/base.h" + } + ], + "global_vars" : [], + "lvalue_reference_types" : [], + "pointer_types" : [], + "qualified_types" : [], + "record_types" : + [ + { + "alignment" : 4, + "fields" : + [ + { + "field_name" : "member", + "referenced_type" : "_ZTIi" + } + ], + "linker_set_key" : "_ZTI6Return", + "name" : "Return", + "referenced_type" : "_ZTI6Return", + "self_type" : "_ZTI6Return", + "size" : 4, + "source_file" : "development/vndk/tools/header-checker/tests/integration/pass_by_value/include/base.h" + }, + { + "alignment" : 4, + "fields" : + [ + { + "field_name" : "member", + "referenced_type" : "_ZTIi" + } + ], + "linker_set_key" : "_ZTI9Parameter", + "name" : "Parameter", + "referenced_type" : "_ZTI9Parameter", + "self_type" : "_ZTI9Parameter", + "size" : 4, + "source_file" : "development/vndk/tools/header-checker/tests/integration/pass_by_value/include/base.h" + } + ], + "rvalue_reference_types" : [] +} diff --git a/vndk/tools/header-checker/tests/reference_dumps/arm64/libreturn_size_diff.so.lsdump b/vndk/tools/header-checker/tests/reference_dumps/arm64/libreturn_size_diff.so.lsdump new file mode 100644 index 000000000..48b480fce --- /dev/null +++ b/vndk/tools/header-checker/tests/reference_dumps/arm64/libreturn_size_diff.so.lsdump @@ -0,0 +1,99 @@ +{ + "array_types" : [], + "builtin_types" : + [ + { + "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" : "_Z11PassByValue9Parameter" + }, + { + "name" : "_Z13ReturnByValuev" + } + ], + "elf_objects" : [], + "enum_types" : [], + "function_types" : [], + "functions" : + [ + { + "function_name" : "PassByValue", + "linker_set_key" : "_Z11PassByValue9Parameter", + "parameters" : + [ + { + "referenced_type" : "_ZTI9Parameter" + } + ], + "return_type" : "_ZTIv", + "source_file" : "development/vndk/tools/header-checker/tests/integration/pass_by_value/include/return_size_diff.h" + }, + { + "function_name" : "ReturnByValue", + "linker_set_key" : "_Z13ReturnByValuev", + "return_type" : "_ZTI6Return", + "source_file" : "development/vndk/tools/header-checker/tests/integration/pass_by_value/include/return_size_diff.h" + } + ], + "global_vars" : [], + "lvalue_reference_types" : [], + "pointer_types" : [], + "qualified_types" : [], + "record_types" : + [ + { + "alignment" : 4, + "fields" : + [ + { + "field_name" : "member", + "referenced_type" : "_ZTIi" + }, + { + "field_name" : "extra_member", + "field_offset" : 32, + "referenced_type" : "_ZTIi" + } + ], + "linker_set_key" : "_ZTI6Return", + "name" : "Return", + "referenced_type" : "_ZTI6Return", + "self_type" : "_ZTI6Return", + "size" : 8, + "source_file" : "development/vndk/tools/header-checker/tests/integration/pass_by_value/include/return_size_diff.h" + }, + { + "alignment" : 4, + "fields" : + [ + { + "field_name" : "member", + "referenced_type" : "_ZTIi" + } + ], + "linker_set_key" : "_ZTI9Parameter", + "name" : "Parameter", + "referenced_type" : "_ZTI9Parameter", + "self_type" : "_ZTI9Parameter", + "size" : 4, + "source_file" : "development/vndk/tools/header-checker/tests/integration/pass_by_value/include/return_size_diff.h" + } + ], + "rvalue_reference_types" : [] +} diff --git a/vndk/tools/header-checker/tests/test.py b/vndk/tools/header-checker/tests/test.py index 849e36e11..e103fc3ee 100755 --- a/vndk/tools/header-checker/tests/test.py +++ b/vndk/tools/header-checker/tests/test.py @@ -407,6 +407,18 @@ class HeaderCheckerTest(unittest.TestCase): flags=["-input-format-new", "Json", "-input-format-old", "Json"], create_old=False, create_new=False) + def test_param_size_diff(self): + self.prepare_and_run_abi_diff_all_archs( + "libpass_by_value", "libparam_size_diff", 8, + flags=["-input-format-new", "Json", "-input-format-old", "Json"], + create_old=False, create_new=False) + + def test_return_size_diff(self): + self.prepare_and_run_abi_diff_all_archs( + "libpass_by_value", "libreturn_size_diff", 8, + flags=["-input-format-new", "Json", "-input-format-old", "Json"], + create_old=False, create_new=False) + if __name__ == '__main__': unittest.main()