Baseline GetByVal and PutByVal for cache ID stubs need to handle exceptions
authormsaboff@apple.com <msaboff@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 10 Aug 2016 23:45:05 +0000 (23:45 +0000)
committermsaboff@apple.com <msaboff@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 10 Aug 2016 23:45:05 +0000 (23:45 +0000)
https://bugs.webkit.org/show_bug.cgi?id=160749

Reviewed by Filip Pizlo.

JSTests:

New test that causes baseline GetByValWithCachedId and PutByValWithCachedId
stubs to be generated and then throws exceptions for those stub to handle
to verify that they are properly handled.

* stress/regress-160749.js: Added.
(testCachedGetByVal.):
(testCachedGetByVal.get for):
(testCachedGetByVal):
(testCachedPutByVal.):
(testCachedPutByVal.set for):
(testCachedPutByVal):

Source/JavaScriptCore:

We were emitting "callOperation()" calls in emitGetByValWithCachedId() and
emitPutByValWithCachedId() without linking the exception checks created by the
code emitted.  This manifested itself in various ways depending on the processor.
This is due to what the destination is for an unlinked branch.  On X86, an unlinked
branch goes tot he next instructions.  On ARM64, we end up with an infinite loop
as we branch to the same instruction.  On ARM we branch to 0 as the branch is to
an absolute address of 0.

Now we save the exception handler address for the original generated function and
link the exception cases for these by-val stubs to this handler.

* bytecode/ByValInfo.h:
(JSC::ByValInfo::ByValInfo): Added the address of the exception handler we should
link to.

* jit/JIT.cpp:
(JSC::JIT::link): Compute the linked exception handler address and pass it to
the ByValInfo constructor.
(JSC::JIT::privateCompileExceptionHandlers): Make sure that we generate the
exception handler if we have any by-val handlers.

* jit/JIT.h:
Added a label for the exception handler.  We'll link this later for the
by value handlers.

* jit/JITPropertyAccess.cpp:
(JSC::JIT::privateCompileGetByValWithCachedId):
(JSC::JIT::privateCompilePutByValWithCachedId):
Link exception branches to the exception handler for the main function.

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

JSTests/ChangeLog
JSTests/stress/regress-160749.js [new file with mode: 0644]
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecode/ByValInfo.h
Source/JavaScriptCore/jit/JIT.cpp
Source/JavaScriptCore/jit/JIT.h
Source/JavaScriptCore/jit/JITPropertyAccess.cpp

index fdabcb2..1a9e1cd 100644 (file)
@@ -1,3 +1,22 @@
+2016-08-10  Michael Saboff  <msaboff@apple.com>
+
+        Baseline GetByVal and PutByVal for cache ID stubs need to handle exceptions
+        https://bugs.webkit.org/show_bug.cgi?id=160749
+
+        Reviewed by Filip Pizlo.
+
+        New test that causes baseline GetByValWithCachedId and PutByValWithCachedId
+        stubs to be generated and then throws exceptions for those stub to handle
+        to verify that they are properly handled.
+
+        * stress/regress-160749.js: Added.
+        (testCachedGetByVal.):
+        (testCachedGetByVal.get for):
+        (testCachedGetByVal):
+        (testCachedPutByVal.):
+        (testCachedPutByVal.set for):
+        (testCachedPutByVal):
+
 2016-08-10  Mark Lam  <mark.lam@apple.com>
 
         DFG's flushForTerminal() needs to add PhantomLocals for bytecode live locals.
