Make parallel frontend alloc-id numbering deterministic and re-enable the tests#157333
Open
xmakro wants to merge 3 commits into
Open
Make parallel frontend alloc-id numbering deterministic and re-enable the tests#157333xmakro wants to merge 3 commits into
xmakro wants to merge 3 commits into
Conversation
These const-eval tests were marked ignore-parallel-frontend for "different alloc ids", but their only nondeterminism under -Zthreads is the alloc id numbering, which compiletest already normalizes by order of first appearance. Their diagnostic output is otherwise deterministic, so remove the directive and bless the line numbers that shift from removing it. The remaining tests in that group emit multiple errors whose order is itself nondeterministic, which also reshuffles the alloc id numbering; those are left ignored.
rustbot
added
S-waiting-on-author
This comment has been minimized.
This comment has been minimized.
Under `-Zthreads` the compiler emits diagnostics in a nondeterministic order. compiletest renumbers allocation ids by their first appearance in the rendered output, so that order leaked into the normalized `ALLOC<n>` indices and made several const-eval UB tests flaky. Sort the rendered diagnostics by their primary source location before normalizing, so the numbering no longer depends on emission order. Ties are broken by the diagnostic text with the raw allocation numbers blinded, since those numbers are themselves racy. This only runs under the parallel frontend, where stderr is already compared line by line, so single-threaded output is unchanged.
These ten const-eval tests emit multiple errors carrying allocation ids. They were ignored under the parallel frontend because the racy emission order produced unstable `ALLOC<n>` numbering. Now that compiletest sorts diagnostics by source location before renumbering, the numbering is stable, so drop the `ignore-parallel-frontend` directives. The stderr changes are only the line-number shift from removing the directive line.
rustbot
added
A-compiletest
xmakro
changed the title
rustbot
added
the
S-waiting-on-review
Collaborator
|
Some changes occurred in src/tools/compiletest cc @jieyouxu |
rustbot
removed
the
S-waiting-on-author
Collaborator
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @tiif (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
Why was this reviewer chosen?The reviewer was selected based on:
|
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
4 participants
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.
Part of #154314. Addresses the "different alloc ids" row (#154278).
A group of const-eval ui tests were marked
//@ ignore-parallel-frontend different alloc ids. AllocIds are handed out from a global counter, so under-Zthreadsthe numbers in the output differ run to run. compiletest already normalizes this: it remapsallocNtoALLOC{index}by order of first appearance (runtest.rs), so a pure renumbering does not affect the comparison.For most of these tests the renumbering is the only difference. Each one emits a single const-eval error, or emits its errors in a deterministic order because const items are evaluated during the analysis phase rather than during parallel mono collection, so the order of first appearance is stable and the normalized output matches the single-threaded blessing. The first commit removes the directive from those tests and blesses the line numbers that shift from removing the directive line. No test output content changes, only span line numbers.
The remaining tests emit several const-eval errors whose emission order is itself nondeterministic under the parallel front-end. That reshuffles which allocation appears first, so the order-of-first-appearance normalization assigns different
ALLOCindices and the normalized line text differs run to run.To fix that at the source, the second commit makes compiletest sort the rendered diagnostics by their primary source location before it renumbers allocation ids. The first-appearance order then no longer depends on the racy emission order, so the
ALLOCnumbering is stable. Ties at the same location are broken by the diagnostic text with the raw allocation numbers blinded, since those numbers are racy too. The sort only runs under the parallel front-end, where stderr is already compared line by line, so single-threaded output is unchanged. With that in place the third commit re-enables the remaining ten tests.Validation: every re-enabled test passes single threaded and under
--parallel-frontend-threads=16 --iteration-count=100 --force-rerun(the multi-allocation ones at 200). Removing the directive shifts span line numbers by one and nothing else; the.32bit.stderrfiles, which cannot be blessed on a 64-bit host, were updated with the same shift and verified against the.64bit.stderrdelta. A baseline run with the sort disabled confirms all ten fail without it and that no other test changes behavior under the parallel front-end.