From eda6cc9c7fac907c23b54cc878f1d3700015f839 Mon Sep 17 00:00:00 2001 From: Nicolas Hake Date: Sun, 13 Nov 2016 10:57:18 +0100 Subject: [PATCH] Aul: Gracefully handle errors in codegen (#1840) By continuing to generate bytecode even after an error is found, we're able to find more syntax errors and will also be able to keep the value stack at the expected height. --- src/script/C4AulCompiler.cpp | 293 ++++++++++++++++++++++------------- tests/aul/AulTest.cpp | 44 ++++++ 2 files changed, 231 insertions(+), 106 deletions(-) diff --git a/src/script/C4AulCompiler.cpp b/src/script/C4AulCompiler.cpp index 6ad33fe2c..a38567177 100644 --- a/src/script/C4AulCompiler.cpp +++ b/src/script/C4AulCompiler.cpp @@ -151,6 +151,7 @@ public: virtual void visit(const ::aul::ast::ParExpr *n) override; virtual void visit(const ::aul::ast::AppendtoPragma *n) override; virtual void visit(const ::aul::ast::IncludePragma *n) override; + virtual void visit(const ::aul::ast::Script *n) override; }; class C4AulCompiler::CodegenAstVisitor : public ::aul::DefaultRecursiveVisitor @@ -212,6 +213,49 @@ class C4AulCompiler::CodegenAstVisitor : public ::aul::DefaultRecursiveVisitor void RemoveLastBCC(); C4AulBCC MakeSetter(const char *SPos, bool fLeaveValue); + void HandleError(const C4AulError &e) + { + AddBCC(nullptr, AB_ERR, (intptr_t)::Strings.RegString(e.what())); + target_host->Engine->ErrorHandler->OnError(e.what()); + } + + template + bool SafeVisit(const T &node) + { + // Swallows exceptions during evaluation of node. Use if you want to + // keep doing syntax checks for subsequent children. (Generated code + // will cause a runtime error if executed.) + try + { + node->accept(this); + return true; + } + catch (C4AulParseError &e) + { + HandleError(e); + return false; + } + } + + class StackGuard + { + // Ensures that the Aul value stack ends up at the expected height + CodegenAstVisitor *parent; + const int32_t target_stack_height; + public: + explicit StackGuard(CodegenAstVisitor *parent, int32_t offset = 0) : parent(parent), target_stack_height(parent->stack_height + offset) + {} + ~StackGuard() + { + assert(parent->stack_height == target_stack_height); + if (parent->stack_height != target_stack_height) + { + parent->HandleError(Error(parent->target_host, parent->host, nullptr, parent->Fn, "internal error: value stack left unbalanced")); + parent->AddBCC(nullptr, AB_STACK, target_stack_height - parent->stack_height); + } + } + }; + public: CodegenAstVisitor(C4ScriptHost *host, C4ScriptHost *source_host) : target_host(host), host(source_host) {} explicit CodegenAstVisitor(C4AulScriptFunc *func) : Fn(func), target_host(func->pOrgScript), host(target_host) {} @@ -502,7 +546,7 @@ void C4AulCompiler::PreparseAstVisitor::visit(const ::aul::ast::FunctionDecl *n) try { - DefaultRecursiveVisitor::visit(n); + DefaultRecursiveVisitor::visit(n); Fn = nullptr; } catch (...) @@ -544,6 +588,21 @@ void C4AulCompiler::PreparseAstVisitor::visit(const ::aul::ast::IncludePragma *n host->Includes.emplace_back(n->what.c_str()); } +void C4AulCompiler::PreparseAstVisitor::visit(const::aul::ast::Script * n) +{ + for (const auto &d : n->declarations) + { + try + { + d->accept(this); + } + catch (C4AulParseError &e) + { + target_host->Engine->GetErrorHandler()->OnError(e.what()); + } + } +} + int C4AulCompiler::CodegenAstVisitor::GetStackValue(C4AulBCCType eType, intptr_t X) { switch (eType) @@ -894,27 +953,31 @@ void C4AulCompiler::CodegenAstVisitor::visit(const ::aul::ast::Noop *) {} void C4AulCompiler::CodegenAstVisitor::visit(const ::aul::ast::StringLit *n) { + StackGuard g(this, 1); AddBCC(n->loc, AB_STRING, (intptr_t)::Strings.RegString(n->value.c_str())); type_of_stack_top = C4V_String; } void C4AulCompiler::CodegenAstVisitor::visit(const ::aul::ast::IntLit *n) { + StackGuard g(this, 1); AddBCC(n->loc, AB_INT, n->value); type_of_stack_top = C4V_Int; } void C4AulCompiler::CodegenAstVisitor::visit(const ::aul::ast::BoolLit *n) { + StackGuard g(this, 1); AddBCC(n->loc, AB_BOOL, n->value); type_of_stack_top = C4V_Bool; } void C4AulCompiler::CodegenAstVisitor::visit(const ::aul::ast::ArrayLit *n) { + StackGuard g(this, 1); for (const auto &e : n->values) { - e->accept(this); + SafeVisit(e); } AddBCC(n->loc, AB_NEW_ARRAY, n->values.size()); type_of_stack_top = C4V_Array; @@ -922,10 +985,12 @@ void C4AulCompiler::CodegenAstVisitor::visit(const ::aul::ast::ArrayLit *n) void C4AulCompiler::CodegenAstVisitor::visit(const ::aul::ast::ProplistLit *n) { + StackGuard g(this, 1); for (const auto &e : n->values) { + StackGuard g(this, 2); AddBCC(n->loc, AB_STRING, (intptr_t)::Strings.RegString(e.first.c_str())); - e.second->accept(this); + SafeVisit(e.second); } AddBCC(n->loc, AB_NEW_PROPLIST, n->values.size()); type_of_stack_top = C4V_PropList; @@ -933,18 +998,21 @@ void C4AulCompiler::CodegenAstVisitor::visit(const ::aul::ast::ProplistLit *n) void C4AulCompiler::CodegenAstVisitor::visit(const ::aul::ast::NilLit *n) { + StackGuard g(this, 1); AddBCC(n->loc, AB_NIL); type_of_stack_top = C4V_Nil; } void C4AulCompiler::CodegenAstVisitor::visit(const ::aul::ast::ThisLit *n) { + StackGuard g(this, 1); AddBCC(n->loc, AB_THIS); type_of_stack_top = C4V_PropList; } void C4AulCompiler::CodegenAstVisitor::visit(const ::aul::ast::VarExpr *n) { + StackGuard g(this, 1); assert(Fn); C4Value dummy; const char *cname = n->identifier.c_str(); @@ -1004,18 +1072,22 @@ void C4AulCompiler::CodegenAstVisitor::visit(const ::aul::ast::VarExpr *n) case C4V_Function: AddBCC(n->loc, AB_CFUNCTION, reinterpret_cast(v._getFunction())); default: + AddBCC(n->loc, AB_NIL); throw Error(target_host, host, n, Fn, "internal error: global constant of unexpected type: %s (of type %s)", cname, v.GetTypeName()); } type_of_stack_top = v.GetType(); } else { + AddBCC(n->loc, AB_NIL); throw Error(target_host, host, n, Fn, "symbol not found in any symbol table: %s", cname); } } void C4AulCompiler::CodegenAstVisitor::visit(const ::aul::ast::UnOpExpr *n) { + StackGuard g(this, 1); + n->operand->accept(this); const auto &op = C4ScriptOpMap[n->op]; if (op.Changer) @@ -1038,7 +1110,9 @@ void C4AulCompiler::CodegenAstVisitor::visit(const ::aul::ast::UnOpExpr *n) void C4AulCompiler::CodegenAstVisitor::visit(const ::aul::ast::BinOpExpr *n) { - n->lhs->accept(this); + StackGuard g(this, 1); + + SafeVisit(n->lhs); const auto &op = C4ScriptOpMap[n->op]; if (op.Code == AB_JUMPAND || op.Code == AB_JUMPOR || op.Code == AB_JUMPNNIL) @@ -1047,19 +1121,26 @@ void C4AulCompiler::CodegenAstVisitor::visit(const ::aul::ast::BinOpExpr *n) // because we don't want to evaluate their rhs operand when the // lhs one already decided the result int jump = AddBCC(n->loc, op.Code); - n->rhs->accept(this); + SafeVisit(n->rhs); UpdateJump(jump, AddJumpTarget()); } else if (op.Changer) { - C4AulBCC setter = MakeSetter(n->loc, true); - n->rhs->accept(this); - AddBCC(n->loc, op.Code); - AddBCC(n->loc, setter); + try + { + C4AulBCC setter = MakeSetter(n->loc, true); + SafeVisit(n->rhs); + AddBCC(n->loc, op.Code); + AddBCC(n->loc, setter); + } + catch (C4AulParseError &e) + { + HandleError(e); + } } else { - n->rhs->accept(this); + SafeVisit(n->rhs); AddBCC(n->loc, op.Code, 0); } @@ -1068,18 +1149,26 @@ void C4AulCompiler::CodegenAstVisitor::visit(const ::aul::ast::BinOpExpr *n) void C4AulCompiler::CodegenAstVisitor::visit(const ::aul::ast::AssignmentExpr *n) { - n->lhs->accept(this); - C4AulBCC setter = MakeSetter(n->loc, false); - n->rhs->accept(this); - AddBCC(n->loc, setter); - + StackGuard g(this, 1); + SafeVisit(n->lhs); + try + { + C4AulBCC setter = MakeSetter(n->loc, false); + SafeVisit(n->rhs); + AddBCC(n->loc, setter); + } + catch (C4AulParseError &e) + { + HandleError(e); + } // Assignment does not change the type of the variable } void C4AulCompiler::CodegenAstVisitor::visit(const ::aul::ast::SubscriptExpr *n) { - n->object->accept(this); - n->index->accept(this); + StackGuard g(this, 1); + SafeVisit(n->object); + SafeVisit(n->index); AddBCC(n->loc, AB_ARRAYA); // FIXME: Check if the subscripted object is a literal and if so, retrieve type @@ -1088,9 +1177,10 @@ void C4AulCompiler::CodegenAstVisitor::visit(const ::aul::ast::SubscriptExpr *n) void C4AulCompiler::CodegenAstVisitor::visit(const ::aul::ast::SliceExpr *n) { - n->object->accept(this); - n->start->accept(this); - n->end->accept(this); + StackGuard g(this, 1); + SafeVisit(n->object); + SafeVisit(n->start); + SafeVisit(n->end); AddBCC(n->loc, AB_ARRAY_SLICE); type_of_stack_top = C4V_Array; @@ -1114,17 +1204,31 @@ void C4AulCompiler::CodegenAstVisitor::visit(const ::aul::ast::CallExpr *n) return; } + if (n->callee == C4AUL_Inherited || n->callee == C4AUL_SafeInherited) + { + // inherited can only be called within the same context + if (n->context) + { + throw Error(target_host, host, n, Fn, "\"%s\" can't be called in a different context", cname); + } + } + + if (n->callee == C4AUL_Inherited && !Fn->OwnerOverloaded) + { + throw Error(target_host, host, n, Fn, "inherited function not found (use " C4AUL_SafeInherited " to disable this message)"); + } + const auto pre_call_stack = stack_height; if (n->context) - n->context->accept(this); + SafeVisit(n->context); std::vector known_par_types; known_par_types.reserve(n->args.size()); for (const auto &arg : n->args) { - arg->accept(this); + SafeVisit(arg); known_par_types.push_back(type_of_stack_top); } @@ -1133,28 +1237,7 @@ void C4AulCompiler::CodegenAstVisitor::visit(const ::aul::ast::CallExpr *n) // Special handling for the overload chain if (n->callee == C4AUL_Inherited || n->callee == C4AUL_SafeInherited) { - if (n->context) - { - throw Error(target_host, host, n, Fn, "\"%s\" can't be called in a different context", cname); - } callee = Fn->OwnerOverloaded; - if (!callee) - { - if (n->callee == C4AUL_SafeInherited) - { - // pop all args off the stack - if (!n->args.empty()) - AddBCC(n->loc, AB_STACK, -(intptr_t)n->args.size()); - // and "return" nil - AddBCC(n->loc, AB_NIL); - type_of_stack_top = C4V_Nil; - return; - } - else - { - throw Error(target_host, host, n, Fn, "inherited function not found (use " C4AUL_SafeInherited " to disable this message)"); - } - } } size_t fn_argc = C4AUL_MAX_Par; @@ -1167,9 +1250,24 @@ void C4AulCompiler::CodegenAstVisitor::visit(const ::aul::ast::CallExpr *n) callee = target_host->Engine->GetFunc(cname); if (callee) + { fn_argc = callee->GetParCount(); + } else - throw Error(target_host, host, n, Fn, "called function not found: %s", cname); + { + // pop all args off the stack + if (!n->args.empty()) + AddBCC(n->loc, AB_STACK, -(intptr_t)n->args.size()); + // and "return" nil + AddBCC(n->loc, AB_NIL); + type_of_stack_top = C4V_Nil; + + if (n->callee != C4AUL_SafeInherited) + { + HandleError(Error(target_host, host, n, Fn, "called function not found: %s", cname)); + } + return; + } } if (n->args.size() > fn_argc) @@ -1266,7 +1364,9 @@ void C4AulCompiler::CodegenAstVisitor::visit(const ::aul::ast::CallExpr *n) void C4AulCompiler::CodegenAstVisitor::visit(const ::aul::ast::ParExpr *n) { - n->arg->accept(this); + StackGuard g(this, 1); + + SafeVisit(n->arg); AddBCC(n->loc, AB_PAR); type_of_stack_top = C4V_Any; } @@ -1275,17 +1375,20 @@ void C4AulCompiler::CodegenAstVisitor::visit(const ::aul::ast::Block *n) { for (const auto &s : n->children) { - const auto pre_statement_stack = stack_height; - s->accept(this); - // If the statement has left a stack value, pop it off - MaybePopValueOf(s); - assert(pre_statement_stack == stack_height); + StackGuard g(this, 0); + if (SafeVisit(s)) + { + // If the statement has left a stack value, pop it off + MaybePopValueOf(s); + } } } void C4AulCompiler::CodegenAstVisitor::visit(const ::aul::ast::Return *n) { - n->value->accept(this); + StackGuard g(this, 0); + + SafeVisit(n->value); AddBCC(n->loc, AB_RETURN); } @@ -1356,28 +1459,29 @@ void C4AulCompiler::CodegenAstVisitor::visit(const ::aul::ast::ForLoop *n) if (n->init) { - n->init->accept(this); - MaybePopValueOf(n->init); + if (SafeVisit(n->init)) + MaybePopValueOf(n->init); } PushLoop(); int cond = AddJumpTarget(); if (n->cond) { - n->cond->accept(this); + SafeVisit(n->cond); active_loops.top().breaks.push_back(AddBCC(n->cond->loc, AB_CONDN)); } + int incr = cond; if (n->incr) { int cond_jump = AddBCC(n->loc, AB_JUMP); incr = AddJumpTarget(); - n->incr->accept(this); - MaybePopValueOf(n->incr); + if (SafeVisit(n->incr)) + MaybePopValueOf(n->incr); AddJumpTo(n->loc, AB_JUMP, cond); UpdateJump(cond_jump, AddJumpTarget()); } - n->body->accept(this); - MaybePopValueOf(n->body); + if (SafeVisit(n->body)) + MaybePopValueOf(n->body); AddJumpTo(n->loc, AB_JUMP, incr); PopLoop(incr); #endif @@ -1403,16 +1507,17 @@ void C4AulCompiler::CodegenAstVisitor::visit(const ::aul::ast::RangeLoop *n) if (var_id == -1) throw Error(target_host, host, n, Fn, "internal error: unable to find variable in foreach: %s", cname); // Emit code for array - n->cond->accept(this); + SafeVisit(n->cond); // Emit code for iteration AddBCC(n->loc, AB_INT, 0); int cond = AddJumpTarget(); PushLoop(); AddVarAccess(n->loc, AB_FOREACH_NEXT, var_id); AddLoopControl(n->loc, Loop::Control::Break); // Will be skipped by AB_FOREACH_NEXT as long as more entries exist - // Emit body - n->body->accept(this); - MaybePopValueOf(n->body); + + // Emit body + if (SafeVisit(n->body)) + MaybePopValueOf(n->body); // continue starts the next iteration of the loop AddLoopControl(n->loc, Loop::Control::Continue); PopLoop(cond); @@ -1460,41 +1565,25 @@ void C4AulCompiler::CodegenAstVisitor::visit(const ::aul::ast::DoLoop *n) { int body = AddJumpTarget(); PushLoop(); - try - { - n->body->accept(this); + if (SafeVisit(n->body)) MaybePopValueOf(n->body); - int cond = AddJumpTarget(); - n->cond->accept(this); - AddJumpTo(n->loc, AB_COND, body); - PopLoop(cond); - } - catch (...) - { - PopLoop(AddJumpTarget()); - throw; - } + int cond = AddJumpTarget(); + SafeVisit(n->cond); + AddJumpTo(n->loc, AB_COND, body); + PopLoop(cond); } void C4AulCompiler::CodegenAstVisitor::visit(const ::aul::ast::WhileLoop *n) { int cond = AddJumpTarget(); PushLoop(); - try - { - n->cond->accept(this); - active_loops.top().breaks.push_back(AddBCC(n->cond->loc, AB_CONDN)); - n->body->accept(this); + SafeVisit(n->cond); + active_loops.top().breaks.push_back(AddBCC(n->cond->loc, AB_CONDN)); + if (SafeVisit(n->body)) MaybePopValueOf(n->body); - // continue starts the next iteration of the loop - AddLoopControl(n->loc, Loop::Control::Continue); - PopLoop(cond); - } - catch (...) - { - PopLoop(AddJumpTarget()); - throw; - } + // continue starts the next iteration of the loop + AddLoopControl(n->loc, Loop::Control::Continue); + PopLoop(cond); } void C4AulCompiler::CodegenAstVisitor::visit(const ::aul::ast::Break *n) @@ -1511,17 +1600,17 @@ void C4AulCompiler::CodegenAstVisitor::visit(const ::aul::ast::Continue *n) void C4AulCompiler::CodegenAstVisitor::visit(const ::aul::ast::If *n) { - n->cond->accept(this); + SafeVisit(n->cond); int jump = AddBCC(n->loc, AB_CONDN); - n->iftrue->accept(this); - MaybePopValueOf(n->iftrue); + if (SafeVisit(n->iftrue)) + MaybePopValueOf(n->iftrue); if (n->iffalse) { int jumpout = AddBCC(n->loc, AB_JUMP); UpdateJump(jump, AddJumpTarget()); jump = jumpout; - n->iffalse->accept(this); - MaybePopValueOf(n->iffalse); + if (SafeVisit(n->iffalse)) + MaybePopValueOf(n->iffalse); } UpdateJump(jump, AddJumpTarget()); } @@ -1537,7 +1626,7 @@ void C4AulCompiler::CodegenAstVisitor::visit(const ::aul::ast::VarDecl *n) if (dec.init) { // Emit code for the initializer - dec.init->accept(this); + SafeVisit(dec.init); int var_idx = Fn->VarNamed.GetItemNr(cname); assert(var_idx >= 0 && "CodegenAstVisitor: var not found in variable table"); if (var_idx < 0) @@ -1598,7 +1687,7 @@ void C4AulCompiler::CodegenAstVisitor::visit(const ::aul::ast::FunctionDecl *n) try { - EmitFunctionCode(n); + EmitFunctionCode(n); Fn = nullptr; } catch (...) @@ -1610,6 +1699,7 @@ void C4AulCompiler::CodegenAstVisitor::visit(const ::aul::ast::FunctionDecl *n) void C4AulCompiler::CodegenAstVisitor::visit(const::aul::ast::FunctionExpr * n) { + AddBCC(n->loc, AB_NIL); throw Error(target_host, host, n, Fn, "can't define a function in a function-scoped proplist"); } @@ -1617,16 +1707,7 @@ void C4AulCompiler::CodegenAstVisitor::visit(const::aul::ast::Script * n) { for (const auto &d : n->declarations) { - try - { - d->accept(this); - } - catch (C4AulParseError &e) - { - // Inform handler - host->Engine->ErrorHandler->OnError(e.what()); - // try next declaration - } + SafeVisit(d); } } diff --git a/tests/aul/AulTest.cpp b/tests/aul/AulTest.cpp index 12cb50051..9e5a024dd 100644 --- a/tests/aul/AulTest.cpp +++ b/tests/aul/AulTest.cpp @@ -136,6 +136,50 @@ for (var i in a) { } return b; )")); + + // Test syntax errors inside loops + { + // Syntax error in for loop initializer + ErrorHandler errh; + EXPECT_CALL(errh, OnError(::testing::_)); + EXPECT_THROW(RunCode("for (var i = missing();;) break;"), C4AulExecError); + } + { + // Syntax error in for loop condition + ErrorHandler errh; + EXPECT_CALL(errh, OnError(::testing::_)); + EXPECT_THROW(RunCode("for (; missing();) break;"), C4AulExecError); + } + { + // Syntax error in for loop incrementor + ErrorHandler errh; + EXPECT_CALL(errh, OnError(::testing::_)); + EXPECT_THROW(RunCode("for (;; missing()) continue;"), C4AulExecError); + } + { + // Syntax error in for loop body + ErrorHandler errh; + EXPECT_CALL(errh, OnError(::testing::_)); + EXPECT_THROW(RunCode("for (;;) missing();"), C4AulExecError); + } + { + // Syntax error in while loop condition + ErrorHandler errh; + EXPECT_CALL(errh, OnError(::testing::_)); + EXPECT_THROW(RunCode("while (missing()) break;"), C4AulExecError); + } + { + // Syntax error in while loop body + ErrorHandler errh; + EXPECT_CALL(errh, OnError(::testing::_)); + EXPECT_THROW(RunCode("while (1) missing();"), C4AulExecError); + } + { + // Syntax error in for-in loop body + ErrorHandler errh; + EXPECT_CALL(errh, OnError(::testing::_)); + EXPECT_THROW(RunCode("for (var i in [1]) { missing(); }"), C4AulExecError); + } } TEST_F(AulTest, Locals)