Skip to content

SessionDiagnostic: Fix non-determinism in generated format string. #76515

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Sep 10, 2020
Merged

SessionDiagnostic: Fix non-determinism in generated format string. #76515

merged 1 commit into from
Sep 10, 2020

Conversation

jumbatm
Copy link
Contributor

@jumbatm jumbatm commented Sep 9, 2020

Fixes #76496.

r? @oli-obk

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 9, 2020
@jumbatm
Copy link
Contributor Author

jumbatm commented Sep 9, 2020 •
edited
Loading

From #76496 (comment):

So the fix is twofold: First change all the HashSet and HashMap to FxHashSet and FxHashMap where possible, and second, collect to a Vec and sort that Vec before iterating over the set/map

rustc_macros can't depend on rustc_data_structures for FxHash* because it depends a circular dependency. Instead, I've switched to a BTreeSet so the iteration order is deterministic.

@jgalenson -- I know it's a bit of an ask, but can you confirm this fixes the problem?

@oli-obk
Copy link
Contributor

oli-obk commented Sep 9, 2020

@bors r+

I think @jgalenson will see whether this fixes it once this is in nightly and their script runs again

@bors
Copy link
Collaborator

bors commented Sep 9, 2020

📌 Commit 8b39250 has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 9, 2020
@oli-obk
Copy link
Contributor

oli-obk commented Sep 9, 2020

Instead, I've switched to a BTreeSet so the iteration order is deterministic.

Perfect! This is much better. The FxHashSet stuff is only for speed, not determinism anyway

@jyn514 jyn514 added A-reproducibility Area: Reproducible / deterministic builds T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 9, 2020
@jgalenson
Copy link

Thanks for the patch! I've started some runs to see if this fixes the problem; I'll reply again when they're done.

@jgalenson
Copy link

Yes, it seems to solve the problem. Thanks!

@jumbatm
Copy link
Contributor Author

jumbatm commented Sep 9, 2020

No worries - thank you for checking :)

@tmandry tmandry mentioned this pull request Sep 9, 2020
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 9, 2020
@bors
Auto merge of rust-lang#76540 - tmandry:rollup-5ogt8x0, r=tmandry
97eb606 
Rollup of 14 pull requests

Successful merges:

 - rust-lang#75094 (Add `-Z combine_cgu` flag)
 - rust-lang#75984 (Improve unresolved use error message)
 - rust-lang#76141 (Address review comments about config.toml from rustc-dev-guide PR)
 - rust-lang#76313 (Improved the MIR spanview output)
 - rust-lang#76430 (Add align to rustc-attrs unstable book)
 - rust-lang#76465 (Add a script to automatically update Rust/Clang versions in documentation)
 - rust-lang#76473 (Add missed spaces to GCC-WARNING.txt)
 - rust-lang#76481 (Convert repetitive target_pointer_width checks to const solution.)
 - rust-lang#76493 (Remove a stray ignore-tidy-undocumented-unsafe)
 - rust-lang#76504 (Capitalize safety comments)
 - rust-lang#76515 (SessionDiagnostic: Fix non-determinism in generated format string.)
 - rust-lang#76516 (Enable GitHub Releases synchronization)
 - rust-lang#76522 (remove redundant clones)
 - rust-lang#76523 (Remove unused PlaceContext::NonUse(NonUseContext::Coverage))

Failed merges:

r? `@ghost`
@bors bors merged commit 98f59bc into rust-lang:master Sep 10, 2020
@rustbot rustbot added this to the 1.48.0 milestone Sep 10, 2020
@jieyouxu jieyouxu mentioned this pull request Aug 14, 2024
Open
81 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@oli-obk oli-obk

Labels
A-reproducibility Area: Reproducible / deterministic builds S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.48.0
Development

Successfully merging this pull request may close these issues.

Reproducibility regression caused by 57edf88
7 participants
@jumbatm @oli-obk @bors @jgalenson @rust-highfive @jyn514 @rustbot