Closed Bug 471531 Opened 16 years ago Closed 16 years ago

js1_7/decompilation/regress-379925.js | js1_8_1/decompilation/regress-371802.js FAIL

Categories

(Core :: JavaScript Engine, defect, P1)

Other Branch
defect

Tracking

()

VERIFIED FIXED
mozilla1.9.1b3

People

(Reporter: bc, Assigned: brendan)

References

Details

(Keywords: regression, testcase, verified1.9.1, Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 1 obsolete file)

bug 470374 regressed this on tracemonkey.

js1_7/decompilation/regress-379925.js

Expected value ' function ( ) { var [ x , x ] = [ , , ] ; for ( [ x , x ] in [ ] ) { } } ', 
Actual value ' function ( ) { x = var ; x = ; for ( [ x , x ] in [ ] ) { } } '
compile actual reason: Expected value 'No Error', Actual value 'SyntaxError: syntax error'

js1_8_1/decompilation/regress-371802.js

Expected value ' function ( a , b , n ) { for ( var [ i , j ] = [ a , b ] ; i < n ; [ i , j ] = [ a , b ] ) { print ( i ) ; } } ', 
Actual value ' function ( a , b , n ) { i = var a ; j = b ; for ( ; i < n ; i = a ; j = b ; a ) { print ( i ) ; } } '
Expected value 'No Error', Actual value 'SyntaxError: syntax error'
Flags: in-testsuite+
Flags: in-litmus-
Assignee: general → brendan
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.9.1b3
Urgh, thought I had run the js1_7 tests with and without that patch -- had the wrong patch pushed. Sorry, fixing today in tm.

/be
Hope to land this in tm today.

/be
Attachment #354903 - Flags: review?(jorendorff)
Testsuite says this patch fixes these bugs:

js1_5/Regress/regress-169559.js
js1_7/decompilation/regress-379925.js
js1_7/extensions/regress-346642-06.js
js1_8_1/decompilation/regress-346642-01.js

The first is the old "STATUS: Global vars should not be more than 2.5 times slower than local" chestnut, but I can reproducibly fail it without the patch for this bug, pass it with. Hrm.

Second and third are of course this bug.

Fourth is an assertbotch:

Assertion failure: strcmp(rval, forelem_cookie) == 0, at ../jsopcode.cpp:3347


so also this bug. Bob, you concur?

/be
re: js1_5/Regress/regress-169559.js, I've been having problems with this and some of the BigQ tests that I haven't resolved yet. I don't see this on my tracemonkey builds for the last couple of days however with or without the patch.

testing on mac: js1_7/decompilation/regres-379925.js and js1_8_1/decompilation/regress-371802.js both pass with this patch and the assert in js1_7/extensions/regress-346642-06.js and js1_8_1/decompilation/regress-346642-01.js is also fixed by this patch.

/me concurs.
Comment on attachment 354903 [details] [diff] [review]
fix boundary condition on group assignment vs. faulting getlocal decomp

> #if JS_HAS_DESTRUCTURING
>-                if (sn && SN_TYPE(sn) == SRC_GROUPASSIGN && len > JSOP_GETLOCAL_LENGTH) {
>+                if (sn &&
>+                    SN_TYPE(sn) == SRC_GROUPASSIGN &&
>+                    (len > JSOP_GETLOCAL_LENGTH || pc > startpc)) {

I don't see how `len > JSOP_GETLOCAL_LENGTH` can ever be true here.  I think the change you want is just to change `len` to `endpc - startpc` on that line.  Your patch works as well, but I think the `len` comparison is nonsense.  Sorry I missed this in the previous review.

There's something else confusing here:

js> function f(x) {var [[a,b]]=[x];}
js> f(null)
typein:17: TypeError: var x is null

Note the "var" in the error message.  This is somehow caused by the source note.  Can you fix that too?
Attachment #354903 - Flags: review?(jorendorff)
Great review comments, endpc - pc is what I mistook len for, and that var regression in the last patch is fixed by this patch.

/be
Attachment #354903 - Attachment is obsolete: true
Attachment #354973 - Flags: review?(jorendorff)
Comment on attachment 354973 [details] [diff] [review]
better fix, thanks

Much better!
Attachment #354973 - Flags: review?(jorendorff) → review+
Fixed in tm:

http://hg.mozilla.org/tracemonkey/rev/34f4df9c4f9c

This fix should go into m-c for sure, and into 1.9.1 too IMHO.

/be
Flags: wanted1.9.1?
Whiteboard: fixed-in-tracemonkey
Flags: wanted1.9.1? → wanted1.9.1+
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Attachment #354973 - Flags: approval1.9.1?
Attachment #354973 - Flags: approval1.9.1? → approval1.9.1+
v 1.9.1, 1.9.2
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: