225061 - Option to js shell to limit stack size

Status: VERIFIED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Igor Bukanov]

Since SM has API to limit C stack consumption, it would be nice if it would be
possible to pass a command line argument to js shell to specify the limit.

Comment 1

[Igor Bukanov]

Attachment: Support for -S stack_size_limit_in_bytes

With the patch I have for tests that checks that the engine can detect too deep
recursion:

~/w/js/mozilla/js/src> ./Linux_All_DBG.OBJ/js \
 -f ../tests/js1_5/shell.js -f ../tests/js1_5/Regress/regress-192414.js
Segmentation fault

~/w/js/mozilla/js/src> ./Linux_All_DBG.OBJ/js -S 100000 \
 -f ../tests/js1_5/shell.js -f ../tests/js1_5/Regress/regress-192414.js
BUGNUMBER: 192414
STATUS: Parser recursion should check stack overflow

So the option works as expected.

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 135049 [details] [diff] [review]
Support for -S stack_size_limit_in_bytes

>Index: js.c
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/js.c,v
>retrieving revision 3.73
>diff -U7 -r3.73 js.c
>--- js.c	2 Nov 2003 01:04:50 -0000	3.73
>+++ js.c	8 Nov 2003 10:41:30 -0000
>@@ -87,14 +87,16 @@
> #include <io.h>     /* for isatty() */
> #endif
> 
> #define EXITCODE_RUNTIME_ERROR 3
> #define EXITCODE_FILE_NOT_FOUND 4
> 
> size_t gStackChunkSize = 8192;
>+static size_t gStackSizeLimit = 0;
>+static unsigned char* stackBasePointer; 

How about using jsuword for the type of the last variable here, to move the
cast from pointer to int to the first opportunity (in main).

Nit, not relevant given jsuword type change: prevailing style would have
'unsigned char *foo', not 'unsigned char* foo'.

Nit: please avoid trailing white space (there's a space after
stackBasePointer;).

May as well use the gStackBasePointer naming convention for foolish
consistency.

>@@ -305,14 +307,26 @@
>             JS_ReportErrorNumber(cx, my_GetErrorMessage, NULL,
>                                  JSSMSG_CANT_OPEN, filename, strerror(errno));
>             gExitCode = EXITCODE_FILE_NOT_FOUND;
>             return;
>         }
>     }
> 
>+    if (gStackSizeLimit == 0) {
>+        JS_SetThreadStackLimit(cx, 0);

Don't do this.	The default works and there's no need to call
JS_SetThreadStackLimit.

>+    } else {
>+        unsigned char* limit;

Nit about pointer style, but make this jsuword too.

>@@ -2127,25 +2150,28 @@
>     env_enumerate, (JSResolveOp) env_resolve,
>     JS_ConvertStub,   JS_FinalizeStub
> };
> 
> int
> main(int argc, char **argv, char **envp)
> {
>+    unsigned char stackDummy;
>     JSVersion version;
>     JSRuntime *rt;
>     JSContext *cx;
>     JSObject *glob, *it, *envobj;
>     int result;
> #ifdef LIVECONNECT
>     JavaVM *java_vm = NULL;
> #endif
> #ifdef JSDEBUGGER_JAVA_UI
>     JNIEnv *java_env;
> #endif
>+    
>+    stackBasePointer = &stackDummy;
> 
> #ifdef XP_OS2
>    /* these streams are normally line buffered on OS/2 and need a \n, *
>     * so we need to unbuffer then to get a reasonable prompt          */
>     setbuf(stdout,0);
>     setbuf(stderr,0);
> #endif

Just a thought: use int stackDummy for consistency with rest of engine, or else
just avoid stackDummy and use the first local (version).

/be

Comment 3

[Igor Bukanov]

Attachment: Patch update to replace unsigned char* by jsuword and use more consistent naming.

In the patch I kept code to always call JS_SetThreadStackLimit even if
gStackSizeLimit since -S can be used several times with different values for
different files.

Comment 4

[Igor Bukanov]

Attachment: Version of the previous patch with removal of accidentally inserted empty lines.

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 135082 [details] [diff] [review]
Version of the previous patch with removal of accidentally inserted empty lines.

I'd just call that first local in main stackDummy (the rest of the code does),
but no big deal. Looks good to me -- thanks!

/be

Comment 6

[Brendan Eich [:brendan]]

Igor, are you gonna check in by tomorrow midnight?

/be

Comment 7

[Igor Bukanov]

Brendan: I have a write access only for Rhino CVS area.

Comment 8

[Brendan Eich [:brendan]]

Checked in.  Phil, can you adapt the testsuite to use the -S option appropriately.

/be

Comment 9

[Phil Schwartau]

> Phil, can you adapt the testsuite to use the -S option appropriately.


We're already set to go. Note the use of the -o option:

[D:/JS_TRUNK/mozilla/js/tests]
perl jsDriver.pl -e smdebug -fTEST.html -k -o "-S 1000000" -l js1_5/Regress
-#- Executing 52 test(s).
-#- Wrote results to 'TEST.html'.
-#- 0 test(s) failed


The -o option to the driver allows you to pass parameters to the JS shell.
Above, I am passing the new -S option to the JS shell, with a value I had
to set by trial and error on my particular machine.

Once I set the right number, I no longer fail on any of those
memory-hogging tests!  AFAIK, this is how we would do it.


Marking Verified FIXED -