From 62081d11b4c60f62c042d028e31f4aca6aebb455 Mon Sep 17 00:00:00 2001 From: Hsin-Yi Chen Date: Tue, 11 Apr 2023 18:36:15 +0800 Subject: [PATCH] Fix the internal type of enum values EnumFieldIR has member functions that determine whether the enum value is signed. The JSON dumper uses the functions to distinguish between negative values and values >= (1 << 63). Bug: 277299761 Test: ./test.py Change-Id: If5f3a33aaf5f53f39019fdd10dde366d48a50340 --- .../src/dumper/abi_wrappers.cpp | 8 +- .../src/repr/abi_diff_helpers.cpp | 5 +- .../src/repr/ir_representation.h | 21 ++- .../src/repr/json/ir_dumper.cpp | 7 +- .../src/repr/json/ir_reader.cpp | 19 +- .../header-checker/src/repr/json/ir_reader.h | 3 + .../src/repr/protobuf/converter.h | 6 +- .../tests/integration/enum/include/base.h | 23 +++ .../tests/integration/enum/map.txt | 4 + vndk/tools/header-checker/tests/module.py | 10 ++ .../reference_dumps/arm64/libenum.so.lsdump | 167 ++++++++++++++++++ vndk/tools/header-checker/tests/test.py | 3 + 12 files changed, 259 insertions(+), 17 deletions(-) create mode 100644 vndk/tools/header-checker/tests/integration/enum/include/base.h create mode 100644 vndk/tools/header-checker/tests/integration/enum/map.txt create mode 100644 vndk/tools/header-checker/tests/reference_dumps/arm64/libenum.so.lsdump diff --git a/vndk/tools/header-checker/src/dumper/abi_wrappers.cpp b/vndk/tools/header-checker/src/dumper/abi_wrappers.cpp index 84ec7ae44..4c815804b 100644 --- a/vndk/tools/header-checker/src/dumper/abi_wrappers.cpp +++ b/vndk/tools/header-checker/src/dumper/abi_wrappers.cpp @@ -882,8 +882,12 @@ bool EnumDeclWrapper::SetupEnumFields(repr::EnumTypeIR *enump) { clang::EnumDecl::enumerator_iterator enum_it = enum_decl_->enumerator_begin(); while (enum_it != enum_decl_->enumerator_end()) { std::string name = enum_it->getQualifiedNameAsString(); - uint64_t field_value = enum_it->getInitVal().getExtValue(); - enump->AddEnumField(repr::EnumFieldIR(name, field_value)); + const llvm::APSInt &value = enum_it->getInitVal(); + if (value.isUnsigned()) { + enump->AddEnumField(repr::EnumFieldIR(name, value.getZExtValue())); + } else { + enump->AddEnumField(repr::EnumFieldIR(name, value.getSExtValue())); + } enum_it++; } return true; 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 c447d0b7e..57c68c688 100644 --- a/vndk/tools/header-checker/src/repr/abi_diff_helpers.cpp +++ b/vndk/tools/header-checker/src/repr/abi_diff_helpers.cpp @@ -147,9 +147,10 @@ void AbiDiffHelper::CompareEnumFields( utils::FindCommonElements(old_fields_map, new_fields_map); std::vector enum_field_diffs; for (auto &&common_fields : cf) { - if (common_fields.first->GetValue() != common_fields.second->GetValue()) { + if (common_fields.first->GetSignedValue() != + common_fields.second->GetSignedValue()) { EnumFieldDiffIR enum_field_diff_ir(common_fields.first, - common_fields.second); + common_fields.second); enum_field_diffs.emplace_back(std::move(enum_field_diff_ir)); } } diff --git a/vndk/tools/header-checker/src/repr/ir_representation.h b/vndk/tools/header-checker/src/repr/ir_representation.h index 9e1eed4eb..ee186ae2b 100644 --- a/vndk/tools/header-checker/src/repr/ir_representation.h +++ b/vndk/tools/header-checker/src/repr/ir_representation.h @@ -447,20 +447,29 @@ class RecordTypeIR : public TypeIR, public TemplatedArtifactIR { class EnumFieldIR { public: - EnumFieldIR(const std::string &name, int value) - : name_(name), value_(value) {} + EnumFieldIR(const std::string &name, int64_t value) + : name_(name), signed_value_(value), is_signed_(true) {} + + EnumFieldIR(const std::string &name, uint64_t value) + : name_(name), unsigned_value_(value), is_signed_(false) {} const std::string &GetName() const { return name_; } - int GetValue() const { - return value_; - } + bool IsSigned() const { return is_signed_; } + + int64_t GetSignedValue() const { return signed_value_; } + + uint64_t GetUnsignedValue() const { return unsigned_value_; } protected: std::string name_; - int value_ = 0; + union { + int64_t signed_value_; + uint64_t unsigned_value_; + }; + bool is_signed_; }; class EnumTypeIR : public TypeIR { diff --git a/vndk/tools/header-checker/src/repr/json/ir_dumper.cpp b/vndk/tools/header-checker/src/repr/json/ir_dumper.cpp index c482808cb..187a1183c 100644 --- a/vndk/tools/header-checker/src/repr/json/ir_dumper.cpp +++ b/vndk/tools/header-checker/src/repr/json/ir_dumper.cpp @@ -203,7 +203,12 @@ static JsonObject ConvertEnumFieldIR(const EnumFieldIR *enum_field_ir) { JsonObject enum_field; enum_field.Set("name", enum_field_ir->GetName()); // Never omit enum values. - enum_field["enum_field_value"] = Json::Int64(enum_field_ir->GetValue()); + Json::Value &enum_field_value = enum_field["enum_field_value"]; + if (enum_field_ir->IsSigned()) { + enum_field_value = Json::Int64(enum_field_ir->GetSignedValue()); + } else { + enum_field_value = Json::UInt64(enum_field_ir->GetUnsignedValue()); + } return enum_field; } diff --git a/vndk/tools/header-checker/src/repr/json/ir_reader.cpp b/vndk/tools/header-checker/src/repr/json/ir_reader.cpp index addca8894..14a15acf9 100644 --- a/vndk/tools/header-checker/src/repr/json/ir_reader.cpp +++ b/vndk/tools/header-checker/src/repr/json/ir_reader.cpp @@ -76,11 +76,16 @@ bool JsonObjectRef::GetBool(const std::string &key) const { } int64_t JsonObjectRef::GetInt(const std::string &key) const { - return Get(key, json_0, &Json::Value::isIntegral).asInt64(); + return Get(key, json_0, &Json::Value::isInt64).asInt64(); } uint64_t JsonObjectRef::GetUint(const std::string &key) const { - return Get(key, json_0, &Json::Value::isIntegral).asUInt64(); + return Get(key, json_0, &Json::Value::isUInt64).asUInt64(); +} + +const Json::Value &JsonObjectRef::GetIntegralValue( + const std::string &key) const { + return Get(key, json_0, &Json::Value::isIntegral); } std::string JsonObjectRef::GetString(const std::string &key) const { @@ -248,9 +253,13 @@ void JsonIRReader::ReadVTableLayout(const JsonObjectRef &record_type, void JsonIRReader::ReadEnumFields(const JsonObjectRef &enum_type, EnumTypeIR *enum_ir) { for (auto &&field : enum_type.GetObjects("enum_fields")) { - EnumFieldIR enum_field_ir(field.GetString("name"), - field.GetInt("enum_field_value")); - enum_ir->AddEnumField(std::move(enum_field_ir)); + std::string name = field.GetString("name"); + const Json::Value &value = field.GetIntegralValue("enum_field_value"); + if (value.isUInt64()) { + enum_ir->AddEnumField(EnumFieldIR(name, value.asUInt64())); + } else { + enum_ir->AddEnumField(EnumFieldIR(name, value.asInt64())); + } } } diff --git a/vndk/tools/header-checker/src/repr/json/ir_reader.h b/vndk/tools/header-checker/src/repr/json/ir_reader.h index 049e070fa..c95337ce2 100644 --- a/vndk/tools/header-checker/src/repr/json/ir_reader.h +++ b/vndk/tools/header-checker/src/repr/json/ir_reader.h @@ -46,6 +46,9 @@ class JsonObjectRef { // Default to 0. uint64_t GetUint(const std::string &key) const; + // Default to 0. + const Json::Value &GetIntegralValue(const std::string &key) const; + // Default to "". std::string GetString(const std::string &key) const; diff --git a/vndk/tools/header-checker/src/repr/protobuf/converter.h b/vndk/tools/header-checker/src/repr/protobuf/converter.h index 84ffb8c65..585001eb0 100644 --- a/vndk/tools/header-checker/src/repr/protobuf/converter.h +++ b/vndk/tools/header-checker/src/repr/protobuf/converter.h @@ -356,7 +356,11 @@ inline bool SetIRToProtobufEnumField( return true; } enum_field_protobuf->set_name(enum_field_ir->GetName()); - enum_field_protobuf->set_enum_field_value(enum_field_ir->GetValue()); + // The "enum_field_value" in the .proto is a signed 64-bit integer. An + // unsigned integer >= (1 << 63) is represented with a negative integer in the + // dump file. Despite the wrong representation, the diff result isn't affected + // because every integer has a unique representation. + enum_field_protobuf->set_enum_field_value(enum_field_ir->GetSignedValue()); return true; } diff --git a/vndk/tools/header-checker/tests/integration/enum/include/base.h b/vndk/tools/header-checker/tests/integration/enum/include/base.h new file mode 100644 index 000000000..3ab28980b --- /dev/null +++ b/vndk/tools/header-checker/tests/integration/enum/include/base.h @@ -0,0 +1,23 @@ +#include + +enum Int8 : int8_t { + ZERO = 0, + MINUS_1 = (int8_t)-1, +}; + +enum Uint8 : uint8_t { + UNSIGNED_255 = UINT8_MAX, +}; + +enum Int64 : int64_t { + SIGNED_MAX = INT64_MAX, + SIGNED_MIN = INT64_MIN, +}; + +enum Uint64 : uint64_t { + UNSIGNED_MAX = UINT64_MAX, +}; + +extern "C" { +void function(Int8, Uint8, Int64, Uint64); +} diff --git a/vndk/tools/header-checker/tests/integration/enum/map.txt b/vndk/tools/header-checker/tests/integration/enum/map.txt new file mode 100644 index 000000000..d9bf6bf32 --- /dev/null +++ b/vndk/tools/header-checker/tests/integration/enum/map.txt @@ -0,0 +1,4 @@ +libenum { + global: + function; +}; diff --git a/vndk/tools/header-checker/tests/module.py b/vndk/tools/header-checker/tests/module.py index 565e97e23..255576138 100755 --- a/vndk/tools/header-checker/tests/module.py +++ b/vndk/tools/header-checker/tests/module.py @@ -789,6 +789,16 @@ TEST_MODULES = [ export_include_dirs=['integration/union/include'], linker_flags=['-output-format', 'Json'], ), + LsdumpModule( + name='libenum', + arch='arm64', + srcs=['integration/enum/include/base.h'], + version_script='integration/enum/map.txt', + export_include_dirs=['integration/enum/include'], + dumper_flags=['-output-format', 'Json'], + linker_flags=['-input-format', 'Json', '-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/libenum.so.lsdump b/vndk/tools/header-checker/tests/reference_dumps/arm64/libenum.so.lsdump new file mode 100644 index 000000000..7a12c7ea5 --- /dev/null +++ b/vndk/tools/header-checker/tests/reference_dumps/arm64/libenum.so.lsdump @@ -0,0 +1,167 @@ +{ + "array_types" : [], + "builtin_types" : + [ + { + "alignment" : 1, + "is_integral" : true, + "linker_set_key" : "_ZTIa", + "name" : "signed char", + "referenced_type" : "_ZTIa", + "self_type" : "_ZTIa", + "size" : 1 + }, + { + "alignment" : 1, + "is_integral" : true, + "is_unsigned" : true, + "linker_set_key" : "_ZTIh", + "name" : "unsigned char", + "referenced_type" : "_ZTIh", + "self_type" : "_ZTIh", + "size" : 1 + }, + { + "alignment" : 8, + "is_integral" : true, + "linker_set_key" : "_ZTIl", + "name" : "long", + "referenced_type" : "_ZTIl", + "self_type" : "_ZTIl", + "size" : 8 + }, + { + "alignment" : 8, + "is_integral" : true, + "is_unsigned" : true, + "linker_set_key" : "_ZTIm", + "name" : "unsigned long", + "referenced_type" : "_ZTIm", + "self_type" : "_ZTIm", + "size" : 8 + }, + { + "linker_set_key" : "_ZTIv", + "name" : "void", + "referenced_type" : "_ZTIv", + "self_type" : "_ZTIv" + } + ], + "elf_functions" : + [ + { + "name" : "function" + } + ], + "elf_objects" : [], + "enum_types" : + [ + { + "alignment" : 1, + "enum_fields" : + [ + { + "enum_field_value" : 0, + "name" : "ZERO" + }, + { + "enum_field_value" : -1, + "name" : "MINUS_1" + } + ], + "linker_set_key" : "_ZTI4Int8", + "name" : "Int8", + "referenced_type" : "_ZTI4Int8", + "self_type" : "_ZTI4Int8", + "size" : 1, + "source_file" : "development/vndk/tools/header-checker/tests/integration/enum/include/base.h", + "underlying_type" : "_ZTIa" + }, + { + "alignment" : 8, + "enum_fields" : + [ + { + "enum_field_value" : 9223372036854775807, + "name" : "SIGNED_MAX" + }, + { + "enum_field_value" : -9223372036854775808, + "name" : "SIGNED_MIN" + } + ], + "linker_set_key" : "_ZTI5Int64", + "name" : "Int64", + "referenced_type" : "_ZTI5Int64", + "self_type" : "_ZTI5Int64", + "size" : 8, + "source_file" : "development/vndk/tools/header-checker/tests/integration/enum/include/base.h", + "underlying_type" : "_ZTIl" + }, + { + "alignment" : 1, + "enum_fields" : + [ + { + "enum_field_value" : 255, + "name" : "UNSIGNED_255" + } + ], + "linker_set_key" : "_ZTI5Uint8", + "name" : "Uint8", + "referenced_type" : "_ZTI5Uint8", + "self_type" : "_ZTI5Uint8", + "size" : 1, + "source_file" : "development/vndk/tools/header-checker/tests/integration/enum/include/base.h", + "underlying_type" : "_ZTIh" + }, + { + "alignment" : 8, + "enum_fields" : + [ + { + "enum_field_value" : 18446744073709551615, + "name" : "UNSIGNED_MAX" + } + ], + "linker_set_key" : "_ZTI6Uint64", + "name" : "Uint64", + "referenced_type" : "_ZTI6Uint64", + "self_type" : "_ZTI6Uint64", + "size" : 8, + "source_file" : "development/vndk/tools/header-checker/tests/integration/enum/include/base.h", + "underlying_type" : "_ZTIm" + } + ], + "function_types" : [], + "functions" : + [ + { + "function_name" : "function", + "linker_set_key" : "function", + "parameters" : + [ + { + "referenced_type" : "_ZTI4Int8" + }, + { + "referenced_type" : "_ZTI5Uint8" + }, + { + "referenced_type" : "_ZTI5Int64" + }, + { + "referenced_type" : "_ZTI6Uint64" + } + ], + "return_type" : "_ZTIv", + "source_file" : "development/vndk/tools/header-checker/tests/integration/enum/include/base.h" + } + ], + "global_vars" : [], + "lvalue_reference_types" : [], + "pointer_types" : [], + "qualified_types" : [], + "record_types" : [], + "rvalue_reference_types" : [] +} diff --git a/vndk/tools/header-checker/tests/test.py b/vndk/tools/header-checker/tests/test.py index 4217d62dc..c99c08d29 100755 --- a/vndk/tools/header-checker/tests/test.py +++ b/vndk/tools/header-checker/tests/test.py @@ -485,6 +485,9 @@ class HeaderCheckerTest(unittest.TestCase): self.assertNotIn("fields_added", diff) self.assertNotIn("fields_removed", diff) + def test_enum_diff(self): + self.prepare_and_absolute_diff_all_archs("libenum", "libenum") + if __name__ == '__main__': unittest.main()