Reland "[error] extend error stack w/ function parameters"

This is a reland of 97628eee.

Original change's description:
> [error] extend error stack w/ function parameters
>
> Extend FrameArray to hold weak references to parameters forfunctions in
> the call stack. The goal here is to provide more metadata for postmortem
> tools (such as llnode), especially in cases of rethrowing (this will be
> particularly useful when using postmortem with promises on Node.js).
>
> Besides postmortem, these changes allow us to print a more detailed
> stack trace for errors with parameters types (or even values), which can
> be useful since JavaScript functions can receive any number of
> parameters of any type, and having a function behave differently
> according to the number of parameters received as well as their types is
> a common pattern on JS libraries and frameworks.
>
> R=<U+200B>bmeurer@google.com, yangguo@google.com
>
> Change-Id: Idf0984d0dbac16041f11d738d4b1c095a8eecd61
> Reviewed-on: https://chromium-review.googlesource.com/c/1289489
> Commit-Queue: Yang Guo <yangguo@chromium.org>
> Reviewed-by: Yang Guo <yangguo@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#58468}

R=bmeurer@google.com, jkummerow@chromium.org, yangguo@google.com

Change-Id: I53d90bb862d9c5e9541116b375fa4de70e3e76dd
Reviewed-on: https://chromium-review.googlesource.com/c/1405568
Commit-Queue: Yang Guo <yangguo@chromium.org>
Reviewed-by: Jakob Kummerow <jkummerow@chromium.org>
Reviewed-by: Yang Guo <yangguo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#59458}

src/flag-definitions.h

