ci: add libstdc++ hardened-assertions leg to agent-check.yml - #133
Merged
Conversation
Builds with PKG_CPPFLAGS=-D_GLIBCXX_ASSERTIONS and runs the full test suite, catching container-bounds address-formation bugs (#51) that plain builds, R CMD check, Valgrind and ASan's own instrumentation all miss. Landed blocking: a build of current cpp-search under the flag plus the full testthat suite ran clean locally. Fixes #60
review found the tee pipe swallowed R CMD INSTALL's exit code without an explicit bash shell (no pipefail), and the leg shared ubuntu's cache key, so a hardened build could silently overwrite or be overwritten by the unhardened one. Also cover tier-3 tests and fail the flag-count check on an unreadable log rather than erroring past it.
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.
Fixes #60
What
Adds a
glibcxx-assertionsjob toagent-check.ymlthat builds TreeSearch withPKG_CPPFLAGS=-D_GLIBCXX_ASSERTIONSand runs the full test suite. This catches container out-of-bounds address formation (e.g.vec[n]withn == vec.size(), no load or store) — the bug class from #51, which is invisible to plain builds,R CMD check, Valgrind, and ASan's own instrumentation (it watches accesses, not address arithmetic).Blocking vs advisory — decided empirically
Built current
origin/cpp-searchlocally with-D_GLIBCXX_ASSERTIONSand ran the fulltestthatsuite (test_dir(..., load_package = 'installed'), mirroring howR CMD checkruns tests, since a naivetest_dir()/attach-only invocation hides internal.-prefixed test helpers and produces false failures — caught that trap before trusting the first run). Result: clean,EXIT=0, no assertion failures — only the usual extended-test skips andMaddisonSlatkinbudget warnings, both pre-existing and unrelated to the flag. Landed blocking.Flag placement, verified
PKG_CPPFLAGS, notPKG_CXXFLAGS— a~/.R/Makevarscan zero the latter (confirmed on the maintainer's own dev machine, which has exactly this). Verified the flag reaches the compiler two ways:grep -c ... -D_GLIBCXX_ASSERTIONS,exit 1on zero).libstdc++ hardened assertions): 34 compiler invocations carry-D_GLIBCXX_ASSERTIONS, e.g.Where
Added to
agent-check.yml(workflow_dispatch-only, what agents already dispatch constantly) rather than a new workflow — fastest feedback, no new container/runner overhead. Runs independently of the existingubuntu/windowsjobs (noneeds:) so it doesn't slow down or get gated by them.Review round
An
external-reviewerpass on the workflow diff found two real issues in the first version, both fixed here:R CMD INSTALL ... | teepipe swallowed a genuine install failure because the step had no explicitshell: bash(GHA's stepless default lackspipefail) — addedshell: bashto allrun:steps.ubuntu's dependency-cache key, so a hardened install could silently overwrite (or be overwritten by) the unhardened one across runs — bumped this job to its owncache-version.Also: extended (tier-3) tests weren't reachable via
TREESEARCH_EXTENDED_TESTS, and the flag-count check errored past a missing log instead of failing closed — both fixed.Re-dispatched after the fixes (run 31018253102) — all three jobs green, leg log again shows 34 flagged compiler invocations.
Verification
cpp-searchtip.No NEWS.md entry — CI-only change, not user-visible.