Remember deopt-id -> context-level mappings in var descriptors.

Remember deopt-id -> context-level mappings in var descriptors.

 - Add deopt ids to DebugStepInstr and StrictCompareInstr since the debugger can stop there.
 - Add missing pc descriptor in DBC's StringInterpolateInstr.

Re-enable async_debugger, which had been crashing flakily from context mismatches.

R=vegorov@google.com

Committed: https://github.com/dart-lang/sdk/commit/2109cc53bf58eed6c71c7fde4af5f4a334c8d3f7

[rmacnak, 2017-05-25 01:16:48]

Builds on https://chromiumcodereview.appspot.com/2900963008/

[rmacnak, 2017-05-25 22:29:21]

Description was changed from

==========
RFC: Remember deopt-id -> context-level mappings in var descriptors.

 - Add deopt ids to DebugStepInstr and StrictCompareInstr since the debugger can
stop there.
 - Add missing pc descriptor in DBC's StringInterpolateInstr.

TODO: Encounter deopt-ids as ranges.
TODO: Kernel.
==========

to

==========
RFC: Remember deopt-id -> context-level mappings in var descriptors.

 - Add deopt ids to DebugStepInstr and StrictCompareInstr since the debugger can
stop there.
 - Add missing pc descriptor in DBC's StringInterpolateInstr.

TODO: Kernel.
==========

[rmacnak, 2017-05-26 00:41:54]

Description was changed from

==========
RFC: Remember deopt-id -> context-level mappings in var descriptors.

 - Add deopt ids to DebugStepInstr and StrictCompareInstr since the debugger can
stop there.
 - Add missing pc descriptor in DBC's StringInterpolateInstr.

TODO: Kernel.
==========

to

==========
Remember deopt-id -> context-level mappings in var descriptors.

 - Add deopt ids to DebugStepInstr and StrictCompareInstr since the debugger can
stop there.
 - Add missing pc descriptor in DBC's StringInterpolateInstr.
==========

[rmacnak, 2017-05-26 22:28:12]

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

[rmacnak, 2017-05-30 20:01:02]

This is now working, PTAL.

[Vyacheslav Egorov (Google), 2017-05-31 14:29:51]

I think overall it looks good

https://codereview.chromium.org/2903993002/diff/100001/runtime/vm/flow_graph_...
File runtime/vm/flow_graph_builder.h (right):

https://codereview.chromium.org/2903993002/diff/100001/runtime/vm/flow_graph_...
runtime/vm/flow_graph_builder.h:199: ZoneGrowableArray<intptr_t>*
context_level_array_;
it might be worth adding a comment here that this array contains 

(deopt_id, context_level)

tuples.

https://codereview.chromium.org/2903993002/diff/100001/runtime/vm/scopes.cc
File runtime/vm/scopes.cc (right):

