Rework IR code generation for try/catch/finally

Simplify the generated code. Add a special block to catch
exceptions thrown inside a catch() statement.

store the exception on the stack when entering finally and
rethrow it at the end. This ensure correct behavior for
break/continue/return statements inside finally.

Don't check for exceptions after calling push_catch_scope
and pop_scope in the JIT'ed code. This can lead to infinite
loops when throwing inside an exception handler.

Change-Id: I67e9325794e2fd25b0773b21e02fbaadb43faab0

Change-Id: Ic1ea9c0c43eec1d49177dc1ab4552a1da04e96fe
Reviewed-by: Simon Hausmann <simon.hausmann@digia.com>

14 files changed, 97 insertions, 61 deletions

diff --git a/src/qml/compiler/qv4codegen.cpp b/src/qml/compiler/qv4codegen.cpp
index b7ea7491d6..b366582ade 100644
--- a/src/qml/compiler/qv4codegen.cpp
+++ b/src/qml/compiler/qv4codegen.cpp
@@ -2328,57 +2328,47 @@ bool Codegen::visit(TryStatement *ast)
             (ast->catchExpression->name == QLatin1String("eval") || ast->catchExpression->name == QLatin1String("arguments")))
         throwSyntaxError(ast->catchExpression->identifierToken, QCoreApplication::translate("qv4codegen", "Catch variable name may not be eval or arguments in strict mode"));
 
+    V4IR::BasicBlock *surroundingExceptionHandler = exceptionHandler();
+
     // We always need a finally body to clean up the exception handler
     // exceptions thrown in finally get catched by the surrounding catch block
-    V4IR::BasicBlock *finallyBody = _function->newBasicBlock(groupStartBlock(), exceptionHandler(), V4IR::Function::DontInsertBlock);
+    V4IR::BasicBlock *finallyBody = 0;
+    V4IR::BasicBlock *catchBody = 0;
+    V4IR::BasicBlock *catchExceptionHandler = 0;
+    V4IR::BasicBlock *end = _function->newBasicBlock(groupStartBlock(), surroundingExceptionHandler, V4IR::Function::DontInsertBlock);
 
-    // the Catch block for catch is the block itself. We protect against recursion by checking the
-    // hasException boolean
-    // all basic blocks within try and catch get catched by catchBody
-    V4IR::BasicBlock *catchBody =  _function->newBasicBlock(groupStartBlock(), 0, V4IR::Function::DontInsertBlock);
-    catchBody->catchBlock = catchBody;
-    pushExceptionHandler(catchBody);
+    if (ast->finallyExpression)
+        finallyBody = _function->newBasicBlock(groupStartBlock(), surroundingExceptionHandler, V4IR::Function::DontInsertBlock);
 
-    int hasException = _block->newTemp();
-    move(_block->TEMP(hasException), _block->CONST(V4IR::BoolType, false));
+    if (ast->catchExpression) {
+        // exception handler for the catch body
+        catchExceptionHandler = _function->newBasicBlock(groupStartBlock(), 0, V4IR::Function::DontInsertBlock);
+        pushExceptionHandler(catchExceptionHandler);
+        catchBody =  _function->newBasicBlock(groupStartBlock(), catchExceptionHandler, V4IR::Function::DontInsertBlock);
+        popExceptionHandler();
+        pushExceptionHandler(catchBody);
+    } else {
+        Q_ASSERT(finallyBody);
+        pushExceptionHandler(finallyBody);
+    }
 
     V4IR::BasicBlock *tryBody = _function->newBasicBlock(groupStartBlock(), exceptionHandler());
     _block->JUMP(tryBody);
 
-    // ### Remove
-    // Pass the hidden "needRethrow" TEMP to the
-    // builtin_delete_exception_handler, in order to have those TEMPs alive for
-    // the duration of the exception handling block.
-    V4IR::ExprList *finishTryArgs = _function->New<V4IR::ExprList>();
-    finishTryArgs->init(_block->TEMP(hasException));
-
-    ScopeAndFinally tcf(_scopeAndFinally, ast->finallyExpression, finishTryArgs);
+    ScopeAndFinally tcf(_scopeAndFinally, ast->finallyExpression);
     _scopeAndFinally = &tcf;
 
     _block = tryBody;
     statement(ast->statement);
-    _block->JUMP(finallyBody);
+    _block->JUMP(finallyBody ? finallyBody : end);
 
