Phase 1 of #149: make the failures visible - #151
Merged
Conversation
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.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #151 +/- ##
==========================================
+ Coverage 78.22% 85.32% +7.09%
==========================================
Files 12 12
Lines 1649 1649
Branches 320 320
==========================================
+ Hits 1290 1407 +117
+ Misses 359 242 -117 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
Contributor
There was a problem hiding this comment.
Pull request overview
Implements Phase 1 of #149 (“make the failures visible”) by tightening compiler warnings on the core library, adding CI sanitization coverage, and introducing an exhaustive availability regression test that ratchets failure counts against a known baseline.
Changes:
- Enable warnings-as-errors for the
dedx_objectslibrary target and resolve newly surfaced sign-compare/unused-parameter warnings. - Add an exhaustive
(program, ion, material)availability sweep test with baseline (“ratchet”) thresholds. - Add an ASan+UBSan CMake preset and CI job; expand Valgrind coverage to run all
test_*binaries (with a documented skip).
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
tests/test_availability_exhaustive.c |
New exhaustive availability “ratchet” regression test for #149 findings. |
src/dedx.c |
Widen loop counters to clear sign-compare warnings under -Werror. |
src/dedx_validate.c |
Widen loop counters / lengths to clear sign-compare warnings under -Werror. |
src/dedx_bethe.c |
Document intentionally-unused err parameter to satisfy -Wunused-parameter. |
src/CMakeLists.txt |
Add -Wall -Wextra -Werror for the library object target (GCC/Clang). |
CMakePresets.json |
Add sanitize preset and build/test presets. |
.gitignore |
Ignore build-sanitize/. |
.github/workflows/ci.yml |
Expand Valgrind test coverage and add sanitize CI job. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- 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).
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.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/CMakeLists.txt:31
- Optional / non-blocking:
-Werroris added unconditionally to thededx_objectstarget, so it applies to every build from source — Release, packaging (.deb/.rpm), and downstream/distro builds — not just CI. A future GCC/Clang/AppleClang release that introduces a new-Wall/-Wextradiagnostic would then turn a mere warning into a hard build failure for anyone simply trying to compile the library, even though the code itself is unchanged. A common way to keep the "fail on new warnings" benefit without the fragility is to gate-Werrorbehind an option (e.g.option(DEDX_WERROR "Treat warnings as errors" OFF)) that CI turns on with-DDEDX_WERROR=ON, leaving-Wall -Wextraon for everyone but-Werroroff by default for released source builds.
if(CMAKE_C_COMPILER_ID MATCHES "GNU|Clang" AND NOT MSVC)
target_compile_options(dedx_objects PRIVATE -Wall -Wextra -Werror)
endif()
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements Phase 1 — "make the failures visible" of the plan of action in #149, building on Phase 0 (#150, merged). This phase adds tooling and a regression test that surface the bugs Phase 2 needs to fix — it does not fix any of them yet, so the counts below are expected to be nonzero today.
Changes
E1 — warnings-as-errors on the library target
-Wall -Wextra -Werrorto thededx_objectstarget (GCC/Clang only — MSVC's/W4baseline is a separate, noisier pass I can't verify in this environment, left for later).-Wsign-comparewarnings this immediately surfaced, by widening loop counters that are only ever compared against unsigned bounds (elements_length,stopping_data.length, etc.) tounsigned int. No behaviour change — every widened counter starts at 0 and only ever counts up against an unsigned bound it was already logically bounded by.-Wunused-parameterwarnings in the Bethe evaluators (evaluate_bethe_model()/evaluate_bethe_model_LEext()) by casting the genuinely-unusederrparameter tovoid— both evaluators are pure arithmetic and never set it. That parameter's fate (drop it vs. wire it up) is tracked separately in Deep audit: DEDX_AUTO returns garbage/unusable results for 174 materials, element-boundary off-by-one, 2 API-reachable memory-safety bugs, plus API/doc inconsistencies — with a plan of action #149's finding E3, which also covers de-duplicating the two evaluators; this PR just documents why it's unused today rather than removing it.E2 — CI and the exhaustive availability test
Added a
sanitizeCMake preset (-fsanitize=address,undefined) and a newsanitizeCI job that builds and runs the fullctestsuite under it.Extended the Valgrind job from one test binary (
test_bethe_ext00) to everytest_*binary intests/, with one deliberate, documented exception (see below).Added
tests/test_availability_exhaustive.c: sweeps every(program, ion, material)triplededx_get_material_list_for_ion()advertises (skippingDEDX_ESTAR, which is unimplemented — same scope as the issue's manual audit), and for each one checks:dedx_load_config()succeeds,dedx_get_min_energy()/dedx_get_max_energy()bounds are accepted bydedx_get_stp(),This reproduces the exact counts from Deep audit: DEDX_AUTO returns garbage/unusable results for 174 materials, element-boundary off-by-one, 2 API-reachable memory-safety bugs, plus API/doc inconsistencies — with a plan of action #149's manual audit: 101957 combinations swept, 407 load failures, 1568 bound mismatches, 174 configs that load with
err == DEDX_OKbut returnENERGY_OUT_OF_RANGEat every energy — the regression net for findings A1, A4, A5, A6.Since Phase 2 hasn't fixed those yet, the test asserts each count stays at or below that known baseline rather than at zero — a ratchet, not a pass/fail gate on perfection. It stays green today and turns red only if a future change makes something worse. Phase 2 PRs should lower the relevant
BASELINE_*constant(s) as each root cause gets fixed, down to 0 once A1/A4/A5/A6 are all done — the test prints aNOTE(not a failure) if the live count ever comes in under the recorded baseline, as a nudge to do that.Why
test_availability_exhaustiveis skipped in the Valgrind jobAt ~102k load/query cycles it runs in ~15s natively and ~25s under ASan+UBSan (see the new
sanitizejob), but takes 15+ minutes under Valgrind's much heavier instrumentation — for close to zero incremental memory-safety coverage over what the other 20test_*binaries already give Valgrind on the exact samededx_load_config()/dedx_get_stp()code paths, just exercised far more times. It's excluded from the Valgrind loop by name, with a comment explaining why, while staying in the plain andsanitizesuites.Verification
Locally, before pushing:
ctestsuite (33/33) green in a plain build.ctestsuite green with-Wall -Wextra -Werrorwired into the actual build (confirmed viacompile_commands.json, and via a throwaway warning that failed the build as expected before being reverted).ctestsuite (33/33) green under-fsanitize=address,undefinedvia the newsanitizepreset.test_*binary excepttest_availability_exhaustivepasses undervalgrind --leak-check=full --track-origins=yes.clang-format --dry-run --Werrorandclang-tidy(bugprone-/clang-analyzer-, warnings-as-errors) clean on every changed/added file.test_availability_exhaustivestandalone reproduces the audit's numbers exactly:total=101957 load_failures=407 bound_mismatches=1568 dead_configs=174.Scope
Deliberately excludes Phase 2 onward (the actual A1/A2/A5/A6/etc. correctness fixes, API contract changes, architecture work) — this PR is tooling and a test, not a behaviour change, matching the issue's own phase sequencing ("each phase leaves the tree green and the next phase cheaper").
Closes nothing on its own; #149 stays open for the remaining phases.
Generated by Claude Code