Avoid wiping full error context on release

Reviewed-on: #25

.gitea/workflows/ci.yaml

@@ -67,34 +67,6 @@ jobs:
           fail_on_failure: 'false'
       - run: echo "🍏 This job's status is ${{ job.status }}."
 
-  thread_sanitizer:
-    runs-on: ubuntu-latest
-    steps:
-      - name: Check out repository code
-        uses: actions/checkout@v4
-      - name: dependencies
-        run: |
-          sudo apt-get update -y
-          sudo apt-get install -y cmake gcc moreutils
-      # The thread tests assert exclusive ownership of pool slots and reserved
-      # ranges, which is checkable without tooling and runs in the job above.
-      # This is the run that proves there is no data race underneath them.
-      # libtsan arrives with gcc (libgcc-N-dev depends on it); the script
-      # disables ASLR because TSan aborts on kernels with vm.mmap_rnd_bits > 28.
-      - name: thread sanitizer
-        run: |
-          scripts/thread_test.sh build/tsan --output-junit "$(pwd)/tsan-junit.xml"
-      - name: publish thread sanitizer results
-        if: always()
-        uses: mikepenz/action-junit-report@v4
-        with:
-          report_paths: 'tsan-junit.xml'
-          annotate_only: true
-          detailed_summary: true
-          include_passed: true
-          fail_on_failure: 'true'
-      - run: echo "🍏 This job's status is ${{ job.status }}."
-
   mutation_test:
     runs-on: ubuntu-latest
     steps:

CMakeLists.txt

@@ -107,6 +107,19 @@ if(AKERR_SANITIZE AND NOT CMAKE_C_COMPILER_ID MATCHES "GNU|Clang")
         "AKERR_SANITIZE requires GCC or Clang, not ${CMAKE_C_COMPILER_ID}")
 endif()
 
+if(AKERR_SANITIZE STREQUAL "thread" AND CMAKE_SYSTEM_NAME STREQUAL "Linux")
+    # ThreadSanitizer reserves a fixed, very large virtual-address range. Linux
+    # ASLR can put a loader mapping inside it before the runtime starts, which
+    # makes every test fail with "unexpected memory mapping" before main().
+    # Run the instrumented tests under the normal util-linux ASLR wrapper.
+    find_program(AKERR_SETARCH_EXECUTABLE setarch)
+    if(NOT AKERR_SETARCH_EXECUTABLE)
+        message(FATAL_ERROR
+            "AKERR_SANITIZE=thread on Linux requires setarch (util-linux) to "
+            "start tests with ASLR disabled.")
+    endif()
+endif()
+
 function(akerr_instrument_for_sanitizers _target)
     if(AKERR_SANITIZE)
         target_compile_options(${_target} PRIVATE
@@ -145,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>
@@ -187,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.
@@ -214,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
@@ -241,18 +289,29 @@ 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)
+    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()
     akerr_instrument_for_sanitizers(test_${_test})
+    if(AKERR_SANITIZE STREQUAL "thread" AND CMAKE_SYSTEM_NAME STREQUAL "Linux")
+        add_test(NAME ${_test}
+                 COMMAND ${AKERR_SETARCH_EXECUTABLE} ${CMAKE_SYSTEM_PROCESSOR}
+                         -R $<TARGET_FILE:test_${_test}>)
+    else()
         add_test(NAME ${_test} COMMAND test_${_test})
+    endif()
     # A sanitizer report is a test failure. Without halt_on_error the runtime
     # prints and continues, which leaves a race to be noticed in the log by
     # somebody reading it -- and under a race storm the reporting itself is slow
@@ -263,6 +322,14 @@ foreach(_test IN LISTS AKERR_TESTS)
     endif()
 endforeach()
 
+# HANDLE_GROUP deliberately enters the next case label. Keep that public macro
+# compiling with the warning enabled, so a future macro edit cannot restore the
+# warning for consumers which adopt -Wextra.
+if(CMAKE_C_COMPILER_ID MATCHES "GNU|Clang")
+    target_compile_options(test_err_handle_group PRIVATE
+                           -Werror=implicit-fallthrough)
+endif()
+
 set_tests_properties(
     ${AKERR_WILL_FAIL_TESTS}
     PROPERTIES WILL_FAIL TRUE
@@ -278,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}
@@ -310,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}
@@ -334,8 +407,17 @@ configure_package_config_file(
     INSTALL_DESTINATION ${akerror_install_cmakedir}
 )
 
+# The SOVERSION is the project major version, so packages with the same major
+# are ABI-compatible and a different major must be rejected.
+write_basic_package_version_file(
+    "${CMAKE_CURRENT_BINARY_DIR}/akerrorConfigVersion.cmake"
+    VERSION ${PROJECT_VERSION}
+    COMPATIBILITY SameMajorVersion
+)
+
 install(FILES
     "${CMAKE_CURRENT_BINARY_DIR}/akerrorConfig.cmake"
+    "${CMAKE_CURRENT_BINARY_DIR}/akerrorConfigVersion.cmake"
     DESTINATION ${akerror_install_cmakedir}
 )
 

UPGRADING.md

@@ -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/building.md

@@ -58,4 +58,6 @@ cmake --install build
 `PATH_MAX` and `NULL` are used unconditionally but only included under the
 stdlib branch, so the header's includes need untangling before
 `-DAKERR_USE_STDLIB=OFF` builds. The list above still states what a replacement
-must provide. See [TODO.md](../TODO.md).
+must provide. That configuration does not currently compile -- `bool`, `PATH_MAX` and `NULL`
+are used unconditionally but included only under the stdlib branch. See
+[issue #12](https://source.starfort.tech/andrew/libakerror/issues/12).

include/akerror.tmpl.h

@@ -391,6 +391,18 @@ akerr_ErrorContext AKERR_NOIGNORE *__akerr_copy_string(char *destination, int ca
  * Defines for the ATTEMPT/CATCH/CLEANUP/PROCESS/HANDLE/FINISH process
  */
 
+/*
+ * HANDLE_GROUP deliberately enters the next case label. GCC and clang both
+ * understand this spelling in the C modes supported by libakerror. Other
+ * compilers keep the established control flow without receiving an attribute
+ * they do not implement.
+ */
+#if defined(__GNUC__) || defined(__clang__)
+#define AKERR_FALLTHROUGH __attribute__((fallthrough));
+#else
+#define AKERR_FALLTHROUGH
+#endif
+
 #define ATTEMPT					\
     switch ( 0 ) {				\
     case 0:					\
@@ -443,6 +455,7 @@ akerr_ErrorContext AKERR_NOIGNORE *__akerr_copy_string(char *destination, int ca
     __err_context->handled = true;
 
 #define HANDLE_GROUP(__err_context, __err_status)			\
+    AKERR_FALLTHROUGH					\
     case __err_status:							\
     __err_context->stacktracebufptr = (char *)&__err_context->stacktracebuf; \
     __err_context->handled = true;

src/error.c

@@ -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];
@@ -377,10 +380,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 +401,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/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
+ * Releasing an error context back to the pool must reset the state that affects
- * that checks it out never sees stale status/message/stacktrace from a previous
+ * the next caller. In particular, a handled error must not make a fresh error
- * error. Mutation testing showed the clearing memset in akerr_release_error
+ * look handled when its slot is recycled.
- * could be deleted without any test noticing.
  */
 
+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;