Bug 842522 - Don't force construction of arguments objects in the presence of dynamic name accesses, r=luke.
authorBrian Hackett <bhackett1024@gmail.com>
Wed, 20 Feb 2013 04:54:13 -0700
changeset 122445 e3b899354a6fde7353da0f3627064940dc19e7ce
parent 122444 4b6b0d48930130b9da23e9776495d4cd0423bb1b
child 122446 95363d69b570281e89ad6e1be74e0b266d6c9962
push id24342
push userryanvm@gmail.com
push dateThu, 21 Feb 2013 13:05:06 +0000
treeherdermozilla-central@702d2814efbf [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersluke
bugs842522
milestone22.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 842522 - Don't force construction of arguments objects in the presence of dynamic name accesses, r=luke.
js/src/frontend/BytecodeCompiler.cpp
js/src/frontend/Parser.cpp
js/src/frontend/SharedContext.h
js/src/ion/Bailouts.cpp
js/src/jit-test/tests/arguments/dynamicBindings.js
js/src/jsanalyze.cpp
js/src/jsscript.cpp
js/src/jsscriptinlines.h
--- a/js/src/frontend/BytecodeCompiler.cpp
+++ b/js/src/frontend/BytecodeCompiler.cpp
@@ -189,23 +189,50 @@ frontend::CompileScript(JSContext *cx, H
             return UnrootedScript(NULL);
 
         parser.freeTree(pn);
     }
 
     if (!SetSourceMap(cx, tokenStream, ss, script))
         return UnrootedScript(NULL);
 
-    // It's an error to use |arguments| in a function that has a rest parameter.
-    if (callerFrame && callerFrame.isFunctionFrame() && callerFrame.fun()->hasRest()) {
+    if (callerFrame && callerFrame.isFunctionFrame()) {
         HandlePropertyName arguments = cx->names().arguments;
         for (AtomDefnRange r = pc.lexdeps->all(); !r.empty(); r.popFront()) {
             if (r.front().key() == arguments) {
-                parser.reportError(NULL, JSMSG_ARGUMENTS_AND_REST);
-                return UnrootedScript(NULL);
+                if (callerFrame.fun()->hasRest()) {
+                    // It's an error to use |arguments| in a function that has
+                    // a rest parameter.
+                    parser.reportError(NULL, JSMSG_ARGUMENTS_AND_REST);
+                    return UnrootedScript(NULL);
+                }
+                // Force construction of arguments objects for functions that
+                // use 'arguments' within an eval.
+                RootedScript script(cx, callerFrame.fun()->nonLazyScript());
+                if (script->argumentsHasVarBinding()) {
+                    if (!JSScript::argumentsOptimizationFailed(cx, script))
+                        return UnrootedScript(NULL);
+                }
+            }
+        }
+
+        // If the eval'ed script contains any debugger statement, force construction
+        // of arguments objects for the caller script and any other scripts it is
+        // transitively nested inside.
+        if (pc.sc->hasDebuggerStatement()) {
+            RootedObject scope(cx, callerFrame.scopeChain());
+            while (scope->isScope() || scope->isDebugScope()) {
+                if (scope->isCall() && !scope->asCall().isForEval()) {
+                    RootedScript script(cx, scope->asCall().callee().nonLazyScript());
+                    if (script->argumentsHasVarBinding()) {
+                        if (!JSScript::argumentsOptimizationFailed(cx, script))
+                            return UnrootedScript(NULL);
+                    }
+                }
+                scope = scope->enclosingScope();
             }
         }
     }
 
     /*
      * Nowadays the threaded interpreter needs a stop instruction, so we
      * do have to emit that here.
      */
--- a/js/src/frontend/Parser.cpp
+++ b/js/src/frontend/Parser.cpp
@@ -880,18 +880,33 @@ Parser::functionBody(FunctionBodyType ty
      * Now that all possible 'arguments' bindings have been added, note whether
      * 'arguments' has a local binding and whether it unconditionally needs an
      * arguments object. (Also see the flags' comments in ContextFlags.)
      */
     if (argumentsHasLocalBinding) {
         FunctionBox *funbox = pc->sc->asFunctionBox();
         funbox->setArgumentsHasLocalBinding();
 
-        /* Dynamic scope access destroys all hope of optimization. */
-        if (pc->sc->bindingsAccessedDynamically())
+        /*
+         * If a script has both explicit mentions of 'arguments' and dynamic
+         * name lookups which could access the arguments, an arguments object
+         * must be created eagerly. The SSA analysis used for lazy arguments
+         * cannot cope with dynamic name accesses, so any 'arguments' accessed
+         * via a NAME opcode must force construction of the arguments object.
+         */
+        if (pc->sc->bindingsAccessedDynamically() && maybeArgDef)
+            funbox->setDefinitelyNeedsArgsObj();
+
+        /*
+         * If a script contains the debugger statement either directly or
+         * within an inner function, the arguments object must be created
+         * eagerly. The debugger can walk the scope chain and observe any
+         * values along it.
+         */
+        if (pc->sc->hasDebuggerStatement())
             funbox->setDefinitelyNeedsArgsObj();
 
         /*
          * 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.
@@ -902,16 +917,19 @@ Parser::functionBody(FunctionBodyType ty
                 for (DefinitionList::Range dr = dlist.all(); !dr.empty(); dr.popFront()) {
                     Definition *dn = dr.front();
                     if (dn->kind() == Definition::ARG && dn->isAssigned()) {
                         funbox->setDefinitelyNeedsArgsObj();
                         goto exitLoop;
                     }
                 }
             }
+            /* Watch for mutation of arguments through e.g. eval(). */
+            if (pc->sc->bindingsAccessedDynamically())
+                funbox->setDefinitelyNeedsArgsObj();
           exitLoop: ;
         }
     }
 
     return pn;
 }
 
 // Create a placeholder Definition node for |atom|.
@@ -1764,16 +1782,18 @@ Parser::functionArgsAndBody(ParseNode *p
     /*
      * Fruit of the poisonous tree: if a closure contains a dynamic name access
      * (eval, with, etc), we consider the parent to do the same. The reason is
      * that the deoptimizing effects of dynamic name access apply equally to
      * parents: any local can be read at runtime.
      */
     if (funbox->bindingsAccessedDynamically())
         outerpc->sc->setBindingsAccessedDynamically();
+    if (funbox->hasDebuggerStatement())
+        outerpc->sc->setHasDebuggerStatement();
 
 #if JS_HAS_DESTRUCTURING
     /*
      * If there were destructuring formal parameters, prepend the initializing
      * comma expression that we synthesized to body. If the body is a return
      * node, we must make a special PNK_SEQ node, to prepend the destructuring
      * code without bracing the decompilation of the function body.
      */
@@ -1806,17 +1826,18 @@ Parser::functionArgsAndBody(ParseNode *p
 #endif
 
     /*
      * If any nested function scope does a dynamic scope access, all enclosing
      * scopes may be accessed dynamically.
      */
     if (funbox->bindingsAccessedDynamically())
         outerpc->sc->setBindingsAccessedDynamically();
-
+    if (funbox->hasDebuggerStatement())
+        outerpc->sc->setHasDebuggerStatement();
 
     pn->pn_funbox = funbox;
     pn->pn_body->append(body);
     pn->pn_body->pn_pos = body->pn_pos;
     pn->pn_blockid = outerpc->blockid();
 
     if (!LeaveFunction(pn, this, funName, kind))
         return false;
@@ -4015,16 +4036,17 @@ Parser::statement()
             return NULL;
         return pn;
 
       case TOK_DEBUGGER:
         pn = new_<DebuggerStatement>(tokenStream.currentToken().pos);
         if (!pn)
             return NULL;
         pc->sc->setBindingsAccessedDynamically();
+        pc->sc->setHasDebuggerStatement();
         break;
 
       case TOK_ERROR:
         return NULL;
 
       default:
         return expressionStatement();
     }
--- a/js/src/frontend/SharedContext.h
+++ b/js/src/frontend/SharedContext.h
@@ -50,20 +50,26 @@ class AnyContextFlags
     // binding access since it does not go through the normal name lookup
     // mechanism. This is debatable and could be changed (although care must be
     // taken not to turn off the whole 'arguments' optimization). To answer the
     // more general "is this argument aliased" question, script->needsArgsObj
     // should be tested (see JSScript::argIsAlised).
     //
     bool            bindingsAccessedDynamically:1;
 
+    // Whether this script, or any of its inner scripts contains a debugger
+    // statement which could potentially read or write anywhere along the
+    // scope chain.
+    bool            hasDebuggerStatement:1;
+
   public:
     AnyContextFlags()
      :  hasExplicitUseStrict(false),
-        bindingsAccessedDynamically(false)
+        bindingsAccessedDynamically(false),
+        hasDebuggerStatement(false)
     { }
 };
 
 class FunctionContextFlags
 {
     // This class's data is all private and so only visible to these friends.
     friend class FunctionBox;
 
@@ -151,19 +157,21 @@ class SharedContext
     inline bool isModuleBox() { return toObjectBox() && toObjectBox()->isModuleBox(); }
     inline bool isFunctionBox() { return toObjectBox() && toObjectBox()->isFunctionBox(); }
     inline GlobalSharedContext *asGlobalSharedContext();
     inline ModuleBox *asModuleBox();
     inline FunctionBox *asFunctionBox();
 
     bool hasExplicitUseStrict()        const { return anyCxFlags.hasExplicitUseStrict; }
     bool bindingsAccessedDynamically() const { return anyCxFlags.bindingsAccessedDynamically; }
+    bool hasDebuggerStatement()        const { return anyCxFlags.hasDebuggerStatement; }
 
     void setExplicitUseStrict()           { anyCxFlags.hasExplicitUseStrict        = true; }
     void setBindingsAccessedDynamically() { anyCxFlags.bindingsAccessedDynamically = true; }
+    void setHasDebuggerStatement()        { anyCxFlags.hasDebuggerStatement        = true; }
 
     // JSOPTION_STRICT warnings or strict mode errors.
     inline bool needStrictChecks();
 };
 
 class GlobalSharedContext : public SharedContext
 {
   private:
--- a/js/src/ion/Bailouts.cpp
+++ b/js/src/ion/Bailouts.cpp
@@ -610,24 +610,21 @@ ion::ThunkToInterpreter(Value *vp)
                 // arguments object, so the frame should not have any argument
                 // object yet.
                 JS_ASSERT(!fp->hasArgsObj());
                 ArgumentsObject *argsobj = ArgumentsObject::createExpected(cx, fp);
                 if (!argsobj) {
                     resumeMode = JSINTERP_RETHROW;
                     break;
                 }
-                InternalBindingsHandle bindings(script, &script->bindings);
-                const unsigned var = Bindings::argumentsVarIndex(cx, bindings);
                 // The arguments is a local binding and needsArgsObj does not
                 // check if it is clobbered. Ensure that the local binding
                 // restored during bailout before storing the arguments object
                 // to the slot.
-                if (fp->unaliasedLocal(var).isMagic(JS_OPTIMIZED_ARGUMENTS))
-                    fp->unaliasedLocal(var) = ObjectValue(*argsobj);
+                SetFrameArgumentsObject(cx, fp, script, argsobj);
             }
             ++iter;
         } while (fp != br->entryfp());
     }
 
     if (activation->entryfp() == br->entryfp()) {
         // If the bailout entry fp is the same as the activation entryfp, then
         // there are no scripted frames below us. In this case, just shortcut
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/arguments/dynamicBindings.js
@@ -0,0 +1,32 @@
+
+function testEval(x, y) {
+  x = 5;
+  eval("arguments[0] += 10");
+  assertEq(x, 15);
+}
+for (var i = 0; i < 5; i++)
+  testEval(3);
+
+function testEvalWithArguments(x, y) {
+  eval("arguments[0] += 10");
+  assertEq(arguments[y], 13);
+}
+for (var i = 0; i < 5; i++)
+  testEvalWithArguments(3, 0);
+
+function testNestedEval(x, y) {
+  x = 5;
+  eval("eval('arguments[0] += 10')");
+  assertEq(x, 15);
+}
+for (var i = 0; i < 5; i++)
+  testNestedEval(3);
+
+function testWith(x, y) {
+  with ({}) {
+    arguments[0] += 10;
+    assertEq(x, 13);
+  }
+}
+for (var i = 0; i < 5; i++)
+  testWith(3);
--- a/js/src/jsanalyze.cpp
+++ b/js/src/jsanalyze.cpp
@@ -1904,25 +1904,44 @@ ScriptAnalysis::needsArgsObj(JSContext *
 }
 
 bool
 ScriptAnalysis::needsArgsObj(JSContext *cx)
 {
     JS_ASSERT(script_->argumentsHasVarBinding());
 
     /*
-     * Since let variables and dynamic name access are not tracked, we cannot
-     * soundly perform this analysis in their presence. Generators can be
-     * suspended when the speculation fails, so disallow it also.
+     * If the script has dynamic name accesses which could reach 'arguments',
+     * the parser will already have checked to ensure there are no explicit
+     * uses of 'arguments' in the function. If there are such uses, the script
+     * will be marked as definitely needing an arguments object.
+     *
+     * New accesses on 'arguments' can occur through 'eval' or the debugger
+     * statement. In the former case, we will dynamically detect the use and
+     * mark the arguments optimization as having failed.
      */
-    if (script_->bindingsAccessedDynamically || script_->funHasAnyAliasedFormal ||
-        localsAliasStack() || cx->compartment->debugMode() || script_->isGenerator)
-    {
+    if (script_->bindingsAccessedDynamically)
+        return false;
+
+    /*
+     * Since let variables and are not tracked, we cannot soundly perform this
+     * analysis in their presence. Generators can be suspended when the
+     * speculation fails, so disallow it also.
+     */
+    if (localsAliasStack() || cx->compartment->debugMode() || script_->isGenerator)
         return true;
-    }
+
+    /*
+     * If a script has explicit mentions of 'arguments' and formals which may
+     * be stored as part of a call object, don't use lazy arguments. The
+     * compiler can then assume that accesses through arguments[i] will be on
+     * unaliased variables.
+     */
+    if (script_->funHasAnyAliasedFormal)
+        return true;
 
     unsigned pcOff = script_->argumentsBytecode() - script_->code;
 
     SeenVector seen(cx);
     return needsArgsObj(cx, seen, SSAValue::PushedValue(pcOff, 0));
 }
 
 CrossSSAValue
--- a/js/src/jsscript.cpp
+++ b/js/src/jsscript.cpp
@@ -2792,49 +2792,81 @@ void
 JSScript::setNeedsArgsObj(bool needsArgsObj)
 {
     JS_ASSERT(!analyzedArgsUsage());
     JS_ASSERT_IF(needsArgsObj, argumentsHasVarBinding());
     needsArgsAnalysis_ = false;
     needsArgsObj_ = needsArgsObj;
 }
 
+void
+js::SetFrameArgumentsObject(JSContext *cx, AbstractFramePtr frame,
+                            HandleScript script, JSObject *argsobj)
+{
+    /*
+     * Replace any optimized arguments in the frame with an explicit arguments
+     * object. Note that 'arguments' may have already been overwritten.
+     */
+
+    InternalBindingsHandle bindings(script, &script->bindings);
+    const unsigned var = Bindings::argumentsVarIndex(cx, bindings);
+
+    if (script->varIsAliased(var)) {
+        /*
+         * Scan the script to find the slot in the call object that 'arguments'
+         * is assigned to.
+         */
+        jsbytecode *pc = script->code;
+        while (*pc != JSOP_ARGUMENTS)
+            pc += GetBytecodeLength(pc);
+        pc += JSOP_ARGUMENTS_LENGTH;
+        JS_ASSERT(*pc == JSOP_SETALIASEDVAR);
+
+        if (frame.callObj().asScope().aliasedVar(pc).isMagic(JS_OPTIMIZED_ARGUMENTS))
+            frame.callObj().asScope().setAliasedVar(pc, ObjectValue(*argsobj));
+    } else {
+        if (frame.unaliasedLocal(var).isMagic(JS_OPTIMIZED_ARGUMENTS))
+            frame.unaliasedLocal(var) = ObjectValue(*argsobj);
+    }
+}
+
 /* static */ bool
 JSScript::argumentsOptimizationFailed(JSContext *cx, HandleScript script)
 {
     AssertCanGC();
+    JS_ASSERT(script->function());
     JS_ASSERT(script->analyzedArgsUsage());
     JS_ASSERT(script->argumentsHasVarBinding());
-    JS_ASSERT(!script->isGenerator);
 
     /*
-     * It is possible that the apply speculation has already failed, everything
-     * has been fixed up, but there was an outstanding magic value on the
-     * stack that has just now flowed into an apply. In this case, there is
-     * nothing to do; GuardFunApplySpeculation will patch in the real argsobj.
+     * It is possible that the arguments optimization has already failed,
+     * everything has been fixed up, but there was an outstanding magic value
+     * on the stack that has just now flowed into an apply. In this case, there
+     * is nothing to do; GuardFunApplySpeculation will patch in the real
+     * argsobj.
      */
     if (script->needsArgsObj())
         return true;
 
+    JS_ASSERT(!script->isGenerator);
+
     script->needsArgsObj_ = true;
 
-    InternalBindingsHandle bindings(script, &script->bindings);
-    const unsigned var = Bindings::argumentsVarIndex(cx, bindings);
-
     /*
-     * By design, the apply-arguments optimization is only made when there
-     * are no outstanding cases of MagicValue(JS_OPTIMIZED_ARGUMENTS) other
-     * than this particular invocation of 'f.apply(x, arguments)'. Thus, there
-     * are no outstanding values of MagicValue(JS_OPTIMIZED_ARGUMENTS) on the
-     * stack. However, there are three things that need fixup:
+     * By design, the arguments optimization is only made when there are no
+     * outstanding cases of MagicValue(JS_OPTIMIZED_ARGUMENTS) at any points
+     * where the optimization could fail, other than an active invocation of
+     * 'f.apply(x, arguments)'. Thus, there are no outstanding values of
+     * MagicValue(JS_OPTIMIZED_ARGUMENTS) on the stack. However, there are
+     * three things that need fixup:
      *  - there may be any number of activations of this script that don't have
      *    an argsObj that now need one.
      *  - jit code compiled (and possible active on the stack) with the static
      *    assumption of !script->needsArgsObj();
-     *  - type inference data for the script assuming script->needsArgsObj; and
+     *  - type inference data for the script assuming script->needsArgsObj
      */
     for (AllFramesIter i(cx->runtime); !i.done(); ++i) {
         /*
          * We cannot reliably create an arguments object for Ion activations of
          * this script.  To maintain the invariant that "script->needsArgsObj
          * implies fp->hasArgsObj", the Ion bail mechanism will create an
          * arguments object right after restoring the StackFrame and before
          * entering the interpreter (in ion::ThunkToInterpreter).  This delay is
@@ -2851,19 +2883,17 @@ JSScript::argumentsOptimizationFailed(JS
                  * We can't leave stack frames with script->needsArgsObj but no
                  * arguments object. It is, however, safe to leave frames with
                  * an arguments object but !script->needsArgsObj.
                  */
                 script->needsArgsObj_ = false;
                 return false;
             }
 
-            /* Note: 'arguments' may have already been overwritten. */
-            if (frame.unaliasedLocal(var).isMagic(JS_OPTIMIZED_ARGUMENTS))
-                frame.unaliasedLocal(var) = ObjectValue(*argsobj);
+            SetFrameArgumentsObject(cx, frame, script, argsobj);
         }
     }
 
 #ifdef JS_METHODJIT
     if (script->hasMJITInfo()) {
         mjit::ExpandInlineFrames(cx->compartment);
         mjit::Recompiler::clearStackReferences(cx->runtime->defaultFreeOp(), script);
         mjit::ReleaseScriptCode(cx->runtime->defaultFreeOp(), script);
--- a/js/src/jsscriptinlines.h
+++ b/js/src/jsscriptinlines.h
@@ -84,16 +84,20 @@ MarkScriptBytecode(JSRuntime *rt, const 
      * As an invariant, a ScriptBytecodeEntry should not be 'marked' outside of
      * a GC. Since SweepScriptBytecodes is only called during a full gc,
      * to preserve this invariant, only mark during a full gc.
      */
     if (rt->gcIsFull)
         SharedScriptData::fromBytecode(bytecode)->marked = true;
 }
 
+void
+SetFrameArgumentsObject(JSContext *cx, AbstractFramePtr frame,
+                        HandleScript script, JSObject *argsobj);
+
 } // namespace js
 
 inline void
 JSScript::setFunction(JSFunction *fun)
 {
     function_ = fun;
 }