Fix scoping for catch blocks
authorSimon Hausmann <simon.hausmann@digia.com>
Fri, 8 Feb 2013 12:33:50 +0000 (13:33 +0100)
committerLars Knoll <lars.knoll@digia.com>
Wed, 13 Feb 2013 09:00:20 +0000 (10:00 +0100)
Based on Lars' idea, done together with Erik.

For proper scoping of the exception variable in catch blocks, it is necessary
to define the new scope (ExecutionContext for us) at run-time, it cannot be
determined at compile time.

This patch implements the necessary logic to create the new execution context
when entering a try block and re-uses the existing pop_scope infrastructure for
destroying it again. Within the catch scope it is necessary to do all lookups by
name, so the existing _function->insideWith variable was re-used and renamed
to _function->insideWithOrCatch. Additionally the new context also stores the
name and value of the separately scoped exception variable that shadows any
existing equally named variables in outter scopes.

CodeGen::unwindException also had a bug that it would generate the finally code
with the wrong _function->insideWithOrCatch level, resulting in name lookups
inside finally instead of local index lookups.

Change-Id: I5616af38c3558e553e971a6a894ce5239ccb8422
Reviewed-by: Lars Knoll <lars.knoll@digia.com>
16 files changed:
src/v4/moth/qv4instr_moth_p.h
src/v4/moth/qv4isel_moth.cpp
src/v4/moth/qv4isel_moth_p.h
src/v4/moth/qv4vme_moth.cpp
src/v4/qmljs_environment.cpp
src/v4/qmljs_environment.h
src/v4/qmljs_runtime.cpp
src/v4/qmljs_runtime.h
src/v4/qv4codegen.cpp
src/v4/qv4ir.cpp
src/v4/qv4ir_p.h
src/v4/qv4isel_masm.cpp
src/v4/qv4isel_masm_p.h
src/v4/qv4isel_p.cpp
src/v4/qv4isel_p.h
tests/TestExpectations

index 741ddba..d573713 100644 (file)
@@ -25,6 +25,7 @@
     F(CallBuiltinDeleteExceptionHandler, callBuiltinDeleteExceptionHandler) \
     F(CallBuiltinGetException, callBuiltinGetException) \
     F(CallBuiltinPushScope, callBuiltinPushScope) \
+    F(CallBuiltinPushCatchScope, callBuiltinPushCatchScope) \
     F(CallBuiltinPopScope, callBuiltinPopScope) \
     F(CallBuiltinForeachIteratorObject, callBuiltinForeachIteratorObject) \
     F(CallBuiltinForeachNextPropertyName, callBuiltinForeachNextPropertyName) \
@@ -242,6 +243,10 @@ union Instr
         MOTH_INSTR_HEADER
         Param arg;
     };
+    struct instr_callBuiltinPushCatchScope {
+        MOTH_INSTR_HEADER
+        VM::String *varName;
+    };
     struct instr_callBuiltinPopScope {
         MOTH_INSTR_HEADER
     };
@@ -452,6 +457,7 @@ union Instr
     instr_callBuiltinDeleteExceptionHandler callBuiltinDeleteExceptionHandler;
     instr_callBuiltinGetException callBuiltinGetException;
     instr_callBuiltinPushScope callBuiltinPushScope;
+    instr_callBuiltinPushCatchScope callBuiltinPushCatchScope;
     instr_callBuiltinPopScope callBuiltinPopScope;
     instr_callBuiltinForeachIteratorObject callBuiltinForeachIteratorObject;
     instr_callBuiltinForeachNextPropertyName callBuiltinForeachNextPropertyName;
index 2f0a614..87b21aa 100644 (file)
@@ -873,6 +873,13 @@ void InstructionSelection::callBuiltinPushWithScope(IR::Temp *arg)
     addInstruction(call);
 }
 
