jingham added a comment.
A bunch of little comments. I was also curious why you directly set the
running private state, but use the async thread to generate the stopped events?
================
Comment at: lldb/source/Plugins/Process/CMakeLists.txt:15
endif()
+if (LLDB_ENABLE_PYTHON)
+ add_subdirectory(scripted)
----------------
Why is this necessary? The code in Plugins/Process/scripted should work for
any script interpreter (and should handle the case when there isn't one.)
================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp:78
+
+ if (!m_interpreter)
+ return;
----------------
If making the scripted process fails because there's no script interpreter, it
would be good to get that fact out to the user. Maybe the ScriptedProcess
constructor should take a Status object that it's caller can somehow bubble up?
================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp:85
+
+ if (object_sp && object_sp->IsValid())
+ m_script_object_sp = object_sp;
----------------
If these two conditions aren't true, do you actually want to continue? Seems
like you should error out here as well.
================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp:170
+
+ if (m_async_thread.IsJoinable()) {
+ m_async_thread.Join(nullptr);
----------------
Does it do any good to broadcast the ShouldExit bit if the Async thread isn't
running? If you're following the llvm style, you would do:
if (!m_async_thread.IsJoinable()) {
// Do your logging
return;
}
First, and then all the actual work of the function outside an if. That would
the odd look of sending an event that you know is going to a particular thread
when the thread isn't running?
================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp:261
+Status ScriptedProcess::WillLaunch(Module *module) {
+ if (m_interpreter)
+ m_pid = GetInterface().GetProcessID();
----------------
I don't know if you actually need this, but it seems weird to be setting the
PID in WillLaunch. I don't think there's any real process plugin that would
know the PID of the launched process in WillLaunch. Maybe it just looks odd,
but still is seems worthwhile to follow the behavior of "real" process plugins
wherever you can.
================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp:300
+
+ if (!m_interpreter)
+ return Status("No interpreter.");
----------------
Maybe these should be asserts. It really shouldn't be possible to get to
DoResume for a ScriptedProcess with no interpreter and no script object. Also
messages in the generic ScriptProcess code shouldn't refer to Python directly.
You don't know that that's the script interpreter language being used here.
================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp:315
+
+ Status status;
+ if (resume) {
----------------
We call Status objects "error" pretty much everywhere. If we end up changing
to use "status" then we should do that wholesale.
================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp:322
+ if (status.Success()) {
+ SetPrivateState(eStateRunning);
+ m_async_broadcaster.BroadcastEvent(eBroadcastBitAsyncContinue);
----------------
This seems asymmetric, why do you use the async thread to generate the stopped
event, but not the running event?
================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp:327
+ } else {
+ status.SetErrorString("thread is suspended");
+ }
----------------
Presumably the Interface knew why Resume failed if it did. Don't erase that
information in the error you return here.
================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp:336
+bool ScriptedProcess::IsAlive() {
+ if (!m_interpreter)
+ return false;
----------------
You checked both m_interpreter & m_script_object_sp above when checking for
validity. Do you need to check both here as well.
================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp:342
+
+size_t ScriptedProcess::ReadMemory(lldb::addr_t addr, void *buf, size_t size,
+ Status &error) {
----------------
The comment above Process::ReadMemory says processes plugins should not
override the method. Why do you need to do this here? If there is a reason,
it should be explained.
================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp:388
+
+ if (!m_interpreter) {
+ error.SetErrorString("No interpreter.");
----------------
You are inconsistent about what you check for "We didn't manage to make the
scripted process at all". Sometimes you just check for the interpreter and
sometimes you check that and the m_script_object_sp...
Maybe you should put those checks into a ScriptedProcess method so you do it
the same way everywhere.
================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp:410
+ ThreadList &new_thread_list) {
+ return new_thread_list.GetSize(false) > 0;
+}
----------------
Presumably this is just unimplemented? This is supposed to get the current set
of threads, if any of them are in old_thread_list then they get copied to
new_thread_list, and then any actually new threads will get added to
new_thread_list.
================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.h:23
+protected:
+ class LaunchInfo {
+ public:
----------------
I'm not sure LaunchInfo is the right name for this class. While it does get
constructed by grabbing a couple of fields out of the ProcessLaunchInfo, it's
actually used just to construct the ScriptedProcess so it isn't directly about
launching the scripted process. Since you do a separate Launch operation, and
you pass that a different entity also called LaunchInfo, I think this name is
confusing.
================
Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.h:118
+ enum {
+ eBroadcastBitAsyncContinue = (1 << 0),
+ eBroadcastBitAsyncThreadShouldExit = (1 << 1),
----------------
The name of this bit is odd. It instructs the Async Thread to switch the
private process state to eStateStopped. So AsyncContinue is a confusing name.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D100384/new/
https://reviews.llvm.org/D100384
_______________________________________________
lldb-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
