[lldb] Move ScriptedProcess private state update to implementation

[mib]

Summary

While debugging a Scripted Process, in order to update its state and
work nicely with lldb's execution model, it needs to toggle its private
state from running to stopped, which will result in broadcasting a
process state changed event to the debugger listener.

Originally, this state update was done systematically in the Scripted
Process C++ plugin, however in order to make scripted process
interactive, we need to be able to update their state dynamically.

This patch makes use of the recent addition of the
SBProcess::ForceScriptedState to programatically, and moves the
process private state update to the python implementation of the resume
method instead of doing it in ScriptedProcess::DoResume.

This patch also removes the unused ShouldStop & Stop scripted
process APIs, and adds new ScriptedInterface transform methods for
boolean arguments. This allow the user to programmatically decide if
after running the process, we should stop it (which is the default setting).

Signed-off-by: Med Ismail Bennani <[e-mail]>

[mib]

Landed this by mistake, please take another look

[mib]

ping @JDevlieghere @bulbazord

[bulbazord]

lldb/examples/python/scripted_process/scripted_process.py

167–170

This seems incomplete?

lldb/source/Plugins/ScriptInterpreter/Python/ScriptedPythonInterface.h

151

Should we do something with the error here?

[mib]

Address @bulbazord comments.

[bulbazord]

No problem from me. Jonas?

[JDevlieghere]

LGTM

[dyung]

Hi @mib, your change is causing a build failure on two build bots, can you take a look and revert if you need time to investigate?

https://lab.llvm.org/buildbot/#/builders/217/builds/20430
https://lab.llvm.org/buildbot/#/builders/243/builds/5438

[mib]

In D145295#4297312, @dyung wrote:

Hi @mib, your change is causing a build failure on two build bots, can you take a look and revert if you need time to investigate?

https://lab.llvm.org/buildbot/#/builders/217/builds/20430
https://lab.llvm.org/buildbot/#/builders/243/builds/5438

Yep, I have a fix already. I'll push it in a few minutes. Thanks for the follow-up @dyung.

[mib]

In D145295#4297322, @mib wrote:

In D145295#4297312, @dyung wrote:

Hi @mib, your change is causing a build failure on two build bots, can you take a look and revert if you need time to investigate?

https://lab.llvm.org/buildbot/#/builders/217/builds/20430
https://lab.llvm.org/buildbot/#/builders/243/builds/5438

Yep, I have a fix already. I'll push it in a few minutes. Thanks for the follow-up @dyung.

@dyung D149218 should fix the build failure.

[dyung]

In D145295#4297347, @mib wrote:

In D145295#4297322, @mib wrote:

In D145295#4297312, @dyung wrote:

Hi @mib, your change is causing a build failure on two build bots, can you take a look and revert if you need time to investigate?

https://lab.llvm.org/buildbot/#/builders/217/builds/20430
https://lab.llvm.org/buildbot/#/builders/243/builds/5438

Yep, I have a fix already. I'll push it in a few minutes. Thanks for the follow-up @dyung.

@dyung D149218 should fix the build failure.

It does, thanks!

lldb/examples/python/scripted_process/scripted_process.py

def attach(self, attach_info):
             attach_info (lldb.SBAttachInfo): The information related to the
             process we're attaching to.
 
         Returns:
             lldb.SBError: An `lldb.SBError` with error code 0.
         """
         return lldb.SBError()
 
-    def resume(self):
+    def resume(self, should_stop=True):
         """ Simulate the scripted process resume.
 
-        Returns:
-            lldb.SBError: An `lldb.SBError` with error code 0.
-        """
-        return lldb.SBError()
-
-    @abstractmethod
-    def should_stop(self):
-        """ Check if the scripted process plugin should produce the stop event.
-
-        Returns:
-            bool: True if scripted process should broadcast a stop event.
-                  False otherwise.
-        """
-        pass
-
-    def stop(self):
-        """ Trigger the scripted process stop.
+        Args:
+            should_stop (bool): If True, resume will also force the process
+            state to stopped after running it.
 
         Returns:
             lldb.SBError: An `lldb.SBError` with error code 0.
         """
+        process = self.target.GetProcess()
+        if not process:
+            error = lldb.SBError()
+            error.SetErrorString("Invalid process.")
+            return error
+
+        process.ForceScriptedState(lldb.eStateRunning);
+        if (should_stop):
+            process.ForceScriptedState(lldb.eStateStopped);
         return lldb.SBError()
 
     @abstractmethod
     def is_alive(self):
         """ Check if the scripted process is alive.
 
         Returns:
             bool: True if scripted process is alive. False otherwise.

lldb/include/lldb/Interpreter/ScriptedProcessInterface.h

public:
   virtual Status Attach(const ProcessAttachInfo &attach_info) {
     return Status("ScriptedProcess did not attach");
   }
 
   virtual Status Launch() { return Status("ScriptedProcess did not launch"); }
 
   virtual Status Resume() { return Status("ScriptedProcess did not resume"); }
 
