Closed Bug 443074 Opened 12 years ago Closed 12 years ago

Incorrect decompilation (missing parens) with genexp in for-loop-condition

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9.1

People

(Reporter: jruderman, Assigned: brendan)

References

Details

(Keywords: regression, testcase)

Attachments

(2 files, 3 obsolete files)

js> f = function () { for(; x || (1 for each (y in [])); ) { } }
function () {
    for (; x || 1 for each (y in []);) {
    }
}

js> eval("(" + f + ")") 
typein:3: SyntaxError: missing ; after for-loop condition:
typein:3:     for (; x || 1 for each (y in []);) {
typein:3: ..................^

Regression from bug 441477 or its followup bug 442342, perhaps?
One of those bugs, yeah.

/be
Assignee: general → brendan
OS: Mac OS X → All
Priority: -- → P1
Hardware: PC → All
Target Milestone: --- → mozilla1.9.1
Locally reverted the patch for bug 442342, testcase still fails -- so this bug is from the patch for bug 441477.

/be
Status: NEW → ASSIGNED
This is really bug 443012, which has many forms. Jason was right to point to the lack of generality in the patch for bug 442342, which lack differently afflicted the patch for bug 441477 too. Marking dependent rather than DUP, will re-test and close when fixed.

/be
Depends on: 443012
Attached patch constant condition folding (obsolete) — Splinter Review
Kind of a tour-de-force but not ultimately complete -- we do not fold boolean expressions other than || and && whose operands are Boolish. But we do know that a genexp in condition context is truthy.

/be
Attachment #343013 - Flags: review?(jorendorff)
Attached patch fix last-minute thinko (obsolete) — Splinter Review
Attachment #343013 - Attachment is obsolete: true
Attachment #343014 - Flags: review?(jorendorff)
Attachment #343013 - Flags: review?(jorendorff)
Attached patch passing testsuite now (obsolete) — Splinter Review
With only some false-positive additions due to decompilation shifting. See next attachment.

/be
Attachment #343014 - Attachment is obsolete: true
Attachment #343016 - Flags: review?(jorendorff)
Attachment #343014 - Flags: review?(jorendorff)
Bob, I see two JIT-related crashers, not due to this patch:

js1_6/extensions/regress-456826.js
js1_7/regress/regress-452703.js

Can you confirm and file if needed? Thanks,

/be
Attachment #343017 - Flags: review?
Attachment #343017 - Flags: review? → review?(bclary)
js1_5/extensions/regress-421621.js, js1_5/extensions/regress-432075.js are both listed in spidermonkey-n-1.9.1.tests.

I'll look into the other changes/crashes.
for js1_6/extensions/regress-456826.js, I don't see a crash but see bug 456826 comment 36 for weirdness.
Comment on attachment 343017 [details]
results for tests that need adjusting to match new decompilations

not sure how to mark a review request for results...

Anyway, dealing with this change for all branches in the same test looks to be a pita without a way to determine the branch from inside the script. I think I would prefer to just add these tests to spidermonkey-n-1.9.1.tests and duplicate/modify them into js1_8_1/ for the 1.9.1 branch and later. Ok?
Attachment #343017 - Flags: review?(bclary)
(In reply to comment #10)
> for js1_6/extensions/regress-456826.js, I don't see a crash but see bug 456826
> comment 36 for weirdness.

Yes, sorry -- that's what I saw too.

Adding to -n*.tests sounds fine for now. Do I have to list that in the -L args by hand, or is it included somehow via the trunk jsDriver.pl?

/be
by hand.
Comment on attachment 343016 [details] [diff] [review]
passing testsuite now

>@@ -2073,35 +2073,32 @@ CheckSideEffects(JSContext *cx, JSCodeGe
>       case PN_LIST:
>+        if (pn->pn_op != JSOP_NOP &&
>+            !(pn->pn_op == JSOP_OR || pn->pn_op == JSOP_AND ||
>+              pn->pn_op == JSOP_STRICTEQ || pn->pn_op == JSOP_STRICTNE)) {

I think this would read better if the "if" and "else" branches were switched:

  if (op == JSOP_NOP || op == JSOP_OR || op == JSOP_AND ...) {
     /* ...short comment... */
  } else {
     /* ...long comment... */
     *vp = JS_TRUE;
  }

I looked at all the stuff that falls under arity==PN_LIST && op==JSOP_NOP.  Stuff like comma-expressions and blocks, fine.  There are also a lot of cases like variable lists that will be auto-disqualified by other nodes up or down the tree, which is fine.

Initializers can be effectful if they trigger setters on Array.prototype or Object.prototype. :-P

>-                    /*
>-                     * A generator expression as loop condition is useless.
>-                     * It won't be called, and as an object it evaluates to
>-                     * true in boolean contexts without any conversion hook
>-                     * being called.
>-                     *
>-                     * This useless condition elimination is mandatory, to
>-                     * help the decompiler. See bug 442342.
>-                     */

Boolish should inherit this comment.

Everything else looks fine.

r=me with initializers fixed and preferably with the comment.
Attachment #343016 - Flags: review?(jorendorff) → review+
(In reply to comment #14)
> (From update of attachment 343016 [details] [diff] [review])
> >@@ -2073,35 +2073,32 @@ CheckSideEffects(JSContext *cx, JSCodeGe
> >       case PN_LIST:
> >+        if (pn->pn_op != JSOP_NOP &&
> >+            !(pn->pn_op == JSOP_OR || pn->pn_op == JSOP_AND ||
> >+              pn->pn_op == JSOP_STRICTEQ || pn->pn_op == JSOP_STRICTNE)) {
> 
> I think this would read better if the "if" and "else" branches were switched:
> 
>   if (op == JSOP_NOP || op == JSOP_OR || op == JSOP_AND ...) {
>      /* ...short comment... */
>   } else {
>      /* ...long comment... */
>      *vp = JS_TRUE;
>   }

Thanks, I was over-minimizing change -- thought about applying De Morgan and held back, but you're right, it's worth it.

> Initializers can be effectful if they trigger setters on Array.prototype or
> Object.prototype. :-P

Doesn't that suck! Ok, I will crud up the logic (since these have pn_op of JSOP_NOP -- perhaps I can use JSOP_NEWINIT).

New patch in a bit, I'll stamp r+ for you. Thanks again,

/be
Attachment #343016 - Attachment is obsolete: true
Attachment #343128 - Flags: review+
We should sync tm and m-c today, and every day where we can (m-c open and good state, tm good state and never worse perf).

/be
Depends on: 459990
Depends on: 460043
Depends on: 460158
/cvsroot/mozilla/js/tests/js1_8/decompilation/regress-443074.js,v  <--  regress-443074.js
initial revision: 1.1

Checking in js1_8_1/decompilation/regress-443074.js;
/cvsroot/mozilla/js/tests/js1_8_1/decompilation/regress-443074.js,v  <--  regress-443074.js
initial revision: 1.1

http://hg.mozilla.org/mozilla-central/rev/6e5c848a2183
Flags: in-testsuite+
Flags: in-litmus-
Depends on: 460870
The following tests need to be modified obsoleted on 1.9.1 and new versions added to the js1_8_1 suite:

js1_7/decompilation/regress-351070-01.js
js1_7/decompilation/regress-443074.js
js1_8/genexps/regress-380237-03.js
js1_8/genexps/regress-380237-04.js
Flags: in-testsuite+ → in-testsuite?
on tracemonkey:

js1_8/genexps/regress-380237-03.js

I assume this is expected:

Expected value ' function ( ) { ( 1 for ( w in [ ] ) if ( 0 ) ) ; } ', Actual value ' function ( ) { ( 1 for ( w in [ ] ) if ( false ) ) ; }

but is this?

Expected value ' function ( ) { if ( 1 , ( x for ( x in [ ] ) ) ) { } } ', Actual value ' function ( ) { if ( ( 1 , ( x for ( x in [ ] ) ) ) ) { } } '

Also, js1_8/genexps/regress-380237-04.js

Is this expected?

 FAILED! Generator expressions parenthesization test: overParenTest genexp snugly in parentheses: function anonymous() {
 FAILED! } : Expected value 'false', Actual value 'true' 
 FAILED! Generator expressions parenthesization test: overParenTest genexp snugly in parentheses: function anonymous() {
 FAILED!     do {
 FAILED!     } while (true);
 FAILED! } : Expected value 'false', Actual value 'true' 
 FAILED! Generator expressions parenthesization test: overParenTest genexp snugly in parentheses: function anonymous() {
 FAILED!     while (true) {
 FAILED!     }
 FAILED! } : Expected value 'false', Actual value 'true' 
 FAILED! Generator expressions parenthesization test: overParenTest genexp snugly in parentheses: function anonymous() {
 FAILED! } : Expected value 'false', Actual value 'true'
> I assume this is expected:

Yes.

> but is this?

It's bug 461111.

> Is this expected?

Yes.
We've merged since this landed on tm and I show it fixed.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
this was landed http://hg.mozilla.org/mozilla-central/rev/6e5c848a2183 and cvs on  2008/10/20 15:46:08
Flags: in-testsuite? → in-testsuite+
 FAILED! [reported from test()] Decompilation of generator expressions : Expected value ' function ( ) { ( [ yield ] for ( x in [ ] ) ) ; } ', Actual value ' function ( ) { ( [ ( yield ) ] for ( x in [ ] ) ) ; } ' 

This overparenthesized yield bugs me (found it by testing using `jsDriver.pl ... -l js1_8/genexps` which does not consult the -n exclusion files). Is it hard to fix?

/be
Spin-out bug wanted if people agree, or just cuz it bugs me ;-).

/be
You need to log in before you can comment on or make changes to this bug.