Raise StackOverflow during bootstrapping

Raise StackOverflow during bootstrapping

See https://github.com/joyent/node/issues/7120

R=jarin@chromium.org
BUG=

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

[Jarin, 2014-02-25 20:45:07]

Thanks for the effort to fix this. I think we will need this covered by a test -
could you write a test that would fail without your patch? The test should also
check that the desired exception is thrown on overflow.

It seems that 'git diff' is confused by your bootstrapper.cc change - it reports
that all lines in the file have been changed. I am not quite sure what happened
there - perhaps the line endings do not match?

https://codereview.chromium.org/178073002/diff/1/src/bootstrapper.cc
File src/bootstrapper.cc (right):

https://codereview.chromium.org/178073002/diff/1/src/bootstrapper.cc#newcode1480
src/bootstrapper.cc:1480: if (check.WillOverflow(5000 * sizeof(intptr_t))) {
Could you possibly create some constant with some descriptive name for the
buffer space?

https://codereview.chromium.org/178073002/diff/1/src/bootstrapper.cc#newcode2615
src/bootstrapper.cc:2615: if (check.WillOverflow(6000 * sizeof(intptr_t))) {
Is there a reason why the magic constant differs from the one above?

[alexis, 2014-03-05 23:07:23]

Thanks Jarin.

> Thanks for the effort to fix this. I think we will need this covered by a test
-
> could you write a test that would fail without your patch? The test should
also
> check that the desired exception is thrown on overflow.
I added a test that passes with my fix and fails without it when snapshots are
disabled. Is there a ways to make this test run with snapshots disabled?

> It seems that 'git diff' is confused by your bootstrapper.cc change - it
reports
> that all lines in the file have been changed. I am not quite sure what
happened
> there - perhaps the line endings do not match?
Yeah, not sure what happened, but it seems fixed now.

> 
> https://codereview.chromium.org/178073002/diff/1/src/bootstrapper.cc
> File src/bootstrapper.cc (right):
> 
>
https://codereview.chromium.org/178073002/diff/1/src/bootstrapper.cc#newcode1480
> src/bootstrapper.cc:1480: if (check.WillOverflow(5000 * sizeof(intptr_t))) {
> Could you possibly create some constant with some descriptive name for the
> buffer space?
Done.

>
https://codereview.chromium.org/178073002/diff/1/src/bootstrapper.cc#newcode2615
> src/bootstrapper.cc:2615: if (check.WillOverflow(6000 * sizeof(intptr_t))) {
> Is there a reason why the magic constant differs from the one above?
These were empirical measures of how much stack is needed to get through
genesis. It turns out that the check in CompileNative is not useful because a)
we have already entered the context and b) it's covered by the other check, so I
remove it.

[Jarin, 2014-03-13 13:40:46]

lgtm

[Jarin, 2014-03-13 15:26:01]

Hmm, let me take this back - the overflow check fails on our test suite on
64-bit architectures, where some of the tests (e.g.
test/mjsunit/regress/regress-1132.js) explicitly set small stack size (and still
crash!).

[alexis, 2014-03-13 19:25:50]

On which platform do these tests fail?

[Jarin, 2014-03-14 07:30:02]

On 2014/03/13 19:25:50, alexis wrote:
> On which platform do these tests fail?

Looking more closely at the log, only variants of the following two tests
failed:

x64 optdebug and release fail on test test/mjsunit/regress/regress-1132.js
arm optdebug fails on your new test (cctest
test-api/APIStackOverflowNestedContexts)

(Simply running "make quickcheck" should produce the failures)

[alexis, 2014-03-14 17:46:15]

> x64 optdebug and release fail on test test/mjsunit/regress/regress-1132.js
A tweak in the reserve size was needed to make this test pass. Also, I added a
check to avoid rasing the exception if this is the first context being created.

> arm optdebug fails on your new test (cctest
> test-api/APIStackOverflowNestedContexts)
Is it possible to reproduce a cctest failure without having access to native arm
hardware?

[Jarin, 2014-03-15 18:20:33]

On 2014/03/14 17:46:15, alexis wrote:
> > x64 optdebug and release fail on test test/mjsunit/regress/regress-1132.js
> A tweak in the reserve size was needed to make this test pass. Also, I added a
> check to avoid rasing the exception if this is the first context being
created.
> 
> > arm optdebug fails on your new test (cctest
> > test-api/APIStackOverflowNestedContexts)
> Is it possible to reproduce a cctest failure without having access to native
arm
> hardware?

On my machine, "make quickcheck" reproduces the failure. You can run ARM tests
after compiling with "make arm.optdebug" - that will run ARM code in the
built-in simulator.

[alexis, 2014-03-17 18:05:25]

Both issues should be fixed now. Checking the exception string on the top call
was causing us to re-enter JS and hit another stack overflow, at least on ARM. I
made it so that the check is only on the bottom call.

[alexis, 2014-03-17 18:05:27]

Both issues should be fixed now. Checking the exception string on the top call
was causing us to re-enter JS and hit another stack overflow, at least on ARM. I
made it so that the check is only on the bottom call.

[alexis, 2014-03-25 12:18:39]

Bump. Looking forward to your feedback... thanks!

[Jarin, 2014-03-25 15:28:33]

Looks good, thanks.

https://codereview.chromium.org/178073002/diff/100001/test/cctest/test-api.cc
File test/cctest/test-api.cc (right):

https://codereview.chromium.org/178073002/diff/100001/test/cctest/test-api.cc...
test/cctest/test-api.cc:4989: void APIStackOverflowNestedContextsHelper(bool
is_bottom_call);
In v8, bools as arguments are normally frowned upon, enums are preferred.
However, since this is a test and the function decl, def and use are close to
each other, I think it is fine to leave as is.

[alexis, 2014-03-27 14:11:59]

Great, thanks! Let me know if you need anything else from me...

[Jarin, 2014-03-28 13:26:39]

Committed patchset #4 manually as r20335 (presubmit successful).

[alexis, 2014-03-28 14:45:04]

Thanks! We would like to include this fix in node.js v0.12, so we were wondering
if you could backport it to 3.24.

[Jarin, 2014-03-28 14:51:16]

On 2014/03/28 14:45:04, alexis wrote:
> Thanks! We would like to include this fix in node.js v0.12, so we were
wondering
> if you could backport it to 3.24.

Unfortunately, the test crashes when running without snapshot (--nosnapshot), so
we had to revert the change.