Test the seven promises a mutation sweep found nothing was checking - #192
Conversation
Issue #191. A full sweep killed 61% of mutants, and triaging the 360 survivors left a short list of things the suite believes without checking. Each item here is one of those, and each is verified the same way: the mutant that exposed it goes from `survived` to `caught`. **equal_range and count.** Six of the eight equal_range overloads never had their `it + 1` checked. The existing test does check `second == ++first`, but only on the non-const exact-key overload; the const one was asked with a single element, where `it + 1` and `end()` are the same iterator and a `second` that is always `end()` passes. The transparent and precomputed_hash forms were not asked at all. They are now, with the key at begin() so the two answers are as far apart as they get, and for a key that is absent, which is what tells `? 0 : 1` apart from `? 1 : 1`. **The hash's output.** Nothing pinned it, so every constant in the secret, every mixing step and most of the length arithmetic could change with the suite still green -- 173 survivors in one function. hash_golden.cpp fixes the values at one length per branch and both sides of every bound, including the lengths that land a loop counter exactly on its limit, which is the only place a `>` that became `>=` differs. This is not a promise that the values never change: replacing wyhash is a legitimate thing to do, and the file says how to regenerate it and that the question to ask then is "did we mean to". **erase()'s return value.** The erase-while-iterating loop erases from begin() every time, where begin() is also the right answer -- so a return value that is always begin() passed it. Erasing from the middle is what tells those apart, and it is the only case where the returned iterator has work to do. **"Looks like a default constructed table."** Three routes reach that state -- moved from, an assignment that threw, a copy of a cleared table -- and the shift that decides how big an array the next insert asks for is not observable except through that insert. The shared fixture now makes it, so all three are held to it, along with max_load_factor, which is carried by hand in three places. **segmented_vector's iterator interface**, which the map only uses a quarter of: post-increment could have called operator--, -= could have added, and all four relational operators could have been each other. Plus back(), resize() growing a vector that is not empty, and shrink_to_fit actually handing blocks back. **The bucket packing**, as a static_assert in the table rather than a test, so a user's own bucket type is held to it too: the fingerprint has to stay strictly below dist_inc, or hash bits add to the distance a bucket claims and silently reorder the probe sequence -- correct answers, wrong shape, nothing notices. **mixed_hash's third branch.** An avalanching hash narrower than 64 bits is multiplied up because the table indexes with the top bits. Without it every key lands in bucket zero and the table still answers everything correctly, just by walking the array, so only a distribution check can see it. Nothing in the suite used a hasher that was both avalanching and 32 bit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
doctest's INFO builds a lambda around what it is given, so `INFO("length ", len)`
where len came from `auto const& [len, want]` is a capture -- allowed by gcc,
rejected by clang under -Werror, which is where CI found it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Move construction and move assignment part with the source through different code -- the constructor exchanges the members itself, the assignment goes through move_everything_from -- and only the first was being asked whether what it left behind looks default constructed. Putting the bug back in the second went unnoticed until now. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Verified: 61% → 81% killed, 33% → 51% by a testFull run of
174 more mutants are now caught by a test, and the named bugs went from 11 survivors to 5. The move-assignment case was a real gap this found: move construction and move assignment part with the source through different code — the constructor exchanges the members itself, the assignment goes through What still survives, and why it should
That leaves ~89 genuine sweep survivors, down from ~129, mostly one-off boundary conditions in paths the ticket ranked below these seven. |
The allocator cannot answer this: m_blocks is itself allocated through it, so its own shrink_to_fit records an event whether or not a single block was freed. "Something happened" was therefore true for a loop that hands nothing back, and the mutant that makes it hand nothing back went unnoticed. capacity() is m_blocks.size() times the block size, which is a direct count of the blocks still held. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
It is its own probe loop, shared with no other insert path -- its own bound, its own key comparison, its own returned iterator -- and the tests that call it gave it a set holding nothing or one element, where a wrong iterator and a wrong "was it inserted" are both still begin() and still the only answer available. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two more clusters, two real bugs, and four tool changesThe two remaining clusters from #191 are covered. The transparent set Writing the
|
Both found by writing the boundary test for replace()'s max_size guard, which issue #191 flagged as unasked. **rehash() could leave a single bucket.** calc_shifts_for_size() walks the shift down until the capacity it computes covers the count, but calc_num_buckets() saturates at max_bucket_count() -- so above max_bucket_count() * max_load_factor() the capacity being compared stops growing while the walk carries on, all the way to zero. calc_num_buckets(0) then evaluates `std::size_t{1} << 64`, which is undefined and on x86 is one. A table asked for billions of buckets came back with one, a mask of zero, and an out-of-bounds read on the next probe. Reachable on the default map<uint32_t, uint32_t>: rehash() does not size the value container first, so there is no enormous allocation to fail before the bucket arithmetic runs. `map.rehash(3865470566)` was enough. The loop now stops once the array is as large as it may get, because decrementing past that adds no buckets. **replace() ignored a container of exactly max_size() elements.** The loop counted in value_idx_type, and max_size() is exactly the number of values that type can hold -- so a container of precisely that many has a size that is not representable in it. The cast wrapped to zero, the loop never ran once, and the table came back reporting size() elements with not one bucket pointing at any of them: size() said 256, every lookup said no. Counted in size_t now; every index the loop produces is representable, it is only the count that is not. That one needs a bucket type whose value index is smaller than the container it is given, so with the shipped types it would take a container of 2^32 pairs. It is reachable with a custom Bucket, which is a supported thing to write, and the suite has one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Four changes to the tool, each from something the first full sweep measured. **--diff is the everyday mode now.** Bare `--diff` means HEAD, which is whatever is uncommitted, and the lines come from the merge base rather than the ref's tip. On a branch that has not caught up, the second reading sweeps every line main moved on without you as though it were yours. **Mutants that cannot have an effect are no longer generated.** 47 of the 360 survivors were `std::enable_if_t<..., bool> = true>` -- the SFINAE idiom, whose parameter exists so the substitution has somewhere to fail and whose value is never read. Recognised by shape rather than by looking for `enable_if_t`, because the idiom is the default value in a `>`-terminated list and not any particular trait. Comments, strings and preprocessor lines were already skipped. **Mutants in code this build does not compile are dropped.** 35 more survivors were in `mum()`, which picks between __uint128_t, an MSVC intrinsic and a long-hand multiply -- so two thirds of it is never seen here, and each one cost a full rebuild of ~90 translation units to come back `survived`. Found by asking the preprocessor which lines survived rather than by matching `#if` in the text, because the answer depends on the flags the build actually uses. The run says which lines were dropped: that is a coverage question, and staying quiet about it would read as "everything here is covered". **--deletions removes whole statements.** The operator the token sweep cannot express, and the one the hand-written bugs turned out to live in: nearly every block in bugs/invariants.txt is a form of "the code forgot to do this" -- the shift down that never happens, the pop_back that is skipped, the bucket that is never repointed -- and not one of them is a single token. On the erase path it finds them: deleting the move of the last element into the erased hole is caught, and six other deletions there hang. Roughly doubles the count, and the ones that cannot compile cost the pre-filter's half second rather than a rebuild. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both from CI. The stdint linter reads comments too, which is right -- a comment that says size_t while the code says std::size_t is the drift it exists to stop. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`--operators deletions` is the run worth having: half of them are rejected by the pre-filter in half a second rather than costing a rebuild, so a survey of deletions alone comes to less than the token sweep, and mixing the two only makes the report harder to read. A flag that adds to the default could not express it. Also corrects the cost model's comment with what a mutant actually costs, since the old figure was inherited rather than measured: 67 CPU-seconds on an idle machine, of which 75 unit translation units at ~0.66s each are 85%, against 3.2 to run the suite and 1 to link. The 100 the estimate uses is right anyway -- under the 32-way contention a real run creates the same work costs about 110. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Closes #193. Three files stood in the way, and each failure had been there and invisible because separate compilation hides this class of thing. `test/app/print.h` had no include guard, so any chunk that pulled it in twice redefined `test::print`. `test/unit/maps_of_maps.cpp` defined a namespace-scope `template <typename Map> void test()`, which collides with `namespace test` -- where the shared fixtures live -- once both are in the same translation unit. And four `test/bench/*.cpp` each defined a `bench()`: they are already in anonymous namespaces, which is exactly the trap, because an anonymous namespace stops isolating a file once its neighbours are merged into the same one. Two of them landing in one chunk made the calls ambiguous. Renamed per file, which is what quick_overall_map.cpp was already doing. Off by default, because merging is bad for development: touching one file recompiles its whole chunk. So there is a CI leg, or it would quietly go back to broken the next time a file adds a helper called bench(). The mutation tool configures its lanes with it, where that objection cannot apply -- a mutant recompiles every file anyway, so there is nothing left to spoil, and merging is 2.5x less compiling for the same work. Measured, one mutant rebuild of the whole suite at -j1: 67.7 CPU-seconds separately, 27.4 merged. On a full sweep that is most of an hour. One thing had to be taught about unity first. compile_commands.json lists the chunks a unity build generates and not the files they include, so the TU the -fsyntax-only pre-filter compiles is not in there at all -- and the pre-filter would have turned itself off silently, which costs a full rebuild for every mutant that is not valid C++ rather than half a second. The flags are the same for every chunk, so one is borrowed and the source path swapped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
kernel.core_pattern is often a bare `core`, so a crashing test or a hand-run reproducer drops one wherever the process was started from -- which is usually the repo root, where `git add -A` then sweeps it up. Three of them made it into this branch that way, while the two size-at-the-limit bugs were being tracked down, and had to be rewritten out of its history. mutate.py already sets RLIMIT_CORE to 0 for everything it spawns, because a crashing mutant is an ordinary verdict there and each dump was ~30 MB in a lane. This is the same guard for everything else. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Reuse, simplification, efficiency and altitude, reviewed independently. Where they overlapped they were right. Three of the four flagged the same thing: test/unit/bucket.cpp restated the static_asserts this branch had just added to the header, in a second formulation, for types the header already checks by instantiating them. Deleted. The comment on bucket_micro's fingerprint_mask had already drifted -- copied from the standard bucket, describing 11 bits for a type with 1 -- which is what two copies of an invariant does. Reuse: test/app/hashers.h now holds the transparent hasher that three test files had each declared, and the 32 bit avalanching one that two had. They carry no assertions, so sharing them couples nothing -- unlike a check_* helper, which stays with its test. lazy_bucket_allocation.cpp builds its tables with test::filled, which already existed and which the file's neighbours already use. Simplification: equal_range's "a hit is exactly one element" was written out three times because the helper picked its own key; it takes the key as a parameter now and the set and transparent cases call it. The avalanching tests were two, the second restating the first's conclusion from a 10000 element map it never queried, under a comment describing probe-distance arithmetic that was not there. One test now, and wide_avalanching is gone -- honest_hash was already the 64 bit avalanching hash it duplicated. Efficiency: a sweep built a whole mutated copy of the header per site -- 130 KB times 1500 sites for both operators, 200 MB, built before --limit or the uncompiled-line filter could throw most of it away and held live beside 32 build subprocesses. Mutants carry the splice now and the lane applies it. Also, is_template_default was being asked about all ~40000 identifiers rather than the few hundred that could produce a mutation, and deletion_sites did its per-character work before consulting the line filter rather than after. Altitude: the mutant text the report keeps and the equivalence rule that stayed quiet. Skipping a template parameter default is now counted and reported, like every other "we will not run this" rule in the file -- a rule that recognises an idiom by shape is the one that most needs to say so, because the shape is wider than the idiom. And _syntax_command's unity fallback no longer guesses silently: it prefers an entry from the build it was asked about, and if the source path in the borrowed command is spelled some other way it gives up rather than checking the wrong file and passing every time. One finding was a bug rather than a cleanup, and is fixed here because it is the same bug this branch already fixed once: clear_and_fill_buckets_from_values() counted in value_idx_type exactly as replace() used to, so a container of precisely max_size() elements wraps the count to zero and the refill never runs. Latent -- a table at max_size() already has the smallest shift, so rehash() and reserve() early out before reaching it -- but the rule was being followed in one of the two places that state it. Skipped, and recorded rather than argued with: folding the uncompiled-line filter into line_filter before generation, which is right but needs the lane setup and the lane-count decision reordered around it; and giving calc_shifts_for_size a min_shifts constant instead of a second loop condition, which is a third library change on a branch that already carries two. Moving compiled_lines() off the critical path was measured rather than argued: 0.13s once per run, against the threading it would take. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
e9ca8b0 to
5c3708d
Compare
Working through #198. Seven tests, each verified against the mutant it targets; survivors go 37 -> 29 and the sweep to 96% killed. reserve() clamps what it is asked for to max_size() before handing it on, and without that a caller asking for more than the table can index reaches std::vector::reserve() with the raw number and gets a length_error out of a call that should have given them everything there is. bucket_micro is what makes that askable: on a default map the clamped call still reserves 2^32 pairs, so the test would be indistinguishable from the bug. Move assignment's recovery -- reset_to_empty() and the bare throw -- needed a throwing move *and* an allocator that neither propagates nor compares equal, because with std::allocator the whole operation is noexcept and the try/catch is not instantiated at all. That is asserted rather than assumed, so the test cannot go quietly vacuous. Also: the transparent extract() overload nothing was calling, which now comes back `compiler` rather than `survived`; rehash() handing back the value container's spare capacity; segmented_vector's self-*move*-assignment guard, which the table's own `&other != this` check means only a vector used directly can reach; and that a move assignment frees the blocks it replaces, counted rather than assumed so it does not have to know how many elements fit in one. Three things this turned up that are not tests. `count = min(count, max_size())` in rehash() cannot be caught by anything, because calc_shifts_for_size() saturates at max_bucket_count() and so answers the same either way. The fix in #192 made the clamp redundant. Two survivors are already covered, by the sanitizer legs rather than by an assertion: removing the `return` guard in clear_buckets() gives UBSan a null pointer passed to memset, and removing the reserve() in a segmented_vector's block growth leaks 12 KB for ASan to find. Both were verified by re-running them under --meson-arg=-Db_sanitize=address,undefined. And #198's claim that bucket_count() derives from m_bucket_mask is wrong -- it returns m_buckets.size() -- which is what the whole bucket-lifecycle group was triaged on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Closes #191.
The full sweep in #191 killed 61% of mutants. Triaging the 360 survivors left a short list of things the suite believes without checking — this is that list. Each item is verified the same way the ticket asks for: the mutant that exposed it goes from
survivedtocaught.equal_range and count
Six of the eight
equal_rangeoverloads never had theirit + 1checked.equal_range.cppdoes assertsecond == ++first, but only on the non-const exact-key overload — the const one was asked with a single element, whereit + 1andend()are the same iterator, so asecondthat is alwaysend()passes it. The transparent andprecomputed_hashforms were not asked at all. All eight are now asked with the key atbegin(), where the two answers are as far apart as they get, and with a key that is absent, which is what tells? 0 : 1apart from? 1 : 1incount.The hash's output
Nothing pinned it, so every constant in the secret, every mixing step and most of the length arithmetic could change with the whole suite green — 173 survivors in one function.
hash_golden.cppfixes the values at one length per branch and both sides of every bound, including the lengths that land a loop counter exactly on its limit (65, 192, 289), which is the only place a>that became>=differs.This is not a promise that the values never change. Replacing wyhash is a legitimate thing to do; the file says so, says how to regenerate, and says the question to ask then is "did we mean to change the hash".
erase()'s return value
The erase-while-iterating loop erases from
begin()every time — wherebegin()is also the correct answer, so a return value that is alwaysbegin()passed it. Erasing from the middle is what tells those apart, and it is the only case where the returned iterator has any work to do."Looks like a default constructed table"
Three routes reach that state — moved from, an assignment that threw, a copy of a cleared table — and the shift that decides how big an array the next insert asks for is not observable except through that insert. The shared fixture now makes that assertion, so all three routes are held to it, along with
max_load_factor, which is carried by hand in three separate places.segmented_vector
The map uses about a quarter of its iterator interface. Post-increment could have called
operator--,-=could have added, and all four relational operators could have been each other. Plusback(),resize()growing a vector that is not empty (every existing case grows from zero, where the arithmetic cannot be wrong), andshrink_to_fitactually handing blocks back.The bucket packing
As a
static_assertintablerather than a test, so a user's own bucket type is held to it too: the fingerprint has to stay strictly belowdist_inc, or hash bits add to the distance a bucket claims and silently reorder the probe sequence — correct answers, wrong shape, nothing notices. The bound is one-sided on purpose: fewer fingerprint bits thandist_incallows is a weaker fingerprint, not a broken one.mixed_hash's third branch
An avalanching hash narrower than 64 bits is multiplied up, because the table indexes with the top bits. Without it every key lands in bucket zero and the table still answers everything correctly, just by walking the array — so only a distribution check can see it. Nothing in the suite used a hasher that was both
is_avalanchingand 32 bit.Not fixed, and worth recording rather than chasing: four of the ticket's eleven named bugs are equivalent mutants (a
>that becomes>=inside theelseof an==test is exactly>), and 36 wyhash survivors are inmum's long-hand branch, which is dead code wherever__uint128_texists — the golden values do cover it on the 32-bit and MSVC legs.Full suite green, and green under
-Db_sanitize=address,undefined.🤖 Generated with Claude Code