Support EnumEntries in EnumClinitAnalysis and EnumTransformer

Summary: - Make EnumClinitAnalysis recognize the new synth field - Make the transformation compatible with the new synth field and removes it from the clinit code - Enable `+EnumEntries` in the instr test, and update the test accordingly Reviewed By: NTillmann Differential Revision: D53681654 fbshipit-source-id: 328eee64d26936afa0d2692dd44feafa6a2e321c
kami-lang · Mar 22, 2024 · 8674ec5 · 8674ec5
1 parent 02e0027
commit 8674ec5
Show file tree

Hide file tree

Showing 6 changed files with 68 additions and 19 deletions.
diff --git a/opt/optimize_enums/EnumClinitAnalysis.cpp b/opt/optimize_enums/EnumClinitAnalysis.cpp
@@ -190,13 +190,15 @@ class EnumOrdinalAnalyzer
  * type are in the result map.
  */
 bool validate_result(const DexClass* cls,
-                     const optimize_enums::EnumConstantsMap& constants) {
+                     const optimize_enums::EnumConstantsMap& constants,
+                     const bool support_kt_19_enum_entries) {
   if (constants.empty()) {
     TRACE(ENUM, 2, "\tEmpty result for %s", SHOW(cls));
     return false;
   }
   std::vector<bool> ordinals(constants.size(), false);
   bool synth_values_field = false;
+  size_t synth_values_field_cnt = 0;
 
   auto enum_field_access = optimize_enums::enum_field_access();
   auto values_access = optimize_enums::synth_access();
@@ -224,15 +226,21 @@ bool validate_result(const DexClass* cls,
         return false;
       }
       if (check_required_access_flags(values_access, access)) {
-        if (!synth_values_field) {
-          synth_values_field = true;
+        if (synth_values_field_cnt < 2) {
+          synth_values_field_cnt++;
         } else {
-          TRACE(ENUM, 2, "\tMultiple static synthetic fields on %s", SHOW(cls));
+          // $VALUES and $ENTRIES
+          TRACE(ENUM, 2, "\tMore static synthetic fields than expected on %s",
+                SHOW(cls));
           return false;
         }
       }
     }
   }
