Strict and sloppy functions shouldn't share structure
authorjfbastien@apple.com <jfbastien@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 29 Nov 2017 17:46:26 +0000 (17:46 +0000)
committerjfbastien@apple.com <jfbastien@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 29 Nov 2017 17:46:26 +0000 (17:46 +0000)
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@225273 268f45cc-cd09-0410-ab3c-d52691b4dbfc

17 files changed:
JSTests/ChangeLog
JSTests/stress/get-by-id-strict-arguments.js [new file with mode: 0644]
JSTests/stress/get-by-id-strict-callee.js [new file with mode: 0644]
JSTests/stress/get-by-id-strict-caller.js [new file with mode: 0644]
JSTests/stress/get-by-id-strict-nested-arguments-2.js [new file with mode: 0644]
JSTests/stress/get-by-id-strict-nested-arguments.js [new file with mode: 0644]
JSTests/stress/strict-function-structure.js [new file with mode: 0644]
JSTests/stress/strict-nested-function-structure.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h
Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp
Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp
Source/JavaScriptCore/runtime/FunctionConstructor.cpp
Source/JavaScriptCore/runtime/JSFunction.cpp
Source/JavaScriptCore/runtime/JSFunctionInlines.h
Source/JavaScriptCore/runtime/JSGlobalObject.cpp
Source/JavaScriptCore/runtime/JSGlobalObject.h

index b384c71..46fe130 100644 (file)
@@ -1,3 +1,52 @@
+2017-11-28  JF Bastien  <jfbastien@apple.com>
+
+        Strict and sloppy functions shouldn't share structure
+        https://bugs.webkit.org/show_bug.cgi?id=180103
+        <rdar://problem/35667847>
+
+        Reviewed by Saam Barati.
+
+        * stress/get-by-id-strict-arguments.js: Added. Used to not throw
+        because the IC was wrong.
+        (foo):
+        (bar):
+        (baz):
+        (catch):
+        * stress/get-by-id-strict-callee.js: Added. Not strictly necessary
+        in this patch, but may as well test odd strict mode corner cases.
+        (bar):
+        (baz):
+        (catch):
+        * stress/get-by-id-strict-caller.js: Added. Also IC'd wrong.
+        (foo):
+        (bar):
+        (baz):
+        (catch):
+        * stress/get-by-id-strict-nested-arguments-2.js: Added. Same as
+        next file, but with invalidation of the FunctionExecutable's
+        singletonFunction() to hit SpeculativeJIT::compileNewFunction's
+        slower path.
+        (foo):
+        (bar.const.x):
+        (bar.const.y):
+        (bar):
+        (catch):
+        * stress/get-by-id-strict-nested-arguments.js: Added. Make sure
+        strict nesting works correctly.
+        (foo):
+        (bar.baz):
+        (bar):
+        * stress/strict-function-structure.js: Added. The test used to
+        assert in objectProtoFuncHasOwnProperty.
+        (foo):
+        (bar):
+        (baz):
+        * stress/strict-nested-function-structure.js: Added. Nesting.
+        (foo):
+        (bar):
+        (baz.boo):
+        (baz):
+
 2017-11-29  Robin Morisset  <rmorisset@apple.com>
 
         The recursive tail call optimisation is wrong on closures
