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

Add cfg(target_has_atomic_equal_alignment) and use it for Atomic::from_mut. #76965

Merged
merged 8 commits into from
Sep 24, 2020

Conversation

m-ou-se
Copy link
Member

@m-ou-se m-ou-se commented Sep 20, 2020

Fixes some platform-specific problems with #74532 by using the actual alignment of the types instead of hardcoding a few target_archs.

r? @RalfJung

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 20, 2020
@m-ou-se m-ou-se mentioned this pull request Sep 20, 2020
@RalfJung
Copy link
Member

This is code I never touched and I also don't know what it takes to make sure this new cfg flag is unstable and who would need to approve such an addition. Usually I'd think it requires an RFC, but maybe that is wrong.

But you'll need to find another reviewer, sorry. For now, assigning to the reviewer of the PR this follows up on.
r? @KodrAus

@m-ou-se
Copy link
Member Author

m-ou-se commented Sep 20, 2020

(If this takes too much time right now, feel free to merge #76967 instead.)

Edit: That PR (temporarily reverting Atomic::from_mut) is now merged. I updated this PR to revert that revert.

@m-ou-se
Copy link
Member Author

m-ou-se commented Sep 20, 2020

Just like the other target_has_atomic* cfg flags, this flag is internal and only used to communicate from rustc to libcore which functions it should make available. Consumers of libcore should probably use the generic cfg(accessible) (#64797) when that stabilizes.

@KodrAus
Copy link
Contributor

KodrAus commented Sep 20, 2020

Thanks @m-ou-se! It looks like we ended up needing a precise cfg for this after all.

So it looks like we added target_has_atomic_load_store in #65214 without going through a fresh RFC (it was a refactoring of existing functionality though).

This seems like a reasonable addition to the current target_has_atomic feature set to me to gate on platforms where the alignment of an atomic type is the same as its non atomic counterpart.

Having said that, I think @Amanieu is probably the best reviewer for this.

r? @Amanieu

@rust-highfive rust-highfive assigned Amanieu and unassigned KodrAus Sep 20, 2020
RalfJung added a commit to RalfJung/rust that referenced this pull request Sep 21, 2020
…mic-from-mut, r=kodrAus

Revert adding Atomic::from_mut.

This reverts rust-lang#74532, which made too many assumptions about platforms, breaking some things.

Will need to be added later with a better way of gating on proper alignment, without hardcoding cfg(target_arch)s.

---

To be merged if fixing from_mut (rust-lang#76965) takes too long.

r? @ghost
RalfJung added a commit to RalfJung/rust that referenced this pull request Sep 21, 2020
…mic-from-mut, r=kodrAus

Revert adding Atomic::from_mut.

This reverts rust-lang#74532, which made too many assumptions about platforms, breaking some things.

Will need to be added later with a better way of gating on proper alignment, without hardcoding cfg(target_arch)s.

---

To be merged if fixing from_mut (rust-lang#76965) takes too long.

r? @ghost
@joshtriplett
Copy link
Member

Adding this new feature-gated cfg for atomic alignment seems perfectly reasonable to me from a language perspective.

Copy link
Member

@Amanieu Amanieu left a comment

Choose a reason for hiding this comment

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

Could you add a compile-fail ui test to ensure that from_mut is not available on x86?

library/core/src/sync/atomic.rs Outdated Show resolved Hide resolved
@bors
Copy link
Contributor

bors commented Sep 21, 2020

☔ The latest upstream changes (presumably #77013) made this pull request unmergeable. Please resolve the merge conflicts.

Note that reviewers usually do not review pull requests until merge conflicts are resolved! Once you resolve the conflicts, you should change the labels applied by bors to indicate that your PR is ready for review. Post this as a comment to change the labels:

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@m-ou-se
Copy link
Member Author

m-ou-se commented Sep 21, 2020

Could you add a compile-fail ui test to ensure that from_mut is not available on x86?

Sure. Working on it.

@rustbot modify labels: -S-waiting-on-review +S-waiting-on-author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 21, 2020
@m-ou-se
Copy link
Member Author

m-ou-se commented Sep 21, 2020

I've added the test. I made it specific for both x86 and linux, because some other x86 platforms do use 64 bit alignment for 64 bit integers:

        target            u64 alignment
i686-unknown-linux-gnu         32
i586-unknown-linux-gnu         32
i686-unknown-linux-musl        32
i586-unknown-linux-musl        32
i686-linux-android             32
i686-unknown-freebsd           32
i686-unknown-openbsd           32
i686-unknown-netbsd            32
i686-unknown-haiku             32
i686-apple-darwin              32
i686-pc-windows-gnu               64
i686-uwp-windows-gnu              64
i686-pc-windows-msvc              64
i686-uwp-windows-msvc             64
i586-pc-windows-msvc              64
i686-unknown-cloudabi          32
i686-unknown-uefi                 64
i686-wrs-vxworks               32

But still, if this test ever happens to run on some x86 Linux target with fully aligned u64s, it'll fail. But that shouldn't affect people cross compiling libcore themselves (-Zbuld-std), as this is a ui test.

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 21, 2020
@Amanieu
Copy link
Member

Amanieu commented Sep 21, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Sep 21, 2020

📌 Commit bcc1d56 has been approved by Amanieu

@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 21, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Sep 22, 2020
…-from-mut, r=Amanieu

Add cfg(target_has_atomic_equal_alignment) and use it for Atomic::from_mut.

Fixes some platform-specific problems with rust-lang#74532 by using the actual alignment of the types instead of hardcoding a few `target_arch`s.

r? @RalfJung
This was referenced Sep 22, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 23, 2020
Rollup of 9 pull requests

Successful merges:

 - rust-lang#76898 (Record `tcx.def_span` instead of `item.span` in crate metadata)
 - rust-lang#76939 (emit errors during AbstractConst building)
 - rust-lang#76965 (Add cfg(target_has_atomic_equal_alignment) and use it for Atomic::from_mut.)
 - rust-lang#76993 (Changing the alloc() to accept &self instead of &mut self)
 - rust-lang#76994 (fix small typo in docs and comments)
 - rust-lang#77017 (Add missing examples on Vec iter types)
 - rust-lang#77042 (Improve documentation for ToSocketAddrs)
 - rust-lang#77047 (Miri: more informative deallocation error messages)
 - rust-lang#77055 (Add #[track_caller] to more panicking Cell functions)

Failed merges:

r? `@ghost`
@bors bors merged commit eaaf5d7 into rust-lang:master Sep 24, 2020
@rustbot rustbot added this to the 1.48.0 milestone Sep 24, 2020
@m-ou-se m-ou-se deleted the fix-atomic-from-mut branch December 30, 2020 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants