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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
