Baseline version of get_by_id may corrupt metadata
authortzagallo@apple.com <tzagallo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 4 Jan 2019 19:52:26 +0000 (19:52 +0000)
committertzagallo@apple.com <tzagallo@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 4 Jan 2019 19:52:26 +0000 (19:52 +0000)
https://bugs.webkit.org/show_bug.cgi?id=193085
<rdar://problem/23453006>

Reviewed by Saam Barati.

JSTests:

* stress/get-by-id-change-mode.js: Added.
(forEach):

Source/JavaScriptCore:

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/JIT.h:
* jit/JITInlines.h:
(JSC::JIT::emitArrayProfilingSiteForBytecodeIndexWithCell): Deleted.
* jit/JITPropertyAccess.cpp:
(JSC::JIT::emit_op_get_by_id):
* jit/JITPropertyAccess32_64.cpp:
(JSC::JIT::emit_op_get_by_id):
* llint/LLIntSlowPaths.cpp:
(JSC::LLInt::LLINT_SLOW_PATH_DECL):

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

12 files changed:
JSTests/ChangeLog
JSTests/stress/get-by-id-change-mode.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecode/BytecodeList.rb
Source/JavaScriptCore/bytecode/CodeBlock.cpp
Source/JavaScriptCore/bytecode/CodeBlock.h
Source/JavaScriptCore/bytecode/CodeBlockInlines.h
Source/JavaScriptCore/jit/JIT.h
Source/JavaScriptCore/jit/JITInlines.h
Source/JavaScriptCore/jit/JITPropertyAccess.cpp
Source/JavaScriptCore/jit/JITPropertyAccess32_64.cpp
Source/JavaScriptCore/llint/LLIntSlowPaths.cpp

index a344798..12c1b78 100644 (file)
@@ -1,3 +1,14 @@
+2019-01-04  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 Saam Barati.
+
+        * stress/get-by-id-change-mode.js: Added.
+        (forEach):
+
 2019-01-02  Yusuke Suzuki  <yusukesuzuki@slowstart.org>
 
         [JSC] Optimize Object.prototype.toString
diff --git a/JSTests/stress/get-by-id-change-mode.js b/JSTests/stress/get-by-id-change-mode.js
new file mode 100644 (file)
index 0000000..647588f
--- /dev/null
@@ -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();
+}
index d2852b8..ef46089 100644 (file)
@@ -1,3 +1,43 @@
+2019-01-04  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 Saam Barati.
+
+        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/JIT.h:
+        * jit/JITInlines.h:
+        (JSC::JIT::emitArrayProfilingSiteForBytecodeIndexWithCell): Deleted.
+        * jit/JITPropertyAccess.cpp:
+        (JSC::JIT::emit_op_get_by_id):
+        * jit/JITPropertyAccess32_64.cpp:
+        (JSC::JIT::emit_op_get_by_id):
+        * llint/LLIntSlowPaths.cpp:
+        (JSC::LLInt::LLINT_SLOW_PATH_DECL):
+
 2019-01-02  Yusuke Suzuki  <yusukesuzuki@slowstart.org>
 
         [JSC] Optimize Object.prototype.toString
index a0672d5..211532d 100644 (file)
@@ -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
index 7afda36..86db869 100644 (file)
@@ -2498,16 +2498,11 @@ ArrayProfile* CodeBlock::getArrayProfile(const ConcurrentJSLocker&, unsigned byt
             return &metadata.modeMetadata.arrayLengthMode.arrayProfile;
         break;
     }
-
     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 +2511,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()
 {
index 8d23bbc..6c766a8 100644 (file)
@@ -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);
index f81f9d5..ed63eb3 100644 (file)
@@ -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)
index fc710b0..a1ca2ea 100644 (file)
@@ -365,7 +365,6 @@ namespace JSC {
         emitValueProfilingSiteIfProfiledOpcode(Op bytecode);
 
         void emitArrayProfilingSiteWithCell(RegisterID cell, RegisterID indexingType, ArrayProfile*);
-        void emitArrayProfilingSiteForBytecodeIndexWithCell(RegisterID cell, RegisterID indexingType, unsigned bytecodeIndex);
         void emitArrayProfileStoreToHoleSpecialCase(ArrayProfile*);
         void emitArrayProfileOutOfBoundsSpecialCase(ArrayProfile*);
         
index d6c7b7f..9ea8deb 100644 (file)
@@ -354,11 +354,6 @@ inline void JIT::emitArrayProfilingSiteWithCell(RegisterID cell, RegisterID inde
     load8(Address(cell, JSCell::indexingTypeAndMiscOffset()), indexingType);
 }
 
-inline void JIT::emitArrayProfilingSiteForBytecodeIndexWithCell(RegisterID cell, RegisterID indexingType, unsigned bytecodeIndex)
-{
-    emitArrayProfilingSiteWithCell(cell, indexingType, m_codeBlock->getOrAddArrayProfile(bytecodeIndex));
-}
-
 inline void JIT::emitArrayProfileStoreToHoleSpecialCase(ArrayProfile* arrayProfile)
 {
     store8(TrustedImm32(1), arrayProfile->addressOfMayStoreToHole());
index bd785be..ad1d3dc 100644 (file)
@@ -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())
-        emitArrayProfilingSiteForBytecodeIndexWithCell(regT0, regT1, m_bytecodeOffset);
+    if (*ident == m_vm->propertyNames->length && shouldEmitProfiling()) {
+        Jump notArrayLengthMode = branch8(NotEqual, AbsoluteAddress(&metadata.mode), TrustedImm32(static_cast<uint8_t>(GetByIdMode::ArrayLength)));
+        emitArrayProfilingSiteWithCell(regT0, regT1, &metadata.modeMetadata.arrayLengthMode.arrayProfile);
+        notArrayLengthMode.link(this);
+    }
 
     JITGetByIdGenerator gen(
         m_codeBlock, CodeOrigin(m_bytecodeOffset), CallSiteIndex(m_bytecodeOffset), RegisterSet::stubUnavailableRegisters(),
index 40ac4f3..e8314ca 100644 (file)
@@ -571,6 +571,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 dst = bytecode.dst.offset();
     int base = bytecode.base.offset();
     const Identifier* ident = &(m_codeBlock->identifier(bytecode.property));
@@ -578,8 +579,11 @@ void JIT::emit_op_get_by_id(const Instruction* currentInstruction)
     emitLoad(base, regT1, regT0);
     emitJumpSlowCaseIfNotJSCell(base, regT1);
 
-    if (*ident == m_vm->propertyNames->length && shouldEmitProfiling())
-        emitArrayProfilingSiteForBytecodeIndexWithCell(regT0, regT2, m_bytecodeOffset);
+    if (*ident == m_vm->propertyNames->length && shouldEmitProfiling()) {
+        Jump notArrayLengthMode = branch8(NotEqual, AbsoluteAddress(&metadata.mode), TrustedImm32(static_cast<uint8_t>(GetByIdMode::ArrayLength)));
+        emitArrayProfilingSiteWithCell(regT0, regT2, &metadata.modeMetadata.arrayLengthMode.arrayProfile);
+        notArrayLengthMode.link(this);
+    }
 
     JITGetByIdGenerator gen(
         m_codeBlock, CodeOrigin(m_bytecodeOffset), CallSiteIndex(currentInstruction), RegisterSet::stubUnavailableRegisters(),
index e7a62a7..7f221cc 100644 (file)
@@ -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.