Fix race condition in feature cache on 32 platforms #837
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @gnzlbg (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
Oh, this is another spin on #811 actually! But this version of the seems simpler. |
|
Can you please try to rebase and see if now the tests pass? |
matklad
force-pushed
the
fix-data-race
branch
from
19fa496
to
d7c4c0f
Compare
|
rebased! |
There was a problem hiding this comment.
It looks fine to me, @gnzlbg and @alexcrichton do you agree?
| /// Feature cache with capacity for `CACHE_CAPACITY` features. | ||
| /// This global variable is a cache of the features supported by the CPU. | ||
| #[cfg(target_pointer_width = "32")] | ||
| static CACHE: [Cache; 2] = [Cache::uninitialized(), Cache::uninitialized()]; |
There was a problem hiding this comment.
Instead of defining this differently on each platform, could this perhaps be unified a bit like so?
#[cfg(target_pointer_width = "64")]
static CACHE: [Cache; 1] = [Cache::uninitialized()];
#[cfg(target_pointer_width = "32")]
static CACHE: [Cache; 2] = [Cache::uninitialized(), Cache::uninitialized()];
I think that'd help cut down on the #[cfg] traffic
matklad
force-pushed
the
fix-data-race
branch
from
d7c4c0f
to
34b6e63
Compare
| /// Feature cache with capacity for `CACHE_CAPACITY` features. | ||
| /// This global variable is a cache of the features supported by the CPU. | ||
| #[cfg(target_pointer_width = "32")] | ||
| static CACHE: [Cache; 2] = [Cache::uninitialized(), Cache::uninitialized()]; |
matklad
force-pushed
the
fix-data-race
branch
3 times, most recently
from
7125312
to
4418cda
Compare
| #[cfg(target_pointer_width = "32")] | ||
| const CACHE_BITS: u32 = 31; | ||
| #[cfg(target_pointer_width = "64")] | ||
| const CACHE_BITS: u32 = 63; |
There was a problem hiding this comment.
Could this constant either be removed or defined in terms of mem::size_of::<usize>() instead of using #[cfg]?
| @@ -194,8 +171,18 @@ pub(crate) fn test<F>(bit: u32, f: F) -> bool | |||
| where | |||
| F: FnOnce() -> Initializer, | |||
| { | |||
| if CACHE.is_uninitialized() { | |||
| initialize(f()); | |||
| #[cfg(target_pointer_width = "32")] | |||
There was a problem hiding this comment.
I think this #[cfg] could be avoided and the math here could be done with mem::size_of::<usize>() as well
| #[inline] | ||
| fn do_initialize(value: Initializer) { | ||
| CACHE[0].initialize((value.0) as usize & CACHE_MASK); | ||
| #[cfg(target_pointer_width = "32")] |
There was a problem hiding this comment.
Like elsewhere, can the #[cfg] be removed here to work with shifts instead?
matklad
force-pushed
the
fix-data-race
branch
4 times, most recently
from
c978abc
to
b5eb5f9
Compare
If we observe that the second word is initialized, we can't really assume that the first is initialized as well. So check each word separately.
Better SeqCst than sorry!
matklad
force-pushed
the
fix-data-race
branch
from
b5eb5f9
to
22231ea
Compare
|
I've tried to get rid of cfgs using clever indexing, but unfortunately rustc was just smart enough to figure out that I try to index out of bounds into array and just dumb enough to not realize that it's dead code :) In the last commit, I've just made both paths use two slots. |
matklad
force-pushed
the
fix-data-race
branch
from
65b65b8
to
9b9cf9f
Compare
|
👍 |
If we observe that the second word is initialized, we can't really
assume that the first is initialized as well. So check each word
separately.
r? @gnzlbg
I've found this by just looking at the code, and I didn't actually test this (
cargo test --target i686-unknown-linux-gnufailed with some build.rs error which I haven't looked into).