[Lldb-commits] [lldb] [lldb] Check for abstract methods implementation in Scripted Plugin Objects (PR #71260)
Med Ismail Bennani via lldb-commits
lldb-commits at lists.llvm.org
Fri Nov 3 17:46:02 PDT 2023
https://github.com/medismailben created https://github.com/llvm/llvm-project/pull/71260
This patch enforces that every scripted object implements all the necessary abstract methods.
Every scripted affordance language interface can implement a list of abstract methods name that checked when the object is instanciated.
Since some scripting affordances implementations can be derived from template base classes, we can't check the object dictionary since it will contain the definition of the base class, so instead, this checks the scripting class dictionary.
Previously, for the various python interfaces, we used `ABC.abstractmethod` decorators but this is too language specific and doesn't work for scripting affordances that are not derived from template base classes (i.e OperatingSystem, ScriptedThreadPlan, ...), so this patch provides generic/language-agnostic checks for every scripted affordance.
>From a39fe96d3c4726ec2a5fd96e10638231e3c14266 Mon Sep 17 00:00:00 2001
From: Med Ismail Bennani <ismail at bennani.ma>
Date: Fri, 3 Nov 2023 17:42:10 -0700
Subject: [PATCH] [lldb] Check for abstract methods implementation in Scripted
Plugin Objects
This patch enforces that every scripted object implements all the
necessary abstract methods.
Every scripted affordance language interface can implement a list of
abstract methods name that checked when the object is instanciated.
Since some scripting affordances implementations can be derived from
template base classes, we can't check the object dictionary since it
will contain the definition of the base class, so instead, this checks
the scripting class dictionary.
Previously, for the various python interfaces, we used `ABC.abstractmethod`
decorators but this is too language specific and doesn't work for
scripting affordances that are not derived from template base classes
(i.e OperatingSystem, ScriptedThreadPlan, ...), so this patch provides
generic/language-agnostic checks for every scripted affordance.
Signed-off-by: Med Ismail Bennani <ismail at bennani.ma>
---
.../Interfaces/ScriptedInterface.h | 2 +
.../Interfaces/ScriptedPlatformInterface.h | 4 +
.../Interfaces/ScriptedProcessInterface.h | 4 +
.../Interfaces/ScriptedThreadInterface.h | 4 +
.../OperatingSystemPythonInterface.h | 7 ++
.../ScriptedPlatformPythonInterface.h | 6 +
.../ScriptedProcessPythonInterface.h | 5 +
.../Interfaces/ScriptedPythonInterface.h | 109 ++++++++++++++++--
.../ScriptedThreadPythonInterface.h | 5 +
.../Python/PythonDataObjects.cpp | 12 ++
.../Python/PythonDataObjects.h | 2 +
.../scripted_process/TestScriptedProcess.py | 46 ++++++++
.../missing_methods_scripted_process.py | 19 +++
.../Python/PythonDataObjectsTests.cpp | 3 +
14 files changed, 219 insertions(+), 9 deletions(-)
create mode 100644 lldb/test/API/functionalities/scripted_process/missing_methods_scripted_process.py
diff --git a/lldb/include/lldb/Interpreter/Interfaces/ScriptedInterface.h b/lldb/include/lldb/Interpreter/Interfaces/ScriptedInterface.h
index e4816352daa5db6..9753a916243b7be 100644
--- a/lldb/include/lldb/Interpreter/Interfaces/ScriptedInterface.h
+++ b/lldb/include/lldb/Interpreter/Interfaces/ScriptedInterface.h
@@ -30,6 +30,8 @@ class ScriptedInterface {
return m_object_instance_sp;
}
+ virtual llvm::SmallVector<llvm::StringLiteral> GetAbstractMethods() const = 0;
+
template <typename Ret>
static Ret ErrorWithMessage(llvm::StringRef caller_name,
llvm::StringRef error_msg, Status &error,
diff --git a/lldb/include/lldb/Interpreter/Interfaces/ScriptedPlatformInterface.h b/lldb/include/lldb/Interpreter/Interfaces/ScriptedPlatformInterface.h
index 2dcbb47ffa6de85..e73565db88b0ce0 100644
--- a/lldb/include/lldb/Interpreter/Interfaces/ScriptedPlatformInterface.h
+++ b/lldb/include/lldb/Interpreter/Interfaces/ScriptedPlatformInterface.h
@@ -26,6 +26,10 @@ class ScriptedPlatformInterface : virtual public ScriptedInterface {
return {llvm::make_error<UnimplementedError>()};
}
+ llvm::SmallVector<llvm::StringLiteral> GetAbstractMethods() const override {
+ return {};
+ }
+
virtual StructuredData::DictionarySP ListProcesses() { return {}; }
virtual StructuredData::DictionarySP GetProcessInfo(lldb::pid_t) {
diff --git a/lldb/include/lldb/Interpreter/Interfaces/ScriptedProcessInterface.h b/lldb/include/lldb/Interpreter/Interfaces/ScriptedProcessInterface.h
index a429cacd862f121..767ad50ba0978e2 100644
--- a/lldb/include/lldb/Interpreter/Interfaces/ScriptedProcessInterface.h
+++ b/lldb/include/lldb/Interpreter/Interfaces/ScriptedProcessInterface.h
@@ -28,6 +28,10 @@ class ScriptedProcessInterface : virtual public ScriptedInterface {
return {llvm::make_error<UnimplementedError>()};
}
+ llvm::SmallVector<llvm::StringLiteral> GetAbstractMethods() const override {
+ return {};
+ }
+
virtual StructuredData::DictionarySP GetCapabilities() { return {}; }
virtual Status Attach(const ProcessAttachInfo &attach_info) {
diff --git a/lldb/include/lldb/Interpreter/Interfaces/ScriptedThreadInterface.h b/lldb/include/lldb/Interpreter/Interfaces/ScriptedThreadInterface.h
index 107e593b5561ef7..17bf8bc42490e84 100644
--- a/lldb/include/lldb/Interpreter/Interfaces/ScriptedThreadInterface.h
+++ b/lldb/include/lldb/Interpreter/Interfaces/ScriptedThreadInterface.h
@@ -27,6 +27,10 @@ class ScriptedThreadInterface : virtual public ScriptedInterface {
return {llvm::make_error<UnimplementedError>()};
}
+ llvm::SmallVector<llvm::StringLiteral> GetAbstractMethods() const override {
+ return {};
+ }
+
virtual lldb::tid_t GetThreadID() { return LLDB_INVALID_THREAD_ID; }
virtual std::optional<std::string> GetName() { return std::nullopt; }
diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/OperatingSystemPythonInterface.h b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/OperatingSystemPythonInterface.h
index ee24e0ee30f91b3..e27f4dd28010662 100644
--- a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/OperatingSystemPythonInterface.h
+++ b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/OperatingSystemPythonInterface.h
@@ -29,6 +29,13 @@ class OperatingSystemPythonInterface
StructuredData::DictionarySP args_sp,
StructuredData::Generic *script_obj = nullptr) override;
+ llvm::SmallVector<llvm::StringLiteral> GetAbstractMethods() const override {
+ return llvm::SmallVector<llvm::StringLiteral>({"get_thread_info"
+ "get_register_data",
+ "get_stop_reason",
+ "get_register_context"});
+ }
+
StructuredData::DictionarySP CreateThread(lldb::tid_t tid,
lldb::addr_t context) override;
diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPlatformPythonInterface.h b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPlatformPythonInterface.h
index e04f2d02ab1d120..0842d3a00342914 100644
--- a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPlatformPythonInterface.h
+++ b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPlatformPythonInterface.h
@@ -28,6 +28,12 @@ class ScriptedPlatformPythonInterface : public ScriptedPlatformInterface,
StructuredData::DictionarySP args_sp,
StructuredData::Generic *script_obj = nullptr) override;
+ llvm::SmallVector<llvm::StringLiteral> GetAbstractMethods() const override {
+ return llvm::SmallVector<llvm::StringLiteral>(
+ {"list_processes", "attach_to_process", "launch_process",
+ "kill_process"});
+ }
+
StructuredData::DictionarySP ListProcesses() override;
StructuredData::DictionarySP GetProcessInfo(lldb::pid_t) override;
diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedProcessPythonInterface.h b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedProcessPythonInterface.h
index f3cff619f6624f0..c75caa9340f2504 100644
--- a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedProcessPythonInterface.h
+++ b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedProcessPythonInterface.h
@@ -29,6 +29,11 @@ class ScriptedProcessPythonInterface : public ScriptedProcessInterface,
StructuredData::DictionarySP args_sp,
StructuredData::Generic *script_obj = nullptr) override;
+ llvm::SmallVector<llvm::StringLiteral> GetAbstractMethods() const override {
+ return llvm::SmallVector<llvm::StringLiteral>(
+ {"read_memory_at_address", "is_alive", "get_scripted_thread_plugin"});
+ }
+
StructuredData::DictionarySP GetCapabilities() override;
Status Attach(const ProcessAttachInfo &attach_info) override;
diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPythonInterface.h b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPythonInterface.h
index 7af981639709997..5039c0978330102 100644
--- a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPythonInterface.h
+++ b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPythonInterface.h
@@ -32,6 +32,42 @@ class ScriptedPythonInterface : virtual public ScriptedInterface {
ScriptedPythonInterface(ScriptInterpreterPythonImpl &interpreter);
~ScriptedPythonInterface() override = default;
+ enum class AbstractMethodCheckerCases {
+ eNotImplemented,
+ eNotAllocated,
+ eNotCallable,
+ eValid
+ };
+
+ llvm::Expected<std::map<llvm::StringLiteral, AbstractMethodCheckerCases>>
+ CheckAbstractMethodImplementation(
+ const python::PythonDictionary &class_dict) const {
+
+ using namespace python;
+
+ std::map<llvm::StringLiteral, AbstractMethodCheckerCases> checker;
+#define HANDLE_ERROR(method_name, error) \
+ { \
+ checker[method_name] = error; \
+ continue; \
+ }
+
+ for (const llvm::StringLiteral &method_name : GetAbstractMethods()) {
+ if (!class_dict.HasKey(method_name))
+ HANDLE_ERROR(method_name, AbstractMethodCheckerCases::eNotImplemented)
+ auto callable_or_err = class_dict.GetItem(method_name);
+ if (!callable_or_err)
+ HANDLE_ERROR(method_name, AbstractMethodCheckerCases::eNotAllocated)
+ if (!PythonCallable::Check(callable_or_err.get().get()))
+ HANDLE_ERROR(method_name, AbstractMethodCheckerCases::eNotCallable)
+ checker[method_name] = AbstractMethodCheckerCases::eValid;
+ }
+
+#undef HANDLE_ERROR
+
+ return checker;
+ }
+
template <typename... Args>
llvm::Expected<StructuredData::GenericSP>
CreatePluginObject(llvm::StringRef class_name,
@@ -74,10 +110,10 @@ class ScriptedPythonInterface : virtual public ScriptedInterface {
m_interpreter.GetDictionaryName());
}
- auto method =
+ auto init =
PythonObject::ResolveNameWithDictionary<python::PythonCallable>(
class_name, dict);
- if (!method.IsAllocated())
+ if (!init.IsAllocated())
return llvm::createStringError(llvm::inconvertibleErrorCode(),
"Could not find script class: %s",
class_name.data());
@@ -86,7 +122,7 @@ class ScriptedPythonInterface : virtual public ScriptedInterface {
auto transformed_args = TransformArgs(original_args);
std::string error_string;
- llvm::Expected<PythonCallable::ArgInfo> arg_info = method.GetArgInfo();
+ llvm::Expected<PythonCallable::ArgInfo> arg_info = init.GetArgInfo();
if (!arg_info) {
llvm::handleAllErrors(
arg_info.takeError(),
@@ -103,24 +139,79 @@ class ScriptedPythonInterface : virtual public ScriptedInterface {
"Resulting object is not initialized.");
std::apply(
- [&method, &expected_return_object](auto &&...args) {
+ [&init, &expected_return_object](auto &&...args) {
llvm::consumeError(expected_return_object.takeError());
- expected_return_object = method(args...);
+ expected_return_object = init(args...);
},
transformed_args);
- if (llvm::Error e = expected_return_object.takeError())
- return std::move(e);
- result = std::move(expected_return_object.get());
+ if (!expected_return_object)
+ return expected_return_object.takeError();
+ result = expected_return_object.get();
}
- if (!result.IsValid())
+ if (!result.IsValid() || !result.HasAttribute("__class__"))
+ return llvm::createStringError(
+ llvm::inconvertibleErrorCode(),
+ "Resulting object is not a valid Python Object.");
+
+ PythonObject obj_class = result.GetAttributeValue("__class__");
+
+ PythonString obj_class_name =
+ obj_class.GetAttributeValue("__name__").AsType<PythonString>();
+
+ PythonObject object_class_mapping_proxy =
+ obj_class.GetAttributeValue("__dict__");
+
+ PythonCallable dict_converter = PythonModule::BuiltinsModule()
+ .ResolveName("dict")
+ .AsType<PythonCallable>();
+ if (!dict_converter.IsAllocated())
+ return llvm::createStringError(
+ llvm::inconvertibleErrorCode(),
+ "Resulting object is not a valid Python Object.");
+
+ PythonDictionary object_class_dict =
+ dict_converter(object_class_mapping_proxy).AsType<PythonDictionary>();
+ if (!object_class_dict.IsAllocated())
return llvm::createStringError(
llvm::inconvertibleErrorCode(),
"Resulting object is not a valid Python Object.");
+ auto checker_or_err = CheckAbstractMethodImplementation(object_class_dict);
+ if (!checker_or_err)
+ return checker_or_err.takeError();
+
+#define LOG_ABSTRACT_METHOD_STATUS(state) \
+ LLDB_LOG(GetLog(LLDBLog::Script), "Abstract method {0}.{1} " #state ".", \
+ obj_class_name.GetString(), method_checker.first)
+
+ for (const auto &method_checker : *checker_or_err)
+ switch (method_checker.second) {
+ case AbstractMethodCheckerCases::eNotImplemented:
+ LOG_ABSTRACT_METHOD_STATUS(not implemented);
+ break;
+ case AbstractMethodCheckerCases::eNotAllocated:
+ LOG_ABSTRACT_METHOD_STATUS(not allocated);
+ break;
+ case AbstractMethodCheckerCases::eNotCallable:
+ LOG_ABSTRACT_METHOD_STATUS(not callable);
+ break;
+ case AbstractMethodCheckerCases::eValid:
+ LOG_ABSTRACT_METHOD_STATUS(implemented);
+ break;
+ }
+#undef LOG_ABSTRACT_METHOD_STATUS
+
+ for (const auto &method_checker : *checker_or_err) {
+ if (method_checker.second != AbstractMethodCheckerCases::eValid)
+ return llvm::createStringError(llvm::inconvertibleErrorCode(),
+ "Abstract method {0}.{1} missing ❌.");
+ }
+
m_object_instance_sp = StructuredData::GenericSP(
new StructuredPythonObject(std::move(result)));
+
return m_object_instance_sp;
}
diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedThreadPythonInterface.h b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedThreadPythonInterface.h
index b7b7439461a03d6..5676f7f1d67528f 100644
--- a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedThreadPythonInterface.h
+++ b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedThreadPythonInterface.h
@@ -28,6 +28,11 @@ class ScriptedThreadPythonInterface : public ScriptedThreadInterface,
StructuredData::DictionarySP args_sp,
StructuredData::Generic *script_obj = nullptr) override;
+ llvm::SmallVector<llvm::StringLiteral> GetAbstractMethods() const override {
+ return llvm::SmallVector<llvm::StringLiteral>(
+ {"get_stop_reason", "get_register_context"});
+ }
+
lldb::tid_t GetThreadID() override;
std::optional<std::string> GetName() override;
diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp b/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
index fe3438c42471543..79dbdcb28b3f2f7 100644
--- a/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
+++ b/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp
@@ -663,6 +663,18 @@ bool PythonDictionary::Check(PyObject *py_obj) {
return PyDict_Check(py_obj);
}
+bool PythonDictionary::HasKey(const llvm::Twine &key) const {
+ if (!IsValid())
+ return false;
+ PythonString key_object(key.str().data());
+
+ if (int res = PyDict_Contains(m_py_obj, key_object.get()) > 0)
+ return res;
+
+ PyErr_Print();
+ return false;
+}
+
uint32_t PythonDictionary::GetSize() const {
if (IsValid())
return PyDict_Size(m_py_obj);
diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h b/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
index 012f16e95e770df..82eee76e42b27a2 100644
--- a/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
+++ b/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h
@@ -562,6 +562,8 @@ class PythonDictionary : public TypedPythonObject<PythonDictionary> {
static bool Check(PyObject *py_obj);
+ bool HasKey(const llvm::Twine &key) const;
+
uint32_t GetSize() const;
PythonList GetKeys() const;
diff --git a/lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py b/lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py
index 1761122999f848e..df3fc731e3e891d 100644
--- a/lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py
+++ b/lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py
@@ -60,6 +60,52 @@ def move_blueprint_to_dsym(self, blueprint_name):
)
shutil.copy(blueprint_origin_path, blueprint_destination_path)
+ def test_missing_methods_scripted_register_context(self):
+ """Test that we only instanciate scripted processes if they implement
+ all the required abstract methods."""
+ self.build()
+
+ os.environ["SKIP_SCRIPTED_PROCESS_LAUNCH"] = "1"
+
+ def cleanup():
+ del os.environ["SKIP_SCRIPTED_PROCESS_LAUNCH"]
+
+ self.addTearDownHook(cleanup)
+
+ target = self.dbg.CreateTarget(self.getBuildArtifact("a.out"))
+ self.assertTrue(target, VALID_TARGET)
+ log_file = self.getBuildArtifact("script.log")
+ self.runCmd("log enable lldb script -f " + log_file)
+ self.assertTrue(os.path.isfile(log_file))
+ self.runCmd(self.getBuildArtifact("missing_methods_scripted_process.py"))
+
+ launch_info = lldb.SBLaunchInfo(None)
+ launch_info.SetProcessPluginName("ScriptedProcess")
+ launch_info.SetScriptedProcessClassName(
+ "missing_methods_scripted_process.MissingMethodsScriptedProcess"
+ )
+ error = lldb.SBError()
+
+ process = target.Launch(launch_info, error)
+
+ self.assertFailure(error)
+
+ with open(log_file, "r") as f:
+ log = f.read()
+
+ self.assertIn(
+ "Abstract method MissingMethodsScriptedProcess.read_memory_at_address not implemented",
+ log,
+ )
+ self.assertIn(
+ "Abstract method MissingMethodsScriptedProcess.is_alive not implemented",
+ log,
+ )
+ self.assertIn(
+ "Abstract method MissingMethodsScriptedProcess.get_scripted_thread_plugin not implemented",
+ log,
+ )
+
@skipUnlessDarwin
def test_invalid_scripted_register_context(self):
"""Test that we can launch an lldb scripted process with an invalid
diff --git a/lldb/test/API/functionalities/scripted_process/missing_methods_scripted_process.py b/lldb/test/API/functionalities/scripted_process/missing_methods_scripted_process.py
new file mode 100644
index 000000000000000..a1db0640033e0ed
--- /dev/null
+++ b/lldb/test/API/functionalities/scripted_process/missing_methods_scripted_process.py
@@ -0,0 +1,19 @@
+import os
+
+
+class MissingMethodsScriptedProcess:
+ def __init__(self, exe_ctx, args):
+ pass
+
+
+def __lldb_init_module(debugger, dict):
+ if not "SKIP_SCRIPTED_PROCESS_LAUNCH" in os.environ:
+ debugger.HandleCommand(
+ "process launch -C %s.%s"
+ % (__name__, MissingMethodsScriptedProcess.__name__)
+ )
+ else:
+ print(
+ "Name of the class that will manage the scripted process: '%s.%s'"
+ % (__name__, MissingMethodsScriptedProcess.__name__)
+ )
diff --git a/lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp b/lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
index 0b816a8ae0a587e..efb8f725f6739ae 100644
--- a/lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
+++ b/lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp
@@ -504,6 +504,9 @@ TEST_F(PythonDataObjectsTest, TestPythonDictionaryManipulation) {
dict.SetItemForKey(keys[i], values[i]);
EXPECT_EQ(dict_entries, dict.GetSize());
+ EXPECT_FALSE(dict.HasKey("not_in_dict"));
+ EXPECT_TRUE(dict.HasKey(key_0));
+ EXPECT_TRUE(dict.HasKey(key_1));
// Verify that the keys and values match
PythonObject chk_value1 = dict.GetItemForKey(keys[0]);
More information about the lldb-commits
mailing list