This is an archive of the discontinued LLVM Phabricator instance.

[lldb/Interpreter] Add ScriptInterpreter Wrapper for ScriptedProcess
ClosedPublic

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

Details

Summary

This patch adds a ScriptedProcess interface to the ScriptInterpreter and
more specifically, to the ScriptInterpreterPython.

This interface will be used in the C++ ScriptProcess Process Plugin to
call the script methods.

At the moment, not all methods are implemented, they will upstreamed in
upcoming patches.

This patch also adds helper methods to the ScriptInterpreter to
convert SBAPI Types (SBData & SBError) to lldb_private types
(DataExtractor & Status).

rdar://65508855

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

Diff Detail

Event Timeline

mib requested review of this revision.Jan 29 2021, 5:15 PM
mib created this revision.
mib updated this revision to Diff 320285.Jan 29 2021, 11:46 PM

Add implementation for ScriptedProcess_{GetNumMemoryRegions,GetNumThreads,GetProcessID,IsAlive,CanDebug,ReadMemoryAtAddress}

mib edited the summary of this revision. (Show Details)Jan 29 2021, 11:47 PM
JDevlieghere added inline comments.Feb 1 2021, 8:52 AM
lldb/include/lldb/API/SBData.h
133

You cannot expose non-SB classes in public methods because that would make them part of the API.

lldb/include/lldb/Interpreter/ScriptInterpreter.h
536

Please use Doxygen groups for this so everyone can benefit from it and not just a single IDE.

/// ScriptedProcessInterface
/// @{
...
/// @}
539

It doesn't seem like these methods belong in ScriptInterpreter. Can they go into their own class of which the ScriptInterpreter holds a single instance?

JDevlieghere added inline comments.Feb 3 2021, 11:57 AM
lldb/include/lldb/Interpreter/ScriptInterpreter.h
539

Also, can this take a StringRef instead of a C string?

mib updated this revision to Diff 322997.Feb 11 2021, 7:08 AM
mib marked 4 inline comments as done.

Address @JDevlieghere feedbacks.

A few small nits, but overall this LGTM.

lldb/include/lldb/API/SBData.h
16

The linter is right. Did you clang-format your patch? It should add it automatically.

lldb/source/Interpreter/ScriptInterpreter.cpp
31

If you gave the scripted_process_interface_sp a default value (scripted_process_interface_sp = {}), you could skip the overload, unless that prevents you from forward declaring the interface in the header?

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

I know you copied this code but let's turn this and the other comments in this file into full sentences.

This revision is now accepted and ready to land.Feb 12 2021, 9:31 AM
mib updated this revision to Diff 323481.Feb 12 2021, 2:30 PM
mib marked 3 inline comments as done.

Address @JDevlieghere comments:

  • Fix namespace closing comment in SBData.h
  • Remove ScriptInterpreter overload by making the ScriptedProcessInterface a default argument.
  • Replace inline comments with error messages.

Also, since TestScriptInterpreterPython inherits from ScriptInterpreterPythonImpl it needed some symbol definitions.

labath added inline comments.Feb 15 2021, 3:14 AM
lldb/unittests/ScriptInterpreter/Python/CMakeLists.txt
6 ↗(On Diff #323481)

This is a bad idea -- it makes the binary get two copies of everything.

mib updated this revision to Diff 323916.Feb 16 2021, 1:36 AM
mib marked an inline comment as done.
mib edited the summary of this revision. (Show Details)
  • Fixed unittest's liblldb linking issue by unwrapping the SBError type into a lldb_private::Status in the ScriptInterpreter.
  • Renamed ScriptInterpreter SBData -> lldb_private::DataExtractor helper function to make it more understandable.
  • Added interface endpoints to fetch MemoryRegions and Threads lazily (as @labath suggested in D95712).
  • Refactored the ScriptedProcessInterface by fixing typos, adding missing arguments, removing useless methods.
mib updated this revision to Diff 323930.Feb 16 2021, 2:27 AM
mib updated this revision to Diff 324071.Feb 16 2021, 11:57 AM

Fixed null deref crash when unwrapping SBError opaque ptr.

JDevlieghere added inline comments.Feb 17 2021, 11:29 AM
lldb/include/lldb/Interpreter/ScriptInterpreter.h
548

Why does this need to be a shared pointer and not a unique one? I think the script interpreter should have unique ownership over the scripted process interface.

mib marked an inline comment as done.Feb 18 2021, 12:54 AM
mib updated this revision to Diff 324558.Feb 18 2021, 1:00 AM

Make ScriptedProcessInterface a unique_ptr instead of shared_ptr in ScriptInterpreter

mib updated this revision to Diff 324579.Feb 18 2021, 3:11 AM

Remove ScriptedProcessInterface.cpp from the Interpreter's CMakeLists.txt since the file doesn't exist.

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.
mib added a comment.Mar 1 2021, 1:29 PM

This does not build on the Windows bot:

https://lab.llvm.org/buildbot/#/builders/83/builds/4214

Looking ...

mib added a comment.Mar 1 2021, 1:40 PM

This does not build on the Windows bot:

https://lab.llvm.org/buildbot/#/builders/83/builds/4214

Should be fixed with 5a9c34918bb1526b7e8c29aa5e4fb8d8e27e27b4 ... I'm also investigating some test failures that are happening on Linux, that will also probably happen on Windows. I should have a fix for that soon.

In D95711#2595566, @mib wrote:

This does not build on the Windows bot:

https://lab.llvm.org/buildbot/#/builders/83/builds/4214

Should be fixed with 5a9c34918bb1526b7e8c29aa5e4fb8d8e27e27b4 ... I'm also investigating some test failures that are happening on Linux, that will also probably happen on Windows. I should have a fix for that soon.

Are these the same failures?

https://lab.llvm.org/buildbot/#/builders/83/builds/4217

mib added a comment.Mar 1 2021, 2:45 PM
In D95711#2595566, @mib wrote:

This does not build on the Windows bot:

https://lab.llvm.org/buildbot/#/builders/83/builds/4214

Should be fixed with 5a9c34918bb1526b7e8c29aa5e4fb8d8e27e27b4 ... I'm also investigating some test failures that are happening on Linux, that will also probably happen on Windows. I should have a fix for that soon.

Are these the same failures?

https://lab.llvm.org/buildbot/#/builders/83/builds/4217

Yes! I'll mark them as XFAIL while working on the fix.

mib added a comment.Mar 1 2021, 3:37 PM

I reverted the ScriptedProcess patches since they caused test failures on the bots.