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

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

When setting a breakpoint at a fast Smi op, replace the fast Smi op instruction with a Nop so we don't skip the following DebugBreak in the Smi case and don't attempt to increment an ICData counter through pool slot 0.

R=zra@google.com

Committed: https://github.com/dart-lang/sdk/commit/3dba3a77c1360ce447a5fb8ecf1f48348067f5de

[rmacnak, 2016-07-28 17:50:19]

rmacnak@google.com changed reviewers:
+ zra@google.com

[zra, 2016-07-28 18:08:49]

https://codereview.chromium.org/2194493002/diff/1/runtime/vm/debugger_dbc.cc
File runtime/vm/debugger_dbc.cc (right):

https://codereview.chromium.org/2194493002/diff/1/runtime/vm/debugger_dbc.cc#...
runtime/vm/debugger_dbc.cc:65: if (Bytecode::DecodeOpcode(saved_value_) ==
Bytecode::kInstanceCall2) {
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:15]

https://codereview.chromium.org/2194493002/diff/1/runtime/vm/debugger_dbc.cc
File runtime/vm/debugger_dbc.cc (right):

https://codereview.chromium.org/2194493002/diff/1/runtime/vm/debugger_dbc.cc#...
runtime/vm/debugger_dbc.cc:65: if (Bytecode::DecodeOpcode(saved_value_) ==
Bytecode::kInstanceCall2) {
On 2016/07/28 18:08:48, zra wrote:
> 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?

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:48]

lgtm

https://codereview.chromium.org/2194493002/diff/1/runtime/vm/debugger_dbc.cc
File runtime/vm/debugger_dbc.cc (right):

https://codereview.chromium.org/2194493002/diff/1/runtime/vm/debugger_dbc.cc#...
runtime/vm/debugger_dbc.cc:65: if (Bytecode::DecodeOpcode(saved_value_) ==
Bytecode::kInstanceCall2) {
On 2016/07/28 20:13:14, rmacnak wrote:
> On 2016/07/28 18:08:48, zra wrote:
> > 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?
> 
> 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.)

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

https://codereview.chromium.org/2194493002/diff/20001/runtime/observatory/tes...
File runtime/observatory/tests/service/breakpoint_two_args_checked_test.dart
(right):

https://codereview.chromium.org/2194493002/diff/20001/runtime/observatory/tes...
runtime/observatory/tests/service/breakpoint_two_args_checked_test.dart:1: //
Copyright (c) 2015, the Dart project authors.  Please see the AUTHORS file
2016

[rmacnak, 2016-07-28 20:40:53]

https://codereview.chromium.org/2194493002/diff/20001/runtime/observatory/tes...
File runtime/observatory/tests/service/breakpoint_two_args_checked_test.dart
(right):

https://codereview.chromium.org/2194493002/diff/20001/runtime/observatory/tes...
runtime/observatory/tests/service/breakpoint_two_args_checked_test.dart:1: //
Copyright (c) 2015, the Dart project authors.  Please see the AUTHORS file
On 2016/07/28 20:17:47, zra wrote:
> 2016

Done.

[rmacnak, 2016-07-28 20:41:26]

Description was changed from

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

When setting a breakpoint at a fast Smi op, replace the fast Smi op instruction
with a Nop so we don't skip the following DebugBreak in the Smi case and don't
attempt to increment an ICData counter through pool slot 0.
==========

to

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

When setting a breakpoint at a fast Smi op, replace the fast Smi op instruction
with a Nop so we don't skip the following DebugBreak in the Smi case and don't
attempt to increment an ICData counter through pool slot 0.

R=zra@google.com

Committed:
https://github.com/dart-lang/sdk/commit/3dba3a77c1360ce447a5fb8ecf1f48348067f5de
==========

[rmacnak, 2016-07-28 20:41:27]

Committed patchset #3 (id:40001) manually as
3dba3a77c1360ce447a5fb8ecf1f48348067f5de (presubmit successful).