-
Notifications
You must be signed in to change notification settings - Fork 13.6k
Fix parallel rustc not being reproducible due to unstable sorts of items #144722
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.
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
base: master
Are you sure you want to change the base?
Conversation
|
Some changes occurred in src/tools/compiletest cc @jieyouxu |
This comment has been minimized.
This comment has been minimized.
I don't think the changes of diagnostic orders are expected.😅 |
b7e0813
to
a03edc6
Compare
This comment has been minimized.
This comment has been minimized.
MonoItem::Static(def_id) => def_id.as_local().map(Idx::index), | ||
MonoItem::GlobalAsm(item_id) => Some(item_id.owner_id.def_id.index()), | ||
}, | ||
local_item_query(item, |def_id| tcx.def_span(def_id)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we try to use def_ident_span
or find_ancestor_not_from_extern_macro
to avoid having to shuffle tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your advice. I'll try it. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def_ident_span
failed on tests/assembly-llvm/emit-intel-att-syntax.rs
about naked and global asm.
find_ancestor_not_from_macro
works here.
This reverts commit 9f47e26.
a03edc6
to
d6294cc
Compare
The job Click to see the possible cause of the failure (guessed by this bot)
|
If we don't sort items again, it would broke existing tests. How about |
Currently, A tuple
(DefId, SymbolName)
is used to determine the order of items in the final binary. HoweverDefId
is expected as non-deterministic, which leads to some not reproducible issues under parallel compilation. (See #140425 (comment))Theoretically, we don't need the sorting because the order of these items is already deterministic.
However, codegen tests reply on the same order of items between in binary and source.
So here we added a new option
codegen-source-order
to indicate whether sorting based on the order in source. For codegen tests, items are sorted according to the order in the source code, whereas in the normal path, no sorting is performed.Specially, for codegen tests, in preparation for parallel compilation potentially being enabled by default in the future, we use
Span
replacingDefId
to make the order deterministic.This PR is purposed to fix #140425, but seemly works on #140413 too.
This behavior hasn't added into any test until we have a test suit for the parallel frontend. (See #143953)
Related discussion: Zulip #144576
Update #113349
r? @oli-obk
cc @lqd @cramertj @matthiaskrgr @Zoxc @SparrowLii @bjorn3 @cjgillot @joshtriplett