[vm/bytecode] Improve single stepping and breakpoint setting in bytecode.

Modified list of bytecodes that check for debug breaks (wip).
Make sure source positions are not interpreted as return addresses.
Fix skipping of bytecode stub frames when stepping.
Fix DEBUG_CHECK macro to call debugger twice for breakpoint and following step.
Various minor bug fixes and printing improvements.

Change-Id: I36713de0548d060eddb7cbfd6fadb05c731e881a
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/108205
Commit-Queue: Régis Crelier <[email protected]>
Reviewed-by: Ryan Macnak <[email protected]>

Author: crelier

pkg/vm/lib/bytecode/assembler.dart

@@ -390,7 +390,6 @@ class BytecodeAssembler {
   }
 
   void emitNativeCall(int rd) {
-    emitSourcePosition();
     _emitInstructionD(Opcode.kNativeCall, rd);
   }
 

pkg/vm/lib/bytecode/gen_bytecode.dart

@@ -1534,6 +1534,12 @@ class BytecodeGenerator extends RecursiveVisitor<Null> {
     } else {
       asm.emitEntry(locals.frameSize);
     }
+    // TODO(alexmarkov): Introduce a new bytecode triggering a debug check in
+    // the interpreter. Its token position should correspond to the declaration
+    // position of the last parameter, which the debugger can inspect at the
+    // point of the debug check.
+    // TODO(regis): Support the new bytecode in the interpreter and dissociate
+    // the debug check from the CheckStack bytecode.
     asm.emitCheckStack(0);
 
     if (isClosure) {
@@ -1667,8 +1673,8 @@ class BytecodeGenerator extends RecursiveVisitor<Null> {
       _genPushContextForVariable(variable);
       asm.emitPush(locals.getOriginalParamSlotIndex(variable));
       _genStoreVar(variable);
-      // TODO(alexmarkov): Do we need to store null at the original parameter
-      // location?
+      // TODO(alexmarkov): We need to store null at the original parameter
+      // location, because the original value may need to be GC'ed.
     }
   }
 

runtime/vm/compiler/assembler/disassembler_kbc.cc

