From 3c6377cd8ba1b557886745fbc2e3fac52561b62c Mon Sep 17 00:00:00 2001 From: Nicolas Williams Date: Sat, 25 May 2019 20:02:28 -0500 Subject: [PATCH] WIP: Fix try/catch catches more than it should #1859 Rename the FORK_OPT opcode to TRY_BEGIN, add a TRY_END opcode, and wrap errors when raising through a TRY_END so that they will not be caught by the matching TRY_BEGIN. --- src/compile.c | 46 +++++++++++++++++++++++------------ src/execute.c | 62 +++++++++++++++++++++++++++++++++++++++++++++-- src/opcode_list.h | 5 ++-- src/parser.c | 4 +-- src/parser.y | 4 +-- 5 files changed, 98 insertions(+), 23 deletions(-) diff --git a/src/compile.c b/src/compile.c index 30a2b4ae0b..20c82fe7b8 100644 --- a/src/compile.c +++ b/src/compile.c @@ -1074,26 +1074,34 @@ block gen_try_handler(block handler) { block gen_try(block exp, block handler) { /* - * Produce something like: - * FORK_OPT
+ * Produce: + * TRY_BEGIN
* + * TRY_END + */ + exp = BLOCK(exp, inst_block(inst_new(TRY_END))); + exp = BLOCK(gen_op_target(TRY_BEGIN, exp), exp); + + /* + * then + * * JUMP * * - * If this is not an internal try/catch, then catch and re-raise - * internal errors to prevent them from leaking. + * if there's a handler. * - * The handler will only execute if we backtrack to the FORK_OPT with - * an error (exception). If produces no value then FORK_OPT - * will backtrack (propagate the `empty`, as it were. If - * produces a value then we'll execute whatever bytecode follows this - * sequence. + * The handler will only execute if we backtrack to the TRY_BEGIN with + * an error (exception) and the exception occurred in the original try + * block expression and not from any opcodes past the TRY_END. See + * jq_next(). + * + * If produces no value then TRY_BEGIN will backtrack, propagating + * the `empty`. If produces a value then we'll execute whatever + * bytecode follows this sequence. */ - if (!handler.first && !handler.last) - // A hack to deal with `.` as the handler; we could use a real NOOP here - handler = BLOCK(gen_op_simple(DUP), gen_op_simple(POP), handler); - exp = BLOCK(exp, gen_op_target(JUMP, handler)); - return BLOCK(gen_op_target(FORK_OPT, exp), exp, handler); + if (block_is_noop(handler)) + return exp; + return BLOCK(exp, gen_op_target(JUMP, handler), handler); } block gen_label(const char *label, block exp) { @@ -1265,6 +1273,7 @@ static int compile(struct bytecode* bc, block b, struct locfile* lf, jv args, jv int errors = 0; int pos = 0; int var_frame_idx = 0; + int try_block_id = 0; bc->nsubfunctions = 0; errors += expand_call_arglist(&b, args, env); b = BLOCK(b, gen_op_simple(RET)); @@ -1288,7 +1297,10 @@ static int compile(struct bytecode* bc, block b, struct locfile* lf, jv args, jv curr->imm.intval = var_frame_idx++; localnames = jv_array_append(localnames, jv_string(curr->symbol)); } - + if (curr->op == TRY_BEGIN) { + assert(curr->imm.target && curr->imm.target->op == TRY_END); + curr->imm.target->imm.intval = curr->imm.intval = ++try_block_id; + } if (curr->op == CLOSURE_CREATE) { assert(curr->bound_by == curr); curr->imm.intval = bc->nsubfunctions++; @@ -1384,6 +1396,10 @@ static int compile(struct bytecode* bc, block b, struct locfile* lf, jv args, jv assert(curr->imm.target->bytecode_pos > pos); // only forward branches code[pos] = curr->imm.target->bytecode_pos - (pos + 1); pos++; + if (curr->op == TRY_BEGIN) + code[pos++] = curr->imm.intval; + } else if (curr->op == TRY_END) { + code[pos++] = curr->imm.intval; } else if (op->length > 1) { assert(0 && "codegen not implemented for this operation"); } diff --git a/src/execute.c b/src/execute.c index c23cb4e92b..f2017603bc 100644 --- a/src/execute.c +++ b/src/execute.c @@ -441,6 +441,7 @@ jv jq_next(jq_state *jq) { int raising; int backtracking = !jq->initial_execution; + jq->initial_execution = 0; assert(jv_get_kind(jq->error) == JV_KIND_NULL); assert(jq->parent == 0 || jq->start_limit >= jq->stk.limit); @@ -910,7 +911,65 @@ jv jq_next(jq_state *jq) { break; } - case FORK_OPT: + case TRY_BEGIN: + stack_save(jq, pc - 1, stack_get_pos(jq)); + pc++; // skip handler offset this time + break; + + case TRY_END: + stack_save(jq, pc - 1, stack_get_pos(jq)); + break; + + case ON_BACKTRACK(TRY_BEGIN): { + if (!raising) { + // `try EXP ...` backtracked here (no value, `empty`), so we backtrack more + jv_free(stack_pop(jq)); + goto do_backtrack; + } + + /* + * We're looking for errors of the form {__jq:}, and if we + * find one then, if it's of the form {__jq:{...}} then we'll unwrap and + * re-raise, else just re-raise it. + * + * On backtrack through TRY_END we'll wrap error values as {__jq:} so + * that we can hit this handling here and not catch those errors here. + */ + jv e = jv_invalid_get_msg(jv_copy(jq->error)); + jv ev = jv_invalid(); + if (jv_get_kind(e) == JV_KIND_OBJECT && jv_is_valid((ev = jv_get(jv_copy(e), jv_string("__jq"))))) { + if (jv_get_kind(ev) == JV_KIND_OBJECT) + set_error(jq, jv_invalid_with_msg(JV_OBJECT(jv_string("__jq"), jv_copy(ev)))); + else + set_error(jq, jv_invalid_with_msg(jv_copy(e))); + jv_free(ev); + jv_free(e); + goto do_backtrack; + } + jv_free(ev); + jv_free(e); + + /* + * The error isn't for the form {__jq:...}, so we must catch it in this + * TRY_BEGIN. + * + * Jump to the try/catch handler. + */ + uint16_t offset = *pc++; + jv_free(stack_pop(jq)); // free the input + stack_push(jq, jv_invalid_get_msg(jq->error)); // push the error's message + jq->error = jv_null(); + pc += offset; + break; + } + case ON_BACKTRACK(TRY_END): + // Wrap the error so the matching TRY_BEGIN doesn't catch it + if (raising) + set_error(jq, + jv_invalid_with_msg(JV_OBJECT(jv_string("__jq"), + jv_invalid_get_msg(jv_copy(jq->error))))); + goto do_backtrack; + case DESTRUCTURE_ALT: case FORK: { stack_save(jq, pc - 1, stack_get_pos(jq)); @@ -918,7 +977,6 @@ jv jq_next(jq_state *jq) { break; } - case ON_BACKTRACK(FORK_OPT): case ON_BACKTRACK(DESTRUCTURE_ALT): { if (jv_is_valid(jq->error)) { // `try EXP ...` backtracked here (no value, `empty`), so we backtrack more diff --git a/src/opcode_list.h b/src/opcode_list.h index 9e30565104..2bf5ae6705 100644 --- a/src/opcode_list.h +++ b/src/opcode_list.h @@ -11,9 +11,10 @@ OP(STORE_GLOBAL, GLOBAL, 0, 0) OP(INDEX, NONE, 2, 1) OP(INDEX_OPT, NONE, 2, 1) OP(EACH, NONE, 1, 1) -OP(EACH_OPT, NONE, 1, 1) +OP(EACH_OPT, NONE, 1, 1) OP(FORK, BRANCH, 0, 0) -OP(FORK_OPT, BRANCH, 0, 0) +OP(TRY_BEGIN, BRANCH, 0, 0) +OP(TRY_END, NONE, 0, 0) OP(JUMP, BRANCH, 0, 0) OP(JUMP_F,BRANCH, 1, 0) OP(BACKTRACK, NONE, 0, 0) diff --git a/src/parser.c b/src/parser.c index 4a63e336b7..add0093a3d 100644 --- a/src/parser.c +++ b/src/parser.c @@ -2576,7 +2576,7 @@ YYLTYPE yylloc = yyloc_default; case 19: #line 396 "src/parser.y" /* yacc.c:1646 */ { - //$$ = BLOCK(gen_op_target(FORK_OPT, $2), $2, $4); + //$$ = BLOCK(gen_op_target(TRY_BEGIN, $2), $2, $4); (yyval.blk) = gen_try((yyvsp[-2].blk), gen_try_handler((yyvsp[0].blk))); } #line 2583 "src/parser.c" /* yacc.c:1646 */ @@ -2585,7 +2585,7 @@ YYLTYPE yylloc = yyloc_default; case 20: #line 400 "src/parser.y" /* yacc.c:1646 */ { - //$$ = BLOCK(gen_op_target(FORK_OPT, $2), $2, gen_op_simple(BACKTRACK)); + //$$ = BLOCK(gen_op_target(TRY_BEGIN, $2), $2, gen_op_simple(BACKTRACK)); (yyval.blk) = gen_try((yyvsp[0].blk), gen_op_simple(BACKTRACK)); } #line 2592 "src/parser.c" /* yacc.c:1646 */ diff --git a/src/parser.y b/src/parser.y index 601f7b11af..6af323fc0c 100644 --- a/src/parser.y +++ b/src/parser.y @@ -394,11 +394,11 @@ Term "as" Patterns '|' Exp { } | "try" Exp "catch" Exp { - //$$ = BLOCK(gen_op_target(FORK_OPT, $2), $2, $4); + //$$ = BLOCK(gen_op_target(TRY_BEGIN, $2), $2, $4); $$ = gen_try($2, gen_try_handler($4)); } | "try" Exp { - //$$ = BLOCK(gen_op_target(FORK_OPT, $2), $2, gen_op_simple(BACKTRACK)); + //$$ = BLOCK(gen_op_target(TRY_BEGIN, $2), $2, gen_op_simple(BACKTRACK)); $$ = gen_try($2, gen_op_simple(BACKTRACK)); } | "try" Exp "catch" error {