Skip to content

Add missing atomic_fence intrinsics as nops #979

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

Merged
merged 2 commits into from
Oct 5, 2019

Conversation

nico-abram
Copy link
Contributor

Fixes #972

@oli-obk
Copy link
Contributor

oli-obk commented Oct 5, 2019

Please add a test calling the stable function from std::sync, so we have some code actually invoking these intrinsics

@nico-abram
Copy link
Contributor Author

nico-abram commented Oct 5, 2019

Would adding a function like atomic_fences to https://github.com/rust-lang/miri/blob/master/tests/run-pass/atomic.rs using https://doc.rust-lang.org/std/sync/atomic/fn.fence.html with all Orderings be enough?

@oli-obk
Copy link
Contributor

oli-obk commented Oct 5, 2019

Yes, definitely

@nico-abram
Copy link
Contributor Author

nico-abram commented Oct 5, 2019

Force pushed, fence(Relaxed) panics (https://doc.rust-lang.org/std/sync/atomic/fn.fence.html#panics) so trying to test that was a bad idea

@oli-obk
Copy link
Contributor

oli-obk commented Oct 5, 2019

@bors r+

@bors
Copy link
Contributor

bors commented Oct 5, 2019

📌 Commit bd4a299 has been approved by oli-obk

@bors
Copy link
Contributor

bors commented Oct 5, 2019

⌛ Testing commit bd4a299 with merge de4a846...

bors added a commit that referenced this pull request Oct 5, 2019
Add missing atomic_fence intrinsics as nops

Fixes #972
@bors
Copy link
Contributor

bors commented Oct 5, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: oli-obk
Pushing de4a846 to master...

@bors bors merged commit bd4a299 into rust-lang:master Oct 5, 2019
fn atomic_fences() {
fence(SeqCst);
fence(Release);
fence(Acquire);
Copy link
Member

Choose a reason for hiding this comment

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

This tests 3 fences but you added 4 intrinsics... I will add a test for the AcqRel ordering.

@RalfJung RalfJung mentioned this pull request Oct 9, 2019
bors added a commit that referenced this pull request Oct 9, 2019
also test AcqRel fence

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

Successfully merging this pull request may close these issues.

Support atomic_fence instrinstic
4 participants