Closed Bug 516255 Opened 10 years ago Closed 9 years ago

ES5 strict mode: arguments objects of strict mode functions must copy argument values

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jimb, Assigned: Waldo)

References

(Blocks 1 open bug)

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(5 files, 3 obsolete files)

17.85 KB, patch
dmandelin
: review+
Details | Diff | Splinter Review
6.31 KB, patch
dmandelin
: review+
Details | Diff | Splinter Review
14.31 KB, patch
dmandelin
: review+
Details | Diff | Splinter Review
34.70 KB, patch
dmandelin
: review+
Details | Diff | Splinter Review
6.01 KB, patch
dmandelin
: review+
Details | Diff | Splinter Review
This is not mentioned in Appendix C, but ES5 Section 10.6 requires, if you follow the logic of CreateArgumentsObject carefully, that arguments objects of strict mode functions have array index named properties that do *not* alias the named arguments, as they do in non-strict mode.

So, strict mode functions that use their arguments object and assign to their arguments must make a copy of their arguments upon entry to the function.
Mike, you mentioned this issue to me at the work week --- thanks for pointing it out.  This issue isn't mentioned in ES5 Appendix C, and even the text describing the arguments object itself seems confused, so it's good that we caught it.

You were concerned about the performance impact, as the ES5 strict semantics apparently require making a copy of the argument values on function entry.  However, if the function doesn't assign to its arguments, then we can put off the copy until someone tries to change the value of the arguments object properties.  It seems to me that stipulating that strict mode functions shouldn't both use their arguments objects and assign to their properties if they want the best performance is not too restrictive.

It still seems to me that the impact on real-world code of simply making the copy eagerly for functions that use their arguments object will be unmeasureable.  Now I'm on the record... :)
From prototypejs.org, prototype-1.6.0.2:

    function klass() {
      this.initialize.apply(this, arguments);
    }
...
        var method = value, value = Object.extend((function(m) {
          return function() { return ancestor[m].apply(this, arguments) };
        })(property).wrap(method), {
          valueOf:  function() { return method },
          toString: function() { return method.toString() }
        });
...
  stop: function() {
    this.updater.options.onComplete = undefined;
    clearTimeout(this.timer);
    (this.onComplete || Prototype.emptyFunction).apply(this, arguments);
  },

I left out Function.prototype.bind, since ES5 supplies it as a built-in.

These all currently do not copy arguments objects, on entry, but the second (var method = ...) captures arguments in a closure and unwinds, leaving the arguments object copying parameters from the dying stack frame into its reserved slots.

It's plausible stop is rarely called. I'd be more worried about klass, used to initialize all Prototype-created instances (IIRC -- check my memory). The klass case does not copy arguments into reserved slots at all, although it does create an arguments object to mediate access to the stack frame slots. This could be optimized harder.

Long story short, I've been burned often by asserting performance non-issues. Every O(n^2) algorithm I've left for the minority case as a simplification, in good 80-20 or 90-10 New-Joisey/Pareto-optimal style, has come back to haunt me.

Of course measurement to avoid premature optimization is best. The difficulty is getting timely feedback before you ship, because if you're badly wrong the users will feel some pain and web devs may turn on you before you can do a patch-release fix (which traditionally have been about security not performance).

The good news is that with the free variable analysis (upvar2), we should be able to tell when formals are assigned in strict code.