-    _function->insertBasicBlock(catchBody);
-    _block = catchBody;
+    popExceptionHandler();
 
     if (ast->catchExpression) {
-        // check if an exception got thrown within catch. Go to finally
-        // and then rethrow
-        V4IR::BasicBlock *cleanupCatchScope = _function->newBasicBlock(groupStartBlock(), exceptionHandler());
-        V4IR::BasicBlock *b = _function->newBasicBlock(groupStartBlock(), exceptionHandler());
-        _block->CJUMP(_block->TEMP(hasException), cleanupCatchScope, b);
-
-        _block = cleanupCatchScope;
-        _block->EXP(_block->CALL(_block->NAME(V4IR::Name::builtin_pop_scope, 0, 0), 0));
-        _block->JUMP(finallyBody);
+        pushExceptionHandler(catchExceptionHandler);
+        _function->insertBasicBlock(catchBody);
+        _block = catchBody;
 
-        _block = b;
-    }
-
-    move(_block->TEMP(hasException), _block->CONST(V4IR::BoolType, true));
-
-    if (ast->catchExpression) {
         ++_function->insideWithOrCatch;
         V4IR::ExprList *catchArgs = _function->New<V4IR::ExprList>();
         catchArgs->init(_block->STRING(_function->newString(ast->catchExpression->name.toString())));
@@ -2391,28 +2381,37 @@ bool Codegen::visit(TryStatement *ast)
         }
         _block->EXP(_block->CALL(_block->NAME(V4IR::Name::builtin_pop_scope, 0, 0), 0));
         --_function->insideWithOrCatch;
-        move(_block->TEMP(hasException), _block->CONST(V4IR::BoolType, false));
+        _block->JUMP(finallyBody ? finallyBody : end);
+        popExceptionHandler();
+
+        _function->insertBasicBlock(catchExceptionHandler);
+        catchExceptionHandler->EXP(catchExceptionHandler->CALL(catchExceptionHandler->NAME(V4IR::Name::builtin_pop_scope, 0, 0), 0));
+        if (finallyBody || surroundingExceptionHandler)
+            catchExceptionHandler->JUMP(finallyBody ? finallyBody : surroundingExceptionHandler);
+        else
+            catchExceptionHandler->EXP(catchExceptionHandler->CALL(catchExceptionHandler->NAME(V4IR::Name::builtin_rethrow, 0, 0), 0));
     }
-    _block->JUMP(finallyBody);
 
     _scopeAndFinally = tcf.parent;
 
-    // exceptions thrown in finally get catched by the surrounding catch statement
-    popExceptionHandler();
+    if (finallyBody) {
+        _function->insertBasicBlock(finallyBody);
+        _block = finallyBody;
 
-    _function->insertBasicBlock(finallyBody);
+        int hasException = _block->newTemp();
+        move(_block->TEMP(hasException), _block->CALL(_block->NAME(V4IR::Name::builtin_unwind_exception, /*line*/0, /*column*/0), 0));
 
-    _block = finallyBody;
-    if (ast->finallyExpression && ast->finallyExpression->statement)
-        statement(ast->finallyExpression->statement);
+        if (ast->finallyExpression && ast->finallyExpression->statement)
+            statement(ast->finallyExpression->statement);
 
-    V4IR::BasicBlock *rethrowBlock = _function->newBasicBlock(groupStartBlock(), exceptionHandler());
-    V4IR::BasicBlock *after = _function->newBasicBlock(groupStartBlock(), exceptionHandler());
-    _block->CJUMP(_block->TEMP(hasException), rethrowBlock, after);
-    _block = rethrowBlock;
-    _block->EXP(_block->CALL(_block->NAME(V4IR::Name::builtin_rethrow, /*line*/0, /*column*/0), 0));
+        V4IR::ExprList *arg = _function->New<V4IR::ExprList>();
+        arg->expr = _block->TEMP(hasException);
+        _block->EXP(_block->CALL(_block->NAME(V4IR::Name::builtin_throw, /*line*/0, /*column*/0), arg));
+        _block->JUMP(end);
+    }
 
-    _block = after;
+    _function->insertBasicBlock(end);
+    _block = end;
 
     return false;
 }
diff --git a/src/qml/compiler/qv4codegen_p.h b/src/qml/compiler/qv4codegen_p.h
index 652f395a88..b0d1962bca 100644
--- a/src/qml/compiler/qv4codegen_p.h
+++ b/src/qml/compiler/qv4codegen_p.h
@@ -238,12 +238,11 @@ protected:
 
         ScopeAndFinally *parent;
         AST::Finally *finally;
-        V4IR::ExprList *finishTryArgs;
         ScopeType type;
 