diff --git a/JSTests/stress/get-by-id-strict-arguments.js b/JSTests/stress/get-by-id-strict-arguments.js
new file mode 100644 (file)
index 0000000..f5607f4
--- /dev/null
@@ -0,0 +1,28 @@
+let warm = 1000;
+
+function foo(f) {
+    return f.arguments;
+}
+noInline(foo);
+
+function bar() {
+    for (let i = 0; i < warm; ++i)
+        foo(bar);
+}
+function baz() {
+    "use strict";
+    foo(baz);
+}
+
+bar();
+
+let caught = false;
+
+try {
+    baz();
+} catch (e) {
+    caught = true;
+}
+
+if (!caught)
+    throw new Error(`bad!`);
diff --git a/JSTests/stress/get-by-id-strict-callee.js b/JSTests/stress/get-by-id-strict-callee.js
new file mode 100644 (file)
index 0000000..929d65e
--- /dev/null
@@ -0,0 +1,24 @@
+let warm = 1000;
+
+function bar() {
+    for (let i = 0; i < warm; ++i)
+        arguments.callee;
+}
+
+function baz() {
+    "use strict";
+    arguments.callee;
+}
+
+bar();
+
+let caught = false;
+
+try {
+    baz();
+} catch (e) {
+    caught = true;
+}
+
+if (!caught)
+    throw new Error(`bad!`);
diff --git a/JSTests/stress/get-by-id-strict-caller.js b/JSTests/stress/get-by-id-strict-caller.js
new file mode 100644 (file)
index 0000000..3462ec0
--- /dev/null
@@ -0,0 +1,28 @@
+let warm = 1000;
+
+function foo(f) {
+    return f.caller;
+}
+noInline(foo);
+
+function bar() {
+    for (let i = 0; i < warm; ++i)
+        foo(bar);
+}
+function baz() {
+    "use strict";
+    foo(baz);
+}
+
+bar();
+
+let caught = false;
+
+try {
+    baz();
+} catch (e) {
+    caught = true;
+}
+
+if (!caught)
+    throw new Error(`bad!`);
diff --git a/JSTests/stress/get-by-id-strict-nested-arguments-2.js b/JSTests/stress/get-by-id-strict-nested-arguments-2.js
new file mode 100644 (file)
index 0000000..fdfc9a8
--- /dev/null
@@ -0,0 +1,42 @@
+let warm = 1000;
+
+function foo(f) {
+    return f.arguments;
+}
+noInline(foo);
+
+let caught = 0;
+
+function bar() {
+    for (let i = 0; i < warm; ++i)
+        foo(bar);
+    const x = function baz1() { "use strict"; return 42; };
+    const y = function baz2() { "use strict"; return 0xc0defefe; };
+    return [x, y];
+}
+
+bar();
+bar();
+const [baz1, baz2] = bar();
+
+
+if (baz1() !== 42)
+    throw new Error(`bad!`);
+
+if (baz2() !== 0xc0defefe)
+    throw new Error(`bad!`);
+
+try {
+    foo(baz1);
+} catch (e) {
+    ++caught;
+}
+
+try {
+    foo(baz2);
+} catch (e) {
+    ++caught;
+}
+
+if (caught !== 2)
+    throw new Error(`bad!`);
diff --git a/JSTests/stress/get-by-id-strict-nested-arguments.js b/JSTests/stress/get-by-id-strict-nested-arguments.js
new file mode 100644 (file)
index 0000000..0d3ceed
--- /dev/null
@@ -0,0 +1,27 @@
+let warm = 1000;
+
+function foo(f) {
+    return f.arguments;
+}
+noInline(foo);
+
+let caught = false;
+
+function bar() {
+    for (let i = 0; i < warm; ++i)
+        foo(bar);
+    function baz() {
+        "use strict";
+        try {
+            foo(baz);
+        } catch (e) {
+            caught = true;
+        }
+    }
+    baz();
+}
+
+bar();
+
+if (!caught)
+    throw new Error(`bad!`);
diff --git a/JSTests/stress/strict-function-structure.js b/JSTests/stress/strict-function-structure.js
new file mode 100644 (file)
index 0000000..2d5ef71
--- /dev/null
@@ -0,0 +1,8 @@
+function foo(f) { f.hasOwnProperty("arguments"); }
+noInline(foo);
+
+function bar() {}
+foo(bar);
+
+function baz() { "use strict"; }
+foo(baz);
diff --git a/JSTests/stress/strict-nested-function-structure.js b/JSTests/stress/strict-nested-function-structure.js
new file mode 100644 (file)
index 0000000..ef9d235
--- /dev/null
@@ -0,0 +1,12 @@
+function foo(f) { f.hasOwnProperty("arguments"); }
+noInline(foo);
+
+function bar() {}
+foo(bar);
+
+function baz() {
+    "use strict";
+    function boo() {}
+    return boo;
+}
+foo(baz());
index ddfb7d1..9577723 100644 (file)
@@ -1,3 +1,38 @@
+2017-11-28  JF Bastien  <jfbastien@apple.com>
+
+        Strict and sloppy functions shouldn't share structure
+        https://bugs.webkit.org/show_bug.cgi?id=180103
+        <rdar://problem/35667847>
+
+        Reviewed by Saam Barati.
+
+        Sloppy and strict functions don't act the same when it comes to
+        arguments, caller, and callee. Sharing a structure means that
+        anything that is cached gets shared, and that's incorrect.
+
+        * dfg/DFGAbstractInterpreterInlines.h:
+        (JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects):
+        * dfg/DFGSpeculativeJIT.cpp:
+        (JSC::DFG::SpeculativeJIT::compileNewFunction):
+        * ftl/FTLLowerDFGToB3.cpp:
+        (JSC::FTL::DFG::LowerDFGToB3::compileNewFunction):
+        * runtime/FunctionConstructor.cpp:
+        (JSC::constructFunctionSkippingEvalEnabledCheck):
+        * runtime/JSFunction.cpp:
+        (JSC::JSFunction::create): the second ::create is always strict
+        because it applies to native functions.
+        * runtime/JSFunctionInlines.h:
+        (JSC::JSFunction::createWithInvalidatedReallocationWatchpoint):
+        * runtime/JSGlobalObject.cpp:
+        (JSC::JSGlobalObject::init):
+        (JSC::JSGlobalObject::visitChildren):
+        * runtime/JSGlobalObject.h:
+        (JSC::JSGlobalObject::strictFunctionStructure const):
+        (JSC::JSGlobalObject::sloppyFunctionStructure const):
+        (JSC::JSGlobalObject::nativeStdFunctionStructure const):
+        (JSC::JSGlobalObject::functionStructure const): Deleted. Renamed.
+        (JSC::JSGlobalObject::namedFunctionStructure const): Deleted. Drive-by, unused.
+
 2017-11-29  Yusuke Suzuki  <utatane.tea@gmail.com>
 
         [JSC] Add MacroAssembler::getEffectiveAddress in all platforms
