llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-lldb Author: Med Ismail Bennani (medismailben) <details> <summary>Changes</summary> When calling scripted affordance methods, Python exceptions were silently consumed by llvm::consumeError(), making it very hard to debug scripted extensions: users would see a generic "Failed to create script object." or just a null result, with no clue about what raised in their script. This patch propagates the actual Python error through the existing error-reporting channel of each call site: - The Dispatch and CallStaticMethod templates now extract the full Python backtrace via PythonException::ReadBacktrace and populate the Status out-parameter with a message of the form "Python exception in <script-class> method '<method>':\n<traceback>". - ErrorWithMessage no longer overwrites a Status that already has detailed content, so the traceback survives the helper's existing logging. - ScriptedInterface now holds the ScriptedMetadata (set by each concrete CreatePluginObject) so the error message can name the script class. - For entry points that have no return-channel for errors (ScriptedProcess::CreateInstance, ScriptedThreadPlan::DidPush, BreakpointResolverScripted, OperatingSystemPython's constructor) the detailed error is broadcast via Debugger::ReportError so users see it instead of a silent failure. - For entry points that already return llvm::Expected<T> or Status (ScriptedThread::Create, ScriptedFrameProvider::CreateInstance, StopHookScripted::SetScriptCallback) the existing return type now carries the detailed error, replacing the previous consumeError + generic-message pattern. - ScriptedProcess::DoAttach / DoReadMemory / DoWriteMemory wrap their affordance Status with the operation that failed. A new test directory at lldb/test/API/functionalities/scripted_extensions/ listens for eBroadcastBitError diagnostics and asserts that each malformed scripted extension produces a user-visible error. --- Patch is 37.86 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/198153.diff 17 Files Affected: - (modified) lldb/include/lldb/Interpreter/Interfaces/ScriptedBreakpointInterface.h (+1-1) - (modified) lldb/include/lldb/Interpreter/Interfaces/ScriptedFrameProviderInterface.h (+1-1) - (modified) lldb/include/lldb/Interpreter/Interfaces/ScriptedInterface.h (+25-10) - (modified) lldb/include/lldb/Interpreter/Interfaces/ScriptedStopHookInterface.h (+1-1) - (modified) lldb/include/lldb/Interpreter/ScriptInterpreter.h (+4) - (modified) lldb/source/Breakpoint/BreakpointResolverScripted.cpp (+7-1) - (modified) lldb/source/Plugins/OperatingSystem/Python/OperatingSystemPython.cpp (+7-1) - (modified) lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp (+54-6) - (modified) lldb/source/Plugins/Process/scripted/ScriptedProcess.h (+12-2) - (modified) lldb/source/Plugins/Process/scripted/ScriptedThread.cpp (+2-5) - (modified) lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPythonInterface.h (+124-8) - (modified) lldb/source/Target/ScriptedThreadPlan.cpp (+5) - (added) lldb/test/API/functionalities/scripted_extensions/Makefile (+3) - (added) lldb/test/API/functionalities/scripted_extensions/TestScriptedExtensionsDiagnostics.py (+124) - (added) lldb/test/API/functionalities/scripted_extensions/main.c (+1) - (added) lldb/test/API/functionalities/scripted_extensions/malformed_scripted_extensions.py (+195) - (modified) lldb/test/API/functionalities/scripted_process/TestScriptedProcess.py (+5-1) ``````````diff diff --git a/lldb/include/lldb/Interpreter/Interfaces/ScriptedBreakpointInterface.h b/lldb/include/lldb/Interpreter/Interfaces/ScriptedBreakpointInterface.h index 982f231d9f0b2..7328aa26db65d 100644 --- a/lldb/include/lldb/Interpreter/Interfaces/ScriptedBreakpointInterface.h +++ b/lldb/include/lldb/Interpreter/Interfaces/ScriptedBreakpointInterface.h @@ -15,7 +15,7 @@ #include "lldb/lldb-private.h" namespace lldb_private { -class ScriptedBreakpointInterface : public ScriptedInterface { +class ScriptedBreakpointInterface : virtual public ScriptedInterface { public: virtual llvm::Expected<StructuredData::GenericSP> CreatePluginObject(const ScriptedMetadata &scripted_metadata, diff --git a/lldb/include/lldb/Interpreter/Interfaces/ScriptedFrameProviderInterface.h b/lldb/include/lldb/Interpreter/Interfaces/ScriptedFrameProviderInterface.h index 757a99cb2387b..8c94df430c103 100644 --- a/lldb/include/lldb/Interpreter/Interfaces/ScriptedFrameProviderInterface.h +++ b/lldb/include/lldb/Interpreter/Interfaces/ScriptedFrameProviderInterface.h @@ -14,7 +14,7 @@ #include "ScriptedInterface.h" namespace lldb_private { -class ScriptedFrameProviderInterface : public ScriptedInterface { +class ScriptedFrameProviderInterface : virtual public ScriptedInterface { public: virtual bool AppliesToThread(llvm::StringRef class_name, lldb::ThreadSP thread_sp) { diff --git a/lldb/include/lldb/Interpreter/Interfaces/ScriptedInterface.h b/lldb/include/lldb/Interpreter/Interfaces/ScriptedInterface.h index 3dbc009a58311..4fc1e5860702b 100644 --- a/lldb/include/lldb/Interpreter/Interfaces/ScriptedInterface.h +++ b/lldb/include/lldb/Interpreter/Interfaces/ScriptedInterface.h @@ -20,6 +20,7 @@ #include "llvm/Support/Compiler.h" +#include <functional> #include <optional> #include <string> @@ -37,6 +38,17 @@ class ScriptedInterface { return m_scripted_metadata; } + /// Set error callback to surface Python exceptions directly to users. + /// + /// This allows command handlers to receive Python exception details + /// immediately rather than relying on diagnostic broadcasts. + /// + /// \param callback Function to call with Status containing exception details. + virtual void SetErrorCallback(std::function<void(const Status &)> callback) {} + + /// Clear the error callback. + virtual void ClearErrorCallback() {} + struct AbstractMethodRequirement { llvm::StringLiteral name; size_t min_arg_count = 0; @@ -62,18 +74,20 @@ class ScriptedInterface { static Ret ErrorWithMessage(llvm::StringRef caller_name, llvm::StringRef error_msg, Status &error, LLDBLog log_category = LLDBLog::Process) { + // Log the error for debugging (includes function signature for context). LLDB_LOGF(GetLog(log_category), "%s ERROR = %s", caller_name.data(), error_msg.data()); - std::string full_error_message = - llvm::Twine(caller_name + llvm::Twine(" ERROR = ") + - llvm::Twine(error_msg)) - .str(); - if (const char *detailed_error = error.AsCString()) - full_error_message += - llvm::Twine(llvm::Twine(" (") + llvm::Twine(detailed_error) + - llvm::Twine(")")) - .str(); - error = Status(std::move(full_error_message)); + + // For user-facing messages, just pass through the Status if it already + // has detailed information (like Python tracebacks); otherwise set it. + llvm::StringRef existing_error = error.AsCString(); + if (!error.Fail() || existing_error.empty()) { + // Status is empty, populate it with the simple error message. + error = Status::FromErrorString(error_msg.data()); + } + // If Status already has content, leave it as-is (it has the Python + // traceback). + return {}; } @@ -105,4 +119,5 @@ class ScriptedInterface { std::optional<ScriptedMetadata> m_scripted_metadata; }; } // namespace lldb_private + #endif // LLDB_INTERPRETER_INTERFACES_SCRIPTEDINTERFACE_H diff --git a/lldb/include/lldb/Interpreter/Interfaces/ScriptedStopHookInterface.h b/lldb/include/lldb/Interpreter/Interfaces/ScriptedStopHookInterface.h index c68498cba1632..ba325cfdf38b2 100644 --- a/lldb/include/lldb/Interpreter/Interfaces/ScriptedStopHookInterface.h +++ b/lldb/include/lldb/Interpreter/Interfaces/ScriptedStopHookInterface.h @@ -14,7 +14,7 @@ #include "ScriptedInterface.h" namespace lldb_private { -class ScriptedStopHookInterface : public ScriptedInterface { +class ScriptedStopHookInterface : virtual public ScriptedInterface { public: virtual llvm::Expected<StructuredData::GenericSP> CreatePluginObject(const ScriptedMetadata &scripted_metadata, diff --git a/lldb/include/lldb/Interpreter/ScriptInterpreter.h b/lldb/include/lldb/Interpreter/ScriptInterpreter.h index 58af82fb48390..2cc3890d86916 100644 --- a/lldb/include/lldb/Interpreter/ScriptInterpreter.h +++ b/lldb/include/lldb/Interpreter/ScriptInterpreter.h @@ -663,6 +663,10 @@ class ScriptInterpreter : public PluginInterface { lldb::TargetSP GetOpaqueTypeFromSBTarget(const lldb::SBTarget &target) const; + /// Get the debugger associated with this script interpreter. + Debugger &GetDebugger() { return m_debugger; } + const Debugger &GetDebugger() const { return m_debugger; } + protected: Debugger &m_debugger; lldb::ScriptLanguage m_script_lang; diff --git a/lldb/source/Breakpoint/BreakpointResolverScripted.cpp b/lldb/source/Breakpoint/BreakpointResolverScripted.cpp index 0719c8b634cd3..0fdb185537c9a 100644 --- a/lldb/source/Breakpoint/BreakpointResolverScripted.cpp +++ b/lldb/source/Breakpoint/BreakpointResolverScripted.cpp @@ -20,6 +20,7 @@ #include "lldb/Target/Target.h" #include "lldb/Utility/Log.h" #include "lldb/Utility/StreamString.h" +#include "llvm/Support/FormatVariadic.h" using namespace lldb; using namespace lldb_private; @@ -94,7 +95,12 @@ void BreakpointResolverScripted::CreateImplementationIfNeeded( m_interface_sp->CreatePluginObject(scripted_metadata, breakpoint_sp); if (!obj_or_err) { m_interface_sp.reset(); - m_error = Status::FromError(obj_or_err.takeError()); + std::string msg = llvm::toString(obj_or_err.takeError()); + Debugger::ReportError( + llvm::formatv("Failed to create BreakpointResolverScripted: {0}", msg) + .str(), + target.GetDebugger().GetID()); + m_error = Status(msg); return; } StructuredData::ObjectSP object_sp = *obj_or_err; diff --git a/lldb/source/Plugins/OperatingSystem/Python/OperatingSystemPython.cpp b/lldb/source/Plugins/OperatingSystem/Python/OperatingSystemPython.cpp index 49df19906929f..662b8117041bc 100644 --- a/lldb/source/Plugins/OperatingSystem/Python/OperatingSystemPython.cpp +++ b/lldb/source/Plugins/OperatingSystem/Python/OperatingSystemPython.cpp @@ -33,6 +33,7 @@ #include "lldb/Utility/StreamString.h" #include "lldb/Utility/StructuredData.h" #include "lldb/ValueObject/ValueObjectVariable.h" +#include "llvm/Support/FormatVariadic.h" #include <memory> @@ -120,7 +121,12 @@ OperatingSystemPython::OperatingSystemPython(lldb_private::Process *process, scripted_metadata, exe_ctx, nullptr); if (!obj_or_err) { - llvm::consumeError(obj_or_err.takeError()); + std::string msg = llvm::toString(obj_or_err.takeError()); + if (process) + Debugger::ReportError( + llvm::formatv("Failed to create OperatingSystemPython: {0}", msg) + .str(), + process->GetTarget().GetDebugger().GetID()); return; } diff --git a/lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp b/lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp index 842eef9524328..b0418307cdebb 100644 --- a/lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp +++ b/lldb/source/Plugins/Process/scripted/ScriptedProcess.cpp @@ -27,6 +27,10 @@ #include "Plugins/ObjectFile/Placeholder/ObjectFilePlaceholder.h" +#include "llvm/Support/Error.h" + +#include <string> + using namespace lldb; using namespace lldb_private; @@ -57,11 +61,27 @@ lldb::ProcessSP ScriptedProcess::CreateInstance(lldb::TargetSP target_sp, ScriptedMetadata scripted_metadata(target_sp->GetProcessLaunchInfo()); + // CreateInstance is invoked for every process plugin during process + // creation; if the launch info doesn't request a scripted process, bail + // out silently rather than treating the missing class name as an error. + if (!scripted_metadata) + return nullptr; + Status error; auto process_sp = std::shared_ptr<ScriptedProcess>( new ScriptedProcess(target_sp, listener_sp, scripted_metadata, error)); if (error.Fail() || !process_sp || !process_sp->m_interface_up) { + // CreateInstance returns nullptr on failure with no Status output + // parameter, so we must report the error via the diagnostic system for + // users to see it. + if (error.Fail()) { + Debugger::ReportError( + llvm::formatv("Failed to create ScriptedProcess: {0}", + error.AsCString()) + .str(), + target_sp->GetDebugger().GetID()); + } LLDB_LOGF(GetLog(LLDBLog::Process), "%s", error.AsCString()); return nullptr; } @@ -112,8 +132,10 @@ ScriptedProcess::ScriptedProcess(lldb::TargetSP target_sp, GetInterface().CreatePluginObject(m_scripted_metadata, exe_ctx); if (!obj_or_err) { - llvm::consumeError(obj_or_err.takeError()); - error = Status::FromErrorString("Failed to create script object."); + // Extract the detailed error message including the Python backtrace. + std::string error_msg = llvm::toString(obj_or_err.takeError()); + error = Status::FromErrorStringWithFormatv( + "Failed to create script object: {0}", error_msg); return; } @@ -189,10 +211,13 @@ Status ScriptedProcess::DoResume(RunDirection direction) { Status ScriptedProcess::DoAttach(const ProcessAttachInfo &attach_info) { Status error = GetInterface().Attach(attach_info); + if (error.Fail()) { + error = Status::FromErrorStringWithFormatv( + "Failed to attach to scripted process: {0}", error.AsCString()); + return error; + } SetPrivateState(eStateRunning); SetPrivateState(eStateStopped); - if (error.Fail()) - return error; // NOTE: We need to set the PID before finishing to attach otherwise we will // hit an assert when calling the attach completion handler. DidLaunch(); @@ -224,8 +249,14 @@ size_t ScriptedProcess::DoReadMemory(lldb::addr_t addr, void *buf, size_t size, lldb::DataExtractorSP data_extractor_sp = GetInterface().ReadMemoryAtAddress(addr, size, error); - if (!data_extractor_sp || !data_extractor_sp->HasData() || error.Fail()) + if (!data_extractor_sp || !data_extractor_sp->HasData() || error.Fail()) { + if (error.Fail()) { + error = Status::FromErrorStringWithFormatv( + "Failed to read memory from scripted process at 0x{0:x-}: {1}", addr, + error.AsCString()); + } return 0; + } offset_t bytes_copied = data_extractor_sp->CopyByteOrderedData( 0, data_extractor_sp->GetByteSize(), buf, size, GetByteOrder()); @@ -251,9 +282,15 @@ size_t ScriptedProcess::DoWriteMemory(lldb::addr_t vm_addr, const void *buf, lldb::offset_t bytes_written = GetInterface().WriteMemoryAtAddress(vm_addr, data_extractor_sp, error); - if (!bytes_written || bytes_written == LLDB_INVALID_OFFSET) + if (!bytes_written || bytes_written == LLDB_INVALID_OFFSET) { + if (error.Fail()) { + error = Status::FromErrorStringWithFormatv( + "Failed to write memory to scripted process at 0x{0:x-}: {1}", vm_addr, + error.AsCString()); + } return ScriptedInterface::ErrorWithMessage<size_t>( LLVM_PRETTY_FUNCTION, "Failed to copy write buffer to memory.", error); + } // FIXME: We should use the diagnostic system to report a warning if the // `bytes_written` is different from `size`. @@ -566,3 +603,14 @@ void *ScriptedProcess::GetImplementation() { return object_instance_sp->GetAsGeneric()->GetValue(); return nullptr; } + +void ScriptedProcess::SetScriptedInterfaceErrorCallback( + std::function<void(const Status &)> callback) { + if (m_interface_up) + m_interface_up->SetErrorCallback(std::move(callback)); +} + +void ScriptedProcess::ClearScriptedInterfaceErrorCallback() { + if (m_interface_up) + m_interface_up->ClearErrorCallback(); +} diff --git a/lldb/source/Plugins/Process/scripted/ScriptedProcess.h b/lldb/source/Plugins/Process/scripted/ScriptedProcess.h index 8371180734217..9ce4a3f8d3cc9 100644 --- a/lldb/source/Plugins/Process/scripted/ScriptedProcess.h +++ b/lldb/source/Plugins/Process/scripted/ScriptedProcess.h @@ -17,8 +17,6 @@ #include "ScriptedThread.h" -#include <mutex> - namespace lldb_private { class ScriptedProcess : public Process { public: @@ -93,6 +91,18 @@ class ScriptedProcess : public Process { void *GetImplementation() override; + /// Set error callback to surface Python exceptions directly to users. + /// + /// This allows command handlers to receive Python exception details + /// immediately rather than relying on diagnostic broadcasts. + /// + /// \param callback Function to call with Status containing exception details. + void SetScriptedInterfaceErrorCallback( + std::function<void(const Status &)> callback); + + /// Clear the error callback. + void ClearScriptedInterfaceErrorCallback(); + void ForceScriptedState(lldb::StateType state) override { // If we're about to stop, we should fetch the loaded dynamic libraries // dictionary before emitting the private stop event to avoid having the diff --git a/lldb/source/Plugins/Process/scripted/ScriptedThread.cpp b/lldb/source/Plugins/Process/scripted/ScriptedThread.cpp index f87b37ca67e09..ac5dd00483f3b 100644 --- a/lldb/source/Plugins/Process/scripted/ScriptedThread.cpp +++ b/lldb/source/Plugins/Process/scripted/ScriptedThread.cpp @@ -69,11 +69,8 @@ ScriptedThread::Create(ScriptedProcess &process, auto obj_or_err = scripted_thread_interface->CreatePluginObject( thread_metadata, exe_ctx, script_object); - if (!obj_or_err) { - llvm::consumeError(obj_or_err.takeError()); - return llvm::createStringError(llvm::inconvertibleErrorCode(), - "Failed to create script object."); - } + if (!obj_or_err) + return obj_or_err.takeError(); StructuredData::GenericSP owned_script_object_sp = *obj_or_err; diff --git a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPythonInterface.h b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPythonInterface.h index 7d0d4cdd3c6d1..5608d5d35d654 100644 --- a/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPythonInterface.h +++ b/lldb/source/Plugins/ScriptInterpreter/Python/Interfaces/ScriptedPythonInterface.h @@ -9,12 +9,14 @@ #ifndef LLDB_SOURCE_PLUGINS_SCRIPTINTERPRETER_PYTHON_INTERFACES_SCRIPTEDPYTHONINTERFACE_H #define LLDB_SOURCE_PLUGINS_SCRIPTINTERPRETER_PYTHON_INTERFACES_SCRIPTEDPYTHONINTERFACE_H +#include <functional> #include <optional> #include <sstream> #include <tuple> #include <type_traits> #include <utility> +#include "lldb/Core/Debugger.h" #include "lldb/Interpreter/Interfaces/ScriptedInterface.h" #include "lldb/Utility/DataBufferHeap.h" @@ -29,6 +31,22 @@ class ScriptedPythonInterface : virtual public ScriptedInterface { ScriptedPythonInterface(ScriptInterpreterPythonImpl &interpreter); ~ScriptedPythonInterface() override = default; + /// Set callback to surface Python exceptions to CommandReturnObject. + /// + /// When set, this callback will be invoked whenever a Python exception occurs + /// in scripting affordance methods, allowing errors to be surfaced directly + /// to the user via CommandReturnObject::AppendError(). + /// + /// If no callback is registered, errors will be reported via + /// Debugger::ReportError() instead. + /// + /// \param callback Function to call with Status containing exception details. + using ErrorCallback = std::function<void(const Status &)>; + void SetErrorCallback(ErrorCallback callback) override { + m_error_callback = std::move(callback); + } + void ClearErrorCallback() override { m_error_callback = nullptr; } + enum class AbstractMethodCheckerCases { eNotImplemented, eNotAllocated, @@ -130,7 +148,15 @@ class ScriptedPythonInterface : virtual public ScriptedInterface { llvm::Expected<PythonObject> callable_or_err = class_dict.GetItem(method_name); if (!callable_or_err) { - llvm::consumeError(callable_or_err.takeError()); + Log *log = GetLog(LLDBLog::Script); + if (log) { + std::string error_msg = + ExtractPythonError(callable_or_err.takeError()); + LLDB_LOGF(log, "Failed to get method '%s': %s", method_name.data(), + error_msg.c_str()); + } else { + llvm::consumeError(callable_or_err.takeError()); + } SET_CASE_AND_CONTINUE(method_name, AbstractMethodCheckerCases::eNotAllocated) } @@ -145,7 +171,15 @@ class ScriptedPythonInterface : virtual public ScriptedInterface { auto arg_info_or_err = callable.GetArgInfo(); if (!arg_info_or_err) { - llvm::consumeError(arg_info_or_err.takeError()); + Log *log = GetLog(LLDBLog::Script); + if (log) { + std::string error_msg = + ExtractPythonError(arg_info_or_err.takeError()); + LLDB_LOGF(log, "Failed to get arg info for method '%s': %s", + method_name.data(), error_msg.c_str()); + } else { + llvm::consumeError(arg_info_or_err.takeError()); + } SET_CASE_AND_CONTINUE(method_name, AbstractMethodCheckerCases::eUnknownArgumentCount) } @@ -258,14 +292,18 @@ class ScriptedPythonInterface : virtual public ScriptedInterface { std::apply( [&init, &expected_return_object](auto &&...args) { - llvm::consumeError(expected_return_object.takeError()); + // Consume placeholder error (expected initial state). + if (!expected_return_object) + llvm::consumeError(expected_return_object.takeError()); expected_return_object = init(args...); }, std::tuple_cat(transformed_args, std::make_tuple(dict))); } else { std::apply( [&init, &expected_return_object](auto &&...args) { - llvm::consumeError(expected_return_object.takeError()); + // Consume placeholder error (expected initial state). + if (!expected_return_object) + llvm::consumeError(expected_return_object.takeError()); expected_return_object = init(args...); }, transformed_args); @@ -467,13 +505,41 @@ class ScriptedPythonInterface : virtual public ScriptedInterface { llvm::createStringError("not initialized"); std::apply( [&method, &expected_return_object](auto &&...args) { - llvm::consumeError(expected_return_object.takeError()); + // Consume placeholder error (expected initial state). + if (!expected_return_object) + llvm::consumeError(expected_return_object.takeError()); expected_return_object = method(args...); }, transformed_args); if (llvm::Error e = expected_return_object.takeError()) { - error = Status::FromError(std::move(e)); + // Extract Python backtrace and log it. + std::string detailed_error = ExtractPythonError(std::move(e)); + + Log *log = GetLog(LLDBLog::Script); + if (log) { + LLDB_LOGF(log, "%s: Python exception in static method %s:\n%s", + caller_signature.c_str(), method_name.data(), + detailed_error.c_str()); + } + + // Create Status with full context including the interface type. + // TODO: Stringify `args` and include them in the message so users + // can see what was passed to the failing call (e.g. + // `read_memory_at_address(0x500000000, 4)`). Requires a SFINAE + // helper that falls back to a placeholder for types without a + // format_provider / o... [truncated] `````````` </details> https://github.com/llvm/llvm-project/pull/198153 _______________________________________________ lldb-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
