Schedule encoding additions + various fixes related to schedules - #316
Draft
sarsko wants to merge 3 commits into
Draft
Schedule encoding additions + various fixes related to schedules#316sarsko wants to merge 3 commits into
sarsko wants to merge 3 commits into
Conversation
A schedule recorded at a panic necessarily stops there, but the replayed execution keeps asking for scheduling decisions while the panic unwinds, because a `Drop` handler that touches a synchronization primitive yields. `ReplayScheduler` ran off the end of the schedule and asserted "schedule ended early". It now keeps scheduling while `std::thread::panicking()`, preferring the unwinding task, because returning `None` there would stop the execution mid-unwind and abandon the very panic being reproduced. Outside of unwinding the assertion stays, with a message that says what actually went wrong. `next_u64` had the same hole and indexed out of bounds instead of asserting. A failure also reported twice, from the panic hook at the moment of the panic and again from the runtime once the panic had unwound. Under `Print` that meant two schedules in the output with nothing to say which to copy, and under `File` two files holding prefixes of each other. Reporting from the hook is not redundant: it is the only thing that gets a schedule out when a second panic during unwinding aborts the process, which is what #202 added it for. But it is only needed for tests at risk of aborting, so it moves behind `Config::eager_failure_reports`, defaulting off. The default is now a single report after the unwind, which is also the schedule that reproduces the failure most faithfully, since it covers the decisions the unwind needed. With the flag set, reporting is grow-only, as #202 established. Worth restating why, since it looks redundant: a panic the test catches itself still runs the hook, so a once-only report would be consumed by a swallowed panic and the real failure would go unreported. Each report must therefore be longer than the last, so it supersedes rather than duplicates it. Under `File` the longer schedule now rewrites the file it superseded, leaving one file per failure holding the most complete schedule; under `Print` the superseding block says that it supersedes. `abort_freedom::max_steps_panic_during_drop`, which exists to check that an aborting test still yields a schedule, opts in. Two further bugs in the same path: `init_panic_hook` used a `static Once` and captured a `Config` by value, so the hook permanently held the config of whichever `Runner` ran first and every later `Runner`'s `FailurePersistence` setting was ignored. The hook now reads the config of the execution that is actually running. The "have we reported" state was never reset between executions, so a long schedule in one execution could suppress the report for a shorter failing schedule in a later one. It is now scoped to the execution, which also fixes a related misattribution: the state outlived the execution entirely, so any later panic on the thread, including the test harness reporting the failure, was reported as a Shuttle failure and serialized the schedule of an execution that had already finished. The scope is a guard, so it covers the unwind path too. Re-enables the 14 tests ignored with "replay mechanism is broken because the schedule is not emitted in the panic output". Their harness scraped the schedule out of the panic payload, which #202 deliberately stopped doing, so the harness now reads the persisted file instead. Nine fixtures had gone stale and are regenerated; the seed-based ones are compared with whitespace stripped, since what they pin is the sequence of scheduling decisions rather than the width the schedule is wrapped at. Note that #202's plan was to fix these by having Shuttle return a `Result` carrying the schedule, at which point they can assert on that instead and this harness can go away. Drops the `regex` dev-dependency, which only existed to parse schedules out of panic output.
A failing schedule was encoded as a fixed-width field per step, sized to the largest TaskId anywhere in the schedule, and rendered as hex. A test with many tasks therefore paid for the largest task ID on every step even though only a handful of tasks are live at any moment, and then spent a whole character on four bits. A 420,000-step schedule over 300 tasks printed as 8,777 lines, which is not something you can copy out of a terminal. Two independent changes, each selectable through `Config`, and both formats remain readable so already-saved schedules keep replaying. `ScheduleEncoding` picks how the payload is built. The new `MoveToFront` default codes each step by its rank in a move-to-front list of recently scheduled tasks, which turns "which of the N tasks in this test" into "which of the few recently run tasks". Ranks are close to uniform over the live set rather than power-law, so they use a flat 4-bit field with an Elias delta escape rather than coding the rank directly. Measured at 4.37 bits per step against 11, on a real 445,000-step schedule. `ScheduleTextEncoding` picks how those bytes become printable text. The new `Unicode` default spends one terminal column on a 14-bit base character plus eight bits for every combining mark stacked on it. Marks are zero-width, so they add information at no column cost. The default stacks as deep as the payload needs, which puts the whole schedule in a single cell: one column, one line, however long the schedule is. The alphabets are generated and constrained so the text survives a terminal, a clipboard and an editor. Base characters are single-column, so a cell is a column; left-to-right or bidi-neutral, so nothing reorders on display; assigned, so Unicode's guarantee that normalized text stays normalized still applies; and unchanged under all four normalization forms. 2^14 is the ceiling under those constraints, since only 23,544 code points qualify. Marks are variation selectors, which are Default_Ignorable and so render as nothing, and have canonical combining class zero. That class matters twice: normalization sorts marks by class, so a pool mixing classes would be silently reordered by anything that normalizes, and class zero makes them starters rather than non-starters, which exempts them from the 30-non-starter cap in the Stream-Safe Text Format of UAX #15. A pool of non-starters would have U+034F COMBINING GRAPHEME JOINER injected into deep stacks, which UAX #15 notes is not canonically equivalent to the original. 2^8 is the ceiling there, and by a wide margin: only 263 code points in all of Unicode meet those requirements. The stacking depth is not recorded anywhere. The decoder infers each character's width from the character itself, so a schedule written at any depth is readable by any version, and re-wrapped or re-flowed text still decodes. A CRC-32 over the payload means a schedule that lost characters in transit, which is what happens when a terminal clips a mark stack, is reported as an error rather than silently replayed as a different schedule. Also widens the wrap width from 76, which came from email conventions, to 120, and fixes a pre-existing panic found by a new fuzz test: the fixed-width reader unwrapped a `None` on the input `[0x91, 0x00, 0x01, 0x00]`. Both readers now sanity-check the claimed length against the bits available, which also closes an out-of-memory path via a bogus varint.
The Unicode alphabet assumes the destination can carry non-ASCII text. That is
true of a file and of most terminals, but not of a terminal under a non-UTF-8
locale, and a schedule that gets mangled on the way out is worse than a verbose
one.
`ScheduleTextEncoding::Auto` is now the default and decides per destination. A
schedule written to a file always gets the Unicode alphabet, because a file is
a byte sink. A schedule printed to a terminal gets it only if the locale says
the terminal expects UTF-8, checked the way POSIX defines for this question:
LC_ALL, then LC_CTYPE, then LANG, first one set wins. A run under LC_ALL=C
falls back to hex on its own.
Worth being precise about what this detects. Whether a terminal accepts UTF-8
says nothing about whether it renders stacked combining marks as zero-width,
and no environment variable answers that. Measuring it would mean writing the
text and querying the cursor position, which needs raw mode on the same
terminal, and schedules are reported from a panic hook, possibly while a panic
is still unwinding. So `Auto` decides the alphabet, which is knowable, and
leaves the stacking depth alone. `Unicode { marks_per_cell: 0 }` remains the
setting whose cost does not depend on the terminal at all, and it still beats
hex better than three to one.
`stderr_accepts_non_ascii` treats a non-terminal stderr as safe, since a pipe,
a file or a captured test log all take UTF-8 without complaint. Note that
libtest's output capture intercepts `eprintln!` above the file descriptor
rather than by replacing it, so this still sees the real terminal under
`cargo test`, which is the terminal the captured output is eventually replayed
to. Verified both ways under a pty.
If resolution somehow leaves the encoding unresolved, serialization falls back
to hex rather than panicking. This runs on the failure-reporting path, where
losing the schedule to a second panic is far worse than a less compact
alphabet.
sarsko
changed the title
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
1 participant
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.
Draft since I still need to review it myself.
I also need to finish this description.
PR does a few related changes.
The goal I set out with was to solve my longstanding pet peeve of getting my terminal clobbered whenever I forgot to run a Shuttle test with
SHUTTLE_TRACE_DIRset. I realize now that it would have been easier to just update myzshrctoexport SHUTTLE_TRACE_DIR=., but alas.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.