put_to_scope/get_from_scope should not cache lexical scopes when expecting a global...
authorkeith_miller@apple.com <keith_miller@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 6 Feb 2018 22:42:26 +0000 (22:42 +0000)
committerkeith_miller@apple.com <keith_miller@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 6 Feb 2018 22:42:26 +0000 (22:42 +0000)
https://bugs.webkit.org/show_bug.cgi?id=182549
<rdar://problem/36189995>

Reviewed by Saam Barati.

JSTests:

* stress/var-injection-cache-invalidation.js: Added.
(allocateLotsOfThings):
(test):

Source/JavaScriptCore:

Previously, the llint/baseline caching for put_to_scope and
get_from_scope would cache lexical environments when the
varInjectionWatchpoint had been fired for global properties. Code
in the DFG does not follow this same assumption so we could
potentially return the wrong result. Additionally, the baseline
would write barrier the global object rather than the lexical
enviroment object. This patch makes it so that we do not cache
anything other than the global object for when the resolve type is
GlobalPropertyWithVarInjectionChecks or GlobalProperty.

* assembler/MacroAssembler.cpp:
(JSC::MacroAssembler::jitAssert):
* assembler/MacroAssembler.h:
* jit/JITPropertyAccess.cpp:
(JSC::JIT::emit_op_get_from_scope):
(JSC::JIT::emit_op_put_to_scope):
* runtime/CommonSlowPaths.h:
(JSC::CommonSlowPaths::tryCachePutToScopeGlobal):
(JSC::CommonSlowPaths::tryCacheGetFromScopeGlobal):
* runtime/Options.h:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@228193 268f45cc-cd09-0410-ab3c-d52691b4dbfc

JSTests/ChangeLog
JSTests/stress/var-injection-cache-invalidation.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/assembler/MacroAssembler.cpp
Source/JavaScriptCore/assembler/MacroAssembler.h
Source/JavaScriptCore/jit/JITPropertyAccess.cpp
Source/JavaScriptCore/runtime/CommonSlowPaths.h
Source/JavaScriptCore/runtime/Options.h

index ed128c9..5317f1b 100644 (file)
@@ -1,3 +1,15 @@
+2018-02-06  Keith Miller  <keith_miller@apple.com>
+
+        put_to_scope/get_from_scope should not cache lexical scopes when expecting a global object
+        https://bugs.webkit.org/show_bug.cgi?id=182549
+        <rdar://problem/36189995>
+
+        Reviewed by Saam Barati.
+
+        * stress/var-injection-cache-invalidation.js: Added.
+        (allocateLotsOfThings):
+        (test):
+
 2018-02-03  Yusuke Suzuki  <utatane.tea@gmail.com>
 
         Unreviewed, follow up for test262 update