diff --git a/JSTests/stress/regress-160749.js b/JSTests/stress/regress-160749.js
new file mode 100644 (file)
index 0000000..9fcd7f7
--- /dev/null
@@ -0,0 +1,91 @@
+// Regression test for 160749.  This test should not exit with an error or crash.
+// Check that the Baseline JIT GetByValWithCacheId and PutByValWithCahcedId stubs properly handle exceptions.
+
+function testCachedGetByVal()
+{
+    o = { };
+    o['a'] = 42;
+
+    let result = 0;
+    let loopCount = 100000;
+    let interationToChange = 90000;
+    let expectedResult = 42 * interationToChange;
+    let exceptions = 0;
+    let expectedExceptions = loopCount - interationToChange;
+
+    for (let i = 0; i < loopCount; i++) {
+        if (i == interationToChange) {
+            Object.defineProperty(o, "a", {
+                enumerable: true,
+                get: function() { throw "error"; return 100; }
+            });
+        }
+
+        for (let v in o) {
+            try {
+                result += o[v.toString()];
+            } catch(e) {
+                if (e == "error")
+                    exceptions++;
+                else
+                    throw "Got wrong exception \"" + e + "\"";
+            }
+        }
+    }
+
+    if (result != expectedResult)
+        throw "Expected a result of " + expectedResult + ", but got " + result;
+    if (exceptions != expectedExceptions)
+        throw "1 Expected " + expectedExceptions + " exceptions, but got " + exceptions;
+}
+
+noDFG(testCachedGetByVal);
+
+function testCachedPutByVal()
+{
+    o = { };
+    o['a'] = 0;
+
+    let result = 0;
+    let loopCount = 100000;
+    let iterationToChange = 90000;
+    let exceptions = 0;
+    let expectedExceptions = loopCount - iterationToChange;
+
+    for (let i = 0; i < loopCount; i++) {
+        if (i == iterationToChange) {
+            result = o.a;
+            Object.defineProperty(o, "_a", {
+                enumerable: false,
+                value: -1
+            });
+            Object.defineProperty(o, "a", {
+                enumerable: true,
+                set: function(v) { throw "error"; o._a = v; }
+            });
+        }
+
+        for (let v in o) {
+            try {
+                o[v.toString()] = i + 1;
+            } catch(e) {
+                if (e == "error")
+                    exceptions++;
+                else
+                    throw "Got wrong exception \"" + e + "\"";
+            }
+        }
+    }
+
+    if (result != iterationToChange)
+        throw "Expected a result of " + result + ", but got " + o.a;
+    if (o._a != -1)
+        throw "Expected o._b to -1, but it is " + o._a;
+    if (exceptions != expectedExceptions)
+        throw "Expected " + expectedExceptions + " exceptions, but got " + exceptions;
+}
+
+noDFG(testCachedPutByVal);
+
+testCachedGetByVal();
+testCachedPutByVal();
index f53af6b..0c3fc4e 100644 (file)
@@ -1,3 +1,40 @@
+2016-08-10  Michael Saboff  <msaboff@apple.com>
+
+        Baseline GetByVal and PutByVal for cache ID stubs need to handle exceptions
+        https://bugs.webkit.org/show_bug.cgi?id=160749
+
+        Reviewed by Filip Pizlo.
+
+        We were emitting "callOperation()" calls in emitGetByValWithCachedId() and
+        emitPutByValWithCachedId() without linking the exception checks created by the
+        code emitted.  This manifested itself in various ways depending on the processor.
+        This is due to what the destination is for an unlinked branch.  On X86, an unlinked
+        branch goes tot he next instructions.  On ARM64, we end up with an infinite loop
+        as we branch to the same instruction.  On ARM we branch to 0 as the branch is to
+        an absolute address of 0.
+
+        Now we save the exception handler address for the original generated function and
+        link the exception cases for these by-val stubs to this handler.
+
+        * bytecode/ByValInfo.h:
+        (JSC::ByValInfo::ByValInfo): Added the address of the exception handler we should
+        link to.
+
+        * jit/JIT.cpp:
+        (JSC::JIT::link): Compute the linked exception handler address and pass it to
+        the ByValInfo constructor.
+        (JSC::JIT::privateCompileExceptionHandlers): Make sure that we generate the
+        exception handler if we have any by-val handlers.
+
+        * jit/JIT.h:
+        Added a label for the exception handler.  We'll link this later for the
+        by value handlers.
+
+        * jit/JITPropertyAccess.cpp:
+        (JSC::JIT::privateCompileGetByValWithCachedId):
+        (JSC::JIT::privateCompilePutByValWithCachedId):
+        Link exception branches to the exception handler for the main function.
+
 2016-08-10  Mark Lam  <mark.lam@apple.com>
 
         DFG's flushForTerminal() needs to add PhantomLocals for bytecode live locals.
