From 897dd358d3b15ec59e63068f7e328e914db49495 Mon Sep 17 00:00:00 2001 From: Med Ismail Bennani Date: Thu, 19 Sep 2024 13:25:06 -0700 Subject: [PATCH] [lldb/Interpreter] Add requirements to Scripted Interface abstract methods This patch adds new requirements to the Scripted Interface abstract method checker to check the minimum number of argument for abstract methods. This check is done when creating the interface object so the object is not created if the user implementation doesn't match the abstract method requirement. Signed-off-by: Med Ismail Bennani --- .../Interfaces/ScriptedInterface.h | 17 +- .../OperatingSystemPythonInterface.h | 5 +- .../ScriptedPlatformPythonInterface.h | 11 +- .../ScriptedProcessPythonInterface.h | 9 +- .../Interfaces/ScriptedPythonInterface.h | 160 +++++++++++++----- .../ScriptedThreadPlanPythonInterface.h | 3 +- .../ScriptedThreadPythonInterface.h | 7 +- 7 files changed, 158 insertions(+), 54 deletions(-) diff --git a/lldb/include/lldb/Interpreter/Interfaces/ScriptedInterface.h b/lldb/include/lldb/Interpreter/Interfaces/ScriptedInterface.h index 89c0b36d6fc2a..a3dc52c435561 100644 --- a/lldb/include/lldb/Interpreter/Interfaces/ScriptedInterface.h +++ b/lldb/include/lldb/Interpreter/Interfaces/ScriptedInterface.h @@ -31,7 +31,22 @@ class ScriptedInterface { return m_object_instance_sp; } - virtual llvm::SmallVector GetAbstractMethods() const = 0; + struct AbstractMethodRequirement { + llvm::StringLiteral name; + size_t min_arg_count = 0; + }; + + virtual llvm::SmallVector + GetAbstractMethodRequirements() const = 0; + + llvm::SmallVector const GetAbstractMethods() const { + llvm::SmallVector abstract_methods; + llvm::transform(GetAbstractMethodRequirements(), abstract_methods.begin(), + [](const AbstractMethodRequirement &requirement) { + return requirement.name; + }); + return abstract_methods; + } template static Ret ErrorWithMessage(llvm::StringRef caller_name, diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/OperatingSystemPythonInterface.h b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/OperatingSystemPythonInterface.h index 92358ac6c34f7..102c3c3953768 100644 --- a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/OperatingSystemPythonInterface.h +++ b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/OperatingSystemPythonInterface.h @@ -31,8 +31,9 @@ class OperatingSystemPythonInterface StructuredData::DictionarySP args_sp, StructuredData::Generic *script_obj = nullptr) override; - llvm::SmallVector GetAbstractMethods() const override { - return llvm::SmallVector({"get_thread_info"}); + llvm::SmallVector + GetAbstractMethodRequirements() const override { + return llvm::SmallVector({{"get_thread_info"}}); } StructuredData::DictionarySP CreateThread(lldb::tid_t tid, diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPlatformPythonInterface.h b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPlatformPythonInterface.h index 36a219a656993..a0da1bbc31c70 100644 --- a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPlatformPythonInterface.h +++ b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPlatformPythonInterface.h @@ -29,10 +29,13 @@ 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"}); + llvm::SmallVector + GetAbstractMethodRequirements() const override { + return llvm::SmallVector( + {{"list_processes"}, + {"attach_to_process", 2}, + {"launch_process", 2}, + {"kill_process", 2}}); } StructuredData::DictionarySP ListProcesses() override; diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedProcessPythonInterface.h b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedProcessPythonInterface.h index 1535d573e72f4..703b942fe5647 100644 --- a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedProcessPythonInterface.h +++ b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedProcessPythonInterface.h @@ -31,9 +31,12 @@ 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"}); + llvm::SmallVector + GetAbstractMethodRequirements() const override { + return llvm::SmallVector( + {{"read_memory_at_address", 4}, + {"is_alive"}, + {"get_scripted_thread_plugin"}}); } StructuredData::DictionarySP GetCapabilities() override; diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPythonInterface.h b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPythonInterface.h index cb6f6ec398926..c715295eb9ff0 100644 --- a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPythonInterface.h +++ b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPythonInterface.h @@ -36,37 +36,78 @@ class ScriptedPythonInterface : virtual public ScriptedInterface { eNotImplemented, eNotAllocated, eNotCallable, + eUnknownArgumentCount, + eInvalidArgumentCount, eValid }; - llvm::Expected> + struct AbstrackMethodCheckerPayload { + + struct InvalidArgumentCountPayload { + InvalidArgumentCountPayload(size_t required, size_t actual) + : required_argument_count(required), actual_argument_count(actual) {} + + size_t required_argument_count; + size_t actual_argument_count; + }; + + AbstractMethodCheckerCases checker_case; + std::variant payload; + }; + + llvm::Expected> CheckAbstractMethodImplementation( const python::PythonDictionary &class_dict) const { using namespace python; - std::map checker; -#define SET_ERROR_AND_CONTINUE(method_name, error) \ + std::map checker; +#define SET_CASE_AND_CONTINUE(method_name, case) \ { \ - checker[method_name] = error; \ + checker[method_name] = {case, {}}; \ continue; \ } - for (const llvm::StringLiteral &method_name : GetAbstractMethods()) { + for (const AbstractMethodRequirement &requirement : + GetAbstractMethodRequirements()) { + llvm::StringLiteral method_name = requirement.name; if (!class_dict.HasKey(method_name)) - SET_ERROR_AND_CONTINUE(method_name, - AbstractMethodCheckerCases::eNotImplemented) + SET_CASE_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; + if (!callable_or_err) { + llvm::consumeError(callable_or_err.takeError()); + SET_CASE_AND_CONTINUE(method_name, + AbstractMethodCheckerCases::eNotAllocated) + } + + PythonCallable callable = callable_or_err->AsType(); + if (!callable) + SET_CASE_AND_CONTINUE(method_name, + AbstractMethodCheckerCases::eNotCallable) + + if (!requirement.min_arg_count) + SET_CASE_AND_CONTINUE(method_name, AbstractMethodCheckerCases::eValid) + + auto arg_info_or_err = callable.GetArgInfo(); + if (!arg_info_or_err) { + llvm::consumeError(arg_info_or_err.takeError()); + SET_CASE_AND_CONTINUE(method_name, + AbstractMethodCheckerCases::eUnknownArgumentCount) + } + + PythonCallable::ArgInfo arg_info = *arg_info_or_err; + if (requirement.min_arg_count <= arg_info.max_positional_args) { + SET_CASE_AND_CONTINUE(method_name, AbstractMethodCheckerCases::eValid) + } else { + checker[method_name] = { + AbstractMethodCheckerCases::eInvalidArgumentCount, + AbstrackMethodCheckerPayload::InvalidArgumentCountPayload( + requirement.min_arg_count, arg_info.max_positional_args)}; + } } -#undef HANDLE_ERROR +#undef SET_CASE_AND_CONTINUE return checker; } @@ -78,8 +119,11 @@ class ScriptedPythonInterface : virtual public ScriptedInterface { using namespace python; using Locker = ScriptInterpreterPythonImpl::Locker; - auto create_error = [](std::string message) { - return llvm::createStringError(llvm::inconvertibleErrorCode(), message); + Log *log = GetLog(LLDBLog::Script); + auto create_error = [](llvm::StringLiteral format, auto &&...ts) { + return llvm::createStringError( + llvm::formatv(format.data(), std::forward(ts)...) + .str()); }; bool has_class_name = !class_name.empty(); @@ -107,16 +151,15 @@ class ScriptedPythonInterface : virtual public ScriptedInterface { PythonModule::MainModule().ResolveName( m_interpreter.GetDictionaryName()); if (!dict.IsAllocated()) - return create_error( - llvm::formatv("Could not find interpreter dictionary: %s", - m_interpreter.GetDictionaryName())); + return create_error("Could not find interpreter dictionary: {0}", + m_interpreter.GetDictionaryName()); auto init = PythonObject::ResolveNameWithDictionary( class_name, dict); if (!init.IsAllocated()) - return create_error(llvm::formatv("Could not find script class: {0}", - class_name.data())); + return create_error("Could not find script class: {0}", + class_name.data()); std::tuple original_args = std::forward_as_tuple(args...); auto transformed_args = TransformArgs(original_args); @@ -186,36 +229,73 @@ class ScriptedPythonInterface : virtual public ScriptedInterface { if (!checker_or_err) return checker_or_err.takeError(); + llvm::Error abstract_method_errors = llvm::Error::success(); for (const auto &method_checker : *checker_or_err) - switch (method_checker.second) { + switch (method_checker.second.checker_case) { case AbstractMethodCheckerCases::eNotImplemented: - LLDB_LOG(GetLog(LLDBLog::Script), - "Abstract method {0}.{1} not implemented.", - obj_class_name.GetString(), method_checker.first); + abstract_method_errors = llvm::joinErrors( + std::move(abstract_method_errors), + std::move(create_error("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); + abstract_method_errors = llvm::joinErrors( + std::move(abstract_method_errors), + std::move(create_error("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); + abstract_method_errors = llvm::joinErrors( + std::move(abstract_method_errors), + std::move(create_error("Abstract method {0}.{1} not callable.", + obj_class_name.GetString(), + method_checker.first))); + break; + case AbstractMethodCheckerCases::eUnknownArgumentCount: + abstract_method_errors = llvm::joinErrors( + std::move(abstract_method_errors), + std::move(create_error( + "Abstract method {0}.{1} has unknown argument count.", + obj_class_name.GetString(), method_checker.first))); break; + case AbstractMethodCheckerCases::eInvalidArgumentCount: { + auto &payload_variant = method_checker.second.payload; + if (!std::holds_alternative< + AbstrackMethodCheckerPayload::InvalidArgumentCountPayload>( + payload_variant)) { + abstract_method_errors = llvm::joinErrors( + std::move(abstract_method_errors), + std::move(create_error( + "Abstract method {0}.{1} has unexpected argument count.", + obj_class_name.GetString(), method_checker.first))); + } else { + auto payload = std::get< + AbstrackMethodCheckerPayload::InvalidArgumentCountPayload>( + payload_variant); + abstract_method_errors = llvm::joinErrors( + std::move(abstract_method_errors), + std::move( + create_error("Abstract method {0}.{1} has unexpected " + "argument count (expected {2} but has {3}).", + obj_class_name.GetString(), method_checker.first, + payload.required_argument_count, + payload.actual_argument_count))); + } + } break; case AbstractMethodCheckerCases::eValid: - LLDB_LOG(GetLog(LLDBLog::Script), - "Abstract method {0}.{1} implemented & valid.", + LLDB_LOG(log, "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)); + if (abstract_method_errors) { + Status error = Status::FromError(std::move(abstract_method_errors)); + LLDB_LOG(log, "Abstract method error in {0}:\n{1}", class_name, + error.AsCString()); + return error.ToError(); + } m_object_instance_sp = StructuredData::GenericSP( new StructuredPythonObject(std::move(result))); diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedThreadPlanPythonInterface.h b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedThreadPlanPythonInterface.h index 5e78ae764eeb5..2d017396df7e2 100644 --- a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedThreadPlanPythonInterface.h +++ b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedThreadPlanPythonInterface.h @@ -30,7 +30,8 @@ class ScriptedThreadPlanPythonInterface : public ScriptedThreadPlanInterface, lldb::ThreadPlanSP thread_plan_sp, const StructuredDataImpl &args_sp) override; - llvm::SmallVector GetAbstractMethods() const override { + llvm::SmallVector + GetAbstractMethodRequirements() const override { return {}; } diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedThreadPythonInterface.h b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedThreadPythonInterface.h index 5676f7f1d6752..1fb23b39c7076 100644 --- a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedThreadPythonInterface.h +++ b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedThreadPythonInterface.h @@ -28,9 +28,10 @@ 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"}); + llvm::SmallVector + GetAbstractMethodRequirements() const override { + return llvm::SmallVector( + {{"get_stop_reason"}, {"get_register_context"}}); } lldb::tid_t GetThreadID() override;