From 0df0a956002ea89422165a00e1d42044bb26549d Mon Sep 17 00:00:00 2001 From: Andrew Kesterson Date: Fri, 31 Jul 2026 09:53:41 -0400 Subject: [PATCH] Consume libakgl 0.2.0 and libakstdlib 0.2.0 Rebasing the defect filing onto libakgl's origin/main brought two upstream commits with it -- "Consume libakstdlib 0.2.0" and "Bump libakgl to 0.2.0" -- and libakgl's sources now use the 0.2.0 akstdlib API. Our CMakeLists declares akstdlib::akstdlib from our own tree first, so libakgl was compiling 0.2.0-era code against 0.1.0 headers and failing on aksl_fwrite, aksl_fread and aksl_realpath. The two have to move together. The code change is one call site. aksl_fwrite now takes a required size_t *nmemb_out and reports a short transfer as AKERR_IO rather than a silent success, so DSAVE onto a full disk is an error a program sees where before it reported nothing. The count is passed and discarded on purpose: the library does the noticing now. The documentation change is larger, because libakstdlib 0.2.0 fixed all six of the confirmed defects TODO.md section 1.9 was built around. That section was a table of bans; leaving it would send an agent around a workaround for functions that now work. The aksl_ato* family raises AKERR_VALUE on no digits or trailing junk and ERANGE on overflow -- exactly the contract that section demanded -- aksl_list_append no longer truncates, aksl_list_iterate no longer skips the first half, AKERR_ITERATOR_BREAK stops a tree traversal, and aksl_realpath no longer reads uninitialised memory. One caveat survives: aksl_strhash_djb2 still sign-extends char, which section 1.3 already covers and the symbol tables still cannot reach. src/convert.c has therefore outlived its reason, and is deliberately left in place. Its own note said to delete it when libakstdlib grew the contract, and that condition is now met -- but doing it touches four call sites, changes the raised status from AKBASIC_ERR_VALUE to AKERR_VALUE where section 1.8 says message text is part of the acceptance contract, and would silently gut the CI mutation job, which is bounded to src/convert.c and src/symtab.c. Section 1.9 now lists all of that. Worth doing on purpose rather than as a side effect of a version bump. libakgl defect #14 is closed by its own 1066ac7, which bumped it to 0.2.0 while this was being written. The requirement is no longer pinned by submodule commit in the README, because AKGL_VERSION_AT_LEAST(0, 2, 0) finally means something. 70/70 core, 71/71 with libakgl, 70/70 under ASan+UBSan, clean under -Wall -Wextra, doxygen clean. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 6 +-- TODO.md | 103 ++++++++++++++++++----------------------- deps/libakgl | 2 +- deps/libakstdlib | 2 +- src/runtime_commands.c | 10 +++- 5 files changed, 59 insertions(+), 64 deletions(-) diff --git a/README.md b/README.md index 0876c3b..ea0855e 100644 --- a/README.md +++ b/README.md @@ -433,10 +433,10 @@ The exception is `FILTER`, which sets the SID's filter cutoff, band switches and # Dependencies * [libakerror](https://source.starfort.tech/andrew/libakerror) 1.0.0 — TRY/CATCH-style error contexts. Every function that can fail returns one. -* [libakstdlib](https://source.starfort.tech/andrew/libakstdlib) 0.1.0 — libc wrappers that report through `libakerror`. -* [libakgl](https://source.starfort.tech/andrew/libakgl) 0.1.0 at commit `42b60f7` or later — **optional**, only for `-DAKBASIC_WITH_AKGL=ON`. Pulls in SDL3. The requirement is pinned by submodule commit rather than by version on purpose: 42b60f7 added 22 public symbols and left the version and the `libakgl.so.0.1` soname alone, so `AKGL_VERSION_AT_LEAST(0, 1, 0)` cannot tell the two trees apart. Filed upstream; `TODO.md` section 8 has the detail. +* [libakstdlib](https://source.starfort.tech/andrew/libakstdlib) 0.2.0 — libc wrappers that report through `libakerror`. +* [libakgl](https://source.starfort.tech/andrew/libakgl) 0.2.0 — **optional**, only for `-DAKBASIC_WITH_AKGL=ON`. Pulls in SDL3, and requires `libakstdlib` 0.2, which is why the two move together. * [basicinterpret](https://source.starfort.tech/andrew/basicinterpret) — the Go original, vendored as the behavioural spec and the acceptance corpus. Not linked, not built. Everything is a submodule; `git submodule update --init --recursive` gets all of it. There is nothing to install first. -Note that `libakstdlib`'s `aksl_atoi`/`atol`/`atoll`/`atof` family is deliberately **not** used here, because it cannot report a conversion failure — `atoi("not a number")` returns success with `0`. `src/convert.c` wraps `strtoll`/`strtod` with the strict contract instead, and `TODO.md` section 1.9 records which of that library's calls are cleared for use. That file will be deleted when `libakstdlib` grows the wrappers its own `TODO.md` section 3.1 already calls for. +`src/convert.c` wraps `strtoll`/`strtod` with a strict contract — no digits consumed, trailing junk and overflow are all errors — because `libakstdlib`'s `aksl_atoi` family could not report a conversion failure at all. As of `libakstdlib` 0.2.0 it can, so that file has outlived its reason and is due for removal; `TODO.md` section 1.9 lists what deleting it touches, including the CI mutation job that currently targets it. diff --git a/TODO.md b/TODO.md index c8a0f46..99d358d 100644 --- a/TODO.md +++ b/TODO.md @@ -264,54 +264,47 @@ interpreter. Where a Go message reads awkwardly, keep it awkward and note it her Numeric formatting must match too: integers via `%" PRId64 "`, floats via `%f` (Go's `%f` and C's `%f` both give six decimals — `tests/language/arithmetic/float.txt` confirms). -### 1.9 Which `libakstdlib` calls are cleared for use, and which are not +### 1.9 Which `libakstdlib` calls are cleared for use — **the bans are lifted** -`deps/libakstdlib` is at **0.1.0** and its `TODO.md` §2.1 lists six confirmed defects. Four -carry a known-failing test, so a green `ctest` over there does not mean what you would -assume. Three of the six sit directly in the path of this port. Check this table before -reaching for anything in `akstdlib.h`: +`deps/libakstdlib` is at **0.2.0**, and that release *fixed all six* of the confirmed defects +this section was built around, plus seventeen contract gaps. The table of bans that used to +live here is gone because it is no longer true, and leaving it would send an agent around a +workaround for a function that now works. -| Call | Verdict | Why | -|---|---|---| -| `aksl_fopen` / `_fread` / `_fwrite` / `_fclose` | **use** | `DLOAD`/`DSAVE` go through these, never bare libc. `aksl_fopen` does not NULL-check `pathname`/`mode` (§2.2.2) — validate before calling. | -| `aksl_malloc` / `_free` | **do not use** | We allocate nothing. Pools only. | -| `aksl_memset` / `_memcpy` | **use** | No open defects. | -| `aksl_printf` / `_fprintf` / `_sprintf` | **use, under protest** | The stdio text sink needs them. All three call `va_start` with no matching `va_end` on any path (§2.1.4) — undefined behaviour, not ours to fix here, but do not add a fourth wrapper in the same shape. | -| `aksl_atoi` / `_atol` / `_atoll` / `_atof` | **BANNED — see below** | Cannot report a conversion failure at all (§2.1.5). | -| `aksl_realpath` | **do not use** | Cannot express a buffer size, never NULL-checks `resolved_path`, and its own error path reads uninitialised memory (§2.1.6). Nothing in the port needs it. | -| `aksl_strhash_djb2` | **use** | See §1.3 for the high-bit caveat, which does not apply to identifiers. | -| `aksl_list_*` | **do not use** | `aksl_list_append` silently truncates to a two-node chain and `aksl_list_iterate` skips the first half of the list — both confirmed (§2.1.1, §2.1.2). The symbol tables in §1.3 are open-addressed and need none of this. | -| `aksl_tree_iterate` | **do not use** | `AKERR_ITERATOR_BREAK` does not stop the traversal (§2.1.3). Nothing in the port needs a tree. | +What changed, in the four that mattered to this port: -**The `aksl_ato*` ban is load-bearing, so here is the whole argument.** `atoi("not a number")` -returns *success* with `0`, and `atoi("99999999999999999999")` returns *success* with a -wrapped value. The Go reference checks the conversion error at every one of these sites and -turns it into a BASIC error: - -| Go site | What it does on bad input | +| Was banned | Now | |---|---| -| `basicgrammar.go:224` `newLiteralInt` → `strconv.ParseInt` | returns `err`, which the parser reports | -| `basicgrammar.go` `newLiteralFloat` → `strconv.ParseFloat` | returns `err` | -| `basicscanner.go` `matchNumber` → `strconv.Atoi` | `PARSE` error, `"INTEGER CONVERSION ON '%s'"` | -| `basicruntime_functions.go:742` `FunctionVAL` | converts a string at runtime and must be able to fail | +| `aksl_atoi` / `_atol` / `_atoll` / `_atof` | Every one takes a `dest` pointer and raises `AKERR_VALUE` on no digits or trailing junk, `ERANGE` on overflow — exactly the contract this section demanded | +| `aksl_list_append` / `_iterate` | Append no longer truncates to two nodes; iterate no longer skips the first half | +| `aksl_tree_iterate` | `AKERR_ITERATOR_BREAK` stops the traversal | +| `aksl_realpath` | Rewritten; no longer reads uninitialised memory on its error path | -Port those onto `aksl_ato*` and every one of them silently succeeds with `0`. That is not a -cosmetic loss: it converts four diagnosable errors into wrong answers, and `VAL("garbage")` -starts returning `0.000000` instead of raising. The existing golden files would not catch it — -`tests/language/functions/val.txt` only covers `"32"`, `"123.456"` and `"-256"`, all valid. +Two signature changes reach this repository. `aksl_fread` and `aksl_fwrite` now take a +required `size_t *nmemb_out` and report a short transfer as `AKERR_IO` instead of a silent +success — so a `DSAVE` onto a full disk is now an error a program sees, where before it +reported nothing. `src/runtime_commands.c` passes the count and discards it, which is correct: +the library does the noticing now. -**Do this instead:** write one local pair of converters in `src/convert.c` -(`akbasic_str_to_int64`, `akbasic_str_to_double`) over `strtoll`/`strtod`, with `errno = 0` -before the call, an `endptr` check for both "no digits consumed" and "trailing junk", and a -range check — raising `AKBASIC_ERR_VALUE` and `ERANGE` respectively. That is the contract -`libakstdlib` §2.1.5 says the `aksl_ato*` family *should* have. When it grows it, delete -`src/convert.c` and switch the call sites over; until then, do not paper over the gap by -calling the broken function and hoping. +**`src/convert.c` is the outstanding consequence, and it is deliberately not done yet.** It +exists only because `aksl_ato*` could not report a conversion failure, and its own note says +what to do when that changed: *"When it grows it, delete `src/convert.c` and switch the call +sites over."* That condition is now met. Doing it touches more than the two functions: -*Acceptance for `src/convert.c`:* `tests/convert.c` — valid decimal, valid hex, empty string, -pure garbage, trailing junk, and an overflowing literal, each asserting the raised status. -Add a `.bas`/`.txt` pair for `VAL` on a non-numeric string in the same commit; there is no -golden coverage for it today. +- Four call sites — `newLiteralInt`/`newLiteralFloat` in `src/grammar.c`, `matchNumber` in + `src/scanner.c`, and `FunctionVAL` in `src/runtime_functions.c`. +- `tests/convert.c`, which would move to asserting `libakstdlib`'s contract rather than ours. +- **`.gitea/workflows/ci.yaml`'s mutation job, which is bounded to `src/convert.c` and + `src/symtab.c`.** Deleting the file without repointing that job silently removes half of the + only gate that checks the error-handling control flow at all. +- The error *status* changes from `AKBASIC_ERR_VALUE` to `AKERR_VALUE`, which is visible in + the message text a program sees — so §1.8 applies and the golden files have to be checked. + +Worth doing, and worth doing on purpose rather than as a side effect of a version bump. + +One caveat survives the upgrade unchanged: `aksl_strhash_djb2` still sign-extends `char`, so a +high-bit byte hashes differently from the `unsigned char` answer. BASIC identifiers are 7-bit +ASCII so the symbol tables cannot reach it — see §1.3, which is still accurate. --- @@ -830,24 +823,18 @@ Dependency baseline: | Submodule | Version | Notes | |---|---|---| | `deps/libakerror` | 1.0.0 | Private ownership-enforced status registry. akbasic reserves 512–767 in `akbasic_error_register()`. Does not namespace its `coverage` target when embedded — worked around in our `CMakeLists.txt`. | -| `deps/libakstdlib` | 0.1.0 | soname `libakstdlib.so.0.1`. `AKSL_VERSION_CHECK()` asserted in `tests/version_check.c`. Its `aksl_ato*` family is banned here — see §1.9. | -| `deps/libakgl` | 0.1.0, **pinned at commit 42b60f7** | Migrated to the 1.0.0 registry and owns 256–260. Linked and tested under `-DAKBASIC_WITH_AKGL=ON`, which still defaults OFF so the core library and its whole suite build on a machine with no SDL. The version number cannot carry this requirement — see below. | +| `deps/libakstdlib` | 0.2.0 | soname `libakstdlib.so.0.2`. `AKSL_VERSION_CHECK()` asserted in `tests/version_check.c`. This release fixed all six confirmed defects the port was working around — see §1.9, where the bans are now lifted. | +| `deps/libakgl` | 0.2.0 | soname `libakgl.so.0.2`. Owns status codes 256–260. Linked and tested under `-DAKBASIC_WITH_AKGL=ON`, which still defaults OFF so the core library and its whole suite build on a machine with no SDL. It requires `libakstdlib` 0.2, which is why the two moved together. | -**The `libakgl` requirement is pinned by commit, not by version, and that is a defect -upstream.** 42b60f7 added 22 public symbols across four headers — `akgl_draw_*` (8), -`akgl_audio_*` (10), `akgl_controller_poll_key`/`_flush_keys`, and `akgl_text_measure`/ -`_measure_wrapped` — and left `project(akgl VERSION 0.1.0)` and the `libakgl.so.0.1` soname -unchanged. By that repository's own stated rule, *"0.1 and 0.2 are different ABIs"*, this -should have been 0.2.0. Two consequences land here: - -- A binary compiled against the new headers links happily against an installed - `libakgl.so.0.1` built from the old tree, and fails at symbol resolution rather than at - configure time. -- `AKGL_VERSION_AT_LEAST(0, 1, 0)` is true for both trees, so nothing here can feature-test - for the new API. The submodule pointer is the only thing that expresses the requirement. - -Filed in `deps/libakgl/TODO.md`. When it is fixed, add the floor to the `find_package` call -and say so here. +**The `libakgl` requirement used to be pinned by commit, and no longer is.** 42b60f7 added 22 +public symbols across four headers and left `project(akgl VERSION 0.1.0)` and the +`libakgl.so.0.1` soname unchanged, so nothing here could feature-test for the new API and the +submodule pointer was the only thing expressing the requirement. That was filed upstream as +`libakgl` defect #14 and fixed by its commit 1066ac7 — the two crossed rather than one +following the other. `libakgl` is 0.2.0 with an `libakgl.so.0.2` soname, so +`AKGL_VERSION_AT_LEAST(0, 2, 0)` now means something and an ABI mismatch is refused at load +time. Kept here because the *reason* is the reusable part: an additive release under an +unchanged soname costs its consumers the ability to detect it at all. CI is split in two. `.gitea/workflows/ci.yaml` runs on push -- the suite, ASan+UBSan, coverage and a two-file mutation run -- and `.gitea/workflows/release.yaml` is manual diff --git a/deps/libakgl b/deps/libakgl index 377c4c0..582008a 160000 --- a/deps/libakgl +++ b/deps/libakgl @@ -1 +1 @@ -Subproject commit 377c4c028f1e02abe52a81160c55994fb59f452f +Subproject commit 582008a411d405c891b0f78f768a00b297a0bf37 diff --git a/deps/libakstdlib b/deps/libakstdlib index fd71bcc..f8425b8 160000 --- a/deps/libakstdlib +++ b/deps/libakstdlib @@ -1 +1 @@ -Subproject commit fd71bcc67bf69eae416ea9f785352a3a8d41da95 +Subproject commit f8425b872909fa2680842cc5d55c355c63bfa7ff diff --git a/src/runtime_commands.c b/src/runtime_commands.c index 433ff6a..939ed18 100644 --- a/src/runtime_commands.c +++ b/src/runtime_commands.c @@ -525,6 +525,7 @@ akerr_ErrorContext *akbasic_cmd_dsave(akbasic_Runtime *obj, akbasic_ASTLeaf *exp FILE *fp = NULL; int64_t i = 0; int count = 0; + size_t written = 0; (void)lval; (void)rval; PASS(errctx, filename_argument(obj, expr, filename, sizeof(filename))); @@ -536,7 +537,14 @@ akerr_ErrorContext *akbasic_cmd_dsave(akbasic_Runtime *obj, akbasic_ASTLeaf *exp continue; } snprintf(line, sizeof(line), "%" PRId64 " %s\n", i, obj->source[i].code); - PASS(errctx, aksl_fwrite(line, 1, strlen(line), fp)); + /* + * The written count is required by libakstdlib 0.2.0 and discarded + * here on purpose: a short write is no longer something a caller has + * to notice for itself, because that release made it an AKERR_IO + * rather than a silent success. Before it, a DSAVE onto a full disk + * reported nothing. + */ + PASS(errctx, aksl_fwrite(line, 1, strlen(line), fp, &written)); count += 1; } } CLEANUP {