index 89886f9..545cdcc 100644 (file)
@@ -207,10 +207,11 @@ inline JITArrayMode jitArrayModeForStructure(Structure* structure)
 struct ByValInfo {
     ByValInfo() { }
 
-    ByValInfo(unsigned bytecodeIndex, CodeLocationJump notIndexJump, CodeLocationJump badTypeJump, JITArrayMode arrayMode, ArrayProfile* arrayProfile, int16_t badTypeJumpToDone, int16_t badTypeJumpToNextHotPath, int16_t returnAddressToSlowPath)
+    ByValInfo(unsigned bytecodeIndex, CodeLocationJump notIndexJump, CodeLocationJump badTypeJump, CodeLocationLabel exceptionHandler, JITArrayMode arrayMode, ArrayProfile* arrayProfile, int16_t badTypeJumpToDone, int16_t badTypeJumpToNextHotPath, int16_t returnAddressToSlowPath)
         : bytecodeIndex(bytecodeIndex)
         , notIndexJump(notIndexJump)
         , badTypeJump(badTypeJump)
+        , exceptionHandler(exceptionHandler)
         , arrayMode(arrayMode)
         , arrayProfile(arrayProfile)
         , badTypeJumpToDone(badTypeJumpToDone)
@@ -226,6 +227,7 @@ struct ByValInfo {
     unsigned bytecodeIndex;
     CodeLocationJump notIndexJump;
     CodeLocationJump badTypeJump;
+    CodeLocationLabel exceptionHandler;
     JITArrayMode arrayMode; // The array mode that was baked into the inline JIT code.
     ArrayProfile* arrayProfile;
     int16_t badTypeJumpToDone;
index 8fd3f58..9fa764a 100644 (file)
@@ -725,27 +725,33 @@ CompilationResult JIT::link()
     for (unsigned i = m_putByIds.size(); i--;)
         m_putByIds[i].finalize(patchBuffer);
 
-    for (const auto& byValCompilationInfo : m_byValCompilationInfo) {
-        PatchableJump patchableNotIndexJump = byValCompilationInfo.notIndexJump;
-        CodeLocationJump notIndexJump = CodeLocationJump();
-        if (Jump(patchableNotIndexJump).isSet())
-            notIndexJump = CodeLocationJump(patchBuffer.locationOf(patchableNotIndexJump));
-        CodeLocationJump badTypeJump = CodeLocationJump(patchBuffer.locationOf(byValCompilationInfo.badTypeJump));
-        CodeLocationLabel doneTarget = patchBuffer.locationOf(byValCompilationInfo.doneTarget);
-        CodeLocationLabel nextHotPathTarget = patchBuffer.locationOf(byValCompilationInfo.nextHotPathTarget);
-        CodeLocationLabel slowPathTarget = patchBuffer.locationOf(byValCompilationInfo.slowPathTarget);
-        CodeLocationCall returnAddress = patchBuffer.locationOf(byValCompilationInfo.returnAddress);
-
-        *byValCompilationInfo.byValInfo = ByValInfo(
-            byValCompilationInfo.bytecodeIndex,
-            notIndexJump,
-            badTypeJump,
-            byValCompilationInfo.arrayMode,
-            byValCompilationInfo.arrayProfile,
-            differenceBetweenCodePtr(badTypeJump, doneTarget),
-            differenceBetweenCodePtr(badTypeJump, nextHotPathTarget),
-            differenceBetweenCodePtr(returnAddress, slowPathTarget));
+    if (m_byValCompilationInfo.size()) {
+        CodeLocationLabel exceptionHandler = patchBuffer.locationOf(m_exceptionHandler);
+
+        for (const auto& byValCompilationInfo : m_byValCompilationInfo) {
+            PatchableJump patchableNotIndexJump = byValCompilationInfo.notIndexJump;
+            CodeLocationJump notIndexJump = CodeLocationJump();
+            if (Jump(patchableNotIndexJump).isSet())
+                notIndexJump = CodeLocationJump(patchBuffer.locationOf(patchableNotIndexJump));
+            CodeLocationJump badTypeJump = CodeLocationJump(patchBuffer.locationOf(byValCompilationInfo.badTypeJump));
+            CodeLocationLabel doneTarget = patchBuffer.locationOf(byValCompilationInfo.doneTarget);
+            CodeLocationLabel nextHotPathTarget = patchBuffer.locationOf(byValCompilationInfo.nextHotPathTarget);
+            CodeLocationLabel slowPathTarget = patchBuffer.locationOf(byValCompilationInfo.slowPathTarget);
+            CodeLocationCall returnAddress = patchBuffer.locationOf(byValCompilationInfo.returnAddress);
+
+            *byValCompilationInfo.byValInfo = ByValInfo(
+                byValCompilationInfo.bytecodeIndex,
+                notIndexJump,
+                badTypeJump,
+                exceptionHandler,
+                byValCompilationInfo.arrayMode,
+                byValCompilationInfo.arrayProfile,
+                differenceBetweenCodePtr(badTypeJump, doneTarget),
+                differenceBetweenCodePtr(badTypeJump, nextHotPathTarget),
+                differenceBetweenCodePtr(returnAddress, slowPathTarget));
+        }
     }
+
     for (unsigned i = 0; i < m_callCompilationInfo.size(); ++i) {
         CallCompilationInfo& compilationInfo = m_callCompilationInfo[i];
         CallLinkInfo& info = *compilationInfo.callLinkInfo;
@@ -825,7 +831,8 @@ void JIT::privateCompileExceptionHandlers()
         jumpToExceptionHandler();
     }
 
-    if (!m_exceptionChecks.empty()) {
+    if (!m_exceptionChecks.empty() || m_byValCompilationInfo.size()) {
+        m_exceptionHandler = label();
         m_exceptionChecks.link(this);
 
         copyCalleeSavesToVMEntryFrameCalleeSavesBuffer();
index 5c36066..699953a 100644 (file)
@@ -943,6 +943,7 @@ namespace JSC {
 
         JumpList m_exceptionChecks;
         JumpList m_exceptionChecksWithCallFrameRollback;
+        Label m_exceptionHandler;
 
         unsigned m_getByIdIndex;
         unsigned m_putByIdIndex;
index 14fb940..bba0f01 100644 (file)
@@ -1331,6 +1331,8 @@ void JIT::privateCompileGetByValWithCachedId(ByValInfo* byValInfo, ReturnAddress
     patchBuffer.link(slowCases, CodeLocationLabel(MacroAssemblerCodePtr::createFromExecutableAddress(returnAddress.value())).labelAtOffset(byValInfo->returnAddressToSlowPath));
     patchBuffer.link(fastDoneCase, byValInfo->badTypeJump.labelAtOffset(byValInfo->badTypeJumpToDone));
     patchBuffer.link(slowDoneCase, byValInfo->badTypeJump.labelAtOffset(byValInfo->badTypeJumpToNextHotPath));
+    if (!m_exceptionChecks.empty())
+        patchBuffer.link(m_exceptionChecks, byValInfo->exceptionHandler);
 
     for (const auto& callSite : m_calls) {
         if (callSite.to)
@@ -1419,6 +1421,9 @@ void JIT::privateCompilePutByValWithCachedId(ByValInfo* byValInfo, ReturnAddress
     LinkBuffer patchBuffer(*m_vm, *this, m_codeBlock);
     patchBuffer.link(slowCases, CodeLocationLabel(MacroAssemblerCodePtr::createFromExecutableAddress(returnAddress.value())).labelAtOffset(byValInfo->returnAddressToSlowPath));
     patchBuffer.link(doneCases, byValInfo->badTypeJump.labelAtOffset(byValInfo->badTypeJumpToDone));
+    if (!m_exceptionChecks.empty())
+        patchBuffer.link(m_exceptionChecks, byValInfo->exceptionHandler);
+
     for (const auto& callSite : m_calls) {
         if (callSite.to)
             patchBuffer.link(callSite.from, FunctionPtr(callSite.to));