Closed Bug 299898 Opened 19 years ago Closed 19 years ago

Get Narcissus running the js tests

Categories

(Core :: JavaScript Engine, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: mrbkap, Assigned: mrbkap)

Details

Attachments

(3 files, 4 obsolete files)

I have a very hacky patch to get Narcissus running the JS testsuite. I'll attach
it here, but it isn't ready for prime time yet.
Attached patch wip (obsolete) — Splinter Review
This adds Narcissus as an option. If it's used, then instead of running the
tests through the given engine, it runs the tests through Narcissus on the
given engine. I think I was seeing an infinite loop on one of the tests, but I
wasn't sure. I'll test more soon.
Assignee: general → mrbkap
Status: NEW → ASSIGNED
Comment on attachment 188523 [details] [diff] [review]
restore Activation object pollution prevention fix backed out the other day

r=me with the spelling and whitespace changes mentioned on IRC.
Attachment #188523 - Flags: review?(mrbkap) → review+
This is slightly cleaned up, and I've checked it over enough that I'm happy
with it. Bob, would you review?
Attachment #188492 - Attachment is obsolete: true
Attachment #188558 - Flags: review?(bob)
(In reply to comment #4)
> Bob, would you review?

Sure, I'll get to it tonight when I get back to the hotel.
Comment on attachment 188558 [details] [diff] [review]
slightly cleaned up jsDriver


> # command line option definition
> my $options = "b=s bugurl>b c=s classpath>c e=s engine>e f=s file>f " .
>   "h help>h i j=s javapath>j k confail>k l=s list>l L=s neglist>L " .
>   "o=s opt>o p=s testpath>p s=s shellpath>s t trace>t u=s lxrurl>u " .
>-  "x noexitmunge>x";
>+  "x noexitmunge>x n:s narcissus>n";

shouldn't that but n=s instead of n:s ?

>+       "(-n|--narcissus) [<path>] Run the test suite through Narcissus, run\n" .
>+       "                          through the given shell. The optional path\n".
>+       "                          is the path to Narcissus' js.js file.\n");

This is a bit confusing since -s is the full path of the js shell, but -n is
the path of the directory containing js.js. Can we specify the full location
including the filename instead?

>     $retval .= " $opt_engine_params";
> 
>+    if ($opt_enable_narcissus == 1) {
>+        my $narcissus_path = &get_narcissus_path;
>+        if ($narcissus_path) {
>+            $retval .= " -f $narcissus_path/js.js";
>+        } else {
>+            die "Cannot find Narcissus";
>+        }
>+    }
>+
>     &dd ("got '$retval'");
> 
>     return $retval;
>+}

The die is misleading since it really didn't check that narcissus
was found. I would move the die into the code below and actually
check that narcissus file exists.

>+
>+#
>+# get the path to the Narcissus js.js file.
>+#
>+sub get_narcissus_path {
>+    my $retval;
>+
>+    if ($opt_narcissus_path) {
>+        $retval = $opt_narcissus_path;
>+    } else {
>+        # For now, just assume that we're in js/tests.
>+        my $path = "../narcissus";
>+
>+        if (-e "$path/js.js") {
>+            $retval = $path;
>+        }
> 
>+        # XXX if it didn't exist, try something more fancy?
>+    }
>+

check that js.js exists here and die if not.

>+    return $retval;
> }