@@ -1072,6 +1072,9 @@ DEFINE_INT(fuzzer_random_seed, 0,
 DEFINE_BOOL(trace_rail, false, "trace RAIL mode")
 DEFINE_BOOL(print_all_exceptions, false,
             "print exception object and stack trace on each thrown exception")
+DEFINE_BOOL(
+    detailed_error_stack_trace, false,
+    "includes arguments for each function call in the error stack frames array")
 
 // runtime.cc
 DEFINE_BOOL(runtime_call_stats, false, "report runtime call counts and times")

src/frames.cc

@@ -1079,9 +1079,10 @@ void JavaScriptFrame::Summarize(std::vector<FrameSummary>* functions) const {
   Code code = LookupCode();
   int offset = static_cast<int>(pc() - code->InstructionStart());
   AbstractCode abstract_code = AbstractCode::cast(code);
-  FrameSummary::JavaScriptFrameSummary summary(isolate(), receiver(),
-                                               function(), abstract_code,
-                                               offset, IsConstructor());
+  Handle<FixedArray> params = GetParameters();
+  FrameSummary::JavaScriptFrameSummary summary(
+      isolate(), receiver(), function(), abstract_code, offset, IsConstructor(),
+      *params);
   functions->push_back(summary);
 }
 
@@ -1241,6 +1242,20 @@ int JavaScriptFrame::ComputeParametersCount() const {
   return function()->shared()->internal_formal_parameter_count();
 }
 
+Handle<FixedArray> JavaScriptFrame::GetParameters() const {
+  if (V8_LIKELY(!FLAG_detailed_error_stack_trace)) {
+    return isolate()->factory()->empty_fixed_array();
+  }
+  int param_count = ComputeParametersCount();
+  Handle<FixedArray> parameters =
+      isolate()->factory()->NewFixedArray(param_count);
+  for (int i = 0; i < param_count; i++) {
+    parameters->set(i, GetParameter(i));
+  }
+
+  return parameters;
+}
+
 int JavaScriptBuiltinContinuationFrame::ComputeParametersCount() const {
   // Assert that the first allocatable register is also the argument count
   // register.
@@ -1277,13 +1292,15 @@ void JavaScriptBuiltinContinuationWithCatchFrame::SetException(
 
 FrameSummary::JavaScriptFrameSummary::JavaScriptFrameSummary(
     Isolate* isolate, Object receiver, JSFunction function,
-    AbstractCode abstract_code, int code_offset, bool is_constructor)
+    AbstractCode abstract_code, int code_offset, bool is_constructor,
+    FixedArray parameters)
     : FrameSummaryBase(isolate, FrameSummary::JAVA_SCRIPT),
       receiver_(receiver, isolate),
       function_(function, isolate),
       abstract_code_(abstract_code, isolate),
       code_offset_(code_offset),
-      is_constructor_(is_constructor) {
+      is_constructor_(is_constructor),
+      parameters_(parameters, isolate) {
   DCHECK(abstract_code->IsBytecodeArray() ||
          Code::cast(abstract_code)->kind() != Code::OPTIMIZED_FUNCTION);
 }
@@ -1529,9 +1546,10 @@ void OptimizedFrame::Summarize(std::vector<FrameSummary>* frames) const {
       }
 
       // Append full summary of the encountered JS frame.
-      FrameSummary::JavaScriptFrameSummary summary(isolate(), *receiver,
-                                                   *function, *abstract_code,
-                                                   code_offset, is_constructor);
+      Handle<FixedArray> params = GetParameters();
+      FrameSummary::JavaScriptFrameSummary summary(
+          isolate(), *receiver, *function, *abstract_code, code_offset,
+          is_constructor, *params);
       frames->push_back(summary);
       is_constructor = false;
     } else if (it->kind() == TranslatedFrame::kConstructStub) {
@@ -1742,9 +1760,10 @@ void InterpretedFrame::WriteInterpreterRegister(int register_index,
 void InterpretedFrame::Summarize(std::vector<FrameSummary>* functions) const {
   DCHECK(functions->empty());
   AbstractCode abstract_code = AbstractCode::cast(GetBytecodeArray());
+  Handle<FixedArray> params = GetParameters();
   FrameSummary::JavaScriptFrameSummary summary(
       isolate(), receiver(), function(), abstract_code, GetBytecodeOffset(),
-      IsConstructor());
+      IsConstructor(), *params);
   functions->push_back(summary);
 }
 

src/frames.h

@@ -446,6 +446,7 @@ class BuiltinExitFrame : public ExitFrame {
   inline Object new_target_slot_object() const;
 
   friend class StackFrameIteratorBase;
+  friend class FrameArrayBuilder;
 };
 
 class StandardFrame;
@@ -480,13 +481,15 @@ class FrameSummary {
    public:
     JavaScriptFrameSummary(Isolate* isolate, Object receiver,
                            JSFunction function, AbstractCode abstract_code,
-                           int code_offset, bool is_constructor);
+                           int code_offset, bool is_constructor,
+                           FixedArray parameters);
 
     Handle<Object> receiver() const { return receiver_; }
     Handle<JSFunction> function() const { return function_; }
     Handle<AbstractCode> abstract_code() const { return abstract_code_; }
     int code_offset() const { return code_offset_; }
     bool is_constructor() const { return is_constructor_; }
+    Handle<FixedArray> parameters() const { return parameters_; }
     bool is_subject_to_debugging() const;
     int SourcePosition() const;
     int SourceStatementPosition() const;
@@ -500,6 +503,7 @@ class FrameSummary {
     Handle<AbstractCode> abstract_code_;
     int code_offset_;
     bool is_constructor_;
+    Handle<FixedArray> parameters_;
   };
 
   class WasmFrameSummary : public FrameSummaryBase {
@@ -694,6 +698,7 @@ class JavaScriptFrame : public StandardFrame {
   inline Address GetParameterSlot(int index) const;
   Object GetParameter(int index) const override;
   int ComputeParametersCount() const override;
+  Handle<FixedArray> GetParameters() const;
 
   // Debugger access.
   void SetParameterValue(int index, Object value) const;

src/isolate.cc

@@ -529,8 +529,6 @@ StackTraceFailureMessage::StackTraceFailureMessage(Isolate* isolate, void* ptr1,
   }
 }
 
-namespace {
-
 class FrameArrayBuilder {
  public:
   enum FrameFilterMode { ALL, CURRENT_SECURITY_CONTEXT };
@@ -573,8 +571,19 @@ class FrameArrayBuilder {
     // The stored bytecode offset is relative to a different base than what
     // is used in the source position table, hence the subtraction.
     offset -= BytecodeArray::kHeaderSize - kHeapObjectTag;
+
+    Handle<FixedArray> parameters = isolate_->factory()->empty_fixed_array();
+    if (V8_UNLIKELY(FLAG_detailed_error_stack_trace)) {
+      int param_count = function->shared()->internal_formal_parameter_count();
+      parameters = isolate_->factory()->NewFixedArray(param_count);
+      for (int i = 0; i < param_count; i++) {
+        parameters->set(i,
+                        generator_object->parameters_and_registers()->get(i));
+      }
+    }
+
     elements_ = FrameArray::AppendJSFrame(elements_, receiver, function, code,
-                                          offset, flags);
+                                          offset, flags, parameters);
   }
 
   void AppendPromiseAllFrame(Handle<Context> context, int offset) {
@@ -587,8 +596,12 @@ class FrameArrayBuilder {
 
     Handle<Object> receiver(native_context->promise_function(), isolate_);
     Handle<AbstractCode> code(AbstractCode::cast(function->code()), isolate_);
+
+    // TODO(mmarchini) save Promises list from Promise.all()
+    Handle<FixedArray> parameters = isolate_->factory()->empty_fixed_array();
+
     elements_ = FrameArray::AppendJSFrame(elements_, receiver, function, code,
-                                          offset, flags);
+                                          offset, flags, parameters);
   }
 
   void AppendJavaScriptFrame(
@@ -606,9 +619,13 @@ class FrameArrayBuilder {
     if (IsStrictFrame(function)) flags |= FrameArray::kIsStrict;
     if (is_constructor) flags |= FrameArray::kIsConstructor;
 
+    Handle<FixedArray> parameters = isolate_->factory()->empty_fixed_array();
+    if (V8_UNLIKELY(FLAG_detailed_error_stack_trace))
+      parameters = summary.parameters();
+
     elements_ = FrameArray::AppendJSFrame(
         elements_, TheHoleToUndefined(isolate_, summary.receiver()), function,
-        abstract_code, offset, flags);
+        abstract_code, offset, flags, parameters);
   }
 
   void AppendWasmCompiledFrame(
@@ -655,9 +672,18 @@ class FrameArrayBuilder {
     if (IsStrictFrame(function)) flags |= FrameArray::kIsStrict;
     if (exit_frame->IsConstructor()) flags |= FrameArray::kIsConstructor;
 
+    Handle<FixedArray> parameters = isolate_->factory()->empty_fixed_array();
+    if (V8_UNLIKELY(FLAG_detailed_error_stack_trace)) {
+      int param_count = exit_frame->ComputeParametersCount();
+      parameters = isolate_->factory()->NewFixedArray(param_count);
+      for (int i = 0; i < param_count; i++) {
+        parameters->set(i, exit_frame->GetParameter(i));
+      }
+    }
+
     elements_ = FrameArray::AppendJSFrame(elements_, receiver, function,
                                           Handle<AbstractCode>::cast(code),
-                                          offset, flags);
+                                          offset, flags, parameters);
   }
 
   bool full() { return elements_->FrameCount() >= limit_; }
@@ -858,8 +884,6 @@ void CaptureAsyncStackTrace(Isolate* isolate, Handle<JSPromise> promise,
   }
 }
 
-}  // namespace
-
 Handle<Object> Isolate::CaptureSimpleStackTrace(Handle<JSReceiver> error_object,
                                                 FrameSkipMode mode,
                                                 Handle<Object> caller) {

src/objects.cc

@@ -3942,7 +3942,8 @@ Handle<FrameArray> FrameArray::AppendJSFrame(Handle<FrameArray> in,
                                              Handle<Object> receiver,
                                              Handle<JSFunction> function,
                                              Handle<AbstractCode> code,
-                                             int offset, int flags) {
+                                             int offset, int flags,
+                                             Handle<FixedArray> parameters) {
   const int frame_count = in->FrameCount();
   const int new_length = LengthFor(frame_count + 1);
   Handle<FrameArray> array =
@@ -3952,6 +3953,7 @@ Handle<FrameArray> FrameArray::AppendJSFrame(Handle<FrameArray> in,
   array->SetCode(frame_count, *code);
   array->SetOffset(frame_count, Smi::FromInt(offset));
   array->SetFlags(frame_count, Smi::FromInt(flags));
+  array->SetParameters(frame_count, *parameters);
   array->set(kFrameCountIndex, Smi::FromInt(frame_count + 1));
   return array;
 }

src/objects/frame-array.h

@@ -25,7 +25,8 @@ class Handle;
   V(Function, JSFunction)             \
   V(Code, AbstractCode)               \
   V(Offset, Smi)                      \
-  V(Flags, Smi)
+  V(Flags, Smi)                       \
+  V(Parameters, FixedArray)
 
 // Container object for data collected during simple stack trace captures.
 class FrameArray : public FixedArray {
@@ -60,7 +61,8 @@ class FrameArray : public FixedArray {
                                           Handle<Object> receiver,
                                           Handle<JSFunction> function,
                                           Handle<AbstractCode> code, int offset,
-                                          int flags);
+                                          int flags,
+                                          Handle<FixedArray> parameters);
   static Handle<FrameArray> AppendWasmFrame(
       Handle<FrameArray> in, Handle<WasmInstanceObject> wasm_instance,
       int wasm_function_index, wasm::WasmCode* code, int offset, int flags);
@@ -87,7 +89,9 @@ class FrameArray : public FixedArray {
 
   static const int kFlagsOffset = 4;
 
-  static const int kElementsPerFrame = 5;
+  static const int kParametersOffset = 5;
+
+  static const int kElementsPerFrame = 6;
 
   // Array layout indices.
 

test/cctest/heap/test-heap.cc

@@ -46,6 +46,7 @@
 #include "src/ic/ic.h"
 #include "src/macro-assembler-inl.h"
 #include "src/objects-inl.h"
+#include "src/objects/frame-array-inl.h"
 #include "src/objects/heap-number-inl.h"
 #include "src/objects/js-array-inl.h"
 #include "src/objects/js-collection-inl.h"
@@ -2938,6 +2939,13 @@ TEST(Regress1465) {
   CHECK_EQ(1, transitions_after);
 }
 
+static i::Handle<JSObject> GetByName(const char* name) {
+  return i::Handle<JSObject>::cast(
+      v8::Utils::OpenHandle(*v8::Local<v8::Object>::Cast(
+          CcTest::global()
+              ->Get(CcTest::isolate()->GetCurrentContext(), v8_str(name))
+              .ToLocalChecked())));
+}
 
 #ifdef DEBUG
 static void AddTransitions(int transitions_count) {
@@ -2950,15 +2958,6 @@ static void AddTransitions(int transitions_count) {
 }
 
 
-static i::Handle<JSObject> GetByName(const char* name) {
-  return i::Handle<JSObject>::cast(
-      v8::Utils::OpenHandle(*v8::Local<v8::Object>::Cast(
-          CcTest::global()
-              ->Get(CcTest::isolate()->GetCurrentContext(), v8_str(name))
-              .ToLocalChecked())));
-}
-
-
 static void AddPropertyTo(
     int gc_count, Handle<JSObject> object, const char* property_name) {
   Isolate* isolate = CcTest::i_isolate();
@@ -3472,6 +3471,119 @@ UNINITIALIZED_TEST(ReleaseStackTraceData) {
   isolate->Dispose();
 }
 
+// TODO(mmarchini) also write tests for async/await and Promise.all
+void DetailedErrorStackTraceTest(const char* src,
+                                 std::function<void(Handle<FrameArray>)> test) {
+  FLAG_detailed_error_stack_trace = true;
+  CcTest::InitializeVM();
+  v8::HandleScope scope(CcTest::isolate());
+
+  v8::TryCatch try_catch(CcTest::isolate());
+  CompileRun(src);
+
+  CHECK(try_catch.HasCaught());
+  Handle<Object> exception = v8::Utils::OpenHandle(*try_catch.Exception());
+
+  Isolate* isolate = CcTest::i_isolate();
+  Handle<Name> key = isolate->factory()->stack_trace_symbol();
+
+  Handle<FrameArray> stack_trace(
+      FrameArray::cast(
+          Handle<JSArray>::cast(
+              Object::GetProperty(isolate, exception, key).ToHandleChecked())
+              ->elements()),
+      isolate);
+
+  test(stack_trace);
+}
+
+// * Test interpreted function error
+TEST(DetailedErrorStackTrace) {
+  static const char* source =
+      "function func1(arg1) {       "
+      "  let err = new Error();     "
+      "  throw err;                 "
+      "}                            "
+      "function func2(arg1, arg2) { "
+      "  func1(42);                 "
+      "}                            "
+      "class Foo {};                "
+      "function main(arg1, arg2) {  "
+      "  func2(arg1, false);        "
+      "}                            "
+      "var foo = new Foo();         "
+      "main(foo);                   ";
+
+  DetailedErrorStackTraceTest(source, [](Handle<FrameArray> stack_trace) {
+    FixedArray foo_parameters = stack_trace->Parameters(0);
+    CHECK_EQ(foo_parameters->length(), 1);
+    CHECK(foo_parameters->get(0)->IsSmi());
+    CHECK_EQ(Smi::ToInt(foo_parameters->get(0)), 42);
+
+    FixedArray bar_parameters = stack_trace->Parameters(1);
+    CHECK_EQ(bar_parameters->length(), 2);
+    CHECK(bar_parameters->get(0)->IsJSObject());
+    CHECK(bar_parameters->get(1)->IsBoolean());
+    Handle<Object> foo = Handle<Object>::cast(GetByName("foo"));
+    CHECK_EQ(bar_parameters->get(0), *foo);
+    CHECK(!bar_parameters->get(1)->BooleanValue(CcTest::i_isolate()));
+
+    FixedArray main_parameters = stack_trace->Parameters(2);
+    CHECK_EQ(main_parameters->length(), 2);
+    CHECK(main_parameters->get(0)->IsJSObject());
+    CHECK(main_parameters->get(1)->IsUndefined());
+    CHECK_EQ(main_parameters->get(0), *foo);
+  });
+}
+
+// * Test optimized function with inline frame error
+TEST(DetailedErrorStackTraceInline) {
+  FLAG_allow_natives_syntax = true;
+  static const char* source =
+      "function add(x) {                 "
+      " if (x == 42)                     "
+      "  throw new Error();              "
+      " return x + x;                    "
+      "}                                 "
+      "add(0);                           "
+      "add(1);                           "
+      "function foo(x) {                 "
+      " return add(x + 1)                "
+      "}                                 "
+      "foo(40);                          "
+      "%OptimizeFunctionOnNextCall(foo); "
+      "foo(41);                          ";
+
+  DetailedErrorStackTraceTest(source, [](Handle<FrameArray> stack_trace) {
+    FixedArray parameters_add = stack_trace->Parameters(0);
+    CHECK_EQ(parameters_add->length(), 1);
+    CHECK(parameters_add->get(0)->IsSmi());
+    CHECK_EQ(Smi::ToInt(parameters_add->get(0)), 42);
+
+    FixedArray parameters_foo = stack_trace->Parameters(1);
+    CHECK_EQ(parameters_foo->length(), 1);
+    CHECK(parameters_foo->get(0)->IsSmi());
+    CHECK_EQ(Smi::ToInt(parameters_foo->get(0)), 41);
+  });
+}
+
+// * Test builtin exit error
+TEST(DetailedErrorStackTraceBuiltinExit) {
+  static const char* source =
+      "function test(arg1) {           "
+      "  (new Number()).toFixed(arg1); "
+      "}                               "
+      "test(9999);                     ";
+
+  DetailedErrorStackTraceTest(source, [](Handle<FrameArray> stack_trace) {
+    FixedArray parameters = stack_trace->Parameters(0);
+
+    CHECK_EQ(parameters->length(), 2);
+    CHECK(parameters->get(0)->IsSmi());
+    CHECK_EQ(Smi::ToInt(parameters->get(0)), 9999);
+  });
+}
+
 TEST(Regress169928) {
   FLAG_allow_natives_syntax = true;
 #ifndef V8_LITE_MODE