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.

Sign up for GitHub

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

Jump to bottom

Only allow using the atomic intrinsics on integer types #32647

Merged
merged 2 commits into from
Apr 5, 2016
Merged

Only allow using the atomic intrinsics on integer types #32647

merged 2 commits into from
Apr 5, 2016

Conversation

Amanieu
Copy link
Member

@Amanieu Amanieu commented Mar 31, 2016

Using these with non-integer types results in LLVM asserts. Atomic operations on non-integer types will require values be transmuted into an integer type of suitable size.

This doesn't affect the standard library since AtomicBool and AtomicPtr currently use usize for atomic operations.

r? @eddyb

@eddyb
Copy link
Member

eddyb commented Mar 31, 2016

Can you add a compile-fail test for the errors?

@Amanieu Amanieu force-pushed the checked_atomic_intrinsics branch from b572125 to 9cc968a Compare March 31, 2016 14:28
@Amanieu
Copy link
Member Author

Amanieu commented Mar 31, 2016

Done (and fixed a few bugs)

@Amanieu Amanieu force-pushed the checked_atomic_intrinsics branch from 9cc968a to 2f492d8 Compare March 31, 2016 14:55
@Amanieu
Copy link
Member Author

Amanieu commented Mar 31, 2016

Updated to fix make check complaints about line length

@Amanieu Amanieu force-pushed the checked_atomic_intrinsics branch from 2f492d8 to c3668f9 Compare March 31, 2016 15:03
@Amanieu
Copy link
Member Author

Amanieu commented Mar 31, 2016

Fixed more line lengths...

@eddyb
Copy link
Member

eddyb commented Mar 31, 2016

@bors r+

@bors
Copy link
Contributor

bors commented Mar 31, 2016

📌 Commit c3668f9 has been approved by eddyb

@Manishearth Manishearth mentioned this pull request Apr 1, 2016
Merged
bors added a commit that referenced this pull request Apr 2, 2016
@bors
Auto merge of #32666 - Manishearth:rollup, r=Manishearth
18172d1 
Rollup of 11 pull requests

- Successful merges: #32622, #32629, #32640, #32641, #32642, #32645, #32647, #32649, #32652, #32654, #32656
- Failed merges:
@bors
Copy link
Contributor

bors commented Apr 2, 2016

⌛ Testing commit c3668f9 with merge 434c58e...

@bors
Copy link
Contributor

bors commented Apr 2, 2016

💔 Test failed - auto-mac-64-nopt-t

@Amanieu Amanieu force-pushed the checked_atomic_intrinsics branch from c3668f9 to d96bacc Compare April 2, 2016 23:27
@Amanieu
Copy link
Member Author

Amanieu commented Apr 2, 2016

Fixed the test

@alexcrichton
Copy link
Member

@bors: r+ d96bacce0752868994e168dcdd8b7e0d9f934f35

@bors
Copy link
Contributor

bors commented Apr 3, 2016

⌛ Testing commit d96bacc with merge 8bbd3d5...

@bors
Copy link
Contributor

bors commented Apr 3, 2016

💔 Test failed - auto-linux-64-opt-mir

@eddyb
Copy link
Member

eddyb commented Apr 3, 2016

@Amanieu ah, you're hitting the issue where MIR lacked precise spans. Now they're there, but not used.
If you want to fix it, have trans_intrinsic_call take a span: Span argument instead of extracting it from call_debug_location, and pass data.terminator().span for that argument, from MirContext::trans_block.
You can then also remove #[rustc_no_mir] from src/test/compile-fail/simd-intrinsic-*.rs.
Alternatively, you can just do what I did in those tests and we'll fix this when we implement debuginfo for MIR.

@Amanieu Amanieu force-pushed the checked_atomic_intrinsics branch from d96bacc to 598e122 Compare April 3, 2016 21:11
@Amanieu
Copy link
Member Author

Amanieu commented Apr 3, 2016

@eddyb I went with the easy option for now...

eddyb
eddyb reviewed Apr 3, 2016
View reviewed changes
src/test/compile-fail/non-interger-atomic.rs
type Bar = &'static Fn();
type Quux = [u8; 100];

#[rustc_no_mir] // FIXME #27840 MIR doesn't provide precise spans for calls.
Copy link
Member

Choose a reason for hiding this comment

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

