From 60284bed4dd8d850918e98c975851068304a82db Mon Sep 17 00:00:00 2001 From: Arnab De Date: Wed, 29 Sep 2021 07:42:02 -0700 Subject: [PATCH] Debugger should not lookup units Summary: There is no need to eagerly lookup units in order to resolve breakpoints. We can resolve breakpoints when an unit is loaded by HHVM. The current scheme works only for source line breakpoints when we have full path of the file. Reviewed By: jeffreytan81 Differential Revision: D30997213 fbshipit-source-id: e6ab0bdccef80dcaf35c71ccce6f84ed3ef42778 --- hphp/runtime/ext/vsdebug/debugger.cpp | 85 ++++++++-------------- hphp/test/slow/ext_vsdebug/breakpoints.php | 2 + hphp/test/slow/ext_vsdebug/common.inc | 8 +- hphp/test/slow/ext_vsdebug/conditional_bp.php | 3 + hphp/test/slow/ext_vsdebug/context.php | 3 + hphp/test/slow/ext_vsdebug/exception_bp.php | 3 + hphp/test/slow/ext_vsdebug/info.php | 3 + hphp/test/slow/ext_vsdebug/step.php | 3 + hphp/test/slow/ext_vsdebug/todebugdisplay.php | 3 + hphp/test/slow/ext_vsdebug/variables_all_types.php | 3 + 10 files changed, 60 insertions(+), 56 deletions(-) diff --git a/hphp/runtime/ext/vsdebug/debugger.cpp b/hphp/runtime/ext/vsdebug/debugger.cpp index 023e6b4831c..6983fab3fd5 100644 --- a/hphp/runtime/ext/vsdebug/debugger.cpp +++ b/hphp/runtime/ext/vsdebug/debugger.cpp @@ -1583,61 +1583,36 @@ void Debugger::tryInstallBreakpoints(DebuggerRequestInfo* ri) { } bool resolved = tryResolveBreakpoint(ri, breakpointId, bp); - if (!resolved) { - if (!RuntimeOption::RepoAuthoritative && - bp->m_type == BreakpointType::Source) { - - // It's possible this compilation unit just isn't loaded yet. Try - // to force a pre-load and compile of the unit and place the bp. - HPHP::String unitPath(bp->m_path.c_str()); - const auto compilationUnit = lookupUnit(unitPath.get(), "", nullptr, - Native::s_noNativeFuncs, false); - - if (compilationUnit != nullptr) { - ri->m_breakpointInfo->m_loadedUnits[bp->m_path] = compilationUnit; - resolved = tryResolveBreakpoint(ri, breakpointId, bp); - } - - // In debugger clients that support multiple languages like Nuclide, - // users tend to leave breakpoints set in files that are for other - // debuggers. It's annoying to see warnings in those cases. Assume - // any file path that doesn't end in PHP is ok not to tell the user - // that the breakpoint failed to set. - const bool phpFile = - bp->m_path.size() >= 4 && - std::equal( - bp->m_path.rbegin(), - bp->m_path.rend(), - std::string(".php").rbegin() - ); - - if (phpFile && !resolved) { - std::string resolveMsg = "Warning: request "; - resolveMsg += std::to_string(getCurrentThreadId()); - resolveMsg += " could not resolve breakpoint #"; - resolveMsg += std::to_string(breakpointId); - resolveMsg += ". The Hack/PHP file at "; - resolveMsg += bp->m_path; - - if (compilationUnit == nullptr) { - resolveMsg += " could not be loaded, or failed to compile."; - } else { - resolveMsg += " was loaded, but the breakpoint did not resolve " - "to any executable instruction."; - } - - sendUserMessage( - resolveMsg.c_str(), - DebugTransport::OutputLevelWarning - ); - } - } - - // This breakpoint could not be resolved yet. As new compilation units - // are loaded, we'll try again. - if (!resolved || bp->isRelativeBp()) { - ri->m_breakpointInfo->m_unresolvedBreakpoints.emplace(breakpointId); - } + // This breakpoint could not be resolved yet. As new compilation units + // are loaded, we'll try again. + // In debugger clients that support multiple languages like Nuclide, + // users tend to leave breakpoints set in files that are for other + // debuggers. It's annoying to see warnings in those cases. Assume + // any file path that doesn't end in PHP is ok not to tell the user + // that the breakpoint failed to set. + const bool phpFile = + bp->m_path.size() >= 4 && + std::equal( + bp->m_path.rbegin(), + bp->m_path.rend(), + std::string(".php").rbegin() + ); + if (phpFile && !resolved) { + std::string resolveMsg = "Warning: request "; + resolveMsg += std::to_string(getCurrentThreadId()); + resolveMsg += " could not resolve breakpoint #"; + resolveMsg += std::to_string(breakpointId); + resolveMsg += ". The Hack/PHP file at "; + resolveMsg += bp->m_path; + resolveMsg += " is not yet loaded, or failed to compile."; + + sendUserMessage( + resolveMsg.c_str(), + DebugTransport::OutputLevelWarning + ); + } + if (!resolved || bp->isRelativeBp()) { + ri->m_breakpointInfo->m_unresolvedBreakpoints.emplace(breakpointId); } } diff --git a/hphp/test/slow/ext_vsdebug/breakpoints.php b/hphp/test/slow/ext_vsdebug/breakpoints.php index 270786bfdcf..1363c039fc5 100644 --- a/hphp/test/slow/ext_vsdebug/breakpoints.php +++ b/hphp/test/slow/ext_vsdebug/breakpoints.php @@ -13,6 +13,8 @@ $breakpoints = varray[ ]; $testProcess = vsDebugLaunch(__FILE__ . ".test", true, $breakpoints); +// Skip breakpoint resolution messages. +skipMessages(count($breakpoints[0]{'breakpoints'})); checkForOutput($testProcess, "Hello world.\n", "stdout"); diff --git a/hphp/test/slow/ext_vsdebug/common.inc b/hphp/test/slow/ext_vsdebug/common.inc index a42eecd8930..4021d15c99c 100644 --- a/hphp/test/slow/ext_vsdebug/common.inc +++ b/hphp/test/slow/ext_vsdebug/common.inc @@ -161,6 +161,12 @@ function getHhvmPath() { return PHP_BINARY; } +function skipMessages($skip) { + for ($i = 0; $i < $skip; $i++) { + getNextVsDebugMessage(); + } +} + function checkForOutput($testProcess, $expectedOutput, $expectedCategory, $error = false) { $pipes = $testProcess[1]; $msg = json_decode(getNextVsDebugMessage(), true); @@ -400,7 +406,7 @@ function vsDebugLaunch($scriptPath, $sendDefaultInit = true, $breakpoints = varr checkObjEqualRecursively($msg, darray["type" => "event", "event" => "readyForEvaluations"]); // Set any initial breakpoints that need to be set before we resume the target. - setBreakpoints($breakpoints); + setBreakpoints($breakpoints, false); // Send configuration done, which resumes the target. $configDoneCommand = darray[ diff --git a/hphp/test/slow/ext_vsdebug/conditional_bp.php b/hphp/test/slow/ext_vsdebug/conditional_bp.php index 9cc315a7e91..8eb05e88cfb 100644 --- a/hphp/test/slow/ext_vsdebug/conditional_bp.php +++ b/hphp/test/slow/ext_vsdebug/conditional_bp.php @@ -15,6 +15,9 @@ $breakpoints = varray[ $testProcess = vsDebugLaunch(__FILE__ . ".test", true, $breakpoints); +// Skip breakpoint resolution messages. +skipMessages(count($breakpoints[0]{'breakpoints'})); + // Breakpoint 1 should not hit: its condition is false. // Breakpoint 2 should hit: condition is true. verifyBpHit($breakpoints[0]{'path'}, $breakpoints[0]{'breakpoints'}[1]); diff --git a/hphp/test/slow/ext_vsdebug/context.php b/hphp/test/slow/ext_vsdebug/context.php index b424095223b..0bf20b0e20d 100644 --- a/hphp/test/slow/ext_vsdebug/context.php +++ b/hphp/test/slow/ext_vsdebug/context.php @@ -17,6 +17,9 @@ $breakpoints = varray[ $testProcess = vsDebugLaunch(__FILE__ . ".test", true, $breakpoints); +// Skip breakpoint resolution messages. +skipMessages(count($breakpoints[0]{'breakpoints'})); + // Verify we hit breakpoint 1. verifyBpHit($breakpoints[0]{'path'}, $breakpoints[0]{'breakpoints'}[0]); diff --git a/hphp/test/slow/ext_vsdebug/exception_bp.php b/hphp/test/slow/ext_vsdebug/exception_bp.php index fcd32bc4d2e..0add5e91fe9 100644 --- a/hphp/test/slow/ext_vsdebug/exception_bp.php +++ b/hphp/test/slow/ext_vsdebug/exception_bp.php @@ -11,6 +11,9 @@ $breakpoints = varray[ $testProcess = vsDebugLaunch(__FILE__ . ".test", true, $breakpoints); +// Skip breakpoint resolution messages. +skipMessages(count($breakpoints[0]{'breakpoints'})); + verifyBpHit($breakpoints[0]{'path'}, $breakpoints[0]{'breakpoints'}[0]); $exnBpCommand = darray[ diff --git a/hphp/test/slow/ext_vsdebug/info.php b/hphp/test/slow/ext_vsdebug/info.php index 93477a9319d..a2b37fb05ec 100644 --- a/hphp/test/slow/ext_vsdebug/info.php +++ b/hphp/test/slow/ext_vsdebug/info.php @@ -37,6 +37,9 @@ $breakpoints = varray[ $testProcess = vsDebugLaunch($path, true, $breakpoints); +// Skip breakpoint resolution messages. +skipMessages(count($breakpoints[0]{'breakpoints'})); + // Verify we hit breakpoint 1. verifyBpHit($breakpoints[0]{'path'}, $breakpoints[0]{'breakpoints'}[0]); diff --git a/hphp/test/slow/ext_vsdebug/step.php b/hphp/test/slow/ext_vsdebug/step.php index 32cbe2e076c..f64b39467ca 100644 --- a/hphp/test/slow/ext_vsdebug/step.php +++ b/hphp/test/slow/ext_vsdebug/step.php @@ -74,6 +74,9 @@ $breakpoints = varray[ $testProcess = vsDebugLaunch(ExtVsdebugStep::$path, true, $breakpoints); +// Skip breakpoint resolution messages. +skipMessages(count($breakpoints[0]{'breakpoints'})); + // Verify we hit breakpoint 1. verifyBpHit($breakpoints[0]{'path'}, $breakpoints[0]{'breakpoints'}[0]); diff --git a/hphp/test/slow/ext_vsdebug/todebugdisplay.php b/hphp/test/slow/ext_vsdebug/todebugdisplay.php index 81d75670f54..102f1c66df0 100644 --- a/hphp/test/slow/ext_vsdebug/todebugdisplay.php +++ b/hphp/test/slow/ext_vsdebug/todebugdisplay.php @@ -12,6 +12,9 @@ $breakpoints = varray[ $testProcess = vsDebugLaunch(__FILE__ . ".test", true, $breakpoints); +// Skip breakpoint resolution messages. +skipMessages(count($breakpoints[0]{'breakpoints'})); + // Verify we hit breakpoint 1. verifyBpHit($breakpoints[0]{'path'}, $breakpoints[0]{'breakpoints'}[0]); diff --git a/hphp/test/slow/ext_vsdebug/variables_all_types.php b/hphp/test/slow/ext_vsdebug/variables_all_types.php index 9fe586cce26..df9aa873c48 100644 --- a/hphp/test/slow/ext_vsdebug/variables_all_types.php +++ b/hphp/test/slow/ext_vsdebug/variables_all_types.php @@ -26,6 +26,9 @@ require(__DIR__ . '/common.inc'); $testProcess = vsDebugLaunch($path, true, $breakpoints); + // Skip breakpoint resolution messages. + skipMessages(count($breakpoints[0]{'breakpoints'})); + // we should have stopped at the first BP verifyBpHit($path, $bp1); -- 2.11.4.GIT