614752 - "Assertion failure: simsp <= i.sp()" with apply

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

Attachment: stack trace

./js -m
js> Object.create(function(){}).apply(null, null);

Assertion failure: simsp <= i.sp(), at js/src/jsfun.cpp:3061

The first bad revision is:
changeset:   d9aceaabef28
user:        Luke Wagner
date:        Thu Oct 21 11:33:22 2010 -0700
summary:     Bug 605192 - JM: make f.apply(x, obj) fast, part 4 (r=dvander)

Comment 1

[Luke Wagner [:luke]]

The assertion is overly strict; a weaker one that should hold is:
  JS_ASSERT_IF(vp < simsp, vp < i.sp());
The reason that vp < i.sp() is that f.apply(x, null) turns into x.f() which ic::SplatApplyArgs achieves by popping the top 'null' before calling (with f as callee and x as this).  This puts sp below what js_ReconstructStackDepth would expect.  If js_fun_apply had the same in-place call optimization (instead of copying everything above sp), it would hit the same assert.

Comment 2

[Luke Wagner [:luke]]

Attachment: weaken assert

Comment 3

[Nicholas Nethercote [inactive]]

(In reply to comment #1)
> The assertion is overly strict; a weaker one that should hold is:
>   JS_ASSERT_IF(vp < simsp, vp < i.sp());
> The reason that vp < i.sp() is that f.apply(x, null) turns into x.f() which
> ic::SplatApplyArgs achieves by popping the top 'null' before calling (with f as
> callee and x as this).  This puts sp below what js_ReconstructStackDepth would
> expect.  If js_fun_apply had the same in-place call optimization (instead of
> copying everything above sp), it would hit the same assert.

Is it worth putting this explanation, or part of it, above the assertion?

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 493208 [details] [diff] [review]
weaken assert

What njn said -- comment please, and weaker assertion does not seem better if we don't need it yet. Let someone botch it, read the comment, then weaken. Or am I missing something in favor of weakening now?

/be

Comment 5

[Luke Wagner [:luke]]

Well, if we don't weaken now, it will continue to assert...

Comment 6

[Luke Wagner [:luke]]

(in the testcase in comment 0)

Comment 7

[Brendan Eich [:brendan]]

Oh, ok -- weaken now, r=me.

/be

Comment 8

[Luke Wagner [:luke]]

Comment on attachment 493208 [details] [diff] [review]
weaken assert

For the record then?

Comment 9

[Luke Wagner [:luke]]

Attachment: patch v.2

I thought about it again and if, in general, the actual stack can be greater or less than the simulated stack, we just need to check for inclusion in the range
  [base, min(actual,simulated))
This patch does that and comments why.

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 497602 [details] [diff] [review]
patch v.2

Great -- but where's the comment?

/be

Comment 11

[Luke Wagner [:luke]]

Attachment: patch v.2 (this time hg qref)

Oops, I post patches out of .hg/patches and I forgot to hg qref.

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 497628 [details] [diff] [review]
patch v.2 (this time hg qref)

>     /*
>      * We try to the print the code that produced vp if vp is a value in the
>      * most recent interpreted stack frame. Note that additional values, not
>      * directly produced by the script, may have been pushed onto the frame's
>      * expression stack (e.g. by pushInvokeArgs) thereby incrementing sp past
>-     * the depth simulated by ReconstructPCStack. Since we must pass an offset
>-     * from the top of the simulated stack to js_ReportValueError3, it is
>-     * important to do bounds checking using the simulated, rather than actual,
>-     * stack depth.
>+     * the depth simulated by ReconstructPCStack. Conversely, values may have

New para before Conversely for better wrapping?

>+     * been popped from the stack in preparation for a call (e.g., by
>+     * SplatApplyArgs). Since we must pass an offset from the top of the
>+     * simulated stack to js_ReportValueError3, it is important to do bounds

Why switch to passive voice? "Since we must ..., we check bounds using ...".

r=me with these picked. Thanks,

/be

Comment 13

[Luke Wagner [:luke]]

Sounds good
http://hg.mozilla.org/tracemonkey/rev/6b7627584c94

Comment 14

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/6b7627584c94

Comment 15

[Christian Holler (:decoder)]

A testcase for this bug was automatically identified at js/src/jit-test/tests/basic/testBug614752.js.