From 208f343e0f2d7b0bbebcd3c4730bd44aae75b588 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 16:31:25 +0000 Subject: [PATCH 1/5] Phase 1 of #149: make the failures visible Ships Phase 1 of the deep-audit plan of action from #149 -- CI/tooling infrastructure and a new regression test that surface the failures Phase 2 needs to fix, without fixing any of them yet. - E1: add -Wall -Wextra -Werror to the dedx_objects library target (GCC/Clang; MSVC's /W4 baseline is a separate, unverified pass left for later). Clears the 8 live -Wsign-compare warnings by widening loop counters that are only ever compared against unsigned bounds (elements_length, stopping_data.length, etc.) to unsigned int, plus 2 -Wunused-parameter warnings in the Bethe evaluators by casting the genuinely-unused `err` parameter to void -- both evaluators are pure arithmetic and never set it (tracked for de-duplication in #149 E3). No behavioural change; full suite still green. - E2: add a `sanitize` CMake preset (-fsanitize=address,undefined) and a new `sanitize` CI job that builds and runs the full ctest suite under it. Extend the existing Valgrind job from a single test binary (test_bethe_ext00) to every test_* binary, with one deliberate exception (see below). - E2: add tests/test_availability_exhaustive.c, sweeping every (program, ion, material) triple dedx_get_material_list_for_ion() advertises (skipping DEDX_ESTAR, which is unimplemented) and checking dedx_load_config() succeeds, the program/ion's own advertised energy bounds are accepted, and at least one sampled energy returns a finite, positive value. This reproduces the exact counts from #149's manual audit: 101957 combinations swept, 407 load failures, 1568 bound mismatches, 174 configs that load successfully but return ENERGY_OUT_OF_RANGE at every energy -- the regression net for findings A1/A4/A5/A6. The test asserts each count stays at or below that known baseline (a ratchet, not a pass/fail on zero) so the tree stays green now; Phase 2 must lower the baselines as each root cause is fixed. At ~102k load/query cycles this takes ~15s natively / ~25s under ASan+UBSan, but 15+ minutes under Valgrind for near-zero incremental coverage over the other test_* binaries already exercising the same code paths there, so it's excluded from the Valgrind job specifically (with a comment explaining why) while staying in the plain and sanitize suites. Verified locally: full ctest suite (33/33) green under a plain build, under -Wall -Wextra -Werror, under ASan+UBSan, and under Valgrind (leak-check=full, track-origins=yes) for every test_* binary except the one documented exception above. clang-format and clang-tidy clean on all changed/added files. --- .github/workflows/ci.yml | 39 +++++- .gitignore | 1 + CMakePresets.json | 21 +++ src/CMakeLists.txt | 7 + src/dedx.c | 8 +- src/dedx_bethe.c | 9 ++ src/dedx_validate.c | 8 +- tests/test_availability_exhaustive.c | 202 +++++++++++++++++++++++++++ 8 files changed, 284 insertions(+), 11 deletions(-) create mode 100644 tests/test_availability_exhaustive.c diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 01ca4ae..1a40b4e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -58,10 +58,43 @@ jobs: sudo apt-get update sudo apt-get install -y valgrind - - name: Valgrind test_bethe_ext00 + - name: Valgrind full test suite run: | - valgrind --leak-check=full --track-origins=yes --error-exitcode=1 \ - ./build/tests/test_bethe_ext00 + set -e + for bin in build/tests/test_*; do + # test_availability_exhaustive runs ~102k load/query cycles, which is fine + # at ~15s natively or ~25s under ASan+UBSan (see the sanitize job) but takes + # 15+ minutes under Valgrind's much heavier instrumentation, for close to + # zero incremental memory-safety coverage over what the other test_* binaries + # already give Valgrind on the same load/query code paths. Skip it here. + if [ "$(basename "$bin")" = "test_availability_exhaustive" ]; then + continue + fi + if [ -x "$bin" ] && [ -f "$bin" ]; then + echo "::group::valgrind $bin" + valgrind --leak-check=full --track-origins=yes --error-exitcode=1 "$bin" + echo "::endgroup::" + fi + done + + sanitize: + runs-on: ubuntu-latest + needs: build_and_test + steps: + - name: Checkout + uses: actions/checkout@v7 + + - name: Configure + run: cmake --preset sanitize + + - name: Build + run: cmake --build --preset sanitize --parallel + + - name: Run CTest under ASan+UBSan + run: ctest --preset sanitize + env: + ASAN_OPTIONS: detect_leaks=1 + UBSAN_OPTIONS: print_stacktrace=1:halt_on_error=1 python_tests: runs-on: ubuntu-latest diff --git a/.gitignore b/.gitignore index 46419a7..44dc620 100644 --- a/.gitignore +++ b/.gitignore @@ -6,6 +6,7 @@ build/ build-release/ build-coverage/ build-packaging/ +build-sanitize/ _CPack_Packages/ *.deb *.rpm diff --git a/CMakePresets.json b/CMakePresets.json index 9c73b09..c7279e7 100644 --- a/CMakePresets.json +++ b/CMakePresets.json @@ -29,6 +29,18 @@ "CMAKE_EXPORT_COMPILE_COMMANDS": "ON", "CMAKE_C_FLAGS": "--coverage -fprofile-arcs -ftest-coverage" } + }, + { + "name": "sanitize", + "displayName": "ASan+UBSan", + "binaryDir": "${sourceDir}/build-sanitize", + "cacheVariables": { + "CMAKE_BUILD_TYPE": "Debug", + "CMAKE_EXPORT_COMPILE_COMMANDS": "ON", + "CMAKE_C_FLAGS": "-fsanitize=address,undefined -fno-omit-frame-pointer -g", + "CMAKE_EXE_LINKER_FLAGS": "-fsanitize=address,undefined", + "CMAKE_SHARED_LINKER_FLAGS": "-fsanitize=address,undefined" + } } ], "buildPresets": [ @@ -43,6 +55,10 @@ { "name": "coverage", "configurePreset": "coverage" + }, + { + "name": "sanitize", + "configurePreset": "sanitize" } ], "testPresets": [ @@ -55,6 +71,11 @@ "name": "coverage", "configurePreset": "coverage", "output": { "outputOnFailure": true } + }, + { + "name": "sanitize", + "configurePreset": "sanitize", + "output": { "outputOnFailure": true } } ] } diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index eb22244..36e64ac 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -20,6 +20,13 @@ target_include_directories(dedx_objects PRIVATE "${PROJECT_SOURCE_DIR}/src" "${PROJECT_BINARY_DIR}" ) +# Warnings-as-errors on the library sources only (not tests/examples), so any new +# warning-worthy code in dedx_objects fails the build instead of silently landing. +# GCC/Clang only for now: MSVC's warning set (/W4) is a separate, noisier baseline +# that would need its own pass to clear -- see issue #149 E1. +if(CMAKE_C_COMPILER_ID MATCHES "GNU|Clang") + target_compile_options(dedx_objects PRIVATE -Wall -Wextra -Werror) +endif() # Version passed as compile definitions, derived from git tag at configure time. target_compile_definitions(dedx_objects PRIVATE DEDX_VERSION_MAJOR=${DEDX_VERSION_MAJOR} diff --git a/src/dedx.c b/src/dedx.c index 6082a25..709d785 100644 --- a/src/dedx.c +++ b/src/dedx.c @@ -62,7 +62,7 @@ static int element_supported_for_ion(int program, int ion, int element); static int material_id_supported(int program, int ion, int material); dedx_workspace *dedx_allocate_workspace(unsigned int count, int *err) { - int i = 0; + unsigned int i = 0; *err = DEDX_OK; dedx_workspace *temp = calloc(1, sizeof(dedx_workspace)); @@ -79,7 +79,7 @@ dedx_workspace *dedx_allocate_workspace(unsigned int count, int *err) { for (i = 0; i < count; i++) { temp->loaded_data[i] = calloc(1, sizeof(dedx_internal_lookup_data)); if (temp->loaded_data[i] == NULL) { /* LCOV_EXCL_START */ - int j; + unsigned int j; for (j = 0; j < i; j++) free(temp->loaded_data[j]); free((void *) temp->loaded_data); @@ -752,7 +752,7 @@ static int find_data(stopping_data *data, dedx_config *config, float *energy, in static int load_compound(dedx_workspace *ws, dedx_config *config, int *err) { int i = 0; - int j = 0; + unsigned int j = 0; int length = config->elements_length; int *targets = config->elements_id; float *weight; @@ -821,7 +821,7 @@ static int load_compound(dedx_workspace *ws, dedx_config *config, int *err) { } static int load_bethe_2(stopping_data *data, dedx_config *config, float *energy, int *err) { - int i = 0; + unsigned int i = 0; float PZ, PA, TZ, TA, rho, pot; *err = DEDX_OK; diff --git a/src/dedx_bethe.c b/src/dedx_bethe.c index 5bfb02b..4794136 100644 --- a/src/dedx_bethe.c +++ b/src/dedx_bethe.c @@ -74,6 +74,12 @@ float dedx_internal_calculate_bethe_energy( static float evaluate_bethe_model_LEext(float PT, dedx_internal_bethe_model bet, dedx_internal_bethe_gold gold, int *err) { + /* err is threaded through for interface symmetry with evaluate_bethe_model() and its + * golden-section callers, but this evaluator is pure arithmetic and never sets it + * (see #149 E3, which tracks de-duplicating the two evaluators). The cast documents + * that this is intentional rather than a bug, without changing the call sites or + * the function's signature/ABI. */ + (void) err; double T = PT; float dedx; double mass = 940 * bet.PA0; @@ -247,6 +253,9 @@ static void gold_section(dedx_internal_bethe_model bet, dedx_internal_bethe_gold } static float evaluate_bethe_model(float PT, dedx_internal_bethe_model bet, int *err) { + /* Same rationale as evaluate_bethe_model_LEext() above: int *err is never used in + * this evaluator, so it's cast to void rather than silently ignored. */ + (void) err; double T = PT; double mass = 940 * bet.PA0; diff --git a/src/dedx_validate.c b/src/dedx_validate.c index fb342cf..a7a50f8 100644 --- a/src/dedx_validate.c +++ b/src/dedx_validate.c @@ -35,7 +35,7 @@ static int dedx_internal_validate_interpolation_mode(dedx_config *config, int *e } int dedx_internal_evaluate_i_pot(dedx_config *config, int *err) { - int i; + unsigned int i; if (config->elements_i_value == NULL && config->target != 0) { if (config->i_value == 0.0) { @@ -85,7 +85,7 @@ int dedx_internal_evaluate_i_pot(dedx_config *config, int *err) { } int dedx_internal_evaluate_compound(dedx_config *config, int *err) { - int i = 0; + unsigned int i = 0; if (config->target > 0 && config->target <= 99) { *err = DEDX_OK; @@ -123,7 +123,7 @@ int dedx_internal_evaluate_compound(dedx_config *config, int *err) { } config->elements_length = compos_len; } else if (config->elements_mass_fraction == NULL && config->elements_atoms != NULL) { - int length = config->elements_length; + unsigned int length = config->elements_length; int *atoms_per_element = config->elements_atoms; float *density = malloc(sizeof(float) * length); float *weight = malloc(sizeof(float) * length); @@ -249,7 +249,7 @@ int dedx_internal_validate_state(dedx_config *config, int *err) { } int dedx_internal_calculate_element_i_pot(dedx_config *config, int *err) { - int i; + unsigned int i; float charge_avg = 0; float avg_pot = 0; float log_x, i_pot_x; diff --git a/tests/test_availability_exhaustive.c b/tests/test_availability_exhaustive.c new file mode 100644 index 0000000..f52e7d0 --- /dev/null +++ b/tests/test_availability_exhaustive.c @@ -0,0 +1,202 @@ +#include + +#include "test_helpers.h" + +/* + * Regression net for issue #149's findings A1, A4, A5 and A6 (Phase 1, item E2 of + * the plan of action there). + * + * This sweeps every (program, ion, material) triple that dedx_get_material_list_for_ion() + * advertises -- for every program except DEDX_ESTAR, which is unimplemented (see + * DEDX_ERR_ESTAR_NOT_IMPL) -- and checks three things dedx_get_material_list_for_ion()'s + * own contract implies should always hold for an advertised combination: + * + * 1. dedx_load_config() succeeds (A1, A5: it currently doesn't for 407 of them -- + * A1's DEDX_AUTO tier-mixing bug and A5's missing ferrous-oxide density row). + * 2. dedx_get_stp() accepts the program/ion pair's own advertised + * dedx_get_min_energy()/dedx_get_max_energy() bounds (A4: the bounds are + * documented as authoritative in dedx.h but are only "best-effort hints" in + * practice, and are rejected for 1568 combinations). + * 3. At least one energy sampled across that range returns a finite, positive + * stopping power (A1a: 174 DEDX_AUTO configs load with err == DEDX_OK but then + * fail at *every* energy, because the spline knots silently end at x == 0). + * + * As of this writing (before any of A1/A4/A5/A6 are fixed) this sweep reproduces the + * exact counts from the issue's manual audit: 101957 combinations swept, 407 load + * failures, 1568 bound mismatches, 174 dead-at-every-energy configs. The BASELINE_* + * constants below pin those counts so this test is a *ratchet*, not a silent no-op: + * + * - It stays green today by asserting "no worse than the known-broken baseline", + * rather than asserting "zero failures" (which would fail immediately and block + * Phase 1, whose job is only to make the failures visible, not fix them yet). + * - Phase 2 (A1, A4, A5, A6) must lower the relevant BASELINE_* constant(s) as each + * root cause is fixed, down to 0 once all four are done. Do NOT raise a baseline + * to make a newly introduced regression pass -- if this test starts failing + * because a count went *up*, that is a real regression, not a stale baseline. + * - TOTAL_COMBINATIONS is asserted with equality (not a ceiling) so a change in + * what dedx_get_material_list_for_ion() advertises -- for better or worse -- is + * always visible here, prompting a deliberate update rather than a silent drift. + */ + +#define TOTAL_COMBINATIONS 101957 +#define BASELINE_LOAD_FAILURES 407 +#define BASELINE_BOUND_MISMATCHES 1568 +#define BASELINE_DEAD_CONFIGS 174 + +/* Number of energies sampled (log-spaced) across each combination's advertised + * [min, max] range for the "dead at every energy" check. */ +#define SAMPLE_COUNT 9 + +typedef struct { + long total; + long load_failures; + long bound_mismatches; + long dead_configs; +} sweep_stats; + +/* Load one (program, ion, target) combination and update stats. Always frees what it + * allocates; never leaves a dangling workspace/config behind for the next iteration -- + * dedx_load_config() is not idempotent (see #149 C2) and there is no unload function + * yet, so each combination needs its own fresh workspace. */ +static void sweep_one(int program, int ion, int target, sweep_stats *stats) { + int err = 0; + dedx_workspace *ws; + dedx_config *cfg; + int rc; + float lo, hi; + int lo_err = 0, hi_err = 0; + int ok_samples = 0; + int i; + + stats->total++; + + ws = dedx_allocate_workspace(1, &err); + cfg = calloc(1, sizeof(dedx_config)); + cfg->program = program; + cfg->ion = ion; + cfg->target = target; + err = 0; + rc = dedx_load_config(ws, cfg, &err); + if (rc != 0 || err != DEDX_OK) { + stats->load_failures++; + dedx_free_config(cfg, &err); + dedx_free_workspace(ws, &err); + return; + } + + lo = dedx_get_min_energy(program, ion); + hi = dedx_get_max_energy(program, ion); + + dedx_get_stp(ws, cfg, lo, &lo_err); + dedx_get_stp(ws, cfg, hi, &hi_err); + if (lo_err != DEDX_OK || hi_err != DEDX_OK) + stats->bound_mismatches++; + + for (i = 0; i < SAMPLE_COUNT; i++) { + float frac = (float) i / (float) (SAMPLE_COUNT - 1); + float e; + int e_err = 0; + float v; + + /* The advertised range spans orders of magnitude (keV to tens of GeV + * depending on program/ion), so sample log-spaced when both bounds are + * positive and ordered; fall back to linear spacing for a malformed range + * rather than dividing by log(0) or log of a negative number. */ + if (lo > 0.0f && hi > lo) { + float log_lo = logf(lo); + float log_hi = logf(hi); + e = expf(log_lo + frac * (log_hi - log_lo)); + } else { + e = lo + frac * (hi - lo); + } + + v = dedx_get_stp(ws, cfg, e, &e_err); + if (e_err == DEDX_OK && isfinite(v) && v > 0.0f) + ok_samples++; + } + if (ok_samples == 0) + stats->dead_configs++; + + dedx_free_config(cfg, &err); + dedx_free_workspace(ws, &err); +} + +static void sweep_program_ion(int program, int ion, sweep_stats *stats) { + int materials[DEDX_MAX_MATERIAL_LIST]; + unsigned int materials_len = 0; + int err = 0; + unsigned int m; + + dedx_get_material_list_for_ion(program, ion, materials, DEDX_MAX_MATERIAL_LIST, &materials_len, &err); + if (err != DEDX_OK) + return; /* program/ion combination itself is invalid; nothing to sweep */ + + for (m = 0; m < materials_len; m++) { + sweep_one(program, ion, materials[m], stats); + } +} + +static int check_baseline(long got, long baseline, const char *label) { + if (got > baseline) { + fprintf(stderr, + "FAIL %s: %ld exceeds the known baseline of %ld -- this is a regression, " + "not a stale baseline; do not silence by raising the constant\n", + label, + got, + baseline); + return 1; + } + if (got < baseline) { + /* Progress! Not a failure, but flag it so whoever fixed part of A1/A4/A5/A6 + * remembers to tighten the baseline in this file as part of that change. */ + fprintf(stderr, + "NOTE %s: %ld is below the recorded baseline of %ld -- please lower " + "BASELINE_* in tests/test_availability_exhaustive.c to match\n", + label, + got, + baseline); + } + return 0; +} + +int main(void) { + const int *programs = dedx_get_program_list(); + sweep_stats stats = {0, 0, 0, 0}; + int failures = 0; + int p; + + for (p = 0; programs[p] != -1; p++) { + int program = programs[p]; + const int *ions; + int i; + + if (program == DEDX_ESTAR) + continue; + + ions = dedx_get_ion_list(program); + for (i = 0; ions[i] != -1; i++) { + sweep_program_ion(program, ions[i], &stats); + } + } + + printf("test_availability_exhaustive: total=%ld load_failures=%ld bound_mismatches=%ld dead_configs=%ld\n", + stats.total, + stats.load_failures, + stats.bound_mismatches, + stats.dead_configs); + + if (stats.total != TOTAL_COMBINATIONS) { + fprintf(stderr, + "FAIL total combinations: got %ld, expected exactly %d -- " + "dedx_get_material_list_for_ion()'s advertised set changed; " + "update TOTAL_COMBINATIONS deliberately after checking why\n", + stats.total, + TOTAL_COMBINATIONS); + failures++; + } + failures += check_baseline(stats.load_failures, BASELINE_LOAD_FAILURES, "load_failures"); + failures += check_baseline(stats.bound_mismatches, BASELINE_BOUND_MISMATCHES, "bound_mismatches"); + failures += check_baseline(stats.dead_configs, BASELINE_DEAD_CONFIGS, "dead_configs"); + + return failures; +} From 77d53dc81e65a529974b7ecba2c4fb5b40e51a31 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 16:36:11 +0000 Subject: [PATCH 2/5] ci: restrict GITHUB_TOKEN to read-only in ci.yml CodeQL's Actions analysis flags ci.yml for not declaring an explicit permissions block, leaving GITHUB_TOKEN at its default (broader) permissions for every job in the file. None of ci.yml's jobs need anything beyond reading the checkout -- no PR comments, no pushes, no releases -- so add contents: read, matching the same pattern already used in cpp-examples.yml. Note: the same gap exists in build-android.yml, build-linux-packages.yml, build-windows.yml, clang-format.yml, clang-tidy.yml, coverage.yml and docs.yml, left out of scope here since this PR only touches ci.yml. --- .github/workflows/ci.yml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1a40b4e..2b52993 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -2,6 +2,10 @@ name: CI on: [push, pull_request] +# Build/test only: the token needs no more than read access to the checkout. +permissions: + contents: read + jobs: build_and_test: runs-on: ${{ matrix.platform }} From 03b49993dec2766c7462d44406c36d15e39c75e8 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 16:43:21 +0000 Subject: [PATCH 3/5] Address Copilot review comments on #151 - tests/test_availability_exhaustive.c: sweep_one() now checks dedx_allocate_workspace()/calloc() for NULL before dereferencing ws/ cfg. An allocation failure is now a hard, unconditional test failure (stats->alloc_failures, reported and asserted separately from the A1/A4/A5/A6 baselines, which allow known counts) with a clear message, instead of a potential segfault. - .github/workflows/ci.yml: the Valgrind loop now uses `shopt -s nullglob` and asserts at least one test binary ran, so the job can no longer silently "pass" having valgrinded nothing if the glob matches no files (e.g. tests didn't get built, or the path changes). Also pulled the single hardcoded exclusion (test_availability_exhaustive) out of an inline `if` into a named, documented SKIP_VALGRIND array, so a future exemption is a one-line addition instead of a new special case in the loop body. - src/CMakeLists.txt: the GCC/Clang warnings-as-errors guard now also excludes MSVC, so it can't match clang-cl (CMAKE_C_COMPILER_ID is "Clang" there too, but it's the MSVC-compatible driver where GCC-style -Wall/-Wextra/-Werror aren't the right flags). Verified locally: full ctest suite (33/33) still green after a clean rebuild; clang-format clean on the changed .c file; the new Valgrind loop logic checked standalone against both a populated and an empty build/tests/ directory (skips exactly the one exempted binary in the first case, hard-fails with a clear message in the second). --- .github/workflows/ci.yml | 31 +++++++++++++++++++----- src/CMakeLists.txt | 6 +++-- tests/test_availability_exhaustive.c | 35 +++++++++++++++++++++++++--- 3 files changed, 61 insertions(+), 11 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2b52993..8e13756 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -65,21 +65,40 @@ jobs: - name: Valgrind full test suite run: | set -e + # Test binaries exempted from this job: still run natively and under + # ASan+UBSan (see the sanitize job), just not here. Add a name below -- + # don't special-case the loop -- if another test earns an exemption. + SKIP_VALGRIND=( + # ~102k load/query cycles: ~15s natively / ~25s under ASan+UBSan, but 15+ + # minutes under Valgrind's much heavier instrumentation, for close to zero + # incremental memory-safety coverage over what the other test_* binaries + # already give Valgrind on the same load/query code paths. + test_availability_exhaustive + ) + + # Without nullglob, an unmatched glob expands to the literal pattern string; + # the -f/-x guard below would then just skip that one non-existent "file" and + # the loop would exit 0 having valgrinded nothing. nullglob plus the counter + # check makes "no test binaries found" a hard failure instead of a silent pass. + shopt -s nullglob + ran=0 for bin in build/tests/test_*; do - # test_availability_exhaustive runs ~102k load/query cycles, which is fine - # at ~15s natively or ~25s under ASan+UBSan (see the sanitize job) but takes - # 15+ minutes under Valgrind's much heavier instrumentation, for close to - # zero incremental memory-safety coverage over what the other test_* binaries - # already give Valgrind on the same load/query code paths. Skip it here. - if [ "$(basename "$bin")" = "test_availability_exhaustive" ]; then + name="$(basename "$bin")" + if printf '%s\n' "${SKIP_VALGRIND[@]}" | grep -qx "$name"; then continue fi if [ -x "$bin" ] && [ -f "$bin" ]; then echo "::group::valgrind $bin" valgrind --leak-check=full --track-origins=yes --error-exitcode=1 "$bin" echo "::endgroup::" + ran=$((ran + 1)) fi done + if [ "$ran" -eq 0 ]; then + echo "::error::no test_* binaries found under build/tests/ -- Valgrind ran nothing" + exit 1 + fi + echo "Valgrind ran $ran test binaries (skipped: ${SKIP_VALGRIND[*]})" sanitize: runs-on: ubuntu-latest diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 36e64ac..9720d4c 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -23,8 +23,10 @@ target_include_directories(dedx_objects PRIVATE # Warnings-as-errors on the library sources only (not tests/examples), so any new # warning-worthy code in dedx_objects fails the build instead of silently landing. # GCC/Clang only for now: MSVC's warning set (/W4) is a separate, noisier baseline -# that would need its own pass to clear -- see issue #149 E1. -if(CMAKE_C_COMPILER_ID MATCHES "GNU|Clang") +# that would need its own pass to clear -- see issue #149 E1. NOT MSVC also excludes +# clang-cl (CMAKE_C_COMPILER_ID is "Clang" there too, but it's the MSVC-compatible +# driver, so GCC-style -Wall/-Wextra/-Werror aren't the right flags for it either). +if(CMAKE_C_COMPILER_ID MATCHES "GNU|Clang" AND NOT MSVC) target_compile_options(dedx_objects PRIVATE -Wall -Wextra -Werror) endif() # Version passed as compile definitions, derived from git tag at configure time. diff --git a/tests/test_availability_exhaustive.c b/tests/test_availability_exhaustive.c index f52e7d0..8e89589 100644 --- a/tests/test_availability_exhaustive.c +++ b/tests/test_availability_exhaustive.c @@ -52,6 +52,7 @@ typedef struct { long load_failures; long bound_mismatches; long dead_configs; + long alloc_failures; } sweep_stats; /* Load one (program, ion, target) combination and update stats. Always frees what it @@ -72,6 +73,24 @@ static void sweep_one(int program, int ion, int target, sweep_stats *stats) { ws = dedx_allocate_workspace(1, &err); cfg = calloc(1, sizeof(dedx_config)); + if (ws == NULL || cfg == NULL) { + /* Hard test failure, not a swept-combination failure: an OOM here means the + * sweep itself can no longer be trusted, so report it clearly instead of + * dereferencing a NULL workspace/config below. dedx_free_config()/ + * dedx_free_workspace() are both NULL-safe, so this cleanup is safe even + * when only one of the two allocations failed. */ + fprintf(stderr, + "FAIL sweep_one: allocation failed (workspace=%p config=%p) for program=%d ion=%d target=%d\n", + (void *) ws, + (void *) cfg, + program, + ion, + target); + stats->alloc_failures++; + dedx_free_config(cfg, &err); + dedx_free_workspace(ws, &err); + return; + } cfg->program = program; cfg->ion = ion; cfg->target = target; @@ -161,7 +180,7 @@ static int check_baseline(long got, long baseline, const char *label) { int main(void) { const int *programs = dedx_get_program_list(); - sweep_stats stats = {0, 0, 0, 0}; + sweep_stats stats = {0, 0, 0, 0, 0}; int failures = 0; int p; @@ -179,11 +198,21 @@ int main(void) { } } - printf("test_availability_exhaustive: total=%ld load_failures=%ld bound_mismatches=%ld dead_configs=%ld\n", + printf("test_availability_exhaustive: total=%ld load_failures=%ld bound_mismatches=%ld dead_configs=%ld " + "alloc_failures=%ld\n", stats.total, stats.load_failures, stats.bound_mismatches, - stats.dead_configs); + stats.dead_configs, + stats.alloc_failures); + + /* Unlike the baselines below, any allocation failure is an unconditional hard + * failure -- there is no acceptable count of "the sweep couldn't get memory". */ + if (stats.alloc_failures > 0) { + fprintf( + stderr, "FAIL: %ld combination(s) hit an allocation failure; see FAIL lines above\n", stats.alloc_failures); + failures++; + } if (stats.total != TOTAL_COMBINATIONS) { fprintf(stderr, From 8976400e821a1ce4135ff9c77262cd1a7b01e24b Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 17:28:15 +0000 Subject: [PATCH 4/5] ci: extract the Valgrind test-suite loop into a standalone script Move the SKIP_VALGRIND/nullglob/ran-counter logic added in the previous commit out of ci.yml's inline `run:` block and into .github/scripts/run_valgrind_suite.sh. Reasons: - It's now runnable and debuggable locally against a real build directory (.github/scripts/run_valgrind_suite.sh build/tests), rather than only via a CI round-trip. - YAML multi-line `run:` blocks don't get shell syntax checking, linting, or a shebang; a real .sh file does. - The ci.yml step shrinks to a single line, which is what "run the Valgrind suite" should look like at the workflow-orchestration level -- the how belongs in the script. No behavioural change: same SKIP_VALGRIND array (still just test_availability_exhaustive today), same nullglob guard, same ran-count hard-failure, same valgrind flags. Verified locally: the script run directly against build/tests valgrinds all 20 eligible binaries and skips test_availability_exhaustive (exit 0); against an empty directory it fails clearly (exit 1); with no argument it prints usage (exit 2). Full ctest suite (33/33) still green. --- .github/scripts/run_valgrind_suite.sh | 53 +++++++++++++++++++++++++++ .github/workflows/ci.yml | 37 +------------------ 2 files changed, 54 insertions(+), 36 deletions(-) create mode 100755 .github/scripts/run_valgrind_suite.sh diff --git a/.github/scripts/run_valgrind_suite.sh b/.github/scripts/run_valgrind_suite.sh new file mode 100755 index 0000000..62bbebf --- /dev/null +++ b/.github/scripts/run_valgrind_suite.sh @@ -0,0 +1,53 @@ +#!/usr/bin/env bash +# Runs every test_* binary in the given directory under Valgrind, except the ones +# listed in SKIP_VALGRIND below. Used by the `valgrind` job in +# .github/workflows/ci.yml; also safe to run locally against a build directory, +# e.g. after `cmake -S . -B build && cmake --build build`: +# +# .github/scripts/run_valgrind_suite.sh build/tests +# +set -euo pipefail + +if [ "$#" -ne 1 ]; then + echo "usage: $0 " >&2 + exit 2 +fi + +test_dir="$1" + +# Test binaries exempted from this script: they still run natively and under +# ASan+UBSan (see the `sanitize` CI job), just not here. Add a name below -- +# don't special-case the loop below -- if another test earns an exemption. +SKIP_VALGRIND=( + # ~102k load/query cycles: ~15s natively / ~25s under ASan+UBSan, but 15+ + # minutes under Valgrind's much heavier instrumentation, for close to zero + # incremental memory-safety coverage over what the other test_* binaries + # already give Valgrind on the same load/query code paths. + test_availability_exhaustive +) + +# Without nullglob, an unmatched glob expands to the literal pattern string; the +# -f/-x guard below would then just skip that one non-existent "file" and the loop +# would exit 0 having valgrinded nothing. nullglob plus the counter check below +# makes "no test binaries found" a hard failure instead of a silent pass. +shopt -s nullglob + +ran=0 +for bin in "$test_dir"/test_*; do + name="$(basename "$bin")" + if printf '%s\n' "${SKIP_VALGRIND[@]}" | grep -qx "$name"; then + continue + fi + if [ -x "$bin" ] && [ -f "$bin" ]; then + echo "::group::valgrind $bin" + valgrind --leak-check=full --track-origins=yes --error-exitcode=1 "$bin" + echo "::endgroup::" + ran=$((ran + 1)) + fi +done + +if [ "$ran" -eq 0 ]; then + echo "::error::no test_* binaries found under $test_dir -- Valgrind ran nothing" >&2 + exit 1 +fi +echo "Valgrind ran $ran test binaries (skipped: ${SKIP_VALGRIND[*]})" diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 8e13756..0c2e79c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -63,42 +63,7 @@ jobs: sudo apt-get install -y valgrind - name: Valgrind full test suite - run: | - set -e - # Test binaries exempted from this job: still run natively and under - # ASan+UBSan (see the sanitize job), just not here. Add a name below -- - # don't special-case the loop -- if another test earns an exemption. - SKIP_VALGRIND=( - # ~102k load/query cycles: ~15s natively / ~25s under ASan+UBSan, but 15+ - # minutes under Valgrind's much heavier instrumentation, for close to zero - # incremental memory-safety coverage over what the other test_* binaries - # already give Valgrind on the same load/query code paths. - test_availability_exhaustive - ) - - # Without nullglob, an unmatched glob expands to the literal pattern string; - # the -f/-x guard below would then just skip that one non-existent "file" and - # the loop would exit 0 having valgrinded nothing. nullglob plus the counter - # check makes "no test binaries found" a hard failure instead of a silent pass. - shopt -s nullglob - ran=0 - for bin in build/tests/test_*; do - name="$(basename "$bin")" - if printf '%s\n' "${SKIP_VALGRIND[@]}" | grep -qx "$name"; then - continue - fi - if [ -x "$bin" ] && [ -f "$bin" ]; then - echo "::group::valgrind $bin" - valgrind --leak-check=full --track-origins=yes --error-exitcode=1 "$bin" - echo "::endgroup::" - ran=$((ran + 1)) - fi - done - if [ "$ran" -eq 0 ]; then - echo "::error::no test_* binaries found under build/tests/ -- Valgrind ran nothing" - exit 1 - fi - echo "Valgrind ran $ran test binaries (skipped: ${SKIP_VALGRIND[*]})" + run: .github/scripts/run_valgrind_suite.sh build/tests sanitize: runs-on: ubuntu-latest From da3b5ea6d3c5d8e201452368314de87f77185973 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 17:59:44 +0000 Subject: [PATCH 5/5] cmake: gate -Werror behind an opt-in DEDX_WERROR option Addresses a Copilot review suggestion on 8976400 (suppressed as optional/non-blocking, but worth taking): -Werror was unconditional on the dedx_objects target, so *every* source build -- Release, the .deb/.rpm packaging jobs, and any downstream/distro build -- would hard-fail the moment a newer or different GCC/Clang introduced a new -Wall/-Wextra diagnostic in code that hasn't actually regressed. This also better matches what issue #149's E1 finding actually recommended: "-Wall -Wextra ... on the library target, -Werror in CI" -- two different scopes, which the previous commit conflated. - New option(DEDX_WERROR "Treat compiler warnings as errors (GCC/Clang only)" OFF) in the top-level CMakeLists.txt, OFF by default. - src/CMakeLists.txt: -Wall/-Wextra stay unconditional (GCC/Clang, not MSVC); -Werror is now added only when DEDX_WERROR is ON. - ci.yml: all three jobs that build the library from source (build_and_test, valgrind, python_tests) now configure with -DDEDX_WERROR=ON, so CI keeps full enforcement. - CMakePresets.json: the sanitize and coverage presets (both CI-only -- build-sanitize/build-coverage aren't release artifacts) also set DEDX_WERROR: ON. debug/release presets are left at the OFF default, since those are for local development and don't want to surprise a developer on a different compiler version. Verified locally: -DDEDX_WERROR=ON reproduces the previous behaviour exactly (compile_commands.json shows -Wall -Wextra -Werror all present); the default configure (no flag) shows -Wall -Wextra present but -Werror genuinely absent; both the sanitize and coverage presets build clean under DEDX_WERROR=ON; full ctest suite (33/33) still green with -DDEDX_WERROR=ON, matching how CI now configures. --- .github/workflows/ci.yml | 6 +++--- CMakeLists.txt | 5 +++++ CMakePresets.json | 6 ++++-- src/CMakeLists.txt | 22 +++++++++++++++------- 4 files changed, 27 insertions(+), 12 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0c2e79c..08c3a94 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -20,7 +20,7 @@ jobs: run: cmake --version - name: Configure - run: cmake -S . -B build + run: cmake -S . -B build -DDEDX_WERROR=ON - name: Build run: cmake --build build --parallel @@ -52,7 +52,7 @@ jobs: uses: actions/checkout@v7 - name: Configure - run: cmake -S . -B build + run: cmake -S . -B build -DDEDX_WERROR=ON - name: Build run: cmake --build build --parallel @@ -100,7 +100,7 @@ jobs: python-version: ${{ matrix.python-version }} - name: Configure - run: cmake -S . -B build + run: cmake -S . -B build -DDEDX_WERROR=ON - name: Build run: cmake --build build --parallel diff --git a/CMakeLists.txt b/CMakeLists.txt index 0063f62..de2c90b 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -12,6 +12,11 @@ include(CMakePackageConfigHelpers) option(DEDX_BUILD_EXAMPLES "Build libdedx example programs" ON) option(DEDX_BUILD_TESTS "Build libdedx test suite" ON) +# -Wall/-Wextra apply unconditionally (GCC/Clang) below, but -Werror is opt-in and +# OFF by default: a release/packaging/downstream source build compiled with a newer +# or different GCC/Clang than CI used could otherwise start failing on a compiler +# diagnostic that's new, not a regression in this code. CI turns this ON explicitly. +option(DEDX_WERROR "Treat compiler warnings as errors (GCC/Clang only)" OFF) # ---- Version from git tag ---- find_package(Git QUIET) diff --git a/CMakePresets.json b/CMakePresets.json index c7279e7..4c0b27d 100644 --- a/CMakePresets.json +++ b/CMakePresets.json @@ -27,7 +27,8 @@ "cacheVariables": { "CMAKE_BUILD_TYPE": "Debug", "CMAKE_EXPORT_COMPILE_COMMANDS": "ON", - "CMAKE_C_FLAGS": "--coverage -fprofile-arcs -ftest-coverage" + "CMAKE_C_FLAGS": "--coverage -fprofile-arcs -ftest-coverage", + "DEDX_WERROR": "ON" } }, { @@ -39,7 +40,8 @@ "CMAKE_EXPORT_COMPILE_COMMANDS": "ON", "CMAKE_C_FLAGS": "-fsanitize=address,undefined -fno-omit-frame-pointer -g", "CMAKE_EXE_LINKER_FLAGS": "-fsanitize=address,undefined", - "CMAKE_SHARED_LINKER_FLAGS": "-fsanitize=address,undefined" + "CMAKE_SHARED_LINKER_FLAGS": "-fsanitize=address,undefined", + "DEDX_WERROR": "ON" } } ], diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 9720d4c..76c7da9 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -20,14 +20,22 @@ target_include_directories(dedx_objects PRIVATE "${PROJECT_SOURCE_DIR}/src" "${PROJECT_BINARY_DIR}" ) -# Warnings-as-errors on the library sources only (not tests/examples), so any new -# warning-worthy code in dedx_objects fails the build instead of silently landing. -# GCC/Clang only for now: MSVC's warning set (/W4) is a separate, noisier baseline -# that would need its own pass to clear -- see issue #149 E1. NOT MSVC also excludes -# clang-cl (CMAKE_C_COMPILER_ID is "Clang" there too, but it's the MSVC-compatible -# driver, so GCC-style -Wall/-Wextra/-Werror aren't the right flags for it either). +# Warnings on the library sources only (not tests/examples). GCC/Clang only for now: +# MSVC's warning set (/W4) is a separate, noisier baseline that would need its own +# pass to clear -- see issue #149 E1. NOT MSVC also excludes clang-cl +# (CMAKE_C_COMPILER_ID is "Clang" there too, but it's the MSVC-compatible driver, so +# GCC-style -Wall/-Wextra/-Werror aren't the right flags for it either). +# +# -Werror is gated behind DEDX_WERROR (see its definition in the top-level +# CMakeLists.txt for why it isn't unconditional): CI passes -DDEDX_WERROR=ON so any +# new warning-worthy code fails the build there, without also making a release, +# packaging, or downstream source build fail on a compiler diagnostic that's merely +# new to whatever GCC/Clang version that build happens to use. if(CMAKE_C_COMPILER_ID MATCHES "GNU|Clang" AND NOT MSVC) - target_compile_options(dedx_objects PRIVATE -Wall -Wextra -Werror) + target_compile_options(dedx_objects PRIVATE -Wall -Wextra) + if(DEDX_WERROR) + target_compile_options(dedx_objects PRIVATE -Werror) + endif() endif() # Version passed as compile definitions, derived from git tag at configure time. target_compile_definitions(dedx_objects PRIVATE