From eec54a0f6c6c8e5b9db98ea524052d220c6c0f72 Mon Sep 17 00:00:00 2001 From: Mike Magruder Date: Fri, 7 Jun 2013 15:12:13 -0700 Subject: [PATCH] Cleanup a bit of exception/error handling between the VM and the debugger We had two similar-but-different functions for getting a notification from the VM about an exception. Cleaned that up by using the proper one for a thrown exception where appropriate, and moving the old one into a hook (like the other VM->debugger hooks) specifically for error messages. --- hphp/runtime/base/builtin_functions.cpp | 7 ++----- hphp/runtime/base/builtin_functions.h | 1 - hphp/runtime/base/execution_context.cpp | 9 ++------- hphp/runtime/debugger/break_point.cpp | 12 ++++++------ hphp/runtime/debugger/break_point.h | 9 ++++++--- hphp/runtime/debugger/cmd/cmd_interrupt.cpp | 2 +- hphp/runtime/debugger/debugger.cpp | 15 --------------- hphp/runtime/debugger/debugger.h | 3 --- hphp/runtime/vm/debugger_hook.cpp | 20 +++++++++++++++++--- hphp/runtime/vm/debugger_hook.h | 3 ++- 10 files changed, 36 insertions(+), 45 deletions(-) diff --git a/hphp/runtime/base/builtin_functions.cpp b/hphp/runtime/base/builtin_functions.cpp index 61d8e3b34..3c3a229c5 100644 --- a/hphp/runtime/base/builtin_functions.cpp +++ b/hphp/runtime/base/builtin_functions.cpp @@ -1433,16 +1433,13 @@ bool function_exists(CStrRef function_name) { /////////////////////////////////////////////////////////////////////////////// // debugger and code coverage instrumentation -inline void throw_exception_unchecked(CObjRef e) { - if (!Eval::Debugger::InterruptException(e)) return; - throw e; -} void throw_exception(CObjRef e) { if (!e.instanceof(SystemLib::s_ExceptionClass)) { raise_error("Exceptions must be valid objects derived from the " "Exception base class"); } - throw_exception_unchecked(e); + DEBUGGER_ATTACHED_ONLY(phpDebuggerExceptionThrownHook(e.get())); + throw e; } /////////////////////////////////////////////////////////////////////////////// diff --git a/hphp/runtime/base/builtin_functions.h b/hphp/runtime/base/builtin_functions.h index 7c80569ee..78ac36221 100644 --- a/hphp/runtime/base/builtin_functions.h +++ b/hphp/runtime/base/builtin_functions.h @@ -230,7 +230,6 @@ void NEVER_INLINE throw_invalid_property_name(CStrRef name) ATTRIBUTE_NORETURN; void NEVER_INLINE throw_null_object_prop(); void NEVER_INLINE throw_null_get_object_prop(); void NEVER_INLINE raise_null_object_prop(); -void throw_exception_unchecked(CObjRef e); void throw_exception(CObjRef e); bool set_line(int line0, int char0 = 0, int line1 = 0, int char1 = 0); diff --git a/hphp/runtime/base/execution_context.cpp b/hphp/runtime/base/execution_context.cpp index e66a898a1..10e3071ff 100644 --- a/hphp/runtime/base/execution_context.cpp +++ b/hphp/runtime/base/execution_context.cpp @@ -617,18 +617,13 @@ void BaseExecutionContext::handleError(const std::string &msg, handled = callUserErrorHandler(ee, errnum, false); } if (mode == AlwaysThrow || (mode == ThrowIfUnhandled && !handled)) { - try { - if (!Eval::Debugger::InterruptException(String(msg))) return; - } catch (const Eval::DebuggerClientExitException &e) {} + DEBUGGER_ATTACHED_ONLY(phpDebuggerErrorHook(msg)); throw FatalErrorException(msg, bt); } if (!handled && (RuntimeOption::NoSilencer || (getErrorReportingLevel() & errnum) != 0)) { - try { - if (!Eval::Debugger::InterruptException(String(msg))) return; - } catch (const Eval::DebuggerClientExitException &e) {} - + DEBUGGER_ATTACHED_ONLY(phpDebuggerErrorHook(msg)); String file = empty_string; int line = 0; if (RuntimeOption::InjectedStackTrace) { diff --git a/hphp/runtime/debugger/break_point.cpp b/hphp/runtime/debugger/break_point.cpp index b13a95ba3..17d6b1ab4 100644 --- a/hphp/runtime/debugger/break_point.cpp +++ b/hphp/runtime/debugger/break_point.cpp @@ -40,9 +40,9 @@ int InterruptSite::getFileLen() const { std::string InterruptSite::desc() const { TRACE(2, "InterruptSite::desc\n"); string ret; - if (m_exception.isNull()) { + if (m_error.isNull()) { ret = "Break"; - } else if (m_exception.isObject()) { + } else if (m_error.isObject()) { ret = "Exception thrown"; } else { ret = "Error occurred"; @@ -74,8 +74,8 @@ std::string InterruptSite::desc() const { return ret; } -InterruptSite::InterruptSite(bool hardBreakPoint, CVarRef e) - : m_exception(e), m_class(nullptr), m_function(nullptr), +InterruptSite::InterruptSite(bool hardBreakPoint, CVarRef error) + : m_error(error), m_class(nullptr), m_function(nullptr), m_file((StringData*)nullptr), m_line0(0), m_char0(0), m_line1(0), m_char1(0), m_unit(nullptr), m_valid(false), m_funcEntry(false) { @@ -338,7 +338,7 @@ bool BreakPointInfo::match(InterruptType interrupt, InterruptSite &site) { checkUrl(site.url()); case ExceptionThrown: return - checkException(site.getException()) && + checkExceptionOrError(site.getError()) && checkUrl(site.url()) && checkClause(); case BreakPointReached: { @@ -879,7 +879,7 @@ bool BreakPointInfo::Match(const char *haystack, int haystack_len, return r.same(1); } -bool BreakPointInfo::checkException(CVarRef e) { +bool BreakPointInfo::checkExceptionOrError(CVarRef e) { TRACE(2, "BreakPointInfo::checkException\n"); assert(!e.isNull()); if (e.isObject()) { diff --git a/hphp/runtime/debugger/break_point.h b/hphp/runtime/debugger/break_point.h index eb4c6adef..c42cc6a9a 100644 --- a/hphp/runtime/debugger/break_point.h +++ b/hphp/runtime/debugger/break_point.h @@ -57,7 +57,10 @@ public: int32_t getChar0() const { return m_char0; } int32_t getLine1() const { return m_line1; } int32_t getChar1() const { return m_char1; } - CVarRef getException() { return m_exception; } + + // Optionally provided by VM, could be an exception object, a string, or null + // depending on the context. + CVarRef getError() { return m_error; } std::string &url() const { return m_url; } std::string desc() const; @@ -72,7 +75,7 @@ public: bool funcEntry() const { return m_funcEntry; } private: - Variant m_exception; + Variant m_error; // cached mutable const char *m_class; @@ -206,7 +209,7 @@ private: int32_t parseFileLocation(const std::string &str, int32_t offset); bool parseLines(const std::string &token); - bool checkException(CVarRef e); + bool checkExceptionOrError(CVarRef e); bool checkUrl(std::string &url); bool checkLines(int line); bool checkStack(InterruptSite &site); diff --git a/hphp/runtime/debugger/cmd/cmd_interrupt.cpp b/hphp/runtime/debugger/cmd/cmd_interrupt.cpp index 95c7ee58b..a3c69fcdd 100644 --- a/hphp/runtime/debugger/cmd/cmd_interrupt.cpp +++ b/hphp/runtime/debugger/cmd/cmd_interrupt.cpp @@ -43,7 +43,7 @@ void CmdInterrupt::sendImpl(DebuggerThriftBuffer &thrift) { thrift.write(m_site->getNamespace()); thrift.write(m_site->getClass()); thrift.write(m_site->getFunction()); - Variant e = m_site->getException(); + Variant e = m_site->getError(); if (e.isNull()) { thrift.write(""); } else if (e.isObject()) { diff --git a/hphp/runtime/debugger/debugger.cpp b/hphp/runtime/debugger/debugger.cpp index e18df27be..aca83756c 100644 --- a/hphp/runtime/debugger/debugger.cpp +++ b/hphp/runtime/debugger/debugger.cpp @@ -220,21 +220,6 @@ void Debugger::InterruptPSPEnded(const char *url) { } } -// Called directly from exception handling to indicate a user error handler -// failed to handle an exeption. NB: this is quite distinct from the hook called -// from iopThrow named phpDebuggerExceptionHook(). -bool Debugger::InterruptException(CVarRef e) { - TRACE(2, "Debugger::InterruptException\n"); - if (RuntimeOption::EnableDebugger) { - ThreadInfo *ti = ThreadInfo::s_threadInfo.getNoCheck(); - if (ti->m_reqInjectionData.getDebugger()) { - HPHP::Transl::VMRegAnchor _; - InterruptVMHook(ExceptionThrown, e); - } - } - return true; -} - // Primary entrypoint for the debugger from the VM. Called in response to a host // of VM events that the debugger is interested in. The debugger will execute // any logic needed to handle the event, and will block below this to wait for diff --git a/hphp/runtime/debugger/debugger.h b/hphp/runtime/debugger/debugger.h index 784a7ff22..fdfd13347 100644 --- a/hphp/runtime/debugger/debugger.h +++ b/hphp/runtime/debugger/debugger.h @@ -80,9 +80,6 @@ public: static void InterruptRequestEnded(const char *url); static void InterruptPSPEnded(const char *url); - // Called when a user handler fails to handle an exception. - static bool InterruptException(CVarRef e); - // Interrupt from VM static void InterruptVMHook(int type = BreakPointReached, CVarRef e = null_variant); diff --git a/hphp/runtime/vm/debugger_hook.cpp b/hphp/runtime/vm/debugger_hook.cpp index 35dbd8586..9baa290cd 100644 --- a/hphp/runtime/vm/debugger_hook.cpp +++ b/hphp/runtime/vm/debugger_hook.cpp @@ -66,14 +66,13 @@ void phpDebuggerOpcodeHook(const uchar* pc) { } // Hook called from iopThrow to signal that we are about to throw an exception. -// NB: this does not hook any portion of exception unwind. -void phpDebuggerExceptionThrownHook(ObjectData* e) { +void phpDebuggerExceptionThrownHook(ObjectData* exception) { TRACE(5, "in phpDebuggerExceptionThrownHook()\n"); if (UNLIKELY(g_vmContext->m_dbgNoBreak)) { TRACE(5, "NoBreak flag is on\n"); return; } - Eval::Debugger::InterruptVMHook(Eval::ExceptionThrown, e); + Eval::Debugger::InterruptVMHook(Eval::ExceptionThrown, exception); TRACE(5, "out phpDebuggerExceptionThrownHook()\n"); } @@ -89,6 +88,21 @@ void phpDebuggerExceptionHandlerHook() { TRACE(5, "out phpDebuggerExceptionHandlerHook()\n"); } +// Hook called when the VM raises an error. +void phpDebuggerErrorHook(const std::string& message) { + TRACE(5, "in phpDebuggerErrorHook()\n"); + if (UNLIKELY(g_vmContext->m_dbgNoBreak)) { + TRACE(5, "NoBreak flag is on\n"); + return; + } + try { + Eval::Debugger::InterruptVMHook(Eval::ExceptionThrown, String(message)); + } catch (const Eval::DebuggerClientExitException &e) { + // Don't disturb the error handler just because a debugger quits. + } + TRACE(5, "out phpDebuggerErrorHook()\n"); +} + bool isDebuggerAttachedProcess() { return Eval::Debugger::CountConnectedProxy() > 0; } diff --git a/hphp/runtime/vm/debugger_hook.h b/hphp/runtime/vm/debugger_hook.h index 8b34c0a1a..d94cc4c57 100644 --- a/hphp/runtime/vm/debugger_hook.h +++ b/hphp/runtime/vm/debugger_hook.h @@ -39,8 +39,9 @@ namespace HPHP { // debugging to give the debugger a chance to act. The debugger may block // execution indefinitely within one of these hooks. void phpDebuggerOpcodeHook(const uchar* pc); -void phpDebuggerExceptionThrownHook(ObjectData* e); +void phpDebuggerExceptionThrownHook(ObjectData* exception); void phpDebuggerExceptionHandlerHook(); +void phpDebuggerErrorHook(const std::string& message); void phpDebuggerEvalHook(const Func* f); void phpDebuggerFileLoadHook(Eval::PhpFile* efile); class Class;