This is an archive of the discontinued LLVM Phabricator instance.

[lldb/Plugins] Introduce Scripted Interface Factory
ClosedPublic

Authored by mib on Aug 4 2021, 7:02 PM.

Details

Summary

This patch splits the previous ScriptedProcessPythonInterface into
multiple specific classes:

  1. The ScriptedInterface abstract class that carries the interface instance object and its virtual pure abstract creation method.
  1. The ScriptedPythonInterface that holds a generic Dispatch method that can be used by various interfaces to call python methods and also keeps a reference to the Python Script Interpreter instance.
  1. The ScriptedProcessInterface that describes the base Scripted Process model with all the methods used in the underlying script.

All these components are used to refactor the ScriptedProcessPythonInterface
class, making it more modular.

This patch is also a requirement for the upcoming work on ScriptedThread.

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

Diff Detail

Event Timeline

mib created this revision.Aug 4 2021, 7:02 PM
mib requested review of this revision.Aug 4 2021, 7:02 PM
mib updated this revision to Diff 364318.Aug 4 2021, 7:10 PM
JDevlieghere added inline comments.Aug 10 2021, 11:43 AM
lldb/bindings/python/python-wrapper.swig
291

Unrelated whitespace change?

lldb/include/lldb/Interpreter/ScriptedInterface.h
26

We generally don't use const when passing by value.

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

Why is this needed?

lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp
38–42

Why was this moved up? The lock should be as tight as possible.

99
102

Seems like you're always calling error_with_message with a constant string, so we can avoid allocating a std::string.

This is a good start, but it seems like there's still a lot of boilerplate once you get to the python side that could be trimmed down.

For instance, ScriptedProcessPythonInterface::GetThreadWithID is almost entirely generic. The only things that make it specific to this use are the method name that's called, and the fact that it passes an lldb::tid_t to and returns an SBStructuredData.

You've started to take care of the latter by building up a set of generic "call a method, return X" routines, and surely an SBStructuredData one will be pretty common.

Then you can add a way to pass arguments to the generic routines. Maybe an array of some little struct that has either the duple "python format string/value" or a PythonObject so the dispatcher can call the code appropriately?

lldb/source/Plugins/ScriptInterpreter/Python/ScriptedPythonInterface.cpp
33–45

It seems like there is a whole lot of duplicated code here. The only difference is in what we do with the returned PythonObject.

Maybe we should have a centralized "dispatch" method and then these routines differ in the conversion.

Also, it seems a shame to throw away error info, but that's what the GetGenericInteger & GetGenericString do when the object instance isn't valid, etc. llvm::Optional is good for forcing validity checks, but has no information on why the operation didn't succeed. It would be better to have all these methods take a status object for errors. You can still return the result wrapped in an optional.

When you are developing one of these classes, it would be nice to be told "no such method" for instance so you can go check if you misspelled it, or something, rather than just getting llvm::None that eventually results in some more remote error.

mib updated this revision to Diff 368704.Aug 25 2021, 12:23 PM
mib marked 7 inline comments as done.
mib edited the summary of this revision. (Show Details)
mib added a reviewer: jingham.

Address @JDevlieghere and implement the generic ScriptedPythonInterface::Dispatch method that @jingham suggested.

This method uses templated variadic arguments and a templated return type to be able to call any python method. This allows removing the type-specific helper functions.

I just started reading this but I have to go now. Had one comment about errors from Dispatch. I'll look more later.

lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp
88–89

It seems like we also do this bit every time we do Dispatch. Maybe Dispatch could check the outgoing obj & obj->IsValid and if they aren't then it can put an appropriate message in the "error" object. Then you would just have to check error.Fail().

That would also simplify the logging. You could always dump the error's GetCString to your error_with_message. Right now, you don't log the error's GetCString in the case where the object is valid, but error.Fail is true, which might end up dropping some useful info.

Nobody will be surprised that I like the templated solution for the dispatch method :-) A small comment similar to Jim's but otherwise this looks good to me.

lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp
38–42

This comment is marked as done, but the lock is still taken at the beginning of the function.

88–89

I had a similar observation but I was thinking that Dispatch should return an Expected<StructuredData::ObjectSP>. That way you can be more specific in your error message too and differentiate between the object being null, invalid or another error.

lldb/source/Plugins/ScriptInterpreter/Python/ScriptedPythonInterface.h
40–46

This is a common pattern in llvm, but totally up to preference

112

FormatArgs has an overload for the no-args case so I don't think you need this?

mib marked 3 inline comments as done.Sep 2 2021, 6:45 AM
mib added inline comments.
lldb/source/Plugins/ScriptInterpreter/Python/ScriptedProcessPythonInterface.cpp
88–89

TBH, I don't see the added value of using Expected<T> because I'll still need to check the expected object !expected which is the same as doing !obj.

Also, regarding @jingham's suggestion, since Dispatch's return type is templated, it is not guaranteed to have an operator bool() method (same with IsValid()) that's why the check is done after calling Dispatch, on a case by case basis.

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

I need it since PyObject_CallMethod expects a nullptr for the format string when the param pack is empty, not an empty string.

mib updated this revision to Diff 370258.EditedSep 2 2021, 6:53 AM
mib marked an inline comment as done.

Address @JDevlieghere feedbacks:

  • Tighten to PythonInterpreter Lock.
  • Use llvm's common pattern when checking SBError*
This revision is now accepted and ready to land.Sep 3 2021, 10:00 AM
This revision was landed with ongoing or failed builds.Sep 3 2021, 10:37 AM
This revision was automatically updated to reflect the committed changes.
jankratochvil added a subscriber: jankratochvil.EditedSep 3 2021, 1:12 PM

It broke Fedora 34 x86_64 buildbot (using gcc) lldb-x86_64-fedora = https://lab.llvm.org/staging/#/builders/16 - the build fails:
https://lab.llvm.org/staging/#/builders/16/builds/10450

mib added a comment.Sep 3 2021, 1:35 PM

It broke Fedora 34 x86_64 buildbot (using gcc) lldb-x86_64-fedora = https://lab.llvm.org/staging/#/builders/16 - the build fails:
https://lab.llvm.org/staging/#/builders/16/builds/10450

Looking! I'll try to send a quick fix ASAP!

mib added a comment.Sep 3 2021, 2:17 PM

I have a fix. I'm testing it locally before landing it.

mib added a comment.Sep 3 2021, 3:22 PM

It broke Fedora 34 x86_64 buildbot (using gcc) lldb-x86_64-fedora = https://lab.llvm.org/staging/#/builders/16 - the build fails:
https://lab.llvm.org/staging/#/builders/16/builds/10450

Looking! I'll try to send a quick fix ASAP!

This should get fixed with rG5f6f33da9ee6cbaef5f10b4a7be34a91d5185b2b