DBC: Fix not-stopping/crashing at fast Smi op breakpoints.

runtime/vm/debugger_dbc.cc

 // Copyright (c) 2016, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #include "vm/globals.h"
 #if defined(TARGET_ARCH_DBC)
 
 #include "vm/code_patcher.h"
 #include "vm/cpu.h"
 #include "vm/debugger.h"
 #include "vm/instructions.h"
 #include "vm/stub_code.h"
 
 namespace dart {
 
 #ifndef PRODUCT
 
 RawCode* CodeBreakpoint::OrigStubAddress() const {
   return reinterpret_cast<RawCode*>(static_cast<uintptr_t>(saved_value_));
 }
 
 
 static Instr* CallInstructionFromReturnAddress(uword pc) {
   return reinterpret_cast<Instr*>(pc) - 1;
 }
 
 
+static Instr* FastSmiInstructionFromCallReturnAddress(uword pc) {
+  return reinterpret_cast<Instr*>(pc) - 2;
+}
+
+
 void CodeBreakpoint::PatchCode() {
   ASSERT(!is_enabled_);
   const Code& code = Code::Handle(code_);
   const Instructions& instrs = Instructions::Handle(code.instructions());
   {
     WritableInstructionsScope writable(instrs.EntryPoint(), instrs.size());
     saved_value_ = *CallInstructionFromReturnAddress(pc_);
     switch (breakpoint_kind_) {
       case RawPcDescriptors::kIcCall:
       case RawPcDescriptors::kUnoptStaticCall: {
         // DebugBreak has an A operand matching the call it replaces.
         // This ensures that Return instructions continue to work - as they
         // look at calls to figure out how many arguments to drop.
         *CallInstructionFromReturnAddress(pc_) =
             Bytecode::Encode(Bytecode::kDebugBreak,
                              Bytecode::DecodeArgc(saved_value_),
                              0,
                              0);
         break;
       }
 
       case RawPcDescriptors::kRuntimeCall: {
         *CallInstructionFromReturnAddress(pc_) = Bytecode::kDebugBreak;
         break;
       }
 
       default:
         UNREACHABLE();
     }
+
+    // If this call is the fall-through for a fast Smi op, also disable the fast
+    // Smi op.
+    if (Bytecode::DecodeOpcode(saved_value_) == Bytecode::kInstanceCall2) {

[zra, 2016/07/28 18:08:48]

Looking at the code in InstanceCallInstr::EmitNativeCode() it looks like we can
generate InstanceCall2 instructions that aren't preceded by a fast smi op. Do we
not have a test that tries to set a breakpoint in that situation?

[rmacnak, 2016/07/28 20:13:14]

So those other cases turn out to be []= and some kinds of 'is' checks. Added a
test that creates breakpoints for those and switched this patching logic to also
check the preceding instruction is a fast Smi op. (Alternatively we could give
the fast Smi ops a different InstanceCall2 instruction.)

[zra, 2016/07/28 20:17:47]

Cool. Thanks for looking into this and adding a test. 

+      saved_value_fastsmi_ = *FastSmiInstructionFromCallReturnAddress(pc_);
+      ASSERT(Bytecode::IsFastSmiOpcode(saved_value_fastsmi_));
+      *FastSmiInstructionFromCallReturnAddress(pc_) =
+          Bytecode::Encode(Bytecode::kNop, 0, 0, 0);
+    }
   }
   is_enabled_ = true;
 }
 
 
 void CodeBreakpoint::RestoreCode() {
   ASSERT(is_enabled_);
   const Code& code = Code::Handle(code_);
   const Instructions& instrs = Instructions::Handle(code.instructions());
   {
     WritableInstructionsScope writable(instrs.EntryPoint(), instrs.size());
     switch (breakpoint_kind_) {
       case RawPcDescriptors::kIcCall:
       case RawPcDescriptors::kUnoptStaticCall:
       case RawPcDescriptors::kRuntimeCall: {
         *CallInstructionFromReturnAddress(pc_) = saved_value_;
         break;
       }
       default:
         UNREACHABLE();
     }
+
+    if (Bytecode::DecodeOpcode(saved_value_) == Bytecode::kInstanceCall2) {
+      Instr current_instr = *FastSmiInstructionFromCallReturnAddress(pc_);
+      ASSERT(Bytecode::DecodeOpcode(current_instr) == Bytecode::kNop);
+      *FastSmiInstructionFromCallReturnAddress(pc_) = saved_value_fastsmi_;
+    }
   }
   is_enabled_ = false;
 }
 
 #endif  // !PRODUCT
 
 }  // namespace dart
 
 #endif  // defined TARGET_ARCH_DBC