Purity analysis should not exclude explicitly specified external/nati…

…ve methods Summary: This was a silly short-coming of the analysis that determines whether methods are pure/have no side effects, which would not consider external/native method, even when they were explicitly specified as pure. The improved analysis benefits CSE and LocalDCE. Reviewed By: amnn Differential Revision: D19737687 fbshipit-source-id: c10aef7e4e3a8ae38efc6043b7d223511d3d1e22
kami-lang · Mar 6, 2020 · 250b7ad · 250b7ad
1 parent 9e40492
commit 250b7ad
Show file tree

Hide file tree

Showing 6 changed files with 112 additions and 28 deletions.
diff --git a/libredex/Purity.cpp b/libredex/Purity.cpp
@@ -309,7 +309,9 @@ std::unordered_set<DexMethodRef*> get_pure_methods() {
 }
 
 MethodOverrideAction get_base_or_overriding_method_action(
-    const DexMethod* method, bool ignore_methods_with_assumenosideeffects) {
+    const DexMethod* method,
+    const std::unordered_set<const DexMethod*>* methods_to_ignore,
+    bool ignore_methods_with_assumenosideeffects) {
   if (method == nullptr || method::is_clinit(method) ||
       method->rstate.no_optimizations()) {
     return MethodOverrideAction::UNKNOWN;
@@ -323,6 +325,10 @@ MethodOverrideAction get_base_or_overriding_method_action(
     return MethodOverrideAction::UNKNOWN;
   }
 
+  if (methods_to_ignore && methods_to_ignore->count(method)) {
+    return MethodOverrideAction::EXCLUDE;
+  }
+
   if (ignore_methods_with_assumenosideeffects && assumenosideeffects(method)) {
     return MethodOverrideAction::EXCLUDE;
   }
@@ -341,33 +347,48 @@ MethodOverrideAction get_base_or_overriding_method_action(
 bool process_base_and_overriding_methods(
     const method_override_graph::Graph* method_override_graph,
     const DexMethod* method,
+    const std::unordered_set<const DexMethod*>* methods_to_ignore,
     bool ignore_methods_with_assumenosideeffects,
     const std::function<bool(DexMethod*)>& handler_func) {
   auto action = get_base_or_overriding_method_action(
-      method, ignore_methods_with_assumenosideeffects);
+      method, methods_to_ignore, ignore_methods_with_assumenosideeffects);
   if (action == MethodOverrideAction::UNKNOWN ||
       (action == MethodOverrideAction::INCLUDE &&
        !handler_func(const_cast<DexMethod*>(method)))) {
     return false;
   }
-  if (method->is_virtual() && !(ignore_methods_with_assumenosideeffects &&
-                                assumenosideeffects(method))) {
-    if (!method_override_graph) {
+  // When the method isn't virtual, there are no overriden methods to consider.
+  if (!method->is_virtual()) {
+    return true;
+  }
+  // But even if there are overriden methods, don't look further when the
+  // method is to be ignored.
+  if (methods_to_ignore && methods_to_ignore->count(method)) {
+    return true;
+  }
+  if (ignore_methods_with_assumenosideeffects && assumenosideeffects(method)) {
+    return true;
+  }
+
+  // When we don't have a method-override graph, let's be conservative and give
+  // up.
+  if (!method_override_graph) {
+    return false;
+  }
+
+  // Okay, let's process all overridden methods just like the base method.
+  auto overriding_methods = method_override_graph::get_overriding_methods(
+      *method_override_graph, method);
+  for (auto overriding_method : overriding_methods) {
+    action = get_base_or_overriding_method_action(
+        overriding_method, methods_to_ignore,
+        ignore_methods_with_assumenosideeffects);
+    if (action == MethodOverrideAction::UNKNOWN ||
+        (action == MethodOverrideAction::INCLUDE &&
+         !handler_func(const_cast<DexMethod*>(overriding_method)))) {
       return false;
     }
-    auto overriding_methods = method_override_graph::get_overriding_methods(
-        *method_override_graph, method);
-    for (auto overriding_method : overriding_methods) {
-      action = get_base_or_overriding_method_action(
-          overriding_method, ignore_methods_with_assumenosideeffects);
-      if (action == MethodOverrideAction::UNKNOWN ||
-          (action == MethodOverrideAction::INCLUDE &&
-           !handler_func(const_cast<DexMethod*>(overriding_method)))) {
-        return false;
-      }
-    }
   }
-
   return true;
 }
 
@@ -555,19 +576,17 @@ static size_t analyze_read_locations(
   return compute_locations_closure(
       scope, method_override_graph,
       [&](DexMethod* method) -> boost::optional<LocationsAndDependencies> {
-        auto action =
-            pure_methods_closure.count(method)
-                ? MethodOverrideAction::EXCLUDE
-                : get_base_or_overriding_method_action(
-                      method, ignore_methods_with_assumenosideeffects);
+        auto action = get_base_or_overriding_method_action(
+            method, &pure_methods_closure,
+            ignore_methods_with_assumenosideeffects);
 
         if (action == MethodOverrideAction::UNKNOWN) {
           return boost::none;
         }
 
         LocationsAndDependencies lads;
         if (!process_base_and_overriding_methods(
-                method_override_graph, method,
+                method_override_graph, method, &pure_methods_closure,
                 ignore_methods_with_assumenosideeffects,
                 [&](DexMethod* other_method) {
                   if (other_method != method) {
@@ -623,6 +642,7 @@ static size_t analyze_read_locations(
                                                       opcode_to_search(opcode));
                   if (!process_base_and_overriding_methods(
                           method_override_graph, invoke_method,
+                          &pure_methods_closure,
                           ignore_methods_with_assumenosideeffects,
                           [&](DexMethod* other_method) {
                             if (other_method != method) {
@@ -696,7 +716,7 @@ bool has_implementor(const method_override_graph::Graph* method_override_graph,
   }
   bool found_implementor = false;
   if (!process_base_and_overriding_methods(
-          method_override_graph, method,
+          method_override_graph, method, /* methods_to_ignore */ nullptr,
           /* ignore_methods_with_assumenosideeffects */ false, [&](DexMethod*) {
             found_implementor = true;
             return true;

diff --git a/libredex/Purity.h b/libredex/Purity.h
@@ -118,7 +118,9 @@ enum class MethodOverrideAction {
 // Determine what action to take for a method while traversing a base method
 // and its overriding methods.
 MethodOverrideAction get_base_or_overriding_method_action(
-    const DexMethod* method, bool ignore_methods_with_assumenosideeffects);
+    const DexMethod* method,
+    const std::unordered_set<const DexMethod*>* methods_to_ignore,
+    bool ignore_methods_with_assumenosideeffects);
 
 // Given a (base) method, iterate over all relevant (base + overriding)
 // methods, and run a handler method for each method that should be included
@@ -128,6 +130,7 @@ MethodOverrideAction get_base_or_overriding_method_action(
 bool process_base_and_overriding_methods(
     const method_override_graph::Graph* method_override_graph,
     const DexMethod* method,
+    const std::unordered_set<const DexMethod*>* methods_to_ignore,
     bool ignore_methods_with_assumenosideeffects,
     const std::function<bool(DexMethod*)>& handler_func);
 

diff --git a/opt/throw-propagation/ThrowPropagationPass.cpp b/opt/throw-propagation/ThrowPropagationPass.cpp
@@ -131,7 +131,7 @@ ThrowPropagationPass::Stats ThrowPropagationPass::run(
       return true;
     };
     if (!process_base_and_overriding_methods(
-            &graph, method,
+            &graph, method, /* methods_to_ignore */ nullptr,
             /* ignore_methods_with_assumenosideeffects */ false,
             check_for_no_return)) {
       return false;

diff --git a/service/cse/CommonSubexpressionElimination.cpp b/service/cse/CommonSubexpressionElimination.cpp
@@ -925,7 +925,8 @@ void SharedState::init_method_barriers(const Scope& scope) {
       scope, m_method_override_graph.get(),
       [&](DexMethod* method) -> boost::optional<LocationsAndDependencies> {
         auto action = get_base_or_overriding_method_action(
-            method, /* ignore_methods_with_assumenosideeffects */ true);
+            method, &m_safe_method_defs,
+            /* ignore_methods_with_assumenosideeffects */ true);
         if (action == MethodOverrideAction::UNKNOWN) {
           return boost::none;
         }
@@ -955,6 +956,7 @@ void SharedState::init_method_barriers(const Scope& scope) {
 
             if (!process_base_and_overriding_methods(
                     m_method_override_graph.get(), barrier.method,
+                    &m_safe_method_defs,
                     /* ignore_methods_with_assumenosideeffects */ true,
                     [&](DexMethod* other_method) {
                       if (other_method != method) {
@@ -994,6 +996,13 @@ void SharedState::init_scope(const Scope& scope) {
     m_pure_methods.insert(const_cast<DexMethod*>(p.first));
   }
 
+  for (auto method_ref : m_safe_methods) {
+    auto method = method_ref->as_def();
+    if (method) {
+      m_safe_method_defs.insert(method);
+    }
+  }
+
   init_method_barriers(scope);
 }
 
@@ -1090,7 +1099,7 @@ CseUnorderedLocationSet SharedState::get_relevant_written_locations(
   DexMethod* method = resolve_method(method_ref, opcode_to_search(insn));
   CseUnorderedLocationSet written_locations;
   if (!process_base_and_overriding_methods(
-          m_method_override_graph.get(), method,
+          m_method_override_graph.get(), method, &m_safe_method_defs,
           /* ignore_methods_with_assumenosideeffects */ true,
           [&](DexMethod* other_method) {
             auto it = m_method_written_locations.find(other_method);

diff --git a/service/cse/CommonSubexpressionElimination.h b/service/cse/CommonSubexpressionElimination.h
@@ -83,7 +83,10 @@ class SharedState {
       const IRInstruction* insn, const CseUnorderedLocationSet& read_locations);
   // after init_scope, m_pure_methods will include m_conditionally_pure_methods
   std::unordered_set<DexMethodRef*> m_pure_methods;
+  // methods which never represent barriers
   std::unordered_set<DexMethodRef*> m_safe_methods;
+  // subset of safe methods which are in fact defs
+  std::unordered_set<const DexMethod*> m_safe_method_defs;
   std::unique_ptr<ConcurrentMap<Barrier, size_t, BarrierHasher>> m_barriers;
   std::unordered_map<const DexMethod*, CseUnorderedLocationSet>
       m_method_written_locations;

diff --git a/test/unit/LocalDceTest.cpp b/test/unit/LocalDceTest.cpp
@@ -17,11 +17,16 @@
 #include "Purity.h"
 #include "RedexTest.h"
 #include "ScopeHelper.h"
+#include "VirtualScope.h"
 
 struct LocalDceTryTest : public RedexTest {
   DexMethod* m_method;
 
   LocalDceTryTest() {
+    // Calling get_vmethods under the hood initializes the object-class, which
+    // we need in the tests to create a proper scope
+    get_vmethods(type::java_lang_Object());
+
     auto args = DexTypeList::make_type_list({});
     auto proto = DexProto::make_proto(type::_void(), args);
     m_method =
@@ -495,6 +500,50 @@ TEST_F(LocalDceEnhanceTest, NoImplementorMayAllocateRegistersTest) {
   auto override_graph = method_override_graph::build_graph(scope);
   LocalDce ldce(pure_methods, override_graph.get(),
                 /* may_allocate_registers */ true);
+}
+
+TEST_F(LocalDceTryTest, invoked_static_method_with_pure_external_barrier) {
+  ClassCreator creator(DexType::make_type("LNativeTest;"));
+  creator.set_super(type::java_lang_Object());
+
+  auto native_method =
+      DexMethod::make_method("LNativeTest;.native:()V")
+          ->make_concrete(ACC_PUBLIC | ACC_STATIC | ACC_NATIVE, false);
+
+  auto method = DexMethod::make_method("LNativeTest;.test:()V")
+                    ->make_concrete(ACC_PUBLIC | ACC_STATIC, false);
+  method->set_code(assembler::ircode_from_string(R"(
+                    (
+                      (invoke-static () "LNativeTest;.native:()V")
+                      (return-void)
+                    )
+                    )"));
+  creator.add_method(method);
+
+  auto code = assembler::ircode_from_string(R"(
+    (
+      (invoke-static () "LNativeTest;.test:()V")
+      (return-void)
+    )
+  )");
+  auto expected_code = assembler::ircode_from_string(R"(
+    (
+      (return-void)
+    )
+  )");
+
+  Scope scope{type_class(type::java_lang_Object()), creator.create()};
+  std::unordered_set<DexMethodRef*> pure_methods = {native_method};
+  // We are computing other no-side-effects method just like the LocalDcePass
+  // would.
+  auto override_graph = method_override_graph::build_graph(scope);
+  std::unordered_set<const DexMethod*> computed_no_side_effects_methods;
+  compute_no_side_effects_methods(scope, override_graph.get(), pure_methods,
+                                  &computed_no_side_effects_methods);
+  for (auto m : computed_no_side_effects_methods) {
+    pure_methods.insert(const_cast<DexMethod*>(m));
+  }
+  LocalDce ldce(pure_methods);
   IRCode* ircode = code.get();
   ldce.dce(ircode);
   EXPECT_CODE_EQ(ircode, expected_code.get());