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 01ca4ae..08c3a94 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 }} @@ -16,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 @@ -48,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 @@ -58,10 +62,27 @@ jobs: sudo apt-get update sudo apt-get install -y valgrind - - name: Valgrind test_bethe_ext00 - run: | - valgrind --leak-check=full --track-origins=yes --error-exitcode=1 \ - ./build/tests/test_bethe_ext00 + - name: Valgrind full test suite + run: .github/scripts/run_valgrind_suite.sh build/tests + + 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 @@ -79,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/.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/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 9c73b09..4c0b27d 100644 --- a/CMakePresets.json +++ b/CMakePresets.json @@ -27,7 +27,21 @@ "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" + } + }, + { + "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", + "DEDX_WERROR": "ON" } } ], @@ -43,6 +57,10 @@ { "name": "coverage", "configurePreset": "coverage" + }, + { + "name": "sanitize", + "configurePreset": "sanitize" } ], "testPresets": [ @@ -55,6 +73,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..76c7da9 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -20,6 +20,23 @@ target_include_directories(dedx_objects PRIVATE "${PROJECT_SOURCE_DIR}/src" "${PROJECT_BINARY_DIR}" ) +# 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) + 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 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..8e89589 --- /dev/null +++ b/tests/test_availability_exhaustive.c @@ -0,0 +1,231 @@ +#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; + long alloc_failures; +} 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)); + 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; + 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, 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 " + "alloc_failures=%ld\n", + stats.total, + stats.load_failures, + stats.bound_mismatches, + 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, + "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; +}