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

Use cfg(target_has_atomic) instead of hard coding which targets support atomics #1033

Closed
bjorn3 opened this issue Oct 26, 2023 · 7 comments · Fixed by #1037
Closed

Use cfg(target_has_atomic) instead of hard coding which targets support atomics #1033

bjorn3 opened this issue Oct 26, 2023 · 7 comments · Fixed by #1037

Comments

@bjorn3
Copy link

bjorn3 commented Oct 26, 2023

This has been stable since 1.60.0: rust-lang/rust#93824 The hard coded list is bound to go out of sync with rustc, especially when new targets get added. As rustc itself depends on crossbeam, this means it isn't possible to build rustc for targets lacking some atomics until a new crossbeam release with those new targets. Using cfg(target_has_atomic) will immediately work. While the MSRV of crossbeam is lower than 1.60.0, it should be possible to use the hard coded list as fallback when the rustc version is too old for stable cfg(target_has_atomic).

@bjorn3
Copy link
Author

bjorn3 commented Oct 26, 2023

@taiki-e
Copy link
Member

taiki-e commented Oct 26, 2023

While the MSRV of crossbeam is lower than 1.60.0, it should be possible to use the hard coded list as fallback when the rustc version is too old for stable cfg(target_has_atomic).

This is an approach I have previously explicitly stated I accept and would accept a PR that implements this.
#937 (comment)

@bjorn3
Copy link
Author

bjorn3 commented Oct 26, 2023

Do you know how to handle cfg(not(crossbeam_no_atomic_cas))? Use cfg(target_has_atomic="ptr")?

@taiki-e
Copy link
Member

taiki-e commented Oct 26, 2023

Use cfg(target_has_atomic="ptr")?

Yes.

  • not(crossbeam_no_atomic_cas) corresponds to target_has_atomic = "ptr".
  • not(crossbeam_no_atomic_64) (almost) corresponds to target_has_atomic = "64". (To be precise, it is closer to unstable target_has_atomic_load_store = "64", but currently, the only time the two would actually have different meanings is when using a custom no-std target.)
  • not(crossbeam_no_atomic) (almost) corresponds to unstable target_has_atomic_load_store = "ptr". There is no stable alternative to this, so I think this may require continuing to use our own cfg. This cfg only affects some no-std targets.

@bjorn3
Copy link
Author

bjorn3 commented Oct 28, 2023

Is adding autocfg as build dependency to the crates that don't have it yet fine?

@taiki-e
Copy link
Member

taiki-e commented Oct 28, 2023

Yeah, it's fine.

That said, given that #1015 will increase MSRV to 1.59, it would also actually be fine to increase MSRV to 1.60 or 1.61 and remove the need for build scripts. (I had forgotten about that PR when I first commented in this issue.)

@bjorn3
Copy link
Author

bjorn3 commented Oct 28, 2023

I think I will wait for that PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants