Enable stepping into callback passed to builtins (e.g. Array.forEach).

Enable stepping into callback passed to builtins (e.g. Array.forEach).

BUG=109564
TEST=

Committed: https://code.google.com/p/v8/source/detail?r=11399

[Yang, 2012-04-19 14:09:42]

In order to step into callback functions from within builtin functions such as
Array.forEach (those are implemented in array.js), we now call
%PrepareBreakSlotsForCallback to set break slots if
%DebugStepIntoBuiltinCallback indicates that we are indeed using the debugger to
step.

Please take a look.

[Søren Gjesse, 2012-04-20 08:33:25]

lgtm with some suggestions

I like the way that step in is propagates through the natives code.

http://codereview.chromium.org/10078014/diff/8001/src/array.js
File src/array.js (right):

http://codereview.chromium.org/10078014/diff/8001/src/array.js#newcode1030
src/array.js:1030: if (%DebugStepIntoBuiltinCallback(f)) {
Maybe rename DebugStepIntoBuiltinCallback to something like
DebugDoesFunctionSupportStepping.

http://codereview.chromium.org/10078014/diff/8001/src/array.js#newcode1030
src/array.js:1030: if (%DebugStepIntoBuiltinCallback(f)) {
The only thing I don't like here is the code duplication. I don't have an
immediate idea, but it would be great if we could get rid of it somehow. It will
be prone to errors when changing the code missing one of the places.

http://codereview.chromium.org/10078014/diff/8001/src/array.js#newcode1030
src/array.js:1030: if (%DebugStepIntoBuiltinCallback(f)) {
An important thing performance wise is that it should be quite simple to write
%DebugStepIntoBuiltinCallback as a %_DebugStepIntoBuiltinCallback (inline
assembly) function if this runtime call gives performance problems.

http://codereview.chromium.org/10078014/diff/8001/src/array.js#newcode1035
src/array.js:1035: %PrepareBreakSlotsForCallback(f);
I think the name here i somewhat misleading. Maybe
"%DebugPrepareStepInIfStepping".

http://codereview.chromium.org/10078014/diff/8001/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/10078014/diff/8001/src/runtime.cc#newcode4712
src/runtime.cc:4712: // Set break slots for the callback function that is passed
to a buil-tin
"break slots" -> "one shot breakpoints"

[Yang, 2012-04-20 11:06:54]

http://codereview.chromium.org/10078014/diff/8001/src/array.js
File src/array.js (right):

http://codereview.chromium.org/10078014/diff/8001/src/array.js#newcode1030
src/array.js:1030: if (%DebugStepIntoBuiltinCallback(f)) {
On 2012/04/20 08:33:25, Søren Gjesse wrote:
> An important thing performance wise is that it should be quite simple to write
> %DebugStepIntoBuiltinCallback as a %_DebugStepIntoBuiltinCallback (inline
> assembly) function if this runtime call gives performance problems.

This would be great, but I have no idea how to check whether the debugger is
active from generated code... I'll implement this as a follow-up CL if you can
give me some hints.

http://codereview.chromium.org/10078014/diff/8001/src/array.js#newcode1030
src/array.js:1030: if (%DebugStepIntoBuiltinCallback(f)) {
On 2012/04/20 08:33:25, Søren Gjesse wrote:
> The only thing I don't like here is the code duplication. I don't have an
> immediate idea, but it would be great if we could get rid of it somehow. It
will
> be prone to errors when changing the code missing one of the places.

I don't see anything simple either. The one way I can think of is to trigger
recompile of builtins for debugging as well, and insert code for
%DebugPrepareStepInIfStepping only when compiling for debugging. But that is a
lot more work and we would need to distinguish between builtins that do callback
and therefore need recompiling, and those that do not.

The way it's currently implemented has little impact on performance. I'll mark
duplicate code pieces with comments.

http://codereview.chromium.org/10078014/diff/8001/src/array.js#newcode1035
src/array.js:1035: %PrepareBreakSlotsForCallback(f);
On 2012/04/20 08:33:25, Søren Gjesse wrote:
> I think the name here i somewhat misleading. Maybe
> "%DebugPrepareStepInIfStepping".

Done.

http://codereview.chromium.org/10078014/diff/8001/src/runtime.cc
File src/runtime.cc (right):

http://codereview.chromium.org/10078014/diff/8001/src/runtime.cc#newcode4712
src/runtime.cc:4712: // Set break slots for the callback function that is passed
to a buil-tin
On 2012/04/20 08:33:25, Søren Gjesse wrote:
> "break slots" -> "one shot breakpoints"

Done.