583143 - tracemonkey branch build fails with MOZ_TRACEVIS

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Steve Fink [:sfink] [:s:]]

Attachment: Fix up js::Value breakage when compiling with MOZ_TRACEVIS or (untested) DEBUG_ARRAYS

I leave it enabled in my build, and it didn't survive the fatvals landing. I don't understand this stuff, so I just patched over it in what seemed like an obvious way. (I haven't actually tried running tracevis yet.)

Dunno if I should just email these directly or what.

Comment 1

[Luke Wagner [:luke]]

Comment on attachment 461414 [details] [diff] [review]
Fix up js::Value breakage when compiling with MOZ_TRACEVIS or (untested) DEBUG_ARRAYS

Thanks!

Comment 2

[Steve Fink [:sfink] [:s:]]

Attachment: Fix up js::Value and vp breakage for various NPOTB options

This never made it in (probably because I neglected to request approval, although it's NPOTB so it shouldn't really need it.) So I'm updating it with a bunch more breakage fixes, this time from the argv/rval -> vp switch.

Comment 3

[Luke Wagner [:luke]]

Comment on attachment 473239 [details] [diff] [review]
Fix up js::Value and vp breakage for various NPOTB options

> #ifdef DEBUG_ARRAYS
> JSBool
>-js_ArrayInfo(JSContext *cx, JSObject *obj, uintN argc, Value *argv, Value *rval)
>+js_ArrayInfo(JSContext *cx, JSObject *obj, uintN argc, jsval *vp)

I think this won't compile -- you need to remove the 'obj'.

>-        bytes = DecompileValueGenerator(cx, JSDVG_SEARCH_STACK, argv[i], NULL);
>+        bytes = DecompileValueGenerator(cx, JSDVG_SEARCH_STACK, vp[i], NULL);

So an important detail is that 'vp' is not just another name for 'argv'. vp points to a stack of values: [callee, this, arg0, ..., argN]. Thus, to get the first argument, you need to skip a few values to arg0. We abstract this (sortof... mostly) with the macro:

  Value *argv = JS_ARGV(cx, vp);

So you'll want to do that for these cases.  I general, you can see the fast/slow natives differences here:
http://groups.google.com/group/mozilla.dev.tech.js-engine/browse_thread/thread/91ee3f1f5642e05b/37971dae04283b5f?lnk=gst&q=slow+natives#37971dae04283b5f

Comment 4

[Steve Fink [:sfink] [:s:]]

Attachment: Fix up js::Value and vp breakage for various NPOTB options

Heh. Yes, I had no clue how things are supposed to work now (nor how they worked before). Thanks for the link. This version ought to at least get substantially closer.

This does make me kind of want a

#define JS_ARG(cx,vp,i) ((Value*)JS_ARGV(cx,vp)[i])

(or better, an inline function overloaded on jsval* vp and Value* vp.)

I left all the functions with a signature of (JSContext*,uintN,jsval*). I don't know if it's better to switch to (JSContext*,uintN,Value*) and then cast with JS_FN().

Comment 5

[Luke Wagner [:luke]]

Comment on attachment 473963 [details] [diff] [review]
Fix up js::Value and vp breakage for various NPOTB options

Another change from slow -> fast natives is that you need to be sure to set the return value.  Slow natives had a default return value which was pre-set to JSVAL_UNDEFINED but fast natives must always explicitly set a return value if they return successfully.  Thus, for pretty much every native in this patch, you need to put a JS_SET_RVAL(cx, vp, JSVAL_UNDEFINED) before any 'return true'.

>-    while (fp && fp->script == NULL)
>+    while (fp && ! fp->hasScript())

>+    if (! obj)
>+        return JS_FALSE;

No space after !.  This happens a few more times in the patch.

>+    jsval val = JSVAL_VOID;
>+    JS_CallFunctionName(cx, p->filenames(), "push", 1, vp, &val);

s/vp/JS_ARGV(cx, vp)/

Also, since a fast native may be passed argc == 0, you'll want to test argc > 0.  Alternatively, you could do something like:

    if (argc < 1) {
        JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_MORE_ARGS_NEEDED,
                             "addScript", "0", "s");
        return JS_FALSE;
    }

