Allow setting breakpoints in function literal field initializers under --dfe.

Allow setting breakpoints in function literal field initializers under --dfe.

The change 3b9cf7351bb0e92e0789eb68b37d130886644bd8 fixed #29581 for the
case when --dfe is not used. This change fixes the issue for the --dfe
configuration as well.

Tests which now pass have been taken out of the status files.

R=asiva@google.com

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

[sivachandra, 2017-07-17 21:44:30]

sivachandra@google.com changed reviewers:
+ asiva@google.com

[sivachandra, 2017-07-17 21:44:31]

I am not sure if the change uses the existing kernel infrastructure in the
spirit that jensj@ or kmillikin@ intended it to be used as. I can work with
them, when they are back from their vacations, to refactor the code in another
round.

[siva, 2017-07-17 22:39:24]

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

https://codereview.chromium.org/2979163002/diff/1/runtime/vm/debugger.cc#newc...
runtime/vm/debugger.cc:2685: #endif  // !DART_PRECOMPILED_RUNTIME
Is it possible to abstract this out as a function in KernelReader and invoke it
conditionally under 'if (FLAG_use_dart_front_end)'

Also can a similar check
if (!field.has_initialized()) {

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

https://codereview.chromium.org/2979163002/diff/1/runtime/vm/kernel_to_il.cc#...
runtime/vm/kernel_to_il.cc:86: allocation_space_(thread->IsMutatorThread() ?
Heap::kNew : Heap::kOld),
When does this constructor run on a non Mutator thread?

[sivachandra, 2017-07-17 23:49:36]

PTaL at patch set 2.

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

https://codereview.chromium.org/2979163002/diff/1/runtime/vm/debugger.cc#newc...
runtime/vm/debugger.cc:2685: #endif  // !DART_PRECOMPILED_RUNTIME
On 2017/07/17 22:39:23, siva wrote:
> Is it possible to abstract this out as a function in KernelReader and invoke
it
> conditionally under 'if (FLAG_use_dart_front_end)'
> 
> Also can a similar check
> if (!field.has_initialized()) {

Done.

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

https://codereview.chromium.org/2979163002/diff/1/runtime/vm/kernel_to_il.cc#...
runtime/vm/kernel_to_il.cc:86: allocation_space_(thread->IsMutatorThread() ?
Heap::kNew : Heap::kOld),
On 2017/07/17 22:39:23, siva wrote:
> When does this constructor run on a non Mutator thread?

Per my limited knowledge, the use case added by this change does not run on a
non-mutator thread. However, I just copied this part from the above constructor
to be consistent.

[sivachandra, 2017-07-18 19:50:45]

Description was changed from

==========
Fix issue #29581 for the --dfe configuration.

Tests which now pass have been taken out of the status files.
==========

to

==========
Allow setting breakpoints in function literal field initializers with dfe.

The change 3b9cf7351bb0e92e0789eb68b37d130886644bd8 fixed #29581 for the
case when --dfe is not used. This change fixes the issue for the --dfe
configuration as well.

Tests which now pass have been taken out of the status files.
==========

[sivachandra, 2017-07-18 19:51:25]

Description was changed from

==========
Allow setting breakpoints in function literal field initializers with dfe.

The change 3b9cf7351bb0e92e0789eb68b37d130886644bd8 fixed #29581 for the
case when --dfe is not used. This change fixes the issue for the --dfe
configuration as well.

Tests which now pass have been taken out of the status files.
==========

to

==========
Allow setting breakpoints in function literal field initializers under --dfe.

The change 3b9cf7351bb0e92e0789eb68b37d130886644bd8 fixed #29581 for the
case when --dfe is not used. This change fixes the issue for the --dfe
configuration as well.

Tests which now pass have been taken out of the status files.
==========

[siva, 2017-07-18 22:49:07]

lgtm with some comments.

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

https://codereview.chromium.org/2979163002/diff/1/runtime/vm/kernel_to_il.cc#...
runtime/vm/kernel_to_il.cc:86: allocation_space_(thread->IsMutatorThread() ?
Heap::kNew : Heap::kOld),
On 2017/07/17 23:49:36, sivachandra wrote:
> On 2017/07/17 22:39:23, siva wrote:
> > When does this constructor run on a non Mutator thread?
> 
> Per my limited knowledge, the use case added by this change does not run on a
> non-mutator thread. However, I just copied this part from the above
constructor
> to be consistent.

True but that was because this code could be executed by the background
compiler, but in your case we should not be ever executing it from anything but
the mutator thread, maybe you could add and assert in the constructor
ASSERT(thread->IsMutatorThread());

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

https://codereview.chromium.org/2979163002/diff/20001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:2683: #endif  // !DART_PRECOMPILED_RUNTIME
Strictly speaking this entire function and most of other Debugger functionality
should not be included in DART_PRECOMPILED_RUNTIME, so maybe it is not necessary
to bracket this code here and instead look at excluding debugger stuff in
another CL.

https://codereview.chromium.org/2979163002/diff/20001/runtime/vm/kernel_reade...
File runtime/vm/kernel_reader.cc (right):

https://codereview.chromium.org/2979163002/diff/20001/runtime/vm/kernel_reade...
runtime/vm/kernel_reader.cc:889: const Script& script = Script::Handle(zone,
field.Script());
KernelReader seems to have a zone_ field so I guess you should be able to do
const Script& script = Script::Handle(Z, field.Script());

https://codereview.chromium.org/2979163002/diff/20001/runtime/vm/kernel_reade...
runtime/vm/kernel_reader.cc:892: Thread::Current(),
script.kernel_string_offsets(),
KernelReader also has a thread_ field that you should be able to use instead of
Thread::Current();

[sivachandra, 2017-07-19 00:14:22]

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

https://codereview.chromium.org/2979163002/diff/1/runtime/vm/kernel_to_il.cc#...
runtime/vm/kernel_to_il.cc:86: allocation_space_(thread->IsMutatorThread() ?
Heap::kNew : Heap::kOld),
On 2017/07/18 22:49:06, siva wrote:
> On 2017/07/17 23:49:36, sivachandra wrote:
> > On 2017/07/17 22:39:23, siva wrote:
> > > When does this constructor run on a non Mutator thread?
> > 
> > Per my limited knowledge, the use case added by this change does not run on
a
> > non-mutator thread. However, I just copied this part from the above
> constructor
> > to be consistent.
> 
> True but that was because this code could be executed by the background
> compiler, but in your case we should not be ever executing it from anything
but
> the mutator thread, maybe you could add and assert in the constructor
> ASSERT(thread->IsMutatorThread());

Done.

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

https://codereview.chromium.org/2979163002/diff/20001/runtime/vm/debugger.cc#...
runtime/vm/debugger.cc:2683: #endif  // !DART_PRECOMPILED_RUNTIME
On 2017/07/18 22:49:07, siva wrote:
> Strictly speaking this entire function and most of other Debugger
functionality
> should not be included in DART_PRECOMPILED_RUNTIME, so maybe it is not
necessary
> to bracket this code here and instead look at excluding debugger stuff in
> another CL.

Acknowledged.

https://codereview.chromium.org/2979163002/diff/20001/runtime/vm/kernel_reade...
File runtime/vm/kernel_reader.cc (right):

https://codereview.chromium.org/2979163002/diff/20001/runtime/vm/kernel_reade...
runtime/vm/kernel_reader.cc:889: const Script& script = Script::Handle(zone,
field.Script());
On 2017/07/18 22:49:07, siva wrote:
> KernelReader seems to have a zone_ field so I guess you should be able to do
> const Script& script = Script::Handle(Z, field.Script());

This new method is a static method. So, kernel fields are not accessible here.

https://codereview.chromium.org/2979163002/diff/20001/runtime/vm/kernel_reade...
runtime/vm/kernel_reader.cc:892: Thread::Current(),
script.kernel_string_offsets(),
On 2017/07/18 22:49:07, siva wrote:
> KernelReader also has a thread_ field that you should be able to use instead
of
> Thread::Current();

Same reason as above that this is a static method.

[sivachandra, 2017-07-19 20:17:30]

Description was changed from

==========
Allow setting breakpoints in function literal field initializers under --dfe.

The change 3b9cf7351bb0e92e0789eb68b37d130886644bd8 fixed #29581 for the
case when --dfe is not used. This change fixes the issue for the --dfe
configuration as well.

Tests which now pass have been taken out of the status files.
==========

to

==========
Allow setting breakpoints in function literal field initializers under --dfe.

The change 3b9cf7351bb0e92e0789eb68b37d130886644bd8 fixed #29581 for the
case when --dfe is not used. This change fixes the issue for the --dfe
configuration as well.

Tests which now pass have been taken out of the status files.

R=asiva@google.com

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

[sivachandra, 2017-07-19 20:17:31]

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