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) <noreply@anthropic.com>
This commit is contained in:
@@ -433,10 +433,10 @@ The exception is `FILTER`, which sets the SID's filter cutoff, band switches and
|
|||||||
# Dependencies
|
# Dependencies
|
||||||
|
|
||||||
* [libakerror](https://source.starfort.tech/andrew/libakerror) 1.0.0 — TRY/CATCH-style error contexts. Every function that can fail returns one.
|
* [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`.
|
* [libakstdlib](https://source.starfort.tech/andrew/libakstdlib) 0.2.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.
|
* [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.
|
* [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.
|
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.
|
||||||
|
|||||||
103
TODO.md
103
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
|
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).
|
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
|
`deps/libakstdlib` is at **0.2.0**, and that release *fixed all six* of the confirmed defects
|
||||||
carry a known-failing test, so a green `ctest` over there does not mean what you would
|
this section was built around, plus seventeen contract gaps. The table of bans that used to
|
||||||
assume. Three of the six sit directly in the path of this port. Check this table before
|
live here is gone because it is no longer true, and leaving it would send an agent around a
|
||||||
reaching for anything in `akstdlib.h`:
|
workaround for a function that now works.
|
||||||
|
|
||||||
| Call | Verdict | Why |
|
What changed, in the four that mattered to this port:
|
||||||
|---|---|---|
|
|
||||||
| `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. |
|
|
||||||
|
|
||||||
**The `aksl_ato*` ban is load-bearing, so here is the whole argument.** `atoi("not a number")`
|
| Was banned | Now |
|
||||||
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 |
|
|
||||||
|---|---|
|
|---|---|
|
||||||
| `basicgrammar.go:224` `newLiteralInt` → `strconv.ParseInt` | returns `err`, which the parser reports |
|
| `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 |
|
||||||
| `basicgrammar.go` `newLiteralFloat` → `strconv.ParseFloat` | returns `err` |
|
| `aksl_list_append` / `_iterate` | Append no longer truncates to two nodes; iterate no longer skips the first half |
|
||||||
| `basicscanner.go` `matchNumber` → `strconv.Atoi` | `PARSE` error, `"INTEGER CONVERSION ON '%s'"` |
|
| `aksl_tree_iterate` | `AKERR_ITERATOR_BREAK` stops the traversal |
|
||||||
| `basicruntime_functions.go:742` `FunctionVAL` | converts a string at runtime and must be able to fail |
|
| `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
|
Two signature changes reach this repository. `aksl_fread` and `aksl_fwrite` now take a
|
||||||
cosmetic loss: it converts four diagnosable errors into wrong answers, and `VAL("garbage")`
|
required `size_t *nmemb_out` and report a short transfer as `AKERR_IO` instead of a silent
|
||||||
starts returning `0.000000` instead of raising. The existing golden files would not catch it —
|
success — so a `DSAVE` onto a full disk is now an error a program sees, where before it
|
||||||
`tests/language/functions/val.txt` only covers `"32"`, `"123.456"` and `"-256"`, all valid.
|
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`
|
**`src/convert.c` is the outstanding consequence, and it is deliberately not done yet.** It
|
||||||
(`akbasic_str_to_int64`, `akbasic_str_to_double`) over `strtoll`/`strtod`, with `errno = 0`
|
exists only because `aksl_ato*` could not report a conversion failure, and its own note says
|
||||||
before the call, an `endptr` check for both "no digits consumed" and "trailing junk", and a
|
what to do when that changed: *"When it grows it, delete `src/convert.c` and switch the call
|
||||||
range check — raising `AKBASIC_ERR_VALUE` and `ERANGE` respectively. That is the contract
|
sites over."* That condition is now met. Doing it touches more than the two functions:
|
||||||
`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.
|
|
||||||
|
|
||||||
*Acceptance for `src/convert.c`:* `tests/convert.c` — valid decimal, valid hex, empty string,
|
- Four call sites — `newLiteralInt`/`newLiteralFloat` in `src/grammar.c`, `matchNumber` in
|
||||||
pure garbage, trailing junk, and an overflowing literal, each asserting the raised status.
|
`src/scanner.c`, and `FunctionVAL` in `src/runtime_functions.c`.
|
||||||
Add a `.bas`/`.txt` pair for `VAL` on a non-numeric string in the same commit; there is no
|
- `tests/convert.c`, which would move to asserting `libakstdlib`'s contract rather than ours.
|
||||||
golden coverage for it today.
|
- **`.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 |
|
| 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/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/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.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/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
|
**The `libakgl` requirement used to be pinned by commit, and no longer is.** 42b60f7 added 22
|
||||||
upstream.** 42b60f7 added 22 public symbols across four headers — `akgl_draw_*` (8),
|
public symbols across four headers and left `project(akgl VERSION 0.1.0)` and the
|
||||||
`akgl_audio_*` (10), `akgl_controller_poll_key`/`_flush_keys`, and `akgl_text_measure`/
|
`libakgl.so.0.1` soname unchanged, so nothing here could feature-test for the new API and the
|
||||||
`_measure_wrapped` — and left `project(akgl VERSION 0.1.0)` and the `libakgl.so.0.1` soname
|
submodule pointer was the only thing expressing the requirement. That was filed upstream as
|
||||||
unchanged. By that repository's own stated rule, *"0.1 and 0.2 are different ABIs"*, this
|
`libakgl` defect #14 and fixed by its commit 1066ac7 — the two crossed rather than one
|
||||||
should have been 0.2.0. Two consequences land here:
|
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
|
||||||
- A binary compiled against the new headers links happily against an installed
|
time. Kept here because the *reason* is the reusable part: an additive release under an
|
||||||
`libakgl.so.0.1` built from the old tree, and fails at symbol resolution rather than at
|
unchanged soname costs its consumers the ability to detect it at all.
|
||||||
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.
|
|
||||||
|
|
||||||
CI is split in two. `.gitea/workflows/ci.yaml` runs on push -- the suite, ASan+UBSan, coverage
|
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
|
and a two-file mutation run -- and `.gitea/workflows/release.yaml` is manual
|
||||||
|
|||||||
2
deps/libakgl
vendored
2
deps/libakgl
vendored
Submodule deps/libakgl updated: 377c4c028f...582008a411
2
deps/libakstdlib
vendored
2
deps/libakstdlib
vendored
Submodule deps/libakstdlib updated: fd71bcc67b...f8425b8729
@@ -525,6 +525,7 @@ akerr_ErrorContext *akbasic_cmd_dsave(akbasic_Runtime *obj, akbasic_ASTLeaf *exp
|
|||||||
FILE *fp = NULL;
|
FILE *fp = NULL;
|
||||||
int64_t i = 0;
|
int64_t i = 0;
|
||||||
int count = 0;
|
int count = 0;
|
||||||
|
size_t written = 0;
|
||||||
|
|
||||||
(void)lval; (void)rval;
|
(void)lval; (void)rval;
|
||||||
PASS(errctx, filename_argument(obj, expr, filename, sizeof(filename)));
|
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;
|
continue;
|
||||||
}
|
}
|
||||||
snprintf(line, sizeof(line), "%" PRId64 " %s\n", i, obj->source[i].code);
|
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;
|
count += 1;
|
||||||
}
|
}
|
||||||
} CLEANUP {
|
} CLEANUP {
|
||||||
|
|||||||
Reference in New Issue
Block a user