Attachment 358213 Details for Bug 193085 – Patch

[patch] Patch

bug-193085-20190103001547.patch (text/plain), 11.61 KB, created by Tadeu Zagallo on 2019-01-02 15:15:50 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Tadeu Zagallo

Created: 2019-01-02 15:15:50 PST

Size: 11.61 KB

patch

obsolete

>Subversion Revision: 239556
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index 17e010891210bef58b46a33cef329ca3a41b7833..2c5eb16eb3c5b15fd7ea3746a6d15d767e2edc20 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,40 @@
>+2019-01-02  Tadeu Zagallo  <tzagallo@apple.com>
>+
>+        Baseline version of get_by_id may corrupt metadata
>+        https://bugs.webkit.org/show_bug.cgi?id=193085
>+        <rdar://problem/23453006>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        The Baseline version of get_by_id unconditionally calls `emitArrayProfilingSiteForBytecodeIndexWithCell`
>+        if the property is `length`. However, since the bytecode rewrite, get_by_id only has an ArrayProfile entry
>+        in the metadata if its mode is `GetByIdMode::ArrayLength`. That might result in one of two bad things:
>+        1) get_by_id's mode is not ArrayLength, and a duplicate, out-of-line ArrayProfile entry will be created by
>+        `CodeBlock::getOrAddArrayProfile`.
>+        2) get_by_id's mode *is* ArrayLength and we generate the array profiling code pointing to the ArrayProfile
>+        that lives in the metadata table. This works fine as long as get_by_id does not change modes. If that happens,
>+        the JIT code will write into the metadata table, overwriting the 'GetByIdModeMetadata` for another mode.
>+
>+        Add a check to the Baseline version of get_by_id so that we only do the ArrayProfiling if the get_by_id's
>+        mode is ArrayLength
>+
>+        * bytecode/BytecodeList.rb:
>+        * bytecode/CodeBlock.cpp:
>+        (JSC::CodeBlock::getArrayProfile):
>+        (JSC::CodeBlock::addArrayProfile): Deleted.
>+        (JSC::CodeBlock::getOrAddArrayProfile): Deleted.
>+        * bytecode/CodeBlock.h:
>+        (JSC::CodeBlock::numberOfArrayProfiles const): Deleted.
>+        (JSC::CodeBlock::arrayProfiles): Deleted.
>+        * bytecode/CodeBlockInlines.h:
>+        (JSC::CodeBlock::forEachArrayProfile):
>+        * jit/JITInlines.h:
>+        (JSC::JIT::emitArrayProfilingSiteForBytecodeIndexWithCell):
>+        * jit/JITPropertyAccess.cpp:
>+        (JSC::JIT::emit_op_get_by_id):
>+        * llint/LLIntSlowPaths.cpp:
>+        (JSC::LLInt::LLINT_SLOW_PATH_DECL):
>+
> 2018-12-27  Alex Christensen  <achristensen@webkit.org>
> 
>         Resurrect Mac CMake build
>diff --git a/Source/JavaScriptCore/bytecode/BytecodeList.rb b/Source/JavaScriptCore/bytecode/BytecodeList.rb
>index a0672d5f4eb8056f55257ae89d7b1325ac9dbae3..211532d0f6602dd178c019c6adae43fc682bff1e 100644
>--- a/Source/JavaScriptCore/bytecode/BytecodeList.rb
>+++ b/Source/JavaScriptCore/bytecode/BytecodeList.rb
>@@ -413,8 +413,8 @@ op :get_by_id,
>     },
>     metadata: {
>         mode: GetByIdMode,
>-        modeMetadata: GetByIdModeMetadata,
>         hitCountForLLIntCaching: unsigned,
>+        modeMetadata: GetByIdModeMetadata,
>         profile: ValueProfile,
>     }
> 
>@@ -473,8 +473,8 @@ op :put_by_id,
>         oldStructure: StructureID,
>         offset: unsigned,
>         newStructure: StructureID,
>-        structureChain: WriteBarrierBase[StructureChain],
>         flags: PutByIdFlags,
>+        structureChain: WriteBarrierBase[StructureChain],
>     },
>     metadata_initializers: {
>         flags: :flags
>diff --git a/Source/JavaScriptCore/bytecode/CodeBlock.cpp b/Source/JavaScriptCore/bytecode/CodeBlock.cpp
>index 7afda363172aab86f39cafa383ad87e6675c6272..8db1dec408b14941bab78d5843362baa3eb92132 100644
>--- a/Source/JavaScriptCore/bytecode/CodeBlock.cpp
>+++ b/Source/JavaScriptCore/bytecode/CodeBlock.cpp
>@@ -2494,20 +2494,14 @@ ArrayProfile* CodeBlock::getArrayProfile(const ConcurrentJSLocker&, unsigned byt
>     case OpGetById::opcodeID: {
>         auto bytecode = instruction->as<OpGetById>();
>         auto& metadata = bytecode.metadata(this);
>-        if (metadata.mode == GetByIdMode::ArrayLength)
>-            return &metadata.modeMetadata.arrayLengthMode.arrayProfile;
>-        break;
>+        return &metadata.modeMetadata.arrayLengthMode.arrayProfile;
>     }
> 
>     default:
>         break;
>     }
> 
>-    for (auto& m_arrayProfile : m_arrayProfiles) {
>-        if (m_arrayProfile.bytecodeOffset() == bytecodeOffset)
>-            return &m_arrayProfile;
>-    }
>-    return 0;
>+    return nullptr;
> }
> 
> ArrayProfile* CodeBlock::getArrayProfile(unsigned bytecodeOffset)
>@@ -2516,33 +2510,6 @@ ArrayProfile* CodeBlock::getArrayProfile(unsigned bytecodeOffset)
>     return getArrayProfile(locker, bytecodeOffset);
> }
> 
>-ArrayProfile* CodeBlock::addArrayProfile(const ConcurrentJSLocker&, unsigned bytecodeOffset)
>-{
>-    m_arrayProfiles.append(ArrayProfile(bytecodeOffset));
>-    return &m_arrayProfiles.last();
>-}
>-
>-ArrayProfile* CodeBlock::addArrayProfile(unsigned bytecodeOffset)
>-{
>-    ConcurrentJSLocker locker(m_lock);
>-    return addArrayProfile(locker, bytecodeOffset);
>-}
>-
>-ArrayProfile* CodeBlock::getOrAddArrayProfile(const ConcurrentJSLocker& locker, unsigned bytecodeOffset)
>-{
>-    ArrayProfile* result = getArrayProfile(locker, bytecodeOffset);
>-    if (result)
>-        return result;
>-    return addArrayProfile(locker, bytecodeOffset);
>-}
>-
>-ArrayProfile* CodeBlock::getOrAddArrayProfile(unsigned bytecodeOffset)
>-{
>-    ConcurrentJSLocker locker(m_lock);
>-    return getOrAddArrayProfile(locker, bytecodeOffset);
>-}
>-
>-
> #if ENABLE(DFG_JIT)
> Vector<CodeOrigin, 0, UnsafeVectorOverflow>& CodeBlock::codeOrigins()
> {
>diff --git a/Source/JavaScriptCore/bytecode/CodeBlock.h b/Source/JavaScriptCore/bytecode/CodeBlock.h
>index 8d23bbcdeed0581f53b6c6986251eee63d7e6b88..6c766a8ab229db0f683e66c06b7d5c50b8e81217 100644
>--- a/Source/JavaScriptCore/bytecode/CodeBlock.h
>+++ b/Source/JavaScriptCore/bytecode/CodeBlock.h
>@@ -464,14 +464,8 @@ public:
> 
>     bool couldTakeSpecialFastCase(InstructionStream::Offset bytecodeOffset);
> 
>-    unsigned numberOfArrayProfiles() const { return m_arrayProfiles.size(); }
>-    const ArrayProfileVector& arrayProfiles() { return m_arrayProfiles; }
>-    ArrayProfile* addArrayProfile(const ConcurrentJSLocker&, unsigned bytecodeOffset);
>-    ArrayProfile* addArrayProfile(unsigned bytecodeOffset);
>     ArrayProfile* getArrayProfile(const ConcurrentJSLocker&, unsigned bytecodeOffset);
>     ArrayProfile* getArrayProfile(unsigned bytecodeOffset);
>-    ArrayProfile* getOrAddArrayProfile(const ConcurrentJSLocker&, unsigned bytecodeOffset);
>-    ArrayProfile* getOrAddArrayProfile(unsigned bytecodeOffset);
> 
>     // Exception handling support
> 
>@@ -987,7 +981,6 @@ private:
>     RefCountedArray<ValueProfile> m_argumentValueProfiles;
>     Vector<std::unique_ptr<ValueProfileAndOperandBuffer>> m_catchProfiles;
>     SegmentedVector<RareCaseProfile, 8> m_rareCaseProfiles;
>-    ArrayProfileVector m_arrayProfiles;
> 
>     // Constant Pool
>     COMPILE_ASSERT(sizeof(Register) == sizeof(WriteBarrier<Unknown>), Register_must_be_same_size_as_WriteBarrier_Unknown);
>diff --git a/Source/JavaScriptCore/bytecode/CodeBlockInlines.h b/Source/JavaScriptCore/bytecode/CodeBlockInlines.h
>index f81f9d5f6b63f2d2d56872bbc1a9e363a95ce98c..ed63eb3b59a687194f2605c1a530f56f9034c0f5 100644
>--- a/Source/JavaScriptCore/bytecode/CodeBlockInlines.h
>+++ b/Source/JavaScriptCore/bytecode/CodeBlockInlines.h
>@@ -51,9 +51,6 @@ void CodeBlock::forEachValueProfile(const Functor& func)
> template<typename Functor>
> void CodeBlock::forEachArrayProfile(const Functor& func)
> {
>-    for (auto& arrayProfile : m_arrayProfiles)
>-        func(arrayProfile);
>-
>     if (m_metadata) {
>         m_metadata->forEach<OpGetById>([&] (auto& metadata) {
>             if (metadata.mode == GetByIdMode::ArrayLength)
>diff --git a/Source/JavaScriptCore/jit/JITInlines.h b/Source/JavaScriptCore/jit/JITInlines.h
>index d6c7b7fa788cc368278cedc61347ead6d301b129..1e3c044f4f2e477cbac90a1d6237c03208cbeaff 100644
>--- a/Source/JavaScriptCore/jit/JITInlines.h
>+++ b/Source/JavaScriptCore/jit/JITInlines.h
>@@ -356,7 +356,7 @@ inline void JIT::emitArrayProfilingSiteWithCell(RegisterID cell, RegisterID inde
> 
> inline void JIT::emitArrayProfilingSiteForBytecodeIndexWithCell(RegisterID cell, RegisterID indexingType, unsigned bytecodeIndex)
> {
>-    emitArrayProfilingSiteWithCell(cell, indexingType, m_codeBlock->getOrAddArrayProfile(bytecodeIndex));
>+    emitArrayProfilingSiteWithCell(cell, indexingType, m_codeBlock->getArrayProfile(bytecodeIndex));
> }
> 
> inline void JIT::emitArrayProfileStoreToHoleSpecialCase(ArrayProfile* arrayProfile)
>diff --git a/Source/JavaScriptCore/jit/JITPropertyAccess.cpp b/Source/JavaScriptCore/jit/JITPropertyAccess.cpp
>index bd785be866d6d83e6a6941a977300b5ad42ab49f..c607a786dd8836e0bf82d2c68a995a3e3fba826f 100644
>--- a/Source/JavaScriptCore/jit/JITPropertyAccess.cpp
>+++ b/Source/JavaScriptCore/jit/JITPropertyAccess.cpp
>@@ -572,6 +572,7 @@ void JIT::emitSlow_op_get_by_id_direct(const Instruction* currentInstruction, Ve
> void JIT::emit_op_get_by_id(const Instruction* currentInstruction)
> {
>     auto bytecode = currentInstruction->as<OpGetById>();
>+    auto& metadata = bytecode.metadata(m_codeBlock);
>     int resultVReg = bytecode.dst.offset();
>     int baseVReg = bytecode.base.offset();
>     const Identifier* ident = &(m_codeBlock->identifier(bytecode.property));
>@@ -580,8 +581,11 @@ void JIT::emit_op_get_by_id(const Instruction* currentInstruction)
>     
>     emitJumpSlowCaseIfNotJSCell(regT0, baseVReg);
>     
>-    if (*ident == m_vm->propertyNames->length && shouldEmitProfiling())
>+    if (*ident == m_vm->propertyNames->length && shouldEmitProfiling()) {
>+        Jump notArrayLengthMode = branch8(NotEqual, AbsoluteAddress(&metadata.mode), TrustedImm32(static_cast<uint8_t>(GetByIdMode::ArrayLength)));
>         emitArrayProfilingSiteForBytecodeIndexWithCell(regT0, regT1, m_bytecodeOffset);
>+        notArrayLengthMode.link(this);
>+    }
> 
>     JITGetByIdGenerator gen(
>         m_codeBlock, CodeOrigin(m_bytecodeOffset), CallSiteIndex(m_bytecodeOffset), RegisterSet::stubUnavailableRegisters(),
>diff --git a/Source/JavaScriptCore/llint/LLIntSlowPaths.cpp b/Source/JavaScriptCore/llint/LLIntSlowPaths.cpp
>index e7a62a7daff0f62e4b1f38a2e86de3095feba88d..7f221cca549ff85a3ce813b969a49175721722cf 100644
>--- a/Source/JavaScriptCore/llint/LLIntSlowPaths.cpp
>+++ b/Source/JavaScriptCore/llint/LLIntSlowPaths.cpp
>@@ -825,6 +825,7 @@ LLINT_SLOW_PATH_DECL(slow_path_get_by_id)
>         && isJSArray(baseValue)
>         && ident == vm.propertyNames->length) {
>         metadata.mode = GetByIdMode::ArrayLength;
>+        new (&metadata.modeMetadata.arrayLengthMode.arrayProfile) ArrayProfile(codeBlock->bytecodeOffset(pc));
>         metadata.modeMetadata.arrayLengthMode.arrayProfile.observeStructure(baseValue.asCell()->structure(vm));
> 
>         // Prevent the prototype cache from ever happening.
>diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog
>index 9e350a323faf25b7963d0be326807ff5799ed587..5e30f621abb58825baf5d980e47fd796b9729441 100644
>--- a/JSTests/ChangeLog
>+++ b/JSTests/ChangeLog
>@@ -1,3 +1,14 @@
>+2019-01-02  Tadeu Zagallo  <tzagallo@apple.com>
>+
>+        Baseline version of get_by_id may corrupt metadata
>+        https://bugs.webkit.org/show_bug.cgi?id=193085
>+        <rdar://problem/23453006>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * stress/get-by-id-change-mode.js: Added.
>+        (forEach):
>+
> 2018-12-13  Yusuke Suzuki  <yusukesuzuki@slowstart.org>
> 
>         [BigInt] Support BigInt in JSON.stringify
>diff --git a/JSTests/stress/get-by-id-change-mode.js b/JSTests/stress/get-by-id-change-mode.js
>new file mode 100644
>index 0000000000000000000000000000000000000000..647588f49cdb06f3c7f757e0dd596fc52afdc3c8
>--- /dev/null
>+++ b/JSTests/stress/get-by-id-change-mode.js
>@@ -0,0 +1,12 @@
>+//@ runDefault("--useConcurrentJIT=0")
>+
>+forEach({ length: 5 }, function() {
>+    for (var i = 0; i < 10; i++) {
>+        forEach([1], function() {});
>+    }
>+});
>+
>+function forEach(a, b) {
>+    for (var c = 0; c < a.length; c++)
>+        b();
>+}

Actions: View | Formatted Diff | Diff

Attachments on bug 193085: 358193 | 358202 | 358213 | 358225 | 358228 | 358239 | 358251