diff --git a/JSTests/stress/var-injection-cache-invalidation.js b/JSTests/stress/var-injection-cache-invalidation.js
new file mode 100644 (file)
index 0000000..b00f38f
--- /dev/null
@@ -0,0 +1,19 @@
+a = 0;
+
+function allocateLotsOfThings(array) {
+    for (let i = 0; i < 1e4; i++)
+        array[i] = { next: array[Math.floor(i / 2)] };
+}
+
+function test() {
+    a = 5;
+    for (var i = 0; i < 1e3; i++) {
+        allocateLotsOfThings([]);
+        edenGC();
+        eval("var a = new Int32Array(100);");
+    }
+}
+noInline(test);
+noDFG(test);
+
+test();
index 40d2def..0f5d7f3 100644 (file)
@@ -1,3 +1,32 @@
+2018-02-06  Keith Miller  <keith_miller@apple.com>
+
+        put_to_scope/get_from_scope should not cache lexical scopes when expecting a global object
+        https://bugs.webkit.org/show_bug.cgi?id=182549
+        <rdar://problem/36189995>
+
+        Reviewed by Saam Barati.
+
+        Previously, the llint/baseline caching for put_to_scope and
+        get_from_scope would cache lexical environments when the
+        varInjectionWatchpoint had been fired for global properties. Code
+        in the DFG does not follow this same assumption so we could
+        potentially return the wrong result. Additionally, the baseline
+        would write barrier the global object rather than the lexical
+        enviroment object. This patch makes it so that we do not cache
+        anything other than the global object for when the resolve type is
+        GlobalPropertyWithVarInjectionChecks or GlobalProperty.
+
+        * assembler/MacroAssembler.cpp:
+        (JSC::MacroAssembler::jitAssert):
+        * assembler/MacroAssembler.h:
+        * jit/JITPropertyAccess.cpp:
+        (JSC::JIT::emit_op_get_from_scope):
+        (JSC::JIT::emit_op_put_to_scope):
+        * runtime/CommonSlowPaths.h:
+        (JSC::CommonSlowPaths::tryCachePutToScopeGlobal):
+        (JSC::CommonSlowPaths::tryCacheGetFromScopeGlobal):
+        * runtime/Options.h:
+
 2018-01-28  Filip Pizlo  <fpizlo@apple.com>
 
         Global objects should be able to use TLCs to allocate from different blocks from each other
index 82b25c8..0d16df2 100644 (file)
 
 #if ENABLE(ASSEMBLER)
 
+#include "Options.h"
 #include "ProbeContext.h"
 #include <wtf/PrintStream.h>
