[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/796474Reviewed-by: Yang Guo <yangguo@chromium.org>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Commit-Queue: Michael Starzinger <mstarzinger@chromium.org>
Cr-Commit-Position: refs/heads/master@{#49721}

src/accessors.cc

@@ -725,12 +725,11 @@ Handle<AccessorInfo> Accessors::MakeFunctionNameInfo(Isolate* isolate) {
 // Accessors::FunctionArguments
 //
 
+namespace {
 
-static Handle<Object> ArgumentsForInlinedFunction(
-    JavaScriptFrame* frame,
-    Handle<JSFunction> inlined_function,
-    int inlined_frame_index) {
-  Isolate* isolate = inlined_function->GetIsolate();
+Handle<JSObject> 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<Object> ArgumentsForInlinedFunction(
                                                          &argument_count);
   TranslatedFrame::iterator iter = translated_frame->begin();
 
-  // Skip the function.
+  // Materialize the function.
+  bool should_deoptimize = iter->IsMaterializedObject();
+  Handle<JSFunction> function = Handle<JSFunction>::cast(iter->GetValue());
   iter++;
 
   // Skip the receiver.
@@ -750,9 +751,8 @@ static Handle<Object> ArgumentsForInlinedFunction(
   argument_count--;
 
   Handle<JSObject> arguments =
-      factory->NewArgumentsObject(inlined_function, argument_count);
+      factory->NewArgumentsObject(function, argument_count);
   Handle<FixedArray> 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<Object> ArgumentsForInlinedFunction(
   return arguments;
 }
 
-
-static int FindFunctionInFrame(JavaScriptFrame* frame,
-                               Handle<JSFunction> function) {
+int FindFunctionInFrame(JavaScriptFrame* frame, Handle<JSFunction> function) {
   std::vector<FrameSummary> 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<JSObject> GetFrameArguments(Isolate* isolate,
+                                   JavaScriptFrameIterator* it,
+                                   int function_index) {
+  JavaScriptFrame* frame = it->frame();
 
-namespace {
+  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);
+  }
 
-Handle<Object> GetFunctionArguments(Isolate* isolate,
-                                    Handle<JSFunction> 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();
 
-    // 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<JSObject> arguments = isolate->factory()->NewArgumentsObject(
-        function, length);
-    Handle<FixedArray> 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);
+  // 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<JSFunction> function(frame->function(), isolate);
+  Handle<JSObject> arguments =
+      isolate->factory()->NewArgumentsObject(function, length);
+  Handle<FixedArray> 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();
     }
-    arguments->set_elements(*array);
-
-    // Return the freshly allocated arguments object.
-    return arguments;
+    array->set(i, value);
   }
+  arguments->set_elements(*array);
 
-  // No frame corresponding to the given function found. Return null.
-  return isolate->factory()->null_value();
+  // Return the freshly allocated arguments object.
+  return arguments;
 }
 
 }  // namespace
 
-
-Handle<JSObject> Accessors::FunctionGetArguments(Handle<JSFunction> function) {
-  Handle<Object> arguments =
-      GetFunctionArguments(function->GetIsolate(), function);
-  CHECK(arguments->IsJSObject());
-  return Handle<JSObject>::cast(arguments);
+Handle<JSObject> 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<JSObject>();
 }
 
 
@@ -857,10 +852,18 @@ void Accessors::FunctionArgumentsGetter(
   HandleScope scope(isolate);
   Handle<JSFunction> function =
       Handle<JSFunction>::cast(Utils::OpenHandle(*info.Holder()));
-  Handle<Object> result =
-      function->shared()->native()
-          ? Handle<Object>::cast(isolate->factory()->null_value())
-          : GetFunctionArguments(isolate, function);
+  Handle<Object> 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));
 }
 

src/accessors.h

@@ -18,6 +18,7 @@ class AccessorInfo;
 template <typename T>
 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<AccessorInfo> MakeModuleNamespaceEntryInfo(Isolate* isolate,
                                                            Handle<String> name);
 
-  // Accessor functions called directly from the runtime system.
-  static Handle<JSObject> FunctionGetArguments(Handle<JSFunction> 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<JSObject> 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|.

src/debug/debug-evaluate.cc

@@ -159,8 +159,7 @@ DebugEvaluate::ContextBuilder::ContextBuilder(Isolate* isolate,
       Handle<StringSet> 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<JSObject> target, Handle<JSFunction> 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<SharedFunctionInfo> shared(function->shared());
+  Handle<ScopeInfo> 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<String> arguments_str = isolate_->factory()->arguments_string();
+    Maybe<bool> maybe = JSReceiver::HasOwnProperty(target, arguments_str);
+    DCHECK(maybe.IsJust());
+    if (maybe.FromJust()) return;
+
+    // FunctionGetArguments can't throw an exception.
+    Handle<JSObject> arguments =
+        Accessors::FunctionGetArguments(frame_, inlined_jsframe_index_);
+    JSObject::SetOwnPropertyIgnoreAttributes(target, arguments_str, arguments,
+                                             NONE)
+        .Check();
+  }
+}
+
 namespace {
 
 bool IntrinsicHasNoSideEffect(Runtime::FunctionId id) {

src/debug/debug-evaluate.h

@@ -14,6 +14,8 @@
 namespace v8 {
 namespace internal {
 
+class FrameInspector;
+
 class DebugEvaluate : public AllStatic {
  public:
   static MaybeHandle<Object> Global(Isolate* isolate, Handle<String> source);
@@ -73,6 +75,10 @@ class DebugEvaluate : public AllStatic {
                              Handle<JSFunction> local_function,
                              Handle<StringSet> non_locals);
 
+    void MaterializeStackLocals(Handle<JSObject> target,
+                                Handle<JSFunction> function,
+                                FrameInspector* frame_inspector);
+
     Handle<SharedFunctionInfo> outer_info_;
     Handle<Context> evaluation_context_;
     std::vector<ContextChainElement> context_chain_;

src/debug/debug-frames.cc

@@ -138,33 +138,6 @@ void FrameInspector::MaterializeStackLocals(Handle<JSObject> target,
   }
 }
 
-void FrameInspector::MaterializeStackLocals(Handle<JSObject> target,
-                                            Handle<JSFunction> 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<SharedFunctionInfo> shared(function->shared());
-  Handle<ScopeInfo> 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<String> arguments_str = isolate_->factory()->arguments_string();
-    Maybe<bool> maybe = JSReceiver::HasOwnProperty(target, arguments_str);
-    DCHECK(maybe.IsJust());
-    if (maybe.FromJust()) return;
-
-    // FunctionGetArguments can't throw an exception.
-    Handle<JSObject> arguments = Accessors::FunctionGetArguments(function);
-    JSObject::SetOwnPropertyIgnoreAttributes(target, arguments_str, arguments,
-                                             NONE)
-        .Check();
-  }
-}
-
 
 void FrameInspector::UpdateStackLocalsFromMaterializedObject(
     Handle<JSObject> target, Handle<ScopeInfo> scope_info) {

src/debug/debug-frames.h

@@ -52,10 +52,6 @@ class FrameInspector {
                               Handle<ScopeInfo> scope_info,
                               bool materialize_arguments_object = false);
 
-  void MaterializeStackLocals(Handle<JSObject> target,
-                              Handle<JSFunction> function,
-                              bool materialize_arguments_object = false);
-
   void UpdateStackLocalsFromMaterializedObject(Handle<JSObject> object,
                                                Handle<ScopeInfo> scope_info);
 

test/debugger/debug/debug-evaluate-arguments.js

+// 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);