-        ScopeAndFinally(ScopeAndFinally *parent, ScopeType t = WithScope) : parent(parent), finally(0), finishTryArgs(0), type(t) {}
-        ScopeAndFinally(ScopeAndFinally *parent, AST::Finally *finally, V4IR::ExprList *finishTryArgs)
-        : parent(parent), finally(finally), finishTryArgs(finishTryArgs), type(TryScope)
+        ScopeAndFinally(ScopeAndFinally *parent, ScopeType t = WithScope) : parent(parent), finally(0), type(t) {}
+        ScopeAndFinally(ScopeAndFinally *parent, AST::Finally *finally)
+        : parent(parent), finally(finally), type(TryScope)
         {}
     };
 
diff --git a/src/qml/compiler/qv4isel_masm.cpp b/src/qml/compiler/qv4isel_masm.cpp
index 6fe0f27ec3..2d13b4d8c8 100644
--- a/src/qml/compiler/qv4isel_masm.cpp
+++ b/src/qml/compiler/qv4isel_masm.cpp
@@ -818,9 +818,8 @@ void InstructionSelection::callBuiltinDeleteValue(V4IR::Temp *result)
 
 void InstructionSelection::callBuiltinThrow(V4IR::Expr *arg)
 {
-    generateFunctionCall(Assembler::Void, __qmljs_throw, Assembler::ContextRegister,
+    generateFunctionCall(Assembler::ReturnValueRegister, __qmljs_throw, Assembler::ContextRegister,
                          Assembler::PointerToValue(arg));
-    _as->jumpToExceptionHandler();
 }
 
 void InstructionSelection::callBuiltinReThrow()
@@ -828,10 +827,16 @@ void InstructionSelection::callBuiltinReThrow()
     _as->jumpToExceptionHandler();
 }
 
+void InstructionSelection::callBuiltinUnwindException(V4IR::Temp *result)
+{
+    generateFunctionCall(result, __qmljs_builtin_unwind_exception, Assembler::ContextRegister);
+
+}
+
 void InstructionSelection::callBuiltinPushCatchScope(const QString &exceptionName)
 {
     Assembler::Pointer s = _as->loadStringAddress(Assembler::ScratchRegister, exceptionName);
-    generateFunctionCall(Assembler::ContextRegister, __qmljs_builtin_push_catch_scope, s, Assembler::ContextRegister);
+    generateFunctionCall(Assembler::ContextRegister, __qmljs_builtin_push_catch_scope, Assembler::ContextRegister, s);
 }
 
 void InstructionSelection::callBuiltinForeachIteratorObject(V4IR::Temp *arg, V4IR::Temp *result)
diff --git a/src/qml/compiler/qv4isel_masm_p.h b/src/qml/compiler/qv4isel_masm_p.h
index 3d1b271cdc..40d1aa5275 100644
--- a/src/qml/compiler/qv4isel_masm_p.h
+++ b/src/qml/compiler/qv4isel_masm_p.h
@@ -95,7 +95,15 @@ template <typename T>
 struct ExceptionCheck {
     enum { NeedsCheck = 1 };
 };
-
+// push_catch and pop context methods shouldn't check for exceptions
+template <>
+struct ExceptionCheck<QV4::ExecutionContext *(*)(QV4::ExecutionContext *)> {
+    enum { NeedsCheck = 0 };
+};
+template <typename A>
+struct ExceptionCheck<QV4::ExecutionContext *(*)(QV4::ExecutionContext *, A)> {
+    enum { NeedsCheck = 0 };
+};
 
 class Assembler : public JSC::MacroAssembler
 {
@@ -1444,6 +1452,7 @@ protected:
     virtual void callBuiltinDeleteValue(V4IR::Temp *result);
     virtual void callBuiltinThrow(V4IR::Expr *arg);
     virtual void callBuiltinReThrow();
+    virtual void callBuiltinUnwindException(V4IR::Temp *);
     virtual void callBuiltinPushCatchScope(const QString &exceptionName);
     virtual void callBuiltinForeachIteratorObject(V4IR::Temp *arg, V4IR::Temp *result);
     virtual void callBuiltinForeachNextPropertyname(V4IR::Temp *arg, V4IR::Temp *result);
diff --git a/src/qml/compiler/qv4isel_moth.cpp b/src/qml/compiler/qv4isel_moth.cpp
index 2e31aa10a0..06fb9caac8 100644
--- a/src/qml/compiler/qv4isel_moth.cpp
+++ b/src/qml/compiler/qv4isel_moth.cpp
@@ -777,6 +777,11 @@ void InstructionSelection::callBuiltinReThrow()
     // ###
 }
 
