Closed michaelgrigoryan25 closed 1 year ago
but the Stacked Borrows rules it violated are still experimental
Note: the stacked borrow rules are still pretty experimental. I haven't investigated the issue further, but if someone makes the effort they should keep in mind that this could be a false positive.
Definitely could be a false positive, agree.
I tried this quickly yesterday and got the same error on *self.ptr.add(index)
in array_utils.rs
, but called from a completely different place.
The stuff in array_utils.rs
is quite simple, don't really see how it could manage to do something unexpected. Especially while still producing the correct results in all tests.
Is this running within miri? If so, it's possible that we're relying on some undefined behavior that ends up producing the correct result in final compiled code.
One obvious source of undefined behavior is that we deliberately create an aliasing mutable pointer. butterflies.rs line 16:
pub(crate) unsafe fn perform_fft_butterfly(&self, buffer: &mut [Complex<T>]) {
self.perform_fft_contiguous(RawSlice::new(buffer), RawSliceMut::new(buffer));
}
I wanted to reuse the same code for inplace and out of place butterflies, since aside from which pointer you're writing to it's always the same logic. So this little bit has been chugging away for 2 and a half years to just pass a single pointer to the underlying code as both the immutable input and mutable output. That way, the in-place transform can go through this path, and the out-of-place transform can directly call self.perform_fft_contiguous
.
It's a night-and-day convenience improvement, but I wouldn't be the least bit surprised if this was the underlying problem miri was complaining about. If so, we should find a way to move away from this code.
Is this running within miri? If so, it's possible that we're relying on some undefined behavior that ends up producing the correct result in final compiled code.
Exactly. This code was indeed running inside of miri.
Is this running within miri? If so, it's possible that we're relying on some undefined behavior that ends up producing the correct result in final compiled code.
I'm not sure this is valid reasoning. If things are "working" during UB for a set of tests, on a set of platforms for a specific configurations, it does not mean it will work for other untested tests, platforms or configurations. From the wiki:
In computer programming, undefined behavior (UB) is the result of executing a program whose behavior is prescribed to be unpredictable, in the language specification to which the computer code adheres. This is different from unspecified behavior, for which the language specification does not prescribe a result, and implementation-defined behavior that defers to the documentation of another component of the platform (such as the ABI or the translator documentation).
I believe you're arguing that the behaviour is "defined" by the tests, even though the compiler hits something it determines as UB. AFAIK it's much more complicated than that.
UB is an indication that some invariant IS being broken. It working for a set of specific configurations does not rule out all configurations. It could be that it will produce incorrect results when:
I'm not convinced that this is not a false positive though. But if this IS UB, then it's definitely something that should be addressed.
To add to @WalterSmuts's comments, this might also be unsound behavior (though I highly doubt that miri would have reported such a case). From "Behavior considered undefined" in Rust's documentation:
unsafe
code that satisfies this property for any safe client is called sound; ifunsafe
code can be misused by safe code to exhibit undefined behavior, it is unsound.
Deterministic compilation is not quite there yet, but there is a moderate amount of work being put into it. Check out https://github.com/rust-lang/rust/labels/A-reproducibility.
I've looked a bit into this and it stems from the RawSlice
and RawSliceMut
structs in the array_utils
module. From the documentation:
/// A RawSliceMut is a normal mutable slice, but aliasable. Its functionality is severely limited.
#[derive(Copy, Clone)]
pub struct RawSliceMut<T> {
ptr: *mut T,
slice_len: usize,
}
This seems to intentionally violate a pretty fundamental invariant in rust. AFAICT it's to de-duplicate the logic that does the in-place and out of place fft's.
I'm open to fixing this. I'm resistant to (but still not entirely ruling out) re-duplicating all of that code, because of the maintenance burden it'd place on the butterfly portion of the project. But if there's another solution I'll gladly take it.
I don't think this issue should be closed.
@ejmahler I believe I've figured out a good solution to remove the usage of RawSlice and RawSliceMut from butterflies.rs and put together PR #113. Please let me know what you think!
PR merged! Thanks to @michaelgrigoryan25 for reporting this, thanks to @WalterSmuts and @HEnquist for contributing some research, and thanks to @groscoe2 for implementing the solution!
The
rustfft::accuracy test_planned_fft_forward_f32
reports undefined behavior, and from what it seems like, it is coming frombutterfles.rs
. Here is the full stacktrace generated by rust-lang/miri: