Attachment 358193 Details for Bug 193085 – Patch

[patch] Patch

bug-193085-20190102213048.patch (text/plain), 4.29 KB, created by Tadeu Zagallo on 2019-01-02 12:30:50 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Tadeu Zagallo

Created: 2019-01-02 12:30:50 PST

Size: 4.29 KB

patch

obsolete

>Subversion Revision: 239556
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index 17e010891210bef58b46a33cef329ca3a41b7833..b9fddb73bd3dcb4dac39d0e977a31d3f2f80b73e 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,26 @@
>+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
>+
>+        * jit/JITPropertyAccess.cpp:
>+        (JSC::JIT::emit_op_get_by_id):
>+
> 2018-12-27  Alex Christensen  <achristensen@webkit.org>
> 
>         Resurrect Mac CMake build
>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/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