You need #[feature(rustc_attrs)] to use this attribute.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, fixed.

@Amanieu Amanieu force-pushed the checked_atomic_intrinsics branch from 598e122 to 8620bba Compare April 3, 2016 22:14
@eddyb
Copy link
Member

eddyb commented Apr 3, 2016

@bors r+

@bors
Copy link
Contributor

bors commented Apr 3, 2016

📌 Commit 8620bba has been approved by eddyb

@bors
Copy link
Contributor

bors commented Apr 4, 2016

⌛ Testing commit 8620bba with merge f8ad947...

bors added a commit that referenced this pull request Apr 4, 2016
@bors
Auto merge of #32647 - Amanieu:checked_atomic_intrinsics, r=eddyb
f8ad947 
Only allow using the atomic intrinsics on integer types

Using these with non-integer types results in LLVM asserts. Atomic operations on non-integer types will require values be transmuted into an integer type of suitable size.

This doesn't affect the standard library since `AtomicBool` and `AtomicPtr` currently use `usize` for atomic operations.

r? @eddyb
@bors
Copy link
Contributor

bors commented Apr 4, 2016

💔 Test failed - auto-win-msvc-64-opt-mir

@eddyb
Copy link
Member

eddyb commented Apr 4, 2016

@bors p=1 retry

@eddyb
Copy link
Member

eddyb commented Apr 4, 2016

@bors force
Looks like a timeout. Doing this to avoid building everything again.

@bors
Copy link
Contributor

bors commented Apr 4, 2016

⚡ Previous build results for auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-mir, auto-linux-64-opt-rustbuild, auto-linux-64-x-android-t, auto-linux-64-x-freebsd, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-64-nopt-t, auto-mac-ios-opt, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-64-opt, auto-win-msvc-64-opt-rustbuild are reusable. Rebuilding only auto-linux-64-debug-opt, auto-linux-64-x-armhf, auto-linux-64-x-armsf, auto-linux-64-x-netbsd, auto-mac-32-opt, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-msvc-32-opt, auto-win-msvc-64-opt-mir...

@bors
Copy link
Contributor

bors commented Apr 4, 2016

💥 Test timed out

@alexcrichton
Copy link
Member

@bors: retry clean

@bors
Copy link
Contributor

bors commented Apr 4, 2016

⌛ Testing commit 8620bba with merge b207e22...

@bors
Copy link
Contributor

bors commented Apr 4, 2016

💔 Test failed - auto-win-msvc-64-opt-rustbuild

@alexcrichton
Copy link
Member

@bors: retry

On Mon, Apr 4, 2016 at 11:41 AM, bors [email protected] wrote:

[image: 💔] Test failed - auto-win-msvc-64-opt-rustbuild
http://buildbot.rust-lang.org/builders/auto-win-msvc-64-opt-rustbuild/builds/582

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#32647 (comment)

@bors
Copy link
Contributor

bors commented Apr 4, 2016

⌛ Testing commit 8620bba with merge 144f980...

@alexcrichton
Copy link
Member

@bors: retry force clean

@bors
Copy link
Contributor

bors commented Apr 4, 2016

⌛ Testing commit 8620bba with merge 600dc35...

bors added a commit that referenced this pull request Apr 4, 2016
@bors
Auto merge of #32647 - Amanieu:checked_atomic_intrinsics, r=eddyb
600dc35 
Only allow using the atomic intrinsics on integer types

Using these with non-integer types results in LLVM asserts. Atomic operations on non-integer types will require values be transmuted into an integer type of suitable size.

This doesn't affect the standard library since `AtomicBool` and `AtomicPtr` currently use `usize` for atomic operations.

r? @eddyb
@bors
Copy link
Contributor

bors commented Apr 5, 2016

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-opt-mir, auto-linux-64-opt-rustbuild, auto-linux-64-x-android-t, auto-linux-64-x-armhf, auto-linux-64-x-armsf, auto-linux-64-x-freebsd, auto-linux-64-x-netbsd, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-mac-ios-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-32-opt-rustbuild, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt, auto-win-msvc-64-opt-mir, auto-win-msvc-64-opt-rustbuild

@bors bors merged commit 8620bba into rust-lang:master Apr 5, 2016
This was referenced May 12, 2016
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@eddyb eddyb

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Amanieu @eddyb @bors @alexcrichton