[Lldb-commits] [PATCH] D100384: [lldb/Plugins] Update ScriptedProcess Process Plugin

[Med Ismail Bennani via Phabricator via lldb-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG312b43da0500: [lldb/Plugins] Add ScriptedProcess Process 
Plugin (authored by mib).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100384

Files:
  lldb/examples/python/scripted_process/my_scripted_process.py
  lldb/examples/python/scripted_process/scripted_process.py
  lldb/include/lldb/Interpreter/ScriptInterpreter.h
  lldb/include/lldb/Interpreter/ScriptedProcessInterface.h
  lldb/include/lldb/Target/Process.h
  lldb/source/Plugins/Process/CMakeLists.txt
  lldb/source/Plugins/Process/scripted/CMakeLists.txt
  lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
  lldb/source/Plugins/Process/scripted/ScriptedProcess.h
  
lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.h
  lldb/source/Target/Target.cpp
  lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py

Index: lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py
===
--- lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py
+++ lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py
@@ -11,7 +11,7 @@
 from lldbsuite.test import lldbtest
 
 
-class PlatformProcessCrashInfoTestCase(TestBase):
+class ScriptedProcesTestCase(TestBase):
 
 mydir = TestBase.compute_mydir(__file__)
 
@@ -43,3 +43,55 @@
 self.expect('script dir(ScriptedProcess)',
 substrs=["launch"])
 
+def test_launch_scripted_process_sbapi(self):
+"""Test that we can launch an lldb scripted process using the SBAPI,
+check its process ID and read string from memory."""
+self.build()
+target = self.dbg.CreateTarget(self.getBuildArtifact("a.out"))
+self.assertTrue(target, VALID_TARGET)
+
+scripted_process_example_relpath = ['..','..','..','..','examples','python','scripted_process','my_scripted_process.py']
+os.environ['SKIP_SCRIPTED_PROCESS_LAUNCH'] = '1'
+self.runCmd("command script import " + os.path.join(self.getSourceDir(),
+*scripted_process_example_relpath))
+
+launch_info = lldb.SBLaunchInfo(None)
+launch_info.SetProcessPluginName("ScriptedProcess")
+launch_info.SetScriptedProcessClassName("my_scripted_process.MyScriptedProcess")
+
+error = lldb.SBError()
+process = target.Launch(launch_info, error)
+self.assertTrue(process and process.IsValid(), PROCESS_IS_VALID)
+self.assertEqual(process.GetProcessID(), 42)
+
+hello_world = "Hello, world!"
+memory_read = process.ReadCStringFromMemory(0x500,
+len(hello_world) + 1, # NULL byte
+error)
+
+self.assertTrue(error.Success(), "Failed to read memory from scripted process.")
+self.assertEqual(hello_world, memory_read)
+
+def test_launch_scripted_process_cli(self):
+"""Test that we can launch an lldb scripted process from the command
+line, check its process ID and read string from memory."""
+self.build()
+target = self.dbg.CreateTarget(self.getBuildArtifact("a.out"))
+self.assertTrue(target, VALID_TARGET)
+
+scripted_process_example_relpath = ['..','..','..','..','examples','python','scripted_process','my_scripted_process.py']
+self.runCmd("command script import " + os.path.join(self.getSourceDir(),
+*scripted_process_example_relpath))
+
+process = target.GetProcess()
+self.assertTrue(process, PROCESS_IS_VALID)
+self.assertEqual(process.GetProcessID(), 42)
+
+error = lldb.SBError()
+hello_world = "Hello, world!"
+memory_read = process.ReadCStringFromMemory(0x500,
+len(hello_world) + 1, # NULL byte
+error)
+
+self.assertTrue(error.Success(), "Failed to read memory from scripted process.")
+self.assertEqual(hello_world, memory_read)
Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -2974,7 +2974,7 @@
   // If we're not already connected to the process, and if we have a platform
   // that can launch a process for debugging, go ahead and do that here.
   if (state != eStateConnected && platform_sp &&
-  platform_sp->CanDebugProcess()) {
+  platform_sp->CanDebugProcess() && 

[Lldb-commits] [PATCH] D100384: [lldb/Plugins] Update ScriptedProcess Process Plugin

[Med Ismail Bennani via Phabricator via lldb-commits]

mib updated this revision to Diff 360779.
mib added a comment.

Fix `ScriptedProcess::IsAlive` crash during process destruction


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100384

Files:
  lldb/examples/python/scripted_process/my_scripted_process.py
  lldb/examples/python/scripted_process/scripted_process.py
  lldb/include/lldb/Interpreter/ScriptInterpreter.h
  lldb/include/lldb/Interpreter/ScriptedProcessInterface.h
  lldb/include/lldb/Target/Process.h
  lldb/source/Plugins/Process/CMakeLists.txt
  lldb/source/Plugins/Process/scripted/CMakeLists.txt
  lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
  lldb/source/Plugins/Process/scripted/ScriptedProcess.h
  
lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.h
  lldb/source/Target/Target.cpp
  lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py

Index: lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py
===
--- lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py
+++ lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py
@@ -11,7 +11,7 @@
 from lldbsuite.test import lldbtest
 
 
-class PlatformProcessCrashInfoTestCase(TestBase):
+class ScriptedProcesTestCase(TestBase):
 
 mydir = TestBase.compute_mydir(__file__)
 
@@ -43,3 +43,55 @@
 self.expect('script dir(ScriptedProcess)',
 substrs=["launch"])
 
+def test_launch_scripted_process_sbapi(self):
+"""Test that we can launch an lldb scripted process using the SBAPI,
+check its process ID and read string from memory."""
+self.build()
+target = self.dbg.CreateTarget(self.getBuildArtifact("a.out"))
+self.assertTrue(target, VALID_TARGET)
+
+scripted_process_example_relpath = ['..','..','..','..','examples','python','scripted_process','my_scripted_process.py']
+os.environ['SKIP_SCRIPTED_PROCESS_LAUNCH'] = '1'
+self.runCmd("command script import " + os.path.join(self.getSourceDir(),
+*scripted_process_example_relpath))
+
+launch_info = lldb.SBLaunchInfo(None)
+launch_info.SetProcessPluginName("ScriptedProcess")
+launch_info.SetScriptedProcessClassName("my_scripted_process.MyScriptedProcess")
+
+error = lldb.SBError()
+process = target.Launch(launch_info, error)
+self.assertTrue(process and process.IsValid(), PROCESS_IS_VALID)
+self.assertEqual(process.GetProcessID(), 42)
+
+hello_world = "Hello, world!"
+memory_read = process.ReadCStringFromMemory(0x500,
+len(hello_world) + 1, # NULL byte
+error)
+
+self.assertTrue(error.Success(), "Failed to read memory from scripted process.")
+self.assertEqual(hello_world, memory_read)
+
+def test_launch_scripted_process_cli(self):
+"""Test that we can launch an lldb scripted process from the command
+line, check its process ID and read string from memory."""
+self.build()
+target = self.dbg.CreateTarget(self.getBuildArtifact("a.out"))
+self.assertTrue(target, VALID_TARGET)
+
+scripted_process_example_relpath = ['..','..','..','..','examples','python','scripted_process','my_scripted_process.py']
+self.runCmd("command script import " + os.path.join(self.getSourceDir(),
+*scripted_process_example_relpath))
+
+process = target.GetProcess()
+self.assertTrue(process, PROCESS_IS_VALID)
+self.assertEqual(process.GetProcessID(), 42)
+
+error = lldb.SBError()
+hello_world = "Hello, world!"
+memory_read = process.ReadCStringFromMemory(0x500,
+len(hello_world) + 1, # NULL byte
+error)
+
+self.assertTrue(error.Success(), "Failed to read memory from scripted process.")
+self.assertEqual(hello_world, memory_read)
Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -2974,7 +2974,7 @@
   // If we're not already connected to the process, and if we have a platform
   // that can launch a process for debugging, go ahead and do that here.
   if (state != eStateConnected && platform_sp &&
-  platform_sp->CanDebugProcess()) {
+  platform_sp->CanDebugProcess() && !launch_info.IsScriptedProcess()) {
 LLDB_LOGF(log, "Target::%s asking the platform to debug the process",
   

[Lldb-commits] [PATCH] D100384: [lldb/Plugins] Update ScriptedProcess Process Plugin

[Med Ismail Bennani via Phabricator via lldb-commits]

mib updated this revision to Diff 360074.
mib edited the summary of this revision.
mib added a comment.

Address @jingham feedbacks:

- Make `ScriptedProcess` plugin language agnostic
- Simplify execution control by removing `Listener-Broadcaster` model
- Solve non-deterministic hangs following D105698 

- Fix `memory read` bug.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100384

Files:
  lldb/examples/python/scripted_process/my_scripted_process.py
  lldb/examples/python/scripted_process/scripted_process.py
  lldb/include/lldb/Interpreter/ScriptInterpreter.h
  lldb/include/lldb/Interpreter/ScriptedProcessInterface.h
  lldb/include/lldb/Target/Process.h
  lldb/source/Plugins/Process/CMakeLists.txt
  lldb/source/Plugins/Process/scripted/CMakeLists.txt
  lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
  lldb/source/Plugins/Process/scripted/ScriptedProcess.h
  
lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp
  lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.h
  lldb/source/Target/Target.cpp
  lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py

Index: lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py
===
--- lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py
+++ lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py
@@ -11,7 +11,7 @@
 from lldbsuite.test import lldbtest
 
 
-class PlatformProcessCrashInfoTestCase(TestBase):
+class ScriptedProcesTestCase(TestBase):
 
 mydir = TestBase.compute_mydir(__file__)
 
@@ -43,3 +43,55 @@
 self.expect('script dir(ScriptedProcess)',
 substrs=["launch"])
 
+def test_launch_scripted_process_sbapi(self):
+"""Test that we can launch an lldb scripted process using the SBAPI,
+check its process ID and read string from memory."""
+self.build()
+target = self.dbg.CreateTarget(self.getBuildArtifact("a.out"))
+self.assertTrue(target, VALID_TARGET)
+
+scripted_process_example_relpath = ['..','..','..','..','examples','python','scripted_process','my_scripted_process.py']
+os.environ['SKIP_SCRIPTED_PROCESS_LAUNCH'] = '1'
+self.runCmd("command script import " + os.path.join(self.getSourceDir(),
+*scripted_process_example_relpath))
+
+launch_info = lldb.SBLaunchInfo(None)
+launch_info.SetProcessPluginName("ScriptedProcess")
+launch_info.SetScriptedProcessClassName("my_scripted_process.MyScriptedProcess")
+
+error = lldb.SBError()
+process = target.Launch(launch_info, error)
+self.assertTrue(process and process.IsValid(), PROCESS_IS_VALID)
+self.assertEqual(process.GetProcessID(), 42)
+
+hello_world = "Hello, world!"
+memory_read = process.ReadCStringFromMemory(0x500,
+len(hello_world) + 1, # NULL byte
+error)
+
+self.assertTrue(error.Success(), "Failed to read memory from scripted process.")
+self.assertEqual(hello_world, memory_read)
+
+def test_launch_scripted_process_cli(self):
+"""Test that we can launch an lldb scripted process from the command
+line, check its process ID and read string from memory."""
+self.build()
+target = self.dbg.CreateTarget(self.getBuildArtifact("a.out"))
+self.assertTrue(target, VALID_TARGET)
+
+scripted_process_example_relpath = ['..','..','..','..','examples','python','scripted_process','my_scripted_process.py']
+self.runCmd("command script import " + os.path.join(self.getSourceDir(),
+*scripted_process_example_relpath))
+
+process = target.GetProcess()
+self.assertTrue(process, PROCESS_IS_VALID)
+self.assertEqual(process.GetProcessID(), 42)
+
+error = lldb.SBError()
+hello_world = "Hello, world!"
+memory_read = process.ReadCStringFromMemory(0x500,
+len(hello_world) + 1, # NULL byte
+error)
+
+self.assertTrue(error.Success(), "Failed to read memory from scripted process.")
+self.assertEqual(hello_world, memory_read)
Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -2974,7 +2974,7 @@
   // If we're not already connected to the process, and if we have a platform
   // that can launch a process for debugging, go ahead and do that here.
   if (state != 

[Lldb-commits] [PATCH] D100384: [lldb/Plugins] Update ScriptedProcess Process Plugin

[Jim Ingham via Phabricator via lldb-commits]

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 ) {

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 

[Lldb-commits] [PATCH] D100384: [lldb/Plugins] Update ScriptedProcess Process Plugin

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere accepted this revision.
JDevlieghere added a comment.
This revision is now accepted and ready to land.

LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100384

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D100384: [lldb/Plugins] Update ScriptedProcess Process Plugin

[Med Ismail Bennani via Phabricator via lldb-commits]

mib updated this revision to Diff 337549.
mib added a comment.

Use Doxygen group comment for ScriptedProcess members.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100384

Files:
  lldb/include/lldb/Interpreter/ScriptInterpreter.h
  lldb/include/lldb/Target/Process.h
  lldb/source/Plugins/Process/CMakeLists.txt
  lldb/source/Plugins/Process/scripted/CMakeLists.txt
  lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
  lldb/source/Plugins/Process/scripted/ScriptedProcess.h
  lldb/source/Target/Target.cpp
  lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py

Index: lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py
===
--- lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py
+++ lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py
@@ -11,7 +11,7 @@
 from lldbsuite.test import lldbtest
 
 
-class PlatformProcessCrashInfoTestCase(TestBase):
+class ScriptedProcesTestCase(TestBase):
 
 mydir = TestBase.compute_mydir(__file__)
 
@@ -43,3 +43,55 @@
 self.expect('script dir(ScriptedProcess)',
 substrs=["launch"])
 
+def test_launch_scripted_process_sbapi(self):
+"""Test that we can launch an lldb scripted process using the SBAPI,
+check its process ID and read string from memory."""
+self.build()
+target = self.dbg.CreateTarget(self.getBuildArtifact("a.out"))
+self.assertTrue(target, VALID_TARGET)
+
+scripted_process_example_relpath = ['..','..','..','..','examples','python','scripted_process','my_scripted_process.py']
+os.environ['SKIP_SCRIPTED_PROCESS_LAUNCH'] = '1'
+self.runCmd("command script import " + os.path.join(self.getSourceDir(),
+*scripted_process_example_relpath))
+
+launch_info = lldb.SBLaunchInfo(None)
+launch_info.SetProcessPluginName("ScriptedProcess")
+launch_info.SetScriptedProcessClassName("my_scripted_process.MyScriptedProcess")
+
+error = lldb.SBError()
+process = target.Launch(launch_info, error)
+self.assertTrue(process and process.IsValid(), PROCESS_IS_VALID)
+self.assertEqual(process.GetProcessID(), 42)
+
+hello_world = "Hello, world!"
+memory_read = process.ReadCStringFromMemory(0x500,
+len(hello_world) + 1, # NULL byte
+error)
+
+self.assertTrue(error.Success(), "Failed to read memory from scripted process.")
+self.assertEqual(hello_world, memory_read)
+
+def test_launch_scripted_process_cli(self):
+"""Test that we can launch an lldb scripted process from the command
+line, check its process ID and read string from memory."""
+self.build()
+target = self.dbg.CreateTarget(self.getBuildArtifact("a.out"))
+self.assertTrue(target, VALID_TARGET)
+
+scripted_process_example_relpath = ['..','..','..','..','examples','python','scripted_process','my_scripted_process.py']
+self.runCmd("command script import " + os.path.join(self.getSourceDir(),
+*scripted_process_example_relpath))
+
+process = target.GetProcess()
+self.assertTrue(process, PROCESS_IS_VALID)
+self.assertEqual(process.GetProcessID(), 42)
+
+error = lldb.SBError()
+hello_world = "Hello, world!"
+memory_read = process.ReadCStringFromMemory(0x500,
+len(hello_world) + 1, # NULL byte
+error)
+
+self.assertTrue(error.Success(), "Failed to read memory from scripted process.")
+self.assertEqual(hello_world, memory_read)
Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -2972,7 +2972,7 @@
   // If we're not already connected to the process, and if we have a platform
   // that can launch a process for debugging, go ahead and do that here.
   if (state != eStateConnected && platform_sp &&
-  platform_sp->CanDebugProcess()) {
+  platform_sp->CanDebugProcess() && !launch_info.IsScriptedProcess()) {
 LLDB_LOGF(log, "Target::%s asking the platform to debug the process",
   __FUNCTION__);
 
Index: lldb/source/Plugins/Process/scripted/ScriptedProcess.h
===
--- /dev/null
+++ lldb/source/Plugins/Process/scripted/ScriptedProcess.h
@@ -0,0 +1,146 @@
+//===-- ScriptedProcess.h - -*- C++ -*-===//
+//
+// Part of the LLVM Project, under the 

[Lldb-commits] [PATCH] D100384: [lldb/Plugins] Update ScriptedProcess Process Plugin

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.h:129-134
+  Broadcaster m_async_broadcaster; ///< The broadcaster emitting process/thread
+   /// state change events.
+  lldb::ListenerSP m_async_listener_sp; ///< The listener waiting for
+/// process/thread state change events.
+  HostThread m_async_thread; ///< The separate lldb thread that monitors 
events.
+  std::recursive_mutex m_async_thread_state_mutex;

The trailing comments are a real pain with the 80 col limit and we're actively 
moving away from them. Please use `///` above the line instead or consider 
documenting the whole thing with a comment group. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100384

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D100384: [lldb/Plugins] Update ScriptedProcess Process Plugin

[Med Ismail Bennani via Phabricator via lldb-commits]

mib updated this revision to Diff 337226.
mib added a comment.

Address @JDevlieghere feedbacks


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100384

Files:
  lldb/include/lldb/Interpreter/ScriptInterpreter.h
  lldb/include/lldb/Target/Process.h
  lldb/source/Plugins/Process/CMakeLists.txt
  lldb/source/Plugins/Process/scripted/CMakeLists.txt
  lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
  lldb/source/Plugins/Process/scripted/ScriptedProcess.h
  lldb/source/Target/Target.cpp
  lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py

Index: lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py
===
--- lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py
+++ lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py
@@ -11,7 +11,7 @@
 from lldbsuite.test import lldbtest
 
 
-class PlatformProcessCrashInfoTestCase(TestBase):
+class ScriptedProcesTestCase(TestBase):
 
 mydir = TestBase.compute_mydir(__file__)
 
@@ -43,3 +43,55 @@
 self.expect('script dir(ScriptedProcess)',
 substrs=["launch"])
 
+def test_launch_scripted_process_sbapi(self):
+"""Test that we can launch an lldb scripted process using the SBAPI,
+check its process ID and read string from memory."""
+self.build()
+target = self.dbg.CreateTarget(self.getBuildArtifact("a.out"))
+self.assertTrue(target, VALID_TARGET)
+
+scripted_process_example_relpath = ['..','..','..','..','examples','python','scripted_process','my_scripted_process.py']
+os.environ['SKIP_SCRIPTED_PROCESS_LAUNCH'] = '1'
+self.runCmd("command script import " + os.path.join(self.getSourceDir(),
+*scripted_process_example_relpath))
+
+launch_info = lldb.SBLaunchInfo(None)
+launch_info.SetProcessPluginName("ScriptedProcess")
+launch_info.SetScriptedProcessClassName("my_scripted_process.MyScriptedProcess")
+
+error = lldb.SBError()
+process = target.Launch(launch_info, error)
+self.assertTrue(process and process.IsValid(), PROCESS_IS_VALID)
+self.assertEqual(process.GetProcessID(), 42)
+
+hello_world = "Hello, world!"
+memory_read = process.ReadCStringFromMemory(0x500,
+len(hello_world) + 1, # NULL byte
+error)
+
+self.assertTrue(error.Success(), "Failed to read memory from scripted process.")
+self.assertEqual(hello_world, memory_read)
+
+def test_launch_scripted_process_cli(self):
+"""Test that we can launch an lldb scripted process from the command
+line, check its process ID and read string from memory."""
+self.build()
+target = self.dbg.CreateTarget(self.getBuildArtifact("a.out"))
+self.assertTrue(target, VALID_TARGET)
+
+scripted_process_example_relpath = ['..','..','..','..','examples','python','scripted_process','my_scripted_process.py']
+self.runCmd("command script import " + os.path.join(self.getSourceDir(),
+*scripted_process_example_relpath))
+
+process = target.GetProcess()
+self.assertTrue(process, PROCESS_IS_VALID)
+self.assertEqual(process.GetProcessID(), 42)
+
+error = lldb.SBError()
+hello_world = "Hello, world!"
+memory_read = process.ReadCStringFromMemory(0x500,
+len(hello_world) + 1, # NULL byte
+error)
+
+self.assertTrue(error.Success(), "Failed to read memory from scripted process.")
+self.assertEqual(hello_world, memory_read)
Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -2972,7 +2972,7 @@
   // If we're not already connected to the process, and if we have a platform
   // that can launch a process for debugging, go ahead and do that here.
   if (state != eStateConnected && platform_sp &&
-  platform_sp->CanDebugProcess()) {
+  platform_sp->CanDebugProcess() && !launch_info.IsScriptedProcess()) {
 LLDB_LOGF(log, "Target::%s asking the platform to debug the process",
   __FUNCTION__);
 
Index: lldb/source/Plugins/Process/scripted/ScriptedProcess.h
===
--- /dev/null
+++ lldb/source/Plugins/Process/scripted/ScriptedProcess.h
@@ -0,0 +1,139 @@
+//===-- ScriptedProcess.h - -*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 

[Lldb-commits] [PATCH] D100384: [lldb/Plugins] Update ScriptedProcess Process Plugin

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp:93
+
+  Log *log(GetLogIfAllCategoriesSet(LIBLLDB_LOG_PROCESS));
+

You can move this into the `if` on line 98, or even just inline it. 



Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp:100-103
+LLDB_LOGF(
+log,
+"ScriptedProcess::%s failed to listen for m_async_broadcaster events",
+__FUNCTION__);





Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp:138
+  Log *log(GetLogIfAllCategoriesSet(LIBLLDB_LOG_PROCESS));
+
+  LLDB_LOGF(log, "ScriptedProcess::%s ()", __FUNCTION__);

I see this newline between `log` and `LLDB_LOGF` across this patch and while I 
don't mind (it's consistent) I'm curious why it's here. The next line is 
immediately using `log` and it other places in this patch where a variable is 
used directly after there is (usually) no newline.



Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp:205
+  bool is_running = false;
+  do {
+switch (event_type) {

Do we need this loop at all? It looks like nothing is ever setting `is_running` 
to true, so effectively this runs just the once? The two nested loops make it 
pretty hard to follow what the control flow is here. 



Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp:236
+}
+  } while (is_running); // do while
+} else {





Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp:316-345
+  bool resume = false;
+  Status status;
+
+  // FIXME: Fetch data from thread.
+  const StateType thread_resume_state = eStateRunning;
+
+  LLDB_LOGF(log, "ScriptedProcess::%s thread_resume_state = %s", __FUNCTION__,





Comment at: lldb/source/Plugins/Process/scripted/ScriptedProcess.h:128
+  lldb_private::StructuredData::ObjectSP m_script_object_sp = nullptr;
+  Broadcaster m_async_broadcaster;
+  lldb::ListenerSP m_async_listener_sp;

Can we add a Doxygen comment (group) that explains what these things are and 
what they're used for? 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100384

___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D100384: [lldb/Plugins] Update ScriptedProcess Process Plugin

[Med Ismail Bennani via Phabricator via lldb-commits]

mib updated this revision to Diff 337184.
mib added a comment.

Dead-code removal


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D100384

Files:
  lldb/include/lldb/Interpreter/ScriptInterpreter.h
  lldb/include/lldb/Target/Process.h
  lldb/source/Plugins/Process/CMakeLists.txt
  lldb/source/Plugins/Process/scripted/CMakeLists.txt
  lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
  lldb/source/Plugins/Process/scripted/ScriptedProcess.h
  lldb/source/Target/Target.cpp
  lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py

Index: lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py
===
--- lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py
+++ lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py
@@ -11,7 +11,7 @@
 from lldbsuite.test import lldbtest
 
 
-class PlatformProcessCrashInfoTestCase(TestBase):
+class ScriptedProcesTestCase(TestBase):
 
 mydir = TestBase.compute_mydir(__file__)
 
@@ -43,3 +43,55 @@
 self.expect('script dir(ScriptedProcess)',
 substrs=["launch"])
 
+def test_launch_scripted_process_sbapi(self):
+"""Test that we can launch an lldb scripted process using the SBAPI,
+check its process ID and read string from memory."""
+self.build()
+target = self.dbg.CreateTarget(self.getBuildArtifact("a.out"))
+self.assertTrue(target, VALID_TARGET)
+
+scripted_process_example_relpath = ['..','..','..','..','examples','python','scripted_process','my_scripted_process.py']
+os.environ['SKIP_SCRIPTED_PROCESS_LAUNCH'] = '1'
+self.runCmd("command script import " + os.path.join(self.getSourceDir(),
+*scripted_process_example_relpath))
+
+launch_info = lldb.SBLaunchInfo(None)
+launch_info.SetProcessPluginName("ScriptedProcess")
+launch_info.SetScriptedProcessClassName("my_scripted_process.MyScriptedProcess")
+
+error = lldb.SBError()
+process = target.Launch(launch_info, error)
+self.assertTrue(process and process.IsValid(), PROCESS_IS_VALID)
+self.assertEqual(process.GetProcessID(), 42)
+
+hello_world = "Hello, world!"
+memory_read = process.ReadCStringFromMemory(0x500,
+len(hello_world) + 1, # NULL byte
+error)
+
+self.assertTrue(error.Success(), "Failed to read memory from scripted process.")
+self.assertEqual(hello_world, memory_read)
+
+def test_launch_scripted_process_cli(self):
+"""Test that we can launch an lldb scripted process from the command
+line, check its process ID and read string from memory."""
+self.build()
+target = self.dbg.CreateTarget(self.getBuildArtifact("a.out"))
+self.assertTrue(target, VALID_TARGET)
+
+scripted_process_example_relpath = ['..','..','..','..','examples','python','scripted_process','my_scripted_process.py']
+self.runCmd("command script import " + os.path.join(self.getSourceDir(),
+*scripted_process_example_relpath))
+
+process = target.GetProcess()
+self.assertTrue(process, PROCESS_IS_VALID)
+self.assertEqual(process.GetProcessID(), 42)
+
+error = lldb.SBError()
+hello_world = "Hello, world!"
+memory_read = process.ReadCStringFromMemory(0x500,
+len(hello_world) + 1, # NULL byte
+error)
+
+self.assertTrue(error.Success(), "Failed to read memory from scripted process.")
+self.assertEqual(hello_world, memory_read)
Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -2972,7 +2972,7 @@
   // If we're not already connected to the process, and if we have a platform
   // that can launch a process for debugging, go ahead and do that here.
   if (state != eStateConnected && platform_sp &&
-  platform_sp->CanDebugProcess()) {
+  platform_sp->CanDebugProcess() && !launch_info.IsScriptedProcess()) {
 LLDB_LOGF(log, "Target::%s asking the platform to debug the process",
   __FUNCTION__);
 
Index: lldb/source/Plugins/Process/scripted/ScriptedProcess.h
===
--- /dev/null
+++ lldb/source/Plugins/Process/scripted/ScriptedProcess.h
@@ -0,0 +1,136 @@
+//===-- ScriptedProcess.h - -*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM 

[Lldb-commits] [PATCH] D100384: [lldb/Plugins] Update ScriptedProcess Process Plugin

[Med Ismail Bennani via Phabricator via lldb-commits]

mib created this revision.
mib added reviewers: JDevlieghere, jingham, jasonmolenda.
mib added a project: LLDB.
Herald added a subscriber: mgorny.
mib requested review of this revision.
Herald added a subscriber: lldb-commits.

This patch is an update of D95713  since the 
commit have been reverted.

It fixes the hangs, crashes and other asynchronous issues by adopting a 
`Listener-Broadcaster` model for Scripted Processes.

rdar://65508855

Signed-off-by: Med Ismail Bennani 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D100384

Files:
  lldb/include/lldb/Interpreter/ScriptInterpreter.h
  lldb/include/lldb/Target/Process.h
  lldb/source/Plugins/Process/CMakeLists.txt
  lldb/source/Plugins/Process/scripted/CMakeLists.txt
  lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
  lldb/source/Plugins/Process/scripted/ScriptedProcess.h
  lldb/source/Target/Target.cpp
  lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py

Index: lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py
===
--- lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py
+++ lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py
@@ -11,7 +11,7 @@
 from lldbsuite.test import lldbtest
 
 
-class PlatformProcessCrashInfoTestCase(TestBase):
+class ScriptedProcesTestCase(TestBase):
 
 mydir = TestBase.compute_mydir(__file__)
 
@@ -43,3 +43,55 @@
 self.expect('script dir(ScriptedProcess)',
 substrs=["launch"])
 
+def test_launch_scripted_process_sbapi(self):
+"""Test that we can launch an lldb scripted process using the SBAPI,
+check its process ID and read string from memory."""
+self.build()
+target = self.dbg.CreateTarget(self.getBuildArtifact("a.out"))
+self.assertTrue(target, VALID_TARGET)
+
+scripted_process_example_relpath = ['..','..','..','..','examples','python','scripted_process','my_scripted_process.py']
+os.environ['SKIP_SCRIPTED_PROCESS_LAUNCH'] = '1'
+self.runCmd("command script import " + os.path.join(self.getSourceDir(),
+*scripted_process_example_relpath))
+
+launch_info = lldb.SBLaunchInfo(None)
+launch_info.SetProcessPluginName("ScriptedProcess")
+launch_info.SetScriptedProcessClassName("my_scripted_process.MyScriptedProcess")
+
+error = lldb.SBError()
+process = target.Launch(launch_info, error)
+self.assertTrue(process and process.IsValid(), PROCESS_IS_VALID)
+self.assertEqual(process.GetProcessID(), 42)
+
+hello_world = "Hello, world!"
+memory_read = process.ReadCStringFromMemory(0x500,
+len(hello_world) + 1, # NULL byte
+error)
+
+self.assertTrue(error.Success(), "Failed to read memory from scripted process.")
+self.assertEqual(hello_world, memory_read)
+
+def test_launch_scripted_process_cli(self):
+"""Test that we can launch an lldb scripted process from the command
+line, check its process ID and read string from memory."""
+self.build()
+target = self.dbg.CreateTarget(self.getBuildArtifact("a.out"))
+self.assertTrue(target, VALID_TARGET)
+
+scripted_process_example_relpath = ['..','..','..','..','examples','python','scripted_process','my_scripted_process.py']
+self.runCmd("command script import " + os.path.join(self.getSourceDir(),
+*scripted_process_example_relpath))
+
+process = target.GetProcess()
+self.assertTrue(process, PROCESS_IS_VALID)
+self.assertEqual(process.GetProcessID(), 42)
+
+error = lldb.SBError()
+hello_world = "Hello, world!"
+memory_read = process.ReadCStringFromMemory(0x500,
+len(hello_world) + 1, # NULL byte
+error)
+
+self.assertTrue(error.Success(), "Failed to read memory from scripted process.")
+self.assertEqual(hello_world, memory_read)
Index: lldb/source/Target/Target.cpp
===
--- lldb/source/Target/Target.cpp
+++ lldb/source/Target/Target.cpp
@@ -2972,7 +2972,7 @@
   // If we're not already connected to the process, and if we have a platform
   // that can launch a process for debugging, go ahead and do that here.
   if (state != eStateConnected && platform_sp &&
-  platform_sp->CanDebugProcess()) {
+  platform_sp->CanDebugProcess() && !launch_info.IsScriptedProcess()) {
 LLDB_LOGF(log, "Target::%s asking the platform to debug the process",
   __FUNCTION__);
 
Index: