Classes: Add support for stepping through default constructors

Classes: Add support for stepping through default constructors

If a class extends another class and it doesn't provide a constructor,
one is created for them. We therefore need to ensure that stepping into
the constructor steps into the super class constructor.

BUG=v8:3674
LOG=Y
R=dslomov@chromium.org, aandrey , yurys

Committed: https://chromium.googlesource.com/v8/v8/+/28da8fd6099922e6ea236ca8cc574982495fd2bc

[arv (Not doing code reviews), 2014-11-14 16:48:24]

Patchset #1 (id:1) has been deleted

[arv (Not doing code reviews), 2014-11-14 16:51:57]

Remove stray change

[arv (Not doing code reviews), 2014-11-14 16:52:39]

PTAL

(I wasn't sure who the right person for debugger related changes is?)

[Dmitry Lomov (no reviews), 2014-11-14 17:19:15]

looks good but Andrey or Yury should take a look as well.

https://codereview.chromium.org/725983002/diff/40001/test/mjsunit/harmony/deb...
File test/mjsunit/harmony/debug-step-into-class-extends.js (right):

https://codereview.chromium.org/725983002/diff/40001/test/mjsunit/harmony/deb...
test/mjsunit/harmony/debug-step-into-class-extends.js:19: print(s);
stray print

https://codereview.chromium.org/725983002/diff/40001/test/mjsunit/harmony/deb...
File test/mjsunit/harmony/debug-step-into-constructor.js (right):

https://codereview.chromium.org/725983002/diff/40001/test/mjsunit/harmony/deb...
test/mjsunit/harmony/debug-step-into-constructor.js:41: var bp =
Debug.setBreakPoint(Base, 0);
You can just use 'debugger;' in the places where you need a break point

[arv (Not doing code reviews), 2014-11-14 17:24:00]

https://codereview.chromium.org/725983002/diff/40001/test/mjsunit/harmony/deb...
File test/mjsunit/harmony/debug-step-into-constructor.js (right):

https://codereview.chromium.org/725983002/diff/40001/test/mjsunit/harmony/deb...
test/mjsunit/harmony/debug-step-into-constructor.js:41: var bp =
Debug.setBreakPoint(Base, 0);
On 2014/11/14 17:19:15, Dmitry Lomov (chromium) wrote:
> You can just use 'debugger;' in the places where you need a break point 

True. In this specific case, I wanted to make sure setting the break point
worked.

In the other cases I could change to use a debugger statement.

[Dmitry Lomov (no reviews), 2014-11-14 17:30:59]

https://codereview.chromium.org/725983002/diff/40001/test/mjsunit/harmony/deb...
File test/mjsunit/harmony/debug-step-into-constructor.js (right):

https://codereview.chromium.org/725983002/diff/40001/test/mjsunit/harmony/deb...
test/mjsunit/harmony/debug-step-into-constructor.js:41: var bp =
Debug.setBreakPoint(Base, 0);
On 2014/11/14 17:24:00, arv wrote:
> On 2014/11/14 17:19:15, Dmitry Lomov (chromium) wrote:
> > You can just use 'debugger;' in the places where you need a break point 
> 
> True. In this specific case, I wanted to make sure setting the break point
> worked.
> 
> In the other cases I could change to use a debugger statement.

Ah right, gotcha. I'll leave it up to you.

[Dmitry Lomov (no reviews), 2014-11-14 17:32:15]

btw lgtm from my side, but please let Yury or Andrey to review as well.

[aandrey, 2014-11-14 19:27:13]

https://codereview.chromium.org/725983002/diff/40001/src/debug.cc
File src/debug.cc (right):

https://codereview.chromium.org/725983002/diff/40001/src/debug.cc#newcode1247
src/debug.cc:1247: if (function_proto->shared()->bound()) {
this if-else block is repeated several times below. maybe extract into a
function?

https://codereview.chromium.org/725983002/diff/40001/src/debug.cc#newcode1491
src/debug.cc:1491: if (js_function->shared()->bound()) {
repeated again

https://codereview.chromium.org/725983002/diff/40001/src/debug.cc#newcode1641
src/debug.cc:1641: if (function->shared()->bound()) {
repeated once more.

https://codereview.chromium.org/725983002/diff/40001/src/debug.cc#newcode1648
src/debug.cc:1648: if (function->shared()->code() ==
maybe the following apply & call checks should be there in the other places
also?

https://codereview.chromium.org/725983002/diff/40001/src/debug.cc#newcode1658
src/debug.cc:1658: if (!js_function->IsFromNativeScript()) {
BTW, is this in the correct order?

https://codereview.chromium.org/725983002/diff/40001/test/mjsunit/harmony/deb...
File test/mjsunit/harmony/debug-step-into-class-extends.js (right):

https://codereview.chromium.org/725983002/diff/40001/test/mjsunit/harmony/deb...
test/mjsunit/harmony/debug-step-into-class-extends.js:36: class Derived extends
GetBase() {} // 0.
should it be "GetBase" instead of "GetBase()" ?
otherwise, looks like we are just calling a function and use it's return value
as a base class.

[arv (Not doing code reviews), 2014-11-14 19:36:58]

Thanks. I'll extract the code into a shared function and see if we should apply
the apply/call logic there.

https://codereview.chromium.org/725983002/diff/40001/src/debug.cc
File src/debug.cc (right):

https://codereview.chromium.org/725983002/diff/40001/src/debug.cc#newcode1658
src/debug.cc:1658: if (!js_function->IsFromNativeScript()) {
On 2014/11/14 19:27:13, aandrey wrote:
> BTW, is this in the correct order?

Yeah. This looks wrong.

https://codereview.chromium.org/725983002/diff/40001/test/mjsunit/harmony/deb...
File test/mjsunit/harmony/debug-step-into-class-extends.js (right):

https://codereview.chromium.org/725983002/diff/40001/test/mjsunit/harmony/deb...
test/mjsunit/harmony/debug-step-into-class-extends.js:19: print(s);
On 2014/11/14 17:19:15, Dmitry Lomov (chromium) wrote:
> stray print

I wanted it but I'll remove it.

https://codereview.chromium.org/725983002/diff/40001/test/mjsunit/harmony/deb...
test/mjsunit/harmony/debug-step-into-class-extends.js:36: class Derived extends
GetBase() {} // 0.
On 2014/11/14 19:27:13, aandrey wrote:
> should it be "GetBase" instead of "GetBase()" ?
> otherwise, looks like we are just calling a function and use it's return value
> as a base class.

It should be GetBase(). We are testing stepping into this function call.

[arv (Not doing code reviews), 2014-11-14 22:02:11]

Refactor to share code

[arv (Not doing code reviews), 2014-11-14 22:02:22]

PTAL

https://codereview.chromium.org/725983002/diff/40001/src/debug.cc
File src/debug.cc (right):

https://codereview.chromium.org/725983002/diff/40001/src/debug.cc#newcode1247
src/debug.cc:1247: if (function_proto->shared()->bound()) {
On 2014/11/14 19:27:13, aandrey wrote:
> this if-else block is repeated several times below. maybe extract into a
> function?

Done.

https://codereview.chromium.org/725983002/diff/40001/src/debug.cc#newcode1491
src/debug.cc:1491: if (js_function->shared()->bound()) {
On 2014/11/14 19:27:13, aandrey wrote:
> repeated again

Done.

https://codereview.chromium.org/725983002/diff/40001/src/debug.cc#newcode1641
src/debug.cc:1641: if (function->shared()->bound()) {
On 2014/11/14 19:27:13, aandrey wrote:
> repeated once more.

Done.

https://codereview.chromium.org/725983002/diff/40001/src/debug.cc#newcode1648
src/debug.cc:1648: if (function->shared()->code() ==
On 2014/11/14 19:27:13, aandrey wrote:
> maybe the following apply & call checks should be there in the other places
> also?

This is the only place where we have the holder.

https://codereview.chromium.org/725983002/diff/40001/src/debug.cc#newcode1658
src/debug.cc:1658: if (!js_function->IsFromNativeScript()) {
On 2014/11/14 19:36:57, arv wrote:
> On 2014/11/14 19:27:13, aandrey wrote:
> > BTW, is this in the correct order?
> 
> Yeah. This looks wrong.

Done.

https://codereview.chromium.org/725983002/diff/40001/test/mjsunit/harmony/deb...
File test/mjsunit/harmony/debug-step-into-class-extends.js (right):

https://codereview.chromium.org/725983002/diff/40001/test/mjsunit/harmony/deb...
test/mjsunit/harmony/debug-step-into-class-extends.js:19: print(s);
On 2014/11/14 19:36:58, arv wrote:
> On 2014/11/14 17:19:15, Dmitry Lomov (chromium) wrote:
> > stray print
> 
> I wanted it but I'll remove it.

Done.

[aandrey, 2014-11-15 08:57:16]

lgtm

[arv (Not doing code reviews), 2014-11-15 19:19:54]

The CQ bit was checked by arv@chromium.org

[commit-bot: I haz the power, 2014-11-15 19:20:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/725983002/60001

[commit-bot: I haz the power, 2014-11-15 19:48:42]

Committed patchset #3 (id:60001)