diff --git a/TODO.md b/TODO.md index 94051c2..4be714c 100644 --- a/TODO.md +++ b/TODO.md @@ -1969,8 +1969,22 @@ in either corpus runs for minutes, which is why the first of these had never bee the argument against it is that it makes `moveto` write to the grid, which it currently does not. -33. **A `TRAP` handler cannot be entered once the value pool is dry, and the error is dropped - in silence.** `akbasic_trap_set_error_variables()` (`src/runtime_trap.c:36`) sets `ER#` +33. ~~**A `TRAP` handler cannot be entered once the value pool is dry, and the error is + dropped in silence.**~~ **Done**, in two halves. `akbasic_runtime_reserve_globals()` + (`src/runtime.c`) creates `ER#` and `EL#` at runtime init and again after `CLR`, so the + dispatch never allocates -- there is nothing left for it to fail at. And + `report_and_reraise()` no longer lets a failure *inside* reporting replace the error the + program actually made: the secondary one is logged where a developer will see it and the + original is re-raised. + + **The reduction below no longer reproduces, and not because of this fix.** ยง6 item 30 + made a scalar free, so the value pool can no longer be emptied by creating names and + `ER#`/`EL#` could be created after all. The defect was still there, reachable through the + *variable* table instead: 124 names and a `TRAP` armed, and the handler was silently + skipped while the program carried on. That is what `tests/trap_verbs.c` now pins, along + with the invariant itself -- both globals present before a program runs. + + The original report: `akbasic_trap_set_error_variables()` (`src/runtime_trap.c:36`) sets `ER#` and `EL#` through `akbasic_runtime_global()` (`:51` and `:53`), which *creates* them if the program never named them. Creating them needs pool slots. So the one error most likely to leave the pool empty -- item 30's -- is the one error whose handler cannot be diff --git a/include/akbasic/runtime.h b/include/akbasic/runtime.h index b4763f8..2b100c5 100644 --- a/include/akbasic/runtime.h +++ b/include/akbasic/runtime.h @@ -752,6 +752,22 @@ akerr_ErrorContext AKERR_NOIGNORE *akbasic_runtime_global(akbasic_Runtime *obj, /* --- Internal API: exposed for the scanner, parser and verb handlers only. --- */ +/** + * @brief Create the globals the runtime writes to, before anything needs them. + * + * `ER#` and `EL#`, which the `TRAP` dispatch sets. Creating a name costs a + * variable slot, and the dispatch runs at exactly the moment the program is + * already in trouble -- so they are made once, up front, where there is always + * room. `akbasic_runtime_init()` calls this; so does `CLR`, which empties the + * table it filled. + * + * @param obj Object to initialize, inspect, or modify. + * @return `NULL` on success, otherwise an error context owned by the caller. + * @throws AKERR_NULLPOINTER When `obj` is NULL. + * @throws AKBASIC_ERR_BOUNDS When no variable slot is free. + */ +akerr_ErrorContext AKERR_NOIGNORE *akbasic_runtime_reserve_globals(akbasic_Runtime *obj); + /** * @brief Take an unused variable from the runtime's pool. * @param obj Object to initialize, inspect, or modify. diff --git a/src/runtime.c b/src/runtime.c index e03ff4e..8badaa9 100644 --- a/src/runtime.c +++ b/src/runtime.c @@ -67,6 +67,32 @@ akerr_ErrorContext *akbasic_runtime_global(akbasic_Runtime *obj, const char *nam SUCCEED_RETURN(errctx); } +akerr_ErrorContext *akbasic_runtime_reserve_globals(akbasic_Runtime *obj) +{ + PREPARE_ERROR(errctx); + akbasic_Variable *variable = NULL; + + FAIL_ZERO_RETURN(errctx, (obj != NULL), AKERR_NULLPOINTER, "NULL runtime in reserve_globals"); + /* + * ER# and EL# exist from the start, because the moment they are needed is + * the moment the interpreter is least able to make them. + * + * akbasic_trap_set_error_variables() reaches them through + * akbasic_runtime_global(), which *creates* a name the program never used -- + * and creating one costs a variable slot. So a program that filled the + * variable table and then raised could not have its handler entered at all, + * and because the failure happened inside the error path rather than raising, + * the program carried on with the failing statement's effect quietly + * missing. A wrong answer delivered as a right one. TODO.md section 6 item + * 33 has the reduction. + * + * Called again from CLR and NEW, which empty the table this fills. + */ + PASS(errctx, akbasic_runtime_global(obj, "ER#", &variable)); + PASS(errctx, akbasic_runtime_global(obj, "EL#", &variable)); + SUCCEED_RETURN(errctx); +} + akerr_ErrorContext *akbasic_runtime_new_function(akbasic_Runtime *obj, akbasic_FunctionDef **dest) { PREPARE_ERROR(errctx); @@ -215,6 +241,7 @@ akerr_ErrorContext *akbasic_runtime_init(akbasic_Runtime *obj, akbasic_TextSink PASS(errctx, akbasic_runtime_new_environment(obj)); PASS(errctx, akbasic_runtime_zero(obj)); PASS(errctx, akbasic_scanner_zero(obj)); + PASS(errctx, akbasic_runtime_reserve_globals(obj)); SUCCEED_RETURN(errctx); } @@ -450,6 +477,7 @@ akerr_ErrorContext *akbasic_runtime_set_mode(akbasic_Runtime *obj, int mode) static akerr_ErrorContext *report_and_reraise(akbasic_Runtime *obj, akerr_ErrorContext *cause) { PREPARE_ERROR(errctx); + akerr_ErrorContext *reporting = NULL; char message[AKERR_MAX_ERROR_CONTEXT_STRING_LENGTH]; int status = cause->status; @@ -458,7 +486,28 @@ static akerr_ErrorContext *report_and_reraise(akbasic_Runtime *obj, akerr_ErrorC obj->lasterrorstatus = status; cause->handled = true; IGNORE(akerr_release_error(cause)); - PASS(errctx, akbasic_runtime_error(obj, AKBASIC_ERRCLASS_RUNTIME, message)); + + /* + * **Reporting can itself fail, and the program's own error still wins.** + * + * The way it fails is the `TRAP` dispatch inside akbasic_runtime_error(): + * entering a handler takes a scope, and a program deep enough to have run + * out of them raises for that reason while already raising for another. A + * plain PASS here would return the *dispatch's* failure and lose the one the + * program needs to hear about -- "Environment pool exhausted" in place of + * the subscript that was actually out of range. + * + * So the secondary failure is logged where a developer will see it and the + * original is re-raised. TODO.md section 6 item 33 is where this was found; + * item 33's other half stops the dispatch needing to allocate at all, which + * is what makes this path rare rather than routine. + */ + reporting = akbasic_runtime_error(obj, AKBASIC_ERRCLASS_RUNTIME, message); + if ( reporting != NULL ) { + LOG_ERROR_WITH_MESSAGE(reporting, "could not report a BASIC error"); + reporting->handled = true; + IGNORE(akerr_release_error(reporting)); + } FAIL_RETURN(errctx, status, "%s", message); } diff --git a/src/runtime_housekeeping.c b/src/runtime_housekeeping.c index 507ef4a..fbb4c6b 100644 --- a/src/runtime_housekeeping.c +++ b/src/runtime_housekeeping.c @@ -72,6 +72,12 @@ static akerr_ErrorContext AKERR_NOIGNORE *clear_variables(akbasic_Runtime *obj) FAIL_ZERO_RETURN(errctx, (root != NULL), AKERR_NULLPOINTER, "Runtime has no root environment"); PASS(errctx, akbasic_symtab_init(&root->variables, AKBASIC_MAX_VARIABLES)); PASS(errctx, akbasic_symtab_init(&root->functions, AKBASIC_MAX_FUNCTIONS)); + /* + * Put the reserved globals back. They were in the table this just emptied, + * and the `TRAP` dispatch relies on never having to create them -- so a CLR + * that left them gone would reopen item 33 for the rest of the session. + */ + PASS(errctx, akbasic_runtime_reserve_globals(obj)); SUCCEED_RETURN(errctx); } diff --git a/tests/trap_verbs.c b/tests/trap_verbs.c index ac9713b..8cd9e61 100644 --- a/tests/trap_verbs.c +++ b/tests/trap_verbs.c @@ -222,6 +222,75 @@ static void test_err_function(void) harness_stop(); } +/** + * @brief `ER#` and `EL#` exist before a program runs, so the dispatch cannot fail. + * + * The dispatch used to reach them through akbasic_runtime_global(), which + * *creates* a name the program never used -- and creating one takes a variable + * slot. So the handler could not be entered by a program that had filled the + * table, which is the one kind of program most likely to raise. Asserted here as + * the invariant rather than only through the symptom below, because the symptom + * needs 124 lines of setup and this needs none. + */ +static void test_error_globals_are_reserved(void) +{ + bool haveer = false; + bool haveel = false; + int i = 0; + + TEST_REQUIRE_OK(harness_start(NULL)); + for ( i = 0; i < AKBASIC_MAX_VARIABLES; i++ ) { + if ( !HARNESS_RUNTIME.variables[i].used ) { + continue; + } + if ( strcmp(HARNESS_RUNTIME.variables[i].name, "ER#") == 0 ) { + haveer = true; + } + if ( strcmp(HARNESS_RUNTIME.variables[i].name, "EL#") == 0 ) { + haveel = true; + } + } + TEST_REQUIRE(haveer, "ER# must exist as soon as the runtime is initialised"); + TEST_REQUIRE(haveel, "EL# must exist as soon as the runtime is initialised"); + harness_stop(); +} + +/** + * @brief A handler is entered even by a program that has filled the variable table. + * + * TODO.md section 6 item 33, and much worse than an abort: the dispatch failed + * *inside* the error path rather than raising, so nothing was reported, the + * handler never ran, and the program carried on with the failing statement's + * effect quietly missing -- a wrong answer delivered as a right one. + * + * 124 names, which leaves the table with no room to invent `ER#` and `EL#` at + * the moment they are wanted. It is 124 rather than 128 because the runtime + * reserves those two and the loop counter is a third. + */ +static void test_handler_runs_with_a_full_variable_table(void) +{ + char source[AKBASIC_MAX_VARIABLES * 32]; + size_t used = 0; + int i = 0; + + used = (size_t)snprintf(source, sizeof(source), "10 TRAP CAUGHT\n"); + for ( i = 0; i < 124; i++ ) { + used += (size_t)snprintf(source + used, sizeof(source) - used, + "%d V%03d# = %d\n", 20 + i, i, i); + } + snprintf(source + used, sizeof(source) - used, + "800 DIM TOOBIG#(5000)\n" + "810 PRINT \"NOT TRAPPED\"\n" + "820 END\n" + "900 LABEL CAUGHT\n" + "910 PRINT \"TRAPPED\"\n" + "920 END\n"); + + TEST_REQUIRE_OK(run_program(source)); + TEST_REQUIRE_STR(HARNESS_OUTPUT, "TRAPPED\n"); + harness_stop(); +} + int main(void) { test_trap_catches(); @@ -233,5 +302,7 @@ int main(void) test_resume_outside_handler(); test_error_inside_handler(); test_err_function(); + test_error_globals_are_reserved(); + test_handler_runs_with_a_full_variable_table(); return akbasic_test_failures; }