index 8b78e00..8978c63 100644 (file)
@@ -2298,8 +2298,13 @@ bool AbstractInterpreter<AbstractStateType>::executeEffects(unsigned clobberLimi
         break;
 
     case NewFunction:
-        forNode(node).set(
-            m_graph, m_codeBlock->globalObjectFor(node->origin.semantic)->functionStructure());
+        if (node->castOperand<FunctionExecutable*>()->isStrictMode()) {
+            forNode(node).set(
+                m_graph, m_codeBlock->globalObjectFor(node->origin.semantic)->strictFunctionStructure());
+        } else {
+            forNode(node).set(
+                m_graph, m_codeBlock->globalObjectFor(node->origin.semantic)->sloppyFunctionStructure());
+        }
         break;
         
     case GetCallee:
index 040a9da..60be911 100644 (file)
@@ -6781,10 +6781,22 @@ void SpeculativeJIT::compileNewFunction(Node* node)
     }
 
     RegisteredStructure structure = m_jit.graph().registerStructure(
-        nodeType == NewGeneratorFunction ? m_jit.graph().globalObjectFor(node->origin.semantic)->generatorFunctionStructure() :
-        nodeType == NewAsyncFunction ? m_jit.graph().globalObjectFor(node->origin.semantic)->asyncFunctionStructure() :
-        nodeType == NewAsyncGeneratorFunction ? m_jit.graph().globalObjectFor(node->origin.semantic)->asyncGeneratorFunctionStructure() :
-        m_jit.graph().globalObjectFor(node->origin.semantic)->functionStructure());
+        [&] () {
+            switch (nodeType) {
+            case NewGeneratorFunction:
+                return m_jit.graph().globalObjectFor(node->origin.semantic)->generatorFunctionStructure();
+            case NewAsyncFunction:
+                return m_jit.graph().globalObjectFor(node->origin.semantic)->asyncFunctionStructure();
+            case NewAsyncGeneratorFunction:
+                return m_jit.graph().globalObjectFor(node->origin.semantic)->asyncGeneratorFunctionStructure();
+            case NewFunction:
+                if (node->castOperand<FunctionExecutable*>()->isStrictMode())
+                    return m_jit.graph().globalObjectFor(node->origin.semantic)->strictFunctionStructure();
+                return m_jit.graph().globalObjectFor(node->origin.semantic)->sloppyFunctionStructure();
+            default:
+                RELEASE_ASSERT_NOT_REACHED();
+            }
+        }());
     
     GPRTemporary result(this);
     GPRTemporary scratch1(this);