-  virtual bool ShouldStop() { return true; }
-
-  virtual Status Stop() { return Status("ScriptedProcess did not stop"); }
-
   virtual std::optional<MemoryRegionInfo>
   GetMemoryRegionContainingAddress(lldb::addr_t address, Status &error) {
     error.SetErrorString("ScriptedProcess have no memory region.");
     return {};
   }
 
   virtual StructuredData::DictionarySP GetThreadsInfo() { return {}; }
 

lldb/source/Plugins/Process/scripted/ScriptedProcess.h

public:
   void ForceScriptedState(lldb::StateType state) override {
     SetPrivateState(state);
   }
 
 protected:
   ScriptedProcess(lldb::TargetSP target_sp, lldb::ListenerSP listener_sp,
                   const ScriptedMetadata &scripted_metadata, Status &error);
 
-  Status DoStop();
-
   void Clear();
 
   bool DoUpdateThreadList(ThreadList &old_thread_list,
                           ThreadList &new_thread_list) override;
 
   Status DoGetMemoryRegionInfo(lldb::addr_t load_addr,
                                MemoryRegionInfo &range_info) override;
 

lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp

 Status ScriptedProcess::DoLoadCore() {
   ProcessLaunchInfo launch_info = GetTarget().GetProcessLaunchInfo();
 
   return DoLaunch(nullptr, launch_info);
 }
 
 Status ScriptedProcess::DoLaunch(Module *exe_module,
                                  ProcessLaunchInfo &launch_info) {
-  /* FIXME: This doesn't reflect how lldb actually launches a process.
-           In reality, it attaches to debugserver, then resume the process. */
-  Status error = GetInterface().Launch();
-  SetPrivateState(eStateRunning);
-
-  if (error.Fail())
-    return error;
-
-  // TODO: Fetch next state from stopped event queue then send stop event
-  //  const StateType state = SetThreadStopInfo(response);
-  //  if (state != eStateInvalid) {
-  //    SetPrivateState(state);
+  LLDB_LOGF(GetLog(LLDBLog::Process), "ScriptedProcess::%s launching process", __FUNCTION__);
   
+  /* MARK: This doesn't reflect how lldb actually launches a process.
+           In reality, it attaches to debugserver, then resume the process.
+           That's not true in all cases.  If debugserver is remote, lldb
+           asks debugserver to launch the process for it. */
+  Status error = GetInterface().Launch();
   SetPrivateState(eStateStopped);
-
-  return {};
+  return error;
 }
 
 void ScriptedProcess::DidLaunch() { m_pid = GetInterface().GetProcessID(); }
 
 void ScriptedProcess::DidResume() {
   // Update the PID again, in case the user provided a placeholder pid at launch
   m_pid = GetInterface().GetProcessID();
   GetLoadedDynamicLibrariesInfos();
 }
 
 Status ScriptedProcess::DoResume() {
-  Log *log = GetLog(LLDBLog::Process);
-  // FIXME: Fetch data from thread.
-  const StateType thread_resume_state = eStateRunning;
-  LLDB_LOGF(log, "ScriptedProcess::%s thread_resume_state = %s", __FUNCTION__,
-            StateAsCString(thread_resume_state));
-
-  bool resume = (thread_resume_state == eStateRunning);
-  assert(thread_resume_state == eStateRunning && "invalid thread resume state");
-
-  Status error;
-  if (resume) {
-    LLDB_LOGF(log, "ScriptedProcess::%s sending resume", __FUNCTION__);
+  LLDB_LOGF(GetLog(LLDBLog::Process), "ScriptedProcess::%s resuming process", __FUNCTION__);
 
-    SetPrivateState(eStateRunning);
-    SetPrivateState(eStateStopped);
-    error = GetInterface().Resume();
-  }
-
-  return error;
+  return GetInterface().Resume();
 }
 
 Status ScriptedProcess::DoAttach(const ProcessAttachInfo &attach_info) {
   Status error = GetInterface().Attach(attach_info);
   SetPrivateState(eStateRunning);
   SetPrivateState(eStateStopped);
   if (error.Fail())
     return error;
Status ScriptedProcess::DoAttachToProcessWithName(
     const char *process_name, const ProcessAttachInfo &attach_info) {
   return DoAttach(attach_info);
 }
 
 void ScriptedProcess::DidAttach(ArchSpec &process_arch) {
   process_arch = GetArchitecture();
 }
 
-Status ScriptedProcess::DoStop() {
-  Log *log = GetLog(LLDBLog::Process);
-
-  if (GetInterface().ShouldStop()) {
-    SetPrivateState(eStateStopped);
-    LLDB_LOGF(log, "ScriptedProcess::%s Immediate stop", __FUNCTION__);
-    return {};
-  }
-
-  LLDB_LOGF(log, "ScriptedProcess::%s Delayed stop", __FUNCTION__);
-  return GetInterface().Stop();
-}
-
 Status ScriptedProcess::DoDestroy() { return Status(); }
 
 bool ScriptedProcess::IsAlive() { return GetInterface().IsAlive(); }
 
 size_t ScriptedProcess::DoReadMemory(lldb::addr_t addr, void *buf, size_t size,
                                      Status &error) {
   lldb::DataExtractorSP data_extractor_sp =
       GetInterface().ReadMemoryAtAddress(addr, size, error);

lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h

 template <> struct PythonFormat<char *> : PassthroughFormat<char *, 's'> {};
 template <> struct PythonFormat<char> : PassthroughFormat<char, 'b'> {};
 template <>
 struct PythonFormat<unsigned char> : PassthroughFormat<unsigned char, 'B'> {};
 template <> struct PythonFormat<short> : PassthroughFormat<short, 'h'> {};
 template <>
 struct PythonFormat<unsigned short> : PassthroughFormat<unsigned short, 'H'> {};
 template <> struct PythonFormat<int> : PassthroughFormat<int, 'i'> {};
+template <> struct PythonFormat<bool> : PassthroughFormat<bool, 'p'> {};
 template <>
 struct PythonFormat<unsigned int> : PassthroughFormat<unsigned int, 'I'> {};
 template <> struct PythonFormat<long> : PassthroughFormat<long, 'l'> {};
 template <>
 struct PythonFormat<unsigned long> : PassthroughFormat<unsigned long, 'k'> {};
 template <>
 struct PythonFormat<long long> : PassthroughFormat<long long, 'L'> {};
 template <>

lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.h

public:
   StructuredData::DictionarySP GetCapabilities() override;
 
   Status Attach(const ProcessAttachInfo &attach_info) override;
 
   Status Launch() override;
 
   Status Resume() override;
 
-  bool ShouldStop() override;
-
-  Status Stop() override;
-
   std::optional<MemoryRegionInfo>
   GetMemoryRegionContainingAddress(lldb::addr_t address,
                                    Status &error) override;
 
   StructuredData::DictionarySP GetThreadsInfo() override;
 
   lldb::DataExtractorSP ReadMemoryAtAddress(lldb::addr_t address, size_t size,
                                             Status &error) override;

lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp

ScriptedProcessPythonInterface::Attach(const ProcessAttachInfo &attach_info) {
   return GetStatusFromMethod("attach", attach_info_sp);
 }
 
 Status ScriptedProcessPythonInterface::Launch() {
   return GetStatusFromMethod("launch");
 }
 
 Status ScriptedProcessPythonInterface::Resume() {
-  return GetStatusFromMethod("resume");
-}
-
-bool ScriptedProcessPythonInterface::ShouldStop() {
-  Status error;
-  StructuredData::ObjectSP obj = Dispatch("is_alive", error);
-
-  if (!CheckStructuredDataObject(LLVM_PRETTY_FUNCTION, obj, error))
-    return {};
-
-  return obj->GetBooleanValue();
-}
-
-Status ScriptedProcessPythonInterface::Stop() {
-  return GetStatusFromMethod("stop");
+  // When calling ScriptedProcess.Resume from lldb we should always stop.
+  return GetStatusFromMethod("resume", /*should_stop=*/true);
 }
 
 std::optional<MemoryRegionInfo>
 ScriptedProcessPythonInterface::GetMemoryRegionContainingAddress(
     lldb::addr_t address, Status &error) {
   auto mem_region = Dispatch<std::optional<MemoryRegionInfo>>(
       "get_memory_region_containing_address", error, address);
 

lldb/source/Plugins/ScriptInterpreter/Python/ScriptedPythonInterface.h

Status GetStatusFromMethod(llvm::StringRef method_name, Args &&...args) {
     return error;
   }
 
   template <typename T> T Transform(T object) {
     // No Transformation for generic usage
     return {object};
   }
 
+  python::PythonObject Transform(bool arg) {
+    // Boolean arguments need to be turned into python objects.
+    return python::PythonBoolean(arg);
+  }
+
   python::PythonObject Transform(Status arg) {
     return python::ToSWIGWrapper(arg);
   }
 
   python::PythonObject Transform(lldb::ProcessAttachInfoSP arg) {
     return python::ToSWIGWrapper(arg);
   }
 
protected:
   }
 
   template <typename T>
   void ReverseTransform(T &original_arg, python::PythonObject transformed_arg,
                         Status &error) {
     original_arg = ExtractValueFromPythonObject<T>(transformed_arg, error);
   }
 
+  template <>
+  void ReverseTransform(bool &original_arg,
+                        python::PythonObject transformed_arg, Status &error) {
+    python::PythonBoolean boolean_arg = python::PythonBoolean(
+        python::PyRefType::Borrowed, transformed_arg.get());
+    if (boolean_arg.IsValid())
+      original_arg = boolean_arg.GetValue();
+    else
+      error.SetErrorString(
+          llvm::formatv("{}: Invalid boolean argument.", LLVM_PRETTY_FUNCTION)
+              .str());
+  }
+
   template <std::size_t... I, typename... Args>
   auto TransformTuple(const std::tuple<Args...> &args,
                       std::index_sequence<I...>) {
     return std::make_tuple(Transform(std::get<I>(args))...);
   }
 
   // This will iterate over the Dispatch parameter pack and replace in-place
   // every `lldb_private` argument that has a SB counterpart.