-
Notifications
You must be signed in to change notification settings - Fork 278
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
Appease miri #515
Appease miri #515
Conversation
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). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this PR already captured my attention, even though I'm not an official reviewer, I saw the const fn
limitation and there actually is a simpler way of constructing an array of Copy
values
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem is that they aren't Copy according to Rustc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try that once this CI run finishes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, works
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How so?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might wanna make the ledger slightly bigger just in case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I think this should be fixed in my latest commit!
|
@paolobarbolini thanks for the review and help! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems fine to me.
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.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.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!