+void InstructionSelection::callBuiltinPushCatchScope(const QString &exceptionVarName)
+{
+    Instruction::CallBuiltinPushCatchScope call;
+    call.varName = identifier(exceptionVarName);
+    addInstruction(call);
+}
+
 void InstructionSelection::callBuiltinPopScope()
 {
     Instruction::CallBuiltinPopScope call;
index 7d57897..012a26b 100644 (file)
@@ -49,6 +49,7 @@ protected:
     virtual void callBuiltinForeachIteratorObject(IR::Temp *arg, IR::Temp *result);
     virtual void callBuiltinForeachNextPropertyname(IR::Temp *arg, IR::Temp *result);
     virtual void callBuiltinPushWithScope(IR::Temp *arg);
+    virtual void callBuiltinPushCatchScope(const QString &exceptionVarName);
     virtual void callBuiltinPopScope();
     virtual void callBuiltinDeclareVar(bool deletable, const QString &name);
     virtual void callBuiltinDefineGetterSetter(IR::Temp *object, const QString &name, IR::Temp *getter, IR::Temp *setter);
index 8ed3fb1..fc06e90 100644 (file)
@@ -283,6 +283,10 @@ VM::Value VME::operator()(QQmlJS::VM::ExecutionContext *context, const uchar *co
         context = __qmljs_builtin_push_with_scope(VALUE(instr.arg), context);
     MOTH_END_INSTR(CallBuiltinPushScope)
 
+    MOTH_BEGIN_INSTR(CallBuiltinPushCatchScope)
+         context = __qmljs_builtin_push_catch_scope(instr.varName, context);
+    MOTH_END_INSTR(CallBuiltinPushCatchScope)
+
     MOTH_BEGIN_INSTR(CallBuiltinPopScope)
         context = __qmljs_builtin_pop_scope(context);
     MOTH_END_INSTR(CallBuiltinPopScope)
index b1339b6..40b8938 100644 (file)
@@ -178,10 +178,18 @@ ExecutionContext *ExecutionContext::createWithScope(Object *with)
     return withCtx;
 }
 
+ExecutionContext *ExecutionContext::createCatchScope(String *exceptionVarName)
+{
+    ExecutionContext *catchCtx = engine->newContext();
+    catchCtx->initForCatch(this, exceptionVarName);
+    engine->current = catchCtx;
+    return catchCtx;
+}
+
 ExecutionContext *ExecutionContext::popScope()
 {
     assert(engine->current == this);
-    assert(withObject != 0);
+    assert(withObject != 0 || exceptionVarName != 0);
 
     engine->current = parent;
     parent = 0;
@@ -222,6 +230,8 @@ void ExecutionContext::init(ExecutionEngine *eng)
     arguments = 0;
     argumentCount = 0;
     locals = 0;
+    exceptionVarName = 0;
+    exceptionValue = Value::undefinedValue();
     strictMode = false;
     activation = 0;
     withObject = 0;
@@ -242,11 +252,31 @@ void ExecutionContext::init(ExecutionContext *p, Object *with)
     arguments = 0;
     argumentCount = 0;
     locals = 0;
+    exceptionVarName = 0;
+    exceptionValue = Value::undefinedValue();
     strictMode = false;
     activation = 0;
     withObject = with;
 }
 
+void ExecutionContext::initForCatch(ExecutionContext *p, String *exceptionVarName)
+{
+    engine = p->engine;
+    parent = p;
+    outer = p;
+    thisObject = p->thisObject;
+
+    function = 0;
+    arguments = 0;
+    argumentCount = 0;
+    locals = 0;
+    this->exceptionVarName = exceptionVarName;
+    exceptionValue = engine->exception;
+    strictMode = p->strictMode;
+    activation = 0;
+    withObject = 0;
+}
+
 void ExecutionContext::destroy()
 {
     delete[] arguments;
@@ -265,6 +295,8 @@ bool ExecutionContext::deleteProperty(String *name)
             if (ctx->activation && ctx->activation->__hasProperty__(this, name))
                 return ctx->activation->__delete__(this, name);
         }
+        if (ctx->exceptionVarName && ctx->exceptionVarName->isEqualTo(name))
+            return false;
         if (FunctionObject *f = ctx->function) {
             if (f->needsActivation || hasWith) {
                 for (unsigned int i = 0; i < f->varCount; ++i)
@@ -294,6 +326,9 @@ void ExecutionContext::mark()
         activation->mark();
     if (withObject)
         withObject->mark();
+    if (exceptionVarName)
+        exceptionVarName->mark();
+    exceptionValue.mark();
 }
 
 void ExecutionContext::setProperty(String *name, Value value)
@@ -307,6 +342,9 @@ void ExecutionContext::setProperty(String *name, Value value)
                 w->__put__(ctx, name, value);
                 return;
             }
+        } else if (ctx->exceptionVarName && ctx->exceptionVarName->isEqualTo(name)) {
+            ctx->exceptionValue = value;
+            return;
         } else {
 //            qDebug() << ctx << "setting mutable binding";
             if (ctx->setMutableBinding(this, name, value))
@@ -326,6 +364,7 @@ Value ExecutionContext::getProperty(String *name)
         return thisObject;
 
     bool hasWith = false;
+    bool hasCatchScope = false;
 //    qDebug() << "=== getProperty" << name->toQString();
     for (ExecutionContext *ctx = this; ctx; ctx = ctx->outer) {
         if (Object *w = ctx->withObject) {
@@ -340,8 +379,14 @@ Value ExecutionContext::getProperty(String *name)
             continue;
         }
 
+        if (ctx->exceptionVarName) {
+            hasCatchScope = true;
+            if (ctx->exceptionVarName->isEqualTo(name))
+                return ctx->exceptionValue;
+        }
+
         if (FunctionObject *f = ctx->function) {
-            if (f->needsActivation || hasWith) {
+            if (f->needsActivation || hasWith || hasCatchScope) {
                 for (unsigned int i = 0; i < f->varCount; ++i)
                     if (f->varList[i]->isEqualTo(name))
                         return ctx->locals[i];
@@ -369,6 +414,7 @@ Value ExecutionContext::getPropertyNoThrow(String *name)
         return thisObject;
 
     bool hasWith = false;
+    bool hasCatchScope = false;
     for (ExecutionContext *ctx = this; ctx; ctx = ctx->outer) {
         if (Object *w = ctx->withObject) {
             hasWith = true;
@@ -379,8 +425,14 @@ Value ExecutionContext::getPropertyNoThrow(String *name)
             continue;
         }
 
+        if (ctx->exceptionVarName) {
+            hasCatchScope = true;
+            if (ctx->exceptionVarName->isEqualTo(name))
+                return ctx->exceptionValue;
+        }
+
         if (FunctionObject *f = ctx->function) {
-            if (f->needsActivation || hasWith) {
+            if (f->needsActivation || hasWith || hasCatchScope) {
                 for (unsigned int i = 0; i < f->varCount; ++i)
                     if (f->varList[i]->isEqualTo(name))
                         return ctx->locals[i];
@@ -408,6 +460,7 @@ Value ExecutionContext::getPropertyAndBase(String *name, Object **base)
         return thisObject;
 
     bool hasWith = false;
+    bool hasCatchScope = false;
     for (ExecutionContext *ctx = this; ctx; ctx = ctx->outer) {
         if (Object *w = ctx->withObject) {
             hasWith = true;
@@ -420,8 +473,14 @@ Value ExecutionContext::getPropertyAndBase(String *name, Object **base)
             continue;
         }
 
+        if (ctx->exceptionVarName) {
+            hasCatchScope = true;
+            if (ctx->exceptionVarName->isEqualTo(name))
+                return ctx->exceptionValue;
+        }
+
         if (FunctionObject *f = ctx->function) {
-            if (f->needsActivation || hasWith) {
+            if (f->needsActivation || hasWith || hasCatchScope) {
                 for (unsigned int i = 0; i < f->varCount; ++i)
                     if (f->varList[i]->isEqualTo(name))
                         return ctx->locals[i];
@@ -503,6 +562,9 @@ void ExecutionContext::initCallContext(ExecutionContext *parent)
     outer = function->scope;
     engine->current = this;
 
+    exceptionVarName = 0;
+    exceptionValue = Value::undefinedValue();
+
     activation = 0;
     withObject = 0;
 
index 3c4e1fb..f118c63 100644 (file)
@@ -88,6 +88,9 @@ struct ExecutionContext
     unsigned int argumentCount;
     Value *locals;
 
+    String *exceptionVarName;
+    Value exceptionValue;
+
     String * const *formals() const;
     unsigned int formalCount() const;
     String * const *variables() const;
@@ -100,6 +103,7 @@ struct ExecutionContext
 
     void init(ExecutionEngine *e);
     void init(ExecutionContext *p, Object *with);
+    void initForCatch(ExecutionContext *p, String *exceptionVarName);
     void destroy();
 
     bool hasBinding(String *name) const;
@@ -109,6 +113,7 @@ struct ExecutionContext
     bool deleteBinding(ExecutionContext *ctx, String *name);
 
     ExecutionContext *createWithScope(Object *with);
+    ExecutionContext *createCatchScope(String* exceptionVarName);
     ExecutionContext *popScope();
 
     void initCallContext(ExecutionContext *parent);
index 51f6b40..4c77715 100644 (file)
@@ -1211,6 +1211,11 @@ ExecutionContext *__qmljs_builtin_push_with_scope(Value o, ExecutionContext *ctx
     return ctx->createWithScope(obj);
 }
 
+ExecutionContext *__qmljs_builtin_push_catch_scope(String *exceptionVarName, ExecutionContext *ctx)
+{
+    return ctx->createCatchScope(exceptionVarName);
+}
+
 ExecutionContext *__qmljs_builtin_pop_scope(ExecutionContext *ctx)
 {
     return ctx->popScope();
index b2efc15..579e832 100644 (file)
@@ -119,6 +119,7 @@ Value __qmljs_builtin_post_decrement_element(Value base, Value index, ExecutionC
 void __qmljs_builtin_throw(Value val, ExecutionContext *context);
 void __qmljs_builtin_rethrow(ExecutionContext *context);
 ExecutionContext *__qmljs_builtin_push_with_scope(Value o, ExecutionContext *ctx);
+ExecutionContext *__qmljs_builtin_push_catch_scope(String *exceptionVarName, ExecutionContext *ctx);
 ExecutionContext *__qmljs_builtin_pop_scope(ExecutionContext *ctx);
 void __qmljs_builtin_declare_var(ExecutionContext *ctx, bool deletable, String *name);
 void __qmljs_builtin_define_property(Value object, String *name, Value val, ExecutionContext *ctx);
index eda6b0c..17cf824 100644 (file)
@@ -849,7 +849,7 @@ void Codegen::variableDeclaration(VariableDeclaration *ast)
     assert(expr.code);
     initializer = *expr;
 
-    if (! _env->parent || _function->insideWith) {
+    if (! _env->parent || _function->insideWithOrCatch) {
         // it's global code.
         move(_block->NAME(ast->name.toString(), ast->identifierToken.startLine, ast->identifierToken.startColumn), initializer);
     } else {
@@ -1374,7 +1374,7 @@ IR::Expr *Codegen::identifier(const QString &name, int line, int col)
 {
     int index = _env->findMember(name);
 
-    if (! _function->hasDirectEval && !_function->insideWith && _env->parent) {
+    if (! _function->hasDirectEval && !_function->insideWithOrCatch && _env->parent) {
         if (index != -1) {
             return _block->TEMP(index);
         }
@@ -2444,23 +2444,20 @@ bool Codegen::visit(TryStatement *ast)
         move(_block->TEMP(inCatch), _block->CONST(IR::BoolType, true));
         move(_block->TEMP(hasException), _block->CONST(IR::BoolType, false));
 
-        const int exception = _block->newTemp();
-        move(_block->TEMP(exception), _block->CALL(_block->NAME(IR::Name::builtin_get_exception, 0, 0), 0));
+        IR::ExprList *catchScopeArgs = _function->New<IR::ExprList>();
+        catchScopeArgs->init(_block->NAME(ast->catchExpression->name.toString(), ast->catchExpression->identifierToken.startLine, ast->catchExpression->identifierToken.startColumn));
+        _block->EXP(_block->CALL(_block->NAME(IR::Name::builtin_push_catch_scope, 0, 0), catchScopeArgs));
 
-        // the variable used in the catch statement is local and hides any global
-        // variable with the same name.
-        const Environment::Member undefinedMember = { Environment::UndefinedMember, -1 , 0 };
-        const Environment::Member catchMember = { Environment::VariableDefinition, exception, 0 };
-        Environment::Member m = _env->members.value(ast->catchExpression->name.toString(), undefinedMember);
-        _env->members.insert(ast->catchExpression->name.toString(), catchMember);
-
-        statement(ast->catchExpression->statement);
+        ++_function->insideWithOrCatch;
+        {
+            ScopeAndFinally scope(_scopeAndFinally);
+            _scopeAndFinally = &scope;
+            statement(ast->catchExpression->statement);
+            _scopeAndFinally = scope.parent;
+        }
+        --_function->insideWithOrCatch;
 
-        // reset the variable name to the one from the outer scope
-        if (m.type == Environment::UndefinedMember)
-            _env->members.remove(ast->catchExpression->name.toString());
-        else
-            _env->members.insert(ast->catchExpression->name.toString(), m);
+        _block->EXP(_block->CALL(_block->NAME(IR::Name::builtin_pop_scope, 0, 0)));
         _block->JUMP(finallyBody);
     }
 
@@ -2489,12 +2486,14 @@ bool Codegen::visit(TryStatement *ast)
 
 void Codegen::unwindException(Codegen::ScopeAndFinally *outest)
 {
+    int savedDepthForWidthOrCatch = _function->insideWithOrCatch;
     ScopeAndFinally *scopeAndFinally = _scopeAndFinally;
     qSwap(_scopeAndFinally, scopeAndFinally);
     while (_scopeAndFinally != outest) {
         if (_scopeAndFinally->popScope) {
             _block->EXP(_block->CALL(_block->NAME(IR::Name::builtin_pop_scope, 0, 0)));
             _scopeAndFinally = _scopeAndFinally->parent;
+            --_function->insideWithOrCatch;
         } else {
             _block->EXP(_block->CALL(_block->NAME(IR::Name::builtin_delete_exception_handler, 0, 0), _scopeAndFinally->deleteExceptionArgs));
             ScopeAndFinally *tc = _scopeAndFinally;
@@ -2504,6 +2503,7 @@ void Codegen::unwindException(Codegen::ScopeAndFinally *outest)
         }
     }
     qSwap(_scopeAndFinally, scopeAndFinally);
+    _function->insideWithOrCatch = savedDepthForWidthOrCatch;
 }
 
 bool Codegen::visit(VariableStatement *ast)
@@ -2546,14 +2546,14 @@ bool Codegen::visit(WithStatement *ast)
     args->init(_block->TEMP(withObject));
     _block->EXP(_block->CALL(_block->NAME(IR::Name::builtin_push_with_scope, 0, 0), args));
 
-    ++_function->insideWith;
+    ++_function->insideWithOrCatch;
     {
         ScopeAndFinally scope(_scopeAndFinally);
         _scopeAndFinally = &scope;
         statement(ast->statement);
         _scopeAndFinally = scope.parent;
     }
-    --_function->insideWith;
+    --_function->insideWithOrCatch;
     _block->EXP(_block->CALL(_block->NAME(IR::Name::builtin_pop_scope, 0, 0), 0));
 
     IR::BasicBlock *next = _function->newBasicBlock();
index 1c2a9d9..fca14df 100644 (file)
@@ -242,6 +242,8 @@ static const char *builtin_to_string(Name::Builtin b)
         return "builtin_foreach_next_property_name";
     case IR::Name::builtin_push_with_scope:
         return "builtin_push_with_scope";
+    case IR::Name::builtin_push_catch_scope:
+        return "builtin_push_catch_scope";
     case IR::Name::builtin_pop_scope:
         return "builtin_pop_scope";
     case IR::Name::builtin_declare_vars:
index eefc039..e39ef94 100644 (file)
@@ -284,6 +284,7 @@ struct Name: Expr {
         builtin_foreach_iterator_object,
         builtin_foreach_next_property_name,
         builtin_push_with_scope,
+        builtin_push_catch_scope,
         builtin_pop_scope,
         builtin_declare_vars,
         builtin_define_property,
@@ -612,7 +613,7 @@ struct Function {
     QList<const QString *> locals;
     QVector<Function *> nestedFunctions;
 
-    int insideWith;
+    int insideWithOrCatch;
 
     uint hasDirectEval: 1;
     uint usesArgumentsObject : 1;
@@ -626,7 +627,7 @@ struct Function {
         , pool(&module->pool)
         , tempCount(0)
         , maxNumberOfArguments(0)
-        , insideWith(0)
+        , insideWithOrCatch(0)
         , hasDirectEval(false)
         , usesArgumentsObject(false)
         , isStrict(false)
index 5098a44..355d8a5 100644 (file)
@@ -600,6 +600,11 @@ void InstructionSelection::callBuiltinPushWithScope(IR::Temp *arg)
     generateFunctionCall(Assembler::ContextRegister, __qmljs_builtin_push_with_scope, arg, Assembler::ContextRegister);
 }
 
+void InstructionSelection::callBuiltinPushCatchScope(const QString &exceptionVarName)
+{
+    generateFunctionCall(Assembler::ContextRegister, __qmljs_builtin_push_catch_scope, identifier(exceptionVarName), Assembler::ContextRegister);
+}
+
 void InstructionSelection::callBuiltinPopScope()
 {
     generateFunctionCall(Assembler::ContextRegister, __qmljs_builtin_pop_scope, Assembler::ContextRegister);
index 3662268..9e2c4e6 100644 (file)
@@ -711,6 +711,7 @@ protected:
     virtual void callBuiltinForeachIteratorObject(IR::Temp *arg, IR::Temp *result);
     virtual void callBuiltinForeachNextPropertyname(IR::Temp *arg, IR::Temp *result);
     virtual void callBuiltinPushWithScope(IR::Temp *arg);
+    virtual void callBuiltinPushCatchScope(const QString &exceptionVarName);
     virtual void callBuiltinPopScope();
     virtual void callBuiltinDeclareVar(bool deletable, const QString &name);
     virtual void callBuiltinDefineGetterSetter(IR::Temp *object, const QString &name, IR::Temp *getter, IR::Temp *setter);
index 6a050b3..e47f349 100644 (file)
@@ -339,6 +339,12 @@ void InstructionSelection::callBuiltin(IR::Call *call, IR::Temp *result)
         callBuiltinPushWithScope(arg);
     } return;
 
+    case IR::Name::builtin_push_catch_scope: {
+        IR::Name *arg = call->args->expr->asName();
+        assert(arg != 0);
+        callBuiltinPushCatchScope(*arg->id);
+    } return;
+
     case IR::Name::builtin_pop_scope:
         callBuiltinPopScope();
         return;
index 5ec50b3..5a0e828 100644 (file)
@@ -109,6 +109,7 @@ public: // to implement by subclasses:
     virtual void callBuiltinForeachIteratorObject(IR::Temp *arg, IR::Temp *result) = 0;
     virtual void callBuiltinForeachNextPropertyname(IR::Temp *arg, IR::Temp *result) = 0;
     virtual void callBuiltinPushWithScope(IR::Temp *arg) = 0;
+    virtual void callBuiltinPushCatchScope(const QString &exceptionVarName) = 0;
     virtual void callBuiltinPopScope() = 0;
     virtual void callBuiltinDeclareVar(bool deletable, const QString &name) = 0;
     virtual void callBuiltinDefineGetterSetter(IR::Temp *object, const QString &name, IR::Temp *getter, IR::Temp *setter) = 0;
index 22cb397..214d5f4 100644 (file)
@@ -42,9 +42,7 @@ Sbp_A4_T1 failing
 Sbp_A4_T2 failing
 S12.4_A1 failing
 S15.2.4.4_A14 failing
-# Try/catch scoping issue
-S12.14_A4 failing
 
 # Function declaration inside if / while
 Sbp_12.5_A9_T3 failing
-Sbp_12.6.2_A13_T3 failing
\ No newline at end of file
+Sbp_12.6.2_A13_T3 failing