480657 - TM: Add an API to define traceable constructors

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Comment 1

[Andreas Gal :gal]

Attachment: patch

Comment 2

[Jason Orendorff [:jorendorff]]

This asserts in FUN_CLASP if you try to record `new Math.sin` or some other traceable native.

I don't like adding a field to JSClass for this.

Per convention, js_Object_tn_ci should be called js_Object_ci.  I am too crosseyed today to figure out exactly which things you would have to rename to make that happen though...

Comment 3

[Andreas Gal :gal]

Attachment: v2

Comment 4

[Brendan Eich [:brendan]]

Comment on attachment 364624 [details] [diff] [review]
v2

>+++ b/js/src/jsapi.h
>@@ -1305,17 +1305,17 @@ struct JSClass {
>     JSPropertyOp        addProperty;
>     JSPropertyOp        delProperty;
>     JSPropertyOp        getProperty;
>     JSPropertyOp        setProperty;
>     JSEnumerateOp       enumerate;
>     JSResolveOp         resolve;
>     JSConvertOp         convert;
>     JSFinalizeOp        finalize;
>-
>+    
>     /* Optionally non-null members start here. */
>     JSGetObjectOps      getObjectOps;
>     JSCheckAccessOp     checkAccess;
>     JSNative            call;
>     JSNative            construct;
>     JSXDRObjectOp       xdrObject;
>     JSHasInstanceOp     hasInstance;
>     JSMarkOp            mark;

Revert, revert! Trailing whitespace red flashing alert!

> #define JS_DEFINE_TRCINFO_1(name, tn0)                                                            \
>     _JS_DEFINE_CALLINFO_n tn0                                                                     \
>     JSTraceableNative name##_trcinfo[] = {                                                        \
>-        { name, _JS_TN_INIT_HELPER_n tn0 }                                                        \
>+        { (JSFastNative)name, _JS_TN_INIT_HELPER_n tn0 }                                          \

Sad to have to cast to JSFastNative, just for constructors. Food for thought.

>+/* Defined in jsobj.cpp */
>+JS_DECLARE_CALLINFO(js_Object_tn)
>+
> /* Defined in jsarray.cpp */

Nit (one preexisting): full stop at end of such sentences in comments.

>+++ b/js/src/jsfun.h
>@@ -68,22 +68,20 @@ struct JSFunction {
>     uint16          nargs;        /* maximum number of specified arguments,
>                                      reflected as f.length/f.arity */
>     uint16          flags;        /* flags, see JSFUN_* below and in jsapi.h */
>     union {
>         struct {
>             uint16      extra;    /* number of arg slots for local GC roots */
>             uint16      spare;    /* reserved for future use */
>             JSNative    native;   /* native method pointer or null */
>+            JSClass    *clasp;    /* class of objects constructed
>+                                     by this function */

Line up * with declarators, not before first declarator column.

>+            JSTraceableNative *trcinfo;  /* tracer metadata; can be first
>+                                            element of array */

I wonder if we shouldn't union trcinfo with extra+spare (which could be one uint32). Only functions defined using JS_InitClass's trailing fs and static_fs parameters, and via JS_DefineFunctions, need extra.

> #define FUN_CLASP(fun)       (JS_ASSERT(!FUN_INTERPRETED(fun)),               \
>                               JS_ASSERT(!((fun)->flags & JSFUN_TRACEABLE)),   \
>-                              fun->u.n.u.clasp)
>+                              fun->u.n.clasp)

The assertion that fun is not traceable is obsolete now.

>+void
>+js_SetTraceableNativeCallInfo(JSContext *cx, JSObject *proto, JSTraceableNative *trcinfo)
>+{
>+    JSFunction *fun = GET_FUNCTION_PRIVATE(cx, JS_GetConstructor(cx, proto));
>+    JS_ASSERT(!(fun->flags & JSFUN_TRACEABLE));
>+    fun->u.n.trcinfo = trcinfo;
>+    fun->flags |= JSFUN_TRACEABLE;
>+}

Use a shorter and arguably more descriptive (in light of flag setting) name: js_SetTraceableNative.

/be

Comment 5

[Brendan Eich [:brendan]]

(In reply to comment #4)
> I wonder if we shouldn't union trcinfo with extra+spare (which could be one
> uint32). Only functions defined using JS_InitClass's trailing fs and static_fs
> parameters, and via JS_DefineFunctions, need extra.

Never mind, this won't work -- lots of such string, etc. methods defined in the engine with extra and traceable. Better to work on booting out extra+spare in a separate bug.

/be

Comment 6

[Andreas Gal :gal]

Attachment: v3

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 364727 [details] [diff] [review]
v3

> js_InitObjectClass(JSContext *cx, JSObject *obj)
> {
>-    return JS_InitClass(cx, obj, NULL, &js_ObjectClass, js_Object, 1,
>-                        object_props, object_methods, NULL,
>-                        object_static_methods);
>+    obj = JS_InitClass(cx, obj, NULL, &js_ObjectClass, js_Object, 1,
>+                       object_props, object_methods, NULL,
>+                       object_static_methods);
>+#ifdef JS_TRACER
>+    if (obj)
>+        js_SetTraceableNative(cx, obj, js_Object_trcinfo);
>+#endif
>+    return obj;
> }

Uber-nit: would be clearer to use JSObject *proto = JS_InitClass(...); ... return proto; here instead of reusing obj (the top-level or global object). r=me anyway, thanks.

/be

Comment 8

[Andreas Gal :gal]

I live to serve. Pushed with uber-nit fixed.

http://hg.mozilla.org/tracemonkey/rev/43d1d9137629

Comment 9

[Brendan Eich [:brendan]]

(In reply to comment #2)
> Per convention, js_Object_tn_ci should be called js_Object_ci.

Looks like this comment from Jason was missed -- sorry I didn't check for it.

/be

Comment 10

[Brendan Eich [:brendan]]

Also, with C++ it's possible to declare at first use (set, initialization) site, so we don't need C's army of uninitialized decls up top in each function. So either

js_InitObjectClass(...)
{
    JSObject *proto = JS_InitClass(...);
 ...

or else

js_InitObjectClass(...)
{
    JSObject *proto;

    proto = JS_InitClass(...);
 ...

but not the checked-in form that crunches lines together with uninitialized decls and no blank line separating decls from stmts.

Same for js_SetTraceableNative.

/be

Comment 11

[Andreas Gal :gal]

The CALLINFO macro picked the js_Object_tn_ci naming, not me. I can't change it.

The extra spaces moves the JS_InitClass beyond the old world boundaries. As long thats ok I will follow up with a quick patch.

Comment 12

[Brendan Eich [:brendan]]

Do we need the _tn suffix on the explicitly defined function name? If that is not dictated by a macro, we could have js_Object, js_Date, etc. Or a convention that extends the name without colliding with existing js_NewObject, etc., names.

I've been going past 79 judiciously in old files. But you could rewrap params, sometimes related ones pair or triple up nicely.

/be

Comment 13

[Andreas Gal :gal]

js_Object_tn is the name of the FASTCALL native we call from trace. I can't rename it to js_Object since thats the name of the actual JSFastNative.

I re-wrapped as suggested.

Comment 14

[Brendan Eich [:brendan]]

Ok, _tn_ci and an occasional _helper, Helper, or _sub it is ;-). Too much of this makes for cybercrud, though.

/be

Comment 15

[Andreas Gal :gal]

Backed out http://hg.mozilla.org/tracemonkey/rev/5be4f3b2fb7a

Comment 17

[Brendan Eich [:brendan]]

Testcase from bug 480791:

eval("new Math.sin");

/be

Comment 18

[Brendan Eich [:brendan]]

Attachment: v4

Looking to get this into TM soon, upvar2 depends on it.

/be

Comment 19

[Brendan Eich [:brendan]]

Why this matters for upvar2: user-defined functions may be constructors, in which case for function f, (new f).__proto__ === f.prototype. This means touching the current js_FastNewObject code in ways that overlap this patch, and benefit from it landing first.

/be

Comment 20

[Jason Orendorff [:jorendorff]]

Comment on attachment 365063 [details] [diff] [review]
v4

The name _JS_CTYPE_CALLEE_PROTO wasn't immediately clear.  Maybe _JS_CTYPE_CLASS_PROTOTYPE?

getClassPrototype looks like it's only valid if the .prototype property is PERMANENT and READONLY (as is the case only for a few builtin constructors).  It could assert that, just as an executable comment. (To avoid adding a lot of clutter, the assertion could use a very-high-level API like JS_GetPropertyAttributes.)

There's code in js_Object_tn that calculates clasp, but the answer is always the same, right?  It should just say:

JS_ASSERT(FUN_INTERPRETED(ctor) || FUN_CLASP(ctor) == &js_ObjectClass);

If there were one traceable native for Object() and a separate one for interpreted constructors, Object() could be faster (it could use CALLEE_PROTO).  Maybe not worth it.

Comment 21

[Brendan Eich [:brendan]]

(In reply to comment #20)
> (From update of attachment 365063 [details] [diff] [review])
> The name _JS_CTYPE_CALLEE_PROTO wasn't immediately clear.  Maybe
> _JS_CTYPE_CLASS_PROTOTYPE?

Will do.

> getClassPrototype looks like it's only valid if the .prototype property is
> PERMANENT and READONLY (as is the case only for a few builtin constructors). 
> It could assert that, just as an executable comment. (To avoid adding a lot of
> clutter, the assertion could use a very-high-level API like
> JS_GetPropertyAttributes.)

Ditto.

> There's code in js_Object_tn that calculates clasp, but the answer is always
> the same, right?  It should just say:
> 
> JS_ASSERT(FUN_INTERPRETED(ctor) || FUN_CLASP(ctor) == &js_ObjectClass);

I realized this over dinner. Fixing.

> If there were one traceable native for Object() and a separate one for
> interpreted constructors, Object() could be faster (it could use CALLEE_PROTO).
>  Maybe not worth it.

I was planning to do this, here or in a followup (possibly upvar2). Will do it here, patch shortly. Thanks,

/be

Comment 22

[Brendan Eich [:brendan]]

Attachment: v5

js_NewInstance follows the user-defined ctor.prototype rules on trace without needing to use FAIL_STATUS. Jason, thanks for the excellent comments -- I would appreciate one more review from you.

/be

Comment 23

[Jason Orendorff [:jorendorff]]

Comment on attachment 365154 [details] [diff] [review]
v5

One bug here:

>+JS_DEFINE_CALLINFO_2(extern, CONSTRUCTOR_RETRY, js_NewInstance, CONTEXT, CLASS_PROTOTYPE, 0, 0)

That should be CALLEE, not CLASS_PROTOTYPE.

IIUC, js_NewInstance has some significant bug fixes over the existing js_FastNewObject code (handling the case where ctor shares a scope, for instance).  Can you give a test case that would expose the bugs?

Style nits: In a few places the new code strikes me as too low-level -- using != in the for-loop condition instead of the idiomatic <, and obj->fslots[JSSLOT_PARENT] instead of OBJ_GET_PARENT(cx, obj).

Except for rooting, the interpreter could use this code too (in case JSOP_NEW when FUN_INTERPRETED(fun)).  Alas I think the root-as-we-go in JSOP_NEW is necessary with JS_THREADSAFE.

r=me with the one fix.

Comment 24

[Brendan Eich [:brendan]]

(In reply to comment #23)
> (From update of attachment 365154 [details] [diff] [review])
> One bug here:
> 
> >+JS_DEFINE_CALLINFO_2(extern, CONSTRUCTOR_RETRY, js_NewInstance, CONTEXT, CLASS_PROTOTYPE, 0, 0)
> 
> That should be CALLEE, not CLASS_PROTOTYPE.

Oops, fixed.

> IIUC, js_NewInstance has some significant bug fixes over the existing
> js_FastNewObject code (handling the case where ctor shares a scope, for
> instance).  Can you give a test case that would expose the bugs?

Gary's fuzzing produced one that depends on the upvar2 patch:

for (var x = 0; x < 3; ++x) { new function(){} }

Without the upvar2 patch, the buggy TraceRecorder::record_JSOP_LAMBDA stuffs the compiler-created function object into the trace, which satisfies the "own scope" assertion in js_FastNewObject but does not provide the right prototype per new call (observable via the completion value for the loop).

I'll see if I can write a test for this tomorrow.

> Style nits: In a few places the new code strikes me as too low-level -- using
> != in the for-loop condition instead of the idiomatic <, and

Clashing styles:

Yoyodyne:src brendaneich$ grep ' < JS_INITIAL_NSLOTS' *.cpp
jstracer.cpp:    if (size_t(p - globalObj->fslots) < JS_INITIAL_NSLOTS)
jstracer.cpp:    return ((size_t(p - globalObj->fslots) < JS_INITIAL_NSLOTS) ||
jstracer.cpp:    if (slot < JS_INITIAL_NSLOTS) {
jstracer.cpp:    JS_ASSERT(slot < JS_INITIAL_NSLOTS);
jstracer.cpp:    if (slot < JS_INITIAL_NSLOTS)
Yoyodyne:src brendaneich$ grep ' != JS_INITIAL_NSLOTS' *.cpp
jsarray.cpp:    for (unsigned i = JSSLOT_ARRAY_COUNT + 1; i != JS_INITIAL_NSLOTS; ++i)
jsdate.cpp:    for (unsigned i = JSSLOT_LOCAL_TIME + 1; i != JS_INITIAL_NSLOTS; ++i)
jsobj.cpp:    for (unsigned i = JSSLOT_PRIVATE; i != JS_INITIAL_NSLOTS; ++i)
jsobj.cpp:    for (unsigned i = JSSLOT_PRIVATE; i != JS_INITIAL_NSLOTS; ++i)
jsobj.cpp:    for (i = JSSLOT_PRIVATE; i != JS_INITIAL_NSLOTS; ++i)

The first two jsobj.cpp ones are from this bug's patch; the last predates and is in js_NewObjectWithGivenProto. I'm gonna stick with that for now.

> obj->fslots[JSSLOT_PARENT] instead of OBJ_GET_PARENT(cx, obj).

That's intentional style, Igor and I have been using it for newborns and other thread-local objects to avoid the macro obfuscation and show the C economics as clearly as possible.
 
> Except for rooting, the interpreter could use this code too (in case JSOP_NEW
> when FUN_INTERPRETED(fun)).  Alas I think the root-as-we-go in JSOP_NEW is
> necessary with JS_THREADSAFE.

Yup.
 
> r=me with the one fix.

Ok, will commit with whatever tests are feasible tomorrow. Thanks again,

/be

Comment 25

[Brendan Eich [:brendan]]

Attachment: v6

The js_SetTraceableNative approach fails in-browser due to security checks failing when trying to get Object.prototype.constructor (given the return value of JS_InitClass in js_InitObjectClass, call it proto, js_SetTraceableNative used JS_GetConstructor to get the ctor, etc. -- when cx->fp is null this means the security manager can't get a subject principal, so it censors the ctor).

I added a JS_InitTraceableClass API. The name isn't quite right but I'm loath to add JS_InitClassWithTraceableConstructor. Suggestions welcome.

/be

Comment 26

[Brendan Eich [:brendan]]

Committed to tm:

http://hg.mozilla.org/tracemonkey/rev/3b5f828befde

I wanted to get this cycling tonight. Jason, let me know if you want any changes. We can do cosmetics and any further work in bug 480759.

/be

Comment 27

[Brendan Eich [:brendan]]

Attachment: followup to keep --disable-jit working

Build bustage for PPC Mac -- checking in.

/be

Comment 28

[Brendan Eich [:brendan]]

Followup fix is in tm:

http://hg.mozilla.org/tracemonkey/rev/f82a4510d4b8

/be

Comment 29

[Jason Orendorff [:jorendorff]]

Brendan, can we change JS_InitTraceableClass -> js_InitClass and move it to jsobj.h?

It seems weird having a function in jsapi.h that embeddings can't call short of including nonpublic headers.

Comment 30

[Brendan Eich [:brendan]]

/be(In reply to comment #29)
> Brendan, can we change JS_InitTraceableClass -> js_InitClass and move it to
> jsobj.h?

Good idea, shoulda thought of that. I will do this in an updated patch for bug 480759. One question, though:
 
> It seems weird having a function in jsapi.h that embeddings can't call short of
> including nonpublic headers.

Do we expect no embedding will want to make its class constructors traceable? Or will they have to use non-public headers?

/be

Comment 31

[Brendan Eich [:brendan]]

(In reply to comment #30)
> > It seems weird having a function in jsapi.h that embeddings can't call short of
> > including nonpublic headers.
> 
> Do we expect no embedding will want to make its class constructors traceable?
> Or will they have to use non-public headers?

To answer my own question, it seems we want a generic traceable native wrapper for outside-the-engine stuff, and internal APIs such as js_InitClass for inside. If the generic wrapper can be done soon and it meets the needs of embeddings (like, the Gecko DOM!), I am more than happy.

/be

Comment 32

[Jason Orendorff [:jorendorff]]

Comment 31 re-raises questions that we should finish discussing in bug 463238 (where conversation apparently trailed off in confusion).

Comment 33

[Brendan Eich [:brendan]]

(In reply to comment #32)
> Comment 31 re-raises questions that we should finish discussing in bug 463238
> (where conversation apparently trailed off in confusion).

I commented in bug 463238 comment 11. HTH,

/be

Comment 34

[Brendan Eich [:brendan]]

Comment on attachment 364727 [details] [diff] [review]
v3

>-    if (!constructing && !(fun->flags & JSFUN_TRACEABLE))
>+    if (!(fun->flags & JSFUN_TRACEABLE))
>         ABORT_TRACE("untraceable native");

Argh, the removal of the !constructing predicate caused bug 481754.

/be

Comment 35

[Brendan Eich [:brendan]]

(In reply to comment #34)
> (From update of attachment 364727 [details] [diff] [review])
> >-    if (!constructing && !(fun->flags & JSFUN_TRACEABLE))
> >+    if (!(fun->flags & JSFUN_TRACEABLE))
> >         ABORT_TRACE("untraceable native");
> 
> Argh, the removal of the !constructing predicate caused bug 481754.

That removal was necessary given the next few lines in the patch, but we need a special case for js_Array, the slow native. Fixing over in bug 481754.

/be

Comment 36

[Bob Clary [:bc] (inactive)]

http://hg.mozilla.org/tracemonkey/rev/1c16405d7d6c
/cvsroot/mozilla/js/tests/js1_8_1/trace/trace-test.js,v  <--  trace-test.js
new revision: 1.12; previous revision: 1.11

Comment 37

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/3b5f828befde

Comment 38

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/ff874d684cf6

Comment 39

[Bob Clary [:bc] (inactive)]

v 1.9.1, 1.9.2