I have some other problems attempting to run the suite on winxp in cygwin due
to the hardcoded path ../narcissus/ found in js.js, and undefined snarf when
running in a DOS window. If we can resolve those issues outside of this bug, I
am ok with this with these comments addressed.
Attachment #188558 - Flags: review?(bob) → review+
(In reply to comment #6)
> (From update of attachment 188558 [details] [diff] [review] [edit])
> >+  "x noexitmunge>x n:s narcissus>n";
> shouldn't that but n=s instead of n:s ?

n:s is an optional value. I'm actually kind of hesitant now to add it as an
optional value and instead make the path another option (-np, perhaps?) to avoid
confusion.

> 
> This is a bit confusing since -s is the full path of the js shell, but -n is
> the path of the directory containing js.js. Can we specify the full location
> including the filename instead?

Sure.

> 
> >     $retval .= " $opt_engine_params";
> > 
> >+    if ($opt_enable_narcissus == 1) {
> >+        my $narcissus_path = &get_narcissus_path;
> >+        if ($narcissus_path) {
> >+            $retval .= " -f $narcissus_path/js.js";
> >+        } else {
> >+            die "Cannot find Narcissus";
> >+        }
> >+    }
> >+
> >     &dd ("got '$retval'");
> > 
> >     return $retval;
> >+}
> 
> The die is misleading since it really didn't check that narcissus
> was found. I would move the die into the code below and actually
> check that narcissus file exists.

Sure.

> I have some other problems attempting to run the suite on winxp in cygwin due
> to the hardcoded path ../narcissus/ found in js.js, and undefined snarf when
> running in a DOS window. If we can resolve those issues outside of this bug, I
> am ok with this with these comments addressed.
> 

Have you pulled an updated Narcissus? snarf should now be defined with a
suitably built engine (make -f Makefile.ref clean && make XCFLAGS=-DNARCISSUS -f
Makefile.ref; should do it).

The hardcoded paths are a problem. Brendan and I were talking about having a
__FILE__ and __LINE__ properties built into the shell, which might alleviate
this problem. Other suggestions are welcome. For now, you'll have to hack js.js
yourself.
Attached patch updated to comments (obsolete) — Splinter Review
Might as well go for one last round of review. This is updated to the comments
above.
Attachment #188558 - Attachment is obsolete: true
Attachment #188691 - Flags: review?(bob)
Comment on attachment 188691 [details] [diff] [review]
updated to comments

>-  "x noexitmunge>x";
>+  "x noexitmunge>x n narcissus>n np=s narcissus-path>np";

using "np" as a short option name causes a problem since 
-np /foo/js.js 
will be treated as -n -p /foo/js.js resulting in an error when it tries to
expand the tests in the testpath specified by -p.

I have an idea regarding the narcissus path. Lets talk on irc either tonight or
tomorrow.
Attachment #188691 - Flags: review?(bob) → review-
Comment on attachment 188558 [details] [diff] [review]
slightly cleaned up jsDriver

This is basically the patch that will be checked in.
Attachment #188558 - Attachment is obsolete: false
Attachment #188691 - Attachment is obsolete: true
This implements evaluate() in the shell and makes snarf() snarf relative to the
calling script rather than to the js shell it's being called from (as Brendan
and I discussed).

Bob, this should fix most of the rest of your woes with the js testsuite and
Narcissus (excluding the infinite loop I suspect we're getting into on the
Array.sort() test!).
Attachment #188993 - Flags: review?(brendan)
A slightly modified form (updated to review comments) of attachment 188558 [details] [diff] [review] has
been checked in. I'm leaving this bug open until (one of) attachment 188993 [details] [diff] [review] ('s
descendents) has been checked in, and there aren't any more path-related problems.
Comment on attachment 188993 [details] [diff] [review]
implement evaluate() in the shell

>+/*
>+ * This small helper loads fname using the correct relative path (which snarf
>+ * does for us).
>+ */

This comment says less than it could -- "small helper" is not needed, and the
parenthetical should be the main point, unparenthesized: it should say that
snarf reads a relative pathname by relating it to the calling script's source
file, not to process current working directory.

>+function do_load(fname) {
>+    evaluate(snarf(fname), fname, 1);
>+}

Nit: filename, not fname.

Nit: my_load?  Your call.

