From 27fd921a28c886e5396b29e2b2f9bd8d12ab6a4b Mon Sep 17 00:00:00 2001 From: Michael Starzinger Date: Wed, 29 Nov 2017 14:18:33 +0100 Subject: [PATCH] [debug] Fix debug-evaluate for de-materialized function. This fixes debug-evaluate in the presence of a de-materialized function object. The creation of an arguments object is now requested based on a given frame (potentially inlined) instead of a target function. It makes sure that multiple calls to {StandardFrame::Summarize} don't cause any confusion when they give back non-identical function objects. R=jgruber@chromium.org TEST=debugger/debug/debug-evaluate-arguments BUG=chromium:788647 Change-Id: I575bb6cb20b4657dc09019e631b5d6e36c1b5189 Reviewed-on: https://chromium-review.googlesource.com/796474 Reviewed-by: Yang Guo Reviewed-by: Jakob Gruber Commit-Queue: Michael Starzinger Cr-Commit-Position: refs/heads/master@{#49721} --- src/accessors.cc | 145 +++++++++--------- src/accessors.h | 8 +- src/debug/debug-evaluate.cc | 35 ++++- src/debug/debug-evaluate.h | 6 + src/debug/debug-frames.cc | 27 ---- src/debug/debug-frames.h | 4 - .../debug/debug-evaluate-arguments.js | 60 ++++++++ 7 files changed, 179 insertions(+), 106 deletions(-) create mode 100644 test/debugger/debug/debug-evaluate-arguments.js diff --git a/src/accessors.cc b/src/accessors.cc index e242b62dda..adaa0be3c6 100644 --- a/src/accessors.cc +++ b/src/accessors.cc @@ -725,12 +725,11 @@ Handle Accessors::MakeFunctionNameInfo(Isolate* isolate) { // Accessors::FunctionArguments // +namespace { -static Handle ArgumentsForInlinedFunction( - JavaScriptFrame* frame, - Handle inlined_function, - int inlined_frame_index) { - Isolate* isolate = inlined_function->GetIsolate(); +Handle ArgumentsForInlinedFunction(JavaScriptFrame* frame, + int inlined_frame_index) { + Isolate* isolate = frame->isolate(); Factory* factory = isolate->factory(); TranslatedState translated_values(frame); @@ -742,7 +741,9 @@ static Handle ArgumentsForInlinedFunction( &argument_count); TranslatedFrame::iterator iter = translated_frame->begin(); - // Skip the function. + // Materialize the function. + bool should_deoptimize = iter->IsMaterializedObject(); + Handle function = Handle::cast(iter->GetValue()); iter++; // Skip the receiver. @@ -750,9 +751,8 @@ static Handle ArgumentsForInlinedFunction( argument_count--; Handle arguments = - factory->NewArgumentsObject(inlined_function, argument_count); + factory->NewArgumentsObject(function, argument_count); Handle array = factory->NewFixedArray(argument_count); - bool should_deoptimize = false; for (int i = 0; i < argument_count; ++i) { // If we materialize any object, we should deoptimize the frame because we // might alias an object that was eliminated by escape analysis. @@ -771,9 +771,7 @@ static Handle ArgumentsForInlinedFunction( return arguments; } - -static int FindFunctionInFrame(JavaScriptFrame* frame, - Handle function) { +int FindFunctionInFrame(JavaScriptFrame* frame, Handle function) { std::vector frames; frame->Summarize(&frames); for (size_t i = frames.size(); i != 0; i--) { @@ -784,69 +782,66 @@ static int FindFunctionInFrame(JavaScriptFrame* frame, return -1; } +Handle GetFrameArguments(Isolate* isolate, + JavaScriptFrameIterator* it, + int function_index) { + JavaScriptFrame* frame = it->frame(); -namespace { - -Handle GetFunctionArguments(Isolate* isolate, - Handle function) { - // Find the top invocation of the function by traversing frames. - for (JavaScriptFrameIterator it(isolate); !it.done(); it.Advance()) { - JavaScriptFrame* frame = it.frame(); - int function_index = FindFunctionInFrame(frame, function); - if (function_index < 0) continue; - - if (function_index > 0) { - // The function in question was inlined. Inlined functions have the - // correct number of arguments and no allocated arguments object, so - // we can construct a fresh one by interpreting the function's - // deoptimization input data. - return ArgumentsForInlinedFunction(frame, function, function_index); - } - - // Find the frame that holds the actual arguments passed to the function. - if (it.frame()->has_adapted_arguments()) { - it.AdvanceOneFrame(); - DCHECK(it.frame()->is_arguments_adaptor()); - } - frame = it.frame(); - - // Get the number of arguments and construct an arguments object - // mirror for the right frame. - const int length = frame->ComputeParametersCount(); - Handle arguments = isolate->factory()->NewArgumentsObject( - function, length); - Handle array = isolate->factory()->NewFixedArray(length); - - // Copy the parameters to the arguments object. - DCHECK(array->length() == length); - for (int i = 0; i < length; i++) { - Object* value = frame->GetParameter(i); - if (value->IsTheHole(isolate)) { - // Generators currently use holes as dummy arguments when resuming. We - // must not leak those. - DCHECK(IsResumableFunction(function->shared()->kind())); - value = isolate->heap()->undefined_value(); - } - array->set(i, value); - } - arguments->set_elements(*array); - - // Return the freshly allocated arguments object. - return arguments; + if (function_index > 0) { + // The function in question was inlined. Inlined functions have the + // correct number of arguments and no allocated arguments object, so + // we can construct a fresh one by interpreting the function's + // deoptimization input data. + return ArgumentsForInlinedFunction(frame, function_index); } - // No frame corresponding to the given function found. Return null. - return isolate->factory()->null_value(); + // Find the frame that holds the actual arguments passed to the function. + if (it->frame()->has_adapted_arguments()) { + it->AdvanceOneFrame(); + DCHECK(it->frame()->is_arguments_adaptor()); + } + frame = it->frame(); + + // Get the number of arguments and construct an arguments object + // mirror for the right frame and the underlying function. + const int length = frame->ComputeParametersCount(); + Handle function(frame->function(), isolate); + Handle arguments = + isolate->factory()->NewArgumentsObject(function, length); + Handle array = isolate->factory()->NewFixedArray(length); + + // Copy the parameters to the arguments object. + DCHECK(array->length() == length); + for (int i = 0; i < length; i++) { + Object* value = frame->GetParameter(i); + if (value->IsTheHole(isolate)) { + // Generators currently use holes as dummy arguments when resuming. We + // must not leak those. + DCHECK(IsResumableFunction(function->shared()->kind())); + value = isolate->heap()->undefined_value(); + } + array->set(i, value); + } + arguments->set_elements(*array); + + // Return the freshly allocated arguments object. + return arguments; } } // namespace - -Handle Accessors::FunctionGetArguments(Handle function) { - Handle arguments = - GetFunctionArguments(function->GetIsolate(), function); - CHECK(arguments->IsJSObject()); - return Handle::cast(arguments); +Handle Accessors::FunctionGetArguments(JavaScriptFrame* frame, + int inlined_jsframe_index) { + Isolate* isolate = frame->isolate(); + Address requested_frame_fp = frame->fp(); + // Forward a frame iterator to the requested frame. This is needed because we + // potentially need for advance it to the arguments adaptor frame later. + for (JavaScriptFrameIterator it(isolate); !it.done(); it.Advance()) { + if (it.frame()->fp() != requested_frame_fp) continue; + return GetFrameArguments(isolate, &it, inlined_jsframe_index); + } + UNREACHABLE(); // Requested frame not found. + return Handle(); } @@ -857,10 +852,18 @@ void Accessors::FunctionArgumentsGetter( HandleScope scope(isolate); Handle function = Handle::cast(Utils::OpenHandle(*info.Holder())); - Handle result = - function->shared()->native() - ? Handle::cast(isolate->factory()->null_value()) - : GetFunctionArguments(isolate, function); + Handle result = isolate->factory()->null_value(); + if (!function->shared()->native()) { + // Find the top invocation of the function by traversing frames. + for (JavaScriptFrameIterator it(isolate); !it.done(); it.Advance()) { + JavaScriptFrame* frame = it.frame(); + int function_index = FindFunctionInFrame(frame, function); + if (function_index >= 0) { + result = GetFrameArguments(isolate, &it, function_index); + break; + } + } + } info.GetReturnValue().Set(Utils::ToLocal(result)); } diff --git a/src/accessors.h b/src/accessors.h index 916e45fdf5..70e6a9200e 100644 --- a/src/accessors.h +++ b/src/accessors.h @@ -18,6 +18,7 @@ class AccessorInfo; template class Handle; class FieldIndex; +class JavaScriptFrame; // The list of accessor descriptors. This is a second-order macro // taking a macro to be applied to all accessor descriptor names. @@ -78,8 +79,11 @@ class Accessors : public AllStatic { static Handle MakeModuleNamespaceEntryInfo(Isolate* isolate, Handle name); - // Accessor functions called directly from the runtime system. - static Handle FunctionGetArguments(Handle object); + // Accessor function called directly from the runtime system. Returns the + // newly materialized arguments object for the given {frame}. Note that for + // optimized frames it is possible to specify an {inlined_jsframe_index}. + static Handle FunctionGetArguments(JavaScriptFrame* frame, + int inlined_jsframe_index); // Returns true for properties that are accessors to object fields. // If true, the matching FieldIndex is returned through |field_index|. diff --git a/src/debug/debug-evaluate.cc b/src/debug/debug-evaluate.cc index afad8e2f68..b6e3f14ed1 100644 --- a/src/debug/debug-evaluate.cc +++ b/src/debug/debug-evaluate.cc @@ -159,8 +159,7 @@ DebugEvaluate::ContextBuilder::ContextBuilder(Isolate* isolate, Handle non_locals = it.GetNonLocals(); MaterializeReceiver(materialized, local_context, local_function, non_locals); - frame_inspector.MaterializeStackLocals(materialized, local_function, - true); + MaterializeStackLocals(materialized, local_function, &frame_inspector); ContextChainElement context_chain_element; context_chain_element.scope_info = it.CurrentScopeInfo(); context_chain_element.materialized_object = materialized; @@ -242,6 +241,38 @@ void DebugEvaluate::ContextBuilder::MaterializeReceiver( JSObject::SetOwnPropertyIgnoreAttributes(target, name, recv, NONE).Check(); } +void DebugEvaluate::ContextBuilder::MaterializeStackLocals( + Handle target, Handle function, + FrameInspector* frame_inspector) { + bool materialize_arguments_object = true; + + // Do not materialize the arguments object for eval or top-level code. + if (function->shared()->is_toplevel()) materialize_arguments_object = false; + + // First materialize stack locals (modulo arguments object). + Handle shared(function->shared()); + Handle scope_info(shared->scope_info()); + frame_inspector->MaterializeStackLocals(target, scope_info, + materialize_arguments_object); + + // Then materialize the arguments object. + if (materialize_arguments_object) { + // Skip if "arguments" is already taken and wasn't optimized out (which + // causes {MaterializeStackLocals} above to skip the local variable). + Handle arguments_str = isolate_->factory()->arguments_string(); + Maybe maybe = JSReceiver::HasOwnProperty(target, arguments_str); + DCHECK(maybe.IsJust()); + if (maybe.FromJust()) return; + + // FunctionGetArguments can't throw an exception. + Handle arguments = + Accessors::FunctionGetArguments(frame_, inlined_jsframe_index_); + JSObject::SetOwnPropertyIgnoreAttributes(target, arguments_str, arguments, + NONE) + .Check(); + } +} + namespace { bool IntrinsicHasNoSideEffect(Runtime::FunctionId id) { diff --git a/src/debug/debug-evaluate.h b/src/debug/debug-evaluate.h index 6327895d57..fbe747d024 100644 --- a/src/debug/debug-evaluate.h +++ b/src/debug/debug-evaluate.h @@ -14,6 +14,8 @@ namespace v8 { namespace internal { +class FrameInspector; + class DebugEvaluate : public AllStatic { public: static MaybeHandle Global(Isolate* isolate, Handle source); @@ -73,6 +75,10 @@ class DebugEvaluate : public AllStatic { Handle local_function, Handle non_locals); + void MaterializeStackLocals(Handle target, + Handle function, + FrameInspector* frame_inspector); + Handle outer_info_; Handle evaluation_context_; std::vector context_chain_; diff --git a/src/debug/debug-frames.cc b/src/debug/debug-frames.cc index 7cacb3ffba..70f3670ee4 100644 --- a/src/debug/debug-frames.cc +++ b/src/debug/debug-frames.cc @@ -138,33 +138,6 @@ void FrameInspector::MaterializeStackLocals(Handle target, } } -void FrameInspector::MaterializeStackLocals(Handle target, - Handle function, - bool materialize_arguments_object) { - // Do not materialize the arguments object for eval or top-level code. - if (function->shared()->is_toplevel()) materialize_arguments_object = false; - - Handle shared(function->shared()); - Handle scope_info(shared->scope_info()); - MaterializeStackLocals(target, scope_info, materialize_arguments_object); - - // Third materialize the arguments object. - if (materialize_arguments_object) { - // Skip if "arguments" is already taken and wasn't optimized out (which - // causes {MaterializeStackLocals} above to skip the local variable). - Handle arguments_str = isolate_->factory()->arguments_string(); - Maybe maybe = JSReceiver::HasOwnProperty(target, arguments_str); - DCHECK(maybe.IsJust()); - if (maybe.FromJust()) return; - - // FunctionGetArguments can't throw an exception. - Handle arguments = Accessors::FunctionGetArguments(function); - JSObject::SetOwnPropertyIgnoreAttributes(target, arguments_str, arguments, - NONE) - .Check(); - } -} - void FrameInspector::UpdateStackLocalsFromMaterializedObject( Handle target, Handle scope_info) { diff --git a/src/debug/debug-frames.h b/src/debug/debug-frames.h index 96593b858d..9b669ea096 100644 --- a/src/debug/debug-frames.h +++ b/src/debug/debug-frames.h @@ -52,10 +52,6 @@ class FrameInspector { Handle scope_info, bool materialize_arguments_object = false); - void MaterializeStackLocals(Handle target, - Handle function, - bool materialize_arguments_object = false); - void UpdateStackLocalsFromMaterializedObject(Handle object, Handle scope_info); diff --git a/test/debugger/debug/debug-evaluate-arguments.js b/test/debugger/debug/debug-evaluate-arguments.js new file mode 100644 index 0000000000..8cf18d7dc8 --- /dev/null +++ b/test/debugger/debug/debug-evaluate-arguments.js @@ -0,0 +1,60 @@ +// Copyright 2017 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: --allow-natives-syntax + +Debug = debug.Debug; +var listened = false; + +function listener(event, exec_state, event_data, data) { + if (event != Debug.DebugEvent.Break) return; + try { + var foo_arguments = exec_state.frame(1).evaluate("arguments").value(); + var bar_arguments = exec_state.frame(0).evaluate("arguments").value(); + assertArrayEquals(foo_expected, foo_arguments); + assertArrayEquals(bar_expected, bar_arguments); + listened = true; + } catch (e) { + print(e); + print(e.stack); + } +} + +Debug.setListener(listener); + +function foo(a) { + function bar(a,b,c) { + debugger; + return a + b + c; + } + return bar(1,2,a); +} + +listened = false; +foo_expected = [3]; +bar_expected = [1,2,3]; +assertEquals(6, foo(3)); +assertTrue(listened); + +listened = false; +foo_expected = [3]; +bar_expected = [1,2,3]; +assertEquals(6, foo(3)); +assertTrue(listened); + +listened = false; +foo_expected = [3]; +bar_expected = [1,2,3]; +%OptimizeFunctionOnNextCall(foo); +assertEquals(6, foo(3)); +assertTrue(listened); + +listened = false; +foo_expected = [3,4,5]; +bar_expected = [1,2,3]; +%OptimizeFunctionOnNextCall(foo); +assertEquals(6, foo(3,4,5)); +assertTrue(listened); + +Debug.setListener(null);