+#include <wtf/ScopedLambda.h>
 
 namespace JSC {
 
 const double MacroAssembler::twoToThe32 = (double)0x100000000ull;
 
+void MacroAssembler::jitAssert(const ScopedLambda<Jump(void)>& functor)
+{
+    if (Options::enableJITDebugAssetions()) {
+        Jump passed = functor();
+        breakpoint();
+        passed.link(this);
+    }
+}
+
 #if ENABLE(MASM_PROBE)
 static void stdFunctionCallback(Probe::Context& context)
 {
index b99b6b3..99d8065 100644 (file)
@@ -61,6 +61,13 @@ namespace JSC { typedef MacroAssemblerX86_64 MacroAssemblerBase; };
 
 #include "MacroAssemblerHelpers.h"
 
+namespace WTF {
+
+template<typename FunctionType>
+class ScopedLambda;
+
+} // namespace WTF
+
 namespace JSC {
 
 #if ENABLE(MASM_PROBE)
@@ -1884,6 +1891,9 @@ public:
         urshift32(src, trustedImm32ForShift(amount), dest);
     }
 
+    // If the result jump is taken that means the assert passed.
+    void jitAssert(const WTF::ScopedLambda<Jump(void)>&);
+
 #if ENABLE(MASM_PROBE)
     // This function emits code to preserve the CPUState (e.g. registers),
     // call a user supplied probe function, and restore the CPUState before
index 5a025c2..27e2e90 100644 (file)
@@ -43,6 +43,7 @@
 #include "ScopedArgumentsTable.h"
 #include "SlowPathCall.h"
 #include "StructureStubInfo.h"
+#include <wtf/ScopedLambda.h>
 #include <wtf/StringPrintStream.h>
 
 
@@ -857,12 +858,16 @@ void JIT::emit_op_get_from_scope(Instruction* currentInstruction)
         switch (resolveType) {
         case GlobalProperty:
         case GlobalPropertyWithVarInjectionChecks: {
-            emitLoadWithStructureCheck(scope, structureSlot); // Structure check covers var injection.
+            emitLoadWithStructureCheck(scope, structureSlot); // Structure check covers var injection since we don't cache structures for anything but the GlobalObject. Additionally, resolve_scope handles checking for the var injection.
             GPRReg base = regT0;
             GPRReg result = regT0;
             GPRReg offset = regT1;
             GPRReg scratch = regT2;
-            
+
+            jitAssert(scopedLambda<Jump(void)>([&] () -> Jump {
+                return branchPtr(Equal, base, TrustedImmPtr(m_codeBlock->globalObject()));
+            }));
+
             load32(operandSlot, offset);
             if (!ASSERT_DISABLED) {
                 Jump isOutOfLine = branch32(GreaterThanOrEqual, offset, TrustedImm32(firstOutOfLineOffset));
@@ -985,9 +990,13 @@ void JIT::emit_op_put_to_scope(Instruction* currentInstruction)
         switch (resolveType) {
         case GlobalProperty:
         case GlobalPropertyWithVarInjectionChecks: {
-            emitLoadWithStructureCheck(scope, structureSlot); // Structure check covers var injection.
+            emitLoadWithStructureCheck(scope, structureSlot); // Structure check covers var injection since we don't cache structures for anything but the GlobalObject. Additionally, resolve_scope handles checking for the var injection.
             emitGetVirtualRegister(value, regT2);
-            
+
+            jitAssert(scopedLambda<Jump(void)>([&] () -> Jump {
+                return branchPtr(Equal, regT0, TrustedImmPtr(m_codeBlock->globalObject()));
+            }));
+
             loadPtr(Address(regT0, JSObject::butterflyOffset()), regT0);
             loadPtr(operandSlot, regT1);
             negPtr(regT1);
index 786f9d2..8f7c444 100644 (file)
@@ -138,8 +138,11 @@ inline void tryCachePutToScopeGlobal(
     }
     
     if (resolveType == GlobalProperty || resolveType == GlobalPropertyWithVarInjectionChecks) {
+        JSGlobalObject* globalObject = codeBlock->globalObject();
+        ASSERT(globalObject == scope || globalObject->varInjectionWatchpoint()->hasBeenInvalidated());
         if (!slot.isCacheablePut()
             || slot.base() != scope
+            || scope != globalObject
             || !scope->structure()->propertyAccessesAreCacheable())
             return;
         
@@ -183,9 +186,11 @@ inline void tryCacheGetFromScopeGlobal(
     }
 
     // Covers implicit globals. Since they don't exist until they first execute, we didn't know how to cache them at compile time.
-    if (slot.isCacheableValue() && slot.slotBase() == scope && scope->structure()->propertyAccessesAreCacheable()) {
-        if (resolveType == GlobalProperty || resolveType == GlobalPropertyWithVarInjectionChecks) {
-            CodeBlock* codeBlock = exec->codeBlock();
+    if (resolveType == GlobalProperty || resolveType == GlobalPropertyWithVarInjectionChecks) {
+        CodeBlock* codeBlock = exec->codeBlock();
+        JSGlobalObject* globalObject = codeBlock->globalObject();
+        ASSERT(scope == globalObject || globalObject->varInjectionWatchpoint()->hasBeenInvalidated());
+        if (slot.isCacheableValue() && slot.slotBase() == scope && scope == globalObject && scope->structure()->propertyAccessesAreCacheable()) {
             Structure* structure = scope->structure(vm);
             {
                 ConcurrentJSLocker locker(codeBlock->m_lock);
index f0c8b96..4f0c636 100644 (file)
@@ -250,6 +250,7 @@ constexpr bool enableAsyncIteration = false;
     v(bool, b3AlwaysFailsBeforeLink, false, Normal, nullptr) \
     v(bool, ftlCrashes, false, Normal, nullptr) /* fool-proof way of checking that you ended up in the FTL. ;-) */\
     v(bool, clobberAllRegsInFTLICSlowPath, !ASSERT_DISABLED, Normal, nullptr) \
+    v(bool, enableJITDebugAssetions, !ASSERT_DISABLED, Normal, nullptr) \
     v(bool, useAccessInlining, true, Normal, nullptr) \
     v(unsigned, maxAccessVariantListSize, 8, Normal, nullptr) \
     v(bool, usePolyvariantDevirtualization, true, Normal, nullptr) \