Incorrect inequality for checking whether a statement is within bounds of a handler

https://bugs.webkit.org/show_bug.cgi?id=104313
<rdar://problem/12808934>

Reviewed by Geoffrey Garen.

Source/JavaScriptCore:

The most relevant change is in handlerForBytecodeOffset(), which fixes the inequality
used for checking whether a handler is pertinent to the current instruction. '<' is
correct, but '<=' isn't, since the 'end' is not inclusive.

Also found, and addressed, a benign goof in how the finally inliner works: sometimes
we will have end > start. This falls out naturally from how the inliner works and how
we pop scopes in the bytecompiler, but it's sufficiently surprising that, to avoid any
future confusion, I added a comment and some code to prune those handlers out. Because
of how the handler resolution works, these handlers would have been skipped anyway.

Also made various fixes to debugging code, which was necessary for tracking this down.

* bytecode/CodeBlock.cpp:
(JSC::CodeBlock::dumpBytecode):
(JSC::CodeBlock::handlerForBytecodeOffset):
* bytecompiler/BytecodeGenerator.cpp:
(JSC::BytecodeGenerator::generate):
* bytecompiler/Label.h:
(JSC::Label::bind):
* interpreter/Interpreter.cpp:
(JSC::Interpreter::throwException):
* llint/LLIntExceptions.cpp:
(JSC::LLInt::interpreterThrowInCaller):
(JSC::LLInt::returnToThrow):
(JSC::LLInt::callToThrow):
* llint/LLIntSlowPaths.cpp:
(JSC::LLInt::LLINT_SLOW_PATH_DECL):
(JSC::LLInt::handleHostCall):

LayoutTests:

* fast/js/jsc-test-list:
* fast/js/script-tests/try-catch-try-try-catch-try-finally-return-catch-finally.js: Added.
(foo):
* fast/js/try-catch-try-try-catch-try-finally-return-catch-finally-expected.txt: Added.
* fast/js/try-catch-try-try-catch-try-finally-return-catch-finally.html: Added.

Change-Id: Ic199b40daa2f8be3fb4dd01a762323d7309dfb47
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@136927 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Reviewed-by: Jocelyn Turcotte <jocelyn.turcotte@digia.com>

6 files changed, 50 insertions, 14 deletions

diff --git a/Source/JavaScriptCore/bytecode/CodeBlock.cpp b/Source/JavaScriptCore/bytecode/CodeBlock.cpp
index 00209f236..fe9f6ac7c 100644
--- a/Source/JavaScriptCore/bytecode/CodeBlock.cpp
+++ b/Source/JavaScriptCore/bytecode/CodeBlock.cpp
@@ -568,7 +568,7 @@ void CodeBlock::dumpBytecode()
         dataLogF("\nException Handlers:\n");
         unsigned i = 0;
         do {
-            dataLogF("\t %d: { start: [%4d] end: [%4d] target: [%4d] }\n", i + 1, m_rareData->m_exceptionHandlers[i].start, m_rareData->m_exceptionHandlers[i].end, m_rareData->m_exceptionHandlers[i].target);
+            dataLogF("\t %d: { start: [%4d] end: [%4d] target: [%4d] depth: [%4d] }\n", i + 1, m_rareData->m_exceptionHandlers[i].start, m_rareData->m_exceptionHandlers[i].end, m_rareData->m_exceptionHandlers[i].target, m_rareData->m_exceptionHandlers[i].scopeDepth);
             ++i;
         } while (i < m_rareData->m_exceptionHandlers.size());
     }
@@ -2473,7 +2473,7 @@ HandlerInfo* CodeBlock::handlerForBytecodeOffset(unsigned bytecodeOffset)
     for (size_t i = 0; i < exceptionHandlers.size(); ++i) {
         // Handlers are ordered innermost first, so the first handler we encounter
         // that contains the source address is the correct handler to use.
-        if (exceptionHandlers[i].start <= bytecodeOffset && exceptionHandlers[i].end >= bytecodeOffset)
+        if (exceptionHandlers[i].start <= bytecodeOffset && exceptionHandlers[i].end > bytecodeOffset)
             return &exceptionHandlers[i];
     }
 
