Skip to content
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

Hack fix for ICE as seen in CI #23295

Merged
merged 1 commit into from
Feb 23, 2022
Merged

Conversation

t-nelson
Copy link
Contributor

Problem

#23233 added BuiltinFeatureTransition
to Bank which triggers rust-lang/rust#92987 while
attempting to resolve Sync on BankRc in AccountsBackgroundService::new ala,

query stack during panic:
#0 [evaluate_obligation] evaluating trait selection obligation `bank::BankRc: core::marker::Sync`
#1 [typeck] type-checking `accounts_background_service::<impl at runtime/src/accounts_background_service.rs:358:1: 520:2>::new`
#2 [typeck_item_bodies] type-checking all item bodies
#3 [analysis] running analysis passes on this crate
end of query stack

Summary of Changes

Yoloing a Sync onto it avoids the auto trait evaluation and thus the ICE.

We should remove this when upgrading to Rust 1.60.0, where the bug has been
fixed by rust-lang/rust#93064

@t-nelson t-nelson requested a review from jstarry February 23, 2022 07:05
// #3 [analysis] running analysis passes on this crate
// end of query stack
//
// Yoloing a `Sync` onto it avoids the auto trait evaluation and thus the ICE.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which is safe because the compiler was cool with the same types in tuple form before #23233, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, but how about adding some extra checks to ensure that our assumptions aren't broken in the future. Here are my suggested changes: t-nelson#32

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand the benefit. Commented over there

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jstarry what assumptions are you referring to that may be broken in the future?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jackcmay the unsafe impl can introduce undefined behavior because the compiler won't complain if they are added for types which are not thread-safe.

@t-nelson t-nelson added CI Pull Request is ready to enter CI and removed CI Pull Request is ready to enter CI labels Feb 23, 2022
@solana-grimes solana-grimes removed the CI Pull Request is ready to enter CI label Feb 23, 2022
@t-nelson t-nelson added the CI Pull Request is ready to enter CI label Feb 23, 2022
@solana-grimes solana-grimes removed the CI Pull Request is ready to enter CI label Feb 23, 2022
@t-nelson t-nelson added the CI Pull Request is ready to enter CI label Feb 23, 2022
@t-nelson t-nelson merged commit c81dd60 into solana-labs:master Feb 23, 2022
@t-nelson t-nelson added the v1.9 label Feb 23, 2022
t-nelson added a commit to t-nelson/solana that referenced this pull request Feb 24, 2022
@t-nelson t-nelson deleted the thaw-ice branch February 24, 2022 05:08
t-nelson added a commit to t-nelson/solana that referenced this pull request Feb 24, 2022
t-nelson added a commit to t-nelson/solana that referenced this pull request Feb 24, 2022
t-nelson added a commit that referenced this pull request Feb 25, 2022
mergify bot pushed a commit that referenced this pull request Feb 25, 2022
(cherry picked from commit 5e0086c)

# Conflicts:
#	runtime/src/builtins.rs
t-nelson added a commit that referenced this pull request Feb 25, 2022
(cherry picked from commit 5e0086c)

# Conflicts:
#	runtime/src/builtins.rs
mergify bot added a commit that referenced this pull request Feb 25, 2022
(cherry picked from commit 5e0086c)

# Conflicts:
#	runtime/src/builtins.rs

Co-authored-by: Trent Nelson <[email protected]>
jeffwashington pushed a commit to jeffwashington/solana that referenced this pull request Mar 4, 2022
brooksprumo added a commit to brooksprumo/solana that referenced this pull request Jun 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Pull Request is ready to enter CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants