Add a valgrind CI leg - #201
Merged
Merged
Conversation
The one class of bug the sanitizer legs cannot see: a read of memory that was never written. ASan reports out-of-bounds and use-after-free, UBSan reports the language-level UB -- it is what catches the memset(nullptr) guard in clear_buckets() -- and neither says a word about an uninitialised read. That is MemorySanitizer's job, and MSan needs the whole standard library rebuilt instrumented before it is usable, which is why it is not run here or almost anywhere. Valgrind gets the same detection for nothing. It is worth having for this container in particular. The buckets are memset rather than constructed, and m_shifts, m_bucket_mask and m_max_bucket_capacity are written by hand in an order chosen so that nothing describing the bucket array is published before the array exists -- see allocate_buckets_from_shift. "Has that field been written yet" is a real question to be able to ask of it. No sharding, because it does not need any: the suite takes 20 seconds under valgrind at --buildtype=debugoptimized against just over two minutes at -O0. The first measurement was of a debug build and made this look six times more expensive than it is. The timeout multiplier is there because meson's default is 30 seconds per test and this project has one test containing all 599 cases. Verified both ways, which for a new leg is the half that matters: it passes clean today, and with an uninitialised read planted behind a noinline call -- opaque enough that -Werror=uninitialized does not catch it first, which it does for the obvious spelling -- valgrind reports "Conditional jump or move depends on uninitialised value(s)" and the leg goes red. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Unsharded it was the slowest job in the whole matrix -- 145s against 99s for the next one -- so it set the wall clock for all of CI by itself. My "no sharding needed" was measured here, where the suite takes 20s under valgrind; the runner takes 113s for the same work, and that is the number that decides it. Replaying the fuzz corpora is 73% of the run and fuzz_api alone is half of that, so the split is that one case against everything else: 9s and 12s rather than 20s. That is the best two-way split there is, because a single test case cannot be divided and fuzz_api is therefore the floor however many shards are used -- which is also why this stops at two rather than the four that were on offer. Written as one name and its complement, so it stays correct without being maintained: a fuzz target added later lands in the second shard rather than in neither. Verified the two halves are 1 and 598 of 599 cases, and that both pass under the exact command the workflow runs. Note for anyone who reaches for it: `meson test --slice` cannot split this, because the project has one test() containing all 599 cases. The split has to be a doctest filter, which is what --test-args carries. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
You were right that the testing is a small part of the job, and I had the diagnosis backwards. `meson test` builds before it tests, so the single step is compile plus run, and a brand-new leg starts with an empty ccache because the key is the matrix id. The 145s I read as "the valgrind run is slow" was mostly a cold compile of all ~90 translation units. Measured rather than reasoned, by shipping the split and looking: the shard running one test case took 127s and the shard running the other 598 took 128s. One second apart, which says the run is the small remainder and the build is the job. Sharding that buys nothing and pays for a second cold cache -- the two shards came to 161s and 156s against 145s for the single leg they replaced. Back to one leg, with the measurement in the comment so the next person sees the job's total time, thinks "shard it", and finds out here that it was already tried. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Adds one matrix entry:
Linux Valgrind (gcc, C++17, 64bit).What it buys that the existing legs do not
A read of memory that was never written. The sanitizer legs do not cover this and cannot:
memset(nullptr)guard inclear_buckets()That gap is worth closing for this container in particular. Buckets are
memsetrather than constructed, andm_shifts/m_bucket_mask/m_max_bucket_capacityare written by hand in an order chosen so nothing describing the bucket array is published before the array exists — see the comment inallocate_buckets_from_shift. "Has that field been written yet" is a real question to be able to ask.It finds nothing today, and that is the expected result
Running it against the current tree, with flags stricter than the leg uses:
The code already passes ASan, UBSan, TSan and four fuzzers, so anything in the overlapping categories was gone before this leg existed. The only category where valgrind could have found something new is uninitialised reads, and there are none. The value is prospective — it is a ratchet, not a discovery, and it can only go red on a new uninitialised read or leak.
Verified in both directions
A leg that cannot fail is worth nothing:
[[gnu::noinline]]call, valgrind reportsConditional jump or move depends on uninitialised value(s)and meson reportsFail: 1.Worth noting from the second check: the obvious spelling of an uninitialised read is rejected at compile time by
-Werror=uninitialized, so the plant had to be opaque enough to get past the compiler. That is itself a small argument for the leg — the easy cases are already caught, so what is left for a runtime tool is exactly what the compiler cannot see.Not sharded, and that was measured rather than assumed
The job's total time invites sharding, and the history here is worth keeping because the obvious reading is wrong.
meson testbuilds before it tests, so the single step is compile plus run — and a brand-new leg starts with an empty ccache, because the key is the matrix id. So the first run's 145s was mostly a cold compile of ~90 translation units, not valgrind.I shipped a two-way split to find out, and the result settles it:
fuzz_apiOne second apart. The run is the small remainder; the build is the job. Sharding it buys nothing and pays for a second cold cache — the two shards came to 161s and 156s against 145s for the single leg they replaced. Reverted, with the measurement left in the workflow comment so the next person to think "shard it" finds out it was already tried.
Risk
The one thing I could not test locally was the runner's valgrind against the runner's glibc, where a mismatch sometimes produces false positives in optimised libc string routines. CI has now run it — clean.
🤖 Generated with Claude Code