Phase 1 of #149: make the failures visible

.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 <test-binary-directory>" >&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[*]})"

.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

.gitignore

@@ -6,6 +6,7 @@ build/
 build-release/
 build-coverage/
 build-packaging/
+build-sanitize/
 _CPack_Packages/
 *.deb
 *.rpm

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)

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 }
         }
     ]
 }

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}

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;

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;
 

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;