https://codereview.chromium.org/2903993002/diff/100001/runtime/vm/scopes.cc#n...
runtime/vm/scopes.cc:294: for (intptr_t start = 0; start <
context_level_array->length();) {
Maybe context_level_array tuples need to be sorted by deopt_id before processing
it here to prevent dependence on IR was generated (e.g. that AST order induces
deopt_order)?

[rmacnak, 2017-05-31 18:03:17]

https://codereview.chromium.org/2903993002/diff/100001/runtime/vm/flow_graph_...
File runtime/vm/flow_graph_builder.h (right):

https://codereview.chromium.org/2903993002/diff/100001/runtime/vm/flow_graph_...
runtime/vm/flow_graph_builder.h:199: ZoneGrowableArray<intptr_t>*
context_level_array_;
On 2017/05/31 14:29:51, Vyacheslav Egorov (Google) wrote:
> it might be worth adding a comment here that this array contains 
> 
> (deopt_id, context_level)
> 
> tuples.

Added.

https://codereview.chromium.org/2903993002/diff/100001/runtime/vm/scopes.cc
File runtime/vm/scopes.cc (right):

https://codereview.chromium.org/2903993002/diff/100001/runtime/vm/scopes.cc#n...
runtime/vm/scopes.cc:294: for (intptr_t start = 0; start <
context_level_array->length();) {
On 2017/05/31 14:29:51, Vyacheslav Egorov (Google) wrote:
> Maybe context_level_array tuples need to be sorted by deopt_id before
processing
> it here to prevent dependence on IR was generated (e.g. that AST order induces
> deopt_order)? 

We don't depend on the order the AST is processed or the order the IR is
generated. We do depend on the flow graph builders to have the correct context
level at the time a deopt id is allocated. The range encoding depends on the
pairs having sorted deopt ids, which happens because we allocate deopt ids
sequentially and record them as they are allocated. We assert in L303 that deopt
ids are ascending. Added a comment explicitly noting this.

[rmacnak, 2017-05-31 18:03:50]

Description was changed from

==========
Remember deopt-id -> context-level mappings in var descriptors.

 - Add deopt ids to DebugStepInstr and StrictCompareInstr since the debugger can
stop there.
 - Add missing pc descriptor in DBC's StringInterpolateInstr.
==========

to

==========
Remember deopt-id -> context-level mappings in var descriptors.

 - Add deopt ids to DebugStepInstr and StrictCompareInstr since the debugger can
stop there.
 - Add missing pc descriptor in DBC's StringInterpolateInstr.

Re-enable async_debugger.
==========

[rmacnak, 2017-05-31 18:04:22]

Description was changed from

==========
Remember deopt-id -> context-level mappings in var descriptors.

 - Add deopt ids to DebugStepInstr and StrictCompareInstr since the debugger can
stop there.
 - Add missing pc descriptor in DBC's StringInterpolateInstr.

Re-enable async_debugger.
==========

to

==========
Remember deopt-id -> context-level mappings in var descriptors.

 - Add deopt ids to DebugStepInstr and StrictCompareInstr since the debugger can
stop there.
 - Add missing pc descriptor in DBC's StringInterpolateInstr.

Re-enable async_debugger, which had been crashing flakily from context
mismatches.
==========

[Vyacheslav Egorov (Google), 2017-06-01 12:32:41]

lgtm

https://codereview.chromium.org/2903993002/diff/100001/runtime/vm/scopes.cc
File runtime/vm/scopes.cc (right):

https://codereview.chromium.org/2903993002/diff/100001/runtime/vm/scopes.cc#n...
runtime/vm/scopes.cc:294: for (intptr_t start = 0; start <
context_level_array->length();) {
On 2017/05/31 18:03:17, rmacnak wrote:
> On 2017/05/31 14:29:51, Vyacheslav Egorov (Google) wrote:
> > Maybe context_level_array tuples need to be sorted by deopt_id before
> processing
> > it here to prevent dependence on IR was generated (e.g. that AST order
induces
> > deopt_order)? 
> 
> We don't depend on the order the AST is processed or the order the IR is
> generated. We do depend on the flow graph builders to have the correct context
> level at the time a deopt id is allocated. The range encoding depends on the
> pairs having sorted deopt ids, which happens because we allocate deopt ids
> sequentially and record them as they are allocated. We assert in L303 that
deopt
> ids are ascending. Added a comment explicitly noting this.

Yeah, I am not sure what I was thinking. GetNextDeoptId obviously returns deopt
ids in the ascending order.

https://codereview.chromium.org/2903993002/diff/120001/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (right):

https://codereview.chromium.org/2903993002/diff/120001/runtime/vm/debugger.cc...
runtime/vm/debugger.cc:704: function().ToQualifiedCString());
Maybe move this code to a helper function. 

Then on all FATAL errors in the debugger we can just call this function and get
the same level of details (e.g. deopt_id, pc, state of registers, disassembly).

[rmacnak, 2017-06-01 19:26:10]

thanks!

https://codereview.chromium.org/2903993002/diff/120001/runtime/vm/debugger.cc
File runtime/vm/debugger.cc (right):

https://codereview.chromium.org/2903993002/diff/120001/runtime/vm/debugger.cc...
runtime/vm/debugger.cc:704: function().ToQualifiedCString());
On 2017/06/01 12:32:41, Vyacheslav Egorov (Google) wrote:
> Maybe move this code to a helper function. 
> 
> Then on all FATAL errors in the debugger we can just call this function and
get
> the same level of details (e.g. deopt_id, pc, state of registers,
disassembly).

Done like PrintContextMismatch.

[rmacnak, 2017-06-01 19:33:38]

Description was changed from

==========
Remember deopt-id -> context-level mappings in var descriptors.

 - Add deopt ids to DebugStepInstr and StrictCompareInstr since the debugger can
stop there.
 - Add missing pc descriptor in DBC's StringInterpolateInstr.

Re-enable async_debugger, which had been crashing flakily from context
mismatches.
==========

to

==========
Remember deopt-id -> context-level mappings in var descriptors.

 - Add deopt ids to DebugStepInstr and StrictCompareInstr since the debugger can
stop there.
 - Add missing pc descriptor in DBC's StringInterpolateInstr.

Re-enable async_debugger, which had been crashing flakily from context
mismatches.

R=vegorov@google.com

Committed:
https://github.com/dart-lang/sdk/commit/2109cc53bf58eed6c71c7fde4af5f4a334c8d3f7
==========

[rmacnak, 2017-06-01 19:33:39]

Committed patchset #8 (id:140001) manually as
2109cc53bf58eed6c71c7fde4af5f4a334c8d3f7 (presubmit successful).