From 3a0ee39cbde6a9778cfc4e2a6a0a8ff68933ff38 Mon Sep 17 00:00:00 2001 From: yangguo Date: Thu, 16 Jul 2015 02:28:12 -0700 Subject: [PATCH] Debugger: use FrameInspector in ScopeIterator to find context. In optimized code, it's not guaranteed that the current context is stored in its frame slot. R=bmeurer@chromium.org BUG=v8:4309 LOG=N Review URL: https://codereview.chromium.org/1239033002 Cr-Commit-Position: refs/heads/master@{#29697} --- src/debug.cc | 11 ---- src/runtime/runtime-debug.cc | 112 ++++++++++++++++----------------- test/mjsunit/es6/debug-blockscopes.js | 18 ++++-- test/mjsunit/regress/regress-4309-1.js | 37 +++++++++++ test/mjsunit/regress/regress-4309-2.js | 34 ++++++++++ test/mjsunit/regress/regress-4309-3.js | 39 ++++++++++++ 6 files changed, 179 insertions(+), 72 deletions(-) create mode 100644 test/mjsunit/regress/regress-4309-1.js create mode 100644 test/mjsunit/regress/regress-4309-2.js create mode 100644 test/mjsunit/regress/regress-4309-3.js diff --git a/src/debug.cc b/src/debug.cc index cdd2b8c..315f999 100644 --- a/src/debug.cc +++ b/src/debug.cc @@ -2735,19 +2735,8 @@ void Debug::HandleDebugBreak() { bool debug_command_only = isolate_->stack_guard()->CheckDebugCommand() && !isolate_->stack_guard()->CheckDebugBreak(); - bool is_debugger_statement = !isolate_->stack_guard()->CheckDebugCommand() && - !isolate_->stack_guard()->CheckDebugBreak(); - isolate_->stack_guard()->ClearDebugBreak(); - if (is_debugger_statement) { - // If we have been called via 'debugger' Javascript statement, - // we might not be prepared for breakpoints. - // TODO(dslomov,yangguo): CheckDebugBreak may race with RequestDebugBreak. - // Revisit this to clean-up. - HandleScope handle_scope(isolate_); - PrepareForBreakPoints(); - } ProcessDebugMessages(debug_command_only); } diff --git a/src/runtime/runtime-debug.cc b/src/runtime/runtime-debug.cc index 8f78998..a51e6d9 100644 --- a/src/runtime/runtime-debug.cc +++ b/src/runtime/runtime-debug.cc @@ -529,6 +529,7 @@ class FrameInspector { Object* GetContext() { return is_optimized_ ? deoptimized_frame_->GetContext() : frame_->context(); } + JavaScriptFrame* GetArgumentsFrame() { return frame_; } // To inspect all the provided arguments the frame might need to be // replaced with the arguments frame. @@ -959,9 +960,7 @@ static void UpdateStackLocalsFromMaterializedObject( Isolate* isolate, Handle target, Handle scope_info, JavaScriptFrame* frame, int inlined_jsframe_index) { if (inlined_jsframe_index != 0 || frame->is_optimized()) { - // Optimized frames are not supported. - // TODO(yangguo): make sure all code deoptimized when debugger is active - // and assert that this cannot happen. + // Optimized frames are not supported. Simply give up. return; } @@ -994,7 +993,7 @@ static void UpdateStackLocalsFromMaterializedObject( MUST_USE_RESULT static MaybeHandle MaterializeLocalContext( Isolate* isolate, Handle target, Handle function, - JavaScriptFrame* frame) { + Handle frame_context) { HandleScope scope(isolate); Handle shared(function->shared()); Handle scope_info(shared->scope_info()); @@ -1002,7 +1001,6 @@ MUST_USE_RESULT static MaybeHandle MaterializeLocalContext( if (!scope_info->HasContext()) return target; // Third fill all context locals. - Handle frame_context(Context::cast(frame->context())); Handle function_context(frame_context->declaration_context()); ScopeInfo::CopyContextLocalsToScopeObject(scope_info, function_context, target); @@ -1058,16 +1056,17 @@ MUST_USE_RESULT static MaybeHandle MaterializeScriptScope( MUST_USE_RESULT static MaybeHandle MaterializeLocalScope( - Isolate* isolate, JavaScriptFrame* frame, int inlined_jsframe_index) { - FrameInspector frame_inspector(frame, inlined_jsframe_index, isolate); - Handle function(JSFunction::cast(frame_inspector.GetFunction())); + Isolate* isolate, FrameInspector* frame_inspector) { + Handle function(JSFunction::cast(frame_inspector->GetFunction())); Handle local_scope = isolate->factory()->NewJSObject(isolate->object_function()); MaterializeStackLocalsWithFrameInspector(isolate, local_scope, function, - &frame_inspector); + frame_inspector); - return MaterializeLocalContext(isolate, local_scope, function, frame); + Handle frame_context(Context::cast(frame_inspector->GetContext())); + + return MaterializeLocalContext(isolate, local_scope, function, frame_context); } @@ -1096,13 +1095,10 @@ static bool SetContextLocalValue(Isolate* isolate, Handle scope_info, static bool SetLocalVariableValue(Isolate* isolate, JavaScriptFrame* frame, - int inlined_jsframe_index, Handle variable_name, Handle new_value) { - if (inlined_jsframe_index != 0 || frame->is_optimized()) { - // Optimized frames are not supported. - return false; - } + // Optimized frames are not supported. + if (frame->is_optimized()) return false; Handle function(frame->function()); Handle shared(function->shared()); @@ -1311,15 +1307,13 @@ static bool SetCatchVariableValue(Isolate* isolate, Handle context, static Handle MaterializeBlockScope(Isolate* isolate, Handle scope_info, Handle context, - JavaScriptFrame* frame, - int inlined_jsframe_index) { + FrameInspector* frame_inspector) { Handle block_scope = isolate->factory()->NewJSObject(isolate->object_function()); - if (frame != nullptr) { - FrameInspector frame_inspector(frame, inlined_jsframe_index, isolate); + if (frame_inspector != nullptr) { MaterializeStackLocalsWithFrameInspector(isolate, block_scope, scope_info, - &frame_inspector); + frame_inspector); } if (!context.is_null()) { @@ -1370,21 +1364,19 @@ class ScopeIterator { ScopeTypeModule }; - ScopeIterator(Isolate* isolate, JavaScriptFrame* frame, - int inlined_jsframe_index, bool ignore_nested_scopes = false) + ScopeIterator(Isolate* isolate, FrameInspector* frame_inspector, + bool ignore_nested_scopes = false) : isolate_(isolate), - frame_(frame), - inlined_jsframe_index_(inlined_jsframe_index), - function_(frame->function()), - context_(Context::cast(frame->context())), + frame_inspector_(frame_inspector), + context_(Context::cast(frame_inspector->GetContext())), nested_scope_chain_(4), seen_script_scope_(false), failed_(false) { // Catch the case when the debugger stops in an internal function. - Handle shared_info(function_->shared()); + Handle shared_info(function()->shared()); Handle scope_info(shared_info->scope_info()); if (shared_info->script() == isolate->heap()->undefined_value()) { - while (context_->closure() == *function_) { + while (context_->closure() == function()) { context_ = Handle(context_->previous(), isolate_); } return; @@ -1408,7 +1400,7 @@ class ScopeIterator { // PC points to the instruction after the current one, possibly a break // location as well. So the "- 1" to exclude it from the search. - Address call_pc = frame->pc() - 1; + Address call_pc = frame()->pc() - 1; // Find the break point where execution has stopped. BreakLocation location = @@ -1421,7 +1413,7 @@ class ScopeIterator { if (scope_info->HasContext()) { context_ = Handle(context_->declaration_context(), isolate_); } else { - while (context_->closure() == *function_) { + while (context_->closure() == function()) { context_ = Handle(context_->previous(), isolate_); } } @@ -1446,7 +1438,7 @@ class ScopeIterator { } else { DCHECK(scope_info->scope_type() == EVAL_SCOPE); info.set_eval(); - info.set_context(Handle(function_->context())); + info.set_context(Handle(function()->context())); } if (Parser::ParseStatic(&info) && Scope::Analyze(&info)) { scope = info.function()->scope(); @@ -1454,7 +1446,7 @@ class ScopeIterator { RetrieveScopeChain(scope, shared_info); } else { // Function code - ParseInfo info(&zone, function_); + ParseInfo info(&zone, Handle(function())); if (Parser::ParseStatic(&info) && Scope::Analyze(&info)) { scope = info.function()->scope(); } @@ -1465,15 +1457,11 @@ class ScopeIterator { ScopeIterator(Isolate* isolate, Handle function) : isolate_(isolate), - frame_(NULL), - inlined_jsframe_index_(0), - function_(function), + frame_inspector_(NULL), context_(function->context()), seen_script_scope_(false), failed_(false) { - if (function->IsBuiltin()) { - context_ = Handle(); - } + if (function->IsBuiltin()) context_ = Handle(); } // More scopes? @@ -1584,7 +1572,7 @@ class ScopeIterator { case ScopeIterator::ScopeTypeLocal: // Materialize the content of the local scope into a JSObject. DCHECK(nested_scope_chain_.length() == 1); - return MaterializeLocalScope(isolate_, frame_, inlined_jsframe_index_); + return MaterializeLocalScope(isolate_, frame_inspector_); case ScopeIterator::ScopeTypeWith: // Return the with object. return Handle(JSObject::cast(CurrentContext()->extension())); @@ -1600,11 +1588,11 @@ class ScopeIterator { Handle context = scope_info->HasContext() ? CurrentContext() : Handle::null(); - return MaterializeBlockScope(isolate_, scope_info, context, frame_, - inlined_jsframe_index_); + return MaterializeBlockScope(isolate_, scope_info, context, + frame_inspector_); } else { return MaterializeBlockScope(isolate_, Handle::null(), - CurrentContext(), nullptr, 0); + CurrentContext(), nullptr); } } case ScopeIterator::ScopeTypeModule: @@ -1631,8 +1619,8 @@ class ScopeIterator { case ScopeIterator::ScopeTypeGlobal: break; case ScopeIterator::ScopeTypeLocal: - return SetLocalVariableValue(isolate_, frame_, inlined_jsframe_index_, - variable_name, new_value); + return SetLocalVariableValue(isolate_, frame(), variable_name, + new_value); case ScopeIterator::ScopeTypeWith: break; case ScopeIterator::ScopeTypeCatch: @@ -1647,7 +1635,7 @@ class ScopeIterator { case ScopeIterator::ScopeTypeBlock: return SetBlockVariableValue( isolate_, HasContext() ? CurrentContext() : Handle::null(), - CurrentScopeInfo(), frame_, variable_name, new_value); + CurrentScopeInfo(), frame(), variable_name, new_value); case ScopeIterator::ScopeTypeModule: // TODO(2399): should we implement it? break; @@ -1694,7 +1682,7 @@ class ScopeIterator { case ScopeIterator::ScopeTypeLocal: { os << "Local:\n"; - function_->shared()->scope_info()->Print(); + function()->shared()->scope_info()->Print(); if (!CurrentContext().is_null()) { CurrentContext()->Print(os); if (CurrentContext()->has_extension()) { @@ -1747,18 +1735,24 @@ class ScopeIterator { private: Isolate* isolate_; - JavaScriptFrame* frame_; - int inlined_jsframe_index_; - Handle function_; + FrameInspector* const frame_inspector_; Handle context_; List > nested_scope_chain_; bool seen_script_scope_; bool failed_; + inline JavaScriptFrame* frame() { + return frame_inspector_->GetArgumentsFrame(); + } + + inline JSFunction* function() { + return JSFunction::cast(frame_inspector_->GetFunction()); + } + void RetrieveScopeChain(Scope* scope, Handle shared_info) { if (scope != NULL) { - int source_position = shared_info->code()->SourcePosition(frame_->pc()); + int source_position = frame_inspector_->GetSourcePosition(); scope->GetNestedScopeChain(isolate_, &nested_scope_chain_, source_position); } else { @@ -1789,10 +1783,11 @@ RUNTIME_FUNCTION(Runtime_GetScopeCount) { StackFrame::Id id = UnwrapFrameId(wrapped_id); JavaScriptFrameIterator it(isolate, id); JavaScriptFrame* frame = it.frame(); + FrameInspector frame_inspector(frame, 0, isolate); // Count the visible scopes. int n = 0; - for (ScopeIterator it(isolate, frame, 0); !it.Done(); it.Next()) { + for (ScopeIterator it(isolate, &frame_inspector); !it.Done(); it.Next()) { n++; } @@ -1917,10 +1912,11 @@ RUNTIME_FUNCTION(Runtime_GetScopeDetails) { StackFrame::Id id = UnwrapFrameId(wrapped_id); JavaScriptFrameIterator frame_it(isolate, id); JavaScriptFrame* frame = frame_it.frame(); + FrameInspector frame_inspector(frame, inlined_jsframe_index, isolate); // Find the requested scope. int n = 0; - ScopeIterator it(isolate, frame, inlined_jsframe_index); + ScopeIterator it(isolate, &frame_inspector); for (; !it.Done() && n < index; it.Next()) { n++; } @@ -1962,9 +1958,10 @@ RUNTIME_FUNCTION(Runtime_GetAllScopesDetails) { StackFrame::Id id = UnwrapFrameId(wrapped_id); JavaScriptFrameIterator frame_it(isolate, id); JavaScriptFrame* frame = frame_it.frame(); + FrameInspector frame_inspector(frame, inlined_jsframe_index, isolate); List > result(4); - ScopeIterator it(isolate, frame, inlined_jsframe_index, ignore_nested_scopes); + ScopeIterator it(isolate, &frame_inspector, ignore_nested_scopes); for (; !it.Done(); it.Next()) { Handle details; ASSIGN_RETURN_FAILURE_ON_EXCEPTION(isolate, details, @@ -2065,8 +2062,9 @@ RUNTIME_FUNCTION(Runtime_SetScopeVariableValue) { StackFrame::Id id = UnwrapFrameId(wrapped_id); JavaScriptFrameIterator frame_it(isolate, id); JavaScriptFrame* frame = frame_it.frame(); + FrameInspector frame_inspector(frame, inlined_jsframe_index, isolate); - ScopeIterator it(isolate, frame, inlined_jsframe_index); + ScopeIterator it(isolate, &frame_inspector); res = SetScopeVariableValue(&it, index, variable_name, new_value); } else { CONVERT_ARG_HANDLE_CHECKED(JSFunction, fun, 0); @@ -2086,7 +2084,9 @@ RUNTIME_FUNCTION(Runtime_DebugPrintScopes) { // Print the scopes for the top frame. StackFrameLocator locator(isolate); JavaScriptFrame* frame = locator.FindJavaScriptFrame(0); - for (ScopeIterator it(isolate, frame, 0); !it.Done(); it.Next()) { + FrameInspector frame_inspector(frame, 0, isolate); + + for (ScopeIterator it(isolate, &frame_inspector); !it.Done(); it.Next()) { it.DebugPrint(); } #endif @@ -2479,7 +2479,7 @@ class EvaluationContextBuilder { Handle inner_context; bool stop = false; - for (ScopeIterator it(isolate, frame, inlined_jsframe_index); + for (ScopeIterator it(isolate, &frame_inspector); !it.Failed() && !it.Done() && !stop; it.Next()) { ScopeIterator::ScopeType scope_type = it.Type(); diff --git a/test/mjsunit/es6/debug-blockscopes.js b/test/mjsunit/es6/debug-blockscopes.js index 31208d4..3f890eb 100644 --- a/test/mjsunit/es6/debug-blockscopes.js +++ b/test/mjsunit/es6/debug-blockscopes.js @@ -25,7 +25,7 @@ // (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. -// Flags: --expose-debug-as debug +// Flags: --expose-debug-as debug --allow-natives-syntax // The functions used for testing backtraces. They are at the top to make the // testing of source line/column easier. @@ -187,6 +187,14 @@ function CheckScopeContent(content, number, exec_state) { } +function assertEqualsUnlessOptimized(expected, value, f) { + try { + assertEquals(expected, value); + } catch (e) { + assertOptimized(f); + } +} + // Simple empty block scope in local scope. BeginTest("Local block 1"); @@ -517,11 +525,11 @@ function shadowing_1() { { let i = 5; debugger; - assertEquals(27, i); + assertEqualsUnlessOptimized(27, i, shadowing_1); } assertEquals(0, i); debugger; - assertEquals(27, i); + assertEqualsUnlessOptimized(27, i, shadowing_1); } listener_delegate = function (exec_state) { @@ -538,9 +546,9 @@ function shadowing_2() { { let j = 5; debugger; - assertEquals(27, j); + assertEqualsUnlessOptimized(27, j, shadowing_2); } - assertEquals(0, i); + assertEqualsUnlessOptimized(0, i, shadowing_2); } listener_delegate = function (exec_state) { diff --git a/test/mjsunit/regress/regress-4309-1.js b/test/mjsunit/regress/regress-4309-1.js new file mode 100644 index 0000000..a13fd43 --- /dev/null +++ b/test/mjsunit/regress/regress-4309-1.js @@ -0,0 +1,37 @@ +// Copyright 2015 the V8 project authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +// Flags: --expose-debug-as debug --allow-natives-syntax + +var Debug = debug.Debug; + +var exception = null; + +function listener(event, exec_state, event_data, data) { + if (event != Debug.DebugEvent.Break) return; + try { + var scopes = exec_state.frame().allScopes(); + assertEquals(3, scopes.length); + assertEquals(debug.ScopeType.Local, scopes[0].scopeType()); + assertEquals(debug.ScopeType.Script, scopes[1].scopeType()); + assertEquals(debug.ScopeType.Global, scopes[2].scopeType()); + } catch (e) { + exception = e; + } +} + +function f() { + eval(''); + debugger; +} + +f(); +f(); + +%OptimizeFunctionOnNextCall(f); +Debug.setListener(listener); + +f(); + +assertNull(exception); diff --git a/test/mjsunit/regress/regress-4309-2.js b/test/mjsunit/regress/regress-4309-2.js new file mode 100644 index 0000000..984b007 --- /dev/null +++ b/test/mjsunit/regress/regress-4309-2.js @@ -0,0 +1,34 @@ +// Copyright 2015 the V8 project authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +// Flags: --expose-debug-as debug --allow-natives-syntax + +var Debug = debug.Debug; + +var exception = null; + +function listener(event, exec_state, event_data, data) { + if (event != Debug.DebugEvent.Break) return; + try { + var scope = exec_state.frame().scope(0); + assertEquals(5, scope.scopeObject().property("i").value().value()); + } catch (e) { + exception = e; + } +} + +function f() { + eval('var i = 5'); + debugger; +} + +f(); +f(); + +%OptimizeFunctionOnNextCall(f); +Debug.setListener(listener); + +f(); + +assertNull(exception); diff --git a/test/mjsunit/regress/regress-4309-3.js b/test/mjsunit/regress/regress-4309-3.js new file mode 100644 index 0000000..687dd4c --- /dev/null +++ b/test/mjsunit/regress/regress-4309-3.js @@ -0,0 +1,39 @@ +// Copyright 2015 the V8 project authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +// Flags: --expose-debug-as debug --allow-natives-syntax + +var Debug = debug.Debug; + +var exception = null; + +function listener(event, exec_state, event_data, data) { + if (event != Debug.DebugEvent.Break) return; + try { + var scopes = exec_state.frame().allScopes(); + assertEquals(4, scopes.length); + assertEquals(debug.ScopeType.With, scopes[0].scopeType()); + assertEquals(debug.ScopeType.Local, scopes[1].scopeType()); + assertEquals(debug.ScopeType.Script, scopes[2].scopeType()); + assertEquals(debug.ScopeType.Global, scopes[3].scopeType()); + } catch (e) { + exception = e; + } +} + +function f() { + with({}) { + debugger; + } +} + +f(); +f(); + +%OptimizeFunctionOnNextCall(f); +Debug.setListener(listener); + +f(); + +assertNull(exception); -- 2.7.4