Show source location in stacktrace after bound function call.

Summary:
In order to handle the clobbering of the stacktraces of bound functions,
we look up the register stack to the caller's frame, and use the caller's
"callee code block" instead of using the `savedCodeBlock` in the callee.

We must also store the saved IP in the callee frame, but because we have
a `null` `SavedCodeBlock`, we can use that as an indicator to still
return as if we were executing a native function call.

Reviewed By: tmikov, motiz88

Differential Revision: D17665340

fbshipit-source-id: 5e8ee86bffd8e3539230eb2321f3675d7ebc5ac3

include/hermes/BCGen/HBC/StackFrameLayout.h

@@ -99,6 +99,10 @@ struct StackFrameLayout {
     /// Saved caller instruction pointer.
     SavedIP = 1,
     /// Saved caller CodeBlock.
+    /// NOTE: If SavedCodeBlock is null but SavedIP is non-null, the current
+    /// frame is the result of a bound function call - the SavedCodeBlock can be
+    /// found using CalleeClosureOrCB on the previous call frame, but the
+    /// SavedIP should have been saved by the bound call in the current frame.
     SavedCodeBlock = 2,
     /// Number of JavaScript arguments passed to the callee excluding "this".
     ArgCount = 3,

include/hermes/VM/Callable.h

@@ -362,9 +362,10 @@ class BoundFunction final : public Callable {
   /// Perform the actual call. This is a light-weight handler which is part of
   /// the private API - it is only used internally and by the interpreter.
   /// Other users of this class must use \c Callable::call().
-  static CallResult<HermesValue> _boundCall(
-      BoundFunction *self,
-      Runtime *runtime);
+  /// \param ip the caller's IP at the point of the call (used for preserving
+  /// stack traces).
+  static CallResult<HermesValue>
+  _boundCall(BoundFunction *self, const Inst *ip, Runtime *runtime);
 
   /// Intialize the length and name and property of a lazily created bound
   /// function.

include/hermes/VM/Interpreter.h

@@ -84,9 +84,11 @@ class Interpreter {
   /// This handles the rest of the cases (native function, bound funcation, and
   /// not even a function).
   /// \param callTarget the register containing the function object
+  /// \param ip the current IP before the call (for preserving stack traces).
   /// \return ExecutionStatus::EXCEPTION if the call threw.
   static CallResult<HermesValue> handleCallSlowPath(
       Runtime *runtime,
+      const Inst *ip,
       PinnedHermesValue *callTarget);
 
   /// Fast path to get primitive value \p base's own properties by name \p id

include/hermes/VM/StackFrame-inline.h

@@ -33,6 +33,9 @@ StackFramePtrT<isConst>::getCalleeCodeBlock() const {
     else
       return nullptr;
   } else {
+    assert(
+        ref.isNativeValue() &&
+        "getCalleeClosureOrCBRef must be NativeValue or Object");
     return ref.template getNativePointer<CodeBlock>();
   }
 }

lib/VM/Callable.cpp

@@ -675,6 +675,7 @@ CallResult<HermesValue> BoundFunction::_newObjectImpl(
 
 CallResult<HermesValue> BoundFunction::_boundCall(
     BoundFunction *self,
+    const Inst *ip,
     Runtime *runtime) {
   ScopedNativeDepthTracker depthTracker{runtime};
   if (LLVM_UNLIKELY(depthTracker.overflowed())) {
@@ -766,7 +767,7 @@ CallResult<HermesValue> BoundFunction::_boundCall(
     auto newCalleeFrame = StackFramePtr::initFrame(
         stack,
         runtime->getCurrentFrame(),
-        nullptr,
+        ip,
         nullptr,
         totalArgCount,
         HermesValue::encodeObjectValue(self->getTarget(runtime)),
@@ -795,7 +796,7 @@ CallResult<HermesValue> BoundFunction::_boundCall(
   StackFramePtr::initFrame(
       originalCalleeFrame.ptr(),
       StackFramePtr{},
-      nullptr,
+      ip,
       nullptr,
       0,
       nullptr,
@@ -811,7 +812,9 @@ CallResult<HermesValue> BoundFunction::_boundCall(
 CallResult<HermesValue> BoundFunction::_callImpl(
     Handle<Callable> selfHandle,
     Runtime *runtime) {
-  return _boundCall(vmcast<BoundFunction>(selfHandle.get()), runtime);
+  // Pass `nullptr` as the IP because this function is never called
+  // from the interpreter, which should use `_boundCall` directly.
+  return _boundCall(vmcast<BoundFunction>(selfHandle.get()), nullptr, runtime);
 }
 
 //===----------------------------------------------------------------------===//

lib/VM/Debugger/Debugger.cpp

@@ -491,8 +491,21 @@ auto Debugger::getStackTrace(InterpreterState state) const -> StackTrace {
   uint32_t ipOffset = state.offset;
   for (auto cf : runtime_->getStackFrames()) {
     frames.push_back(getCallFrameInfo(codeBlock, ipOffset));
+
     codeBlock = cf.getSavedCodeBlock();
-    ipOffset = codeBlock ? codeBlock->getOffsetOf(cf->getSavedIP()) : 0;
+    const Inst *const savedIP = cf.getSavedIP();
+    if (!codeBlock && savedIP) {
+      // If we have a saved IP but no saved code block, this was a bound call.
+      // Go up one frame and get the callee code block but use the current
+      // frame's saved IP.
+      StackFramePtr prev = cf->getPreviousFrame();
+      assert(prev && "bound function calls must have a caller");
+      if (CodeBlock *parentCB = prev->getCalleeCodeBlock()) {
+        codeBlock = parentCB;
+      }
+    }
+
+    ipOffset = (codeBlock && savedIP) ? codeBlock->getOffsetOf(savedIP) : 0;
   }
   return StackTrace(std::move(frames));
 }

lib/VM/Interpreter.cpp

@@ -302,6 +302,7 @@ ExecutionStatus Interpreter::handleGetPNameList(
 
 CallResult<HermesValue> Interpreter::handleCallSlowPath(
     Runtime *runtime,
+    const Inst *ip,
     PinnedHermesValue *callTarget) {
   if (auto *native = dyn_vmcast<NativeFunction>(*callTarget)) {
     ++NumNativeFunctionCalls;
@@ -310,7 +311,7 @@ CallResult<HermesValue> Interpreter::handleCallSlowPath(
   } else if (auto *bound = dyn_vmcast<BoundFunction>(*callTarget)) {
     ++NumBoundFunctionCalls;
     // Call the bound function.
-    return BoundFunction::_boundCall(bound, runtime);
+    return BoundFunction::_boundCall(bound, ip, runtime);
   } else {
     return runtime->raiseTypeErrorForValue(
         Handle<>(callTarget), " is not a function");
@@ -1540,7 +1541,7 @@ CallResult<HermesValue> Interpreter::interpretFunction(
         goto tailCall;
 #endif
       }
-      res = Interpreter::handleCallSlowPath(runtime, &O2REG(Call));
+      res = Interpreter::handleCallSlowPath(runtime, ip, &O2REG(Call));
       if (LLVM_UNLIKELY(res == ExecutionStatus::EXCEPTION)) {
         goto exception;
       }

lib/VM/JSError.cpp

@@ -408,18 +408,31 @@ ExecutionStatus JSError::recordStackTrace(
     }
   }
 
+  const StackFramePtr framesEnd = *runtime->getStackFrames().end();
+
   // Fill in the call stack.
   // Each stack frame tracks information about the caller.
   for (StackFramePtr cf : runtime->getStackFrames()) {
-    auto *savedCodeBlock = cf.getSavedCodeBlock();
-    stack->emplace_back(
-        savedCodeBlock,
-        savedCodeBlock ? savedCodeBlock->getOffsetOf(cf.getSavedIP()) : 0);
-    if (savedCodeBlock) {
+    CodeBlock *savedCodeBlock = cf.getSavedCodeBlock();
+    const Inst *const savedIP = cf.getSavedIP();
+    // Go up one frame and get the callee code block but use the current
+    // frame's saved IP. This also allows us to account for bound functions,
+    // which have savedCodeBlock == nullptr in order to allow proper returns in
+    // the interpreter.
+    StackFramePtr prev = cf->getPreviousFrame();
+    if (prev && prev != framesEnd) {
+      if (CodeBlock *parentCB = prev->getCalleeCodeBlock()) {
+        savedCodeBlock = parentCB;
+      }
+    }
+    if (savedCodeBlock && savedIP) {
+      stack->emplace_back(savedCodeBlock, savedCodeBlock->getOffsetOf(savedIP));
       if (LLVM_UNLIKELY(
               addDomain(savedCodeBlock) == ExecutionStatus::EXCEPTION)) {
         return ExecutionStatus::EXCEPTION;
       }
+    } else {
+      stack->emplace_back(nullptr, 0);
     }
   }
   selfHandle->domains_.set(runtime, domains.get(), &runtime->getHeap());

test/debugger/backtrace.js

@@ -13,6 +13,7 @@ function baz(x, y) {
 
 function bar() {
   baz.call(undefined, 1,2);
+  baz.bind(null)(1, 2);
 }
 
 function foo() {
@@ -25,6 +26,13 @@ foo();
 // CHECK-NEXT: > 0: baz: {{.*}}:10:3
 // CHECK-NEXT:   1: (native)
 // CHECK-NEXT:   2: bar: {{.*}}:15:11
-// CHECK-NEXT:   3: foo: {{.*}}:19:6
-// CHECK-NEXT:   4: global: {{.*}}:22:4
+// CHECK-NEXT:   3: foo: {{.*}}:20:6
+// CHECK-NEXT:   4: global: {{.*}}:23:4
+// CHECK-NEXT: Continuing execution
+
+// CHECK: Break on 'debugger' statement in baz: {{.*}}:10:3
+// CHECK-NEXT: > 0: baz: {{.*}}:10:3
+// CHECK-NEXT:   1: bar: {{.*}}:16:17
+// CHECK-NEXT:   2: foo: {{.*}}:20:6
+// CHECK-NEXT:   3: global: {{.*}}:23:4
 // CHECK-NEXT: Continuing execution

test/debugger/backtrace.js.debug

@@ -1,2 +1,4 @@
 backtrace
 continue
+backtrace
+continue

test/hermes/stacktrace-bound.js

@@ -0,0 +1,41 @@
+// Copyright (c) Facebook, Inc. and its affiliates.
+//
+// This source code is licensed under the MIT license found in the LICENSE
+// file in the root directory of this source tree.
+//
+// RUN: %hermes -O -fno-inline %s | %FileCheck --match-full-lines %s
+
+function foo() {
+  throw new Error("qwerty");
+}
+
+try { foo() } catch (e) { print(e.stack) }
+// CHECK: Error: qwerty
+// CHECK-NEXT:     at foo ({{.*}}/stacktrace-bound.js:9:18)
+// CHECK-NEXT:     at global ({{.*}}/stacktrace-bound.js:12:10)
+
+try { foo.bind(null)() } catch (e) { print(e.stack) }
+// CHECK: Error: qwerty
+// CHECK-NEXT:     at foo ({{.*}}/stacktrace-bound.js:9:18)
+// CHECK-NEXT:     at global ({{.*}}/stacktrace-bound.js:17:21)
+
+try { foo.bind(null).bind(null)() } catch (e) { print(e.stack) }
+// CHECK: Error: qwerty
+// CHECK-NEXT:     at foo ({{.*}}/stacktrace-bound.js:9:18)
+// CHECK-NEXT:     at global ({{.*}}/stacktrace-bound.js:22:32)
+
+function chain1() {
+  chain2bound();
+}
+
+function chain2() {
+  throw new Error("asdf");
+}
+
+var chain2bound = chain2.bind(null);
+
+try { chain1.bind(null)() } catch (e) { print(e.stack) }
+// CHECK: Error: asdf
+// CHECK-NEXT:     at chain2 ({{.*}}/stacktrace-bound.js:32:18)
+// CHECK-NEXT:     at chain1 ({{.*}}/stacktrace-bound.js:28:14)
+// CHECK-NEXT:     at global ({{.*}}/stacktrace-bound.js:37:24)