/be
(In reply to comment #2)
>         var method = value, value = Object.extend((function(m) {
>           return function() { return ancestor[m].apply(this, arguments) };
>         })(property).wrap(method), {
>           valueOf:  function() { return method },
>           toString: function() { return method.toString() }
>         });
> ...
> These all currently do not copy arguments objects, on entry, but the second
> (var method = ...) captures arguments in a closure and unwinds, leaving the
> arguments object copying parameters from the dying stack frame into its
> reserved slots.

Is that right?  I'm not familiar with prototypejs, but it looks to me like the reference to 'arguments' there must be to the inner (formal-less) function's own arguments, which it is immediately applying.  Similarly for 'stop'.

Given the conditions I suggested, I don't think any of this code would require copying argument values on entry to the function.
(In reply to comment #3)
> (In reply to comment #2)
> >         var method = value, value = Object.extend((function(m) {
> >           return function() { return ancestor[m].apply(this, arguments) };
> >         })(property).wrap(method), {
> >           valueOf:  function() { return method },
> >           toString: function() { return method.toString() }
> >         });
> > ...
> > These all currently do not copy arguments objects, on entry, but the second
> > (var method = ...) captures arguments in a closure and unwinds, leaving the
> > arguments object copying parameters from the dying stack frame into its
> > reserved slots.
> 
> Is that right?  I'm not familiar with prototypejs, but it looks to me like the
> reference to 'arguments' there must be to the inner (formal-less) function's
> own arguments, which it is immediately applying.

Oops, you're right!

> Given the conditions I suggested, I don't think any of this code would require
> copying argument values on entry to the function.

Yes, Prototype looks clean.

But I think we should take advantage of the assignment and def/use analysis to do better. Ollie Hunt of Apple was testifying on IRC about lack of analysis in JSC meaning their strict mode had to deoptimize. It could be another premature worry but my spider sense is tingling.

/be
(In reply to comment #4)
> But I think we should take advantage of the assignment and def/use analysis to
> do better. Ollie Hunt of Apple was testifying on IRC about lack of analysis in
> JSC meaning their strict mode had to deoptimize. It could be another premature
> worry but my spider sense is tingling.

We are in agreement.  The full expansion of what I said was:

Strict mode will, in general, require copying arguments upon entry to the function.  However, if a function never assigns to its formals, then the copy is not necessary; this special case seems to cover the popular use patterns.  We can use our def/use data to detect it.
Cool, thanks -- don't want to be the old guy in the corner muttering war stories, do want to avoid my past mistakes of prematurely de-optimizing (almost[*] every O(n^2) minority-case growth rate I've perpetrated has bit back).

/be

[*] http://mxr.mozilla.org/mozilla-central/source/js/src/jsscript.cpp#1188
also http://mxr.mozilla.org/mozilla-central/source/js/src/jsscript.cpp#1633
knock on wood...
Assignee: general → jwalden+bmo
OS: Linux → All
Hardware: x86 → All
I *think* I might have the patches done for this, just working through writing tests now (tracing tests in particular, normal tests good to go) -- will have everything ready by end of day.
Status: NEW → ASSIGNED
The patches build atop bug 514563's work, so this bug depends on that one.
Depends on: 514563
The new class and its ops are mostly identical to the "current" ones with the dependency bug's patch applied.  This is intentional: separate that switch from the functional one, as much as possible, to keep things more bite-sized.  They'll change in a later patch here.
Attachment #465174 - Flags: review?(dmandelin)
We create arguments objects lazily now, but that can't work if assigning to a parameter is supposed to leave arguments[n] unchanged.  Either you need sideband information for fork-on-set, or you create it early and disconnect it from parameter value storage when you do so.  We mostly have the name analysis to know when we may need to create early, so that's definitely the easier course.  (I'll make a blog post after this lands talking about what we do and how to not trigger bad behaviors -- that should be adequate for most web developers.)

This is just the analysis: no use of the collected information just yet.
Attachment #465176 - Flags: review?(dmandelin)
Since strict mode is a lexical switch, this affords flexibility to make some optimizations (some of which are really more than that, because they affect correctness), like not having to do the heavy work of putting arguments objects on frame unwind, and just looking to dslots[i] for arguments[i].

We now have to remember to copy in arguments values when creating the arguments object.  The bad behavior that would result from forgetting to copy would be arguments[i] having the wrong value.  There are only a couple places that create arguments objects, so it's not hard to verify this happens.
Attachment #465177 - Flags: review?(dmandelin)
I figured it would be nicer if the vegetables were separate from the meat, so the meal doesn't look too big.  ;-)
Attachment #465179 - Flags: review?(dmandelin)
The XXX comment ignored (added, even, for strict mode getter/setter!) in previous patches is easily addressed.  I wonder if ES3 specified the attributes for arguments[i] weirdly, or web-incompatibly, or something, because it seems entirely obvious what should be happening here.  An eagle eye might also have noticed that at one point in the sequence of arguments-altering patches arguments.caller wasn't being enumerated for strict-mode arguments, so I *think* it would have been |Object.getOwnPropertyNames(arguments).indexOf("caller") < 0| rather than, correctly, >=.  But my memory is starting to go hazy, and it's really late now, so I'm not going to try to figure out exactly when that error was introduced, or if it was something similar yet subtly different.  ;-)

I claim this attachment's post time (it's the last one for the bug, if all goes well) meets the definition of "today" (cf. comment 7) that ranges from 0600-2959.  ;-)
Attachment #465182 - Flags: review?(dmandelin)
Great work!  So your patches do avoid copying a strict |arguments| object's properties when the actual argument variables themselves are never assigned to, and do lazily copy properties of a strict |arguments| object when they are set? That's the optimization I was hoping we could do.
+
+                /*
+                 * If any uses of this name are assignments, we have to
+                 * determine whether it was a parameter in strict mode code.
+                 * If it was, we need to flag that function for eager arguments
+                 * object creation, to ensure parameter values are copied into
+                 * the arguments object prior to any possible mutations.
+                 */
+                if (funtc->inStrictMode() && dn->isAssigned()) {
+                    JSDefinition *def = outer_dn->resolve();
+                    if (def->kind() == JSDefinition::ARG) {
+                        for (JSTreeContext *defTc = tc;
+                             defTc && defTc->inStrictMode() && defTc->inFunction();
+                             defTc = defTc->parent) {
+                            if (JSAtomListElement *dfnAle = defTc->decls.lookup(atom)) {
+                                if (ALE_DEFN(dfnAle) == def) {
+                                    defTc->noteParameterMutation();
+                                    break;
+                                }
+                            }
+                        }
+                    }
+                }

Why can't we simply make a pass over only the arguments of the function being finished, and check their PND_ASSIGNED flags? We'll get to the outer tc's eventually.
Re: comment 15:

Right, because we propagage dflags:

                    outer_dn->pn_dflags |= dn->pn_dflags & ~PND_PLACEHOLDER;

when leaving a function, any assignments to upvars that are parameters will be flagged in due course.

/be
Attachment #465174 - Flags: review?(dmandelin) → review+
Comment on attachment 465174 [details] [diff] [review]
1: Represent strict mode arguments with a different class

>@@ -169,25 +169,28 @@ NewArguments(JSContext *cx, JSObject *pa
>     if (!js_GetClassPrototype(cx, parent, JSProto_Object, &proto))
>         return NULL;
> 
>+    bool strict = false;
>+    if (callee) {

The callee parameter cannot be null. I noticed this when reviewing fatvals, sorry for not bugging it sooner. Fixing it here is fine.

>+        JSFunction *fun = callee->getFunctionPrivate();
>+        if (fun->isInterpreted() && fun->u.i.script->strictModeCode)

How about a fun->inStrictMode() helper?

>     argsobj->setArgsCallee(ObjectOrNullValue(callee));

So this call should pass ObjectValue(*callee).

/be
Comment on attachment 465176 [details] [diff] [review]
2: Analyze name uses and eval calls, and record that information at parse time for use later

We already modified the front end to record which functions call eval in JM. I think they don't conflict very much, though. But we'll need to be careful not to make too much of a mess there.

I'm not highly qualified on the front end parts, but it basically looks ok to me. Except that these names seem a little funny:

>+    void noteCallsEval() {
>+        flags |= TCF_FUN_CALLS_EVAL;
>+    }
>+

>+    void noteParameterMutation() {
>+        JS_ASSERT(inFunction());
>+        flags |= TCF_FUN_MUTATES_PARAMETER;
>+    }
>+

>+    void noteArgumentsUse() {
>+        JS_ASSERT(inFunction());
>+        flags |= TCF_FUN_USES_ARGUMENTS;
>+        if (funbox)
>+            funbox->node->pn_dflags |= PND_FUNARG;

Wouldn't |setCallsEval|, |setParameterMutation|, and |setUsesArguments| be more idiomatic? Do you use those names in the followon patches? If so, I guess I should just r+ this and then we can tweak names at the end if desired. Otherwise, might as well do that now.
Attachment #465176 - Flags: review?(dmandelin)
Will look at comments after I get to the office today (shortly), just want to note that the MS ES5 tests have made me aware of JSOP_ARGSUB, which needs to be made aware of this change.  (This also makes me wonder whether JSOP_CALLEE needs to be made aware of strict mode too -- something else to investigate.)  So at least that needs to be addressed before this is fully fixed.
JSOP_CALLEE is for unambiguous self-name references in named function expression bodies, not for any .callee property. So don't worry about it.

/be
Re: comment 18, Waldo is picking up on my use of "note" as metaphor instead of "set", as the parser drops verbs and names methods after non-terminals in the gramar (or I should write *a* grammar -- not necessarily ECMA-262's verbose-name-style LR(1) grammar).

"Set" is a big ambiguous in that light, while "note" (we hope) conveys the compiler taking notes as it parses, not noun-ing verbs for non-terminal naming.

/be
Comment on attachment 465176 [details] [diff] [review]
2: Analyze name uses and eval calls, and record that information at parse time for use later

Comments re: note make sense to me.
Attachment #465176 - Flags: review+
I took a stab at making JSOP_ARGSUB work with the current setup, but on trace I think I need phi nodes to make it work.  So instead I just interjected some logic to not generate JSOP_ARGSUB in the potentially problematic cases.
Attachment #465179 - Attachment is obsolete: true
Attachment #465402 - Flags: review?(dmandelin)
Attachment #465179 - Flags: review?(dmandelin)
Attachment #465402 - Attachment description: 4: Tests for arguments/parameter values with various access patterns -- now without JSOP_ARGSUB fail! → 3: Use name analysis information to synthesize an arguments access at the start of functions which might modify parameters
Attachment #465177 - Attachment is obsolete: true
Attachment #465177 - Flags: review?(dmandelin)
This adds tests which exercise what used to be the JSOP_ARGSUB cases, but which now are deoptimized into normal JSOP_GETELEM and such.
Attachment #465407 - Flags: review?(dmandelin)
Looking into comment 15 and comment 16's suggestion, sounds reasonable, just tripping over this case (already in tests here \o/) at the moment, debugging why now:

function strictAssignOuterParam(p)
{
  "use strict";
  function inner() { p = 17; }
  inner();
  return arguments;
}; assertEq(strictAssignOuterParam(42)[0], 42);
Comment on attachment 465182 [details] [diff] [review]
5: Fix the attributes on arguments[i], and test them

Nice. I see a couple of comments that note that the arguments object in a strict mode frame must be created before any parameters are modified. But the front end flags ensure that this always happens, correct? It might be more informative to note that as well.
Attachment #465182 - Flags: review?(dmandelin) → review+
Error in previous comment was caused by a thinko (too many tc flags floating around), all tests out with that bit changed to check once per function at the end of LeaveFunction.
Attachment #465176 - Attachment is obsolete: true
Attachment #465477 - Flags: review?(dmandelin)
Comment on attachment 465402 [details] [diff] [review]
3: Use name analysis information to synthesize an arguments access at the start of functions which might modify parameters

Previous r+ and remark on comment about the create-argsobj-before-modify-param was meant to apply to this patch.
Attachment #465402 - Flags: review?(dmandelin) → review+
Attachment #465182 - Flags: review+ → review?(dmandelin)
Attachment #465477 - Flags: review?(dmandelin) → review+
Attachment #465407 - Flags: review?(dmandelin) → review+
Attachment #465182 - Flags: review?(dmandelin) → review+
Comment on attachment 465477 [details] [diff] [review]
2: Analyze parameter assignments and eval calls, record for use later

Given this code in LeaveFunction:

>+    /*
>+     * Check whether any parameters have been assigned within this function.
>+     * In strict mode parameters do not alias arguments[i], and to make the
>+     * arguments object reflect initial parameter values prior to any mutation
>+     * we create it eagerly whenever parameters are (or might, in the case of
>+     * calls to eval) be assigned.
>+     */
>+    if (funtc->inStrictMode() && funbox->object->getFunctionPrivate()->nargs > 0) {
>+        JSAtomListIterator iter(&funtc->decls);
>+        JSAtomListElement *ale;
>+
>+        while ((ale = iter()) != NULL) {
>+            JSDefinition *dn = ALE_DEFN(ale);
>+            if (dn->kind() == JSDefinition::ARG && dn->isAssigned()) {
>+                funbox->tcflags |= TCF_FUN_MUTATES_PARAMETER;
>+                break;
>+            }
>+        }
>+    }

Why do you need any changes at all in NoveLValue?

/be
A good point, I don't -- removed it, ran tests, all seems well, will push with that removed.
http://hg.mozilla.org/mozilla-central/rev/76f962d07ce0
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Depends on: 620750
Depends on: 632924
Depends on: 644015
You need to log in before you can comment on or make changes to this bug.