+void InstructionSelection::callBuiltinUnwindException(V4IR::Temp *)
+{
+    // ###
+}
+
 
 void InstructionSelection::callBuiltinPushCatchScope(const QString &exceptionName)
 {
diff --git a/src/qml/compiler/qv4isel_moth_p.h b/src/qml/compiler/qv4isel_moth_p.h
index 8be28b5605..f5a14e15ae 100644
--- a/src/qml/compiler/qv4isel_moth_p.h
+++ b/src/qml/compiler/qv4isel_moth_p.h
@@ -94,6 +94,7 @@ protected:
     virtual void callBuiltinDeleteValue(V4IR::Temp *result);
     virtual void callBuiltinThrow(V4IR::Expr *arg);
     virtual void callBuiltinReThrow();
+    virtual void callBuiltinUnwindException(V4IR::Temp *);
     virtual void callBuiltinPushCatchScope(const QString &exceptionName);
     virtual void callBuiltinForeachIteratorObject(V4IR::Temp *arg, V4IR::Temp *result);
     virtual void callBuiltinForeachNextPropertyname(V4IR::Temp *arg, V4IR::Temp *result);
diff --git a/src/qml/compiler/qv4isel_p.cpp b/src/qml/compiler/qv4isel_p.cpp
index 7d10230f43..851d5661ff 100644
--- a/src/qml/compiler/qv4isel_p.cpp
+++ b/src/qml/compiler/qv4isel_p.cpp
@@ -270,6 +270,10 @@ void IRDecoder::callBuiltin(V4IR::Call *call, V4IR::Temp *result)
         callBuiltinReThrow();
     } return;
 
