428706 - "Assertion failure: regs.sp < vp"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

js> [1 for ([,,] in [])]
Assertion failure: regs.sp < vp, at jsinterp.c:6489

This assertion was added in bug 389605.

Comment 1

[Igor Bukanov]

Attachment: v1

In the patch for bug 389605 I missed that the list comprehension also allows derestructuring pattern for the for loop. This patch fixes this by moving the call to EnsureNonEmptyLet into DestructuringExpr.

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 315312 [details] [diff] [review]
v1

I missed that too, alas. Good to fix this quickly, thanks.

/be

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 315312 [details] [diff] [review]
v1

a1.9=beltzner

Comment 4

[Igor Bukanov]

Attachment: alternative fix

This an alternative fix that tries to minimize the amount of code to support "let" with empty destructuring patterns. The idea is to add the synthetic empty property right after [] or {} even if the "let" block or expression would have subsequent non-empty declarations. 

This is suboptimal from the performance point of view, but simplifies code and makes sure that no destructuring patterns are left unchecked.

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 315514 [details] [diff] [review]
alternative fix

>+     * would violate this assumption as the there would be no let locals to
>+     * store on the stack. To satisfy it we add an empty property to such
>+     * blocks so OBJ_BLOCK_COUNT(cx, blockObj), that gives the number of
>+     * slots, would be always positive.

Suggest "so that OBJ_BLOCK_COUNT(cx, blockObj), which gives ...".

>+     *
>+     * Note that we add such property even if there would be locals after the

Suggest consistent subjunctive or none at all, latter is best: "Note that we add such a property even if the block has locals due to later let declarations in it." Or something like that (missing "a" added before "property" too).

/be

Comment 6

[Igor Bukanov]

Attachment: alternative fix v1b

The new version of the patch addresses the nits from the comment 5.

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 315942 [details] [diff] [review]
alternative fix v1b

a1.9=beltzner

Comment 8

[Igor Bukanov]

I checked in the patch from the comment 6 to the trunk:

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1208503764&maxdate=1208503862&who=igor%25mir2.org

Comment 9

[Igor Bukanov]

I backed out the checking to investigate startup segfaults.

Comment 10

[Igor Bukanov]

Attachment: v2

Unfortunately I have managed to skip substantial testing of the patch. Running full js shell test suite or mochi test against the code that includes the patch would reveal the regression: I missed that data in CheckDestructuring can be null.

But the patch also missed to bump xdr count as, however tiny, it still introduces the bytecode incompatibility. Another bug was that it would not bump BindData.u.let.index when adding the synthetic property. Yet another thing was that the patch used "JSPROP_READONLY | JSPROP_PERMANENT", not "JSPROP_ENUMERATE | JSPROP_PERMANENT", as attributes for the property. Although harmless, after the xdr was not aware of it and would restore the synthetic property as "JSPROP_ENUMERATE | JSPROP_PERMANENT".

The new version fixes these and makes sure that the synthetic property behaves in the exactly the same was as normal let names.

Comment 11

[Igor Bukanov]

I will ask for an review after testing the patch.

Comment 12

[Igor Bukanov]

Attachment: v2b

The patch now passes shell suite and mochi tests.

Compared with v2 the new version bumps the XDR version by 1 to 24, not 25.

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 316559 [details] [diff] [review]
v2b

r=me, straightforward even if it looks big-ish.

/be

Comment 14

[Damon Sicore (:damons)]

Comment on attachment 316559 [details] [diff] [review]
v2b

a1.9+=damons

Comment 15

[Igor Bukanov]

I checked in the patch from the comment 12 to the trunk:

http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&branch=HEAD&cvsroot=%2Fcvsroot&date=explicit&mindate=1209144309&maxdate=1209144484&who=igor%25mir2.org

Comment 16

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_7/regress/regress-428706.js,v  <--  regress-428706.js
initial revision: 1.1

Comment 17

[Bob Clary [:bc] (inactive)]

v 1.9.0