394673 - js1_5/Regress/regress-89443.js - InternalError: too much recursion (mac debug only)

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bob Clary [:bc] (inactive)]

On mac powerpc, recently I switched from using gcc 3.3 on the trunk with an older XTools to using gcc 4.0.1 with the most recent Xtools. It appears that this change has resulted in js1_5/Regress/regress-89443.js failing in debug shell and browser on mac powerpc with an "InternalError: too much recursion". My mac i386 boxes are offline and I can't test them to see if this is reproducible there.

Comment 1

[Igor Bukanov]

If you pass explicit -S option, at which value it will stop producing the error? It seems that the default value of 500000 is too low on PowerPC. 

For references on my Fedora-7 laptop:

~/m/trunk/mozilla/js/tests $ ./../src/Linux_All_DBG.OBJ/js -S 200000 -f ./shell.js -f ./js1_5/shell.js -f ./js1_5/Regress/shell.js -f ./js1_5/Regress/regress-89443.js -f ./js-test-driver-end.js
InternalError: too much recursion
~/m/trunk/mozilla/js/tests $ ./../src/Linux_All_DBG.OBJ/js -S 220000 -f ./shell.js -f ./js1_5/shell.js -f ./js1_5/Regress/shell.js -f ./js1_5/Regress/regress-89443.js -f ./js-test-driver-end.js
BUGNUMBER: 89443
STATUS: Testing this script will compile without stack overflow
PASSED! 

jstest: js1_5/Regress/regress-89443.js bug:  result: PASSED type: shell description:  expected: No Crash actual: No Crash reason: 

Comment 2

[Igor Bukanov]

Attachment: shell session to show effect of -S

Here is the shell test session from comment 1 with long lines preserved.

Comment 3

[Bob Clary [:bc] (inactive)]

(In reply to comment #1)
> If you pass explicit -S option, at which value it will stop producing the
> error? It seems that the default value of 500000 is too low on PowerPC. 

562277 passes. 562276 fails.

Comment 4

[Igor Bukanov]

(In reply to comment #3)
> (In reply to comment #1)
> > If you pass explicit -S option, at which value it will stop producing the
> > error? It seems that the default value of 500000 is too low on PowerPC. 
> 
> 562277 passes. 562276 fails.
> 

Is this 64 bit power pc?

Comment 5

[Bob Clary [:bc] (inactive)]

(In reply to comment #4)

> 
> Is this 64 bit power pc?
> 

I seriously doubt it: Mac mini, Powermac10,2, PowerPC G4(1.2)

Comment 6

[Igor Bukanov]

One way to fix this is to increase the default values for the native stack limit from 500K to a bigger value on debug builds for Power PC. Another one is to try to tune build options on Power PC so the compiler would use more compact layout. Yet the third one is to avoid using recursion when parsing the test case. I will try the last one first. 

Comment 7

[Igor Bukanov]

Attachment: test case with better coverage

The new test case provide better coverage for testing behavior with long chains of "&&" or "||". Since the current code uses recursion during its parsing or when folding constants, it exists with too much recursion errors. It also tests the emitter when it has to generate jumps over more then 64K of the bytecode.

Comment 8

[Igor Bukanov]

Attachment: fix v1

this is backup of something that passes the test case from comment 7.

Comment 9

[Igor Bukanov]

Comment on attachment 281315 [details] [diff] [review]
fix v1

To avoid C recursion both during the parsing and folding of constants when dealing with a long chain of "||" or "&&", the patch adds JOF_LEFTASSOC flag to both JSOP_AND and JSOP_OR, changes the parser to treat "&&" and "||" in the same way as it is done with other left-associative binaries, and teaches the emitter how to deal with PN_LIST associativity in JSOP_AND and JSOP_OR.

The patch passes the test suite, but it surprised me that using JOF_LEFTASSOC was enough to solve the problem especially given that the emitter even before the patch avoided the recursion when generating the code for a1||a2||...||aN.

Comment 10

[Brendan Eich [:brendan]]

(In reply to comment #9)
> The patch passes the test suite, but it surprised me that using JOF_LEFTASSOC
> was enough to solve the problem especially given that the emitter even before
> the patch avoided the recursion when generating the code for a1||a2||...||aN.

JOF_LEFTASSOC is used only by jsparse.c:NewBinary to build lists instead of left-heavy trees. The parser build right-associated trees for && and || before your patch, so just using JOF_LEFTASSOC was indeed not enough: you had to change the parser to match. r/a+ in a trice.

/be

Comment 11

[Igor Bukanov]

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

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

Comment 12

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/js1_5/Expressions/regress-394673.js,v  <--  regress-394673.js
initial revision: 1.1

The initial failure is gone on the trunk. This new test fails on 1.8.1 Linux debug shell. I'll verify after my next full run.

Comment 13

[Bob Clary [:bc] (inactive)]

verified fixed 1.9.0 linux|mac|win