From e01511429dd98e9b6cba9ee2c24910e14917f0ea Mon Sep 17 00:00:00 2001 From: Peace-Maker Date: Sun, 15 Apr 2018 21:31:19 +0200 Subject: [PATCH 01/13] Reenable IPluginContext::SetDebugBreak Call the given callback on every `dbreak` instruction in the code stream - which is emitted for every line break in the code. --- include/sp_vm_api.h | 11 +++++---- include/sp_vm_types.h | 12 ++++++++++ vm/AMBuilder | 1 + vm/base-context.cpp | 2 +- vm/base-context.h | 2 +- vm/debugging.cpp | 56 +++++++++++++++++++++++++++++++++++++++++++ vm/debugging.h | 29 ++++++++++++++++++++++ vm/environment.cpp | 5 ++++ vm/interpreter.cpp | 8 +++++++ vm/interpreter.h | 1 + vm/jit.cpp | 3 +++ vm/jit.h | 4 ++++ vm/pcode-reader.h | 6 ++--- vm/pcode-visitor.h | 5 ++++ vm/plugin-context.cpp | 13 ++++++++++ vm/plugin-context.h | 6 +++++ vm/stack-frames.cpp | 6 +++++ vm/stack-frames.h | 2 ++ vm/watchdog_timer.cpp | 4 +++- vm/watchdog_timer.h | 9 +++++++ vm/x86/jit_x86.cpp | 34 ++++++++++++++++++++++++++ vm/x86/jit_x86.h | 2 ++ 22 files changed, 210 insertions(+), 11 deletions(-) create mode 100644 vm/debugging.cpp create mode 100644 vm/debugging.h diff --git a/include/sp_vm_api.h b/include/sp_vm_api.h index 9e1416183..0501e4c14 100644 --- a/include/sp_vm_api.h +++ b/include/sp_vm_api.h @@ -611,12 +611,15 @@ namespace SourcePawn virtual bool IsDebugging() =0; /** - * @brief Deprecated, does nothing. + * @brief Installs a debug break and returns the old one, if any. + * This will fail if the plugin is not debugging. + * + * @param newpfn New function pointer. + * @param oldpfn Pointer to retrieve old function pointer. * - * @param newpfn Unused. - * @param oldpfn Unused. + * @return Error number. */ - virtual int SetDebugBreak(void *newpfn, void *oldpfn) =0; + virtual int SetDebugBreak(SPVM_DEBUGBREAK newpfn, SPVM_DEBUGBREAK *oldpfn) =0; /** * @brief Deprecated, do not use. diff --git a/include/sp_vm_types.h b/include/sp_vm_types.h index b3cb65e7d..d4964d8cd 100644 --- a/include/sp_vm_types.h +++ b/include/sp_vm_types.h @@ -109,6 +109,7 @@ namespace SourcePawn class IPluginContext; class IVirtualMachine; class IProfiler; + class IErrorReport; }; struct sp_context_s; @@ -246,4 +247,15 @@ typedef struct sp_debug_symbol_s sp_debug_symbol_raw_t *sym; /**< Pointer to original symbol */ } sp_debug_symbol_t; +/** + * Breaks into a debugger + * Params: + * [0] - plugin context + * [1] - frm + * [2] - cip + * [3] - memory + * [4] - exception + */ +typedef void(*SPVM_DEBUGBREAK)(SourcePawn::IPluginContext *, cell_t, cell_t, void *, const SourcePawn::IErrorReport *); + #endif //_INCLUDE_SOURCEPAWN_VM_TYPES_H diff --git a/vm/AMBuilder b/vm/AMBuilder index 05319e5f9..727dfb078 100644 --- a/vm/AMBuilder +++ b/vm/AMBuilder @@ -53,6 +53,7 @@ library.sources += [ 'code-stubs.cpp', 'control-flow.cpp', 'compiled-function.cpp', + 'debugging.cpp', 'environment.cpp', 'file-utils.cpp', 'graph-builder.cpp', diff --git a/vm/base-context.cpp b/vm/base-context.cpp index a20e5dd41..1dc2c0dc2 100644 --- a/vm/base-context.cpp +++ b/vm/base-context.cpp @@ -74,7 +74,7 @@ BasePluginContext::IsDebugging() } int -BasePluginContext::SetDebugBreak(void* newpfn, void* oldpfn) +BasePluginContext::SetDebugBreak(SPVM_DEBUGBREAK newpfn, SPVM_DEBUGBREAK *oldpfn) { return SP_ERROR_ABORTED; } diff --git a/vm/base-context.h b/vm/base-context.h index a10cb70df..05aa8727e 100644 --- a/vm/base-context.h +++ b/vm/base-context.h @@ -43,6 +43,7 @@ class BasePluginContext : public SourcePawn::IPluginContext cell_t BlamePluginError(SourcePawn::IPluginFunction* pf, const char* msg, ...) override; IFrameIterator* CreateFrameIterator() override; void DestroyFrameIterator(IFrameIterator* it) override; + int SetDebugBreak(SPVM_DEBUGBREAK newpfn, SPVM_DEBUGBREAK *oldpfn) override; // Removed functions. int PushCell(cell_t value) override; @@ -56,7 +57,6 @@ class BasePluginContext : public SourcePawn::IPluginContext SourcePawn::IVirtualMachine* GetVirtualMachine() override; sp_context_t* GetContext() override; bool IsDebugging() override; - int SetDebugBreak(void* newpfn, void* oldpfn) override; SourcePawn::IPluginDebugInfo* GetDebugInfo() override; int Execute(uint32_t code_addr, cell_t* result) override; int Execute2(SourcePawn::IPluginFunction* function, const cell_t* params, unsigned int num_params, cell_t* result) override; diff --git a/vm/debugging.cpp b/vm/debugging.cpp new file mode 100644 index 000000000..9818cecb8 --- /dev/null +++ b/vm/debugging.cpp @@ -0,0 +1,56 @@ +// vim: set sts=2 ts=8 sw=2 tw=99 et: +// +// Copyright (C) 2016-2018 AlliedModders LLC +// +// This file is part of SourcePawn. SourcePawn is free software: you can +// redistribute it and/or modify it under the terms of the GNU General Public +// License as published by the Free Software Foundation, either version 3 of +// the License, or (at your option) any later version. +// +// You should have received a copy of the GNU General Public License along with +// SourcePawn. If not, see http://www.gnu.org/licenses/. +// +#include "debugging.h" +#include "stack-frames.h" +#include "environment.h" +#include "watchdog_timer.h" +#include + +namespace sp { + +void InvokeDebugger(PluginContext *ctx, const IErrorReport *report) +{ + // Continue normal execution, if this plugin isn't being debugged. + if (!ctx->debugbreak()) + return; + + if (!ctx->IsDebugging()) { + ctx->ReportErrorNumber(SP_ERROR_NOTDEBUGGING); + return; + } + + cell_t cip = 0; + + // Find first scripted frame on the stack to get the cip from. + // There might be some native or helper frames beforehand. + { + FrameIterator iter; + for (; !iter.Done(); iter.Next()) { + if (iter.IsScriptedFrame()) { + cip = iter.cip(); + break; + } + } + } + + // Tell the watchdog to take a break. + // We might stay in the debugger callback for a while, + // so don't let the watchdog hit immediately after + // continueing with execution. + ke::SaveAndSet(&Environment::get()->watchdog()->ignore_timeout_, true); + + // Call debug callback. + ctx->debugbreak()(ctx, ctx->frm(), cip, ctx->memory(), report); +} + +} // namespace sp \ No newline at end of file diff --git a/vm/debugging.h b/vm/debugging.h new file mode 100644 index 000000000..cf4a3e2d1 --- /dev/null +++ b/vm/debugging.h @@ -0,0 +1,29 @@ +// vim: set ts=8 sts=2 sw=2 tw=99 et: +// +// This file is part of SourcePawn. +// +// SourcePawn is free software: you can redistribute it and/or modify +// it under the terms of the GNU General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// SourcePawn is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU General Public License for more details. +// +// You should have received a copy of the GNU General Public License +// along with SourcePawn. If not, see . +// +#ifndef _include_sourcepawn_vm_debugging_h_ +#define _include_sourcepawn_vm_debugging_h_ + +#include "plugin-context.h" + +namespace sp { + +void InvokeDebugger(PluginContext *ctx, const IErrorReport *report); + +} // namespace sp + +#endif // _include_sourcepawn_vm_debugging_h_ \ No newline at end of file diff --git a/vm/environment.cpp b/vm/environment.cpp index 6e4b194b4..a404f4b96 100644 --- a/vm/environment.cpp +++ b/vm/environment.cpp @@ -24,6 +24,7 @@ #endif #include "interpreter.h" #include "builtins.h" +#include "debugging.h" #include using namespace sp; @@ -410,6 +411,10 @@ Environment::DispatchReport(const ErrorReport& report) // For now, we always report exceptions even if they might be handled. if (debugger_) debugger_->ReportError(report, iter); + + // See if the plugin is being debugged + if (top_) + InvokeDebugger(top_->cx(), &report); } void diff --git a/vm/interpreter.cpp b/vm/interpreter.cpp index 7d77eba2d..d5016a460 100644 --- a/vm/interpreter.cpp +++ b/vm/interpreter.cpp @@ -16,6 +16,7 @@ // along with SourcePawn. If not, see . // #include "interpreter.h" +#include "debugging.h" #include "environment.h" #include "method-info.h" #include "plugin-context.h" @@ -1032,6 +1033,13 @@ Interpreter::visitSTRADJUST_PRI() return true; } +bool +Interpreter::visitBREAK() +{ + InvokeDebugger(cx_, nullptr); + return !env_->hasPendingException(); +} + bool Interpreter::visitHALT(cell_t value) { diff --git a/vm/interpreter.h b/vm/interpreter.h index aa3ba3549..c6015e43b 100644 --- a/vm/interpreter.h +++ b/vm/interpreter.h @@ -148,6 +148,7 @@ class Interpreter final : public PcodeVisitor bool visitTRACKER_PUSH_C(cell_t amount) override; bool visitTRACKER_POP_SETHEAP() override; bool visitSTRADJUST_PRI() override; + bool visitBREAK() override; bool visitHALT(cell_t value) override; bool visitREBASE(cell_t addr, cell_t iv_size, cell_t data_size) override; diff --git a/vm/jit.cpp b/vm/jit.cpp index 8b4a4f35f..76961d9e3 100644 --- a/vm/jit.cpp +++ b/vm/jit.cpp @@ -138,6 +138,9 @@ CompilerBase::emit() emitThrowPathIfNeeded(SP_ERROR_INTEGER_OVERFLOW); emitThrowPathIfNeeded(SP_ERROR_INVALID_NATIVE); + // Common path for invoking line debugger. + emitDebugBreakHandler(); + // This has to come very, very last, since it checks whether return paths // are used. emitErrorHandlers(); diff --git a/vm/jit.h b/vm/jit.h index bb8d4c405..b7f6b8050 100644 --- a/vm/jit.h +++ b/vm/jit.h @@ -72,6 +72,7 @@ class CompilerBase : public PcodeVisitor virtual void emitThrowPath(int err) = 0; virtual void emitErrorHandlers() = 0; virtual void emitOutOfBoundsErrorPath(OutOfBoundsErrorPath* path) = 0; + virtual void emitDebugBreakHandler() = 0; // Helpers. static int CompileFromThunk(PluginContext* cx, cell_t pcode_offs, void** addrp, uint8_t* pc); @@ -122,6 +123,9 @@ class CompilerBase : public PcodeVisitor Label report_error_; Label return_reported_error_; + // Debugging. + Label debug_break_; + ke::Vector backward_jumps_; ke::Vector cip_map_; }; diff --git a/vm/pcode-reader.h b/vm/pcode-reader.h index 5d470c7be..143a15856 100644 --- a/vm/pcode-reader.h +++ b/vm/pcode-reader.h @@ -87,11 +87,9 @@ class PcodeReader case OP_NOP: return true; - // This opcode is used to note where line breaks occur. We don't support - // live debugging, and if we did, we could build this map from the lines - // table. So we don't do any callbacks here. + // This opcode is used to note where line breaks occur. case OP_BREAK: - return true; + return visitor_->visitBREAK(); case OP_LOAD_PRI: case OP_LOAD_ALT: diff --git a/vm/pcode-visitor.h b/vm/pcode-visitor.h index 52cb16cf9..6f3e774fd 100644 --- a/vm/pcode-visitor.h +++ b/vm/pcode-visitor.h @@ -46,6 +46,7 @@ static_assert(sizeof(CaseTableEntry) == sizeof(cell_t) * 2, class PcodeVisitor { public: + virtual bool visitBREAK() = 0; virtual bool visitLOAD(PawnReg dest, cell_t srcaddr) = 0; virtual bool visitLOAD_S(PawnReg dest, cell_t srcoffs) = 0; virtual bool visitLREF_S(PawnReg dest, cell_t srcoffs) = 0; @@ -139,6 +140,10 @@ class PcodeVisitor class IncompletePcodeVisitor : public PcodeVisitor { public: + virtual bool visitBREAK() override { + assert(false); + return false; + } virtual bool visitLOAD(PawnReg dest, cell_t srcaddr) override { assert(false); return false; diff --git a/vm/plugin-context.cpp b/vm/plugin-context.cpp index a6958233b..c2893b59b 100644 --- a/vm/plugin-context.cpp +++ b/vm/plugin-context.cpp @@ -33,6 +33,7 @@ PluginContext::PluginContext(PluginRuntime* pRuntime) memory_(nullptr), data_size_(m_pRuntime->data().length), mem_size_(m_pRuntime->image()->HeapSize()), + dbreak_(nullptr), m_pNullVec(nullptr), m_pNullString(nullptr) { @@ -380,6 +381,18 @@ PluginContext::GetNullRef(SP_NULL_TYPE type) return NULL; } +int +PluginContext::SetDebugBreak(SPVM_DEBUGBREAK newpfn, SPVM_DEBUGBREAK *oldpfn) +{ + if (!IsDebugging()) + return SP_ERROR_NOTDEBUGGING; + + *oldpfn = dbreak_; + dbreak_ = newpfn; + + return SP_ERROR_NONE; +} + bool PluginContext::IsInExec() { diff --git a/vm/plugin-context.h b/vm/plugin-context.h index 93c270377..937b777f0 100644 --- a/vm/plugin-context.h +++ b/vm/plugin-context.h @@ -57,6 +57,7 @@ class PluginContext : public BasePluginContext int LocalToStringNULL(cell_t local_addr, char** addr) override; IPluginRuntime* GetRuntime() override; cell_t* GetLocalParams() override; + int SetDebugBreak(SPVM_DEBUGBREAK newpfn, SPVM_DEBUGBREAK *oldpfn) override; bool Invoke(funcid_t fnid, const cell_t* params, unsigned int num_params, cell_t* result); @@ -72,6 +73,9 @@ class PluginContext : public BasePluginContext PluginRuntime* runtime() const { return m_pRuntime; } + SPVM_DEBUGBREAK debugbreak() const { + return dbreak_; + } public: bool IsInExec() override; @@ -139,6 +143,8 @@ class PluginContext : public BasePluginContext uint32_t data_size_; uint32_t mem_size_; + SPVM_DEBUGBREAK dbreak_; + cell_t* m_pNullVec; cell_t* m_pNullString; diff --git a/vm/stack-frames.cpp b/vm/stack-frames.cpp index 4297eee4a..d09ecf737 100644 --- a/vm/stack-frames.cpp +++ b/vm/stack-frames.cpp @@ -242,6 +242,12 @@ FrameIterator::nextInvokeFrame() } } +cell_t +FrameIterator::cip() const +{ + return frame_cursor_->cip(); +} + void FrameIterator::Next() { diff --git a/vm/stack-frames.h b/vm/stack-frames.h index 56506435b..bd8673fd7 100644 --- a/vm/stack-frames.h +++ b/vm/stack-frames.h @@ -231,6 +231,8 @@ class FrameIterator : public SourcePawn::IFrameIterator IPluginContext* Context() const override; bool IsInternalFrame() const override; + cell_t cip() const; + private: void nextInvokeFrame(); diff --git a/vm/watchdog_timer.cpp b/vm/watchdog_timer.cpp index 2660f0384..e6e597a8f 100644 --- a/vm/watchdog_timer.cpp +++ b/vm/watchdog_timer.cpp @@ -24,6 +24,7 @@ WatchdogTimer::WatchdogTimer(Environment* env) : env_(env), terminate_(false), mainthread_(ke::GetCurrentThreadId()), + ignore_timeout_(false), last_frame_id_(0), second_timeout_(false), timedout_(false) @@ -90,12 +91,13 @@ WatchdogTimer::Run() // frame, then we assume the server is still moving enough to process // frames. We also make sure JIT code is actually on the stack, since // our concept of frames won't move if JIT code is not running. + // If we're told to ignore timeouts, we do so. // // Note that it's okay if these two race: it's just a heuristic, and // worst case, we'll miss something that might have timed out but // ended up resuming. uintptr_t frame_id = env_->FrameId(); - if (frame_id != last_frame_id_ || !env_->RunningCode()) { + if (frame_id != last_frame_id_ || !env_->RunningCode() || ignore_timeout_) { last_frame_id_ = frame_id; second_timeout_ = false; continue; diff --git a/vm/watchdog_timer.h b/vm/watchdog_timer.h index f31c56eb9..89b1c0e43 100644 --- a/vm/watchdog_timer.h +++ b/vm/watchdog_timer.h @@ -21,14 +21,22 @@ #include #include +namespace SourcePawn { + class IErrorReport; +} + namespace sp { class Environment; +class PluginContext; typedef bool (*WatchdogCallback)(); class WatchdogTimer { + // Allow line debugger callback to disable timeouts. + friend void InvokeDebugger(PluginContext *ctx, const SourcePawn::IErrorReport *report); + public: WatchdogTimer(Environment* env); ~WatchdogTimer(); @@ -50,6 +58,7 @@ class WatchdogTimer bool terminate_; size_t timeout_ms_; ke::ThreadId mainthread_; + bool ignore_timeout_; ke::AutoPtr thread_; ke::ConditionVariable cv_; diff --git a/vm/x86/jit_x86.cpp b/vm/x86/jit_x86.cpp index 6b4706f1f..68426c6f5 100644 --- a/vm/x86/jit_x86.cpp +++ b/vm/x86/jit_x86.cpp @@ -43,6 +43,7 @@ #include "outofline-asm.h" #include "method-info.h" #include "runtime-helpers.h" +#include "debugging.h" #define __ masm. @@ -1116,6 +1117,14 @@ Compiler::visitREBASE(cell_t addr, cell_t iv_size, cell_t data_size) return true; } +bool +Compiler::visitBREAK() +{ + __ call(&debug_break_); + emitCipMapping(op_cip_); + return true; +} + bool Compiler::visitHALT(cell_t value) { @@ -1601,6 +1610,31 @@ Compiler::emitThrowPath(int err) __ jmp(&report_error_); } +void +Compiler::emitDebugBreakHandler() +{ + // Common path for invoking debugger. + __ bind(&debug_break_); + + // Get and store the current stack pointer. + __ movl(tmp, stk); + __ subl(tmp, dat); + __ movl(Operand(spAddr()), tmp); + + // Enter the exit frame. This aligns the stack. + __ enterExitFrame(ExitFrameType::Helper, 0); + + // Get the context pointer and call the debugging break handler. + __ push(0); // IErrorReport* + __ push(intptr_t(rt_->GetBaseContext())); + __ call(ExternalAddress((void *)InvokeDebugger)); + __ addl(esp, 8); + __ leaveExitFrame(); + __ testl(eax, eax); + jumpOnError(not_zero); + __ ret(); +} + void CompilerBase::PatchCallThunk(uint8_t* pc, void* target) { diff --git a/vm/x86/jit_x86.h b/vm/x86/jit_x86.h index 370a60959..58b75f469 100644 --- a/vm/x86/jit_x86.h +++ b/vm/x86/jit_x86.h @@ -44,6 +44,7 @@ class Compiler : public CompilerBase Compiler(PluginRuntime* rt, cell_t pcode_offs); ~Compiler(); + bool visitBREAK() override; bool visitLOAD(PawnReg dest, cell_t srcaddr) override; bool visitLOAD_S(PawnReg dest, cell_t srcoffs) override; bool visitLREF_S(PawnReg dest, cell_t srcoffs) override; @@ -144,6 +145,7 @@ class Compiler : public CompilerBase void emitThrowPath(int err) override; void emitErrorHandlers() override; void emitOutOfBoundsErrorPath(OutOfBoundsErrorPath* path) override; + void emitDebugBreakHandler() override; void emitLegacyNativeCall(uint32_t native_index, NativeEntry* native); void emitGenArray(bool autozero); From d937d752546687e1c8773d722f78f048a6162790 Mon Sep 17 00:00:00 2001 From: Peace-Maker Date: Thu, 3 May 2018 10:35:14 +0200 Subject: [PATCH 02/13] Use a versioned struct in debug break callback Don't pass the state in multiple variables, but use a struct to hold the VM state. --- include/sp_vm_api.h | 4 ++-- include/sp_vm_types.h | 28 ++++++++++++++++++++++------ vm/base-context.cpp | 2 +- vm/base-context.h | 2 +- vm/debugging.cpp | 15 ++++++++++++--- vm/debugging.h | 2 +- vm/plugin-context.cpp | 2 +- vm/plugin-context.h | 2 +- vm/watchdog_timer.h | 2 +- 9 files changed, 42 insertions(+), 17 deletions(-) diff --git a/include/sp_vm_api.h b/include/sp_vm_api.h index 0501e4c14..26ba37f0c 100644 --- a/include/sp_vm_api.h +++ b/include/sp_vm_api.h @@ -24,7 +24,7 @@ /** SourcePawn Engine API Versions */ #define SOURCEPAWN_ENGINE2_API_VERSION 0xC -#define SOURCEPAWN_API_VERSION 0x020D +#define SOURCEPAWN_API_VERSION 0x020E namespace SourceMod { struct IdentityToken_t; @@ -619,7 +619,7 @@ namespace SourcePawn * * @return Error number. */ - virtual int SetDebugBreak(SPVM_DEBUGBREAK newpfn, SPVM_DEBUGBREAK *oldpfn) =0; + virtual int SetDebugBreak(SPVM_DEBUGBREAK newpfn, SPVM_DEBUGBREAK* oldpfn) =0; /** * @brief Deprecated, do not use. diff --git a/include/sp_vm_types.h b/include/sp_vm_types.h index d4964d8cd..2593bdc6f 100644 --- a/include/sp_vm_types.h +++ b/include/sp_vm_types.h @@ -55,6 +55,8 @@ typedef uint32_t funcid_t; /**< Function index code */ #define SP_PROF_CALLBACKS (1<<1) /**< Profile callbacks. */ #define SP_PROF_FUNCTIONS (1<<2) /**< Profile functions. */ +#define DEBUG_BREAK_INFO_VERSION 0x0001 /**< Version of the sp_debug_break_info_t struct. */ + /** * @brief Error codes for SourcePawn routines. */ @@ -248,14 +250,28 @@ typedef struct sp_debug_symbol_s } sp_debug_symbol_t; /** - * Breaks into a debugger + * @brief Context describing the VM state when the SPVM_DEBUGBREAK + * callback is called. + */ +typedef struct sp_debug_break_info_s +{ + uint16_t version; /**< Version of this struct */ + cell_t cip; /**< Current virtual instruction pointer */ + cell_t frm; /**< Current virtual frame pointer */ + cell_t sp; /**< Current virtual stack pointer */ + uint8_t* memory; /**< Pointer to raw plugin image in memory */ + size_t mem_size; /**< Total size of the plugin image in memory */ +} sp_debug_break_info_t; + +/** + * Breaks into a debugger. + * If the exception parameter is not null, + * the function is called with the location of the error. * Params: * [0] - plugin context - * [1] - frm - * [2] - cip - * [3] - memory - * [4] - exception + * [1] - debug info + * [2] - exception */ -typedef void(*SPVM_DEBUGBREAK)(SourcePawn::IPluginContext *, cell_t, cell_t, void *, const SourcePawn::IErrorReport *); +typedef void(*SPVM_DEBUGBREAK)(SourcePawn::IPluginContext*, sp_debug_break_info_t&, const SourcePawn::IErrorReport*); #endif //_INCLUDE_SOURCEPAWN_VM_TYPES_H diff --git a/vm/base-context.cpp b/vm/base-context.cpp index 1dc2c0dc2..309270301 100644 --- a/vm/base-context.cpp +++ b/vm/base-context.cpp @@ -74,7 +74,7 @@ BasePluginContext::IsDebugging() } int -BasePluginContext::SetDebugBreak(SPVM_DEBUGBREAK newpfn, SPVM_DEBUGBREAK *oldpfn) +BasePluginContext::SetDebugBreak(SPVM_DEBUGBREAK newpfn, SPVM_DEBUGBREAK* oldpfn) { return SP_ERROR_ABORTED; } diff --git a/vm/base-context.h b/vm/base-context.h index 05aa8727e..6fe552b25 100644 --- a/vm/base-context.h +++ b/vm/base-context.h @@ -43,7 +43,7 @@ class BasePluginContext : public SourcePawn::IPluginContext cell_t BlamePluginError(SourcePawn::IPluginFunction* pf, const char* msg, ...) override; IFrameIterator* CreateFrameIterator() override; void DestroyFrameIterator(IFrameIterator* it) override; - int SetDebugBreak(SPVM_DEBUGBREAK newpfn, SPVM_DEBUGBREAK *oldpfn) override; + int SetDebugBreak(SPVM_DEBUGBREAK newpfn, SPVM_DEBUGBREAK* oldpfn) override; // Removed functions. int PushCell(cell_t value) override; diff --git a/vm/debugging.cpp b/vm/debugging.cpp index 9818cecb8..edb1ecb5d 100644 --- a/vm/debugging.cpp +++ b/vm/debugging.cpp @@ -18,7 +18,7 @@ namespace sp { -void InvokeDebugger(PluginContext *ctx, const IErrorReport *report) +void InvokeDebugger(PluginContext* ctx, const IErrorReport* report) { // Continue normal execution, if this plugin isn't being debugged. if (!ctx->debugbreak()) @@ -49,8 +49,17 @@ void InvokeDebugger(PluginContext *ctx, const IErrorReport *report) // continueing with execution. ke::SaveAndSet(&Environment::get()->watchdog()->ignore_timeout_, true); + // Fill in the debug info struct. + sp_debug_break_info_t dbginfo; + dbginfo.version = DEBUG_BREAK_INFO_VERSION; + dbginfo.cip = cip; + dbginfo.frm = ctx->frm(); + dbginfo.sp = ctx->sp(); + dbginfo.memory = ctx->memory(); + dbginfo.mem_size = ctx->HeapSize(); + // Call debug callback. - ctx->debugbreak()(ctx, ctx->frm(), cip, ctx->memory(), report); + ctx->debugbreak()(ctx, dbginfo, report); } -} // namespace sp \ No newline at end of file +} // namespace sp diff --git a/vm/debugging.h b/vm/debugging.h index cf4a3e2d1..baf0a0cdb 100644 --- a/vm/debugging.h +++ b/vm/debugging.h @@ -22,7 +22,7 @@ namespace sp { -void InvokeDebugger(PluginContext *ctx, const IErrorReport *report); +void InvokeDebugger(PluginContext* ctx, const IErrorReport* report); } // namespace sp diff --git a/vm/plugin-context.cpp b/vm/plugin-context.cpp index c2893b59b..773b58821 100644 --- a/vm/plugin-context.cpp +++ b/vm/plugin-context.cpp @@ -382,7 +382,7 @@ PluginContext::GetNullRef(SP_NULL_TYPE type) } int -PluginContext::SetDebugBreak(SPVM_DEBUGBREAK newpfn, SPVM_DEBUGBREAK *oldpfn) +PluginContext::SetDebugBreak(SPVM_DEBUGBREAK newpfn, SPVM_DEBUGBREAK* oldpfn) { if (!IsDebugging()) return SP_ERROR_NOTDEBUGGING; diff --git a/vm/plugin-context.h b/vm/plugin-context.h index 937b777f0..2ba311d26 100644 --- a/vm/plugin-context.h +++ b/vm/plugin-context.h @@ -57,7 +57,7 @@ class PluginContext : public BasePluginContext int LocalToStringNULL(cell_t local_addr, char** addr) override; IPluginRuntime* GetRuntime() override; cell_t* GetLocalParams() override; - int SetDebugBreak(SPVM_DEBUGBREAK newpfn, SPVM_DEBUGBREAK *oldpfn) override; + int SetDebugBreak(SPVM_DEBUGBREAK newpfn, SPVM_DEBUGBREAK* oldpfn) override; bool Invoke(funcid_t fnid, const cell_t* params, unsigned int num_params, cell_t* result); diff --git a/vm/watchdog_timer.h b/vm/watchdog_timer.h index 89b1c0e43..309246027 100644 --- a/vm/watchdog_timer.h +++ b/vm/watchdog_timer.h @@ -35,7 +35,7 @@ typedef bool (*WatchdogCallback)(); class WatchdogTimer { // Allow line debugger callback to disable timeouts. - friend void InvokeDebugger(PluginContext *ctx, const SourcePawn::IErrorReport *report); + friend void InvokeDebugger(PluginContext* ctx, const SourcePawn::IErrorReport* report); public: WatchdogTimer(Environment* env); From e66fe8d67bab3d5f3c87d90b4640fd3b71200c59 Mon Sep 17 00:00:00 2001 From: Peace-Maker Date: Thu, 3 May 2018 11:33:42 +0200 Subject: [PATCH 03/13] Add functions to iterate source file names to IPluginDebugInfo Expose the file names of the source files. --- include/sp_vm_api.h | 14 ++++++++++++++ vm/legacy-image.h | 8 ++++++++ vm/plugin-runtime.cpp | 12 ++++++++++++ vm/plugin-runtime.h | 2 ++ vm/smx-v1-image.cpp | 18 ++++++++++++++++++ vm/smx-v1-image.h | 2 ++ 6 files changed, 56 insertions(+) diff --git a/include/sp_vm_api.h b/include/sp_vm_api.h index 26ba37f0c..3b6a2b837 100644 --- a/include/sp_vm_api.h +++ b/include/sp_vm_api.h @@ -293,6 +293,20 @@ namespace SourcePawn * @param line Pointer to store line number in. */ virtual int LookupLine(ucell_t addr, uint32_t *line) =0; + + /** + * @brief Returns the number of source files compiled into this plugin. + */ + virtual size_t NumFiles() =0; + + /** + * @brief Returns the full file name and path of the source file + * at the given index. + * + * @param index Index of selected file in the list of source files. + * @return Full file name of source file or NULL if not found. + */ + virtual const char* GetFileName(size_t index) =0; }; class ICompilation; diff --git a/vm/legacy-image.h b/vm/legacy-image.h index 90e71e5e6..ef17a931b 100644 --- a/vm/legacy-image.h +++ b/vm/legacy-image.h @@ -54,6 +54,8 @@ class LegacyImage virtual const char* LookupFile(uint32_t code_offset) = 0; virtual const char* LookupFunction(uint32_t code_offset) = 0; virtual bool LookupLine(uint32_t code_offset, uint32_t* line) = 0; + virtual size_t NumFiles() const = 0; + virtual const char* GetFileName(size_t index) const = 0; }; class EmptyImage : public LegacyImage @@ -122,6 +124,12 @@ class EmptyImage : public LegacyImage bool LookupLine(uint32_t code_offset, uint32_t* line) override { return false; } + size_t NumFiles() const override { + return 0; + } + const char* GetFileName(size_t index) const override { + return nullptr; + } private: size_t heap_size_; diff --git a/vm/plugin-runtime.cpp b/vm/plugin-runtime.cpp index 7e0bdb4f5..4206a6e0c 100644 --- a/vm/plugin-runtime.cpp +++ b/vm/plugin-runtime.cpp @@ -554,3 +554,15 @@ PluginRuntime::LookupFile(ucell_t addr, const char** out) *out = name; return SP_ERROR_NONE; } + +size_t +PluginRuntime::NumFiles() +{ + return image_->NumFiles(); +} + +const char* +PluginRuntime::GetFileName(size_t index) +{ + return image_->GetFileName(index); +} diff --git a/vm/plugin-runtime.h b/vm/plugin-runtime.h index 227cc8a61..23da37d8f 100644 --- a/vm/plugin-runtime.h +++ b/vm/plugin-runtime.h @@ -89,6 +89,8 @@ class PluginRuntime int LookupLine(ucell_t addr, uint32_t* line) override; int LookupFunction(ucell_t addr, const char** name) override; int LookupFile(ucell_t addr, const char** filename) override; + size_t NumFiles() override; + const char* GetFileName(size_t index) override; const char* GetFilename() override { return full_name_.chars(); } diff --git a/vm/smx-v1-image.cpp b/vm/smx-v1-image.cpp index 74d3e45f0..6e90b79aa 100644 --- a/vm/smx-v1-image.cpp +++ b/vm/smx-v1-image.cpp @@ -756,3 +756,21 @@ SmxV1Image::LookupLine(uint32_t addr, uint32_t* line) *line = debug_lines_[low].line + 1; return true; } + +size_t +SmxV1Image::NumFiles() const +{ + return debug_files_.length(); +} + +const char* +SmxV1Image::GetFileName(size_t index) const +{ + if (index >= debug_files_.length()) + return nullptr; + + if (debug_files_[index].name >= debug_names_section_->size) + return nullptr; + + return debug_names_ + debug_files_[index].name; +} diff --git a/vm/smx-v1-image.h b/vm/smx-v1-image.h index 53a1fac86..a6e39eda0 100644 --- a/vm/smx-v1-image.h +++ b/vm/smx-v1-image.h @@ -57,6 +57,8 @@ class SmxV1Image const char* LookupFile(uint32_t code_offset) override; const char* LookupFunction(uint32_t code_offset) override; bool LookupLine(uint32_t code_offset, uint32_t* line) override; + size_t NumFiles() const override; + const char* GetFileName(size_t index) const override; private: struct Section From c53f52b572e3f63b5967b627e75a2fe0d34b1beb Mon Sep 17 00:00:00 2001 From: Peace-Maker Date: Thu, 3 May 2018 12:14:32 +0200 Subject: [PATCH 04/13] Remove unnecassary info from VM state struct Keep it as simple as possible. --- include/sp_vm_types.h | 3 --- vm/debugging.cpp | 3 --- 2 files changed, 6 deletions(-) diff --git a/include/sp_vm_types.h b/include/sp_vm_types.h index 2593bdc6f..08def9dd6 100644 --- a/include/sp_vm_types.h +++ b/include/sp_vm_types.h @@ -258,9 +258,6 @@ typedef struct sp_debug_break_info_s uint16_t version; /**< Version of this struct */ cell_t cip; /**< Current virtual instruction pointer */ cell_t frm; /**< Current virtual frame pointer */ - cell_t sp; /**< Current virtual stack pointer */ - uint8_t* memory; /**< Pointer to raw plugin image in memory */ - size_t mem_size; /**< Total size of the plugin image in memory */ } sp_debug_break_info_t; /** diff --git a/vm/debugging.cpp b/vm/debugging.cpp index edb1ecb5d..240160f77 100644 --- a/vm/debugging.cpp +++ b/vm/debugging.cpp @@ -54,9 +54,6 @@ void InvokeDebugger(PluginContext* ctx, const IErrorReport* report) dbginfo.version = DEBUG_BREAK_INFO_VERSION; dbginfo.cip = cip; dbginfo.frm = ctx->frm(); - dbginfo.sp = ctx->sp(); - dbginfo.memory = ctx->memory(); - dbginfo.mem_size = ctx->HeapSize(); // Call debug callback. ctx->debugbreak()(ctx, dbginfo, report); From ca3eb9a67d1d4272fb9f2d11a81166263f50d792 Mon Sep 17 00:00:00 2001 From: Peace-Maker Date: Sat, 26 May 2018 16:31:50 +0200 Subject: [PATCH 05/13] Add functions to lookup the address of a function or line in a file To be able to set breakpoints we need to be able to get the cip of the function or line we want to break on. --- include/sp_vm_api.h | 19 +++++++ vm/legacy-image.h | 8 +++ vm/plugin-runtime.cpp | 16 ++++++ vm/plugin-runtime.h | 2 + vm/smx-v1-image.cpp | 118 ++++++++++++++++++++++++++++++++++++++++++ vm/smx-v1-image.h | 5 ++ 6 files changed, 168 insertions(+) diff --git a/include/sp_vm_api.h b/include/sp_vm_api.h index 3b6a2b837..dd551d696 100644 --- a/include/sp_vm_api.h +++ b/include/sp_vm_api.h @@ -294,6 +294,25 @@ namespace SourcePawn */ virtual int LookupLine(ucell_t addr, uint32_t *line) =0; + /** + * @brief Given the name of a function and a source file, finds the code pointer + * of the start of the function. + * + * @param function Name of the function to lookup. + * @param file Name of the file containing the function to lookup. + * @param addr Output pointer to store address of function in. + */ + virtual int LookupFunctionAddress(const char* function, const char* file, ucell_t* addr) =0; + + /** + * @brief Given a line number and a source file, finds the code pointer of the line. + * + * @param line The line number. + * @param file Name of the file containing the line to lookup. + * @param addr Output pointer to store address of line in. + */ + virtual int LookupLineAddress(const uint32_t line, const char* file, ucell_t* addr) =0; + /** * @brief Returns the number of source files compiled into this plugin. */ diff --git a/vm/legacy-image.h b/vm/legacy-image.h index ef17a931b..6f21dd84d 100644 --- a/vm/legacy-image.h +++ b/vm/legacy-image.h @@ -54,6 +54,8 @@ class LegacyImage virtual const char* LookupFile(uint32_t code_offset) = 0; virtual const char* LookupFunction(uint32_t code_offset) = 0; virtual bool LookupLine(uint32_t code_offset, uint32_t* line) = 0; + virtual bool LookupFunctionAddress(const char* function, const char* file, ucell_t *addr) = 0; + virtual bool LookupLineAddress(const uint32_t line, const char* file, ucell_t* addr) = 0; virtual size_t NumFiles() const = 0; virtual const char* GetFileName(size_t index) const = 0; }; @@ -124,6 +126,12 @@ class EmptyImage : public LegacyImage bool LookupLine(uint32_t code_offset, uint32_t* line) override { return false; } + bool LookupFunctionAddress(const char* function, const char* file, ucell_t* addr) override { + return false; + } + bool LookupLineAddress(const uint32_t line, const char* file, ucell_t* addr) override { + return false; + } size_t NumFiles() const override { return 0; } diff --git a/vm/plugin-runtime.cpp b/vm/plugin-runtime.cpp index 4206a6e0c..a62df09e7 100644 --- a/vm/plugin-runtime.cpp +++ b/vm/plugin-runtime.cpp @@ -566,3 +566,19 @@ PluginRuntime::GetFileName(size_t index) { return image_->GetFileName(index); } + +int +PluginRuntime::LookupFunctionAddress(const char* function, const char* file, ucell_t* addr) +{ + if (!image_->LookupFunctionAddress(function, file, addr)) + return SP_ERROR_NOT_FOUND; + return SP_ERROR_NONE; +} + +int +PluginRuntime::LookupLineAddress(const uint32_t line, const char* file, ucell_t* addr) +{ + if (!image_->LookupLineAddress(line, file, addr)) + return SP_ERROR_NOT_FOUND; + return SP_ERROR_NONE; +} diff --git a/vm/plugin-runtime.h b/vm/plugin-runtime.h index 23da37d8f..f8eeec960 100644 --- a/vm/plugin-runtime.h +++ b/vm/plugin-runtime.h @@ -91,6 +91,8 @@ class PluginRuntime int LookupFile(ucell_t addr, const char** filename) override; size_t NumFiles() override; const char* GetFileName(size_t index) override; + int LookupFunctionAddress(const char* function, const char* file, ucell_t* addr) override; + int LookupLineAddress(const uint32_t line, const char* file, ucell_t* addr) override; const char* GetFilename() override { return full_name_.chars(); } diff --git a/vm/smx-v1-image.cpp b/vm/smx-v1-image.cpp index 6e90b79aa..b42152b59 100644 --- a/vm/smx-v1-image.cpp +++ b/vm/smx-v1-image.cpp @@ -774,3 +774,121 @@ SmxV1Image::GetFileName(size_t index) const return debug_names_ + debug_files_[index].name; } + +template +bool +SmxV1Image::getFunctionAddress(const SymbolType* syms, const char* function, ucell_t* funcaddr, uint32_t& index) +{ + const uint8_t* cursor = reinterpret_cast(syms); + const uint8_t* cursor_end = cursor + debug_symbols_section_->size; + for (uint32_t i = index; i < debug_info_->num_syms; i++) { + if (cursor + sizeof(SymbolType) > cursor_end) + break; + + const SymbolType *sym = reinterpret_cast(cursor); + if (sym->ident == sp::IDENT_FUNCTION && + sym->name < debug_names_section_->size && + !strcmp(debug_names_ + sym->name, function)) + { + *funcaddr = sym->addr; + return true; + } + + if (sym->dimcount > 0) + cursor += sizeof(DimType) * sym->dimcount; + cursor += sizeof(SymbolType); + } + return false; +} + +bool +SmxV1Image::LookupFunctionAddress(const char* function, const char* file, ucell_t* funcaddr) +{ + uint32_t index = 0; + const char* tgtfile; + *funcaddr = 0; + for (;;) { + // find (next) matching function + if (debug_syms_) { + getFunctionAddress(debug_syms_, function, funcaddr, index); + } + else { + getFunctionAddress(debug_syms_unpacked_, function, funcaddr, index); + } + + if (index >= debug_info_->num_syms) + return false; + + // verify that this function is defined in the apprpriate file + tgtfile = LookupFile(*funcaddr); + if (tgtfile != nullptr && strcmp(file, tgtfile) == 0) + break; + index++; + } + assert(index < debug_info_->num_syms); + + // now find the first line in the function where we can "break" on + for (index = 0; index < debug_info_->num_lines && debug_lines_[index].addr < *funcaddr; index++) + /* nothing */; + + if (index >= debug_info_->num_lines) + return false; + + *funcaddr = debug_lines_[index].addr; + return true; +} + +bool +SmxV1Image::LookupLineAddress(const uint32_t line, const char* filename, uint32_t* addr) +{ + /* Find a suitable "breakpoint address" close to the indicated line (and in + * the specified file). The address is moved up to the next "breakable" line + * if no "breakpoint" is available on the specified line. You can use function + * LookupLine() to find out at which precise line the breakpoint was set. + * + * The filename comparison is strict (case sensitive and path sensitive). + */ + *addr = 0; + + uint32_t bottomaddr, topaddr; + uint32_t file; + uint32_t index = 0; + for (file = 0; file < debug_info_->num_files; file++) { + // find the (next) matching instance of the file + if (debug_files_[file].name >= debug_names_section_->size || + strcmp(debug_names_ + debug_files_[file].name, filename) != 0) + { + continue; + } + + // get address range for the current file + bottomaddr = debug_files_[file].addr; + topaddr = (file + 1 < debug_info_->num_files) ? debug_files_[file + 1].addr : (uint32_t)-1; + + // go to the starting address in the line table + while (index < debug_info_->num_lines && debug_lines_[index].addr < bottomaddr) + index++; + + // browse until the line is found or until the top address is exceeded + while (index < debug_info_->num_lines && + debug_lines_[index].line < line && + debug_lines_[index].addr < topaddr) + { + index++; + } + + if (index >= debug_info_->num_lines) + return false; + if (debug_lines_[index].line >= line) + break; + + // if not found (and the line table is not yet exceeded) try the next + // instance of the same file (a file may appear twice in the file table) + } + if (file >= debug_info_->num_files) + return false; + + assert(index < debug_info_->num_lines); + *addr = debug_lines_[index].addr; + return true; +} diff --git a/vm/smx-v1-image.h b/vm/smx-v1-image.h index a6e39eda0..4dcfee0cc 100644 --- a/vm/smx-v1-image.h +++ b/vm/smx-v1-image.h @@ -17,6 +17,7 @@ #include #include #include +#include #include "file-utils.h" #include "legacy-image.h" @@ -57,6 +58,8 @@ class SmxV1Image const char* LookupFile(uint32_t code_offset) override; const char* LookupFunction(uint32_t code_offset) override; bool LookupLine(uint32_t code_offset, uint32_t* line) override; + bool LookupFunctionAddress(const char* function, const char* file, ucell_t* addr) override; + bool LookupLineAddress(const uint32_t line, const char* file, ucell_t* addr) override; size_t NumFiles() const override; const char* GetFileName(size_t index) const override; @@ -190,6 +193,8 @@ class SmxV1Image private: template const char* lookupFunction(const SymbolType* syms, uint32_t addr); + template + bool getFunctionAddress(const SymbolType* syms, const char* function, ucell_t* funcaddr, uint32_t& index); const smx_rtti_table_header* findRttiSection(const char* name) { const Section* section = findSection(name); From 799d51dd121f1f75a9a630702d986005828f7598 Mon Sep 17 00:00:00 2001 From: Peace-Maker Date: Sun, 27 May 2018 16:03:40 +0200 Subject: [PATCH 06/13] Fix disabling VM watchdog during debug callback --- include/sp_vm_types.h | 2 +- vm/debugging.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/include/sp_vm_types.h b/include/sp_vm_types.h index 08def9dd6..ea50223d1 100644 --- a/include/sp_vm_types.h +++ b/include/sp_vm_types.h @@ -111,7 +111,7 @@ namespace SourcePawn class IPluginContext; class IVirtualMachine; class IProfiler; - class IErrorReport; + class IErrorReport; }; struct sp_context_s; diff --git a/vm/debugging.cpp b/vm/debugging.cpp index 240160f77..64cde7065 100644 --- a/vm/debugging.cpp +++ b/vm/debugging.cpp @@ -47,7 +47,7 @@ void InvokeDebugger(PluginContext* ctx, const IErrorReport* report) // We might stay in the debugger callback for a while, // so don't let the watchdog hit immediately after // continueing with execution. - ke::SaveAndSet(&Environment::get()->watchdog()->ignore_timeout_, true); + ke::SaveAndSet disableWatchdog(&Environment::get()->watchdog()->ignore_timeout_, true); // Fill in the debug info struct. sp_debug_break_info_t dbginfo; From 5dbd11413aff0000b4e408a541ae8949cead31e9 Mon Sep 17 00:00:00 2001 From: Peace-Maker Date: Sun, 27 May 2018 17:00:13 +0200 Subject: [PATCH 07/13] Support RTTI tables Lookup the function address from the rtti.methods table if available. --- vm/smx-v1-image.cpp | 44 +++++++++++++++++++++++++++++--------------- 1 file changed, 29 insertions(+), 15 deletions(-) diff --git a/vm/smx-v1-image.cpp b/vm/smx-v1-image.cpp index b42152b59..58e4c5fa5 100644 --- a/vm/smx-v1-image.cpp +++ b/vm/smx-v1-image.cpp @@ -807,25 +807,39 @@ SmxV1Image::LookupFunctionAddress(const char* function, const char* file, ucell_ uint32_t index = 0; const char* tgtfile; *funcaddr = 0; - for (;;) { - // find (next) matching function - if (debug_syms_) { - getFunctionAddress(debug_syms_, function, funcaddr, index); - } - else { - getFunctionAddress(debug_syms_unpacked_, function, funcaddr, index); + if (rtti_methods_) { + for (uint32_t i = 0; i < rtti_methods_->row_count; i++) { + const smx_rtti_method* method = getRttiRow(rtti_methods_, i); + const char* name = names_ + method->name; + if (strcmp(name, function) != 0) + continue; + + *funcaddr = method->pcode_start; + // verify that this function is defined in the appropriate file + tgtfile = LookupFile(*funcaddr); + if (tgtfile != nullptr && !strcmp(file, tgtfile)) + break; } + } else { + for (;;) { + // find (next) matching function + if (debug_syms_) { + getFunctionAddress(debug_syms_, function, funcaddr, index); + } else { + getFunctionAddress(debug_syms_unpacked_, function, funcaddr, index); + } - if (index >= debug_info_->num_syms) - return false; + if (index >= debug_info_->num_syms) + return false; - // verify that this function is defined in the apprpriate file - tgtfile = LookupFile(*funcaddr); - if (tgtfile != nullptr && strcmp(file, tgtfile) == 0) - break; - index++; + // verify that this function is defined in the appropriate file + tgtfile = LookupFile(*funcaddr); + if (tgtfile != nullptr && strcmp(file, tgtfile) == 0) + break; + index++; + } + assert(index < debug_info_->num_syms); } - assert(index < debug_info_->num_syms); // now find the first line in the function where we can "break" on for (index = 0; index < debug_info_->num_lines && debug_lines_[index].addr < *funcaddr; index++) From ab9e10c21b8be0c1e26b80ffc9b186a546e287a2 Mon Sep 17 00:00:00 2001 From: Peace-Maker Date: Mon, 18 Jun 2018 18:49:36 +0200 Subject: [PATCH 08/13] Fix nits --- vm/smx-v1-image.cpp | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/vm/smx-v1-image.cpp b/vm/smx-v1-image.cpp index 58e4c5fa5..5a5bc54f5 100644 --- a/vm/smx-v1-image.cpp +++ b/vm/smx-v1-image.cpp @@ -804,8 +804,6 @@ SmxV1Image::getFunctionAddress(const SymbolType* syms, const char* function, uce bool SmxV1Image::LookupFunctionAddress(const char* function, const char* file, ucell_t* funcaddr) { - uint32_t index = 0; - const char* tgtfile; *funcaddr = 0; if (rtti_methods_) { for (uint32_t i = 0; i < rtti_methods_->row_count; i++) { @@ -816,13 +814,14 @@ SmxV1Image::LookupFunctionAddress(const char* function, const char* file, ucell_ *funcaddr = method->pcode_start; // verify that this function is defined in the appropriate file - tgtfile = LookupFile(*funcaddr); + const char* tgtfile = LookupFile(*funcaddr); if (tgtfile != nullptr && !strcmp(file, tgtfile)) break; } } else { for (;;) { // find (next) matching function + uint32_t index = 0; if (debug_syms_) { getFunctionAddress(debug_syms_, function, funcaddr, index); } else { @@ -833,16 +832,17 @@ SmxV1Image::LookupFunctionAddress(const char* function, const char* file, ucell_ return false; // verify that this function is defined in the appropriate file - tgtfile = LookupFile(*funcaddr); + const char* tgtfile = LookupFile(*funcaddr); if (tgtfile != nullptr && strcmp(file, tgtfile) == 0) break; index++; + assert(index < debug_info_->num_syms); } - assert(index < debug_info_->num_syms); } // now find the first line in the function where we can "break" on - for (index = 0; index < debug_info_->num_lines && debug_lines_[index].addr < *funcaddr; index++) + uint32_t index = 0; + for (; index < debug_info_->num_lines && debug_lines_[index].addr < *funcaddr; index++) /* nothing */; if (index >= debug_info_->num_lines) From 34ddb67b384655bcd8c9b724af1482b5706a34d6 Mon Sep 17 00:00:00 2001 From: Peace-Maker Date: Fri, 17 Aug 2018 21:28:54 +0200 Subject: [PATCH 09/13] Align stack before calling the debug handler The stack should be aligned to 16 bytes before a call. --- vm/x86/jit_x86.cpp | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/vm/x86/jit_x86.cpp b/vm/x86/jit_x86.cpp index 68426c6f5..a9b53d8a8 100644 --- a/vm/x86/jit_x86.cpp +++ b/vm/x86/jit_x86.cpp @@ -1624,11 +1624,15 @@ Compiler::emitDebugBreakHandler() // Enter the exit frame. This aligns the stack. __ enterExitFrame(ExitFrameType::Helper, 0); + // Allocate enough memory to keep the stack aligned. + static const size_t kStackNeeded = 2 * sizeof(void *); + static const size_t kStackReserve = ke::Align(kStackNeeded, 16); + __ subl(esp, kStackReserve); + // Get the context pointer and call the debugging break handler. - __ push(0); // IErrorReport* - __ push(intptr_t(rt_->GetBaseContext())); + __ movl(Operand(esp, 1 * sizeof(void *)), 0); // IErrorReport* + __ movl(Operand(esp, 0 * sizeof(void *)), intptr_t(rt_->GetBaseContext())); __ call(ExternalAddress((void *)InvokeDebugger)); - __ addl(esp, 8); __ leaveExitFrame(); __ testl(eax, eax); jumpOnError(not_zero); From e40096fb16e937db723985de0f5424011d89bb03 Mon Sep 17 00:00:00 2001 From: Peace-Maker Date: Fri, 28 Sep 2018 15:24:09 +0200 Subject: [PATCH 10/13] Use a single debug break handler instead of one per context Simplify the API to only allow one debug break handler for all plugins instead of maintaining different ones per runtime. --- include/sp_vm_api.h | 22 +++++++++++++++------- vm/api.cpp | 12 +++++++++++- vm/api.h | 1 + vm/base-context.cpp | 2 +- vm/base-context.h | 2 +- vm/debugging.cpp | 6 +++--- vm/environment.cpp | 3 ++- vm/environment.h | 9 +++++++++ vm/plugin-context.cpp | 13 ------------- vm/plugin-context.h | 6 ------ 10 files changed, 43 insertions(+), 33 deletions(-) diff --git a/include/sp_vm_api.h b/include/sp_vm_api.h index dd551d696..977cd5a01 100644 --- a/include/sp_vm_api.h +++ b/include/sp_vm_api.h @@ -644,15 +644,12 @@ namespace SourcePawn virtual bool IsDebugging() =0; /** - * @brief Installs a debug break and returns the old one, if any. - * This will fail if the plugin is not debugging. - * - * @param newpfn New function pointer. - * @param oldpfn Pointer to retrieve old function pointer. + * @brief Deprecated, does nothing. * - * @return Error number. + * @param newpfn Unused. + * @param oldpfn Unused. */ - virtual int SetDebugBreak(SPVM_DEBUGBREAK newpfn, SPVM_DEBUGBREAK* oldpfn) =0; + virtual int SetDebugBreak(void *newpfn, void *oldpfn) =0; /** * @brief Deprecated, do not use. @@ -1378,6 +1375,17 @@ namespace SourcePawn * @param ptr Address to free. */ virtual void FreePageMemory(void *ptr) =0; + + + /* + * @brief Installs a debug break handler. + * + * This should be called once on application startup. + * + * @param handler Function pointer to debug break handler. + * @return SP_ERROR_* code. + */ + virtual int SetDebugBreakHandler(SPVM_DEBUGBREAK handler) =0; }; class ExceptionHandler; diff --git a/vm/api.cpp b/vm/api.cpp index b4f82fe20..036c2bdc6 100644 --- a/vm/api.cpp +++ b/vm/api.cpp @@ -166,10 +166,20 @@ SourcePawnEngine::SetDebugListener(IDebugListener* pListener) return old; } +int +SourcePawnEngine::SetDebugBreakHandler(SPVM_DEBUGBREAK handler) +{ + if (!Environment::get()->IsDebugBreakEnabled()) + return SP_ERROR_NOTDEBUGGING; + + Environment::get()->SetDebugBreakHandler(handler); + return SP_ERROR_NONE; +} + unsigned int SourcePawnEngine::GetEngineAPIVersion() { - return 4; + return 5; } unsigned int diff --git a/vm/api.h b/vm/api.h index 0826a161a..5c4db8553 100644 --- a/vm/api.h +++ b/vm/api.h @@ -41,6 +41,7 @@ class SourcePawnEngine : public ISourcePawnEngine void FreePageMemory(void* ptr) override; void SetReadWriteExecute(void* ptr); const char* GetErrorString(int err); + int SetDebugBreakHandler(SPVM_DEBUGBREAK handler) override; }; class SourcePawnEngine2 : public ISourcePawnEngine2 diff --git a/vm/base-context.cpp b/vm/base-context.cpp index 309270301..a20e5dd41 100644 --- a/vm/base-context.cpp +++ b/vm/base-context.cpp @@ -74,7 +74,7 @@ BasePluginContext::IsDebugging() } int -BasePluginContext::SetDebugBreak(SPVM_DEBUGBREAK newpfn, SPVM_DEBUGBREAK* oldpfn) +BasePluginContext::SetDebugBreak(void* newpfn, void* oldpfn) { return SP_ERROR_ABORTED; } diff --git a/vm/base-context.h b/vm/base-context.h index 6fe552b25..a10cb70df 100644 --- a/vm/base-context.h +++ b/vm/base-context.h @@ -43,7 +43,6 @@ class BasePluginContext : public SourcePawn::IPluginContext cell_t BlamePluginError(SourcePawn::IPluginFunction* pf, const char* msg, ...) override; IFrameIterator* CreateFrameIterator() override; void DestroyFrameIterator(IFrameIterator* it) override; - int SetDebugBreak(SPVM_DEBUGBREAK newpfn, SPVM_DEBUGBREAK* oldpfn) override; // Removed functions. int PushCell(cell_t value) override; @@ -57,6 +56,7 @@ class BasePluginContext : public SourcePawn::IPluginContext SourcePawn::IVirtualMachine* GetVirtualMachine() override; sp_context_t* GetContext() override; bool IsDebugging() override; + int SetDebugBreak(void* newpfn, void* oldpfn) override; SourcePawn::IPluginDebugInfo* GetDebugInfo() override; int Execute(uint32_t code_addr, cell_t* result) override; int Execute2(SourcePawn::IPluginFunction* function, const cell_t* params, unsigned int num_params, cell_t* result) override; diff --git a/vm/debugging.cpp b/vm/debugging.cpp index 64cde7065..1f23dce60 100644 --- a/vm/debugging.cpp +++ b/vm/debugging.cpp @@ -20,8 +20,8 @@ namespace sp { void InvokeDebugger(PluginContext* ctx, const IErrorReport* report) { - // Continue normal execution, if this plugin isn't being debugged. - if (!ctx->debugbreak()) + // Continue normal execution, if there is no listener registered. + if (!Environment::get()->debugbreak()) return; if (!ctx->IsDebugging()) { @@ -56,7 +56,7 @@ void InvokeDebugger(PluginContext* ctx, const IErrorReport* report) dbginfo.frm = ctx->frm(); // Call debug callback. - ctx->debugbreak()(ctx, dbginfo, report); + Environment::get()->debugbreak()(ctx, dbginfo, report); } } // namespace sp diff --git a/vm/environment.cpp b/vm/environment.cpp index a404f4b96..16371bd20 100644 --- a/vm/environment.cpp +++ b/vm/environment.cpp @@ -33,7 +33,8 @@ using namespace SourcePawn; static Environment* sEnvironment = nullptr; Environment::Environment() - : debugger_(nullptr), + : debug_break_handler_(nullptr), + debugger_(nullptr), eh_top_(nullptr), exception_code_(SP_ERROR_NONE), profiler_(nullptr), diff --git a/vm/environment.h b/vm/environment.h index 0a4dc755b..7527b65f6 100644 --- a/vm/environment.h +++ b/vm/environment.h @@ -116,6 +116,13 @@ class Environment : public ISourcePawnEnvironment return debugger_; } + void SetDebugBreakHandler(SPVM_DEBUGBREAK handler) { + debug_break_handler_ = handler; + } + SPVM_DEBUGBREAK debugbreak() const { + return debug_break_handler_; + } + WatchdogTimer* watchdog() const { return watchdog_timer_; } @@ -170,6 +177,8 @@ class Environment : public ISourcePawnEnvironment ke::AutoPtr builtins_; ke::Mutex mutex_; + SPVM_DEBUGBREAK debug_break_handler_; + IDebugListener* debugger_; ExceptionHandler* eh_top_; int exception_code_; diff --git a/vm/plugin-context.cpp b/vm/plugin-context.cpp index 773b58821..a6958233b 100644 --- a/vm/plugin-context.cpp +++ b/vm/plugin-context.cpp @@ -33,7 +33,6 @@ PluginContext::PluginContext(PluginRuntime* pRuntime) memory_(nullptr), data_size_(m_pRuntime->data().length), mem_size_(m_pRuntime->image()->HeapSize()), - dbreak_(nullptr), m_pNullVec(nullptr), m_pNullString(nullptr) { @@ -381,18 +380,6 @@ PluginContext::GetNullRef(SP_NULL_TYPE type) return NULL; } -int -PluginContext::SetDebugBreak(SPVM_DEBUGBREAK newpfn, SPVM_DEBUGBREAK* oldpfn) -{ - if (!IsDebugging()) - return SP_ERROR_NOTDEBUGGING; - - *oldpfn = dbreak_; - dbreak_ = newpfn; - - return SP_ERROR_NONE; -} - bool PluginContext::IsInExec() { diff --git a/vm/plugin-context.h b/vm/plugin-context.h index 2ba311d26..93c270377 100644 --- a/vm/plugin-context.h +++ b/vm/plugin-context.h @@ -57,7 +57,6 @@ class PluginContext : public BasePluginContext int LocalToStringNULL(cell_t local_addr, char** addr) override; IPluginRuntime* GetRuntime() override; cell_t* GetLocalParams() override; - int SetDebugBreak(SPVM_DEBUGBREAK newpfn, SPVM_DEBUGBREAK* oldpfn) override; bool Invoke(funcid_t fnid, const cell_t* params, unsigned int num_params, cell_t* result); @@ -73,9 +72,6 @@ class PluginContext : public BasePluginContext PluginRuntime* runtime() const { return m_pRuntime; } - SPVM_DEBUGBREAK debugbreak() const { - return dbreak_; - } public: bool IsInExec() override; @@ -143,8 +139,6 @@ class PluginContext : public BasePluginContext uint32_t data_size_; uint32_t mem_size_; - SPVM_DEBUGBREAK dbreak_; - cell_t* m_pNullVec; cell_t* m_pNullString; From 7e9ae3e7316e7a5d8d51418a786a37bed89b5d4f Mon Sep 17 00:00:00 2001 From: Peace-Maker Date: Fri, 28 Sep 2018 15:24:50 +0200 Subject: [PATCH 11/13] Only call the debug handler if debugging was enabled globally ISourcePawnEnvironment::EnableDebugBreak() has to be called before any plugins are loaded to have the call to the debug break handler compiled into the jitted code of the plugins. This is disabled by default, so it has no impact on performance. --- include/sp_vm_api.h | 4 ++++ vm/debugging.cpp | 4 ++++ vm/environment.cpp | 14 +++++++++++++- vm/environment.h | 5 +++++ vm/x86/jit_x86.cpp | 3 +++ 5 files changed, 29 insertions(+), 1 deletion(-) diff --git a/include/sp_vm_api.h b/include/sp_vm_api.h index 977cd5a01..a7e4256c4 100644 --- a/include/sp_vm_api.h +++ b/include/sp_vm_api.h @@ -1600,6 +1600,10 @@ namespace SourcePawn // @brief Returns the message of the pending exception. virtual const char *GetPendingExceptionMessage(const ExceptionHandler *handler) = 0; + + // @brief Enables the line debugger callbacks. This must be called + // before any plugins are loaded. + virtual bool EnableDebugBreak() = 0; }; // @brief This class is the entry-point to using SourcePawn from a DLL. diff --git a/vm/debugging.cpp b/vm/debugging.cpp index 1f23dce60..2322bd2ed 100644 --- a/vm/debugging.cpp +++ b/vm/debugging.cpp @@ -20,6 +20,10 @@ namespace sp { void InvokeDebugger(PluginContext* ctx, const IErrorReport* report) { + // Ignore any calls if this isn't enabled. + if (!Environment::get()->IsDebugBreakEnabled()) + return; + // Continue normal execution, if there is no listener registered. if (!Environment::get()->debugbreak()) return; diff --git a/vm/environment.cpp b/vm/environment.cpp index 16371bd20..5c9241561 100644 --- a/vm/environment.cpp +++ b/vm/environment.cpp @@ -33,7 +33,8 @@ using namespace SourcePawn; static Environment* sEnvironment = nullptr; Environment::Environment() - : debug_break_handler_(nullptr), + : debug_break_enabled_(false), + debug_break_handler_(nullptr), debugger_(nullptr), eh_top_(nullptr), exception_code_(SP_ERROR_NONE), @@ -114,6 +115,17 @@ Environment::SetJitEnabled(bool enabled) jit_enabled_ = enabled; } +bool +Environment::EnableDebugBreak() +{ + // Can't change this after any plugins are loaded. + if (!runtimes_.empty()) + return false; + + debug_break_enabled_ = true; + return true; +} + void Environment::EnableProfiling() { diff --git a/vm/environment.h b/vm/environment.h index 7527b65f6..69e30eec4 100644 --- a/vm/environment.h +++ b/vm/environment.h @@ -61,6 +61,7 @@ class Environment : public ISourcePawnEnvironment void LeaveExceptionHandlingScope(ExceptionHandler* handler) override; bool HasPendingException(const ExceptionHandler* handler) override; const char* GetPendingExceptionMessage(const ExceptionHandler* handler) override; + bool EnableDebugBreak() override; // Runtime functions. const char* GetErrorString(int err); @@ -116,6 +117,9 @@ class Environment : public ISourcePawnEnvironment return debugger_; } + bool IsDebugBreakEnabled() const { + return debug_break_enabled_; + } void SetDebugBreakHandler(SPVM_DEBUGBREAK handler) { debug_break_handler_ = handler; } @@ -177,6 +181,7 @@ class Environment : public ISourcePawnEnvironment ke::AutoPtr builtins_; ke::Mutex mutex_; + bool debug_break_enabled_; SPVM_DEBUGBREAK debug_break_handler_; IDebugListener* debugger_; diff --git a/vm/x86/jit_x86.cpp b/vm/x86/jit_x86.cpp index a9b53d8a8..dfc4e5da1 100644 --- a/vm/x86/jit_x86.cpp +++ b/vm/x86/jit_x86.cpp @@ -1120,6 +1120,9 @@ Compiler::visitREBASE(cell_t addr, cell_t iv_size, cell_t data_size) bool Compiler::visitBREAK() { + if (!Environment::get()->IsDebugBreakEnabled()) + return true; + __ call(&debug_break_); emitCipMapping(op_cip_); return true; From 632169f7dbf115f398a2701763397ca27797513c Mon Sep 17 00:00:00 2001 From: Peace-Maker Date: Sun, 30 Sep 2018 15:00:23 +0200 Subject: [PATCH 12/13] Fix style nits --- vm/smx-v1-image.cpp | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/vm/smx-v1-image.cpp b/vm/smx-v1-image.cpp index 5a5bc54f5..333d306c4 100644 --- a/vm/smx-v1-image.cpp +++ b/vm/smx-v1-image.cpp @@ -787,8 +787,8 @@ SmxV1Image::getFunctionAddress(const SymbolType* syms, const char* function, uce const SymbolType *sym = reinterpret_cast(cursor); if (sym->ident == sp::IDENT_FUNCTION && - sym->name < debug_names_section_->size && - !strcmp(debug_names_ + sym->name, function)) + sym->name < debug_names_section_->size && + !strcmp(debug_names_ + sym->name, function)) { *funcaddr = sym->addr; return true; @@ -843,7 +843,7 @@ SmxV1Image::LookupFunctionAddress(const char* function, const char* file, ucell_ // now find the first line in the function where we can "break" on uint32_t index = 0; for (; index < debug_info_->num_lines && debug_lines_[index].addr < *funcaddr; index++) - /* nothing */; + continue; if (index >= debug_info_->num_lines) return false; @@ -855,13 +855,12 @@ SmxV1Image::LookupFunctionAddress(const char* function, const char* file, ucell_ bool SmxV1Image::LookupLineAddress(const uint32_t line, const char* filename, uint32_t* addr) { - /* Find a suitable "breakpoint address" close to the indicated line (and in - * the specified file). The address is moved up to the next "breakable" line - * if no "breakpoint" is available on the specified line. You can use function - * LookupLine() to find out at which precise line the breakpoint was set. - * - * The filename comparison is strict (case sensitive and path sensitive). - */ + // Find a suitable "breakpoint address" close to the indicated line (and in + // the specified file). The address is moved up to the next "breakable" line + // if no "breakpoint" is available on the specified line. You can use function + // LookupLine() to find out at which precise line the breakpoint was set. + + // The filename comparison is strict (case sensitive and path sensitive). *addr = 0; uint32_t bottomaddr, topaddr; @@ -870,7 +869,7 @@ SmxV1Image::LookupLineAddress(const uint32_t line, const char* filename, uint32_ for (file = 0; file < debug_info_->num_files; file++) { // find the (next) matching instance of the file if (debug_files_[file].name >= debug_names_section_->size || - strcmp(debug_names_ + debug_files_[file].name, filename) != 0) + strcmp(debug_names_ + debug_files_[file].name, filename) != 0) { continue; } @@ -885,8 +884,8 @@ SmxV1Image::LookupLineAddress(const uint32_t line, const char* filename, uint32_ // browse until the line is found or until the top address is exceeded while (index < debug_info_->num_lines && - debug_lines_[index].line < line && - debug_lines_[index].addr < topaddr) + debug_lines_[index].line < line && + debug_lines_[index].addr < topaddr) { index++; } From 9c8a1c05181935e3093109d1f91b829b77df8dcc Mon Sep 17 00:00:00 2001 From: Peace-Maker Date: Sun, 30 Sep 2018 15:02:25 +0200 Subject: [PATCH 13/13] Move IsDebugBreakEnabled check into interpreter No need to check it again when coming from the JIT path, because the debugger wouldn't be called at all if it wasn't enabled. --- vm/debugging.cpp | 4 ---- vm/interpreter.cpp | 4 ++++ 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/vm/debugging.cpp b/vm/debugging.cpp index 2322bd2ed..1f23dce60 100644 --- a/vm/debugging.cpp +++ b/vm/debugging.cpp @@ -20,10 +20,6 @@ namespace sp { void InvokeDebugger(PluginContext* ctx, const IErrorReport* report) { - // Ignore any calls if this isn't enabled. - if (!Environment::get()->IsDebugBreakEnabled()) - return; - // Continue normal execution, if there is no listener registered. if (!Environment::get()->debugbreak()) return; diff --git a/vm/interpreter.cpp b/vm/interpreter.cpp index d5016a460..05a67fd68 100644 --- a/vm/interpreter.cpp +++ b/vm/interpreter.cpp @@ -1036,6 +1036,10 @@ Interpreter::visitSTRADJUST_PRI() bool Interpreter::visitBREAK() { + // Ignore opcode if this isn't enabled. + if (!Environment::get()->IsDebugBreakEnabled()) + return true; + InvokeDebugger(cx_, nullptr); return !env_->hasPendingException(); }