Attachment 361529 Details for Bug 194446 – proposed patch.

[patch] proposed patch.

bug-194446.patch (text/plain), 12.28 KB, created by Mark Lam on 2019-02-08 13:19:21 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Mark Lam

Created: 2019-02-08 13:19:21 PST

Size: 12.28 KB

patch

obsolete

>Index: Source/JavaScriptCore/ChangeLog
>===================================================================
>--- Source/JavaScriptCore/ChangeLog	(revision 241204)
>+++ Source/JavaScriptCore/ChangeLog	(working copy)
>@@ -1,3 +1,56 @@
>+2019-02-08  Mark Lam  <mark.lam@apple.com>
>+
>+        Fix DFG's doesGC() for CheckTierUp*, GetByVal, PutByVal*, and StringCharAt nodes.
>+        https://bugs.webkit.org/show_bug.cgi?id=194446
>+        <rdar://problem/47926792>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Fix doesGC() for the following nodes:
>+
>+            CheckTierUpAtReturn:
>+                Calls triggerTierUpNow(), which calls triggerFTLReplacementCompile(),
>+                which calls Worklist::completeAllReadyPlansForVM(), which uses DeferGC.
>+
>+            CheckTierUpInLoop:
>+                Calls triggerTierUpNowInLoop(), which calls tierUpCommon(), which calls
>+                Worklist::completeAllReadyPlansForVM(), which uses DeferGC.
>+
>+            CheckTierUpAndOSREnter:
>+                Calls triggerOSREntryNow(), which calls tierUpCommon(), which calls
>+                Worklist::completeAllReadyPlansForVM(), which uses DeferGC.
>+
>+            GetByVal:
>+                case Array::String calls operationSingleCharacterString(), which calls
>+                jsSingleCharacterString(), which can allocate a string.
>+
>+            PutByValDirect:
>+            PutByVal:
>+            PutByValAlias:
>+                For the DFG only, the integer TypeArrays calls compilePutByValForIntTypedArray(),
>+                which may call slow paths operationPutByValDirectStrict(), operationPutByValDirectNonStrict(),
>+                operationPutByValStrict(), or operationPutByValNonStrict().  All of these
>+                slow paths call putByValInternal(), which may create exception objects, or
>+                call the generic JSValue::put() which may execute arbitrary code.
>+
>+            StringCharAt:
>+                Can call operationSingleCharacterString(), which calls jsSingleCharacterString(),
>+                which can allocate a string.
>+
>+        Also fix DFG::SpeculativeJIT::compileGetByValOnString() and FTL's compileStringCharAt()
>+        to use the maxSingleCharacterString constant instead of a literal constant.
>+
>+        * dfg/DFGDoesGC.cpp:
>+        (JSC::DFG::doesGC):
>+        * dfg/DFGSpeculativeJIT.cpp:
>+        (JSC::DFG::SpeculativeJIT::compileGetByValOnString):
>+        * dfg/DFGSpeculativeJIT64.cpp:
>+        (JSC::DFG::SpeculativeJIT::compile):
>+        * ftl/FTLLowerDFGToB3.cpp:
>+        (JSC::FTL::DFG::LowerDFGToB3::compileGetByVal):
>+        (JSC::FTL::DFG::LowerDFGToB3::compilePutByVal):
>+        (JSC::FTL::DFG::LowerDFGToB3::compileStringCharAt):
>+
> 2019-02-08  Yusuke Suzuki  <ysuzuki@apple.com>
> 
>         [JSC] SourceProviderCacheItem should be small
>Index: Source/JavaScriptCore/dfg/DFGDoesGC.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGDoesGC.cpp	(revision 241204)
>+++ Source/JavaScriptCore/dfg/DFGDoesGC.cpp	(working copy)
>@@ -53,6 +53,7 @@ bool doesGC(Graph& graph, Node* node)
>     //        unless it is a known transition between previously allocated structures
>     //        such as between Array types.
>     //     5. Calls to a JS function, which can execute arbitrary code including allocating objects.
>+    //     6. Calls operations that uses DeferGC, because it may GC in its destructor.
> 
>     switch (node->op()) {
>     case JSConstant:
>@@ -177,9 +178,6 @@ bool doesGC(Graph& graph, Node* node)
>     case ExtractOSREntryLocal:
>     case ExtractCatchLocal:
>     case ClearCatchLocals:
>-    case CheckTierUpInLoop:
>-    case CheckTierUpAtReturn:
>-    case CheckTierUpAndOSREnter:
>     case LoopHint:
>     case StoreBarrier:
>     case FencedStoreBarrier:
>@@ -196,16 +194,11 @@ bool doesGC(Graph& graph, Node* node)
>     case Int52Rep:
>     case GetGetter:
>     case GetSetter:
>-    case GetByVal:
>     case GetArrayLength:
>     case GetVectorLength:
>-    case StringCharAt:
>     case StringCharCodeAt:
>     case GetTypedArrayByteOffset:
>     case GetPrototypeOf:
>-    case PutByValDirect:
>-    case PutByVal:
>-    case PutByValAlias:
>     case PutStructure:
>     case GetByOffset:
>     case GetGetterSetterByOffset:
>@@ -272,6 +265,9 @@ bool doesGC(Graph& graph, Node* node)
>     case CallForwardVarargs:
>     case CallObjectConstructor:
>     case CallVarargs:
>+    case CheckTierUpAndOSREnter:
>+    case CheckTierUpAtReturn:
>+    case CheckTierUpInLoop:
>     case Construct:
>     case ConstructForwardVarargs:
>     case ConstructVarargs:
>@@ -325,6 +321,7 @@ bool doesGC(Graph& graph, Node* node)
>     case ResolveScope:
>     case ResolveScopeForHoistingFuncDeclInEval:
>     case Return:
>+    case StringCharAt:
>     case TailCall:
>     case TailCallForwardVarargs:
>     case TailCallForwardVarargsInlinedCaller:
>@@ -412,10 +409,30 @@ bool doesGC(Graph& graph, Node* node)
>         return true;
> 
>     case GetIndexedPropertyStorage:
>+    case GetByVal:
>         if (node->arrayMode().type() == Array::String)
>             return true;
>         return false;
> 
>+    case PutByValDirect:
>+    case PutByVal:
>+    case PutByValAlias:
>+        if (!graph.m_plan.isFTL()) {
>+            switch (node->arrayMode().modeForPut().type()) {
>+            case Array::Int8Array:
>+            case Array::Int16Array:
>+            case Array::Int32Array:
>+            case Array::Uint8Array:
>+            case Array::Uint8ClampedArray:
>+            case Array::Uint16Array:
>+            case Array::Uint32Array:
>+                return true;
>+            default:
>+                break;
>+            }
>+        }
>+        return false;
>+
>     case MapHash:
>         switch (node->child1().useKind()) {
>         case BooleanUse:
>Index: Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp	(revision 241204)
>+++ Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp	(working copy)
>@@ -2344,8 +2344,11 @@ void SpeculativeJIT::compile(Node* node)
> 
>     case GetByVal: {
>         switch (node->arrayMode().type()) {
>-        case Array::SelectUsingPredictions:
>+        case Array::AnyTypedArray:
>         case Array::ForceExit:
>+        case Array::SelectUsingArguments:
>+        case Array::SelectUsingPredictions:
>+        case Array::Unprofiled:
>             DFG_CRASH(m_jit.graph(), node, "Bad array mode type");
>             break;
>         case Array::Undecided: {
>@@ -2566,7 +2569,15 @@ void SpeculativeJIT::compile(Node* node)
>         case Array::ScopedArguments:
>             compileGetByValOnScopedArguments(node);
>             break;
>-        default: {
>+        case Array::Int8Array:
>+        case Array::Int16Array:
>+        case Array::Int32Array:
>+        case Array::Uint8Array:
>+        case Array::Uint8ClampedArray:
>+        case Array::Uint16Array:
>+        case Array::Uint32Array:
>+        case Array::Float32Array:
>+        case Array::Float64Array: {
>             TypedArrayType type = node->arrayMode().typedArrayType();
>             if (isInt(type))
>                 compileGetByValOnIntTypedArray(node, type);
>@@ -2800,14 +2811,35 @@ void SpeculativeJIT::compile(Node* node)
>             break;
>         }
>             
>-        default: {
>+        case Array::Int8Array:
>+        case Array::Int16Array:
>+        case Array::Int32Array:
>+        case Array::Uint8Array:
>+        case Array::Uint8ClampedArray:
>+        case Array::Uint16Array:
>+        case Array::Uint32Array:
>+        case Array::Float32Array:
>+        case Array::Float64Array: {
>             TypedArrayType type = arrayMode.typedArrayType();
>             if (isInt(type))
>                 compilePutByValForIntTypedArray(base.gpr(), property.gpr(), node, type);
>             else
>                 compilePutByValForFloatTypedArray(base.gpr(), property.gpr(), node, type);
>-        } }
>+            break;
>+        }
> 
>+        case Array::AnyTypedArray:
>+        case Array::String:
>+        case Array::DirectArguments:
>+        case Array::ForceExit:
>+        case Array::Generic:
>+        case Array::ScopedArguments:
>+        case Array::SelectUsingArguments:
>+        case Array::SelectUsingPredictions:
>+        case Array::Undecided:
>+        case Array::Unprofiled:
>+            RELEASE_ASSERT_NOT_REACHED();
>+        }
>         break;
>     }
>         
>Index: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp	(revision 241204)
>+++ Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp	(working copy)
>@@ -2194,7 +2194,7 @@ void SpeculativeJIT::compileGetByValOnSt
>     m_jit.load16(MacroAssembler::BaseIndex(storageReg, propertyReg, MacroAssembler::TimesTwo, 0), scratchReg);
> 
>     JITCompiler::Jump bigCharacter =
>-        m_jit.branch32(MacroAssembler::AboveOrEqual, scratchReg, TrustedImm32(0x100));
>+        m_jit.branch32(MacroAssembler::Above, scratchReg, TrustedImm32(maxSingleCharacterString));
> 
>     // 8 bit string values don't need the isASCII check.
>     cont8Bit.link(&m_jit);
>Index: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp
>===================================================================
>--- Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp	(revision 241204)
>+++ Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp	(working copy)
>@@ -4163,13 +4163,21 @@ private:
>             return;
>         }
>             
>-        default: {
>+        case Array::Int8Array:
>+        case Array::Int16Array:
>+        case Array::Int32Array:
>+        case Array::Uint8Array:
>+        case Array::Uint8ClampedArray:
>+        case Array::Uint16Array:
>+        case Array::Uint32Array:
>+        case Array::Float32Array:
>+        case Array::Float64Array: {
>             LValue index = lowInt32(m_graph.varArgChild(m_node, 1));
>             LValue storage = lowStorage(m_graph.varArgChild(m_node, 2));
>             
>             TypedArrayType type = m_node->arrayMode().typedArrayType();
>-            
>-            if (isTypedView(type)) {
>+            ASSERT(isTypedView(type));
>+            {
>                 TypedPointer pointer = pointerIntoTypedArray(storage, index, type);
>                 
>                 if (isInt(type)) {
>@@ -4196,10 +4204,16 @@ private:
>                 setDouble(result);
>                 return;
>             }
>-            
>+        }
>+
>+        case Array::AnyTypedArray:
>+        case Array::ForceExit:
>+        case Array::SelectUsingArguments:
>+        case Array::SelectUsingPredictions:
>+        case Array::Unprofiled:
>             DFG_CRASH(m_graph, m_node, "Bad array type");
>             return;
>-        } }
>+        }
>     }
>     
>     void compileGetMyArgumentByVal()
>@@ -4488,10 +4502,19 @@ private:
>             return;
>         }
>             
>-        default: {
>+        case Array::Int8Array:
>+        case Array::Int16Array:
>+        case Array::Int32Array:
>+        case Array::Uint8Array:
>+        case Array::Uint8ClampedArray:
>+        case Array::Uint16Array:
>+        case Array::Uint32Array:
>+        case Array::Float32Array:
>+        case Array::Float64Array: {
>             TypedArrayType type = arrayMode.typedArrayType();
>             
>-            if (isTypedView(type)) {
>+            ASSERT(isTypedView(type));
>+            {
>                 TypedPointer pointer = TypedPointer(
>                     m_heaps.typedArrayProperties,
>                     m_out.add(
>@@ -4544,11 +4567,21 @@ private:
>                 
>                 return;
>             }
>+        }
> 
>+        case Array::AnyTypedArray:
>+        case Array::String:
>+        case Array::DirectArguments:
>+        case Array::ForceExit:
>+        case Array::Generic:
>+        case Array::ScopedArguments:
>+        case Array::SelectUsingArguments:
>+        case Array::SelectUsingPredictions:
>+        case Array::Undecided:
>+        case Array::Unprofiled:
>             DFG_CRASH(m_graph, m_node, "Bad array type");
>             break;
>         }
>-        }
>     }
> 
>     void compilePutAccessorById()
>@@ -6579,7 +6612,7 @@ private:
>                 provenValue(m_graph.child(m_node, 1))));
>         ValueFromBlock char16Bit = m_out.anchor(char16BitValue);
>         m_out.branch(
>-            m_out.aboveOrEqual(char16BitValue, m_out.constInt32(0x100)),
>+            m_out.above(char16BitValue, m_out.constInt32(maxSingleCharacterString)),
>             rarely(bigCharacter), usually(bitsContinuation));
>             
>         m_out.appendTo(bigCharacter, bitsContinuation);

Flags:

saam: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 194446: 361529