index fd51cbc..97999ac 100644 (file)
@@ -4737,11 +4737,25 @@ private:
             return;
         }
         
+
         RegisteredStructure structure = m_graph.registerStructure(
-            isGeneratorFunction ? m_graph.globalObjectFor(m_node->origin.semantic)->generatorFunctionStructure() :
-            isAsyncFunction ? m_graph.globalObjectFor(m_node->origin.semantic)->asyncFunctionStructure() :
-            isAsyncGeneratorFunction ? m_graph.globalObjectFor(m_node->origin.semantic)->asyncGeneratorFunctionStructure() :
-            m_graph.globalObjectFor(m_node->origin.semantic)->functionStructure());
+            [&] () {
+                switch (m_node->op()) {
+                case NewGeneratorFunction:
+                    return m_graph.globalObjectFor(m_node->origin.semantic)->generatorFunctionStructure();
+                case NewAsyncFunction:
+                    return m_graph.globalObjectFor(m_node->origin.semantic)->asyncFunctionStructure();
+                case NewAsyncGeneratorFunction:
+                    return m_graph.globalObjectFor(m_node->origin.semantic)->asyncGeneratorFunctionStructure();
+                case NewFunction:
+                    if (m_node->castOperand<FunctionExecutable*>()->isStrictMode())
+                        return m_graph.globalObjectFor(m_node->origin.semantic)->strictFunctionStructure();
+                    return m_graph.globalObjectFor(m_node->origin.semantic)->sloppyFunctionStructure();
+                    break;
+                default:
+                    RELEASE_ASSERT_NOT_REACHED();
+                }
+            }());
         
         LBasicBlock slowPath = m_out.newBlock();
         LBasicBlock continuation = m_out.newBlock();
