Merge pull request 'Avoid wiping full error context on release' (#27) from 26 into main

Reviewed-on: #27

CMakeLists.txt

@@ -1,7 +1,7 @@
 cmake_minimum_required(VERSION 3.10)
 # 1.0.0 replaced the consumer-sized __AKERR_ERROR_NAMES array with private
 # storage. 2.0.0 makes the library thread safe, which is a second ABI break in
-# the same places: __akerr_last_ignored became thread-local storage, and
+# the same places: akerr_last_ignored became thread-local storage, and
 # ENSURE_ERROR_READY no longer takes the pool reference that akerr_next_error()
 # now takes for it. Consumer code compiled against a 1.x header would
 # double-count every reference. Hence the major bump and the SOVERSION, so a
@@ -158,10 +158,18 @@ add_custom_command(
   VERBATIM
 )
 
+# More than one library target consumes the generated sources below. Route
+# them through one explicit prerequisite so parallel Make builds cannot invoke
+# the same generator twice and interleave writes to errno.c.
+add_custom_target(akerror_generated
+  DEPENDS ${GENERATED_ERRNO_C} ${GENERATED_AKERROR_H}
+)
+
 add_library(akerror SHARED
   src/error.c
   ${GENERATED_ERRNO_C}
 )
+add_dependencies(akerror akerror_generated)
 
 target_include_directories(akerror PUBLIC
   $<BUILD_INTERFACE:${GENERATED_DIR}/include>
@@ -200,6 +208,32 @@ set_target_properties(akerror PROPERTIES
 akerr_instrument_for_coverage(akerror)
 akerr_instrument_for_sanitizers(akerror)
 
+# akerr_init() must terminate if it cannot reserve the library-owned status
+# band. The production table sizes are deliberately PRIVATE, so exercise that
+# otherwise unreachable startup failure with a second library target whose
+# private registries cannot accept even the first reservation. Keeping this a
+# distinct target is the test: no compile definition leaks into consumers or
+# weakens the production library.
+add_library(akerror_init_failure SHARED
+  src/error.c
+  ${GENERATED_ERRNO_C}
+)
+add_dependencies(akerror_init_failure akerror_generated)
+target_include_directories(akerror_init_failure PUBLIC
+  ${GENERATED_DIR}/include
+)
+target_compile_definitions(akerror_init_failure
+  PUBLIC AKERR_USE_STDLIB=${AKERR_USE_STDLIB}
+  PRIVATE AKERR_STATUS_NAME_SLOTS=8
+  PRIVATE AKERR_MAX_RESERVED_STATUS_RANGES=0
+  PRIVATE ${AKERR_THREADS_DEFINE}
+)
+if(AKERR_THREAD_SAFE)
+    target_link_libraries(akerror_init_failure PRIVATE Threads::Threads)
+endif()
+akerr_instrument_for_coverage(akerror_init_failure)
+akerr_instrument_for_sanitizers(akerror_init_failure)
+
 # Each test is one source file in tests/ built into test_<name> and registered
 # as CTest <name>. Tests expected to abort (unhandled error / contract
 # violation) go in AKERR_WILL_FAIL_TESTS; all others must exit 0.
@@ -227,6 +261,7 @@ set(AKERR_TESTS
     err_registry_init_order
     err_status_exception
     err_copy_string
+    err_init_reservation_fatal
     err_library_status_fatal
     err_refcount_double_fail
     err_stacktrace_bounds
@@ -254,13 +289,18 @@ endif()
 set(AKERR_WILL_FAIL_TESTS
     err_trace
     err_improper_closure
+    err_init_reservation_fatal
     err_library_status_fatal
 )
 
 foreach(_test IN LISTS AKERR_TESTS)
     add_executable(test_${_test} tests/${_test}.c)
     target_include_directories(test_${_test} PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/tests)
-    target_link_libraries(test_${_test} PRIVATE akerror)
+    if(_test STREQUAL "err_init_reservation_fatal")
+        target_link_libraries(test_${_test} PRIVATE akerror_init_failure)
+    else()
+        target_link_libraries(test_${_test} PRIVATE akerror)
+    endif()
     if(AKERR_THREAD_SAFE)
         target_link_libraries(test_${_test} PRIVATE Threads::Threads)
     endif()
@@ -305,7 +345,14 @@ if(Python3_FOUND)
     # The script configures and drives its own instrumented build tree (under
     # ${CMAKE_BINARY_DIR}/coverage) so this build's binaries and its coverage
     # counters can never be stale or half-instrumented. Reports via gcov.
-    add_custom_target(coverage
+    # Keep the convenient generic name at the top level, but namespace it when
+    # embedded so a parent project can provide its own coverage target.
+    if(CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR)
+        set(AKERR_COVERAGE_TARGET coverage)
+    else()
+        set(AKERR_COVERAGE_TARGET akerror_coverage)
+    endif()
+    add_custom_target(${AKERR_COVERAGE_TARGET}
         COMMAND ${Python3_EXECUTABLE}
                 ${CMAKE_CURRENT_SOURCE_DIR}/scripts/coverage.py
                 --source-root ${CMAKE_CURRENT_SOURCE_DIR}
@@ -337,7 +384,6 @@ if(Python3_FOUND)
     )
 endif()
 
-set(main_lib_dest "lib/my_library-${MY_LIBRARY_VERSION}")
 install(TARGETS akerror
   EXPORT akerrorTargets
   ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}

README.md

@@ -170,7 +170,7 @@ the exit code was the status truncated to a byte, and every consumer status
 starts at 256. Use `akerr_exit()` instead of `exit()` — see
 [docs/exit-status.md](docs/exit-status.md). No ABI break.
 
-2.0.0 makes the library thread safe. That is an ABI break — `__akerr_last_ignored`
+2.0.0 makes the library thread safe. That is an ABI break — `akerr_last_ignored`
 became thread-local storage and the pool now takes its own reference — so
 everything built against a 1.x header must be rebuilt. 1.0.0 replaced the
 consumer-sized status-name array with a private, ownership-enforced registry.

UPGRADING.md

@@ -52,7 +52,7 @@ accident.
 
 What moved at the ABI:
 
-* `__akerr_last_ignored` is thread-local storage. An ignored error is a fact
+* `akerr_last_ignored` is thread-local storage. An ignored error is a fact
   about the thread that ignored it, and one shared slot had two threads
   overwriting each other's. The `IGNORE` macro expands at *your* call site, so
   your objects reference the symbol under whichever storage model your header
@@ -132,6 +132,11 @@ One recursive lock covers both the pool and the registry, so error
 correctness there is worth more than throughput, but a program that raises
 errors in a hot loop will feel it.
 
+Releasing the last reference to a context remains serialized under that same
+pool lock, but it now resets only the handled/status/reported state, the string
+heads, and the stack-trace cursor. It no longer wipes the whole context buffer,
+so release is a fixed handful of stores rather than a tens-of-kilobytes write.
+
 The per-thread last-ditch context is a whole `akerr_ErrorContext` (tens of
 kilobytes) in thread-local storage, allocated per thread on first use of the
 library from that thread.

docs/thread-safety.md

@@ -14,9 +14,12 @@ What that covers:
   against each other and against lookups. Two threads reserving the same range
   cannot both win — exactly one gets `NULL` and the other gets
   `AKERR_STATUS_RANGE_OVERLAP` naming the winner.
-* **Per-thread state.** The context behind `IGNORE` (`__akerr_last_ignored`) and
-  the last-ditch context used to report `akerr_release_error(NULL)` are
-  thread-local, so one thread's ignored error is never another's.
+* **Per-thread state.** `IGNORE` copies the swallowed context into its
+  thread-local `akerr_last_ignored` snapshot before releasing the pool slot.
+  The snapshot remains valid until that thread ignores another error, so a
+  later pool checkout cannot overwrite it. The snapshot and the last-ditch
+  context used to report `akerr_release_error(NULL)` are thread-local, so
+  concurrent calls cannot overwrite each other's state.
 * **Handing a context from one thread to another.** A context is not thread
   state — it lives in `AKERR_ARRAY_ERROR`, which is process-global — so it
   outlives the thread that raised it. The reference count is the only field the

include/akerror.tmpl.h

@@ -15,7 +15,7 @@
  * scripts/generrno.sh stamps this value in at build time from the AKERR_THREADS
  * build option, the same way it stamps AKERR_LAST_ERRNO_VALUE. It is generated
  * rather than defined by the consumer on purpose: whether the library
- * serializes its global state and whether __akerr_last_ignored is a
+ * serializes its global state and whether akerr_last_ignored is a
  * thread-local are the same decision, and a consumer that disagreed with the
  * library about it would link against a differently shaped symbol.
  *
@@ -173,11 +173,12 @@ extern akerr_ErrorContext AKERR_ARRAY_ERROR[AKERR_MAX_ARRAY_ERROR];
 extern akerr_ErrorUnhandledErrorHandler akerr_handler_unhandled_error;
 extern akerr_ErrorLogFunction akerr_log_method;
 /*
- * The error IGNORE() last swallowed, per thread: an ignored error is a fact
- * about the thread that ignored it, and one shared slot would have two threads
- * overwriting each other's. Thread local only when AKERR_THREAD_SAFE is 1.
+ * IGNORE()'s public per-thread snapshot. IGNORE() copies the swallowed error here
+ * before releasing its pool context, so this remains a useful debugging aid
+ * after the pool slot is reused. The snapshot is read-only and is replaced by
+ * the next ignored error. Thread local only when AKERR_THREAD_SAFE is 1.
  */
-extern AKERR_THREAD_LOCAL akerr_ErrorContext *__akerr_last_ignored;
+static AKERR_THREAD_LOCAL akerr_ErrorContext akerr_last_ignored;
 
 /*
  * Drop one reference, returning NULL once the last one is gone so the caller can
@@ -434,10 +435,20 @@ akerr_ErrorContext AKERR_NOIGNORE *__akerr_copy_string(char *destination, int ca
     FINISH_LOGIC(__err_context, true);
 
 #define IGNORE(__stmt)							\
-    __akerr_last_ignored = __stmt;					\
-    if ( __akerr_last_ignored != NULL ) {				\
-	LOG_ERROR_WITH_MESSAGE(__akerr_last_ignored, "** IGNORED ERROR **"); \
-    }
+    do {								\
+	akerr_ErrorContext *__akerr_ignored = __stmt;				\
+	if ( __akerr_ignored != NULL ) {				\
+	    memcpy(&akerr_last_ignored, __akerr_ignored,	\
+		   sizeof(akerr_last_ignored));			\
+	    akerr_last_ignored.stacktracebufptr =		\
+		(char *)&akerr_last_ignored.stacktracebuf;	\
+	    akerr_ErrorContext *__akerr_ignored_snapshot =	\
+		&akerr_last_ignored;					\
+	    LOG_ERROR_WITH_MESSAGE(__akerr_ignored_snapshot,	\
+				   "** IGNORED ERROR **");		\
+	    RELEASE_ERROR(__akerr_ignored);				\
+	}								\
+    } while ( 0 )
 
 #define CLEANUP					\
     }; 

src/error.c

@@ -20,10 +20,10 @@
  * It is not small (an akerr_ErrorContext is tens of kilobytes), but the storage
  * is allocated per thread only when that thread first touches the library's
  * thread-local block, and the alternative is a shared buffer that two threads
- * can be writing at once.
+ * can be writing at once. The per-thread IGNORE() snapshot lives in the public
+ * template header because the macro copies into it at the call site.
  */
 static AKERR_THREAD_LOCAL akerr_ErrorContext __akerr_last_ditch;
-AKERR_THREAD_LOCAL akerr_ErrorContext *__akerr_last_ignored;
 akerr_ErrorUnhandledErrorHandler akerr_handler_unhandled_error;
 akerr_ErrorLogFunction akerr_log_method = NULL;
 
@@ -88,7 +88,10 @@ typedef struct
 
 static akerr_StatusName akerr_status_names[AKERR_STATUS_NAME_SLOTS];
 static int akerr_status_name_count;
-static akerr_StatusRange akerr_status_ranges[AKERR_MAX_RESERVED_STATUS_RANGES];
+/* C has no portable zero-length arrays. Keep one unused physical slot when a
+ * test build sets the logical capacity to zero to drive init's fatal path. */
+static akerr_StatusRange akerr_status_ranges[
+    AKERR_MAX_RESERVED_STATUS_RANGES > 0 ? AKERR_MAX_RESERVED_STATUS_RANGES : 1];
 static int akerr_status_range_count;
 
 akerr_ErrorContext AKERR_ARRAY_ERROR[AKERR_MAX_ARRAY_ERROR];
@@ -233,7 +236,6 @@ static void akerr_init_state(void)
 	AKERR_ARRAY_ERROR[i].arrayid = i;
 	AKERR_ARRAY_ERROR[i].stacktracebufptr = (char *)&AKERR_ARRAY_ERROR[i].stacktracebuf;
     }
-    __akerr_last_ignored = NULL;
     (void)akerr_last_ditch_context();
     if ( akerr_log_method == NULL ) {
 	akerr_log_method = &akerr_default_logger;
@@ -377,10 +379,10 @@ akerr_ErrorContext *akerr_next_error()
 }
 
 /*
- * The wipe returns the slot to the pool, so it and the decrement that triggers
+ * The reset returns the slot to the pool, so it and the decrement that triggers
  * it are one operation under the lock. Otherwise a thread that saw the count
  * reach zero could be handed the slot by akerr_next_error() and start writing
- * its error into it while the releasing thread was still memsetting it.
+ * its error into it while the releasing thread was still resetting it.
  */
 akerr_ErrorContext *akerr_release_error(akerr_ErrorContext *err)
 {
@@ -398,7 +400,13 @@ akerr_ErrorContext *akerr_release_error(akerr_ErrorContext *err)
     }
     if ( err->refcount == 0 ) {
 	oldid = err->arrayid;
-	memset(err, 0x00, sizeof(akerr_ErrorContext));
+	err->handled = false;
+	err->status = 0;
+	err->reported = false;
+	err->message[0] = '\0';
+	err->fname[0] = '\0';
+	err->function[0] = '\0';
+	err->stacktracebuf[0] = '\0';
 	err->stacktracebufptr = (char *)&err->stacktracebuf;
 	err->arrayid = oldid;
 	remaining = NULL;

tests/MUTATION.md

@@ -107,7 +107,7 @@ The remaining survivors are dominated by:
 
 * **Equivalent mutants** in `akerr_init`: deleting the `memset`/`NULL` setup of
   file-scope statics (`AKERR_ARRAY_ERROR`, `__akerr_last_ditch`,
-  `__akerr_last_ignored`) changes nothing, because C already zero-initializes
+  `akerr_last_ignored`) changes nothing, because C already zero-initializes
   objects with static storage duration. `int oldid = 0;` → `1` is likewise
   dead: it is overwritten before use, and so is clearing `akerr_initializing`
   at the end of initialization — nothing reads that flag once the once-routine

tests/err_ignore.c

@@ -1,11 +1,8 @@
 #include "akerror.h"
 #include "err_capture.h"
+#include <string.h>
 
-/*
- * IGNORE deliberately swallows an error: it records the context in
- * __akerr_last_ignored, logs it with an "IGNORED ERROR" marker, and lets
- * execution continue.
- */
+/* IGNORE snapshots and logs an error, releases its pool slot, then continues. */
 
 akerr_ErrorContext *boom(void)
 {
@@ -21,11 +18,19 @@ int main(void)
     PREPARE_ERROR(e);
     (void)e;
 
-    IGNORE(boom());
+    /* More failures than the pool has slots must remain safe: a leaking
+     * IGNORE used to exhaust the pool and terminate the process here. The
+     * copied snapshot must also survive the slot being reused on the next
+     * iteration. */
+    for ( int i = 0; i < AKERR_MAX_ARRAY_ERROR + 1; i++ ) {
+	IGNORE(boom());
+	AKERR_CHECK(akerr_last_ignored.status == AKERR_VALUE);
+	AKERR_CHECK(strcmp(akerr_last_ignored.message,
+			  "this error is ignored on purpose") == 0);
+	AKERR_CHECK(akerr_slots_in_use() == 0);
+    }
     reached_after_ignore = 1;
 
-    AKERR_CHECK(__akerr_last_ignored != NULL);
-    AKERR_CHECK(__akerr_last_ignored->status == AKERR_VALUE);
     AKERR_CHECK(reached_after_ignore == 1);
     AKERR_CHECK_CONTAINS("IGNORED ERROR");
     AKERR_CHECK_CONTAINS("this error is ignored on purpose");

tests/err_init_reservation_fatal.c

@@ -0,0 +1,20 @@
+#include "akerror.h"
+
+#include <stdio.h>
+
+/*
+ * This executable links to akerror_init_failure, a test-only library target
+ * with no status-range slots. The first reservation in akerr_init() must be
+ * terminal: continuing would leave every library status unowned and make all
+ * subsequent name registrations invalid.
+ *
+ * CTest marks this WILL_FAIL. Reaching the message and returning zero means
+ * initialization swallowed its own reservation failure.
+ */
+int main(void)
+{
+    akerr_init();
+
+    fprintf(stderr, "err_init_reservation_fatal: akerr_init did not terminate\n");
+    return 0;
+}

tests/err_release_clears.c

@@ -1,24 +1,32 @@
 #include "akerror.h"
 #include "err_capture.h"
-#include <string.h>
 
 /*
- * Releasing an error context back to the pool must wipe it, so the next caller
- * that checks it out never sees stale status/message/stacktrace from a previous
- * error. Mutation testing showed the clearing memset in akerr_release_error
- * could be deleted without any test noticing.
+ * Releasing an error context back to the pool must reset the state that affects
+ * the next caller. In particular, a handled error must not make a fresh error
+ * look handled when its slot is recycled.
  */
 
+static int unhandled_calls = 0;
+static int unhandled_status = 0;
+
+static void test_unhandled_handler(akerr_ErrorContext *errctx)
+{
+    unhandled_calls++;
+    unhandled_status = (errctx != NULL) ? errctx->status : 0;
+}
+
 akerr_ErrorContext *boom(void)
 {
     PREPARE_ERROR(e);
-    FAIL_RETURN(e, AKERR_VALUE, "stale dirty message that must not survive");
+    FAIL_RETURN(e, AKERR_VALUE, "first error is handled");
 }
 
 int main(void)
 {
     akerr_capture_install();
     akerr_init();
+    akerr_handler_unhandled_error = &test_unhandled_handler;
 
     /* Raise and fully handle an error; FINISH_NORETURN releases it to the pool. */
     PREPARE_ERROR(e);
@@ -32,13 +40,31 @@ int main(void)
 
     AKERR_CHECK(e == NULL);
 
-    /* The next context handed out is the slot we just released: it must be clean. */
+    /* A fresh error in the recycled slot must not inherit handled=true. */
+    PREPARE_ERROR(fresh);
+    ATTEMPT {
+	CATCH(fresh, boom());
+    } CLEANUP {
+    } PROCESS(fresh) {
+    } FINISH_NORETURN(fresh);
+
+    AKERR_CHECK(unhandled_calls == 1);
+    AKERR_CHECK(unhandled_status == AKERR_VALUE);
+    AKERR_CHECK(fresh == NULL);
+
+    /* The next context handed out is the same slot, with recycle state reset. */
     akerr_ErrorContext *slot = akerr_next_error();
     AKERR_CHECK(slot != NULL);
+    AKERR_CHECK(slot->handled == false);
     AKERR_CHECK(slot->status == 0);
+    AKERR_CHECK(slot->reported == false);
     AKERR_CHECK(slot->message[0] == '\0');
+    AKERR_CHECK(slot->fname[0] == '\0');
+    AKERR_CHECK(slot->function[0] == '\0');
     AKERR_CHECK(slot->stacktracebuf[0] == '\0');
-    AKERR_CHECK(strstr(slot->message, "stale dirty message") == NULL);
+    AKERR_CHECK(slot->stacktracebufptr == (char *)&slot->stacktracebuf);
+    RELEASE_ERROR(slot);
+    AKERR_CHECK(akerr_slots_in_use() == 0);
 
     fprintf(stderr, "err_release_clears ok\n");
     return 0;

tests/err_threads_pool.c

@@ -100,17 +100,21 @@ static void *pool_body(void *raw)
 	one_checkout(arg);
     }
 
-    /* An ignored error is a fact about the thread that ignored it: each thread
-     * must see its own, not the last one any thread swallowed. */
+    /* IGNORE's snapshot is thread-local while logging and remains valid after
+     * release. Concurrent ignored errors must all return their pool slots. */
     IGNORE(ignorable(arg));
-    AKERR_TCHECK(arg, __akerr_last_ignored != NULL);
-    if ( __akerr_last_ignored != NULL ) {
-	AKERR_TCHECK(arg, __akerr_last_ignored->status == AKERR_IO);
-	AKERR_TCHECK(arg, strcmp(__akerr_last_ignored->message, expected) == 0);
+    AKERR_TCHECK(arg, akerr_last_ignored.status == AKERR_IO);
+    AKERR_TCHECK(arg, strcmp(akerr_last_ignored.message, expected) == 0);
+
+    /* Reuse a slot after IGNORE and prove that the copied snapshot did not
+     * become an alias for the newly acquired context. */
+    akerr_ErrorContext *reused = akerr_next_error();
+    AKERR_TCHECK(arg, reused != NULL);
+    if ( reused != NULL ) {
+	RELEASE_ERROR(reused);
     }
-    /* IGNORE keeps the reference by design; hand it back so the pool is empty
-     * at the end of the test. */
-    RELEASE_ERROR(__akerr_last_ignored);
+    AKERR_TCHECK(arg, akerr_last_ignored.status == AKERR_IO);
+    AKERR_TCHECK(arg, strcmp(akerr_last_ignored.message, expected) == 0);
 
     return NULL;
 }