Appease miri #515
Appease miri #515
Conversation
Noah-Kennedy
force-pushed
the
noah/appease-miri
branch
from
6ca8cbf
to
f9a86b3
Compare
|
Looks like the nightly job is failing because of the very old version bytes/.github/workflows/ci.yml Line 14 in ebc61e5 |
|
Yeah that would do it |
|
I'll change it to a more recent one |
|
Fixed an issue with the tests that was causing CI to hang (not sure why it didn't fail outright). |
tests/test_bytes_vec_alloc.rs
Outdated
| // equivalent to size of (AtomicPtr<u8>, AtomicUsize), hopefully | ||
| #[cfg(target_pointer_width = "64")] | ||
| let tricky_bits = 0u128; |
There was a problem hiding this comment.
Stupid GitHub UI not allowing suggestions across deleted lines
| // equivalent to size of (AtomicPtr<u8>, AtomicUsize), hopefully | |
| #[cfg(target_pointer_width = "64")] | |
| let tricky_bits = 0u128; |
tests/test_bytes_vec_alloc.rs
Outdated
| #[cfg(target_pointer_width = "32")] | ||
| let tricky_bits = 0u64; | ||
|
|
||
| let magic_table = [tricky_bits; 512]; | ||
|
|
||
| // i know this looks bad but all the good ways to do this are unstable or not yet | ||
| // supported in const contexts (even though they should be!) | ||
| let alloc_table = unsafe { mem::transmute(magic_table) }; |
There was a problem hiding this comment.
| #[cfg(target_pointer_width = "32")] | |
| let tricky_bits = 0u64; | |
| let magic_table = [tricky_bits; 512]; | |
| // i know this looks bad but all the good ways to do this are unstable or not yet | |
| // supported in const contexts (even though they should be!) | |
| let alloc_table = unsafe { mem::transmute(magic_table) }; | |
| const ELEM: (AtomicPtr<u8>, AtomicUsize) = | |
| (AtomicPtr::new(null_mut()), AtomicUsize::new(0)); | |
| let alloc_table = [ELEM; 512]; |
There was a problem hiding this comment.
The problem is that they aren't Copy according to Rustc
There was a problem hiding this comment.
It appears to work correctly if you put the element in a const, like I did in the suggestion
There was a problem hiding this comment.
I'll try that once this CI run finishes
|
Still failing, weird, works on my machine. |
tests/test_bytes_vec_alloc.rs
Outdated
| .compare_exchange(ptr, null_mut(), Ordering::SeqCst, Ordering::SeqCst) | ||
| .is_ok() | ||
| { | ||
| return entry_size.swap(0, Ordering::Relaxed); |
There was a problem hiding this comment.
This swap might be problematic. Using load should be enough. The entry_size will we overwritten when the next allocation is done
There was a problem hiding this comment.
I realize it isn't needed because we already zero the ptr, but I don't see how this would cause issues.
There was a problem hiding this comment.
This is a test so after all going down this level of detail might not make sense, but I think it's totally possible that between the compare_exchange and the swap another thread allocates. swap could then zero the entry_size of the next allocation.
IDK just mentioned it because I also noticed the hang with the CI, it would probably never happen with this very small test
There was a problem hiding this comment.
Ah, good catch! The hang was because I wasn't removing it because I wasn't handling realloc cases correctly.
There was a problem hiding this comment.
I think even a load would still have issues, as we could read the allocation from the other thread. I suppose setting this to the max value would be better.
There was a problem hiding this comment.
Might wanna make the ledger slightly bigger just in case.
There was a problem hiding this comment.
I think even a load would still have issues, as we could read the allocation from the other thread
True 😃
There was a problem hiding this comment.
Thanks, I think this should be fixed in my latest commit!
|
|
|
@paolobarbolini thanks for the review and help! |
Co-authored-by: Alice Ryhl <alice@ryhl.io>
Rewrote the ledger in test_bytes_vec_alloc.rs to not piss off miri. The ledger is now a table within the allocator, which seems to satisfy miri. The old solution was to bundle an extra usize into the beginning of each allocation and then index past the start when deallocating data to get the size.
Rewrote the ledger in
test_bytes_vec_alloc.rsto not piss off miri. The ledger is now a table within the allocator, which seems to satisfy miri. The old solution was to bundle an extra usize into the beginning of each allocation and then index past the start when deallocating data to get the size.There is one janky bit here with a transmute, but there isn't a better way to initialize an array with zeroed data in a const context that I am aware of sadly.Thanks to @paolobarbolini for finding a better way to do this!