+    case V4IR::Name::builtin_unwind_exception: {
+        callBuiltinUnwindException(result);
+    } return;
+
     case V4IR::Name::builtin_push_catch_scope: {
         V4IR::String *s = call->args->expr->asString();
         Q_ASSERT(s);
diff --git a/src/qml/compiler/qv4isel_p.h b/src/qml/compiler/qv4isel_p.h
index 9895e2fd04..e3146add66 100644
--- a/src/qml/compiler/qv4isel_p.h
+++ b/src/qml/compiler/qv4isel_p.h
@@ -120,6 +120,7 @@ public: // to implement by subclasses:
     virtual void callBuiltinDeleteValue(V4IR::Temp *result) = 0;
     virtual void callBuiltinThrow(V4IR::Expr *arg) = 0;
     virtual void callBuiltinReThrow() = 0;
+    virtual void callBuiltinUnwindException(V4IR::Temp *) = 0;
     virtual void callBuiltinPushCatchScope(const QString &exceptionName) = 0;
     virtual void callBuiltinForeachIteratorObject(V4IR::Temp *arg, V4IR::Temp *result) = 0;
     virtual void callBuiltinForeachNextPropertyname(V4IR::Temp *arg, V4IR::Temp *result) = 0;
diff --git a/src/qml/compiler/qv4jsir.cpp b/src/qml/compiler/qv4jsir.cpp
index f0a37895b5..1c9620e1f5 100644
--- a/src/qml/compiler/qv4jsir.cpp
+++ b/src/qml/compiler/qv4jsir.cpp
@@ -397,6 +397,8 @@ static const char *builtin_to_string(Name::Builtin b)
         return "builtin_throw";
     case Name::builtin_rethrow:
         return "builtin_rethrow";
+    case Name::builtin_unwind_exception:
+        return "builtin_unwind_exception";
     case Name::builtin_push_catch_scope:
         return "builtin_push_catch_scope";
     case V4IR::Name::builtin_foreach_iterator_object:
diff --git a/src/qml/compiler/qv4jsir_p.h b/src/qml/compiler/qv4jsir_p.h
index ee8e0c0636..fe8425ab71 100644
--- a/src/qml/compiler/qv4jsir_p.h
+++ b/src/qml/compiler/qv4jsir_p.h
@@ -311,6 +311,7 @@ struct Name: Expr {
         builtin_delete,
         builtin_throw,
         builtin_rethrow,
+        builtin_unwind_exception,
         builtin_push_catch_scope,
         builtin_foreach_iterator_object,
         builtin_foreach_next_property_name,
diff --git a/src/qml/compiler/qv4regalloc.cpp b/src/qml/compiler/qv4regalloc.cpp
index 5d1265b4b6..9c90388c9e 100644
--- a/src/qml/compiler/qv4regalloc.cpp
+++ b/src/qml/compiler/qv4regalloc.cpp
@@ -194,6 +194,7 @@ protected: // IRDecoder
     virtual void callBuiltinDeleteValue(V4IR::Temp *) {}
     virtual void callBuiltinThrow(V4IR::Expr *) {}
     virtual void callBuiltinReThrow() {}
+    virtual void callBuiltinUnwindException(V4IR::Temp *) {}
     virtual void callBuiltinPushCatchScope(const QString &) {};
     virtual void callBuiltinForeachIteratorObject(V4IR::Temp *, V4IR::Temp *) {}
     virtual void callBuiltinForeachNextProperty(V4IR::Temp *, V4IR::Temp *) {}
diff --git a/src/qml/jsruntime/qv4engine.cpp b/src/qml/jsruntime/qv4engine.cpp
index 3449c1aa3c..73db54174c 100644
--- a/src/qml/jsruntime/qv4engine.cpp
+++ b/src/qml/jsruntime/qv4engine.cpp
@@ -834,7 +834,7 @@ ReturnedValue ExecutionEngine::catchException(ExecutionContext *catchingContext,
     exceptionStackTrace.clear();
     hasException = false;
     ReturnedValue res = exceptionValue.asReturnedValue();
-    exceptionValue = Encode::undefined();
+    exceptionValue = Primitive::emptyValue();
     return res;
 }
 
diff --git a/src/qml/jsruntime/qv4runtime.cpp b/src/qml/jsruntime/qv4runtime.cpp
index 9b16f4e45a..1eed693c3d 100644
--- a/src/qml/jsruntime/qv4runtime.cpp
+++ b/src/qml/jsruntime/qv4runtime.cpp
@@ -902,7 +902,8 @@ ReturnedValue __qmljs_construct_property(ExecutionContext *context, const ValueR
 
 void __qmljs_throw(ExecutionContext *context, const ValueRef value)
 {
-    context->throwError(value);
+    if (!value->isEmpty())
+        context->throwError(value);
 }
 
 ReturnedValue __qmljs_builtin_typeof(ExecutionContext *ctx, const ValueRef value)
@@ -967,7 +968,14 @@ ExecutionContext *__qmljs_builtin_push_with_scope(const ValueRef o, ExecutionCon
     return ctx->newWithContext(obj);
 }
 
-ExecutionContext *__qmljs_builtin_push_catch_scope(const StringRef exceptionVarName, ExecutionContext *ctx)
+ReturnedValue __qmljs_builtin_unwind_exception(ExecutionContext *ctx)
+{
+    if (!ctx->engine->hasException)
+        return Primitive::emptyValue().asReturnedValue();
+    return ctx->engine->catchException(ctx, 0);
+}
+
+ExecutionContext *__qmljs_builtin_push_catch_scope(ExecutionContext *ctx, const StringRef exceptionVarName)
 {
     Scope scope(ctx);
     ScopedValue v(scope, ctx->engine->catchException(ctx, 0));
diff --git a/src/qml/jsruntime/qv4runtime_p.h b/src/qml/jsruntime/qv4runtime_p.h
index 50a1ce17d1..f61a9805ac 100644
--- a/src/qml/jsruntime/qv4runtime_p.h
+++ b/src/qml/jsruntime/qv4runtime_p.h
@@ -129,8 +129,9 @@ QV4::ReturnedValue __qmljs_builtin_typeof_element(QV4::ExecutionContext* context
 
 void __qmljs_builtin_rethrow(QV4::ExecutionContext *context);
 QV4::ExecutionContext *__qmljs_builtin_push_with_scope(const QV4::ValueRef o, QV4::ExecutionContext *ctx);
-QV4::ExecutionContext *__qmljs_builtin_push_catch_scope(const QV4::StringRef exceptionVarName, QV4::ExecutionContext *ctx);
+QV4::ExecutionContext *__qmljs_builtin_push_catch_scope(QV4::ExecutionContext *ctx, const QV4::StringRef exceptionVarName);
 QV4::ExecutionContext *__qmljs_builtin_pop_scope(QV4::ExecutionContext *ctx);
+ReturnedValue __qmljs_builtin_unwind_exception(ExecutionContext *ctx);
 void __qmljs_builtin_declare_var(QV4::ExecutionContext *ctx, bool deletable, const QV4::StringRef name);
 void __qmljs_builtin_define_property(QV4::ExecutionContext *ctx, const QV4::ValueRef object, const QV4::StringRef name, QV4::ValueRef val);
 QV4::ReturnedValue __qmljs_builtin_define_array(QV4::ExecutionContext *ctx, QV4::Value *values, uint length);