Merge r225273 - Strict and sloppy functions shouldn't share structure

JSTests/ChangeLog

@@ -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-07  Mark Lam  <mark.lam@apple.com>
 
         AccessCase::generateImpl() should exclude the result register when restoring registers after a call.

JSTests/stress/get-by-id-strict-arguments.js

@@ -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!`);

JSTests/stress/get-by-id-strict-callee.js

@@ -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!`);

JSTests/stress/get-by-id-strict-caller.js

@@ -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!`);

JSTests/stress/get-by-id-strict-nested-arguments-2.js

@@ -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!`);

JSTests/stress/get-by-id-strict-nested-arguments.js

@@ -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!`);

JSTests/stress/strict-function-structure.js

@@ -0,0 +1,8 @@
+function foo(f) { f.hasOwnProperty("arguments"); }
+noInline(foo);
+
+function bar() {}
+foo(bar);
+
+function baz() { "use strict"; }
+foo(baz);

JSTests/stress/strict-nested-function-structure.js

@@ -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());

Source/JavaScriptCore/ChangeLog

@@ -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-07  Mark Lam  <mark.lam@apple.com>
 
         AccessCase::generateImpl() should exclude the result register when restoring registers after a call.

Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h

@@ -2109,8 +2109,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:

Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp

@@ -6487,9 +6487,20 @@ 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() :
-        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 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);

Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp

@@ -4340,10 +4340,23 @@ class LowerDFGToB3 {
             return;
         }
 
+
         RegisteredStructure structure = m_graph.registerStructure(
-            isGeneratorFunction ? m_graph.globalObjectFor(m_node->origin.semantic)->generatorFunctionStructure() :
-            isAsyncFunction ? m_graph.globalObjectFor(m_node->origin.semantic)->asyncFunctionStructure() :
-            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 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();

Source/JavaScriptCore/runtime/FunctionConstructor.cpp

@@ -95,18 +95,14 @@ 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;
     }
@@ -184,6 +180,22 @@ 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;
+    }
+
     Structure* subclassStructure = InternalFunction::createSubclassStructure(exec, newTarget, structure);
     RETURN_IF_EXCEPTION(scope, nullptr);
 

Source/JavaScriptCore/runtime/JSFunction.cpp

@@ -65,7 +65,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)
@@ -78,7 +79,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;