ci: make the sanitizer legs able to fail, and the install able to be consumed - #555
Merged
Merged
Conversation
…consumed Three gates that reported success while checking nothing (morph#540, #541, #542). Each fix ships with a check that fails without it. morph#541 -- UBSan recovers by default: it prints the diagnostic and lets the process exit 0, so a leg judged by exit status learns nothing. Measured: a TU constructing `Rational{INT64_MIN, DecimalPlaces{2}}` printed three `runtime error: negation of -9223372036854775808` lines on the clang-ubsan leg and exited 0 -- which is how morph#537 survived a green sanitizer job. `-fno-sanitize-recover=undefined` goes into `apply_sanitizers()` rather than a per-job UBSAN_OPTIONS, because the asan arm is `-fsanitize=address,undefined` and carried recovering UBSan too; my first run of the new check confirmed both legs were blind. One place, both legs, no job can forget it. morph#542 -- `morph_offline_sqlite_tests` and `morph_concepts_tests` compiled with zero sanitizer flags and ran on every sanitizer leg. The first is the sharp one: the clang-asan leg turns MORPH_BUILD_OFFLINE_SQLITE on precisely because the SQLite queue is "where the memory/threading/UB risk actually lives -- a C API", then checked none of it. The existing `nm` assertion could not catch either: it covered only the ladder's binaries and only `__asan_`, which is vacuous on the tsan and ubsan legs. morph#540 -- `forms/detail/session_common.hpp` was added with morph#515 and never listed in the install FILE_SET, so an installed `morph/forms/app.hpp`, `flows.hpp` and `sections.hpp` could not be compiled at all while `cmake --install` exited 0 and three gates stayed green. The checks, and why each is behavioural rather than a grep: - scripts/check_sanitizer_can_fail.sh drives `apply_sanitizers()` and requires a program with real undefined behaviour to exit non-zero. A grep for the flag would keep passing if the flag stopped reaching the compile line -- the failure morph#298 already produced once here. - scripts/check_sanitizer_instrumentation.sh walks the binaries ctest will actually run, keyed on the preset's own symbol, with a floor so a run that examined nothing fails rather than passes. A hand-kept target list is what let two suites be added without anyone noticing they were never covered. - check_install_export.sh's consumer TU is now generated from the prefix -- all 49 installed public headers -- instead of hand-listing four. The hand-list was the bug. `detail/` stays excluded deliberately: `morph/detail/quantity_equation.hpp` is not self-contained by design, so globbing it would fail on a file working as intended. Verified: both new checks fail on the unfixed tree for the stated reason and pass after; the full suite is 1466/1466 under non-recovering UBSan with both newly instrumented suites running; an asan configure shows 8/8 and 1/1 TUs now carrying -fsanitize where they previously carried none; check_install_export.sh is green end to end. Not folded in, per the triage of morph#541: `apply_sanitizers()` silently ignores an unrecognised `mode`, so `-DAF_SANITIZER=ASAN` configures cleanly with zero sanitizer flags. Verified separately, filed on its own rather than widened into this change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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.
Closes #540, #541, #542 — three gates that reported success while checking nothing. Each fix ships with a check that fails without it.
What was broken
#541 — the sanitizer legs could not fail. UndefinedBehaviorSanitizer recovers by default: it prints the diagnostic and lets the process exit 0, so a leg judged by exit status learns nothing. Measured before the fix: a TU constructing
Rational{INT64_MIN, DecimalPlaces{2}}printed threeruntime error: negation of -9223372036854775808lines on theclang-ubsanleg and exited 0. That is how #537's defect survived a sanitizer job that was green throughout.#542 — two suites were never instrumented.
morph_offline_sqlite_tests(1 TU) andmorph_concepts_tests(8 TUs) compiled with zero sanitizer flags and ran on every sanitizer leg. The first is the sharp one: theclang-asanleg setsMORPH_BUILD_OFFLINE_SQLITE=ONprecisely because the SQLite queue is "where the memory/threading/UB risk actually lives — a C API", then checked none of it.#540 — the install could not be consumed.
forms/detail/session_common.hppwas added with #515 and never listed in the installFILE_SET, so an installedmorph/forms/app.hpp,flows.hppandsections.hppcould not be compiled at all — whilecmake --installexited 0 and three separate gates stayed green.Why each check is behavioural, not a grep
scripts/check_sanitizer_can_fail.shdrivesapply_sanitizers()itself and requires a program with real undefined behaviour to exit non-zero. A grep for the flag would keep passing if the flag stopped reaching the compile line — the failure cmake: -Weverything is gated on CXX_COMPILER_ID:Clang, so it never applies on macOS (AppleClang) #298 already produced once here.scripts/check_sanitizer_instrumentation.shwalks the binaries ctest will actually run, keyed on the preset's own symbol (__asan_/__tsan_/__ubsan_), with a floor so a run that examined nothing fails rather than passes quietly. The existing assertion covered only the ladder's binaries and only__asan_, which is vacuous on the tsan and ubsan legs — the same "control that measures nothing" it exists to prevent. A hand-kept target list is what let two suites be added unnoticed.check_install_export.sh's consumer TU is now generated from the prefix — all 49 installed public headers — instead of hand-listing four. The hand-list was the bug.detail/stays excluded deliberately:morph/detail/quantity_equation.hppis not self-contained by design, so globbing it would fail on a file working as intended.Placement
-fno-sanitize-recover=undefinedgoes inapply_sanitizers()rather than a per-jobUBSAN_OPTIONS, because the asan arm is-fsanitize=address,undefinedand carried recovering UBSan too — the first run of the new check confirmed both legs were blind. One place, both legs, no future job can forget it.Verification
morph_concepts_tests8/8 andmorph_offline_sqlite_tests1/1 TUs now carrying-fsanitize, where both previously carried none.check_install_export.shgreen end to end.While writing the instrumentation check I hit its own trap:
grep -qcloses the pipe early,nmtakes SIGPIPE, andset -o pipefailturned a match into a failure — so the first version reported every instrumented binary as uninstrumented. Caught by running it against a known-good build before trusting it; the fix and the reason are recorded in the script.Not folded in
apply_sanitizers()silently ignores an unrecognisedmode, so-DAF_SANITIZER=ASAN(wrong case) configures cleanly with zero sanitizer flags across the whole build. Verified separately during triage. It is an independent defect and gets its own issue rather than widening this change.🤖 Generated with Claude Code