and make it an error instead of silent failure.

Comment 6

[Steve Fink [:sfink] [:s:]]

Attachment: Fix up js::Value and vp breakage for various NPOTB options

Ok, thanks!

I assume you meant JSVAL_VOID instead of JSVAL_UNDEFINED?

I also noticed that there were two separate syntax errors that look like they predate fatvals. They're mixed in.

Also mixed in are some JS_TRUE->true, JS_FALSE->false changes; sorry if they clutter up the patch, but AIUI true/false are the recommended new way and I was adding some of those JS_TRUE/FALSEs, so I updated the methods I touched to be consistent.

I don't think the argc check was strictly necessary, because it would have resulted in filename==NULL and thus a silent successful early exit, but I reworked it to be more explicit. I did not convert it to an error because I don't want to have to worry about changing the API, even though the ethogram stuff seems to be currently unused and ignored. I may actually try to revive it before too long; if so, I'll break the API then.

Comment 7

[Steve Fink [:sfink] [:s:]]

Attachment: Fix up js::Value and vp breakage for various NPOTB options

Bug 595544 just landed, partially fixing some of this. I've updated the patch to adjust, and also to fix one problem introduced in that patch.

Specifically, it used 'jsval *rval = JS_RVAL(cx, vp)'. Would it be better if that were to work? As in, make JS_RVAL just be vp, instead of *vp as it is now? Or are there plans afoot to make JS_SET_RVAL necessary? I was tempted to use 'jsval *rval = &JS_RVAL(cx, vp)'.

Also, I'm no longer confident of jstv_Filename, given the recent comments in jsinterp.h for JStackFrame::prev() -- it seems like it could potentially walk up to find the wrong script now. (Or does it not matter enough to worry about?)

Comment 8

[Luke Wagner [:luke]]

Comment on attachment 475194 [details] [diff] [review]
Fix up js::Value and vp breakage for various NPOTB options

Sorry for the delay!  Looks good.  I'm pretty sure js_ArrayInfo still doesn't compile; it seems like you should either build with -DDEBUG_ARRAYS or leave it unmodified for whoever uses it to fix.

Comment 9

[Steve Fink [:sfink] [:s:]]

Attachment: Fix up js::Value and vp breakage for various NPOTB options

Finishing this to get it out of my queue.

You're right; I compiled with -DDEBUG_ARRAYS and the compile was broken. I wonder if the option is worth keeping; maybe the better change would be to remove it.

The only changes in this latest version of the patch are the compile fixes for DEBUG_ARRAYS.

Comment 10

[Brendan Eich [:brendan]]

Our highly-evolved monkey-debugging doctrine is #ifdef DEBUG or it bitrotted, use getenv (once, startup or cache in a static) to guard expensive logging code. So DEBUG_ARRAYS should either go away or become fast-enough-by-default DEBUG code.

/be

Comment 11

[Steve Fink [:sfink] [:s:]]

Not sure I completely follow what you're saying, but DEBUG_ARRAYS is not logging code and has no runtime cost. It just adds a js_ArrayInfo API entry that prints out basic information about the array to stderr (length, capacity, dense/sparse). And it adds an arrayInfo command to the JS shell to call it.

Are you saying I should make it DEBUG instead of DEBUG_ARRAYS? That seems like an improvement; there is no configure option to turn on DEBUG_ARRAYS right now, and there's no reason to not have it in DEBUG.

Comment 12

[Brendan Eich [:brendan]]

Sorry, should have quoted. I was playing on "pics or it didn't happen": "#ifdef DEBUG or it bit-rotted" is too true in our experience. So yeah, DEBUG if this is essentially zero-cost.

/be

Comment 13

[Luke Wagner [:luke]]

Comment on attachment 477198 [details] [diff] [review]
Fix up js::Value and vp breakage for various NPOTB options

r+ with s/DEBUG_ARRAY/DEBUG/ as Brendan suggests.  Since checkin-needed, I'll just make that change and check it in.

Comment 14

[Luke Wagner [:luke]]

http://hg.mozilla.org/tracemonkey/rev/1ed90254b97f

Comment 15

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/1ed90254b97f