Attachment 342697 Details for Bug 132333 – Patch for landing

[patch] Patch for landing

bug-132333-20180613182009.patch (text/plain), 11.12 KB, created by Caitlin Potter (:caitp) on 2018-06-13 15:20:10 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Caitlin Potter (:caitp)

Created: 2018-06-13 15:20:10 PDT

Size: 11.12 KB

patch

obsolete

>Subversion Revision: 232814
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index 206fcc33552f73f7f4e382aeb7c5c358d8a8af6b..dc9322fd54acb973f47082355ea17a38d4c704bc 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,30 @@
>+2018-06-06  Caitlin Potter  <caitp@igalia.com>
>+
>+        [LLInt] use loadp consistently for get_from_scope/put_to_scope
>+        https://bugs.webkit.org/show_bug.cgi?id=132333
>+
>+        Reviewed by Mark Lam.
>+
>+        Using `loadis` for register indexes and `loadp` for constant scopes /
>+        symboltables makes sense, but is problematic for big-endian
>+        architectures.
>+
>+        Consistently treating the operand as a pointer simplifies determining
>+        how to access the operand, and helps avoid bad accesses and crashes on
>+        big-endian ports.
>+
>+        * bytecode/CodeBlock.cpp:
>+        (JSC::CodeBlock::finishCreation):
>+        * bytecode/Instruction.h:
>+        * jit/JITOperations.cpp:
>+        * llint/LLIntSlowPaths.cpp:
>+        (JSC::LLInt::LLINT_SLOW_PATH_DECL):
>+        * llint/LowLevelInterpreter32_64.asm:
>+        * llint/LowLevelInterpreter64.asm:
>+        * runtime/CommonSlowPaths.h:
>+        (JSC::CommonSlowPaths::tryCachePutToScopeGlobal):
>+        (JSC::CommonSlowPaths::tryCacheGetFromScopeGlobal):
>+
> 2018-06-13  Keith Miller  <keith_miller@apple.com>
> 
>         AutomaticThread should have a way to provide a thread name
>diff --git a/Source/JavaScriptCore/bytecode/CodeBlock.cpp b/Source/JavaScriptCore/bytecode/CodeBlock.cpp
>index 98ef06f47a33611351c74cba411ac2f31b864a8f..f656d761026fbcdd8e1d49a153729b53aaf6a34a 100644
>--- a/Source/JavaScriptCore/bytecode/CodeBlock.cpp
>+++ b/Source/JavaScriptCore/bytecode/CodeBlock.cpp
>@@ -694,7 +694,7 @@ bool CodeBlock::finishCreation(VM& vm, ScriptExecutable* ownerExecutable, Unlink
>                 instructions[i + 5].u.watchpointSet = op.watchpointSet;
>             else if (op.structure)
>                 instructions[i + 5].u.structure.set(vm, this, op.structure);
>-            instructions[i + 6].u.pointer = reinterpret_cast<void*>(op.operand);
>+            instructions[i + 6].u.operandPointer = op.operand;
>             break;
>         }
> 
>@@ -731,7 +731,7 @@ bool CodeBlock::finishCreation(VM& vm, ScriptExecutable* ownerExecutable, Unlink
>                     op.watchpointSet->invalidate(vm, PutToScopeFireDetail(this, ident));
>             } else if (op.structure)
>                 instructions[i + 5].u.structure.set(vm, this, op.structure);
>-            instructions[i + 6].u.pointer = reinterpret_cast<void*>(op.operand);
>+            instructions[i + 6].u.operandPointer = op.operand;
> 
>             break;
>         }
>diff --git a/Source/JavaScriptCore/bytecode/Instruction.h b/Source/JavaScriptCore/bytecode/Instruction.h
>index c133578b3263d3029845e48379a35960704a6efd..1182b4033c7673ffadfa0eff883fc496db6193ab 100644
>--- a/Source/JavaScriptCore/bytecode/Instruction.h
>+++ b/Source/JavaScriptCore/bytecode/Instruction.h
>@@ -123,6 +123,7 @@ struct Instruction {
>         Opcode opcode;
>         int operand;
>         unsigned unsignedValue;
>+        intptr_t operandPointer;
>         WriteBarrierBase<Structure> structure;
>         StructureID structureID;
>         WriteBarrierBase<SymbolTable> symbolTable;
>diff --git a/Source/JavaScriptCore/jit/JITOperations.cpp b/Source/JavaScriptCore/jit/JITOperations.cpp
>index 5381dd6d04e69a9240ace66db2ee4e3bfaf36bbc..ccd9b2be4c28e9181077d8a5069fd6a47c501c56 100644
>--- a/Source/JavaScriptCore/jit/JITOperations.cpp
>+++ b/Source/JavaScriptCore/jit/JITOperations.cpp
>@@ -2381,7 +2381,7 @@ void JIT_OPERATION operationPutToScope(ExecState* exec, Instruction* bytecodePC)
> 
>     if (getPutInfo.resolveType() == LocalClosureVar) {
>         JSLexicalEnvironment* environment = jsCast<JSLexicalEnvironment*>(scope);
>-        environment->variableAt(ScopeOffset(pc[6].u.operand)).set(vm, environment, value);
>+        environment->variableAt(ScopeOffset(pc[6].u.operandPointer)).set(vm, environment, value);
>         if (WatchpointSet* set = pc[5].u.watchpointSet)
>             set->touch(vm, "Executed op_put_scope<LocalClosureVar>");
>         return;
>diff --git a/Source/JavaScriptCore/llint/LLIntSlowPaths.cpp b/Source/JavaScriptCore/llint/LLIntSlowPaths.cpp
>index c8d3e4714cefd9249feb9885f05fcf68b695e520..a0b6e9cb436aa5ba719596cfd7f9641d4ddf2b35 100644
>--- a/Source/JavaScriptCore/llint/LLIntSlowPaths.cpp
>+++ b/Source/JavaScriptCore/llint/LLIntSlowPaths.cpp
>@@ -1729,7 +1729,7 @@ LLINT_SLOW_PATH_DECL(slow_path_put_to_scope)
>     GetPutInfo getPutInfo = GetPutInfo(pc[4].u.operand);
>     if (getPutInfo.resolveType() == LocalClosureVar) {
>         JSLexicalEnvironment* environment = jsCast<JSLexicalEnvironment*>(scope);
>-        environment->variableAt(ScopeOffset(pc[6].u.operand)).set(vm, environment, value);
>+        environment->variableAt(ScopeOffset(pc[6].u.operandPointer)).set(vm, environment, value);
>         
>         // Have to do this *after* the write, because if this puts the set into IsWatched, then we need
>         // to have already changed the value of the variable. Otherwise we might watch and constant-fold
>diff --git a/Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm b/Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm
>index 80f41d804a6dfa0d9124c94ec41dd11061e06489..8a7bcf4fd2f9972f47bad087a0483932b55004df 100644
>--- a/Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm
>+++ b/Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm
>@@ -2339,7 +2339,7 @@ macro loadWithStructureCheck(operand, slowPath)
> end
> 
> macro getProperty()
>-    loadisFromInstruction(6, t3)
>+    loadpFromInstruction(6, t3)
>     loadPropertyAtVariableOffset(t3, t0, t1, t2)
>     valueProfile(t1, t2, 28, t0)
>     loadisFromInstruction(1, t0)
>@@ -2359,7 +2359,7 @@ macro getGlobalVar(tdzCheckIfNecessary)
> end
> 
> macro getClosureVar()
>-    loadisFromInstruction(6, t3)
>+    loadpFromInstruction(6, t3)
>     loadp JSLexicalEnvironment_variables + TagOffset[t0, t3, 8], t1
>     loadp JSLexicalEnvironment_variables + PayloadOffset[t0, t3, 8], t2
>     valueProfile(t1, t2, 28, t0)
>@@ -2434,7 +2434,7 @@ _llint_op_get_from_scope:
> macro putProperty()
>     loadisFromInstruction(3, t1)
>     loadConstantOrVariable(t1, t2, t3)
>-    loadisFromInstruction(6, t1)
>+    loadpFromInstruction(6, t1)
>     storePropertyAtVariableOffset(t1, t0, t2, t3)
> end
> 
>@@ -2451,7 +2451,7 @@ end
> macro putClosureVar()
>     loadisFromInstruction(3, t1)
>     loadConstantOrVariable(t1, t2, t3)
>-    loadisFromInstruction(6, t1)
>+    loadpFromInstruction(6, t1)
>     storei t2, JSLexicalEnvironment_variables + TagOffset[t0, t1, 8]
>     storei t3, JSLexicalEnvironment_variables + PayloadOffset[t0, t1, 8]
> end
>@@ -2463,7 +2463,7 @@ macro putLocalClosureVar()
>     btpz t5, .noVariableWatchpointSet
>     notifyWrite(t5, .pDynamic)
> .noVariableWatchpointSet:
>-    loadisFromInstruction(6, t1)
>+    loadpFromInstruction(6, t1)
>     storei t2, JSLexicalEnvironment_variables + TagOffset[t0, t1, 8]
>     storei t3, JSLexicalEnvironment_variables + PayloadOffset[t0, t1, 8]
> end
>diff --git a/Source/JavaScriptCore/llint/LowLevelInterpreter64.asm b/Source/JavaScriptCore/llint/LowLevelInterpreter64.asm
>index f867597fc46e531a385561f271b871a98422bab0..c39988c13c8c595fafdac33f0cf26cafd9618602 100644
>--- a/Source/JavaScriptCore/llint/LowLevelInterpreter64.asm
>+++ b/Source/JavaScriptCore/llint/LowLevelInterpreter64.asm
>@@ -1500,7 +1500,7 @@ _llint_op_put_by_id:
>     bineq t1, JSCell::m_structureID[t3], .opPutByIdSlow
> 
> .opPutByIdDoneCheckingTypes:
>-    loadisFromInstruction(6, t1)
>+    loadpFromInstruction(6, t1)
>     
>     btiz t1, .opPutByIdNotTransition
> 
>@@ -1530,7 +1530,7 @@ _llint_op_put_by_id:
> 
> .opPutByIdTransitionChainDone:
>     # Reload the new structure, since we clobbered it above.
>-    loadisFromInstruction(6, t1)
>+    loadpFromInstruction(6, t1)
> 
> .opPutByIdTransitionDirect:
>     storei t1, JSCell::m_structureID[t0]
>@@ -2355,7 +2355,7 @@ macro loadWithStructureCheck(operand, slowPath)
> end
> 
> macro getProperty()
>-    loadisFromInstruction(6, t1)
>+    loadpFromInstruction(6, t1)
>     loadPropertyAtVariableOffset(t1, t0, t2)
>     valueProfile(t2, 7, t0)
>     loadisFromInstruction(1, t0)
>@@ -2372,7 +2372,7 @@ macro getGlobalVar(tdzCheckIfNecessary)
> end
> 
> macro getClosureVar()
>-    loadisFromInstruction(6, t1)
>+    loadpFromInstruction(6, t1)
>     loadq JSLexicalEnvironment_variables[t0, t1, 8], t0
>     valueProfile(t0, 7, t1)
>     loadisFromInstruction(1, t1)
>@@ -2445,7 +2445,7 @@ _llint_op_get_from_scope:
> macro putProperty()
>     loadisFromInstruction(3, t1)
>     loadConstantOrVariable(t1, t2)
>-    loadisFromInstruction(6, t1)
>+    loadpFromInstruction(6, t1)
>     storePropertyAtVariableOffset(t1, t0, t2)
> end
> 
>@@ -2461,7 +2461,7 @@ end
> macro putClosureVar()
>     loadisFromInstruction(3, t1)
>     loadConstantOrVariable(t1, t2)
>-    loadisFromInstruction(6, t1)
>+    loadpFromInstruction(6, t1)
>     storeq t2, JSLexicalEnvironment_variables[t0, t1, 8]
> end
> 
>@@ -2472,7 +2472,7 @@ macro putLocalClosureVar()
>     btpz t3, .noVariableWatchpointSet
>     notifyWrite(t3, .pDynamic)
> .noVariableWatchpointSet:
>-    loadisFromInstruction(6, t1)
>+    loadpFromInstruction(6, t1)
>     storeq t2, JSLexicalEnvironment_variables[t0, t1, 8]
> end
> 
>diff --git a/Source/JavaScriptCore/runtime/CommonSlowPaths.h b/Source/JavaScriptCore/runtime/CommonSlowPaths.h
>index 1ece89592cd63118dd9b89f1b96bd008dd0ab5ed..e2630118364c555a267171b12af054b967c5a17a 100644
>--- a/Source/JavaScriptCore/runtime/CommonSlowPaths.h
>+++ b/Source/JavaScriptCore/runtime/CommonSlowPaths.h
>@@ -138,7 +138,7 @@ inline void tryCachePutToScopeGlobal(
>             ASSERT(!entry.isNull());
>             ConcurrentJSLocker locker(codeBlock->m_lock);
>             pc[5].u.watchpointSet = entry.watchpointSet();
>-            pc[6].u.pointer = static_cast<void*>(globalLexicalEnvironment->variableAt(entry.scopeOffset()).slot());
>+            pc[6].u.pointer = globalLexicalEnvironment->variableAt(entry.scopeOffset()).slot();
>         }
>     }
>     
>@@ -162,7 +162,7 @@ inline void tryCachePutToScopeGlobal(
> 
>         ConcurrentJSLocker locker(codeBlock->m_lock);
>         pc[5].u.structure.set(vm, codeBlock, scope->structure(vm));
>-        pc[6].u.operand = slot.cachedOffset();
>+        pc[6].u.operandPointer = slot.cachedOffset();
>     }
> }
> 
>@@ -186,7 +186,7 @@ inline void tryCacheGetFromScopeGlobal(
>             ConcurrentJSLocker locker(exec->codeBlock()->m_lock);
>             pc[4].u.operand = GetPutInfo(getPutInfo.resolveMode(), newResolveType, getPutInfo.initializationMode()).operand();
>             pc[5].u.watchpointSet = entry.watchpointSet();
>-            pc[6].u.pointer = static_cast<void*>(globalLexicalEnvironment->variableAt(entry.scopeOffset()).slot());
>+            pc[6].u.pointer = globalLexicalEnvironment->variableAt(entry.scopeOffset()).slot();
>         }
>     }
> 
>@@ -200,7 +200,7 @@ inline void tryCacheGetFromScopeGlobal(
>             {
>                 ConcurrentJSLocker locker(codeBlock->m_lock);
>                 pc[5].u.structure.set(vm, codeBlock, structure);
>-                pc[6].u.operand = slot.cachedOffset();
>+                pc[6].u.operandPointer = slot.cachedOffset();
>             }
>             structure->startWatchingPropertyForReplacements(vm, slot.cachedOffset());
>         }

Actions: View | Formatted Diff | Diff

Attachments on bug 132333: 230367 | 230369 | 230475 | 311588 | 311689 | 311721 | 342072 | 342075 | 342697 | 360475 | 360479 | 360482