[error] extend error stack w/ function parameters

Extend FrameArray to hold weak references to parameters for functions 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=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}

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

@@ -1082,9 +1082,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);
 }
 
@@ -1242,6 +1243,20 @@ int JavaScriptFrame::ComputeParametersCount() const {
   return GetNumberOfIncomingArguments();
 }
 
+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.
@@ -1278,13 +1293,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);
 }
@@ -1530,9 +1547,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) {
@@ -1743,9 +1761,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

@@ -442,6 +442,7 @@ class BuiltinExitFrame : public ExitFrame {
   inline Object* new_target_slot_object() const;
 
   friend class StackFrameIteratorBase;
+  friend class FrameArrayBuilder;
 };
 
 class StandardFrame;
@@ -476,13 +477,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;
@@ -496,6 +499,7 @@ class FrameSummary {
     Handle<AbstractCode> abstract_code_;
     int code_offset_;
     bool is_constructor_;
+    Handle<FixedArray> parameters_;
   };
 
   class WasmFrameSummary : public FrameSummaryBase {
@@ -690,6 +694,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

@@ -470,8 +470,6 @@ StackTraceFailureMessage::StackTraceFailureMessage(Isolate* isolate, void* ptr1,
   }
 }
 
-namespace {
-
 class FrameArrayBuilder {
  public:
   FrameArrayBuilder(Isolate* isolate, FrameSkipMode mode, int limit,
@@ -507,8 +505,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) {
@@ -521,8 +530,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(
@@ -540,9 +553,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(
@@ -589,9 +606,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_; }
@@ -790,8 +816,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

@@ -10733,7 +10733,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 =
@@ -10743,6 +10744,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 {
@@ -59,7 +60,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);
@@ -86,7 +88,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"
@@ -2952,6 +2953,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) {
@@ -2964,15 +2972,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();
@@ -3488,6 +3487,160 @@ 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) {
+  FLAG_opt = false;
+  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) {
+    FixedArrayArgType 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);
+
+    FixedArrayArgType 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()));
+
+    FixedArrayArgType 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 error
+TEST(DetailedErrorStackTraceOpt) {
+  FLAG_always_opt = true;
+  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) {
+    FixedArrayArgType 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);
+
+    FixedArrayArgType 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()));
+
+    FixedArrayArgType 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) {
+    FixedArrayArgType 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);
+
+    FixedArrayArgType 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) {
+    FixedArrayArgType 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