Page MenuHomePhabricator

[lldb/Plugins] Add ScriptedProcess Process Plugin
ClosedPublic

Authored by mib on Jan 29 2021, 5:21 PM.

Details

Summary

This patch introduces Scripted Processes to lldb.

The goal, here, is to be able to attach in the debugger to fake processes
that are backed by script files (in Python, Lua, Swift, etc ...) and
inspect them statically.

Scripted Processes can be used in cooperative multithreading environments
like the XNU Kernel or other real-time operating systems, but it can
also help us improve the debugger testing infrastructure by writting
synthetic tests that simulates hard-to-reproduce process/thread states.

Although ScriptedProcess is not feature-complete at the moment, it has
basic execution capabilities and will improve in the following patches.

rdar://65508855

Signed-off-by: Med Ismail Bennani <medismail.bennani@gmail.com>

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
mib requested review of this revision.Jan 29 2021, 5:21 PM
mib updated this revision to Diff 320248.Jan 29 2021, 5:26 PM

Remove dead code.

mib updated this revision to Diff 320249.Jan 29 2021, 5:29 PM

Fix typo in copyright header.

mib updated this revision to Diff 320287.Jan 29 2021, 11:55 PM
mib edited the summary of this revision. (Show Details)
  • Set Process PID in ScriptedProcess::DidLaunch
  • Updated ScriptedProcess::DoReadMemory implementation.
  • Fix typos.
JDevlieghere added inline comments.Feb 3 2021, 12:08 PM
lldb/source/Plugins/Process/CMakeLists.txt
15

nit: I'd make this lowercase for consistency with the other plugins (and keep Utility the exception as it's not a plugin).