index d803616..77e6c18 100644 (file)
@@ -84,22 +84,17 @@ JSObject* constructFunctionSkippingEvalEnabledCheck(
     auto scope = DECLARE_THROW_SCOPE(vm);
 
     const char* prefix = nullptr;
-    Structure* structure = nullptr;
     switch (functionConstructionMode) {
     case FunctionConstructionMode::Function:
-        structure = globalObject->functionStructure();
         prefix = "function ";
         break;
     case FunctionConstructionMode::Generator:
-        structure = globalObject->generatorFunctionStructure();
         prefix = "function *";
         break;
     case FunctionConstructionMode::Async:
-        structure = globalObject->asyncFunctionStructure();
         prefix = "async function ";
         break;
     case FunctionConstructionMode::AsyncGenerator:
-        structure = globalObject->asyncGeneratorFunctionStructure();
         prefix = "{async function*";
         break;
     }
@@ -177,6 +172,25 @@ JSObject* constructFunctionSkippingEvalEnabledCheck(
         return throwException(exec, scope, exception);
     }
 
+    Structure* structure = nullptr;
+    switch (functionConstructionMode) {
+    case FunctionConstructionMode::Function:
+        if (function->isStrictMode())
+            structure = globalObject->strictFunctionStructure();
+        else
+            structure = globalObject->sloppyFunctionStructure();
+        break;
+    case FunctionConstructionMode::Generator:
+        structure = globalObject->generatorFunctionStructure();
+        break;
+    case FunctionConstructionMode::Async:
+        structure = globalObject->asyncFunctionStructure();
+        break;
+    case FunctionConstructionMode::AsyncGenerator:
+        structure = globalObject->asyncGeneratorFunctionStructure();
+        break;
+    }
+
     Structure* subclassStructure = InternalFunction::createSubclassStructure(exec, newTarget, structure);
     RETURN_IF_EXCEPTION(scope, nullptr);
 
index 3686ff3..071c57d 100644 (file)
@@ -67,7 +67,8 @@ bool JSFunction::isHostFunctionNonInline() const
 
 JSFunction* JSFunction::create(VM& vm, FunctionExecutable* executable, JSScope* scope)
 {
-    return create(vm, executable, scope, scope->globalObject(vm)->functionStructure());
+    Structure* structure = executable->isStrictMode() ? scope->globalObject(vm)->strictFunctionStructure() : scope->globalObject(vm)->sloppyFunctionStructure();
+    return create(vm, executable, scope, structure);
 }
 
 JSFunction* JSFunction::create(VM& vm, FunctionExecutable* executable, JSScope* scope, Structure* structure)
@@ -80,7 +81,8 @@ JSFunction* JSFunction::create(VM& vm, FunctionExecutable* executable, JSScope*
 JSFunction* JSFunction::create(VM& vm, JSGlobalObject* globalObject, int length, const String& name, NativeFunction nativeFunction, Intrinsic intrinsic, NativeFunction nativeConstructor, const DOMJIT::Signature* signature)
 {
     NativeExecutable* executable = vm.getHostFunction(nativeFunction, intrinsic, nativeConstructor, signature, name);
-    JSFunction* function = new (NotNull, allocateCell<JSFunction>(vm.heap)) JSFunction(vm, globalObject, globalObject->functionStructure());
+    Structure* structure = globalObject->strictFunctionStructure();
+    JSFunction* function = new (NotNull, allocateCell<JSFunction>(vm.heap)) JSFunction(vm, globalObject, structure);
     // Can't do this during initialization because getHostFunction might do a GC allocation.
     function->finishCreation(vm, executable, length, name);
     return function;
index 263a00c..7f853ca 100644 (file)
@@ -35,7 +35,8 @@ inline JSFunction* JSFunction::createWithInvalidatedReallocationWatchpoint(
     VM& vm, FunctionExecutable* executable, JSScope* scope)
 {
     ASSERT(executable->singletonFunction()->hasBeenInvalidated());
-    return createImpl(vm, executable, scope, scope->globalObject(vm)->functionStructure());
+    Structure* structure = executable->isStrictMode() ? scope->globalObject(vm)->strictFunctionStructure() : scope->globalObject(vm)->sloppyFunctionStructure();
+    return createImpl(vm, executable, scope, structure);
 }
 
 inline JSFunction::JSFunction(VM& vm, FunctionExecutable* executable, JSScope* scope, Structure* structure)
index bd77b83..bcfc8a6 100644 (file)
@@ -386,7 +386,8 @@ void JSGlobalObject::init(VM& vm)
     ExecState::initGlobalExec(JSGlobalObject::globalExec(), globalCallee);
     ExecState* exec = JSGlobalObject::globalExec();
 
-    m_functionStructure.set(vm, this, JSFunction::createStructure(vm, this, m_functionPrototype.get()));
+    m_strictFunctionStructure.set(vm, this, JSFunction::createStructure(vm, this, m_functionPrototype.get()));
+    m_sloppyFunctionStructure.set(vm, this, JSFunction::createStructure(vm, this, m_functionPrototype.get()));
     m_customGetterSetterFunctionStructure.initLater(
         [] (const Initializer<Structure>& init) {
             init.set(JSCustomGetterSetterFunction::createStructure(init.vm, init.owner, init.owner->m_functionPrototype.get()));
@@ -400,10 +401,6 @@ void JSGlobalObject::init(VM& vm)
         [] (const Initializer<Structure>& init) {
             init.set(JSNativeStdFunction::createStructure(init.vm, init.owner, init.owner->m_functionPrototype.get()));
         });
-    m_namedFunctionStructure.initLater(
-        [] (const Initializer<Structure>& init) {
-            init.set(Structure::addPropertyTransition(init.vm, init.owner->m_functionStructure.get(), init.vm.propertyNames->name, PropertyAttribute::DontDelete | PropertyAttribute::ReadOnly | PropertyAttribute::DontEnum, init.owner->m_functionNameOffset));
-        });
     JSFunction* callFunction = nullptr;
     JSFunction* applyFunction = nullptr;
     JSFunction* hasInstanceSymbolFunction = nullptr;
@@ -1318,12 +1315,12 @@ void JSGlobalObject::visitChildren(JSCell* cell, SlotVisitor& visitor)
     thisObject->m_nullPrototypeObjectStructure.visit(visitor);
     visitor.append(thisObject->m_errorStructure);
     visitor.append(thisObject->m_calleeStructure);
-    visitor.append(thisObject->m_functionStructure);
+    visitor.append(thisObject->m_strictFunctionStructure);
+    visitor.append(thisObject->m_sloppyFunctionStructure);
     thisObject->m_customGetterSetterFunctionStructure.visit(visitor);
     thisObject->m_boundFunctionStructure.visit(visitor);
     visitor.append(thisObject->m_getterSetterStructure);
     thisObject->m_nativeStdFunctionStructure.visit(visitor);
-    thisObject->m_namedFunctionStructure.visit(visitor);
     visitor.append(thisObject->m_symbolObjectStructure);
     visitor.append(thisObject->m_regExpStructure);
     visitor.append(thisObject->m_generatorFunctionStructure);
index 563b444..b8d6813 100644 (file)
@@ -321,12 +321,12 @@ public:
 #endif
     LazyProperty<JSGlobalObject, Structure> m_nullPrototypeObjectStructure;
     WriteBarrier<Structure> m_calleeStructure;
-    WriteBarrier<Structure> m_functionStructure;
+    WriteBarrier<Structure> m_strictFunctionStructure;
+    WriteBarrier<Structure> m_sloppyFunctionStructure;
     LazyProperty<JSGlobalObject, Structure> m_boundFunctionStructure;
     LazyProperty<JSGlobalObject, Structure> m_customGetterSetterFunctionStructure;
     WriteBarrier<Structure> m_getterSetterStructure;
     LazyProperty<JSGlobalObject, Structure> m_nativeStdFunctionStructure;
-    LazyProperty<JSGlobalObject, Structure> m_namedFunctionStructure;
     PropertyOffset m_functionNameOffset;
     WriteBarrier<Structure> m_regExpStructure;
     WriteBarrier<AsyncFunctionPrototype> m_asyncFunctionPrototype;
@@ -629,12 +629,12 @@ public:
     Structure* nullPrototypeObjectStructure() const { return m_nullPrototypeObjectStructure.get(this); }
     Structure* errorStructure() const { return m_errorStructure.get(); }
     Structure* calleeStructure() const { return m_calleeStructure.get(); }
-    Structure* functionStructure() const { return m_functionStructure.get(); }
+    Structure* strictFunctionStructure() const { return m_strictFunctionStructure.get(); }
+    Structure* sloppyFunctionStructure() const { return m_sloppyFunctionStructure.get(); }
     Structure* boundFunctionStructure() const { return m_boundFunctionStructure.get(this); }
     Structure* customGetterSetterFunctionStructure() const { return m_customGetterSetterFunctionStructure.get(this); }
     Structure* getterSetterStructure() const { return m_getterSetterStructure.get(); }
     Structure* nativeStdFunctionStructure() const { return m_nativeStdFunctionStructure.get(this); }
-    Structure* namedFunctionStructure() const { return m_namedFunctionStructure.get(this); }
     PropertyOffset functionNameOffset() const { return m_functionNameOffset; }
     Structure* numberObjectStructure() const { return m_numberObjectStructure.get(); }
     Structure* mapStructure() const { return m_mapStructure.get(); }