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

RESOLVED FIXED

Status

()

--
critical
RESOLVED FIXED
8 years ago
6 years ago

People

(Reporter: jruderman, Assigned: luke)

Tracking

(Blocks: 1 bug, {assertion, regression, testcase})

Trunk
x86
Mac OS X
assertion, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

8 years ago
Created attachment 493199 [details]
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)
(Assignee)

Comment 1

8 years ago
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.
(Assignee)

Comment 2

8 years ago
Created attachment 493208 [details] [diff] [review]
weaken assert
Attachment #493208 - Flags: review?(brendan)
(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 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
Attachment #493208 - Flags: review?(brendan) → review-
(Assignee)

Comment 5

8 years ago
Well, if we don't weaken now, it will continue to assert...
(Assignee)

Comment 6

8 years ago
(in the testcase in comment 0)
Oh, ok -- weaken now, r=me.

/be
(Assignee)

Comment 8

8 years ago
Comment on attachment 493208 [details] [diff] [review]
weaken assert

For the record then?
Attachment #493208 - Flags: review- → review?(brendan)
Attachment #493208 - Flags: review?(brendan) → review+
(Assignee)

Comment 9

8 years ago
Created attachment 497602 [details] [diff] [review]
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.
Assignee: general → lw
Attachment #493208 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #497602 - Flags: review?(brendan)
Comment on attachment 497602 [details] [diff] [review]
patch v.2

Great -- but where's the comment?

/be
Attachment #497602 - Flags: review?(brendan) → review+
(Assignee)

Comment 11

8 years ago
Created attachment 497628 [details] [diff] [review]
patch v.2 (this time hg qref)

Oops, I post patches out of .hg/patches and I forgot to hg qref.
Attachment #497602 - Attachment is obsolete: true
Attachment #497628 - Flags: review?(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
Attachment #497628 - Flags: review?(brendan) → review+
(Assignee)

Comment 13

8 years ago
Sounds good
http://hg.mozilla.org/tracemonkey/rev/6b7627584c94
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/6b7627584c94
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
A testcase for this bug was automatically identified at js/src/jit-test/tests/basic/testBug614752.js.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.