lldb/source/Plugins/Process/Scripted/ScriptedProcess.cpp
1 ↗(On Diff #320287)

This is shorter than the closing ASCII art. Missing -s?

41 ↗(On Diff #320287)

Maybe qualify this as ScriptedProcess::LaunchInfo to make it clear you're not just making a copy but actually converting it.

This also answers my earlier question about who holds onto the LaunchInfo: nobody once this function returns. I would pass he process launch info to the ScriptedProcess and have the constructor instantiate the ScriptedProcess::LaunchInfo as its ivar.

43 ↗(On Diff #320287)

This will always return a valid ScriptedProcess which (IIRC) is the mechanism used by the plugin framework to communicate that the current plugin is the right one and able to handle the process. Shouldn't it only return a ScriptedProcess when the user has requested one? Does this work when you launch without -S?

159 ↗(On Diff #320287)

size_t

167 ↗(On Diff #320287)

size_t

174 ↗(On Diff #320287)

Seems like this is still outstanding ;-)

lldb/source/Plugins/Process/Scripted/ScriptedProcess.h
1 ↗(On Diff #320287)

Header needs the C++ thingy

33 ↗(On Diff #320287)

I think someone else already pointed that out elsewhere but this should probably be a std::string.

102 ↗(On Diff #320287)

This seems suspicious. Who manages the lifetime of the launch info and how do we guarantee it matches the lifetime of the process?

mib updated this revision to Diff 323943.Feb 16 2021, 3:03 AM
mib marked 10 inline comments as done.
mib updated this revision to Diff 323945.Feb 16 2021, 3:06 AM
mib updated this revision to Diff 324075.Feb 16 2021, 12:03 PM

Update ScriptedProcess::GetMemoryRegions to load regions lazily.

JDevlieghere added inline comments.Feb 17 2021, 11:40 AM
lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
118

m_interpreter->GetScriptedProcessInterface() seems to repeated a lot. Maybe it's worth having a private helper method that wraps it?

ScriptedProcess::GetInferface() { 
  return m_interpreter->GetScriptedProcessInterface();
}
154

Remembering to set the size seems error prone. I would use a ScopeExit (and clear it at the end on success).

lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py
46

I think this should work on all platforms?

53

can you make this more portable with os.path.join or os.dirname?

54

Why is this needed?

mib added inline comments.Feb 17 2021, 12:41 PM
lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py
54

I use this to load the script without running the process launch -C .... command.

You can see the rest of the logic in D95712, in the MyScriptedProcess example.

mib updated this revision to Diff 324564.Feb 18 2021, 1:39 AM
mib marked 4 inline comments as done.

Address @JDevlieghere feedbacks:

  • Make test more portable
  • Add helper function to get the ScriptedProcessInterface
mib updated this revision to Diff 324587.Feb 18 2021, 3:37 AM

Fix test typo.

JDevlieghere accepted this revision.Feb 18 2021, 11:47 AM

This LGTM

This revision is now accepted and ready to land.Feb 18 2021, 11:47 AM
mib updated this revision to Diff 324813.Feb 18 2021, 4:51 PM

Test ScriptedProcess::ReadMemory
Test launching the scripted process from both the command line and from the SBAPI.

mib updated this revision to Diff 327216.Mar 1 2021, 11:41 AM

Add support for loading a corefile.
Fix incomplete scripted process instantiation.

This revision was landed with ongoing or failed builds.Mar 1 2021, 12:14 PM
This revision was automatically updated to reflect the committed changes.

An auto-bisecting cron job has identified this change as a regression on Fedora 33 (x86-64). Can we get a quick fix or revert this? Here is the build output:

https://znu.io/dd391e1ef762d79f86112dc2480a89c9be066ce1-bisect.txt

I've reverted this. If you need help debugging this, please let me know. Also, as a reminder, please see the attached build log from earlier in the conversation. Thanks!

mib added a comment.Wed, Mar 24, 6:40 AM

I've reverted this. If you need help debugging this, please let me know. Also, as a reminder, please see the attached build log from earlier in the conversation. Thanks!

@davezarzycki Sorry for the inconvenience, I made sure it passed on the macOS and Debian bot (http://lab.llvm.org:8011/#/builders/68/builds/9225) before signing off but it seems that the patch caused some issues on other platforms.

Thanks for letting me know about that, I'll take a look at the build log and try to reproduce the issue.

The tests are failing because Dave's bot is running without enabled Python. The same is true for the Windows bot. Putting the plugin behind #ifdef LLDB_ENABLE_PYTHON should fix this.

FWIW, even with enabled Python this seems to have some minor problems with command parsing:

FAIL: test_double_type_dwo (TestDoubleTypes.DoubleTypesTestCase)
   Test that double-type variables are displayed correctly.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/teemperor/work/ci/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 1846, in test_method
    return attrvalue(self)
  File "/home/teemperor/work/ci/llvm-project/lldb/test/API/types/TestDoubleTypes.py", line 21, in test_double_type
    self.build_and_run('double.cpp', set(['double']))
  File "/home/teemperor/work/ci/llvm-project/lldb/test/API/types/AbstractBase.py", line 67, in build_and_run
    self.build_and_run_with_source_atoms_expr(
  File "/home/teemperor/work/ci/llvm-project/lldb/test/API/types/AbstractBase.py", line 88, in build_and_run_with_source_atoms_expr
    self.generic_type_tester(
  File "/home/teemperor/work/ci/llvm-project/lldb/test/API/types/AbstractBase.py", line 145, in generic_type_tester
    self.process_launch_o()
  File "/home/teemperor/work/ci/llvm-project/lldb/test/API/types/AbstractBase.py", line 111, in process_launch_o
    self.runCmd(
  File "/home/teemperor/work/ci/llvm-project/lldb/packages/Python/lldbsuite/test/lldbtest.py", line 2176, in runCmd
    self.assertTrue(self.res.Succeeded(),
AssertionError: False is not True : Command 'process launch -o "/home/teemperor/work/ci/build/lldb-test-build.noindex/types/TestDoubleTypes.test_double_type_dwo/test_double_type_dwo-golden-output.txt"
Error output:
error: invalid combination of options for the given command
' did not return successfully

(This is with enabled Python)

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

You probably want to use default initializers in the header for this stuff.

The tests are failing because Dave's bot is running without enabled Python. The same is true for the Windows bot. Putting the plugin behind #ifdef LLDB_ENABLE_PYTHON should fix this.

My cron job is not intentionally disabling python support. That just happened to be the fallout from the normal feature detection dance before the build starts:

  • Could NOT find SWIG (missing: SWIG_EXECUTABLE SWIG_DIR) (Required is at least version "3.0")
  • SWIG 3 or later is required for Python support in LLDB but could not be found
  • Could NOT find PythonAndSwig (missing: Python3_LIBRARIES Python3_INCLUDE_DIRS SWIG_EXECUTABLE)
  • Enable Python scripting support in LLDB: FALSE

The tests are failing because Dave's bot is running without enabled Python. The same is true for the Windows bot. Putting the plugin behind #ifdef LLDB_ENABLE_PYTHON should fix this.

My cron job is not intentionally disabling python support. That just happened to be the fallout from the normal feature detection dance before the build starts:

  • Could NOT find SWIG (missing: SWIG_EXECUTABLE SWIG_DIR) (Required is at least version "3.0")
  • SWIG 3 or later is required for Python support in LLDB but could not be found
  • Could NOT find PythonAndSwig (missing: Python3_LIBRARIES Python3_INCLUDE_DIRS SWIG_EXECUTABLE)
  • Enable Python scripting support in LLDB: FALSE

To clarify what I meant: The patch is wrong and the tests should not fail when SWIG>3 can't be found (which disables Python support). I was just pointing out what's the bug here. That your setup doesn't have SWIG>3 is perfectly fine (I'm in fact setting up another bot that is disabling SWIG/Python right now because this doesn't seem really covered beside the Windows bot and your bisect one)

mib updated this revision to Diff 333054.Wed, Mar 24, 11:02 AM
mib marked 2 inline comments as done.
mib added a reviewer: teemperor.

Enable ScriptedProcess process plugin only when LLDB_ENABLE_PYTHON is true.

This should fix the test failures that happen when python support is disabled.

mib added a comment.Wed, Mar 24, 11:10 AM

With the help of @teemperor, I was able to test that my latest changes work on Linux with and without enabling python for lldb. I'll re-land the patch and monitor the bots to make sure there is no other issues.

This revision was landed with ongoing or failed builds.Wed, Mar 24, 11:11 AM
stella.stamenova added inline comments.
lldb/bindings/python/CMakeLists.txt
114 ↗(On Diff #333054)

How is this different than the package on line 107?

The tests are failing because Dave's bot is running without enabled Python. The same is true for the Windows bot. Putting the plugin behind #ifdef LLDB_ENABLE_PYTHON should fix this.

The windows bot actually has LLDB_ENABLE_PYTHON set to true, so the failure has a different root cause. The error it is reporting is access violation. I'll get @mib some logs to investigate when I get a chance.

mib added inline comments.Wed, Mar 24, 1:47 PM
lldb/bindings/python/CMakeLists.txt
114 ↗(On Diff #333054)

This might have been copied twice ... getting rid of it.

mib updated this revision to Diff 333114.Wed, Mar 24, 1:51 PM
mib marked an inline comment as done.

Addressing @stella.stamenova feedback.

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

This is where the AV happens on Windows. It looks like GetInterface returns null

mib added inline comments.Thu, Mar 25, 7:43 AM
lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
69

Thanks @stella.stamenova for taking a closer look at this. I don't know why would the test call this code and crash, but I'll continue investigating it.

davezarzycki added inline comments.Thu, Mar 25, 3:20 PM
lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp
69

I've seen this crash on Fedora (x86-64) too so I don't think it's Windows specific.