+  if (synth_values_field_cnt == 2) {
+    always_assert_log(support_kt_19_enum_entries,
+                      "Only Kotlin 1.9 enums have two synth fields.");
+  }
   if (std::all_of(ordinals.begin(), ordinals.end(),
                   [](bool item) { return item; })) {
     return true;
@@ -249,7 +257,8 @@ DexAccessFlags enum_field_access() { return ACC_STATIC | ACC_ENUM; }
 
 DexAccessFlags synth_access() { return ACC_STATIC | ACC_FINAL | ACC_SYNTHETIC; }
 
-EnumAttributes analyze_enum_clinit(const DexClass* cls) {
+EnumAttributes analyze_enum_clinit(const DexClass* cls,
+                                   const bool support_kt_19_enum_entries) {
   always_assert(is_enum(cls));
 
   auto* code = cls->get_clinit()->get_code();
@@ -357,7 +366,8 @@ EnumAttributes analyze_enum_clinit(const DexClass* cls) {
       return EnumAttributes();
     }
   }
-  if (!validate_result(cls, attributes.m_constants_map)) {
+  if (!validate_result(cls, attributes.m_constants_map,
+                       support_kt_19_enum_entries)) {
     return EnumAttributes();
   }
   return attributes;

diff --git a/opt/optimize_enums/EnumClinitAnalysis.h b/opt/optimize_enums/EnumClinitAnalysis.h
@@ -63,6 +63,7 @@ struct EnumAttributes {
  * Returns an EnumInstanceFieldMap and an EnumConstantsMap if success,
  * otherwise, return empty maps.
  */
-EnumAttributes analyze_enum_clinit(const DexClass* cls);
+EnumAttributes analyze_enum_clinit(const DexClass* cls,
+                                   const bool support_kt_19_enum_entries);
 
 } // namespace optimize_enums
diff --git a/opt/optimize_enums/EnumTransformer.cpp b/opt/optimize_enums/EnumTransformer.cpp
@@ -142,6 +142,9 @@ struct EnumUtil {
       "Ljava/lang/IllegalArgumentException;.<init>:(Ljava/lang/String;)V");
   DexMethodRef* STRING_EQ_METHOD =
       DexMethod::make_method("Ljava/lang/String;.equals:(Ljava/lang/Object;)Z");
+  DexMethodRef* KT_ENUM_ENTRIES_FACTORY_METHOD = DexMethod::make_method(
+      "Lkotlin/enums/EnumEntriesKt;.enumEntries:([Ljava/lang/Enum;)Lkotlin/"
+      "enums/EnumEntries;");
 
   InsertOnlyConcurrentMap<DexClass*, DexMethod*>
       m_user_defined_tostring_method_cache;
@@ -1076,7 +1079,8 @@ class EnumTransformer final {
          it != config.candidate_enums.end();
          ++it) {
       auto enum_cls = type_class(*it);
-      auto attributes = optimize_enums::analyze_enum_clinit(enum_cls);
+      auto attributes = optimize_enums::analyze_enum_clinit(
+          enum_cls, config.support_kt_19_enum_entries);
       size_t num_enum_constants = attributes.m_constants_map.size();
       if (num_enum_constants == 0) {
         TRACE(ENUM, 2, "\tCannot analyze enum %s : ord %zu sfields %zu",
@@ -1470,27 +1474,32 @@ class EnumTransformer final {
     auto& enum_constants =
         m_enum_attributes_map[enum_cls->get_type()].m_constants_map;
     auto synth_field_access = synth_access();
-    DexField* values_field = nullptr;
+    std::unordered_set<DexField*> synth_fields;
 
     std20::erase_if(sfields, [&](auto* field) {
       if (enum_constants.count(field)) {
         return true;
       }
       if (check_required_access_flags(synth_field_access,
                                       field->get_access())) {
-        always_assert(!values_field);
-        values_field = field;
+        always_assert(synth_fields.size() < 2);
+        TRACE(ENUM, 5, "erasing field %s", SHOW(field));
+        synth_fields.insert(field);
+        if (synth_fields.size() == 2) {
+          always_assert(m_enum_util->m_config.support_kt_19_enum_entries);
+        }
         return true;
       }
       return false;
     });
 
-    always_assert(values_field);
+    always_assert(!synth_fields.empty());
     auto& dmethods = enum_cls->get_dmethods();
     // Delete <init>, values() and valueOf(String) methods, and clean <clinit>.
     std20::erase_if(dmethods, [&, this](auto* method) {
       if (method::is_clinit(method)) {
-        clean_clinit(enum_constants, enum_cls, method, values_field);
+        clean_clinit(enum_constants, m_enum_util->m_config, enum_cls, method,
+                     synth_fields, m_enum_util->KT_ENUM_ENTRIES_FACTORY_METHOD);
         return empty(method->get_code());
       }
       return this->is_generated_enum_method(method);
@@ -1522,9 +1531,11 @@ class EnumTransformer final {
    * ... // register v0 may be used.
    */
   static void clean_clinit(const EnumConstantsMap& enum_constants,
+                           const Config& config,
                            DexClass* enum_cls,
                            DexMethod* clinit,
-                           DexField* values_field) {
+                           const std::unordered_set<DexField*>& synth_fields,
+                           DexMethodRef* kt_enum_entries_factory) {
     auto code = clinit->get_code();
     auto ctors = enum_cls->get_ctors();
     always_assert(ctors.size() == 1);
@@ -1543,12 +1554,16 @@ class EnumTransformer final {
               it,
               dasm(IOPCODE_MOVE_RESULT_PSEUDO_OBJECT, {{VREG, insn->src(0)}}));
           m.remove(it);
-        } else if (field == values_field) {
+        } else if (synth_fields.count(field)) {
           m.remove(it);
         }
       } else if (opcode::is_invoke_direct(insn->opcode()) &&
                  insn->get_method() == ctor) {
         summaries.emplace(insn, side_effects::Summary());
+      } else if (opcode::is_invoke_static(insn->opcode()) &&
+                 insn->get_method() == kt_enum_entries_factory) {
+        summaries.emplace(insn, side_effects::Summary());
+        always_assert(config.support_kt_19_enum_entries);
       }
     }
     m.flush();
@@ -1566,7 +1581,8 @@ class EnumTransformer final {
     // Assert no instruction about the $VALUES field.
     for (auto& mie : cfg::InstructionIterable(cfg)) {
       auto insn = mie.insn;
-      always_assert_log(!insn->has_field() || insn->get_field() != values_field,
+      always_assert_log(!insn->has_field() ||
+                            !synth_fields.count(insn->get_field()->as_def()),
                         "%s can not be deleted", SHOW(insn));
     }
   }

diff --git 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-NOT: (PUBLIC, STATIC, FINAL) f3:java.lang.Integer
+// POSTCHECK: (PUBLIC, STATIC, FINAL) f3:java.lang.Integer
 // POSTCHECK-NOT: (PUBLIC, STATIC, FINAL) f4:java.lang.Integer
 
 // Simple case is optimized.

diff --git a/test/instr/KotlinEnumTransformTest.config b/test/instr/KotlinEnumTransformTest.config
@@ -0,0 +1,20 @@
+{
+  "redex" : {
+    "passes" : [
+      "RemoveUnreachablePass",
+      "OptimizeEnumsPass",
+      "RegAllocPass"
+    ]
+  },
+  "OptimizeEnumsPass" : {
+    "max_enum_size" : 4,
+    "break_reference_equality_allowlist" : ["Lredex/PURE_SCORE;"],
+    "support_kt_19_enum_entries" : true
+  },
+  "ir_type_checker": {
+    "run_after_passes" : [
+      "OptimizeEnumsPass"
+    ],
+    "verify_moves" : true
+  }
+}
diff --git a/test/integ/EnumClinitAnalysisTest.cpp b/test/integ/EnumClinitAnalysisTest.cpp
@@ -42,7 +42,8 @@ TEST_F(EnumClinitAnalysisTest, OrdinalAnalysis) {
   for (auto* m : enum_cls->get_dmethods()) {
     m->get_code()->build_cfg();
   }
-  auto attributes = analyze_enum_clinit(enum_cls);
+  auto attributes =
+      analyze_enum_clinit(enum_cls, /*support_kt_19_enum_entries*/ false);
   auto& enum_constants = attributes.m_constants_map;
   auto& ifield_map = attributes.m_field_map;
 
@@ -84,7 +85,8 @@ TEST_F(EnumClinitAnalysisTest, OrdinalAnalysis) {
     for (auto* m : enum_cls->get_dmethods()) {
       m->get_code()->build_cfg();
     }
-    attributes = analyze_enum_clinit(enum_cls);
+    attributes =
+        analyze_enum_clinit(enum_cls, /*support_kt_19_enum_entries*/ false);
     EXPECT_TRUE(attributes.m_constants_map.empty());
     EXPECT_TRUE(attributes.m_field_map.empty());
   }