>+static JSBool
>+Evaluate(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval)
>+{
>+    /* function evaluate(source, filename, lineno) { ... } */
>+    JSString *source;
>+    const char *filename = "evaluate()";
>+    jsuint lineno = 1;

How about more standard defaults: "" and 0?  No point in saying something
useless in more bytes than 0 for the filename ;-).  And 0 lineno is well-known
to be not a valid line number.

>+    uint32 oldopts;
>+    JSErrorReporter older;
>+    JSScript *script;
>+    JSBool ok = JS_TRUE;
>+
>+    if (argc == 0) {
>+        *rval = JSVAL_VOID;
>+        return JS_TRUE;
>+    }
>+
>+    if (!JS_ConvertArguments(cx, argc, argv, "S/su",
>+                             &source, &filename, &lineno))
>+        return JS_FALSE;
>+
>+    older = JS_SetErrorReporter(cx, my_LoadErrorReporter);

D'oh, I have a patch to rip out this JS_SetErrorReporter nonsense from Load. 
Please don't clone it here, it's just wrong -- it leads to uncatchable
errors-as-exceptions.

>+    oldopts = JS_GetOptions(cx);
>+    JS_SetOptions(cx, oldopts | JSOPTION_COMPILE_N_GO);
>+    script = JS_CompileUCScript(cx, obj, JSSTRING_CHARS(source),
>+                                JSSTRING_LENGTH(source), filename,
>+                                lineno);
>+    if (!script) {
>+        ok = JS_FALSE;
>+    } else {
>+        ok = JS_ExecuteScript(cx, obj, script, rval);
>+        JS_DestroyScript(cx, script);

Here's the part where you want to simplify to avoid using a JSScript * local at
all, by calling JS_EvaluateUCScript.

>+/*
>+ * Returns a JS_malloc()'d string (that the caller needs to JS_free())

Nit: those ()s don't add value -- I know, I used to embolden code citations
with such decorations, but they just earn you negative LISP Karma. ;-)

>+ * containing the directory (non-leaf) part of |from|. If |from| is a directory,
>+ * it is returned as-is. If |from| is empty or is a leaf, an empty string is
>+ * returned.
>+ * NULL is returned if there is an error.

Nit: whole lotta passive voice going on -- rephrase?

>+ */
>+static char *
>+dirname(JSContext *cx, const char *from)
>+{
>+    size_t targetsize;
>+    const char *slash = 0, *it;

NULL in js/src for null, not 0.  Also, and this is giving way in some cases to
modern sensibilities, the house style avoids mixing initializers into
declarations at the top of functions.

>+    char *ret;
>+
>+    it = from;
>+    while (*it) {
>+        if (*it == '/'
>+#ifdef XP_WIN
>+                || *it == '\\'

Nit: indent to underhang the condition.

>+#endif
>+           ) {
>+            slash = it;
>+        }
>+
>+        ++it;

Is cp a better name than "it"?	I keep thinking of Cousin It, or the it object
in the shell.

>+    }
>+
>+    if (!slash) {
>+        /* We were given a leaf, or |from| was empty. */
>+        ret = JS_malloc(cx, 1);
>+        if (!ret)
>+            return NULL;
>+        *ret = 0;

Nit: '\0', not 0, to match prevailing style.

>+        return ret;
>+    }
>+
>+    /* Else, we were given a real pathname, return that. */
>+    targetsize = slash - from + 1;
>+    ret = JS_malloc(cx, targetsize + 1);
>+    if (!ret)
>+        return NULL;
>+
>+    strncpy(ret, from, targetsize);
>+    ret[targetsize] = 0;

In light of the targetsize name, rename ret to target?	Or shorten to
dir/dirlen?  I prefer those.

>+
>+    return ret;
>+}
>+
> static JSBool
> snarf(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval)
> {
>     JSString *str;
>-    const char *filename;
>+    const char *filename, *scriptname;
>+    char *relname;
>+    JSStackFrame *fp;
>     int fd, cc;
>     JSBool ok;
>     size_t len;
>     char *buf;
>     struct stat sb;
> 
>     str = JS_ValueToString(cx, argv[0]);
>     if (!str)
>         return JS_FALSE;
>     filename = JS_GetStringBytes(str);
>-    fd = open(filename, O_RDONLY);
>+    
>+    /* Get the currently executing script's name. */
>+    fp = JS_GetScriptedCaller(cx, NULL);
>+    JS_ASSERT(fp && fp->script->filename);
>+    scriptname = JS_GetScriptFilename(cx, fp->script);

Just use fp->script->filename.

>+    relname = dirname(cx, scriptname);
>+    if (!relname)
>+        return JS_FALSE;

Extra newline here, let it breathe a bit?

>+    len = JSSTRING_LENGTH(str);
>+    len += strlen(relname) + 1;
>+    relname = JS_realloc(cx, relname, len);

Bug: realloc can fail.

>+    strcat(relname, filename);

At this point relname may be a full pathname.  Suggest renaming relname to
pathname and getting rid of single-use scriptname.

Getting close, pick some nits and fix the few bugs above, and I'll stamp.

/be
Attachment #188993 - Flags: review?(brendan) → review-
Attached patch revised evaluate() (obsolete) — Splinter Review
This should have all of the nits picks (and oh how I hate realloc!).
Attachment #188993 - Attachment is obsolete: true
Attachment #189023 - Flags: review?(brendan)
Comment on attachment 189023 [details] [diff] [review]
revised evaluate()

>+    oldopts = JS_GetOptions(cx);
>+    JS_SetOptions(cx, oldopts | JSOPTION_COMPILE_N_GO);
>+    if (!JS_EvaluateUCScript(cx, obj, JSSTRING_CHARS(source),
>+                             JSSTRING_LENGTH(source), filename,
>+                             lineno, rval)) {
>+        return JS_FALSE;
>+    }
>+    JS_SetOptions(cx, oldopts);

Use a JSBool ok local to capture the return value of JS_EvaluateUCScript,
restore options, then return ok -- otherwise you'll leave the
JSOPTION_COMPILE_N_GO flag stuck, which would break precompilation cases on the
cx (reused for the rest of the js shell session).

>+    pathname = dirname(cx, fp->script->filename);
>+    if (!pathname)
>+        return JS_FALSE;
>+
>+    len = JSSTRING_LENGTH(str);
>+    len += strlen(pathname) + 1;
>+    pathname = JS_realloc(cx, pathname, len);
>+    if (!pathname)
>+        return JS_FALSE;

Since you hate realloc, how about reorganizing this code to pass str in so that
dirname (perhaps it should be called something else, say MakeAbsolutePathname
or abspath or some such) can allocate exactly the needed buffer.  True, dirname
as a primitive is slightly clearer, but this is the js shell -- you could even
inline all the dirname code here and sculpt it down to minimal size.

/be
Attachment #189023 - Flags: review?(brendan) → review-
No more realloc in this version. Bug with compile options is fixed. I decided
to keep MakeAbsolutePathname as its own function since I don't think I could
get it to be much smaller than it is (and it might even proove to be a handy
utility function at some point).

I couldn't get double bootstrapping to even try to work from js/src, but it's
late and I'm tired, so I might be doing something silly.
Attachment #189023 - Attachment is obsolete: true
Attachment #189027 - Flags: review?(brendan)
Comment on attachment 189027 [details] [diff] [review]
and with no realloc

Thanks, looks great.

/be
Attachment #189027 - Flags: review?(brendan) → review+
Summary: Get Narcissus running the js tests → Narcissus: Get Narcissus running the js tests
I just checked attachment 198027 [details] [diff] [review] in. Marking this bug as FIXED since there
should be no more problems actually _running_ the tests.
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Keywords: narcissus
Summary: Narcissus: Get Narcissus running the js tests → Get Narcissus running the js tests
Flags: testcase-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: