From bea7f553a09b0e4590e5c36e7f2ef02a1302a5d7 Mon Sep 17 00:00:00 2001 From: "Wei Zhang (Devinfra)" Date: Fri, 15 Mar 2024 21:08:07 -0700 Subject: [PATCH] Update synthetic fields check for Kotlin EnumEntries Summary: - Restore the disabled Kotlin enum test w/ `EnumEntries` disabled - Add `support_kt_19_enum_entries` option to enable a new safe enum check logic path to accommodate the new structure of Kotlin 1.9 enums Reviewed By: beicy Differential Revision: D53681655 fbshipit-source-id: 0bc947904c876e17dac6fcf66b2e2cd89b5a6f0d --- opt/optimize_enums/EnumUpcastAnalysis.h | 4 ++ opt/optimize_enums/OptimizeEnums.cpp | 69 ++++++++++++++++++-- opt/optimize_enums/OptimizeEnums.h | 1 + opt/optimize_enums/OptimizeEnumsUnsafeType.h | 4 ++ test/instr/EnumTransformTest.config | 3 +- test/instr/EnumTransformTest.kt | 2 +- 6 files changed, 74 insertions(+), 9 deletions(-) diff --git a/opt/optimize_enums/EnumUpcastAnalysis.h b/opt/optimize_enums/EnumUpcastAnalysis.h index e6cdd49104d..4285b959695 100644 --- a/opt/optimize_enums/EnumUpcastAnalysis.h +++ b/opt/optimize_enums/EnumUpcastAnalysis.h @@ -31,6 +31,10 @@ bool is_enum_valueof(const DexMethodRef* method); */ bool is_enum_values(const DexMethodRef* method); +constexpr const char* KT_ENUM_ENTRIES_TYPE = "Lkotlin/enums/EnumEntries;"; +constexpr const char* ENUM_VALUES_FIELD = "$VALUES"; +constexpr const char* ENUM_ENTRIES_FIELD = "$ENTRIES"; + // Store possible types for a register although we only care about Object, Enum // and Enum's subtypes. using EnumTypes = sparta::PatriciaTreeSetAbstractDomain; diff --git a/opt/optimize_enums/OptimizeEnums.cpp b/opt/optimize_enums/OptimizeEnums.cpp index 1dd54ea570c..917d058cef3 100644 --- a/opt/optimize_enums/OptimizeEnums.cpp +++ b/opt/optimize_enums/OptimizeEnums.cpp @@ -375,6 +375,7 @@ class OptimizeEnums { PassManager& mgr, int max_enum_size, bool skip_sanity_check, + const bool support_kt_19_enum_entries, const std::vector& allowlist, ConfigFiles& conf, std::unordered_map& unsafe_counts) { @@ -396,8 +397,8 @@ class OptimizeEnums { * fields, and virtual methods are safe. */ - auto is_safe_enum = [this, &base_enum_check](const DexClass* cls, - UnsafeTypes& utypes) { + auto is_safe_enum = [this, &base_enum_check, support_kt_19_enum_entries]( + const DexClass* cls, UnsafeTypes& utypes) { if (!base_enum_check(cls)) { return false; } @@ -411,8 +412,14 @@ class OptimizeEnums { if (!cls->get_interfaces()->empty()) { utypes.insert(UnsafeType::kHasInterfaces); } - if (!only_one_static_synth_field(cls)) { - utypes.insert(UnsafeType::kMoreThanOneSynthField); + if (support_kt_19_enum_entries) { + if (!only_has_expected_static_synth_fields(cls)) { + utypes.insert(UnsafeType::kUnexpectedSynthFields); + } + } else { + if (!only_one_static_synth_field(cls)) { + utypes.insert(UnsafeType::kMoreThanOneSynthField); + } } const auto& ctors = cls->get_ctors(); @@ -584,6 +591,52 @@ class OptimizeEnums { return true; } + bool only_has_expected_static_synth_fields(const DexClass* cls) { + const auto synth_access = optimize_enums::synth_access(); + const auto enum_entries_type = + DexType::get_type(optimize_enums::KT_ENUM_ENTRIES_TYPE); + always_assert(enum_entries_type != nullptr); + std::set expected_fields = {ENUM_VALUES_FIELD, + ENUM_ENTRIES_FIELD}; + for (auto field : cls->get_sfields()) { + if (!check_required_access_flags(synth_access, field->get_access())) { + continue; + } + auto fname = field->get_name()->str_copy(); + if (expected_fields.count(fname)) { + expected_fields.erase(fname); + if (fname == ENUM_ENTRIES_FIELD && + field->get_type() != enum_entries_type) { + TRACE(ENUM, 2, "Unexpected synthetic field %s", SHOW(field)); + return false; + } + } else { + TRACE(ENUM, 2, "Unexpected synthetic field %s", SHOW(field)); + return false; + } + } + if (expected_fields.empty()) { + return true; + } + if (expected_fields.size() == 1 && + *expected_fields.begin() == ENUM_ENTRIES_FIELD) { + return true; + } + + if (traceEnabled(ENUM, 2)) { + std::ostringstream s; + for (const auto& field : cls->get_sfields()) { + if (!check_required_access_flags(synth_access, field->get_access())) { + continue; + } + s << field->get_name()->str_copy() << ", "; + } + TRACE(ENUM, 2, "Unexpected synthetic fields found on %s %s", SHOW(cls), + s.str().c_str()); + } + return false; + } + /** * Returns true if the constructor invokes `Enum.`, sets its instance * fields, and then returns. We want to make sure there are no side affects. @@ -867,6 +920,8 @@ void OptimizeEnumsPass::bind_config() { "enum fields, try to erase them without considering reference equality " "of the enum objects. Do not add enums to the allowlist!"); bind("skip_sanity_check", false, m_skip_sanity_check, "May skip some check."); + bind("support_kt_19_enum_entries", false, m_support_kt_19_enum_entries, + "Try to optimize Kotlin 1.9 Enums with EnumEntries feature."); } void OptimizeEnumsPass::run_pass(DexStoresVector& stores, @@ -875,9 +930,9 @@ void OptimizeEnumsPass::run_pass(DexStoresVector& stores, OptimizeEnums opt_enums(stores, conf); opt_enums.remove_redundant_generated_classes(); std::unordered_map unsafe_counts; - opt_enums.replace_enum_with_int(mgr, m_max_enum_size, m_skip_sanity_check, - m_enum_to_integer_allowlist, conf, - unsafe_counts); + opt_enums.replace_enum_with_int( + mgr, m_max_enum_size, m_skip_sanity_check, m_support_kt_19_enum_entries, + m_enum_to_integer_allowlist, conf, unsafe_counts); opt_enums.remove_enum_generated_methods(); opt_enums.stats(mgr); for (auto& p : unsafe_counts) { diff --git a/opt/optimize_enums/OptimizeEnums.h b/opt/optimize_enums/OptimizeEnums.h index 53bfc70ffaf..2c9d81bc09f 100644 --- a/opt/optimize_enums/OptimizeEnums.h +++ b/opt/optimize_enums/OptimizeEnums.h @@ -31,6 +31,7 @@ class OptimizeEnumsPass : public Pass { private: int m_max_enum_size; bool m_skip_sanity_check; + bool m_support_kt_19_enum_entries; std::vector m_enum_to_integer_allowlist; }; diff --git a/opt/optimize_enums/OptimizeEnumsUnsafeType.h b/opt/optimize_enums/OptimizeEnumsUnsafeType.h index 1b878a991d1..5018c53f936 100644 --- a/opt/optimize_enums/OptimizeEnumsUnsafeType.h +++ b/opt/optimize_enums/OptimizeEnumsUnsafeType.h @@ -19,6 +19,7 @@ enum class UnsafeType { kCannotDelete, kHasInterfaces, kMoreThanOneSynthField, + kUnexpectedSynthFields, kMultipleCtors, kComplexCtor, kUnrenamableDmethod, @@ -57,6 +58,9 @@ inline std::ostream& operator<<(std::ostream& os, const UnsafeType& u) { case UnsafeType::kMoreThanOneSynthField: os << "MoreThanOneSynthField"; break; + case UnsafeType::kUnexpectedSynthFields: + os << "UnexpectedSynthFields"; + break; case UnsafeType::kMultipleCtors: os << "MultipleCtors"; break; diff --git a/test/instr/EnumTransformTest.config b/test/instr/EnumTransformTest.config index 6a6db111533..d9f3a7366e7 100644 --- a/test/instr/EnumTransformTest.config +++ b/test/instr/EnumTransformTest.config @@ -7,7 +7,8 @@ }, "OptimizeEnumsPass" : { "max_enum_size" : 4, - "break_reference_equality_allowlist" : ["Lredex/PURE_SCORE;"] + "break_reference_equality_allowlist" : ["Lredex/PURE_SCORE;"], + "support_kt_19_enum_entries" : true }, "ir_type_checker": { "run_after_passes" : [ diff --git a/test/instr/EnumTransformTest.kt b/test/instr/EnumTransformTest.kt index 41eedd07499..e05fe693101 100644 --- a/test/instr/EnumTransformTest.kt +++ b/test/instr/EnumTransformTest.kt @@ -18,7 +18,7 @@ import org.junit.Test // POSTCHECK: (PUBLIC, STATIC, FINAL) f0:java.lang.Integer // POSTCHECK: (PUBLIC, STATIC, FINAL) f1:java.lang.Integer // POSTCHECK: (PUBLIC, STATIC, FINAL) f2:java.lang.Integer -// POSTCHECK: (PUBLIC, STATIC, FINAL) f3:java.lang.Integer +// POSTCHECK-NOT: (PUBLIC, STATIC, FINAL) f3:java.lang.Integer // POSTCHECK-NOT: (PUBLIC, STATIC, FINAL) f4:java.lang.Integer // Simple case is optimized.