@@ -57,7 +57,11 @@ static void Fmttgt(char** buf,
                    intptr_t* size,
                    const KBCInstr* instr,
                    int32_t value) {
-  FormatOperand(buf, size, "-> %" Px, instr + value);
+  if (FLAG_disassemble_relative) {
+    FormatOperand(buf, size, "-> %" Pd, value);
+  } else {
+    FormatOperand(buf, size, "-> %" Px, instr + value);
+  }
 }
 
 static void Fmtlit(char** buf,
@@ -376,11 +380,16 @@ void KernelBytecodeDisassembler::Disassemble(const Function& function) {
     const int addr_width = (kBitsPerWord / 4) + 2;
     // "*" in a printf format specifier tells it to read the field width from
     // the printf argument list.
-    THR_Print("%-*s\tpos\n", addr_width, "pc");
+    THR_Print("%-*s\tpos\tline\tcolumn\tyield\n", addr_width, "pc");
+    const Script& script = Script::Handle(zone, function.script());
     kernel::BytecodeSourcePositionsIterator iter(zone, bytecode);
     while (iter.MoveNext()) {
-      THR_Print("%#-*" Px "\t%s\n", addr_width, base + iter.PcOffset(),
-                iter.TokenPos().ToCString());
+      TokenPosition pos = iter.TokenPos();
+      intptr_t line = -1, column = -1;
+      script.GetTokenLocation(pos, &line, &column);
+      THR_Print("%#-*" Px "\t%s\t%" Pd "\t%" Pd "\t%s\n", addr_width,
+                base + iter.PcOffset(), pos.ToCString(), line, column,
+                iter.IsYieldPoint() ? "yield" : "");
     }
     THR_Print("}\n");
   }

runtime/vm/constants_kbc.h

@@ -860,6 +860,11 @@ class KernelBytecode {
     return bc + kInstructionSize[DecodeOpcode(bc)];
   }
 
+  DART_FORCE_INLINE static uword Next(uword pc) {
+    return pc + kInstructionSize[DecodeOpcode(
+                    reinterpret_cast<const KBCInstr*>(pc))];
+  }
+
   DART_FORCE_INLINE static bool IsJumpOpcode(const KBCInstr* instr) {
     switch (DecodeOpcode(instr)) {
       case KernelBytecode::kJump:
@@ -949,10 +954,15 @@ class KernelBytecode {
     }
   }
 
-  // The interpreter and this function must agree on the opcodes.
+  // The interpreter, the bytecode generator, and this function must agree on
+  // this list of opcodes.
+  // The interpreter checks for a debug break at each instruction with listed
+  // opcode and the bytecode generator emits a source position at each
+  // instruction with listed opcode.
   DART_FORCE_INLINE static bool IsDebugBreakCheckedOpcode(
       const KBCInstr* instr) {
     switch (DecodeOpcode(instr)) {
+      case KernelBytecode::kAllocate:
       case KernelBytecode::kPopLocal:
       case KernelBytecode::kPopLocal_Wide:
       case KernelBytecode::kStoreLocal:
@@ -972,6 +982,33 @@ class KernelBytecode {
       case KernelBytecode::kThrow:
       case KernelBytecode::kJump:
       case KernelBytecode::kJump_Wide:
+      case KernelBytecode::kEqualsNull:
+      case KernelBytecode::kNegateInt:
+      case KernelBytecode::kNegateDouble:
+      case KernelBytecode::kAddInt:
+      case KernelBytecode::kSubInt:
+      case KernelBytecode::kMulInt:
+      case KernelBytecode::kTruncDivInt:
+      case KernelBytecode::kModInt:
+      case KernelBytecode::kBitAndInt:
+      case KernelBytecode::kBitOrInt:
+      case KernelBytecode::kBitXorInt:
+      case KernelBytecode::kShlInt:
+      case KernelBytecode::kShrInt:
+      case KernelBytecode::kCompareIntEq:
+      case KernelBytecode::kCompareIntGt:
+      case KernelBytecode::kCompareIntLt:
+      case KernelBytecode::kCompareIntGe:
+      case KernelBytecode::kCompareIntLe:
+      case KernelBytecode::kAddDouble:
+      case KernelBytecode::kSubDouble:
+      case KernelBytecode::kMulDouble:
+      case KernelBytecode::kDivDouble:
+      case KernelBytecode::kCompareDoubleEq:
+      case KernelBytecode::kCompareDoubleGt:
+      case KernelBytecode::kCompareDoubleLt:
+      case KernelBytecode::kCompareDoubleGe:
+      case KernelBytecode::kCompareDoubleLe:
         return true;
       default:
         return false;

runtime/vm/debugger.cc

@@ -247,7 +247,8 @@ ActivationFrame::ActivationFrame(uword pc,
       var_descriptors_(LocalVarDescriptors::ZoneHandle()),
       desc_indices_(8),
       pc_desc_(PcDescriptors::ZoneHandle()) {
-  ASSERT(!function_.IsNull());  // Frames with bytecode stubs should be skipped.
+  // The frame of a bytecode stub has a null function. It may be encountered
+  // when single stepping.
 }
 #endif  // !defined(DART_PRECOMPILED_RUNTIME)
 
@@ -676,7 +677,8 @@ void ActivationFrame::GetVarDescriptors() {
 }
 
 bool ActivationFrame::IsDebuggable() const {
-  return Debugger::IsDebuggable(function());
+  // When stepping in bytecode stub, function is null.
+  return !function().IsNull() && Debugger::IsDebuggable(function());
 }
 
 void ActivationFrame::PrintDescriptorsError(const char* message) {
@@ -996,8 +998,9 @@ void ActivationFrame::ExtractTokenPositionFromAsyncClosure() {
         if (yield_point_index == await_jump_var) {
           token_pos_ = iter.TokenPos();
           token_pos_initialized_ = true;
-          try_index_ = bytecode.GetTryIndexAtPc(bytecode.PayloadStart() +
-                                                iter.PcOffset());
+          const uword return_address =
+              KernelBytecode::Next(bytecode.PayloadStart() + iter.PcOffset());
+          try_index_ = bytecode.GetTryIndexAtPc(return_address);
           return;
         }
         ++yield_point_index;
@@ -1561,15 +1564,18 @@ const char* ActivationFrame::ToCString() {
   intptr_t line = LineNumber();
   const char* func_name = Debugger::QualifiedFunctionName(function());
   return Thread::Current()->zone()->PrintToString(
-      "[ Frame pc(0x%" Px ") fp(0x%" Px ") sp(0x%" Px
+      "[ Frame pc(0x%" Px " offset:0x%" Px ") fp(0x%" Px ") sp(0x%" Px
       ")\n"
       "\tfunction = %s\n"
       "\turl = %s\n"
       "\tline = %" Pd
       "\n"
       "\tcontext = %s\n"
       "\tcontext level = %" Pd " ]\n",
-      pc(), fp(), sp(), func_name, url.ToCString(), line, ctx_.ToCString(),
+      pc(),
+      pc() -
+          (IsInterpreted() ? bytecode().PayloadStart() : code().PayloadStart()),
+      fp(), sp(), func_name, url.ToCString(), line, ctx_.ToCString(),
       ContextLevel());
 }
 
@@ -2580,9 +2586,11 @@ ActivationFrame* Debugger::TopDartFrame() const {
 #if !defined(DART_PRECOMPILED_RUNTIME)
     if (frame->is_interpreted()) {
       Bytecode& bytecode = Bytecode::Handle(frame->LookupDartBytecode());
-      if (bytecode.function() == Function::null()) {
-        continue;  // Skip bytecode stub frame.
-      }
+      // Note that we do not skip bytecode stub frame (with a null function),
+      // so that we can ignore a single stepping breakpoint in such a frame.
+      // A bytecode stub contains a VM internal bytecode followed by a
+      // ReturnTOS bytecode. The single step on the ReturnTOS bytecode
+      // needs to be skipped.
       ActivationFrame* activation =
           new ActivationFrame(frame->pc(), frame->fp(), frame->sp(), bytecode);
       return activation;
@@ -3053,19 +3061,20 @@ TokenPosition Debugger::ResolveBreakpointPos(bool in_bytecode,
 
 #if !defined(DART_PRECOMPILED_RUNTIME)
 // Find a 'debug break checked' bytecode in the range [pc..end_pc[ and return
-// the pc after it or nullptr.
-static const KBCInstr* FindBreakpointCheckedInstr(const KBCInstr* pc,
-                                                  const KBCInstr* end_pc) {
-  while ((pc < end_pc) && !KernelBytecode::IsDebugBreakCheckedOpcode(pc)) {
+// the pc after it or 0 if not found.
+static uword FindBreakpointCheckedInstr(uword pc, uword end_pc) {
+  while ((pc < end_pc) && !KernelBytecode::IsDebugBreakCheckedOpcode(
+                              reinterpret_cast<const KBCInstr*>(pc))) {
     pc = KernelBytecode::Next(pc);
   }
   if (pc < end_pc) {
-    ASSERT(KernelBytecode::IsDebugBreakCheckedOpcode(pc));
+    ASSERT(KernelBytecode::IsDebugBreakCheckedOpcode(
+        reinterpret_cast<const KBCInstr*>(pc)));
     // The checked debug break pc must point to the next bytecode.
     return KernelBytecode::Next(pc);
   }
   // No 'debug break checked' bytecode in the range.
-  return nullptr;
+  return 0;
 }
 #endif  // !defined(DART_PRECOMPILED_RUNTIME)
 
@@ -3079,45 +3088,40 @@ void Debugger::MakeCodeBreakpointAt(const Function& func,
   if (func.HasBytecode()) {
     Bytecode& bytecode = Bytecode::Handle(func.bytecode());
     ASSERT(!bytecode.IsNull());
-    const KBCInstr* pc = nullptr;
+    uword pc = 0;
     if (bytecode.HasSourcePositions()) {
       kernel::BytecodeSourcePositionsIterator iter(Thread::Current()->zone(),
                                                    bytecode);
       bool check_range = false;
       while (iter.MoveNext()) {
         if (check_range) {
-          const KBCInstr* end_pc =
-              reinterpret_cast<const KBCInstr*>(bytecode.PayloadStart()) +
-              iter.PcOffset();
+          uword end_pc = bytecode.PayloadStart() + iter.PcOffset();
           check_range = false;
           // Find a 'debug break checked' bytecode in the range [pc..end_pc[.
           pc = FindBreakpointCheckedInstr(pc, end_pc);
-          if (pc != nullptr) {
+          if (pc != 0) {
             // TODO(regis): We may want to find all PCs for a token position,
             // e.g. in the case of duplicated bytecode in finally clauses.
             break;
           }
         }
         if (iter.TokenPos() == loc->token_pos_) {
-          pc = reinterpret_cast<const KBCInstr*>(bytecode.PayloadStart()) +
-               iter.PcOffset();
+          pc = bytecode.PayloadStart() + iter.PcOffset();
           check_range = true;
         }
       }
       if (check_range) {
-        ASSERT(pc != nullptr);
+        ASSERT(pc != 0);
         // Use the end of the bytecode as the end of the range to check.
         pc = FindBreakpointCheckedInstr(
-            pc, reinterpret_cast<const KBCInstr*>(bytecode.PayloadStart()) +
-                    bytecode.Size());
+            pc, bytecode.PayloadStart() + bytecode.Size());
       }
     }
-    if (pc != nullptr) {
-      CodeBreakpoint* code_bpt = GetCodeBreakpoint(reinterpret_cast<uword>(pc));
+    if (pc != 0) {
+      CodeBreakpoint* code_bpt = GetCodeBreakpoint(pc);
       if (code_bpt == NULL) {
         // No code breakpoint for this code exists; create one.
-        code_bpt = new CodeBreakpoint(bytecode, loc->token_pos_,
-                                      reinterpret_cast<uword>(pc));
+        code_bpt = new CodeBreakpoint(bytecode, loc->token_pos_, pc);
         RegisterCodeBreakpoint(code_bpt);
       }
       code_bpt->set_bpt_location(loc);

runtime/vm/debugger.h

@@ -297,7 +297,6 @@ class ActivationFrame : public ZoneAllocated {
   uword fp() const { return fp_; }
   uword sp() const { return sp_; }
   const Function& function() const {
-    ASSERT(!function_.IsNull());
     return function_;
   }
   const Code& code() const {