Andrew Kesterson
be24f80022
Every entry point may now be called from any thread. akerr_init() runs exactly once however many threads race into it, the pool hands each slot to exactly one thread, and reservations, registrations and lookups are serialized against each other. One recursive lock covers both tables (src/lock.h, private). Recursive because raising an error re-enters the library -- FAIL needs a pool slot and a status name -- and single because two locks would mean an ordering to get wrong. Registry bodies that use the early-returning FAIL_*_RETURN macros are split into *_locked functions behind wrappers that take and release the lock on one path; consumer callbacks are never called under it. This is an ABI break, hence 2.0.0 and SOVERSION 2: - akerr_next_error() now returns a context that already holds its reference. Finding a free slot and claiming it has to be one operation, or two threads scanning at once are handed the same slot. ENSURE_ERROR_READY no longer increments. - __akerr_last_ignored is thread-local, as is the last-ditch context used to report akerr_release_error(NULL). The threading backend is chosen at configure time by AKERR_THREADS (auto, pthread, none). auto fails the configure when it cannot find POSIX threads rather than quietly building a library that reports itself thread safe and is not. generrno.sh stamps the decision into the generated header as AKERR_THREAD_SAFE, so a consumer cannot disagree with the library about it. Tests: err_threads_init, err_threads_pool and err_threads_registry assert exclusive slot ownership, exactly one winner for a contested range, and every registered name readable back under contention. AKERR_SANITIZE builds the library and the tests with any sanitizer; scripts/thread_test.sh runs the suite under ThreadSanitizer and CI runs it. Removing the pool lock makes both the sanitizer and the plain assertions fail, so the tests are not vacuous. Documented in README.md and UPGRADING.md, including what this does not cover: renaming a status while another thread looks it up, and which of two simultaneous unhandled errors sets the exit status. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
108 lines
5.6 KiB
Markdown
108 lines
5.6 KiB
Markdown
# TODO
|
|
|
|
Working notes for `libakerror`. Outstanding items only.
|
|
|
|
## 1. Only ThreadSanitizer is wired into CI, not ASan/UBSan
|
|
|
|
`AKERR_SANITIZE` builds the library and the tests with any sanitizer list, and
|
|
CI runs `-DAKERR_SANITIZE=thread` through `scripts/thread_test.sh`. Nothing runs
|
|
`address,undefined` yet, and that is the one that covers the original
|
|
motivation: mutation testing caught an out-of-bounds probe in the status-name
|
|
hash table that the suite could not, because the failure mode was a write into
|
|
adjacent BSS, which does not crash. Sharpening one test closed that instance;
|
|
ASan would catch the whole class regardless of how sharp the assertions are.
|
|
|
|
The machinery is in place — this is one more job in
|
|
`.gitea/workflows/ci.yaml` running
|
|
`cmake -S . -B build/asan -DAKERR_SANITIZE=address,undefined`. Left separate
|
|
because ASan and TSan cannot be combined in one build.
|
|
|
|
## 2. `HANDLE`-level status aliasing is still undetectable
|
|
|
|
Two components can compile the same integer into a `case` label without ever
|
|
reserving a range or registering a name, and nothing sees it. Ownership
|
|
enforcement covers *naming*, which is the part the library mediates; the `case`
|
|
label never reaches it.
|
|
|
|
Closing this needs the `if`/`else if` handler ladder — rewriting
|
|
`PROCESS`/`HANDLE`/`HANDLE_GROUP`/`HANDLE_DEFAULT`/`FINISH` so status matching
|
|
is not restricted to integer constant expressions. That would also allow
|
|
matching on ranges or predicates, and would let a handler resolve a code through
|
|
its owner. It touches the most load-bearing code in the library and every
|
|
consumer's error handling at once, so it wants its own change.
|
|
|
|
Note it would *not* by itself fix the "don't use `CATCH` or `FAIL_*_BREAK`
|
|
inside a loop" hazard: that comes from exiting via `break`, not from `switch`.
|
|
|
|
## 3. No registry introspection
|
|
|
|
There is no way to ask who owns a status, or to enumerate reservations. The
|
|
"coordinate ranges at the dependency-stack level" advice in UPGRADING.md
|
|
therefore has no tooling behind it.
|
|
|
|
A read-only accessor plus a dump through `akerr_log_method` would let a startup
|
|
self-check or a CI job print the whole map for a linked stack. Cheap, additive,
|
|
and the natural next step for multi-component adoption.
|
|
|
|
## 4. No way to release a reservation
|
|
|
|
A plugin host that `dlopen`s many distinct plugins over a process lifetime
|
|
accumulates ranges until the table fills. Reloading the *same* plugin is fine —
|
|
an identical repeat by the same owner is idempotent.
|
|
|
|
## 5. Renaming a status is not safe against a concurrent lookup
|
|
|
|
`akerr_name_for_status(status, NULL)` returns a pointer into the registry rather
|
|
than a copy, which is what makes it usable from inside `FAIL` — it needs no
|
|
buffer and no error context of its own. Registering a *second* name for a status
|
|
that already has one (`tests/err_name_ownership.c` covers that it is allowed)
|
|
overwrites that buffer in place, so a thread reading the name at that moment can
|
|
see a torn string. Every other registry operation is serialized; this one cannot
|
|
be, because the reader is outside the lock by the time it reads the characters.
|
|
|
|
Documented in README.md and UPGRADING.md as "register names during
|
|
initialization". Closing it properly means making a registered name immutable —
|
|
either refusing a rename outright (a behavior change, and
|
|
`tests/err_name_ownership.c` asserts the current contract), or copying names
|
|
into a bump-allocated arena and publishing the pointer with a release store, so
|
|
a rename allocates new storage instead of rewriting live storage. The arena is
|
|
the better answer; it costs a second capacity limit and its exhaustion path.
|
|
|
|
## 6. Deprecate the two-argument name-registration path
|
|
|
|
`akerr_name_for_status(status, name)` cannot identify its caller, so it can only
|
|
check that *some* reservation covers the status, not that the caller owns it. It
|
|
exists for migration. Once consumers have moved to
|
|
`akerr_register_status_name()`, make the set path a no-op or remove it and leave
|
|
`akerr_name_for_status()` as pure lookup.
|
|
|
|
## 7. `akerr_init()`'s own reservation failure is untested
|
|
|
|
`tests/err_library_status_fatal.c` covers the terminal path in
|
|
`__akerr_name_library_status()` by naming a status the library does not own. The
|
|
band reservation in `akerr_init()` has no such handle: it can only fail in a
|
|
build whose tables are too small for the library's own entries, and both sizes
|
|
are `PRIVATE` to the library target, so a test executable cannot set them.
|
|
|
|
Closing it means a second library target built with tiny tables plus a
|
|
`WILL_FAIL` test linked against it. Nothing in the CMake does that yet: the
|
|
sanitizer and coverage options vary the *flags* of the one library target, not
|
|
its compile definitions.
|
|
|
|
Related: branch coverage on `src/error.c` now sits just above its 50% gate.
|
|
Every `FAIL_*` site carries about six branch outcomes of error-construction
|
|
machinery (`ENSURE_ERROR_READY`, `AKERR_STACKTRACE_APPEND`) that only run when
|
|
that specific failure fires, and every `PASS` site around a call that cannot
|
|
fail carries about twenty-five. Validating more inputs therefore lowers the
|
|
ratio by construction. Before adding defensive checks, expect to add a test that
|
|
drives them, as `tests/err_copy_string.c` does.
|
|
|
|
## Unrelated pre-existing issues
|
|
|
|
- The `AKERR_USE_STDLIB=OFF` build does not compile at all: `bool`, `PATH_MAX`
|
|
and `NULL` are used unconditionally but only included under the stdlib branch.
|
|
The README's dependency list states what a replacement must provide, but the
|
|
header still needs its includes untangled for that configuration to work.
|
|
- `CMakeLists.txt` sets `main_lib_dest` from `MY_LIBRARY_VERSION`, which is never
|
|
defined and never read. Dead line.
|