diff --git a/Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp b/Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp
index 35976257b..507241696 100644
--- a/Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp
+++ b/Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp
@@ -157,10 +157,38 @@ ParserError BytecodeGenerator::generate()
     
     for (unsigned i = 0; i < m_tryRanges.size(); ++i) {
         TryRange& range = m_tryRanges[i];
+        int start = range.start->bind();
+        int end = range.end->bind();
+        
+        // This will happen for empty try blocks and for some cases of finally blocks:
+        //
+        // try {
+        //    try {
+        //    } finally {
+        //        return 42;
+        //        // *HERE*
+        //    }
+        // } finally {
+        //    print("things");
+        // }
+        //
+        // The return will pop scopes to execute the outer finally block. But this includes
+        // popping the try context for the inner try. The try context is live in the fall-through
+        // part of the finally block not because we will emit a handler that overlaps the finally,
+        // but because we haven't yet had a chance to plant the catch target. Then when we finish
+        // emitting code for the outer finally block, we repush the try contex, this time with a
+        // new start index. But that means that the start index for the try range corresponding
+        // to the inner-finally-following-the-return (marked as "*HERE*" above) will be greater
+        // than the end index of the try block. This is harmless since end < start handlers will
+        // never get matched in our logic, but we do the runtime a favor and choose to not emit
+        // such handlers at all.
+        if (end <= start)
+            continue;
+        
         ASSERT(range.tryData->targetScopeDepth != UINT_MAX);
         UnlinkedHandlerInfo info = {
-            static_cast<uint32_t>(range.start->bind(0, 0)), static_cast<uint32_t>(range.end->bind(0, 0)),
-            static_cast<uint32_t>(range.tryData->target->bind(0, 0)),
+            static_cast<uint32_t>(start), static_cast<uint32_t>(end),
+            static_cast<uint32_t>(range.tryData->target->bind()),
             range.tryData->targetScopeDepth
         };
         m_codeBlock->addExceptionHandler(info);
diff --git a/Source/JavaScriptCore/bytecompiler/Label.h b/Source/JavaScriptCore/bytecompiler/Label.h
index 21fa46309..8b444de91 100644
--- a/Source/JavaScriptCore/bytecompiler/Label.h
+++ b/Source/JavaScriptCore/bytecompiler/Label.h
@@ -66,6 +66,12 @@ namespace JSC {
         int refCount() const { return m_refCount; }
 
         bool isForward() const { return m_location == invalidLocation; }
+        
+        int bind()
+        {
+            ASSERT(!isForward());
+            return bind(0, 0);
+        }
 
     private:
         typedef Vector<std::pair<int, int>, 8> JumpVector;
diff --git a/Source/JavaScriptCore/interpreter/Interpreter.cpp b/Source/JavaScriptCore/interpreter/Interpreter.cpp
index 9b69d1b3d..7d9e6f92e 100644
--- a/Source/JavaScriptCore/interpreter/Interpreter.cpp
+++ b/Source/JavaScriptCore/interpreter/Interpreter.cpp
@@ -785,9 +785,12 @@ NEVER_INLINE HandlerInfo* Interpreter::throwException(CallFrame*& callFrame, JSV
     JSScope* scope = callFrame->scope();
     int scopeDelta = 0;
     if (!codeBlock->needsFullScopeChain() || codeBlock->codeType() != FunctionCode 
-        || callFrame->uncheckedR(codeBlock->activationRegister()).jsValue())
-        scopeDelta = depth(codeBlock, scope) - handler->scopeDepth;
-    ASSERT(scopeDelta >= 0);
+        || callFrame->uncheckedR(codeBlock->activationRegister()).jsValue()) {
+        int currentDepth = depth(codeBlock, scope);
+        int targetDepth = handler->scopeDepth;
+        scopeDelta = currentDepth - targetDepth;
+        ASSERT(scopeDelta >= 0);
+    }
     while (scopeDelta--)
         scope = scope->next();
     callFrame->setScope(scope);
diff --git a/Source/JavaScriptCore/llint/LLIntExceptions.cpp b/Source/JavaScriptCore/llint/LLIntExceptions.cpp
index 5e6bba365..17c15aa51 100644
--- a/Source/JavaScriptCore/llint/LLIntExceptions.cpp
+++ b/Source/JavaScriptCore/llint/LLIntExceptions.cpp
@@ -50,7 +50,7 @@ void interpreterThrowInCaller(ExecState* exec, ReturnAddressPtr pc)
     JSGlobalData* globalData = &exec->globalData();
     NativeCallFrameTracer tracer(globalData, exec);
 #if LLINT_SLOW_PATH_TRACING
-    dataLogF("Throwing exception %s.\n", globalData->exception.description());
+    dataLog("Throwing exception ", globalData->exception, ".\n");
 #endif
     fixupPCforExceptionIfNeeded(exec);
     genericThrow(
@@ -69,7 +69,7 @@ Instruction* returnToThrow(ExecState* exec, Instruction* pc)
     JSGlobalData* globalData = &exec->globalData();
     NativeCallFrameTracer tracer(globalData, exec);
 #if LLINT_SLOW_PATH_TRACING
-    dataLogF("Throwing exception %s (returnToThrow).\n", globalData->exception.description());
+    dataLog("Throwing exception ", globalData->exception, " (returnToThrow).\n");
 #endif
     fixupPCforExceptionIfNeeded(exec);
     genericThrow(globalData, exec, globalData->exception, pc - exec->codeBlock()->instructions().begin());
@@ -82,7 +82,7 @@ void* callToThrow(ExecState* exec, Instruction* pc)
     JSGlobalData* globalData = &exec->globalData();
     NativeCallFrameTracer tracer(globalData, exec);
 #if LLINT_SLOW_PATH_TRACING
-    dataLogF("Throwing exception %s (callToThrow).\n", globalData->exception.description());
+    dataLog("Throwing exception ", globalData->exception, " (callToThrow).\n");
 #endif
     fixupPCforExceptionIfNeeded(exec);
     genericThrow(globalData, exec, globalData->exception, pc - exec->codeBlock()->instructions().begin());
diff --git a/Source/JavaScriptCore/llint/LLIntSlowPaths.cpp b/Source/JavaScriptCore/llint/LLIntSlowPaths.cpp
index 584100e50..0bd19d46f 100644
--- a/Source/JavaScriptCore/llint/LLIntSlowPaths.cpp
+++ b/Source/JavaScriptCore/llint/LLIntSlowPaths.cpp
@@ -635,8 +635,7 @@ LLINT_SLOW_PATH_DECL(slow_path_add)
     JSValue v2 = LLINT_OP_C(3).jsValue();
     
 #if LLINT_SLOW_PATH_TRACING
-    dataLogF("Trying to add %s", v1.description());
-    dataLogF(" to %s.\n", v2.description());
+    dataLog("Trying to add ", v1, " to ", v2, ".\n");
 #endif
     
     if (v1.isString() && !v2.isObject())
@@ -1367,7 +1366,7 @@ static SlowPathReturnType handleHostCall(ExecState* execCallee, Instruction* pc,
         }
         
 #if LLINT_SLOW_PATH_TRACING
-        dataLogF("Call callee is not a function: %s\n", callee.description());
+        dataLog("Call callee is not a function: ", callee, "\n");
 #endif
 
         ASSERT(callType == CallTypeNone);
@@ -1390,7 +1389,7 @@ static SlowPathReturnType handleHostCall(ExecState* execCallee, Instruction* pc,
     }
     
 #if LLINT_SLOW_PATH_TRACING
-    dataLogF("Constructor callee is not a function: %s\n", callee.description());
+    dataLog("Constructor callee is not a function: ", callee, "\n");
 #endif
 
     ASSERT(constructType == ConstructTypeNone);