453730 - TM: We don't trace JSOP_ARGUMENTS

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Andreas Gal :gal]

This is the concrete use-case. Can we special-case this to use the current arguments already on the native stack if argc == nargs?

var Class = {
  create: function() {
    return function() {
      this.initialize.apply(this, arguments);
    }
  }
}

Comment 1

[Andreas Gal :gal]

This would be really handy to have. Its the only abort we currently hit on v8-raytrace (and we hit it a lot).

Comment 2

[Brendan Eich [:brendan]]

Working on it.

/be

Comment 3

[Brendan Eich [:brendan]]

This much landed:

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

I'll do more tomorrow, but if we sync with trunk before I fix this, I'll clone this bug and close the clone-parent as covering the above landing, leaving the clone-child for the remaining work (a custom "Object"-named arguments class for the js_Arguments built-in, whose class get and setProperty hooks can reach into the right native frame).

/be

Comment 4

[Brendan Eich [:brendan]]

(In reply to comment #3)
> This much landed:
> 
> http://hg.mozilla.org/tracemonkey/rev/ffb36911e55d
> 
> I'll do more tomorrow, but if we sync with trunk before I fix this, I'll clone
> this bug and close the clone-parent as covering the above landing, leaving the
> clone-child for the remaining work

No sync between tm and m-c yet, we really need one today.

> (a custom "Object"-named arguments class for
> the js_Arguments built-in, whose class get and setProperty hooks can reach into
> the right native frame).

Scratch that, we must use the same js_ArgumentsClass as the interpreter uses. I have this more-or-less in hand, not quite working, but the approach is sound. More in a bit.

/be

Comment 5

[Brendan Eich [:brendan]]

Attachment: patch that exposes latent bug(?) in deep bail-out code

Andreas, David: if you could apply this and give v8/run-raytrace.js a try (cd to v8 of course), and see what you can see, I'll be in your debt. I'm going to debug more later tonight.

/be

Comment 6

[Brendan Eich [:brendan]]

Reconstructing frames correctly, AFACT:

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_PROTECTION_FAILURE at address: 0x00000004
0x000e5532 in NativeToValue (cx=0x1100b00, v=@0x1808278, type=0 '\0', slot=0xbfffa730) at jstracer.cpp:1210
1210            debug_only_v(printf("object<%p:%s> ", JSVAL_TO_OBJECT(v),
(gdb) p/x *&v
$1 = 0x2
(gdb) up
#1  0x000e630a in FlushNativeStackFrame (cx=0x1100b00, callDepth=1, mp=0x6b6897 "", np=0xbfffa730, stopFrame=0x0) at jstracer.cpp:1300
1300        FORALL_SLOTS_IN_PENDING_FRAMES(cx, callDepth,
(gdb) x/g np
0xbfffa730:     0x4026beb800000002
(gdb) p callDepth
$2 = 1
(gdb) p *fsp
$3 = (JSStackFrame *) 0x18081f8
(gdb) p fp
$4 = (JSStackFrame *) 0x18081f8
(gdb) p fp.script.code
$5 = (jsbytecode *) 0x1119c64 ">W"
(gdb) p fp.regs.pc - $
$6 = 71
(gdb) p fp.script.length
$7 = 154
(gdb) call js_PCToLineNumber(cx, fp.script, fp.regs.pc)
$8 = 3202
(gdb) p cx.fp == fp
$9 = false
(gdb) p cx.fp.down == fp
$10 = true
(gdb) call js_PCToLineNumber(cx, cx.fp.script, cx.fp.regs.pc)
$11 = 2971
(gdb) up
#2  0x000ec95a in js_ExecuteTree (cx=0x1100b00, treep=0xbfffcfb8, inlineCallCount=@0xbfffdb88, innermostNestedGuardp=0xbfffcfb4) at jstracer.cpp:2572
2572        slots = FlushNativeStackFrame(cx, e->calldepth, e->typeMap + e->numGlobalSlots, stack, NULL);
(gdb) p lr.from.root.ip
$12 = (const void *) 0x1119c8b
(gdb) p cx.fp.regs.pc
$13 = (jsbytecode *) 0x1831d8e "?"
(gdb) p cx.fp.down.regs.pc
$14 = (jsbytecode *) 0x1119cab ":"
(gdb) p cx.fp.down.down.regs.pc
$15 = (jsbytecode *) 0x1840cda ":"
(gdb) p $14 - (jsbytecode*)$12
$16 = 32
(gdb) call js_PCToLineNumber(cx, cx.fp.down.script, lr.from.root.ip)
$18 = 3199

  3193      testIntersection: function(ray, scene, exclude){
  3194          var hits = 0;
  3195          var best = new Flog.RayTracer.IntersectionInfo();
  3196          best.distance = 2000;
  3197  
  3198          for(var i=0; i<scene.shapes.length; i++){
  3199              var shape = scene.shapes[i];
  3200  
  3201              if(shape != exclude){
  3202                  var info = shape.intersect(ray);
  3203                  if(info.isHit && info.distance >= 0 && info.distance < best.distance){
  3204                      best = info;
  3205                      hits++;
  3206                  }
  3207              }
  3208          }
  3209          best.hitCount = hits;
  3210          return best;
  3211      },

We're executing a trace that inlined intersect into testIntersection (callDepth is 1 in FlushNativeStackFrame), but the last (index 4) var in testIntersection, var shape, has not yet been initialized by the return value of the inlined shape.intersect(ray) call. But when we recorded this path, it had been (due to hotloop being 2) so its mapped type was object.

If we can prove that shape is not used before set, then we could import its uninitialized value, undefined, as null. ValueToNative will do that. But it is not what stored undefined (unboxed as 2) in the native stack slot -- rather, the local-var-initializing loop in TraceRecorder::record_EnterFrame did the deed (storing void_ins) as we inlined into testIntersection from rayTrace, the outer shallow frame that's also reconstructed (first) in this crash case.

So again, if we can prove no use before set, we can use the mapped type in the inner (callDepth 1) tree's entry frame, which is object, to choose a better default value than undefined.

Anything wrong with this analysis?

I'll file a new bug to do this work and make it block this bug.

/be

Comment 7

[Brendan Eich [:brendan]]

Forgot to show

(gdb) call js_PCToLineNumber(cx, cx.fp.down.script, cx.fp.down.regs.pc)
$21 = 3202

which points to the inlined call:

  3202                  var info = shape.intersect(ray);

Line 3199 (lr.from.root.ip) is of course the loop header.

/be

Comment 8

[Brendan Eich [:brendan]]

(In reply to comment #6)
> If we can prove that shape is not used before set, then we could import its
> uninitialized value, undefined, as null. ValueToNative will do that. But it is
> not what stored undefined (unboxed as 2) in the native stack slot -- rather,

[I should have written] "the machine code generated by"

> the local-var-initializing loop in TraceRecorder::record_EnterFrame did the
> deed (storing void_ins) as we inlined into testIntersection from rayTrace, the
> outer shallow frame that's also reconstructed (first) in this crash case.

The void stuffing for missing args and local vars should respect any existing mapped types.

This would help the SunSpider t/3d-raytrace.js benchmark too (where a call passing four args is traced, and the trace later triggered with two args and undefined for the missing actuals). See bug 454315 where the case of entry to the 3d-raytrace.js trace from js_ExecuteTree is covered.

A case like this bug's where we trigger an outer tree that nests an inner one would need the machine code generated by record_EnterFrame to store the right "undefined" default value for the mapped type.

Stop me if I stopped making sense!

/be

Comment 9

[Damon Sicore (:damons)]

blocking1.9.1+, requires a beta.

Comment 10

[Brendan Eich [:brendan]]

Committed code got ahead of itself by tracing JSOP_ARGUMENTS, which led to a MochiTest failure. I #if'ed that out so record_JSOP_ARGUMENTS aborts:

http://hg.mozilla.org/tracemonkey/rev/4a988152e0e0

This bug needs more work, but after b1.

/be

Comment 11

[Brendan Eich [:brendan]]

Dave, another one you should feel perfectly free to steal.

/be

Comment 12

[David Mandelin [:dmandelin]]

Attachment: WIP

I took the last patch and started working with it. Box2D (see bug 479090) is a good example of a program that relies on |arguments|. So far, I don't think the WIP helps much, because I'm not tracing apply yet. But the easier features seem to work ok.

Comment 13

[David Mandelin [:dmandelin]]

Attachment: WIP 2

This patch covers the most common use cases and gives a big speedup on microbenchmarks. The main problem is that things go wrong if an |arguments| object is created on trace and then we exit to the interpreter with the arguments object still live, or otherwise try to do something else with it. The reason is that the on-trace |arguments| object is not a real |arguments| object. Real |arguments| objects have a pointer to a JSStackFrame, which of course doesn't exist on trace.

I think the solution is to teach the frame reconstruction code how to turn fake |arguments| objects into real ones.

Comment 14

[Brendan Eich [:brendan]]

You don't need to change the object's identity if you are already using the right class to create it -- just JS_SetPrivate(cx, argsobj, fp), IIRC.

/be

Comment 15

[David Mandelin [:dmandelin]]

Attachment: Patch

* Functionality

This patch enables tracing of most (?) uses of the JS |arguments| construct, including .length, reading elements, and passing to apply. The main |arguments| usage that is still *not* traced is the not-in-range case: an |arguments| object can't be used on trace if it was created off trace.

* Design Overview

The main idea is to import fp->argsobj to the on-trace native stack area, exactly in the same way as callee, this, arguments, and slots are imported already. For JSOP_ARGUMENTS, the tracer records operations that create an |arguments| object and store it in the on-trace native stack area, so that the native stack area representation is isomorphic to the interpreter representation, just as is done in the tracer for the already imported fields.

The main difference is that ((OBJ_PRIVATE(fp->argsobj) == fp)) in the interpreter, but this cannot be maintained on trace because fp does not exist. Instead, on trace ((OBJ_PRIVATE(|arguments|) == 0)). When the object is used on trace, we use the tracker, native tracker, and native stack navigation to find the length and/or argument values. When we leave trace, as part of synthesizing stack frames we update the private field.

* Performance

It appears that this patch regresses SS perf by about 8ms. I would guess that the extra cost is mainly in the import of fp->argsobj when calling a trace, and of copying it back when leaving.

|arguments| operations get a big speedup in a microbenchmark. We also remove all |arguments|-related aborts for a noticeable improvement in v8 raytrace and earley-boyer:

                                  before-patch          after-patch
   f.apply(this, arguments)           242 ms                44 ms
   arguments[const]                   406 ms                42 ms

   v8 raytrace.js                     214 runs/s           254 runs/s
   v8 earley-boyer                    335 runs/s           363 runs/s

Comment 16

[Andreas Gal :gal]

Comment on attachment 385514 [details] [diff] [review]
Patch

Use on-trace jump and alloc instead of handing cached in as parameters.

> JSObject* FASTCALL
>-js_Arguments(JSContext* cx)
>+js_Arguments(JSContext* cx, JSObject* parent, JSObject* cached)
> {
>-    return NULL;
>+    if (cached)
>+        return cached;
>+    return js_NewObject(cx, &js_ArgumentsClass, NULL, NULL, 0);
> }
>-JS_DEFINE_CALLINFO_1(extern, OBJECT, js_Arguments, CONTEXT, 0, 0)
>+JS_DEFINE_CALLINFO_3(extern, OBJECT, js_Arguments, CONTEXT, OBJECT, OBJECT, 0, 0)
> 

File a bug to make a custom Arguments class that we can just memcpy the frame into (two different formats, interp and native).

>+    // If we have created an |arguments| object for the frame, we must copy the argument
>+    // values into the object as properties in case it is used after this frame returns.
>+    if (cx->fp->argsobj) {
>+        LIns* argsobj_ins = get(argsobj_as_jsval(cx->fp));
>+        LIns* length_ins = INS_CONST(cx->fp->argc);
>+        LIns* callee_ins = get(&cx->fp->argv[-2]);
>+        LIns* args_ins = lir->insAlloc(sizeof(jsval) * cx->fp->argc);
>+        for (uintN i = 0; i < cx->fp->argc; ++i) {
>+            LIns* arg_ins = get(&cx->fp->argv[i]);
>+            box_jsval(cx->fp->argv[i], arg_ins);
>+            lir->insStorei(arg_ins, args_ins, i * sizeof(jsval));
>+        }
>+        LIns* args[] = { args_ins, callee_ins, length_ins, argsobj_ins, cx_ins };
>+        LIns* call_ins = lir->insCall(&js_PutArguments_ci, args);
>+        guard(false, lir->ins_eq0(call_ins), STATUS_EXIT);
>+    }
>+
>     /* If we inlined this function call, make the return value available to the caller code. */
>     jsval& rval = stackval(-1);
>     JSStackFrame *fp = cx->fp;
>     if ((cx->fp->flags & JSFRAME_CONSTRUCTING) && JSVAL_IS_PRIMITIVE(rval)) {
>         JS_ASSERT(OBJECT_TO_JSVAL(fp->thisp) == fp->argv[-1]);
>         rval_ins = get(&fp->argv[-1]);
>     } else {
>         rval_ins = get(&rval);

Instead of abusing the tracker here you could just put another variable into the TraceRecorder and maintain this state there.

>+    LIns* a_ins = get(argsobj_as_jsval(cx->fp));
>+    JSObject* global = JS_GetGlobalForObject(cx, cx->fp->scopeChain);
>+    LIns* global_ins = INS_CONSTPTR(global);
>+    LIns* args[] = { a_ins, global_ins, cx_ins };
>+    a_ins = lir->insCall(&js_Arguments_ci, args);
>     guard(false, lir->ins_eq0(a_ins), OOM_EXIT);
>     stack(0, a_ins);
>-    return JSRS_CONTINUE;
>-#endif
>+    set(argsobj_as_jsval(cx->fp), a_ins);
>+    return JSRS_CONTINUE;
> }


>+#if 0
>+            LIns* fp_ins = stobj_get_private(aobj_ins);
>+            LIns* length_ins = lir->insLoad(LIR_ld, fp_ins, INS_CONST(offsetof(JSStackFrame, argc)));
>+            guard(true, lir->ins2i(LIR_eq, length_ins, length), BRANCH_EXIT);
>+#endif


????

Comment 17

[David Mandelin [:dmandelin]]

Attachment: Patch 2

Comment 18

[David Mandelin [:dmandelin]]

Patch 2 crashes on Mochitest. Here's a shell test case:

var isNotEmpty = function (obj) {
  for (var i = 0; i < arguments.length; i++) {
    var o = arguments[i];
    if (!(o && o.length)) {
      return false;
    }
  }
  return true;
};
print(isNotEmpty([1], [1], [1], "asdf"));

The problem is that I'm not checking for type stability when I get an arguments element with a non-constant index out of the native frame area. I think I just need to read the type out of the FrameInfo and test it, similar to upvar.

Comment 19

[David Mandelin [:dmandelin]]

Attachment: Patch 3

This version passes unit tests on the try server (other than a failure on linux which seems to have been a build timeout--but the standalone linux build passed). The main change from |Patch 2| is the verification of type stability for expressions of the form |args[i]| where |args| is a variable containing the arguments object and |i| is a non-constant integer expression.

Comment 20

[Andreas Gal :gal]

Comment on attachment 387301 [details] [diff] [review]
Patch 3

>diff -r 39882ccf1a24 js/src/builtins.tbl
>--- a/js/src/builtins.tbl	Tue Jul 07 09:49:55 2009 +1000
>+++ b/js/src/builtins.tbl	Tue Jul 07 14:57:59 2009 -0700
>@@ -82,10 +82,10 @@ BUILTIN2(extern, SIDEEXIT,  js_CallTree,
> BUILTIN3(extern, BOOL,      js_AddProperty, CONTEXT, OBJECT, SCOPEPROP,         0, 0)
> BUILTIN3(extern, BOOL,      js_HasNamedProperty, CONTEXT, OBJECT, STRING,       0, 0)
> BUILTIN3(extern, BOOL,      js_HasNamedPropertyInt32, CONTEXT, OBJECT, INT32,   0, 0)
> BUILTIN3(extern, JSVAL,     js_CallGetter, CONTEXT, OBJECT, SCOPEPROP,          0, 0)
> BUILTIN2(extern, STRING,    js_TypeOfObject, CONTEXT, OBJECT,                   1, 1)
> BUILTIN2(extern, STRING,    js_TypeOfBoolean, CONTEXT, INT32,                   1, 1)
> BUILTIN2(extern, DOUBLE,    js_BooleanOrUndefinedToNumber, CONTEXT, INT32,      1, 1)
> BUILTIN2(extern, STRING,    js_BooleanOrUndefinedToString, CONTEXT, INT32,      1, 1)
>-BUILTIN1(extern, OBJECT,    js_Arguments, CONTEXT,                              0, 0)
>+BUILTIN2(extern, OBJECT,    js_Arguments, CONTEXT, OBJECT                       0, 0)
> BUILTIN4(extern, OBJECT,    js_NewNullClosure, CONTEXT, OBJECT, OBJECT, OBJECT, 0, 0)
>diff -r 39882ccf1a24 js/src/jsbuiltins.cpp
>--- a/js/src/jsbuiltins.cpp	Tue Jul 07 09:49:55 2009 +1000
>+++ b/js/src/jsbuiltins.cpp	Tue Jul 07 14:57:59 2009 -0700
>@@ -390,21 +390,23 @@ JSString* FASTCALL
> js_BooleanOrUndefinedToString(JSContext *cx, int32 unboxed)
> {
>     JS_ASSERT(uint32(unboxed) <= 2);
>     return ATOM_TO_STRING(cx->runtime->atomState.booleanAtoms[unboxed]);
> }
> JS_DEFINE_CALLINFO_2(extern, STRING, js_BooleanOrUndefinedToString, CONTEXT, INT32, 1, 1)
> 
> JSObject* FASTCALL
>-js_Arguments(JSContext* cx)
>+js_Arguments(JSContext* cx, JSObject* parent, JSObject* cached)
> {
>-    return NULL;
>+    if (cached)
>+        return cached;
>+    return js_NewObject(cx, &js_ArgumentsClass, NULL, NULL, 0);
> }

This could be done on trace using jumps and LIR_chose, bypassing the call overhead in the common case. Probably not a big perf win though.

>-JS_DEFINE_CALLINFO_1(extern, OBJECT, js_Arguments, CONTEXT, 0, 0)
>+JS_DEFINE_CALLINFO_3(extern, OBJECT, js_Arguments, CONTEXT, OBJECT, OBJECT, 0, 0)
> 
> JSObject* FASTCALL
> js_NewNullClosure(JSContext* cx, JSObject* funobj, JSObject* proto, JSObject* parent)
> {
>     JS_ASSERT(HAS_FUNCTION_CLASS(funobj));
>     JS_ASSERT(HAS_FUNCTION_CLASS(proto));
>     JS_ASSERT(JS_ON_TRACE(cx));
> 
>diff -r 39882ccf1a24 js/src/jsbuiltins.h
>--- a/js/src/jsbuiltins.h	Tue Jul 07 09:49:55 2009 +1000
>+++ b/js/src/jsbuiltins.h	Tue Jul 07 14:57:59 2009 -0700
>@@ -449,16 +449,17 @@ JS_DECLARE_CALLINFO(js_NewInstance)
> /* Defined in jsarray.cpp. */
> JS_DECLARE_CALLINFO(js_Array_dense_setelem)
> JS_DECLARE_CALLINFO(js_NewEmptyArray)
> JS_DECLARE_CALLINFO(js_NewUninitializedArray)
> JS_DECLARE_CALLINFO(js_ArrayCompPush)
> 
> /* Defined in jsfun.cpp. */
> JS_DECLARE_CALLINFO(js_AllocFlatClosure)
>+JS_DECLARE_CALLINFO(js_PutArguments)
> 
> /* Defined in jsnum.cpp. */
> JS_DECLARE_CALLINFO(js_NumberToString)
> 
> /* Defined in jsstr.cpp. */
> JS_DECLARE_CALLINFO(js_String_tn)
> JS_DECLARE_CALLINFO(js_CompareStrings)
> JS_DECLARE_CALLINFO(js_ConcatStrings)
>diff -r 39882ccf1a24 js/src/jsfun.cpp
>--- a/js/src/jsfun.cpp	Tue Jul 07 09:49:55 2009 +1000
>+++ b/js/src/jsfun.cpp	Tue Jul 07 14:57:59 2009 -0700
>@@ -119,17 +119,17 @@ js_GetArgsValue(JSContext *cx, JSStackFr
> static JSBool
> MarkArgDeleted(JSContext *cx, JSStackFrame *fp, uintN slot)
> {
>     JSObject *argsobj;
>     jsval bmapval, bmapint;
>     size_t nbits, nbytes;
>     jsbitmap *bitmap;
> 
>-    argsobj = fp->argsobj;
>+    argsobj = JSVAL_TO_OBJECT(fp->argsobj);
>     (void) JS_GetReservedSlot(cx, argsobj, 0, &bmapval);
>     nbits = fp->argc;
>     JS_ASSERT(slot < nbits);
>     if (JSVAL_IS_VOID(bmapval)) {
>         if (nbits <= JSVAL_INT_BITS) {
>             bmapint = 0;
>             bitmap = (jsbitmap *) &bmapint;
>         } else {
>@@ -160,17 +160,17 @@ MarkArgDeleted(JSContext *cx, JSStackFra
> /* NB: Infallible predicate, false does not mean error/exception. */
> static JSBool
> ArgWasDeleted(JSContext *cx, JSStackFrame *fp, uintN slot)
> {
>     JSObject *argsobj;
>     jsval bmapval, bmapint;
>     jsbitmap *bitmap;
> 
>-    argsobj = fp->argsobj;
>+    argsobj = JSVAL_TO_OBJECT(fp->argsobj);
>     (void) JS_GetReservedSlot(cx, argsobj, 0, &bmapval);
>     if (JSVAL_IS_VOID(bmapval))
>         return JS_FALSE;
>     if (fp->argc <= JSVAL_INT_BITS) {
>         bmapint = JSVAL_TO_INT(bmapval);
>         bitmap = (jsbitmap *) &bmapint;
>     } else {
>         bitmap = (jsbitmap *) JSVAL_TO_PRIVATE(bmapval);
>@@ -203,38 +203,38 @@ js_GetArgsProperty(JSContext *cx, JSStac
>         return OBJ_GET_PROPERTY(cx, obj, id, vp);
>     }
> 
>     *vp = JSVAL_VOID;
>     if (JSID_IS_INT(id)) {
>         slot = (uintN) JSID_TO_INT(id);
>         if (slot < fp->argc) {
>             if (fp->argsobj && ArgWasDeleted(cx, fp, slot))
>-                return OBJ_GET_PROPERTY(cx, fp->argsobj, id, vp);
>+                return OBJ_GET_PROPERTY(cx, JSVAL_TO_OBJECT(fp->argsobj), id, vp);
>             *vp = fp->argv[slot];
>         } else {
>             /*
>              * Per ECMA-262 Ed. 3, 10.1.8, last bulleted item, do not share
>              * storage between the formal parameter and arguments[k] for all
>              * fp->argc <= k && k < fp->fun->nargs.  For example, in
>              *
>              *   function f(x) { x = 42; return arguments[0]; }
>              *   f();
>              *
>              * the call to f should return undefined, not 42.  If fp->argsobj
>              * is null at this point, as it would be in the example, return
>              * undefined in *vp.
>              */
>             if (fp->argsobj)
>-                return OBJ_GET_PROPERTY(cx, fp->argsobj, id, vp);
>+                return OBJ_GET_PROPERTY(cx, JSVAL_TO_OBJECT(fp->argsobj), id, vp);
>         }
>     } else {
>         if (id == ATOM_TO_JSID(cx->runtime->atomState.lengthAtom)) {
>             if (fp->argsobj && TEST_OVERRIDE_BIT(fp, ARGS_LENGTH))
>-                return OBJ_GET_PROPERTY(cx, fp->argsobj, id, vp);
>+                return OBJ_GET_PROPERTY(cx, JSVAL_TO_OBJECT(fp->argsobj), id, vp);
>             *vp = INT_TO_JSVAL((jsint) fp->argc);
>         }
>     }
>     return JS_TRUE;
> }
> 
> JSObject *
> js_GetArgsObject(JSContext *cx, JSStackFrame *fp)
>@@ -247,17 +247,17 @@ js_GetArgsObject(JSContext *cx, JSStackF
>      */
>     JS_ASSERT(fp->fun && (!(fp->fun->flags & JSFUN_HEAVYWEIGHT) || fp->varobj));
> 
>     /* Skip eval and debugger frames. */
>     while (fp->flags & JSFRAME_SPECIAL)
>         fp = fp->down;
> 
>     /* Create an arguments object for fp only if it lacks one. */
>-    argsobj = fp->argsobj;
>+    argsobj = JSVAL_TO_OBJECT(fp->argsobj);
>     if (argsobj)
>         return argsobj;
> 
>     /* Link the new object to fp so it can get actual argument values. */
>     argsobj = js_NewObject(cx, &js_ArgumentsClass, NULL, NULL, 0);
>     if (!argsobj || !JS_SetPrivate(cx, argsobj, fp)) {
>         cx->weakRoots.newborn[GCX_OBJECT] = NULL;
>         return NULL;
>@@ -273,17 +273,17 @@ js_GetArgsObject(JSContext *cx, JSStackF
>      * to a standard class object in the program will fail to resolve due to
>      * js_GetClassPrototype not being able to find a global object containing
>      * the standard prototype by starting from arguments and following parent.
>      */
>     global = fp->scopeChain;
>     while ((parent = OBJ_GET_PARENT(cx, global)) != NULL)
>         global = parent;
>     STOBJ_SET_PARENT(argsobj, global);
>-    fp->argsobj = argsobj;
>+    fp->argsobj = OBJECT_TO_JSVAL(argsobj);
>     return argsobj;
> }
> 
> static JSBool
> args_enumerate(JSContext *cx, JSObject *obj);
> 
> JS_FRIEND_API(JSBool)
> js_PutArgsObject(JSContext *cx, JSStackFrame *fp)
>@@ -293,17 +293,17 @@ js_PutArgsObject(JSContext *cx, JSStackF
>     JSBool ok;
>     JSRuntime *rt;
> 
>     /*
>      * Reuse args_enumerate here to reflect fp's actual arguments as indexed
>      * elements of argsobj.  Do this first, before clearing and freeing the
>      * deleted argument slot bitmap, because args_enumerate depends on that.
>      */
>-    argsobj = fp->argsobj;
>+    argsobj = JSVAL_TO_OBJECT(fp->argsobj);
>     ok = args_enumerate(cx, argsobj);
> 
>     /*
>      * Now clear the deleted argument number bitmap slot and free the bitmap,
>      * if one was actually created due to 'delete arguments[0]' or similar.
>      */
>     (void) JS_GetReservedSlot(cx, argsobj, 0, &bmapval);
>     if (!JSVAL_IS_VOID(bmapval)) {
>@@ -718,16 +718,35 @@ args_enumerate(JSContext *cx, JSObject *
>         if (!js_LookupProperty(cx, obj, INT_TO_JSID((jsint)slot), &pobj, &prop))
>             return JS_FALSE;
>         if (prop)
>             OBJ_DROP_PROPERTY(cx, pobj, prop);
>     }
>     return JS_TRUE;
> }
> 
>+JSBool JS_FASTCALL
>+js_PutArguments(JSContext* cx, JSObject* argsobj, uint32 length, JSObject* callee, jsval* args)
>+{
>+    if (!js_DefineProperty(cx, argsobj, ATOM_TO_JSID(cx->runtime->atomState.lengthAtom), 
>+                           INT_TO_JSVAL(length), args_getProperty, args_setProperty, 0, NULL))
>+        return JS_FALSE;
>+    if (!js_DefineProperty(cx, argsobj, ATOM_TO_JSID(cx->runtime->atomState.calleeAtom), 
>+                           OBJECT_TO_JSVAL(callee), args_getProperty, args_setProperty, 0, NULL))
>+        return JS_FALSE;
>+
>+    for (uintN i = 0; i < length; ++i) {
>+        if (!js_DefineProperty(cx, argsobj, INT_TO_JSID(i), args[i],
>+                               args_getProperty, args_setProperty, 0, NULL))
>+            return JS_FALSE;
>+    }
>+    return JS_TRUE;
>+}
>+JS_DEFINE_CALLINFO_5(extern, BOOL, js_PutArguments, CONTEXT, OBJECT, UINT32, OBJECT, JSVALPTR, 0, 0)
>+
> #if JS_HAS_GENERATORS
> /*
>  * If a generator-iterator's arguments or call object escapes, it needs to
>  * mark its generator object.
>  */
> static void
> args_or_call_trace(JSTracer *trc, JSObject *obj)
> {
>@@ -927,17 +946,17 @@ js_PutCallObject(JSContext *cx, JSStackF
> 
>     /*
>      * Get the arguments object to snapshot fp's actual argument values.
>      */
>     ok = JS_TRUE;
>     if (fp->argsobj) {
>         if (!TEST_OVERRIDE_BIT(fp, CALL_ARGUMENTS)) {
>             STOBJ_SET_SLOT(callobj, JSSLOT_CALL_ARGUMENTS,
>-                           OBJECT_TO_JSVAL(fp->argsobj));
>+                           fp->argsobj);
>         }
>         ok &= js_PutArgsObject(cx, fp);
>     }
> 
>     fun = fp->fun;
>     JS_ASSERT(fun == GetCallObjectFunction(callobj));
>     n = fun->countArgsAndVars();
>     if (n != 0) {
>diff -r 39882ccf1a24 js/src/jsgc.cpp
>--- a/js/src/jsgc.cpp	Tue Jul 07 09:49:55 2009 +1000
>+++ b/js/src/jsgc.cpp	Tue Jul 07 14:57:59 2009 -0700
>@@ -2877,17 +2877,17 @@ gc_lock_traversal(JSDHashTable *table, J
> void
> js_TraceStackFrame(JSTracer *trc, JSStackFrame *fp)
> {
>     uintN nslots, minargs, skip;
> 
>     if (fp->callobj)
>         JS_CALL_OBJECT_TRACER(trc, fp->callobj, "call");
>     if (fp->argsobj)
>-        JS_CALL_OBJECT_TRACER(trc, fp->argsobj, "arguments");
>+        JS_CALL_OBJECT_TRACER(trc, JSVAL_TO_OBJECT(fp->argsobj), "arguments");
>     if (fp->varobj)
>         JS_CALL_OBJECT_TRACER(trc, fp->varobj, "variables");
>     if (fp->script) {
>         js_TraceScript(trc, fp->script);
> 
>         /* fp->slots is null for watch pseudo-frames, see js_watch_set. */
>         if (fp->slots) {
>             /*
>diff -r 39882ccf1a24 js/src/jsinterp.cpp
>--- a/js/src/jsinterp.cpp	Tue Jul 07 09:49:55 2009 +1000
>+++ b/js/src/jsinterp.cpp	Tue Jul 07 14:57:59 2009 -0700
>@@ -1302,17 +1302,18 @@ have_fun:
>      * Initialize the frame.
>      *
>      * To set thisp we use an explicit cast and not JSVAL_TO_OBJECT, as vp[1]
>      * can be a primitive value here for those native functions specified with
>      * JSFUN_THISP_(NUMBER|STRING|BOOLEAN) flags.
>      */
>     frame.thisp = (JSObject *)vp[1];
>     frame.varobj = NULL;
>-    frame.callobj = frame.argsobj = NULL;
>+    frame.callobj = NULL;
>+    frame.argsobj = NULL;
>     frame.script = script;
>     frame.callee = funobj;
>     frame.fun = fun;
>     frame.argc = argc;
>     frame.argv = argv;
> 
>     /* Default return value for a constructor is the new object. */
>     frame.rval = (flags & JSINVOKE_CONSTRUCT) ? vp[1] : JSVAL_VOID;
>@@ -1581,17 +1582,18 @@ js_Execute(JSContext *cx, JSObject *chai
>         if (down->flags & JSFRAME_COMPUTED_THIS)
>             flags |= JSFRAME_COMPUTED_THIS;
>         frame.argc = down->argc;
>         frame.argv = down->argv;
>         frame.annotation = down->annotation;
>         frame.sharpArray = down->sharpArray;
>         JS_ASSERT(script->nfixed == 0);
>     } else {
>-        frame.callobj = frame.argsobj = NULL;
>+        frame.callobj = NULL;
>+        frame.argsobj = NULL;
>         obj = chain;
>         if (cx->options & JSOPTION_VAROBJFIX) {
>             while ((tmp = OBJ_GET_PARENT(cx, obj)) != NULL)
>                 obj = tmp;
>         }
>         frame.varobj = obj;
>         frame.callee = NULL;
>         frame.fun = NULL;
>diff -r 39882ccf1a24 js/src/jsinterp.h
>--- a/js/src/jsinterp.h	Tue Jul 07 09:49:55 2009 +1000
>+++ b/js/src/jsinterp.h	Tue Jul 07 14:57:59 2009 -0700
>@@ -66,17 +66,17 @@ typedef struct JSFrameRegs {
>  * sharp* and xml* members should be moved onto the stack as local variables
>  * with well-known slots, if possible.
>  */
> struct JSStackFrame {
>     JSFrameRegs     *regs;
>     jsbytecode      *imacpc;        /* null or interpreter macro call pc */
>     jsval           *slots;         /* variables, locals and operand stack */
>     JSObject        *callobj;       /* lazily created Call object */
>-    JSObject        *argsobj;       /* lazily created arguments object */
>+    jsval           argsobj;        /* lazily created arguments object, must be JSVAL_OBJECT */
>     JSObject        *varobj;        /* variables object, where vars go */
>     JSObject        *callee;        /* function or script object */
>     JSScript        *script;        /* script being interpreted */
>     JSFunction      *fun;           /* function being called or null */
>     JSObject        *thisp;         /* "this" pointer if in method */
>     uintN           argc;           /* actual argument count */
>     jsval           *argv;          /* base of argument stack slots */
>     jsval           rval;           /* function return value */
>diff -r 39882ccf1a24 js/src/jsiter.cpp
>--- a/js/src/jsiter.cpp	Tue Jul 07 09:49:55 2009 +1000
>+++ b/js/src/jsiter.cpp	Tue Jul 07 14:57:59 2009 -0700
>@@ -725,17 +725,17 @@ js_NewGenerator(JSContext *cx, JSStackFr
>     /* Steal away objects reflecting fp and point them at gen->frame. */
>     gen->frame.callobj = fp->callobj;
>     if (fp->callobj) {
>         JS_SetPrivate(cx, fp->callobj, &gen->frame);
>         fp->callobj = NULL;
>     }
>     gen->frame.argsobj = fp->argsobj;
>     if (fp->argsobj) {
>-        JS_SetPrivate(cx, fp->argsobj, &gen->frame);
>+        JS_SetPrivate(cx, JSVAL_TO_OBJECT(fp->argsobj), &gen->frame);
>         fp->argsobj = NULL;
>     }
> 
>     /* These two references can be shared with fp until it goes away. */
>     gen->frame.varobj = fp->varobj;
>     gen->frame.thisp = fp->thisp;
> 
>     /* Copy call-invariant script and function references. */
>diff -r 39882ccf1a24 js/src/jsobj.cpp
>--- a/js/src/jsobj.cpp	Tue Jul 07 09:49:55 2009 +1000
>+++ b/js/src/jsobj.cpp	Tue Jul 07 14:57:59 2009 -0700
>@@ -6234,17 +6234,17 @@ js_DumpStackFrame(JSStackFrame *fp)
>                 }
>             }
>         } else {
>             fprintf(stderr, "  sp:    %p\n", (void *) sp);
>             fprintf(stderr, "  slots: %p\n", (void *) fp->slots);
>         }
>         fprintf(stderr, "  argv:  %p (argc: %u)\n", (void *) fp->argv, (unsigned) fp->argc);
>         MaybeDumpObject("callobj", fp->callobj);
>-        MaybeDumpObject("argsobj", fp->argsobj);
>+        MaybeDumpObject("argsobj", JSVAL_TO_OBJECT(fp->argsobj));
>         MaybeDumpObject("varobj", fp->varobj);
>         MaybeDumpObject("this", fp->thisp);
>         fprintf(stderr, "  rval: ");
>         dumpValue(fp->rval);
>         fputc('\n', stderr);
> 
>         fprintf(stderr, "  flags:");
>         if (fp->flags == 0)
>diff -r 39882ccf1a24 js/src/jstracer.cpp
>--- a/js/src/jstracer.cpp	Tue Jul 07 09:49:55 2009 +1000
>+++ b/js/src/jstracer.cpp	Tue Jul 07 14:57:59 2009 -0700
>@@ -241,16 +241,17 @@ js_InitJITStatsClass(JSContext *cx, JSOb
> #else
> #define AUDIT(x) ((void)0)
> #endif /* JS_JIT_SPEW */
> 
> #define INS_CONST(c)        addName(lir->insImm(c), #c)
> #define INS_CONSTPTR(p)     addName(lir->insImmPtr(p), #p)
> #define INS_CONSTFUNPTR(p)  addName(lir->insImmPtr(JS_FUNC_TO_DATA_PTR(void*, p)), #p)
> #define INS_CONSTWORD(v)    addName(lir->insImmPtr((void *) v), #v)
>+#define INS_VOID()          INS_CONST(JSVAL_TO_PSEUDO_BOOLEAN(JSVAL_VOID))
> 
> using namespace avmplus;
> using namespace nanojit;
> 
> static GC gc = GC();
> static avmplus::AvmCore s_core = avmplus::AvmCore();
> static avmplus::AvmCore* core = &s_core;
> 
>@@ -1247,30 +1248,41 @@ public:
>             }
>             if (s0->isCall() && s0->callInfo() == &js_UnboxDouble_ci)
>                 return callArgN(s0, 0);
>         }
>         return out->insCall(ci, args);
>     }
> };
> 
>+/*
>+ * Visit the values in the given JSStackFrame that the tracer cares about. This visitor
>+ * function is (implicitly) the primary definition of the native stack area layout. There
>+ * are a few other independent pieces of code that must be maintained to assume the same
>+ * layout. They are marked like this:
>+ *
>+ *   Duplicate native stack layout computation: see VisitFrameSlots header comment.
>+ */
> template <typename Visitor>
> JS_REQUIRES_STACK static bool
> VisitFrameSlots(Visitor &visitor, unsigned depth, JSStackFrame *fp,
>                 JSStackFrame *up)
> {
>     if (depth > 0 && !VisitFrameSlots(visitor, depth-1, fp->down, fp))
>         return false;
> 
>     if (fp->callee) {
>         if (depth == 0) {
>             visitor.setStackSlotKind("args");
>             if (!visitor.visitStackSlots(&fp->argv[-2], argSlots(fp) + 2, fp))
>                 return false;
>         }
>+        visitor.setStackSlotKind("arguments");
>+        if (!visitor.visitStackSlots(&fp->argsobj, 1, fp))
>+            return false;
>         visitor.setStackSlotKind("var");
>         if (!visitor.visitStackSlots(fp->slots, fp->script->nfixed, fp))
>             return false;
>     }
>     visitor.setStackSlotKind("stack");
>     JS_ASSERT(fp->regs->sp >= StackBase(fp));
>     if (!visitor.visitStackSlots(StackBase(fp),
>                                  size_t(fp->regs->sp - StackBase(fp)),
>@@ -1409,20 +1421,21 @@ public:
>    all the way back to the entry frame, including the current stack usage. */
> JS_REQUIRES_STACK unsigned
> js_NativeStackSlots(JSContext *cx, unsigned callDepth)
> {
>     JSStackFrame* fp = cx->fp;
>     unsigned slots = 0;
>     unsigned depth = callDepth;
>     for (;;) {
>+        /* Duplicate native stack layout computation: see VisitFrameSlots header comment. */
>         unsigned operands = fp->regs->sp - StackBase(fp);
>         slots += operands;
>         if (fp->callee)
>-            slots += fp->script->nfixed;
>+            slots += fp->script->nfixed + 1 /*argsobj*/;
>         if (depth-- == 0) {
>             if (fp->callee)
>                 slots += 2/*callee,this*/ + argSlots(fp);
> #ifdef DEBUG
>             CountSlotsVisitor visitor;
>             VisitStackSlots(visitor, cx, callDepth);
>             JS_ASSERT(visitor.count() == slots && !visitor.stopped());
> #endif
>@@ -1739,17 +1752,23 @@ TraceRecorder::nativeGlobalOffset(jsval*
> /* Determine whether a value is a global stack slot */
> bool
> TraceRecorder::isGlobal(jsval* p) const
> {
>     return ((size_t(p - globalObj->fslots) < JS_INITIAL_NSLOTS) ||
>             (size_t(p - globalObj->dslots) < (STOBJ_NSLOTS(globalObj) - JS_INITIAL_NSLOTS)));
> }
> 
>-/* Determine the offset in the native stack for a jsval we track */
>+/* 
>+ * Return the offset in the native stack for the given jsval. More formally,
>+ * |p| must be the address of a jsval that is represented in the native stack
>+ * area. The return value is the offset, from InterpState::stackBase, in bytes,
>+ * where the native representation of |*p| is stored. To get the offset relative
>+ * to InterpState::sp, subtract TreeInfo::nativeStackBase.
>+ */
> JS_REQUIRES_STACK ptrdiff_t
> TraceRecorder::nativeStackOffset(jsval* p) const
> {
>     CountSlotsVisitor visitor(p);
>     VisitStackSlots(visitor, cx, callDepth);
>     size_t offset = visitor.count() * sizeof(double);
>     /*
>      * If it's not in a pending frame, it must be on the stack of the current frame above
>@@ -2259,16 +2278,19 @@ FlushNativeStackFrame(JSContext* cx, uns
>             // Skip over stopFrame itself.
>             JS_ASSERT(n != 0);
>             --n;
>             fp = fp->down;
>         }
>         for (; n != 0; fp = fp->down) {
>             --n;
>             if (fp->callee) {
>+                if (fp->argsobj)
>+                    JS_SetPrivate(cx, JSVAL_TO_OBJECT(fp->argsobj), fp);
>+
>                 /*
>                  * We might return from trace with a different callee object, but it still
>                  * has to be the same JSFunction (FIXME: bug 471425, eliminate fp->callee).
>                  */
>                 JS_ASSERT(JSVAL_IS_OBJECT(fp->argv[-1]));
>                 JS_ASSERT(HAS_FUNCTION_CLASS(JSVAL_TO_OBJECT(fp->argv[-2])));
>                 JS_ASSERT(GET_FUNCTION_PRIVATE(cx, JSVAL_TO_OBJECT(fp->argv[-2])) ==
>                           GET_FUNCTION_PRIVATE(cx, fp->callee));
>@@ -4314,22 +4336,23 @@ js_SynthesizeFrame(JSContext* cx, const 
>      */
>     JSInterpreterHook hook = cx->debugHooks->callHook;
>     if (hook) {
>         newifp->hookData = hook(cx, fp, JS_TRUE, 0, cx->debugHooks->callHookData);
>     } else {
>         newifp->hookData = NULL;
>     }
> 
>+    /* Duplicate native stack layout computation: see VisitFrameSlots header comment. */
>     // FIXME? we must count stack slots from caller's operand stack up to (but not including)
>     // callee's, including missing arguments. Could we shift everything down to the caller's
>     // fp->slots (where vars start) and avoid some of the complexity?
>     return (fi.spdist - fp->down->script->nfixed) +
>            ((fun->nargs > fp->argc) ? fun->nargs - fp->argc : 0) +
>-           script->nfixed;
>+           script->nfixed + 1/*argsobj*/;
> }
> 
> static void
> SynthesizeSlowNativeFrame(JSContext *cx, VMSideExit *exit)
> {
>     VOUCH_DOES_NOT_REQUIRE_STACK();
> 
>     void *mark;
>@@ -6330,26 +6353,36 @@ TraceRecorder::stackval(int n) const
> JS_REQUIRES_STACK LIns*
> TraceRecorder::scopeChain() const
> {
>     return lir->insLoad(LIR_ldp,
>                         lir->insLoad(LIR_ldp, cx_ins, offsetof(JSContext, fp)),
>                         offsetof(JSStackFrame, scopeChain));
> }
> 
>-static inline bool
>-FrameInRange(JSStackFrame* fp, JSStackFrame *target, unsigned callDepth)
>-{
>-    while (fp != target) {
>-        if (callDepth-- == 0)
>-            return false;
>+/*
>+ * Return the frame of a call object if that frame is part of the current trace. |depthp| is an
>+ * optional outparam: if it is non-null, it will be filled in with the depth of the call object's
>+ * frame relevant to cx->fp.
>+ */
>+JS_REQUIRES_STACK JSStackFrame*
>+TraceRecorder::frameIfInRange(JSObject* obj, unsigned* depthp) const
>+{
>+    JSStackFrame* ofp = (JSStackFrame*) JS_GetPrivate(cx, obj);
>+    JSStackFrame* fp = cx->fp;
>+    for (unsigned depth = 0; depth <= callDepth; ++depth) {
>+        if (fp == ofp) {
>+            if (depthp)
>+                *depthp = depth;
>+            return ofp;
>+        }
>         if (!(fp = fp->down))
>-            return false;
>-    }
>-    return true;
>+            break;
>+    }
>+    return NULL;
> }
> 
> JS_REQUIRES_STACK JSRecordingStatus
> TraceRecorder::activeCallOrGlobalSlot(JSObject* obj, jsval*& vp)
> {
>     // Lookup a name in the scope chain, arriving at a property either in the
>     // global object or some call object's fp->slots, and import that property
>     // into the trace's native stack frame. This could theoretically do *lookup*
>@@ -6388,18 +6421,18 @@ TraceRecorder::activeCallOrGlobalSlot(JS
>         OBJ_DROP_PROPERTY(cx, obj2, prop);
>         return JSRS_CONTINUE;
>     }
> 
>     if (wasDeepAborted())
>         ABORT_TRACE("deep abort from property lookup");
> 
>     if (obj == obj2 && OBJ_GET_CLASS(cx, obj) == &js_CallClass) {
>-        JSStackFrame* cfp = (JSStackFrame*) JS_GetPrivate(cx, obj);
>-        if (cfp && FrameInRange(cx->fp, cfp, callDepth)) {
>+        JSStackFrame* cfp = frameIfInRange(obj);
>+        if (cfp) {
>             JSScopeProperty* sprop = (JSScopeProperty*) prop;
> 
>             uint32 setflags = (js_CodeSpec[*cx->fp->regs->pc].format & (JOF_SET | JOF_INCDEC | JOF_FOR));
>             if (setflags && (sprop->attrs & JSPROP_READONLY))
>                 ABORT_TRACE("writing to a read-only property");
> 
>             uintN slot = sprop->shortid;
> 
>@@ -7842,21 +7875,25 @@ TraceRecorder::clearFrameSlotsFromCache(
> {
>     /* Clear out all slots of this frame in the nativeFrameTracker. Different locations on the
>        VM stack might map to different locations on the native stack depending on the
>        number of arguments (i.e.) of the next call, so we have to make sure we map
>        those in to the cache with the right offsets. */
>     JSStackFrame* fp = cx->fp;
>     jsval* vp;
>     jsval* vpstop;
>+    // Duplicate native stack layout computation: see VisitFrameSlots header comment.
>+    // This doesn't do layout arithmetic, but it must clear out all the slots defined as
>+    // imported by VisitFrameSlots.
>     if (fp->callee) {
>         vp = &fp->argv[-2];
>         vpstop = &fp->argv[argSlots(fp)];
>         while (vp < vpstop)
>             nativeFrameTracker.set(vp++, (LIns*)0);
>+        nativeFrameTracker.set(&fp->argsobj, (LIns*)0);
>     }
>     vp = &fp->slots[0];
>     vpstop = &fp->slots[fp->script->nslots];
>     while (vp < vpstop)
>         nativeFrameTracker.set(vp++, (LIns*)0);
> }
> 
> JS_REQUIRES_STACK JSRecordingStatus
>@@ -7875,30 +7912,34 @@ TraceRecorder::record_EnterFrame()
>                       js_AtomToPrintableString(cx, cx->fp->fun->atom),
>                       callDepth);
>     debug_only_stmt(
>         if (js_LogController.lcbits & LC_TMRecorder) {
>             js_Disassemble(cx, cx->fp->script, JS_TRUE, stdout);
>             debug_only_print0(LC_TMTracer, "----\n");
>         }
>     )
>-    LIns* void_ins = INS_CONST(JSVAL_TO_PSEUDO_BOOLEAN(JSVAL_VOID));
>-
>+    LIns* void_ins = INS_VOID();
>+
>+    // Duplicate native stack layout computation: see VisitFrameSlots header comment.
>+    // This doesn't do layout arithmetic, but it must initialize in the tracker all the
>+    // slots defined as imported by VisitFrameSlots.
>     jsval* vp = &fp->argv[fp->argc];
>     jsval* vpstop = vp + ptrdiff_t(fp->fun->nargs) - ptrdiff_t(fp->argc);
>     while (vp < vpstop) {
>         if (vp >= fp->down->regs->sp)
>             nativeFrameTracker.set(vp, (LIns*)0);
>         set(vp++, void_ins, true);
>     }
> 
>     vp = &fp->slots[0];
>     vpstop = vp + fp->script->nfixed;
>     while (vp < vpstop)
>         set(vp++, void_ins, true);
>+    set(&fp->argsobj, INS_CONSTPTR(0), true);
>     return JSRS_CONTINUE;
> }
> 
> JS_REQUIRES_STACK JSRecordingStatus
> TraceRecorder::record_LeaveFrame()
> {
>     debug_only_stmt(
>         if (cx->fp->fun)
>@@ -7956,16 +7997,33 @@ TraceRecorder::record_JSOP_RETURN()
> {
>     /* A return from callDepth 0 terminates the current loop. */
>     if (callDepth == 0) {
>         AUDIT(returnLoopExits);
>         endLoop(traceMonitor);
>         return JSRS_STOP;
>     }
> 
>+    // If we have created an |arguments| object for the frame, we must copy the argument
>+    // values into the object as properties in case it is used after this frame returns.
>+    if (cx->fp->argsobj) {
>+        LIns* argsobj_ins = get(&cx->fp->argsobj);
>+        LIns* length_ins = INS_CONST(cx->fp->argc);
>+        LIns* callee_ins = get(&cx->fp->argv[-2]);
>+        LIns* args_ins = lir->insAlloc(sizeof(jsval) * cx->fp->argc);
>+        for (uintN i = 0; i < cx->fp->argc; ++i) {
>+            LIns* arg_ins = get(&cx->fp->argv[i]);
>+            box_jsval(cx->fp->argv[i], arg_ins);
>+            lir->insStorei(arg_ins, args_ins, i * sizeof(jsval));
>+        }
>+        LIns* args[] = { args_ins, callee_ins, length_ins, argsobj_ins, cx_ins };
>+        LIns* call_ins = lir->insCall(&js_PutArguments_ci, args);
>+        guard(false, lir->ins_eq0(call_ins), STATUS_EXIT);
>+    }
>+
>     /* If we inlined this function call, make the return value available to the caller code. */
>     jsval& rval = stackval(-1);
>     JSStackFrame *fp = cx->fp;
>     if ((cx->fp->flags & JSFRAME_CONSTRUCTING) && JSVAL_IS_PRIMITIVE(rval)) {
>         JS_ASSERT(OBJECT_TO_JSVAL(fp->thisp) == fp->argv[-1]);
>         rval_ins = get(&fp->argv[-1]);
>     } else {
>         rval_ins = get(&rval);
>@@ -8006,25 +8064,25 @@ JS_REQUIRES_STACK JSRecordingStatus
> TraceRecorder::record_JSOP_IFNE()
> {
>     return ifop();
> }
> 
> JS_REQUIRES_STACK JSRecordingStatus
> TraceRecorder::record_JSOP_ARGUMENTS()
> {
>-#if 1
>-    ABORT_TRACE("can't trace arguments yet");
>-#else
>-    LIns* args[] = { cx_ins };
>-    LIns* a_ins = lir->insCall(&js_Arguments_ci, args);
>+    LIns* a_ins = get(&cx->fp->argsobj);
>+    JSObject* global = JS_GetGlobalForObject(cx, cx->fp->scopeChain);
>+    LIns* global_ins = INS_CONSTPTR(global);
>+    LIns* args[] = { a_ins, global_ins, cx_ins };
>+    a_ins = lir->insCall(&js_Arguments_ci, args);
>     guard(false, lir->ins_eq0(a_ins), OOM_EXIT);
>     stack(0, a_ins);
>-    return JSRS_CONTINUE;
>-#endif
>+    set(&cx->fp->argsobj, a_ins);
>+    return JSRS_CONTINUE;
> }
> 
> JS_REQUIRES_STACK JSRecordingStatus
> TraceRecorder::record_JSOP_DUP()
> {
>     stack(0, get(&stackval(-1)));
>     return JSRS_CONTINUE;
> }
>@@ -9186,16 +9244,90 @@ TraceRecorder::record_JSOP_GETELEM()
> 
>         // The object is not guaranteed to be a dense array at this point, so it might be the
>         // global object, which we have to guard against.
>         CHECK_STATUS(guardNotGlobalObject(obj, obj_ins));
> 
>         return call_imacro(call ? callelem_imacros.callprop : getelem_imacros.getprop);
>     }
> 
>+    if (STOBJ_GET_CLASS(obj) == &js_ArgumentsClass) {
>+        guardClass(obj, obj_ins, &js_ArgumentsClass, snapshot(MISMATCH_EXIT));
>+
>+        unsigned depth;
>+        JSStackFrame* afp = frameIfInRange(obj, &depth);
>+        if (afp) {
>+            uintN int_idx = JSVAL_TO_INT(idx);
>+            jsval* vp = &afp->argv[int_idx];
>+            if (idx_ins->isconstq()) {
>+                if (int_idx >= 0 && int_idx < afp->argc)
>+                    v_ins = get(vp);
>+                else
>+                    v_ins = INS_VOID();
>+            } else {
>+                // If the index is not a constant expression, we generate LIR to load the value from
>+                // the native stack area. The guard on js_ArgumentClass above ensures the up-to-date
>+                // value has been written back to the native stack area.
>+
>+                idx_ins = makeNumberInt32(idx_ins);
>+                if (int_idx >= 0 && int_idx < afp->argc) {
>+                    JSTraceType type = getCoercedType(*vp);
>+
>+                    // Guard that the argument has the same type on trace as during recording.
>+                    LIns* typemap_ins;
>+                    if (callDepth == depth) {
>+                        // In this case, we are in the same frame where the arguments object was created.
>+                        // The entry type map is not necessarily up-to-date, so we capture a new type map
>+                        // for this point in the code.
>+                        unsigned stackSlots = js_NativeStackSlots(cx, 0/*callDepth*/);
>+                        if (stackSlots * sizeof(JSTraceType) > NJ_MAX_SKIP_PAYLOAD_SZB)
>+                            ABORT_TRACE("|arguments| requires saving too much stack");
>+                        JSTraceType* typemap = (JSTraceType*) lir->insSkip(stackSlots * sizeof(JSTraceType))->payload();
>+                        DetermineTypesVisitor detVisitor(*this, typemap);
>+                        VisitStackSlots(detVisitor, cx, 0);
>+                        typemap_ins = INS_CONSTPTR(typemap + 2 /*callee,this*/);
>+                    } else {
>+                        // In this case, we are in a deeper frame from where the arguments object was
>+                        // created. The type map at the point of the call out from the creation frame
>+                        // is accurate.
>+                        // Note: this relies on the assumption that we abort on setting an element of
>+                        // an arguments object in any deeper frame.
>+                        LIns* fip_ins = lir->insLoad(LIR_ldp, lirbuf->rp, (callDepth-depth)*sizeof(FrameInfo*));
>+                        typemap_ins = lir->ins2(LIR_add, fip_ins, INS_CONST(sizeof(FrameInfo) + 2/*callee,this*/ * sizeof(JSTraceType)));
>+                    }
>+
>+                    LIns* typep_ins = lir->ins2(LIR_add, typemap_ins, 
>+                                                lir->ins2(LIR_mul, idx_ins, INS_CONST(sizeof(JSTraceType))));
>+                    LIns* type_ins = lir->insLoad(LIR_ldcb, typep_ins, 0);
>+                    guard(true,
>+                          addName(lir->ins2(LIR_eq, type_ins, lir->insImm(type)),
>+                                  "guard(type-stable upvar)"),
>+                          BRANCH_EXIT);
>+
>+                    // Read the value out of the native stack area.
>+                    guard(true, lir->ins2(LIR_ult, idx_ins, INS_CONST(afp->argc)), 
>+                          snapshot(BRANCH_EXIT));
>+                    size_t stackOffset = -treeInfo->nativeStackBase + nativeStackOffset(&afp->argv[0]);
>+                    LIns* args_addr_ins = lir->ins2(LIR_add, lirbuf->sp, INS_CONST(stackOffset));
>+                    LIns* argi_addr_ins = lir->ins2(LIR_add, args_addr_ins,
>+                                                    lir->ins2(LIR_mul, idx_ins, INS_CONST(sizeof(double))));
>+                    v_ins = stackLoad(argi_addr_ins, type);
>+                } else {
>+                    guard(false, lir->ins2(LIR_ult, idx_ins, INS_CONST(afp->argc)), 
>+                          snapshot(BRANCH_EXIT));
>+                    v_ins = INS_VOID();
>+                }
>+            }
>+            JS_ASSERT(v_ins);
>+            set(&lval, v_ins);
>+            return JSRS_CONTINUE;
>+        }
>+        ABORT_TRACE("can't reach arguments object's frame");
>+    }
>+
>     if (!guardDenseArray(obj, obj_ins, BRANCH_EXIT)) {
>         CHECK_STATUS(guardNotGlobalObject(obj, obj_ins));
> 
>         return call_imacro(call ? callelem_imacros.callelem : getelem_imacros.getelem);
>     }
> 
>     // Fast path for dense arrays accessed with a integer index.
>     jsval* vp;
>@@ -9436,17 +9568,25 @@ TraceRecorder::upvar(JSScript* script, J
>         cx_ins
>     };
>     LIns* call_ins = lir->insCall(ci, args);
>     JSTraceType type = getCoercedType(v);
>     guard(true,
>           addName(lir->ins2(LIR_eq, call_ins, lir->insImm(type)),
>                   "guard(type-stable upvar)"),
>           BRANCH_EXIT);
>-
>+    return stackLoad(outp, type);
>+}
>+
>+/*
>+ * Generate LIR to load a value from the native stack. This method ensures that the
>+ * correct LIR load operator is used.
>+ */
>+LIns* TraceRecorder::stackLoad(LIns* base, uint8 type)
>+{
>     LOpcode loadOp;
>     switch (type) {
>       case TT_DOUBLE:
>         loadOp = LIR_ldq;
>         break;
>       case TT_OBJECT:
>       case TT_STRING:
>       case TT_FUNCTION:
>@@ -9458,17 +9598,17 @@ TraceRecorder::upvar(JSScript* script, J
>         loadOp = LIR_ld;
>         break;
>       case TT_JSVAL:
>       default:
>         JS_NOT_REACHED("found jsval type in an upvar type map entry");
>         return NULL;
>     }
> 
>-    LIns* result = lir->insLoad(loadOp, outp, 0);
>+    LIns* result = lir->insLoad(loadOp, base, 0);
>     if (type == TT_INT32)
>         result = lir->ins1(LIR_i2f, result);
>     return result;
> }
> 
> JS_REQUIRES_STACK JSRecordingStatus
> TraceRecorder::record_JSOP_GETUPVAR()
> {
>@@ -9522,19 +9662,17 @@ TraceRecorder::guardCallee(jsval& callee
>     JS_ASSERT(VALUE_IS_FUNCTION(cx, callee));
> 
>     VMSideExit* branchExit = snapshot(BRANCH_EXIT);
>     JSObject* callee_obj = JSVAL_TO_OBJECT(callee);
>     LIns* callee_ins = get(&callee);
> 
>     guard(true,
>           lir->ins2(LIR_eq,
>-                    lir->ins2(LIR_piand,
>-                              stobj_get_fslot(callee_ins, JSSLOT_PRIVATE),
>-                              INS_CONSTWORD(~JSVAL_INT)),
>+                    stobj_get_private(callee_ins),
>                     INS_CONSTPTR(OBJ_GET_PRIVATE(cx, callee_obj))),
>           branchExit);
>     guard(true,
>           lir->ins2(LIR_eq,
>                     stobj_get_fslot(callee_ins, JSSLOT_PARENT),
>                     INS_CONSTPTR(OBJ_GET_PARENT(cx, callee_obj))),
>           branchExit);
>     return JSRS_CONTINUE;
>@@ -9668,40 +9806,41 @@ TraceRecorder::record_JSOP_APPLY()
>     if (apply && argc >= 2) {
>         if (argc != 2)
>             ABORT_TRACE("apply with excess arguments");
>         if (JSVAL_IS_PRIMITIVE(vp[3]))
>             ABORT_TRACE("arguments parameter of apply is primitive");
>         aobj = JSVAL_TO_OBJECT(vp[3]);
>         aobj_ins = get(&vp[3]);
> 
>-        /*
>-         * We expect a dense array for the arguments (the other
>-         * frequent case is the arguments object, but that we
>-         * don't trace at the moment).
>-         */
>-        if (!guardDenseArray(aobj, aobj_ins))
>-            ABORT_TRACE("arguments parameter of apply is not a dense array");
>-
>-        /*
>-         * We trace only apply calls with a certain number of arguments.
>-         */
>-        length = jsuint(aobj->fslots[JSSLOT_ARRAY_LENGTH]);
>+        /* 
>+         * We trace dense arrays and arguments objects. The code we generate for apply
>+         * uses imacros to handle a specific number of arguments.
>+         */
>+        if (OBJ_IS_DENSE_ARRAY(cx, aobj)) {
>+            guardDenseArray(aobj, aobj_ins);
>+            length = jsuint(aobj->fslots[JSSLOT_ARRAY_LENGTH]);
>+            guard(true,
>+                  lir->ins2i(LIR_eq,
>+                             stobj_get_fslot(aobj_ins, JSSLOT_ARRAY_LENGTH),
>+                             length),
>+                  BRANCH_EXIT);
>+        } else if (OBJ_GET_CLASS(cx, aobj) == &js_ArgumentsClass) {
>+            guardClass(aobj, aobj_ins, &js_ArgumentsClass, snapshot(MISMATCH_EXIT));
>+            JSStackFrame* afp = frameIfInRange(aobj);
>+            if (!afp)
>+                ABORT_TRACE("arguments object not in range");
>+            length = afp->argc;
>+        } else {
>+            ABORT_TRACE("arguments parameter of apply is not a dense array or argments object");
>+        }
>+
>         if (length >= JS_ARRAY_LENGTH(apply_imacro_table))
>             ABORT_TRACE("too many arguments to apply");
> 
>-        /*
>-         * Make sure the array has the same length at runtime.
>-         */
>-        guard(true,
>-              lir->ins2i(LIR_eq,
>-                         stobj_get_fslot(aobj_ins, JSSLOT_ARRAY_LENGTH),
>-                         length),
>-              BRANCH_EXIT);
>-
>         return call_imacro(apply_imacro_table[length]);
>     }
> 
>     if (argc >= JS_ARRAY_LENGTH(call_imacro_table))
>         ABORT_TRACE("too many arguments to call");
> 
>     return call_imacro(call_imacro_table[argc]);
> }
>@@ -9965,18 +10104,17 @@ TraceRecorder::prop(JSObject* obj, LIns*
>                             snapshot(MISMATCH_EXIT));
>                 return JSRS_CONTINUE;
>             }
>             if (setflags == 0 &&
>                 sprop->getter == js_StringClass.getProperty &&
>                 sprop->id == ATOM_KEY(cx->runtime->atomState.lengthAtom)) {
>                 if (!guardClass(obj, obj_ins, &js_StringClass, snapshot(MISMATCH_EXIT)))
>                     ABORT_TRACE("can't trace String.length on non-String objects");
>-                LIns* str_ins = stobj_get_fslot(obj_ins, JSSLOT_PRIVATE);
>-                str_ins = lir->ins2(LIR_piand, str_ins, INS_CONSTWORD(~JSVAL_TAGMASK));
>+                LIns* str_ins = stobj_get_private(obj_ins, JSVAL_TAGMASK);
>                 v_ins = lir->ins1(LIR_i2f, getStringLength(str_ins));
>                 return JSRS_CONTINUE;
>             }
>             ABORT_TRACE("non-stub getter");
>         }
>         if (!SPROP_HAS_VALID_SLOT(sprop, OBJ_SCOPE(obj2)))
>             ABORT_TRACE("no valid slot");
>         slot = sprop->slot;
>@@ -10879,20 +11017,21 @@ TraceRecorder::record_JSOP_NOP()
> }
> 
> JS_REQUIRES_STACK JSRecordingStatus
> TraceRecorder::record_JSOP_ARGSUB()
> {
>     JSStackFrame* fp = cx->fp;
>     if (!(fp->fun->flags & JSFUN_HEAVYWEIGHT)) {
>         uintN slot = GET_ARGNO(fp->regs->pc);
>-        if (slot < fp->fun->nargs && slot < fp->argc && !fp->argsobj) {
>+        if (slot < fp->argc)
>             stack(0, get(&cx->fp->argv[slot]));
>-            return JSRS_CONTINUE;
>-        }
>+        else
>+            stack(0, INS_VOID());
>+        return JSRS_CONTINUE;
>     }
>     ABORT_TRACE("can't trace JSOP_ARGSUB hard case");
> }
> 
> JS_REQUIRES_STACK JSRecordingStatus
> TraceRecorder::record_JSOP_ARGCNT()
> {
>     if (!(cx->fp->fun->flags & JSFUN_HEAVYWEIGHT)) {
>@@ -11725,16 +11864,27 @@ TraceRecorder::record_JSOP_LENGTH()
>         if (!JSVAL_IS_STRING(l))
>             ABORT_TRACE("non-string primitive JSOP_LENGTH unsupported");
>         set(&l, lir->ins1(LIR_i2f, getStringLength(get(&l))));
>         return JSRS_CONTINUE;
>     }
> 
>     JSObject* obj = JSVAL_TO_OBJECT(l);
>     LIns* obj_ins = get(&l);
>+
>+    if (STOBJ_GET_CLASS(obj) == &js_ArgumentsClass) {
>+        guardClass(obj, obj_ins, &js_ArgumentsClass, snapshot(MISMATCH_EXIT));
>+        JSStackFrame* afp = frameIfInRange(obj);
>+        if (afp) {
>+            LIns* v_ins = lir->ins1(LIR_i2f, INS_CONST(afp->argc));
>+            set(&l, v_ins);
>+            return JSRS_CONTINUE;
>+        }
>+    }
>+
>     LIns* v_ins;
>     if (OBJ_IS_ARRAY(cx, obj)) {
>         if (OBJ_IS_DENSE_ARRAY(cx, obj)) {
>             if (!guardDenseArray(obj, obj_ins, BRANCH_EXIT)) {
>                 JS_NOT_REACHED("OBJ_IS_DENSE_ARRAY but not?!?");
>                 return JSRS_STOP;
>             }
>         } else {
>diff -r 39882ccf1a24 js/src/jstracer.h
>--- a/js/src/jstracer.h	Tue Jul 07 09:49:55 2009 +1000
>+++ b/js/src/jstracer.h	Tue Jul 07 14:57:59 2009 -0700
>@@ -611,23 +611,25 @@ class TraceRecorder : public avmplus::GC
>                                                nanojit::Fragment** stable_peer,
>                                                bool& demote);
> 
>     JS_REQUIRES_STACK jsval& argval(unsigned n) const;
>     JS_REQUIRES_STACK jsval& varval(unsigned n) const;
>     JS_REQUIRES_STACK jsval& stackval(int n) const;
> 
>     JS_REQUIRES_STACK nanojit::LIns* scopeChain() const;
>+    JS_REQUIRES_STACK JSStackFrame* frameIfInRange(JSObject* obj, unsigned* depthp = NULL) const;
>     JS_REQUIRES_STACK JSRecordingStatus activeCallOrGlobalSlot(JSObject* obj, jsval*& vp);
> 
>     JS_REQUIRES_STACK nanojit::LIns* arg(unsigned n);
>     JS_REQUIRES_STACK void arg(unsigned n, nanojit::LIns* i);
>     JS_REQUIRES_STACK nanojit::LIns* var(unsigned n);
>     JS_REQUIRES_STACK void var(unsigned n, nanojit::LIns* i);
>     JS_REQUIRES_STACK nanojit::LIns* upvar(JSScript* script, JSUpvarArray* uva, uintN index, jsval& v);
>+    nanojit::LIns* stackLoad(nanojit::LIns* addr, uint8 type);
>     JS_REQUIRES_STACK nanojit::LIns* stack(int n);
>     JS_REQUIRES_STACK void stack(int n, nanojit::LIns* i);
> 
>     JS_REQUIRES_STACK nanojit::LIns* alu(nanojit::LOpcode op, jsdouble v0, jsdouble v1,
>                                          nanojit::LIns* s0, nanojit::LIns* s1);
>     nanojit::LIns* f2i(nanojit::LIns* f);
>     JS_REQUIRES_STACK nanojit::LIns* makeNumberInt32(nanojit::LIns* f);
>     JS_REQUIRES_STACK nanojit::LIns* stringify(jsval& v);
>@@ -673,16 +675,21 @@ class TraceRecorder : public avmplus::GC
>     void stobj_set_slot(nanojit::LIns* obj_ins, unsigned slot, nanojit::LIns*& dslots_ins,
>                         nanojit::LIns* v_ins);
> 
>     nanojit::LIns* stobj_get_fslot(nanojit::LIns* obj_ins, unsigned slot);
>     nanojit::LIns* stobj_get_dslot(nanojit::LIns* obj_ins, unsigned index,
>                                    nanojit::LIns*& dslots_ins);
>     nanojit::LIns* stobj_get_slot(nanojit::LIns* obj_ins, unsigned slot,
>                                   nanojit::LIns*& dslots_ins);
>+    nanojit::LIns* stobj_get_private(nanojit::LIns* obj_ins, jsval mask=JSVAL_INT) {
>+        return lir->ins2(nanojit::LIR_piand,
>+                         stobj_get_fslot(obj_ins, JSSLOT_PRIVATE),
>+                         lir->insImmPtr((void*) ~mask));
>+    }
>     JSRecordingStatus native_set(nanojit::LIns* obj_ins, JSScopeProperty* sprop,
>                                  nanojit::LIns*& dslots_ins, nanojit::LIns* v_ins);
>     JSRecordingStatus native_get(nanojit::LIns* obj_ins, nanojit::LIns* pobj_ins,
>                                  JSScopeProperty* sprop, nanojit::LIns*& dslots_ins,
>                                  nanojit::LIns*& v_ins);
> 
>     nanojit::LIns* getStringLength(nanojit::LIns* str_ins);
>

Comment 21

[Andreas Gal :gal]

Oh boy, I am going to get yelled at again for not shrinking the patch. Dave, whats the perf impact of the extra stack slot on the native stack? I assume its very small?

Comment 22

[David Mandelin [:dmandelin]]

Pushed to TM as c76558a87dd9 for baking. We can apply any changes from further reviews later. The perf impact seems to be minimal.

Comment 23

[Brendan Eich [:brendan]]

Comment on attachment 387301 [details] [diff] [review]
Patch 3

>+JSBool JS_FASTCALL
>+js_PutArguments(JSContext* cx, JSObject* argsobj, uint32 length, JSObject* callee, jsval* args)

Nit: use T *p style here, and in general (this was hashed out in another bug, jorendorff pointed out that the weight of tradition favors this style, and it wins if you have multiple declarators per declaration).

>+    if (!js_DefineProperty(cx, argsobj, ATOM_TO_JSID(cx->runtime->atomState.lengthAtom), 
>+                           INT_TO_JSVAL(length), args_getProperty, args_setProperty, 0, NULL))
>+        return JS_FALSE;
>+    if (!js_DefineProperty(cx, argsobj, ATOM_TO_JSID(cx->runtime->atomState.calleeAtom), 
>+                           OBJECT_TO_JSVAL(callee), args_getProperty, args_setProperty, 0, NULL))
>+        return JS_FALSE;

>+
>+    for (uintN i = 0; i < length; ++i) {
>+        if (!js_DefineProperty(cx, argsobj, INT_TO_JSID(i), args[i],
>+                               args_getProperty, args_setProperty, 0, NULL))
>+            return JS_FALSE;
>+    }

Nits: multiline conditions want braced consequent.

Looks good otherwise, I wonder why you didn't ins_choose the cached arg to js_Arguments too. Any data on this being worthwhile yet?

/be

Comment 24

[Brendan Eich [:brendan]]

Nits again: trailing space on two js_DefineProperty lines, and use false and true even for JSBool return value, now that C++ is here to stay even though nanojit or x86 ABI requires JSBool not bool.

/be

Comment 25

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/c76558a87dd9