[Lldb-commits] [PATCH] D145297: [lldb] Add an example of interactive scripted process debugging

[Alex Langford via Phabricator via lldb-commits]

bulbazord added a comment.

A few high-level comments:

- You're doing a lot of type-annotations in python which is good, but you're not being very consistent about it. It would be tremendously helpful if you could add type annotations everywhere.
- I would recommend using f-strings over `%` and `.format` for string interpolation. They are available as of python 3.6 and the minimum version to use `lit` is 3.6 (if LLVM's CMakeLists.txt is to be believed).





================
Comment at: lldb/test/API/functionalities/interactive_scripted_process/TestInteractiveScriptedProcess.py:2
+"""
+Test the functionality of scripted processes
+"""
----------------
nit


================
Comment at: lldb/test/API/functionalities/interactive_scripted_process/TestInteractiveScriptedProcess.py:8
+from lldbsuite.test.lldbtest import *
+import os
+
----------------
nit: import json


================
Comment at: lldb/test/API/functionalities/interactive_scripted_process/TestInteractiveScriptedProcess.py:54
+
+        mux_class = self.script_module + "." + "MultiplexerScriptedProcess"
+        script_dict = {'driving_target_idx' : real_target_id}
----------------



================
Comment at: lldb/test/API/functionalities/interactive_scripted_process/TestInteractiveScriptedProcess.py:60-61
+
+        self.runCmd("log enable lldb state -f /tmp/state.log")
+        # self.runCmd("log enable lldb event -f /tmp/event.log")
+        self.dbg.SetAsync(True)
----------------
Remove these log lines, I don't think they're needed for the test.


================
Comment at: lldb/test/API/functionalities/interactive_scripted_process/TestInteractiveScriptedProcess.py:89
+        self.assertEqual(len(real_process.threads), len(mux_process.threads), "Same number of threads")
+        for id in range(0, len(real_process.threads)):
+            real_pc = real_process.threads[id].frame[0].pc
----------------
Defaults to 0 if you don't specify a start.


================
Comment at: lldb/test/API/functionalities/interactive_scripted_process/TestInteractiveScriptedProcess.py:102-103
+        self.assertState(lldb.SBProcess.GetStateFromEvent(event), lldb.eStateRunning)
+        event = lldbutil.fetch_next_event(self, mux_process_listener, mux_process.GetBroadcaster())
+        self.assertState(lldb.SBProcess.GetStateFromEvent(event), lldb.eStateStopped)
+
----------------
Did you mean to get info from the `mux_process_listener` twice? Was one of these supposed to be for the real process?


================
Comment at: lldb/test/API/functionalities/interactive_scripted_process/interactive_scripted_process.py:9
+
+import os,json,struct,signal
+import time
----------------
nit: import these all on different lines


================
Comment at: lldb/test/API/functionalities/interactive_scripted_process/interactive_scripted_process.py:74-89
+    def write_memory_at_address(self, addr, data, error):
+        return self.driving_process.WriteMemory(addr,
+                                                bytearray(data.uint8.all()),
+                                                error)
+
+    def get_loaded_images(self):
+        return self.loaded_images
----------------
Some functions have their parameter types and return types annotated but others don't. Can you add all the annotations to be consistent?


================
Comment at: lldb/test/API/functionalities/interactive_scripted_process/interactive_scripted_process.py:89
+    def get_scripted_thread_plugin(self):
+        return PassthruScriptedThread.__module__ + "." + PassthruScriptedThread.__name__
+
----------------



================
Comment at: lldb/test/API/functionalities/interactive_scripted_process/interactive_scripted_process.py:132
+    def get_scripted_thread_plugin(self):
+        return MultiplexedScriptedThread.__module__ + "." + MultiplexedScriptedThread.__name__
+
----------------



================
Comment at: lldb/test/API/functionalities/interactive_scripted_process/interactive_scripted_process.py:165
+    def get_name(self) -> str:
+        return PassthruScriptedThread.__name__ + ".thread-" + str(self.idx)
+
----------------



================
Comment at: lldb/test/API/functionalities/interactive_scripted_process/interactive_scripted_process.py:168
+    def get_stop_reason(self) -> Dict[str, Any]:
+        stop_reason = { "type": lldb.eStopReasonInvalid, "data": {  }}
+
----------------



================
Comment at: lldb/test/API/functionalities/interactive_scripted_process/interactive_scripted_process.py:204
+
+        return struct.pack("{}Q".format(len(self.register_ctx)), *self.register_ctx.values())
+
----------------



================
Comment at: lldb/test/API/functionalities/interactive_scripted_process/interactive_scripted_process.py:209
+        parity = "Odd" if self.scripted_process.parity % 2 else "Even"
+        return parity + MultiplexedScriptedThread.__name__ + ".thread-" + str(self.idx)
+
----------------



================
Comment at: lldb/test/API/functionalities/interactive_scripted_process/interactive_scripted_process.py:223
+            for driving_thread in self.driving_process:
+                log("%d New thread %s" % (len(self.threads), hex(driving_thread.id)))
+                structured_data = lldb.SBStructuredData()
----------------



================
Comment at: lldb/test/API/functionalities/interactive_scripted_process/interactive_scripted_process.py:247
+                    if state == lldb.eStateStopped:
+                        log("Received public process state event: %s" % state)
+                        # If it's a stop event, iterate over the driving process
----------------



================
Comment at: lldb/test/API/functionalities/interactive_scripted_process/interactive_scripted_process.py:253
+                    else:
+                        log("Received public process state event: %s" % state)
+            else:
----------------



================
Comment at: lldb/test/API/functionalities/interactive_scripted_process/interactive_scripted_process.py:269
+        if not self.driving_target:
+            return lldb.SBError("%s.resume: Invalid driving target." %
+                                self.__class__.__name)
----------------
f-string this


================
Comment at: lldb/test/API/functionalities/interactive_scripted_process/interactive_scripted_process.py:273
+        if self.driving_process:
+            return lldb.SBError("%s.resume: Invalid driving process." %
+                                self.__class__.__name)
----------------
f-string this


================
Comment at: lldb/test/API/functionalities/interactive_scripted_process/interactive_scripted_process.py:310
+        if not self.driving_process:
+            return lldb.SBError("%s.resume: Invalid driving process." %
+                                self.__class__.__name__)
----------------
f-string


================
Comment at: lldb/test/API/functionalities/interactive_scripted_process/interactive_scripted_process.py:344-350
+def extract_value_from_structured_data(data, default_val):
+    if data and data.IsValid():
+        if data.GetType() == lldb.eStructuredDataTypeInteger:
+            return data.GetIntegerValue(default_val)
+        if data.GetType() == lldb.eStructuredDataTypeString:
+            return int(data.GetStringValue(100))
+    return None
----------------
This function is a little confusing to me. Why does `default_val` only apply if the `data` is valid? Why not return the `default_val` when it's invalid?


================
Comment at: lldb/test/API/functionalities/interactive_scripted_process/main.cpp:20
+int main() {
+  size_t num_threads = 10;
+  std::vector<std::thread> threads;
----------------
nit: constexpr


================
Comment at: lldb/test/API/functionalities/interactive_scripted_process/main.cpp:27
+
+  std::cout << "Spawned " << threads.size() << " threads!"; // Break here
+
----------------



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D145297/new/

https://reviews.llvm.org/D145297