only retain overriding vmethods of instantiable types

Summary: Other passes (DelInit, RemoveUninstantiables) already do something similar. This brings it to RMU (with the long-term goal of making some or all of those other passes obsolete, and have RMU do it all.)

Reviewed By: jimmycFB

Differential Revision: D45468656

fbshipit-source-id: 33fb5423c12663f56eb39379b36f15f6077ef800

libredex/Reachability.cpp

@@ -194,6 +194,11 @@ void RootSetMarker::push_seed(const DexMethod* method) {
   m_cond_marked->methods.insert(method);
 }
 
+void RootSetMarker::push_seed_if_instantiable(const DexMethod* method) {
+  if (!method) return;
+  m_cond_marked->methods_if_instantiable.insert(method);
+}
+
 template <class Seed>
 void RootSetMarker::record_is_seed(Seed* seed) {
   if (m_record_reachability) {
@@ -221,7 +226,7 @@ void RootSetMarker::mark_external_method_overriders() {
       if (!overriding->is_external()) {
         TRACE(REACH, 3, "Visiting seed: %s (implements %s)", SHOW(overriding),
               SHOW(method));
-        push_seed(overriding);
+        push_seed_if_instantiable(overriding);
       }
     }
   }
@@ -317,18 +322,21 @@ void TransitiveClosureMarker::push(const DexMethodRef* parent,
   this->template push<DexMethodRef>(parent, method);
 }
 
-void TransitiveClosureMarker::push_cond(const DexMethod* method) {
+void TransitiveClosureMarker::push_if_instantiable(const DexMethod* method) {
   if (!method || m_reachable_objects->marked(method)) return;
-  TRACE(REACH, 4, "Conditionally marking method: %s", SHOW(method));
+  TRACE(REACH, 4,
+        "Conditionally marking method if declaring class is instantiable: %s",
+        SHOW(method));
   auto clazz = type_class(method->get_class());
-  m_cond_marked->methods.insert(method);
-  // If :clazz has been marked, we cannot count on visit(DexClass*) to move
-  // the conditionally-marked methods into the actually-marked ones -- we have
-  // to do it ourselves. Note that we must do this check after adding :method
-  // to m_cond_marked to avoid a race condition where we add to m_cond_marked
-  // after visit(DexClass*) has finished moving its contents over to
-  // m_reachable_objects.
-  if (m_reachable_objects->marked(clazz)) {
+  m_cond_marked->methods_if_instantiable.insert(method);
+  // If :clazz is already known to be instantiable, then we cannot count on
+  // instantiable(DexClass*) to have moved the
+  // conditionally-if-instantiable-marked methods into the actually-marked ones
+  // -- we have to do it ourselves. Note that we must do this check after adding
+  // :method to m_cond_marked to avoid a race condition where we add to
+  // m_cond_marked after instantiable(DexClass*) has finished moving its
+  // contents over to m_reachable_objects.
+  if (m_instantiable_types->count(clazz)) {
     push(clazz, method);
   }
 }
@@ -397,7 +405,7 @@ void TransitiveClosureMarker::gather_and_push(DexMethod* meth) {
   push(meth, refs.fields.begin(), refs.fields.end());
   push(meth, refs.methods.begin(), refs.methods.end());
   for (auto* cond_meth : refs.cond_methods) {
-    push_cond(cond_meth);
+    push_if_instantiable(cond_meth);
   }
 }
 
@@ -425,6 +433,9 @@ void TransitiveClosureMarker::push_typelike_strings(
 
 void TransitiveClosureMarker::visit_cls(const DexClass* cls) {
   TRACE(REACH, 4, "Visiting class: %s", SHOW(cls));
+  if (is_native(cls)) {
+    instantiable(cls->get_type());
+  }
   for (auto& m : cls->get_dmethods()) {
     if (method::is_clinit(m)) {
       if (!m->get_code() || !method::is_trivial_clinit(*m->get_code())) {
@@ -458,6 +469,7 @@ void TransitiveClosureMarker::visit_cls(const DexClass* cls) {
       gather_and_push(anno.get());
     }
   }
+
   for (auto const& m : cls->get_ifields()) {
     if (m_cond_marked->fields.count(m)) {
       push(cls, m);
@@ -521,10 +533,29 @@ DexMethod* TransitiveClosureMarker::resolve_without_context(
   return nullptr;
 }
 
+void TransitiveClosureMarker::instantiable(DexType* type) {
+  auto cls = type_class(type);
+  if (!cls || cls->is_external()) {
+    return;
+  }
+  if (!m_instantiable_types->insert(cls)) {
+    return;
+  }
+  instantiable(cls->get_super_class());
+  for (auto* intf : *cls->get_interfaces()) {
+    instantiable(intf);
+  }
+  for (auto const& m : cls->get_vmethods()) {
+    if (m_cond_marked->methods_if_instantiable.count(m)) {
+      push(cls, m);
+    }
+  }
+}
+
 void TransitiveClosureMarker::visit_method_ref(const DexMethodRef* method) {
   TRACE(REACH, 4, "Visiting method: %s", SHOW(method));
-  auto resolved_method =
-      resolve_without_context(method, type_class(method->get_class()));
+  auto cls = type_class(method->get_class());
+  auto resolved_method = resolve_without_context(method, cls);
   if (resolved_method != nullptr) {
     TRACE(REACH, 5, "    Resolved to: %s", SHOW(resolved_method));
     push(method, resolved_method);
@@ -535,6 +566,9 @@ void TransitiveClosureMarker::visit_method_ref(const DexMethodRef* method) {
   for (auto const& t : *method->get_proto()->get_args()) {
     push(method, t);
   }
+  if (cls && !is_abstract(cls) && method::is_init(method)) {
+    instantiable(method->get_class());
+  }
   auto m = method->as_def();
   if (!m) {
     return;
@@ -545,7 +579,7 @@ void TransitiveClosureMarker::visit_method_ref(const DexMethodRef* method) {
     const auto& overriding_methods =
         mog::get_overriding_methods(m_method_override_graph, m);
     for (auto* overriding : overriding_methods) {
-      push_cond(overriding);
+      push_if_instantiable(overriding);
     }
   }
 }
@@ -574,10 +608,8 @@ std::unique_ptr<ReachableObjects> compute_reachable_objects(
   auto method_override_graph = mog::build_graph(scope);
 
   ConcurrentSet<ReachableObject, ReachableObjectHash> root_set;
-  RootSetMarker root_set_marker(*method_override_graph,
-                                record_reachability,
-                                &cond_marked,
-                                reachable_objects.get(),
+  RootSetMarker root_set_marker(*method_override_graph, record_reachability,
+                                &cond_marked, reachable_objects.get(),
                                 &root_set);
 
   if (should_mark_all_as_seed) {
@@ -586,14 +618,15 @@ std::unique_ptr<ReachableObjects> compute_reachable_objects(
     root_set_marker.mark(scope);
   }
 
+  InstantiableTypes instantiable_types;
   size_t num_threads = redex_parallel::default_num_threads();
   auto stats_arr = std::make_unique<Stats[]>(num_threads);
   workqueue_run<ReachableObject>(
       [&](MarkWorkerState* worker_state, const ReachableObject& obj) {
         TransitiveClosureMarker transitive_closure_marker(
             ignore_sets, *method_override_graph, record_reachability,
-            &cond_marked, reachable_objects.get(), worker_state,
-            &stats_arr[worker_state->worker_id()],
+            &cond_marked, reachable_objects.get(), &instantiable_types,
+            worker_state, &stats_arr[worker_state->worker_id()],
             remove_no_argument_constructors);
         transitive_closure_marker.visit(obj);
         return nullptr;

libredex/Reachability.h

@@ -171,8 +171,11 @@ class ReachableObjects {
 struct ConditionallyMarked {
   ConcurrentSet<const DexField*> fields;
   ConcurrentSet<const DexMethod*> methods;
+  ConcurrentSet<const DexMethod*> methods_if_instantiable;
 };
 
+using InstantiableTypes = ConcurrentSet<DexClass*>;
+
 struct References {
   std::vector<const DexString*> strings;
   std::vector<DexType*> types;
@@ -247,6 +250,8 @@ class RootSetMarker {
 
   void push_seed(const DexMethod* method);
 
+  void push_seed_if_instantiable(const DexMethod* method);
+
   template <class Seed>
   void record_is_seed(Seed* seed);
 
@@ -270,6 +275,7 @@ class TransitiveClosureMarker {
       bool record_reachability,
       ConditionallyMarked* cond_marked,
       ReachableObjects* reachable_objects,
+      InstantiableTypes* instantiable_types,
       MarkWorkerState* worker_state,
       Stats* stats,
       bool remove_no_argument_constructors = false)
@@ -278,6 +284,7 @@ class TransitiveClosureMarker {
         m_record_reachability(record_reachability),
         m_cond_marked(cond_marked),
         m_reachable_objects(reachable_objects),
+        m_instantiable_types(instantiable_types),
         m_worker_state(worker_state),
         m_stats(stats),
         m_remove_no_argument_constructors(remove_no_argument_constructors) {
@@ -327,7 +334,7 @@ class TransitiveClosureMarker {
 
   void push(const DexMethodRef* parent, const DexMethodRef* method);
 
-  void push_cond(const DexMethod* method);
+  void push_if_instantiable(const DexMethod* method);
 
   bool has_class_forname(DexMethod* meth);
 
@@ -350,11 +357,14 @@ class TransitiveClosureMarker {
   static DexMethod* resolve_without_context(const DexMethodRef* method,
                                             const DexClass* cls);
 
+  void instantiable(DexType* type);
+
   const IgnoreSets& m_ignore_sets;
   const method_override_graph::Graph& m_method_override_graph;
   bool m_record_reachability;
   ConditionallyMarked* m_cond_marked;
   ReachableObjects* m_reachable_objects;
+  InstantiableTypes* m_instantiable_types;
   MarkWorkerState* m_worker_state;
   Stats* m_stats;
   bool m_remove_no_argument_constructors;

opt/reachable-natives/ReachableNatives.cpp

@@ -70,6 +70,7 @@ void ReachableNativesPass::run_pass(DexStoresVector& stores,
 
   auto scope = build_class_scope(stores);
   auto reachable_objects = std::make_unique<reachability::ReachableObjects>();
+  reachability::InstantiableTypes instantiable_types;
   reachability::ConditionallyMarked cond_marked;
   auto method_override_graph = method_override_graph::build_graph(scope);
 
@@ -99,7 +100,7 @@ void ReachableNativesPass::run_pass(DexStoresVector& stores,
           const reachability::ReachableObject& obj) {
         reachability::TransitiveClosureMarker transitive_closure_marker(
             ignore_sets, *method_override_graph, false, &cond_marked,
-            reachable_objects.get(), worker_state,
+            reachable_objects.get(), &instantiable_types, worker_state,
             &stats_arr[worker_state->worker_id()], false);
         transitive_closure_marker.visit(obj);
         return nullptr;

opt/remove-unreachable/TypeAnalysisAwareRemoveUnreachable.cpp

@@ -46,6 +46,7 @@ class TypeAnaysisAwareClosureMarker final
       bool record_reachability,
       ConditionallyMarked* cond_marked,
       ReachableObjects* reachable_objects,
+      InstantiableTypes* instantiable_types,
       MarkWorkerState* worker_state,
       Stats* stats,
       std::shared_ptr<type_analyzer::global::GlobalTypeAnalyzer> gta)
@@ -54,6 +55,7 @@ class TypeAnaysisAwareClosureMarker final
                                               record_reachability,
                                               cond_marked,
                                               reachable_objects,
+                                              instantiable_types,
                                               worker_state,
                                               stats),
         m_gta(std::move(gta)) {}
@@ -72,8 +74,8 @@ class TypeAnaysisAwareClosureMarker final
 
   void visit_method_ref(const DexMethodRef* method) override {
     TRACE(REACH, 4, "Visiting method: %s", SHOW(method));
-    auto resolved_method =
-        resolve_without_context(method, type_class(method->get_class()));
+    auto cls = type_class(method->get_class());
+    auto resolved_method = resolve_without_context(method, cls);
     if (resolved_method != nullptr) {
       TRACE(REACH, 5, "    Resolved to: %s", SHOW(resolved_method));
       this->push(method, resolved_method);
@@ -84,6 +86,9 @@ class TypeAnaysisAwareClosureMarker final
     for (auto const& t : *method->get_proto()->get_args()) {
       push(method, t);
     }
+    if (cls && !is_abstract(cls) && method::is_init(method)) {
+      instantiable(method->get_class());
+    }
 
     auto m = method->as_def();
     if (!m || !root(m) || m->is_external()) {
@@ -99,7 +104,7 @@ class TypeAnaysisAwareClosureMarker final
               overriding_methods.size(), SHOW(m));
       }
       for (auto* overriding : overriding_methods) {
-        push_cond(overriding);
+        push_if_instantiable(overriding);
         TRACE(REACH, 3, "marking root override: %s", SHOW(overriding));
       }
     }
@@ -235,14 +240,13 @@ std::unique_ptr<ReachableObjects> compute_reachable_objects_with_type_anaysis(
   });
 
   auto reachable_objects = std::make_unique<ReachableObjects>();
+  InstantiableTypes instantiable_types;
   ConditionallyMarked cond_marked;
   auto method_override_graph = mog::build_graph(scope);
 
   ConcurrentSet<ReachableObject, ReachableObjectHash> root_set;
-  RootSetMarker root_set_marker(*method_override_graph,
-                                record_reachability,
-                                &cond_marked,
-                                reachable_objects.get(),
+  RootSetMarker root_set_marker(*method_override_graph, record_reachability,
+                                &cond_marked, reachable_objects.get(),
                                 &root_set);
   root_set_marker.mark(scope);
 
@@ -252,8 +256,8 @@ std::unique_ptr<ReachableObjects> compute_reachable_objects_with_type_anaysis(
       [&](MarkWorkerState* worker_state, const ReachableObject& obj) {
         TypeAnaysisAwareClosureMarker transitive_closure_marker(
             ignore_sets, *method_override_graph, record_reachability,
-            &cond_marked, reachable_objects.get(), worker_state,
-            &stats_arr[worker_state->worker_id()], gta);
+            &cond_marked, reachable_objects.get(), &instantiable_types,
+            worker_state, &stats_arr[worker_state->worker_id()], gta);
         transitive_closure_marker.visit(obj);
         return nullptr;
       },

test/common/RedexTest.h

@@ -143,6 +143,19 @@ struct RedexIntegrationTest : public RedexTest {
     return it == ifields.end() ? nullptr : *it;
   }
 
+  template <typename C>
+  DexMethod* find_dmethod(const C& clazzes,
+                          const char* cls,
+                          const char* rtype,
+                          const char* name,
+                          const std::vector<const char*>& args) {
+    const auto* c = find_class(clazzes, cls);
+    const auto& dmethods = c->get_dmethods();
+    const auto it = std::find(dmethods.begin(), dmethods.end(),
+                              DexMethod::make_method(cls, name, rtype, args));
+    return it == dmethods.end() ? nullptr : *it;
+  }
+
   template <typename C>
   DexMethod* find_vmethod(const C& clazzes,
                           const char* cls,

test/integ/ReachabilityTest.cpp

@@ -30,8 +30,8 @@ TEST_F(ReachabilityTest, ReachabilityFromProguardTest) {
 
   reachability::ObjectCounts before = reachability::count_objects(stores);
 
-  EXPECT_EQ(before.num_classes, 19);
-  EXPECT_EQ(before.num_methods, 37);
+  EXPECT_EQ(before.num_classes, 20);
+  EXPECT_EQ(before.num_methods, 39);
   EXPECT_EQ(before.num_fields, 3);
 
   int num_ignore_check_strings = 0;
@@ -66,8 +66,8 @@ TEST_F(ReachabilityTest, ReachabilityMarkAllTest) {
 
   reachability::ObjectCounts before = reachability::count_objects(stores);
 
-  EXPECT_EQ(before.num_classes, 19);
-  EXPECT_EQ(before.num_methods, 37);
+  EXPECT_EQ(before.num_classes, 20);
+  EXPECT_EQ(before.num_methods, 39);
   EXPECT_EQ(before.num_fields, 3);
 
   int num_ignore_check_strings = 0;
@@ -81,7 +81,7 @@ TEST_F(ReachabilityTest, ReachabilityMarkAllTest) {
 
   reachability::ObjectCounts after = reachability::count_objects(stores);
 
-  EXPECT_EQ(after.num_classes, 19);
-  EXPECT_EQ(after.num_methods, 37);
+  EXPECT_EQ(after.num_classes, 20);
+  EXPECT_EQ(after.num_methods, 39);
   EXPECT_EQ(after.num_fields, 3);
 }

test/integ/RemoveUnreachableTest.cpp

@@ -113,7 +113,12 @@ TEST_F(RemoveUnreachableTest, Inheritance3Test) {
 
   // Another tricky inheritance case.
   ASSERT_TRUE(find_class(*classes, "LHoneyBadger;"));
-  ASSERT_TRUE(find_vmethod(*classes, "LHoneyBadger;", "Z", "isAwesome", {}));
+  ASSERT_FALSE(find_dmethod(*classes, "LHoneyBadger;", "V", "<init>", {"Z"}));
+  ASSERT_FALSE(find_vmethod(*classes, "LHoneyBadger;", "Z", "isAwesome", {}));
+  ASSERT_TRUE(
+      find_dmethod(*classes, "LHoneyBadgerInstantiated;", "V", "<init>", {}));
+  ASSERT_TRUE(find_vmethod(
+      *classes, "LHoneyBadgerInstantiated;", "Z", "isAwesome", {}));
   // You might think that HogBadger.isAwesome() can be removed, since it
   // doesn't extend Badger.  But it's very tricky to remove this while still
   // getting the Guava Hasher case (below) correct.
@@ -247,7 +252,12 @@ TEST_F(RemoveUnreachableTest, TypeAnalysisInheritance3Test) {
 
   // Another tricky inheritance case.
   ASSERT_TRUE(find_class(*classes, "LHoneyBadger;"));
-  ASSERT_TRUE(find_vmethod(*classes, "LHoneyBadger;", "Z", "isAwesome", {}));
+  ASSERT_FALSE(find_dmethod(*classes, "LHoneyBadger;", "V", "<init>", {"Z"}));
+  ASSERT_FALSE(find_vmethod(*classes, "LHoneyBadger;", "Z", "isAwesome", {}));
+  ASSERT_TRUE(
+      find_dmethod(*classes, "LHoneyBadgerInstantiated;", "V", "<init>", {}));
+  ASSERT_TRUE(find_vmethod(
+      *classes, "LHoneyBadgerInstantiated;", "Z", "isAwesome", {}));
   // You might think that HogBadger.isAwesome() can be removed, since it
   // doesn't extend Badger.  But it's very tricky to remove this while still
   // getting the Guava Hasher case (below) correct.