Repeating life cycles #168

ZeyadTarekk · 2024-08-07T12:57:04Z

The current syntax only allows the generation of a succession of method calls.
We have changed the design slightly, we now want the user to provide a control flow graph structure.

arthaud · 2024-08-07T13:29:08Z

source/LifecycleMethod.h

+        }
+      };
+
+      std::unordered_map<LifecycleMethodCall, std::vector<LifecycleMethodCall>, NodeHasher> adj_list_;


This representation doesn't seem quite right. Why is the key a MethodCall itself? I thought we said it should be the name used as the key in the json. It should just be a string.
The parsing logic is quite weird, since we seem to create one node for each call, but a node is a std::vector. Basically, you are never using vectors of more than 1 element.

I think this should be redesigned to be something like:
std::unordered_map<std::stringg, LifecycleGraphNode> nodes_
And LifecycleGraphNode should have a std::vector<LifecycleMethodCall> has well as a std::vector<std::string> successors.

arthaud · 2024-08-07T13:30:18Z

source/LifecycleMethod.h

@@ -174,7 +201,7 @@ class LifecycleMethod {

  std::string base_class_name_;
  std::string method_name_;
-  std::vector<LifecycleMethodCall> callees_;
+  std::variant<std::vector<LifecycleMethodCall>,LifeCycleMethodGraph> callees_;


Maybe rename this as body_ since this represents the method body (i.e content).

^ rename this body_ as said before.

arthaud · 2024-08-07T13:30:56Z

source/LifecycleMethod.cpp

-  for (const auto& callee : callees_) {
-    auto* dex_method = callee.get_dex_method(dex_klass);
-    if (!dex_method) {
-      // Dex method does not apply for current APK.
-      // See `LifecycleMethod::validate()`.
-      continue;
-    }
-
-    ++callee_count;
-
-    std::vector<Location> invoke_with_registers{this_location};
-    auto* type_list = callee.get_argument_types();
-    // This should have been verified at the start of `create_methods`
-    mt_assert(type_list != nullptr);
-    for (auto* type : *type_list) {
-      auto argument_register = method.get_local(type_index_map.at(type));
-      invoke_with_registers.push_back(argument_register);
-    }
-    main_block->invoke(
-        IROpcode::OPCODE_INVOKE_VIRTUAL, dex_method, invoke_with_registers);
-  }
+  // for (const auto& callee : callees_) {
+  //   auto* dex_method = callee.get_dex_method(dex_klass);
+  //   if (!dex_method) {
+  //     // Dex method does not apply for current APK.
+  //     // See `LifecycleMethod::validate()`.
+  //     continue;
+  //   }
+
+  //   ++callee_count;
+
+  //   std::vector<Location> invoke_with_registers{this_location};
+  //   auto* type_list = callee.get_argument_types();
+  //   // This should have been verified at the start of `create_methods`
+  //   mt_assert(type_list != nullptr);
+  //   for (auto* type : *type_list) {
+  //     auto argument_register = method.get_local(type_index_map.at(type));
+  //     invoke_with_registers.push_back(argument_register);
+  //   }
+  //   main_block->invoke(
+  //       IROpcode::OPCODE_INVOKE_VIRTUAL, dex_method, invoke_with_registers);
+  // }


Obviously, we will need to keep this from now otherwise this will break existing lifecycle configurations.

arthaud · 2024-08-14T09:55:10Z

source/LifecycleMethod.h

+      const std::vector<std::string>& successors() const;
+      bool operator==(const LifecycleGraphNode& other) const;
+
+      INCLUDE_DEFAULT_COPY_CONSTRUCTORS_AND_ASSIGNMENTS(LifecycleGraphNode)


for consistency with the rest of our codebase, please move this right after the constructor

arthaud · 2024-08-14T09:56:32Z

source/LifecycleMethod.h

+      const std::vector<LifecycleMethodCall>& method_calls() const;
+      const std::vector<std::string>& successors() const;


nitpick: inline these since the implementation is really small

can we inline the implementation of these, since those should be small?

Suggested change

const std::vector<LifecycleMethodCall>& method_calls() const;

const std::vector<std::string>& successors() const;

const std::vector<LifecycleMethodCall>& method_calls() const { return method_calls_; }

const std::vector<std::string>& successors() const { return successors_; }

arthaud · 2024-08-14T09:59:31Z

source/LifecycleMethod.h

+      void addNode(const std::string& node_name,std::vector<LifecycleMethodCall> method_calls,std::vector<std::string> successors);
+
+      const LifecycleGraphNode* getNode(const std::string& node_name) const;


nitpick: we use snake_case for method names, so it should be add_node and get_node.

arthaud · 2024-08-14T09:59:56Z

source/LifecycleMethod.h

@@ -174,7 +201,7 @@ class LifecycleMethod {

  std::string base_class_name_;
  std::string method_name_;
-  std::vector<LifecycleMethodCall> callees_;
+  std::variant<std::vector<LifecycleMethodCall>,LifeCycleMethodGraph> callees_;


^ rename this body_ as said before.

arthaud · 2024-08-14T10:00:32Z

source/LifecycleMethod.cpp

+  if (nodes_.size() != other.nodes_.size()) {
+    return false;
+  }
+


This is not really necessary, nodes_ == other.nodes_ will do the same check.

arthaud · 2024-08-14T10:02:02Z

source/LifecycleMethod.cpp

+    JsonValidation::validate_object(node, "node");
+    const auto& instructions = node["instructions"];


I think those are not necessary anymore?

arthaud · 2024-08-14T10:03:04Z

source/LifecycleMethod.cpp

+    for (const auto& callee : JsonValidation::null_or_array(value, "callees")) {
+      callees.push_back(LifecycleMethodCall::from_json(callee));
+    }
+    return LifecycleMethod(base_class_name, method_name, callees);


nitpick: std::move

Suggested change

return LifecycleMethod(base_class_name, method_name, callees);

return LifecycleMethod(base_class_name, method_name, std::move(callees));

arthaud · 2024-08-14T10:03:17Z

source/LifecycleMethod.cpp

+  } else if (JsonValidation::has_field(value, "control_flow_graph")) {
+    JsonValidation::validate_object(value, "control_flow_graph");
+    LifeCycleMethodGraph graph = LifeCycleMethodGraph::from_json(JsonValidation::object(value, "control_flow_graph"));
+    return LifecycleMethod(base_class_name, method_name, graph);


also here

Suggested change

return LifecycleMethod(base_class_name, method_name, graph);

return LifecycleMethod(base_class_name, method_name, std::move(graph));

arthaud · 2024-08-14T10:06:01Z

source/LifecycleMethod.cpp

  }
-
-  return LifecycleMethod(base_class_name, method_name, callees);
+  throw std::invalid_argument("Invalid JSON format for LifecycleMethod");


let's be a bit more descriptive, otherwise this will be confusing to the user.
We should also use JsonValidationError.

Suggested change

throw std::invalid_argument("Invalid JSON format for LifecycleMethod");

throw JsonValidationError(

value,

/* field */ std::nullopt,

"key `callees` or `control_flow_graph`");

arthaud · 2024-08-14T10:09:53Z

source/LifecycleMethod.cpp

+  // for (const auto& callee : callees_) {
+  //   callee.validate(base_class, class_hierarchies);
+  // }


This still needs to be addressed. And same thing below.
You can do something like:

if (const auto* callees = std::get_if<std::vector<LifecycleMethodCall>>(callees_)) { // handle *callees } else { const auto& graph = std::get< LifeCycleMethodGraph>(callees_); // handle graph }

arthaud · 2024-08-14T10:28:09Z

Could we also add tests, as I suggested last week?

arthaud · 2024-08-14T11:29:12Z

source/LifecycleMethod.cpp

+    for (const auto& callee : *callees) {
+      auto* dex_method = callee.get_dex_method(dex_klass);
+      if (!dex_method) {
+        // Dex method does not apply for current APK.
+        // See `LifecycleMethod::validate()`.
+        continue;
+      }
+
+      ++callee_count;

-    std::vector<Location> invoke_with_registers{this_location};
-    auto* type_list = callee.get_argument_types();
-    // This should have been verified at the start of `create_methods`
-    mt_assert(type_list != nullptr);
-    for (auto* type : *type_list) {
-      auto argument_register = method.get_local(type_index_map.at(type));
-      invoke_with_registers.push_back(argument_register);
+      std::vector<Location> invoke_with_registers{this_location};
+      auto* type_list = callee.get_argument_types();
+      // This should have been verified at the start of `create_methods`
+      mt_assert(type_list != nullptr);
+      for (auto* type : *type_list) {
+        auto argument_register = method.get_local(type_index_map.at(type));
+        invoke_with_registers.push_back(argument_register);
+      }
+      main_block->invoke(
+          IROpcode::OPCODE_INVOKE_VIRTUAL, dex_method, invoke_with_registers);
    }
-    main_block->invoke(
-        IROpcode::OPCODE_INVOKE_VIRTUAL, dex_method, invoke_with_registers);


maybe move these in a function generate_invoke_instructions that take a list of LifecycleMethodCall and the main_block. This will be useful for the next step.

arthaud · 2024-08-14T11:31:23Z

source/LifecycleMethod.cpp

-  for (const auto& callee : callees_) {
-    callee.validate(base_class, class_hierarchies);
+  if (const auto* callees = std::get_if<std::vector<LifecycleMethodCall>>(&body_)) {
+    // handle *callees


nitpick: remove this comment

arthaud · 2024-08-20T10:28:24Z

source/LifecycleMethod.h

@@ -53,6 +53,8 @@ class LifecycleMethodCall {

  static LifecycleMethodCall from_json(const Json::Value& value);

+  const std::string get_method_name() const { return method_name_; }


missing & here

Suggested change

const std::string get_method_name() const { return method_name_; }

const std::string& get_method_name() const { return method_name_; }

source/tests/JsonTest.cpp

facebook-github-bot · 2024-08-20T10:50:27Z

@arthaud has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2024-08-21T12:01:11Z

@arthaud merged this pull request in c20adf3.

ZeyadTarekk added 3 commits August 7, 2024 14:15

Add the new graph class

7446f34

Handle the lifecyclemethod in the new way

6608266

Fix some errors

9cb0804

facebook-github-bot added the cla signed label Aug 7, 2024

arthaud requested changes Aug 7, 2024

View reviewed changes

Change the representation of the graph

d3aa914

ZeyadTarekk requested a review from arthaud August 14, 2024 09:53

arthaud requested changes Aug 14, 2024

View reviewed changes

ZeyadTarekk added 4 commits August 14, 2024 13:22

Move the default constructor

282a3fb

Add the 2 methods inline

0217506

use snake_case

450d711

Update == operator

fd64f4b

ZeyadTarekk added 2 commits August 14, 2024 13:30

Use different error handling

f42c533

Handle the variant type

29e2dda

ZeyadTarekk requested a review from arthaud August 14, 2024 11:06

arthaud requested changes Aug 14, 2024

View reviewed changes

ZeyadTarekk added 2 commits August 19, 2024 18:02

Remove comment

59d1843

Add unit test for parsing the graph

d99209a

ZeyadTarekk requested a review from arthaud August 19, 2024 16:55

arthaud requested changes Aug 20, 2024

View reviewed changes

Uncomment tests

6f1c2f4

ZeyadTarekk requested a review from arthaud August 20, 2024 10:48

facebook-github-bot closed this in c20adf3 Aug 21, 2024

facebook-github-bot added the Merged label Aug 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Repeating life cycles #168

Repeating life cycles #168

ZeyadTarekk commented Aug 7, 2024

arthaud Aug 7, 2024

arthaud Aug 7, 2024

arthaud Aug 14, 2024

arthaud Aug 7, 2024

arthaud Aug 14, 2024

arthaud Aug 14, 2024

arthaud Aug 14, 2024

arthaud Aug 14, 2024

arthaud Aug 14, 2024

arthaud Aug 14, 2024

arthaud Aug 14, 2024

arthaud Aug 14, 2024

arthaud Aug 14, 2024

arthaud Aug 14, 2024

arthaud commented Aug 14, 2024

arthaud Aug 14, 2024

arthaud Aug 14, 2024

arthaud Aug 20, 2024

facebook-github-bot commented Aug 20, 2024

facebook-github-bot commented Aug 21, 2024

		const std::vector<LifecycleMethodCall>& method_calls() const;
		const std::vector<std::string>& successors() const;

		void addNode(const std::string& node_name,std::vector<LifecycleMethodCall> method_calls,std::vector<std::string> successors);

		const LifecycleGraphNode* getNode(const std::string& node_name) const;

		JsonValidation::validate_object(node, "node");
		const auto& instructions = node["instructions"];

	return LifecycleMethod(base_class_name, method_name, callees);
	return LifecycleMethod(base_class_name, method_name, std::move(callees));

-  throw std::invalid_argument("Invalid JSON format for LifecycleMethod");
+  throw JsonValidationError(
+          value,
+          /* field */ std::nullopt,
+          "key `callees` or `control_flow_graph`");

		@@ -53,6 +53,8 @@ class LifecycleMethodCall {

		static LifecycleMethodCall from_json(const Json::Value& value);

		const std::string get_method_name() const { return method_name_; }

	const std::string get_method_name() const { return method_name_; }
	const std::string& get_method_name() const { return method_name_; }

Repeating life cycles #168

Repeating life cycles #168

Conversation

ZeyadTarekk commented Aug 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arthaud commented Aug 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Aug 20, 2024

facebook-github-bot commented Aug 21, 2024