search

tokio-rs / bytes

Utilities for working with bytes
MIT License
1.91k stars 288 forks source link

Drop separate miri codepath for ptr_map when LLVM issues are fixed #565

Open RalfJung opened 2 years ago

RalfJung commented 2 years ago

ptr_map currently has a separate codepath for Miri, which is always dicy since it means the codepath used 'for real' is not actually checked by Miri. (FWIW that codepath would work fine in Miri these days, but it would lead to warnings about int2ptr casts.) So it might make sense to track removing that temporary hack when it is no longer needed.

Judging from https://github.com/rust-lang/rust/issues/96152, once Rust uses LLVM 15, the code used for Miri should also produce good runtime code, hopefully making the hack no longer needed.

Darksonn commented 2 years ago

While I agree it would be good to avoid this kind of thing, we currently have an MSRV of 1.39.0. As long as we support compilers that miscompile the version used by miri, we will need to keep the separate codepath.

saethlin commented 2 years ago

I think there might be some confusion here. There are two ways to write with_addr, the one in std and sptr which uses a single wrapping_offset and one that I came up with which uses wrapping_sub + wrapping_add. The latter optimizes "better" but also seems to miscompile. You can read all about this little debacle here: https://github.com/tokio-rs/bytes/pull/542

I think we are in the clear with respect to miscompilation regardless of rustc/LLVM version because tokio doesn't use the wrapping_sub + wrapping_add pattern. So I think the question is whether tokio is okay with the 1 instruction of overhead.

As far as I know nikic hunted down all the missing optimizations with the wrapping_offset pattern, and mentions those efforts here: https://github.com/rust-lang/rust/issues/96152. I do not know if the miscompilation with the wrapping_sub + wrapping_addr pattern was reported or fixed. Based on how incredibly dramatic the miscompilation was I think there's probably no LLVM but we could run into with the wrapping_offset version, we should have noticed by now if there were.

RalfJung commented 2 years ago

I do not know if the miscompilation with the wrapping_sub + wrapping_addr pattern was reported or fixed.

Nikic fixed it, I think: https://github.com/rust-lang/rust/pull/96538#issuecomment-1113453398

So, yeah, the consequence of using the Miri version for old versions of rustc is slightly worse codegen, but there are no known miscompilations.

Darksonn commented 2 years ago

Ah, right, I forgot that the version we're currently using doesn't miscompile. I'm more open to dropping the separate codepath once the past few stable releases compile it properly.