From 9df90a51f96f6311fbfb0c03b7d372573997270d Mon Sep 17 00:00:00 2001 From: Danny Su Date: Wed, 9 Oct 2024 19:43:58 -0700 Subject: [PATCH] Return past breakpoints on a call Summary: Imported from static_h Original Author: avp@meta.com Original Git: b623a8de461502a4c11a9740d536ed449fc244fe Original Reviewed By: dannysu Original Revision: D63920292 The interpreter inspects the IP up the stack to determine where to return to, so check whether there's a breakpoint there before doing `nextInstCall`. The check is now faster because we maintain a count of how many installed breakpoints each `CodeBlock` has when the debugger is enabled and check that before doing the more expensive map lookup on every Ret. Reviewed By: avp Differential Revision: D64125238 fbshipit-source-id: 3bf400146c76d6d6cb715b8973ec6dea289f317f --- include/hermes/VM/CodeBlock.h | 10 +++++++++ include/hermes/VM/Debugger/Debugger.h | 8 ++++---- lib/VM/CodeBlock.cpp | 2 ++ lib/VM/Interpreter.cpp | 16 +++++++++++++++ test/debugger/ret-breakpoint.js | 29 +++++++++++++++++++++++++++ test/debugger/ret-breakpoint.js.debug | 2 ++ 6 files changed, 63 insertions(+), 4 deletions(-) create mode 100644 test/debugger/ret-breakpoint.js create mode 100644 test/debugger/ret-breakpoint.js.debug diff --git a/include/hermes/VM/CodeBlock.h b/include/hermes/VM/CodeBlock.h index f96f7a96f99..bae1d82c2b6 100644 --- a/include/hermes/VM/CodeBlock.h +++ b/include/hermes/VM/CodeBlock.h @@ -51,6 +51,11 @@ class CodeBlock final /// ID of this function in the module's function list. uint32_t functionID_; +#ifdef HERMES_ENABLE_DEBUGGER + /// The number of breakpoints currently installed in this function. + uint32_t numInstalledBreakpoints_ = 0; +#endif + /// Total size of the property cache. const uint32_t propertyCacheSize_; @@ -302,6 +307,11 @@ class CodeBlock final /// Requires that a breakpoint has been set at \p offset. /// Decrements the user count of the associated runtime module. void uninstallBreakpointAtOffset(uint32_t offset, uint8_t opCode); + + /// \return the number of breakpoints installed in this code block. + uint32_t getNumInstalledBreakpoints() const { + return numInstalledBreakpoints_; + } #endif }; diff --git a/include/hermes/VM/Debugger/Debugger.h b/include/hermes/VM/Debugger/Debugger.h index 628d8961c3c..6acc226127d 100644 --- a/include/hermes/VM/Debugger/Debugger.h +++ b/include/hermes/VM/Debugger/Debugger.h @@ -408,6 +408,10 @@ class Debugger { ScriptID resolveScriptId(RuntimeModule *runtimeModule, uint32_t filenameId) const; + /// Get the actual OpCode produced from the source without being affected by + /// any user installed breakpoint "Debugger" OpCode overrides. + inst::OpCode getRealOpCode(CodeBlock *block, uint32_t offset) const; + private: /// The primary debugger command loop. ExecutionStatus debuggerLoop( @@ -557,10 +561,6 @@ class Debugger { /// Set breakpoints at all possible next instructions after the current one. void breakAtPossibleNextInstructions(const InterpreterState &state); - - /// Get the actual OpCode produced from the source without being affected by - /// any user installed breakpoint "Debugger" OpCode overrides. - inst::OpCode getRealOpCode(CodeBlock *block, uint32_t offset) const; }; } // namespace vm diff --git a/lib/VM/CodeBlock.cpp b/lib/VM/CodeBlock.cpp index 3d33df6ff82..794eccee8df 100644 --- a/lib/VM/CodeBlock.cpp +++ b/lib/VM/CodeBlock.cpp @@ -424,6 +424,7 @@ void CodeBlock::installBreakpointAtOffset(uint32_t offset) { makeWritable(address, sizeof(inst::DebuggerInst)); *address = debuggerOpcode; + ++numInstalledBreakpoints_; } void CodeBlock::uninstallBreakpointAtOffset( @@ -440,6 +441,7 @@ void CodeBlock::uninstallBreakpointAtOffset( // This is valid because we can only uninstall breakpoints that we installed. // Therefore, the page here must be writable. *address = opCode; + --numInstalledBreakpoints_; } #endif diff --git a/lib/VM/Interpreter.cpp b/lib/VM/Interpreter.cpp index 541eefc5776..9414a6e2914 100644 --- a/lib/VM/Interpreter.cpp +++ b/lib/VM/Interpreter.cpp @@ -1818,7 +1818,23 @@ CallResult Interpreter::interpretFunction( INIT_STATE_FOR_CODEBLOCK(curCodeBlock); O1REG(Call) = res.getValue(); + +#ifdef HERMES_ENABLE_DEBUGGER + // Only do the more expensive check for breakpoint location (in + // getRealOpCode) if there are breakpoints installed in the function + // we're returning into. + if (LLVM_UNLIKELY(curCodeBlock->getNumInstalledBreakpoints() > 0)) { + ip = IPADD(inst::getInstSize( + runtime.debugger_.getRealOpCode(curCodeBlock, CUROFFSET))); + } else { + // No breakpoints in the function being returned to, just use + // nextInstCall(). + ip = nextInstCall(ip); + } +#else ip = nextInstCall(ip); +#endif + DISPATCH; } diff --git a/test/debugger/ret-breakpoint.js b/test/debugger/ret-breakpoint.js new file mode 100644 index 00000000000..678de5e8cb9 --- /dev/null +++ b/test/debugger/ret-breakpoint.js @@ -0,0 +1,29 @@ +/** + * Copyright (c) Meta Platforms, Inc. and affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +// RUN: %hdb %s < %s.debug | %FileCheckOrRegen --match-full-lines %s +// REQUIRES: debugger + +function foo() { + debugger; + // Ret has to check the actual opcode at the callsite, + // which means it has to account for breakpoints being installed there. + return 1; +} + +function main(f) { + return f(); +} + +print(main(foo)); + +// Auto-generated content below. Please do not modify manually. + +// CHECK:Break on 'debugger' statement in foo: {{.*}}ret-breakpoint.js[2]:12:3 +// CHECK-NEXT:Set breakpoint 1 at {{.*}}ret-breakpoint.js[2]:19:11 +// CHECK-NEXT:Continuing execution +// CHECK-NEXT:1 diff --git a/test/debugger/ret-breakpoint.js.debug b/test/debugger/ret-breakpoint.js.debug new file mode 100644 index 00000000000..beb9e1546bb --- /dev/null +++ b/test/debugger/ret-breakpoint.js.debug @@ -0,0 +1,2 @@ +break 19 11 +continue