From f3d5a8abb18808848979672e4f7cfafeb694b42a Mon Sep 17 00:00:00 2001 From: Mike Magruder Date: Tue, 25 Jun 2013 12:47:27 -0700 Subject: [PATCH] Allow ctrl-c during eval A client couldn't break execution during eval. There used to be a lot of barriers to making that right, but I fixed most of them with a previous diff on unifying client-side event loops. Now the only barrier was that a server-side thread processing an interrupt was blocking the signal polling thread by holding a mutex while processing the interrupt. Changed to set a flag to disable polling when starting to process the interrupt (and unsetting it when done), while still synchronizing with the signal polling thread to ensure only one thread is sending the client messages at a time. Added logic to re-enable polling while executing PHP for eval, print, etc. Plumbed the proxy thru to the point where we check the clause on conditional breakpoints, too, since that's the third (and final) place we do this. --- hphp/runtime/debugger/break_point.cpp | 12 ++-- hphp/runtime/debugger/break_point.h | 6 +- hphp/runtime/debugger/cmd/cmd_eval.cpp | 7 ++- hphp/runtime/debugger/cmd/cmd_interrupt.cpp | 5 +- hphp/runtime/debugger/cmd/cmd_interrupt.h | 3 +- hphp/runtime/debugger/cmd/cmd_print.cpp | 10 +++- hphp/runtime/debugger/debugger_proxy.cpp | 63 ++++++++++++++++++--- hphp/runtime/debugger/debugger_proxy.h | 14 ++++- 8 files changed, 96 insertions(+), 24 deletions(-) diff --git a/hphp/runtime/debugger/break_point.cpp b/hphp/runtime/debugger/break_point.cpp index 7befd2ca6..a0036e84a 100644 --- a/hphp/runtime/debugger/break_point.cpp +++ b/hphp/runtime/debugger/break_point.cpp @@ -353,7 +353,8 @@ bool BreakPointInfo::same(BreakPointInfoPtr bpi) { return desc() == bpi->desc(); } -bool BreakPointInfo::match(InterruptType interrupt, InterruptSite &site) { +bool BreakPointInfo::match(DebuggerProxy &proxy, InterruptType interrupt, + InterruptSite &site) { TRACE(2, "BreakPointInfo::match\n"); if (m_interruptType == interrupt) { switch (interrupt) { @@ -365,13 +366,13 @@ bool BreakPointInfo::match(InterruptType interrupt, InterruptSite &site) { case ExceptionThrown: return checkExceptionOrError(site.getError()) && - checkUrl(site.url()) && checkClause(); + checkUrl(site.url()) && checkClause(proxy); case BreakPointReached: { bool match = Match(site.getFile(), site.getFileLen(), m_file, m_regex, false) && checkLines(site.getLine0()) && checkStack(site) && - checkUrl(site.url()) && checkClause(); + checkUrl(site.url()) && checkClause(proxy); if (!getFuncName().empty()) { // function entry breakpoint @@ -999,7 +1000,7 @@ bool BreakPointInfo::checkStack(InterruptSite &site) { return true; } -bool BreakPointInfo::checkClause() { +bool BreakPointInfo::checkClause(DebuggerProxy &proxy) { TRACE(2, "BreakPointInfo::checkClause\n"); if (!m_clause.empty()) { if (m_php.empty()) { @@ -1014,7 +1015,8 @@ bool BreakPointInfo::checkClause() { // Don't hit more breakpoints while attempting to decide if we should stop // at this breakpoint. EvalBreakControl eval(true); - Variant ret = DebuggerProxy::ExecutePHP(m_php, output, false, 0); + Variant ret = proxy.ExecutePHP(m_php, output, 0, + DebuggerProxy::ExecutePHPFlagsNone); if (m_check) { return ret.toBoolean(); } diff --git a/hphp/runtime/debugger/break_point.h b/hphp/runtime/debugger/break_point.h index a22a24100..82b22ad09 100644 --- a/hphp/runtime/debugger/break_point.h +++ b/hphp/runtime/debugger/break_point.h @@ -119,6 +119,7 @@ private: DECLARE_BOOST_TYPES(DFunctionInfo); DECLARE_BOOST_TYPES(BreakPointInfo); +DECLARE_BOOST_TYPES(DebuggerProxy); class BreakPointInfo { public: // The state of the break point @@ -157,7 +158,8 @@ public: bool valid(); bool same(BreakPointInfoPtr bpi); - bool match(InterruptType interrupt, InterruptSite &site); + bool match(DebuggerProxy &proxy, InterruptType interrupt, + InterruptSite &site); int index() const { return m_index;} std::string state(bool padding) const; @@ -242,7 +244,7 @@ private: bool checkUrl(std::string &url); bool checkLines(int line); bool checkStack(InterruptSite &site); - bool checkClause(); + bool checkClause(DebuggerProxy &proxy); }; /////////////////////////////////////////////////////////////////////////////// diff --git a/hphp/runtime/debugger/cmd/cmd_eval.cpp b/hphp/runtime/debugger/cmd/cmd_eval.cpp index 737a3f967..192366348 100644 --- a/hphp/runtime/debugger/cmd/cmd_eval.cpp +++ b/hphp/runtime/debugger/cmd/cmd_eval.cpp @@ -67,11 +67,16 @@ void CmdEval::setClientOutput(DebuggerClient &client) { client.setOTValues(values.create()); } +// NB: unlike most other commands, the client expects that more interrupts +// can occur while we're doing the server-side work for an eval. bool CmdEval::onServer(DebuggerProxy &proxy) { PCFilter* locSave = g_vmContext->m_lastLocFilter; g_vmContext->m_lastLocFilter = new PCFilter(); g_vmContext->setDebuggerBypassCheck(m_bypassAccessCheck); - DebuggerProxy::ExecutePHP(m_body, m_output, !proxy.isLocal(), m_frame); + proxy.ExecutePHP(m_body, m_output, m_frame, + DebuggerProxy::ExecutePHPFlagsAtInterrupt | + (!proxy.isLocal() ? DebuggerProxy::ExecutePHPFlagsLog : + DebuggerProxy::ExecutePHPFlagsNone)); g_vmContext->setDebuggerBypassCheck(false); delete g_vmContext->m_lastLocFilter; g_vmContext->m_lastLocFilter = locSave; diff --git a/hphp/runtime/debugger/cmd/cmd_interrupt.cpp b/hphp/runtime/debugger/cmd/cmd_interrupt.cpp index 7be8b463b..51daaa04d 100644 --- a/hphp/runtime/debugger/cmd/cmd_interrupt.cpp +++ b/hphp/runtime/debugger/cmd/cmd_interrupt.cpp @@ -283,7 +283,8 @@ bool CmdInterrupt::onServer(DebuggerProxy &proxy) { return proxy.sendToClient(this); } -bool CmdInterrupt::shouldBreak(const BreakPointInfoPtrVec &bps, +bool CmdInterrupt::shouldBreak(DebuggerProxy &proxy, + const BreakPointInfoPtrVec &bps, int stackDepth) { switch (m_interrupt) { @@ -301,7 +302,7 @@ bool CmdInterrupt::shouldBreak(const BreakPointInfoPtrVec &bps, for (unsigned int i = 0; i < bps.size(); i++) { if (bps[i]->m_state != BreakPointInfo::Disabled && bps[i]->breakable(stackDepth) && - bps[i]->match(getInterruptType(), *getSite())) { + bps[i]->match(proxy, getInterruptType(), *getSite())) { BreakPointInfoPtr bp(new BreakPointInfo()); *bp = *bps[i]; // make a copy m_matched.push_back(bp); diff --git a/hphp/runtime/debugger/cmd/cmd_interrupt.h b/hphp/runtime/debugger/cmd/cmd_interrupt.h index 303e39539..edcbb9c67 100644 --- a/hphp/runtime/debugger/cmd/cmd_interrupt.h +++ b/hphp/runtime/debugger/cmd/cmd_interrupt.h @@ -47,7 +47,8 @@ public: virtual void setClientOutput(DebuggerClient &client); virtual bool onServer(DebuggerProxy &proxy); - bool shouldBreak(const BreakPointInfoPtrVec &bps, int stackDepth); + bool shouldBreak(DebuggerProxy &proxy, const BreakPointInfoPtrVec &bps, + int stackDepth); std::string getFileLine() const; InterruptSite *getSite() { return m_site;} diff --git a/hphp/runtime/debugger/cmd/cmd_print.cpp b/hphp/runtime/debugger/cmd/cmd_print.cpp index 19ded3e73..ebdfec5a2 100644 --- a/hphp/runtime/debugger/cmd/cmd_print.cpp +++ b/hphp/runtime/debugger/cmd/cmd_print.cpp @@ -373,14 +373,20 @@ void CmdPrint::setClientOutput(DebuggerClient &client) { client.setOTValues(values); } +// NB: unlike most other commands, the client expects that more interrupts +// can occur while we're doing the server-side work for a print. bool CmdPrint::onServer(DebuggerProxy &proxy) { PCFilter* locSave = g_vmContext->m_lastLocFilter; g_vmContext->m_lastLocFilter = new PCFilter(); g_vmContext->setDebuggerBypassCheck(m_bypassAccessCheck); { EvalBreakControl eval(m_noBreak); - m_ret = DebuggerProxy::ExecutePHP(DebuggerProxy::MakePHPReturn(m_body), - m_output, !proxy.isLocal(), m_frame); + m_ret = + proxy.ExecutePHP(DebuggerProxy::MakePHPReturn(m_body), + m_output, m_frame, + DebuggerProxy::ExecutePHPFlagsAtInterrupt | + (!proxy.isLocal() ? DebuggerProxy::ExecutePHPFlagsLog : + DebuggerProxy::ExecutePHPFlagsNone)); } g_vmContext->setDebuggerBypassCheck(false); delete g_vmContext->m_lastLocFilter; diff --git a/hphp/runtime/debugger/debugger_proxy.cpp b/hphp/runtime/debugger/debugger_proxy.cpp index 081eb91ad..25d01e52d 100644 --- a/hphp/runtime/debugger/debugger_proxy.cpp +++ b/hphp/runtime/debugger/debugger_proxy.cpp @@ -34,7 +34,7 @@ DebuggerProxy::DebuggerProxy(SmartPtr socket, bool local) : m_stopped(false), m_local(local), m_dummySandbox(nullptr), m_hasBreakPoints(false), m_threadMode(Normal), m_thread(0), m_signalThread(this, &DebuggerProxy::pollSignal), - m_signum(CmdSignal::SignalNone) { + m_okayToPoll(true), m_signum(CmdSignal::SignalNone) { TRACE(2, "DebuggerProxy::DebuggerProxy\n"); m_thrift.create(socket); m_dummyInfo = DSandboxInfo::CreateDummyInfo((int64_t)this); @@ -224,8 +224,12 @@ void DebuggerProxy::interrupt(CmdInterrupt &cmd) { if (checkFlowBreak(cmd)) { while (true) { try { - Lock lock(m_signalMutex); // Block the signal polling thread. - m_signum = CmdSignal::SignalNone; + // We're about to send the client an interrupt and start + // waiting for commands back from it. Disable signal polling + // during this time, since our protocol requires that only one + // thread talk to the client at a time. + disableSignalPolling(); + SCOPE_EXIT { enableSignalPolling(); }; processInterrupt(cmd); // If we've just processed a breakpoint, change it so we don't break at // it again until we're off this site. @@ -278,6 +282,24 @@ bool DebuggerProxy::sendToClient(DebuggerCommand *cmd) { return cmd->send(m_thrift); } +// Allow the signal polling thread to send CmdSignal messages to the +// client to see if it there is a signal to repond to. +void DebuggerProxy::enableSignalPolling() { + Lock lock(m_signalMutex); + m_okayToPoll = true; +} + +// Prevent the signal polling thread from sending CmdSignal messages +// to the client. Polling is disabled while a thread is stopped at an +// interrupt and responding to messages from the client. +void DebuggerProxy::disableSignalPolling() { + Lock lock(m_signalMutex); + m_okayToPoll = false; + // Drop any pending signal since we're about to start processing + // interrupts again anyway. + m_signum = CmdSignal::SignalNone; +} + void DebuggerProxy::startSignalThread() { TRACE(2, "DebuggerProxy::startSignalThread\n"); m_signalThread.start(); @@ -309,6 +331,9 @@ void DebuggerProxy::pollSignal() { // Block any threads that might be interrupting from communicating with the // client until we're done with this poll. Lock lock(m_signalMutex); + // Don't actually poll if another thread is already in a command + // processing loop with the client. + if (!m_okayToPoll) continue; // Send CmdSignal over to the client and wait for a response. CmdSignal cmd; @@ -409,6 +434,8 @@ std::string DebuggerProxy::MakePHPReturn(const std::string &php) { return "swapOutputBuffer(nullptr); g_context->setStdout(append_stdout, &sb); - if (log) { + if (flags & ExecutePHPFlagsLog) { Logger::SetThreadHook(append_stderr, &sb); } try { String code(php.c_str(), php.size(), CopyString); + // @TODO: enable this once task #2608250 is completed. +#if 0 + // We're about to start executing more PHP. This is typcially done + // in response to commands from the client, and the client expects + // those commands to send more interrupts since, of course, the + // user might want to debug the code we're about to run. If we're + // already processing an interrupt, enable signal polling around + // the execution fo the new PHP to ensure that we can handle + // signals while doing so. + if (flags & ExecutePHPFlagsAtInterrupt) enableSignalPolling(); + SCOPE_EXIT { + if (flags & ExecutePHPFlagsAtInterrupt) disableSignalPolling(); + }; +#endif g_vmContext->evalPHPDebugger((TypedValue*)&ret, code.get(), frame); } catch (InvalidFunctionCallException &e) { sb.append(Debugger::ColorStderr(String(e.what()))); @@ -670,7 +715,7 @@ Variant DebuggerProxy::ExecutePHP(const std::string &php, String &output, } g_context->setStdout(nullptr, nullptr); g_context->swapOutputBuffer(save); - if (log) { + if (flags & ExecutePHPFlagsLog) { Logger::SetThreadHook(nullptr, nullptr); } output = sb.detach(); @@ -684,7 +729,7 @@ BreakPointInfoPtr DebuggerProxy::getBreakPointAtCmd(CmdInterrupt& cmd) { for (unsigned int i = 0; i < m_breakpoints.size(); ++i) { BreakPointInfoPtr bp = m_breakpoints[i]; if (bp->m_state != BreakPointInfo::Disabled && - bp->match(cmd.getInterruptType(), *cmd.getSite())) { + bp->match(*this, cmd.getInterruptType(), *cmd.getSite())) { return bp; } } @@ -732,7 +777,7 @@ void DebuggerProxy::changeBreakPointDepth(CmdInterrupt& cmd) { // if the site changes, then update the breakpoint depth BreakPointInfoPtr bp = m_breakpoints[i]; if (bp->m_state != BreakPointInfo::Disabled && - !bp->match(cmd.getInterruptType(), *cmd.getSite())) { + !bp->match(*this, cmd.getInterruptType(), *cmd.getSite())) { m_breakpoints[i]->changeBreakPointDepth(getRealStackDepth()); } } diff --git a/hphp/runtime/debugger/debugger_proxy.h b/hphp/runtime/debugger/debugger_proxy.h index e73bf6733..0c026bbb1 100644 --- a/hphp/runtime/debugger/debugger_proxy.h +++ b/hphp/runtime/debugger/debugger_proxy.h @@ -58,8 +58,6 @@ public: static std::string MakePHP(const std::string &php); static std::string MakePHPReturn(const std::string &php); - static Variant ExecutePHP(const std::string &php, String &output, bool log, - int frame); public: DebuggerProxy(SmartPtr socket, bool local); @@ -103,11 +101,22 @@ public: bool getClientConnectionInfo(VRefParam address, VRefParam port); + enum ExecutePHPFlags { + ExecutePHPFlagsNone = 0x0, // No logging, not at an interrupt + ExecutePHPFlagsLog = 0x1, // Add logs to the output string + ExecutePHPFlagsAtInterrupt = 0x02 // Called when stopped at an interrupt + }; + + Variant ExecutePHP(const std::string &php, String &output, int frame, + int flags); + private: bool blockUntilOwn(CmdInterrupt &cmd, bool check); bool checkBreakPoints(CmdInterrupt &cmd); bool checkFlowBreak(CmdInterrupt &cmd); void processInterrupt(CmdInterrupt &cmd); + void enableSignalPolling(); + void disableSignalPolling(); DThreadInfoPtr createThreadInfo(const std::string &desc); @@ -137,6 +146,7 @@ private: Mutex m_signalMutex; // who can talk to client AsyncFunc m_signalThread; // polling signals from client + bool m_okayToPoll; // whether the polling thread can send polls to the client Mutex m_signumMutex; int m_signum;