From a98cf851f5947546eb76196ef945ce1fa8d9bfaa Mon Sep 17 00:00:00 2001 From: Med Ismail Bennani Date: Mon, 6 Nov 2023 14:15:12 -0800 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 --- .../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 | 156 ++++++++++++++---- .../ScriptedThreadPythonInterface.h | 5 + .../Python/PythonDataObjects.cpp | 14 ++ .../Python/PythonDataObjects.h | 2 + .../scripted_process/TestScriptedProcess.py | 49 ++++++ .../missing_methods_scripted_process.py | 19 +++ .../Python/PythonDataObjectsTests.cpp | 3 + 14 files changed, 251 insertions(+), 29 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 e4816352daa5d..9753a916243b7 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 GetAbstractMethods() const = 0; + template 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 7feaa01fe89b8..167149cbf5b8f 100644 --- a/lldb/include/lldb/Interpreter/Interfaces/ScriptedPlatformInterface.h +++ b/lldb/include/lldb/Interpreter/Interfaces/ScriptedPlatformInterface.h @@ -24,6 +24,10 @@ class ScriptedPlatformInterface : virtual public ScriptedInterface { StructuredData::DictionarySP args_sp, StructuredData::Generic *script_obj = nullptr) = 0; + llvm::SmallVector 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 10203b1f8baa7..e41f488d201e2 100644 --- a/lldb/include/lldb/Interpreter/Interfaces/ScriptedProcessInterface.h +++ b/lldb/include/lldb/Interpreter/Interfaces/ScriptedProcessInterface.h @@ -26,6 +26,10 @@ class ScriptedProcessInterface : virtual public ScriptedInterface { StructuredData::DictionarySP args_sp, StructuredData::Generic *script_obj = nullptr) = 0; + llvm::SmallVector 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 a7cfc690b67dc..0520b6905e250 100644 --- a/lldb/include/lldb/Interpreter/Interfaces/ScriptedThreadInterface.h +++ b/lldb/include/lldb/Interpreter/Interfaces/ScriptedThreadInterface.h @@ -25,6 +25,10 @@ class ScriptedThreadInterface : virtual public ScriptedInterface { StructuredData::DictionarySP args_sp, StructuredData::Generic *script_obj = nullptr) = 0; + llvm::SmallVector GetAbstractMethods() const override { + return {}; + } + virtual lldb::tid_t GetThreadID() { return LLDB_INVALID_THREAD_ID; } virtual std::optional 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 ee24e0ee30f91..e27f4dd280106 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 GetAbstractMethods() const override { + return llvm::SmallVector({"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 e04f2d02ab1d1..0842d3a003429 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 GetAbstractMethods() const override { + return llvm::SmallVector( + {"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 f3cff619f6624..c75caa9340f25 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 GetAbstractMethods() const override { + return llvm::SmallVector( + {"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 7af9816397099..163659234466d 100644 --- a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPythonInterface.h +++ b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPythonInterface.h @@ -32,6 +32,45 @@ class ScriptedPythonInterface : virtual public ScriptedInterface { ScriptedPythonInterface(ScriptInterpreterPythonImpl &interpreter); ~ScriptedPythonInterface() override = default; + enum class AbstractMethodCheckerCases { + eNotImplemented, + eNotAllocated, + eNotCallable, + eValid + }; + + llvm::Expected> + CheckAbstractMethodImplementation( + const python::PythonDictionary &class_dict) const { + + using namespace python; + + std::map checker; +#define SET_ERROR_AND_CONTINUE(method_name, error) \ + { \ + checker[method_name] = error; \ + continue; \ + } + + for (const llvm::StringLiteral &method_name : GetAbstractMethods()) { + if (!class_dict.HasKey(method_name)) + SET_ERROR_AND_CONTINUE(method_name, + AbstractMethodCheckerCases::eNotImplemented) + auto callable_or_err = class_dict.GetItem(method_name); + if (!callable_or_err) + SET_ERROR_AND_CONTINUE(method_name, + AbstractMethodCheckerCases::eNotAllocated) + if (!PythonCallable::Check(callable_or_err.get().get())) + SET_ERROR_AND_CONTINUE(method_name, + AbstractMethodCheckerCases::eNotCallable) + checker[method_name] = AbstractMethodCheckerCases::eValid; + } + +#undef HANDLE_ERROR + + return checker; + } + template llvm::Expected CreatePluginObject(llvm::StringRef class_name, @@ -39,20 +78,20 @@ class ScriptedPythonInterface : virtual public ScriptedInterface { using namespace python; using Locker = ScriptInterpreterPythonImpl::Locker; + auto create_error = [](std::string message) { + return llvm::createStringError(llvm::inconvertibleErrorCode(), message); + }; + bool has_class_name = !class_name.empty(); bool has_interpreter_dict = !(llvm::StringRef(m_interpreter.GetDictionaryName()).empty()); if (!has_class_name && !has_interpreter_dict && !script_obj) { if (!has_class_name) - return llvm::createStringError(llvm::inconvertibleErrorCode(), - "Missing script class name."); + return create_error("Missing script class name."); else if (!has_interpreter_dict) - return llvm::createStringError( - llvm::inconvertibleErrorCode(), - "Invalid script interpreter dictionary."); + return create_error("Invalid script interpreter dictionary."); else - return llvm::createStringError(llvm::inconvertibleErrorCode(), - "Missing scripting object."); + return create_error("Missing scripting object."); } Locker py_lock(&m_interpreter, Locker::AcquireLock | Locker::NoSTDIN, @@ -67,26 +106,23 @@ class ScriptedPythonInterface : virtual public ScriptedInterface { auto dict = PythonModule::MainModule().ResolveName( m_interpreter.GetDictionaryName()); - if (!dict.IsAllocated()) { - return llvm::createStringError( - llvm::inconvertibleErrorCode(), - "Could not find interpreter dictionary: %s", - m_interpreter.GetDictionaryName()); - } + if (!dict.IsAllocated()) + return create_error( + llvm::formatv("Could not find interpreter dictionary: %s", + m_interpreter.GetDictionaryName())); - auto method = + auto init = PythonObject::ResolveNameWithDictionary( class_name, dict); - if (!method.IsAllocated()) - return llvm::createStringError(llvm::inconvertibleErrorCode(), - "Could not find script class: %s", - class_name.data()); + if (!init.IsAllocated()) + return create_error(llvm::formatv("Could not find script class: %s", + class_name.data())); std::tuple original_args = std::forward_as_tuple(args...); auto transformed_args = TransformArgs(original_args); std::string error_string; - llvm::Expected arg_info = method.GetArgInfo(); + llvm::Expected arg_info = init.GetArgInfo(); if (!arg_info) { llvm::handleAllErrors( arg_info.takeError(), @@ -99,25 +135,87 @@ class ScriptedPythonInterface : virtual public ScriptedInterface { } llvm::Expected expected_return_object = - llvm::createStringError(llvm::inconvertibleErrorCode(), - "Resulting object is not initialized."); + create_error("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()) - return llvm::createStringError( - llvm::inconvertibleErrorCode(), - "Resulting object is not a valid Python Object."); + return create_error("Resulting object is not a valid Python Object."); + if (!result.HasAttribute("__class__")) + return create_error("Resulting object doesn't have '__class__' member."); + + PythonObject obj_class = result.GetAttributeValue("__class__"); + if (!obj_class.IsValid()) + return create_error("Resulting class object is not a valid."); + if (!obj_class.HasAttribute("__name__")) + return create_error( + "Resulting object class doesn't have '__name__' member."); + PythonString obj_class_name = + obj_class.GetAttributeValue("__name__").AsType(); + + PythonObject object_class_mapping_proxy = + obj_class.GetAttributeValue("__dict__"); + if (!obj_class.HasAttribute("__dict__")) + return create_error( + "Resulting object class doesn't have '__dict__' member."); + + PythonCallable dict_converter = PythonModule::BuiltinsModule() + .ResolveName("dict") + .AsType(); + if (!dict_converter.IsAllocated()) + return create_error( + "Python 'builtins' module doesn't have 'dict' class."); + + PythonDictionary object_class_dict = + dict_converter(object_class_mapping_proxy).AsType(); + if (!object_class_dict.IsAllocated()) + return create_error("Coudn't create dictionary from resulting object " + "class mapping proxy object."); + + auto checker_or_err = CheckAbstractMethodImplementation(object_class_dict); + if (!checker_or_err) + return checker_or_err.takeError(); + + for (const auto &method_checker : *checker_or_err) + switch (method_checker.second) { + case AbstractMethodCheckerCases::eNotImplemented: + LLDB_LOG(GetLog(LLDBLog::Script), + "Abstract method {0}.{1} not implemented.", + obj_class_name.GetString(), method_checker.first); + break; + case AbstractMethodCheckerCases::eNotAllocated: + LLDB_LOG(GetLog(LLDBLog::Script), + "Abstract method {0}.{1} not allocated.", + obj_class_name.GetString(), method_checker.first); + break; + case AbstractMethodCheckerCases::eNotCallable: + LLDB_LOG(GetLog(LLDBLog::Script), + "Abstract method {0}.{1} not callable.", + obj_class_name.GetString(), method_checker.first); + break; + case AbstractMethodCheckerCases::eValid: + LLDB_LOG(GetLog(LLDBLog::Script), + "Abstract method {0}.{1} implemented & valid.", + obj_class_name.GetString(), method_checker.first); + break; + } + + for (const auto &method_checker : *checker_or_err) + if (method_checker.second != AbstractMethodCheckerCases::eValid) + return create_error( + llvm::formatv("Abstract method {0}.{1} missing. Enable lldb " + "script log for more details.", + obj_class_name.GetString(), method_checker.first)); m_object_instance_sp = StructuredData::GenericSP( new StructuredPythonObject(std::move(result))); diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedThreadPythonInterface.h b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedThreadPythonInterface.h index b7b7439461a03..5676f7f1d6752 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 GetAbstractMethods() const override { + return llvm::SmallVector( + {"get_stop_reason", "get_register_context"}); + } + lldb::tid_t GetThreadID() override; std::optional GetName() override; diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp b/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp index fe3438c424715..ea0a1cdff40f1 100644 --- a/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp +++ b/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp @@ -663,6 +663,20 @@ 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.isSingleStringRef() ? key.getSingleStringRef() + : key.str()); + + 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 012f16e95e770..82eee76e42b27 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 { 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 1761122999f84..8c6b9c74bde47 100644 --- a/lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py +++ b/lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py @@ -60,6 +60,55 @@ 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)) + script_path = os.path.join( + self.getSourceDir(), "missing_methods_scripted_process.py" + ) + self.runCmd("command script import " + script_path) + + 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 0000000000000..a1db0640033e0 --- /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 0b816a8ae0a58..efb8f725f6739 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]);