From c2ad9f8b607931430e86da7420493599c48e62a0 Mon Sep 17 00:00:00 2001 From: Med Ismail Bennani Date: Tue, 7 Nov 2023 13:04:01 -0800 Subject: [PATCH] Revert "[lldb] Check for abstract methods implementation in Scripted Plugin Objects (#71260)" This reverts commit cc9ad72713405ef8f2468c7a714a137b4a3343ba since it breaks some tests upstream: https://lab.llvm.org/buildbot/#/builders/68/builds/63112 ******************** Failed Tests (4): lldb-api :: functionalities/gdb_remote_client/TestThreadSelectionBug.py lldb-api :: functionalities/plugins/python_os_plugin/TestPythonOSPlugin.py lldb-api :: functionalities/plugins/python_os_plugin/stepping_plugin_threads/TestOSPluginStepping.py lldb-api :: functionalities/postmortem/mach-core/TestMachCore.py --- .../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, 29 insertions(+), 251 deletions(-) delete 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 9753a916243b..e4816352daa5 100644 --- a/lldb/include/lldb/Interpreter/Interfaces/ScriptedInterface.h +++ b/lldb/include/lldb/Interpreter/Interfaces/ScriptedInterface.h @@ -30,8 +30,6 @@ public: 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 167149cbf5b8..7feaa01fe89b 100644 --- a/lldb/include/lldb/Interpreter/Interfaces/ScriptedPlatformInterface.h +++ b/lldb/include/lldb/Interpreter/Interfaces/ScriptedPlatformInterface.h @@ -24,10 +24,6 @@ public: 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 e41f488d201e..10203b1f8baa 100644 --- a/lldb/include/lldb/Interpreter/Interfaces/ScriptedProcessInterface.h +++ b/lldb/include/lldb/Interpreter/Interfaces/ScriptedProcessInterface.h @@ -26,10 +26,6 @@ public: 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 0520b6905e25..a7cfc690b67d 100644 --- a/lldb/include/lldb/Interpreter/Interfaces/ScriptedThreadInterface.h +++ b/lldb/include/lldb/Interpreter/Interfaces/ScriptedThreadInterface.h @@ -25,10 +25,6 @@ public: 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 e27f4dd28010..ee24e0ee30f9 100644 --- a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/OperatingSystemPythonInterface.h +++ b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/OperatingSystemPythonInterface.h @@ -29,13 +29,6 @@ public: 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 0842d3a00342..e04f2d02ab1d 100644 --- a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPlatformPythonInterface.h +++ b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPlatformPythonInterface.h @@ -28,12 +28,6 @@ public: 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 c75caa9340f2..f3cff619f662 100644 --- a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedProcessPythonInterface.h +++ b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedProcessPythonInterface.h @@ -29,11 +29,6 @@ public: 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 163659234466..7af981639709 100644 --- a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPythonInterface.h +++ b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPythonInterface.h @@ -32,45 +32,6 @@ public: 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, @@ -78,20 +39,20 @@ public: 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 create_error("Missing script class name."); + return llvm::createStringError(llvm::inconvertibleErrorCode(), + "Missing script class name."); else if (!has_interpreter_dict) - return create_error("Invalid script interpreter dictionary."); + return llvm::createStringError( + llvm::inconvertibleErrorCode(), + "Invalid script interpreter dictionary."); else - return create_error("Missing scripting object."); + return llvm::createStringError(llvm::inconvertibleErrorCode(), + "Missing scripting object."); } Locker py_lock(&m_interpreter, Locker::AcquireLock | Locker::NoSTDIN, @@ -106,23 +67,26 @@ public: auto dict = PythonModule::MainModule().ResolveName( m_interpreter.GetDictionaryName()); - if (!dict.IsAllocated()) - return create_error( - llvm::formatv("Could not find interpreter dictionary: %s", - m_interpreter.GetDictionaryName())); + if (!dict.IsAllocated()) { + return llvm::createStringError( + llvm::inconvertibleErrorCode(), + "Could not find interpreter dictionary: %s", + m_interpreter.GetDictionaryName()); + } - auto init = + auto method = PythonObject::ResolveNameWithDictionary( class_name, dict); - if (!init.IsAllocated()) - return create_error(llvm::formatv("Could not find script class: %s", - class_name.data())); + if (!method.IsAllocated()) + return llvm::createStringError(llvm::inconvertibleErrorCode(), + "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 = init.GetArgInfo(); + llvm::Expected arg_info = method.GetArgInfo(); if (!arg_info) { llvm::handleAllErrors( arg_info.takeError(), @@ -135,87 +99,25 @@ public: } llvm::Expected expected_return_object = - create_error("Resulting object is not initialized."); + llvm::createStringError(llvm::inconvertibleErrorCode(), + "Resulting object is not initialized."); std::apply( - [&init, &expected_return_object](auto &&...args) { + [&method, &expected_return_object](auto &&...args) { llvm::consumeError(expected_return_object.takeError()); - expected_return_object = init(args...); + expected_return_object = method(args...); }, transformed_args); - if (!expected_return_object) - return expected_return_object.takeError(); - result = expected_return_object.get(); + if (llvm::Error e = expected_return_object.takeError()) + return std::move(e); + result = std::move(expected_return_object.get()); } if (!result.IsValid()) - 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)); + return llvm::createStringError( + llvm::inconvertibleErrorCode(), + "Resulting object is not a valid Python Object."); 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 5676f7f1d675..b7b7439461a0 100644 --- a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedThreadPythonInterface.h +++ b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedThreadPythonInterface.h @@ -28,11 +28,6 @@ public: 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 ea0a1cdff40f..fe3438c42471 100644 --- a/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp +++ b/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.cpp @@ -663,20 +663,6 @@ 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 82eee76e42b2..012f16e95e77 100644 --- a/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h +++ b/lldb/source/Plugins/ScriptInterpreter/Python/PythonDataObjects.h @@ -562,8 +562,6 @@ public: 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 8c6b9c74bde4..1761122999f8 100644 --- a/lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py +++ b/lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py @@ -60,55 +60,6 @@ class ScriptedProcesTestCase(TestBase): ) 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 deleted file mode 100644 index a1db0640033e..000000000000 --- a/lldb/test/API/functionalities/scripted_process/missing_methods_scripted_process.py +++ /dev/null @@ -1,19 +0,0 @@ -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 efb8f725f673..0b816a8ae0a5 100644 --- a/lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp +++ b/lldb/unittests/ScriptInterpreter/Python/PythonDataObjectsTests.cpp @@ -504,9 +504,6 @@ 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]);