Skip to content
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

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

Open
RalfJung opened this issue Aug 13, 2022 · 4 comments
Open

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

RalfJung opened this issue Aug 13, 2022 · 4 comments

Comments

@RalfJung
Copy link
Contributor

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 rust-lang/rust#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
Copy link
Contributor

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
Copy link
Contributor

saethlin commented Aug 13, 2022

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: #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: rust-lang/rust#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
Copy link
Contributor Author

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

Nikic fixed it, I think: rust-lang/rust#96538 (comment)

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
Copy link
Contributor

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.

shahn added a commit to shahn/bytes that referenced this issue Sep 21, 2023
Since Rust 1.65, which introduced LLVM 15, codegen for both versions of
ptr_map is the same. The release happened in Nov 2022, which should mean
the majority of users have upgraded.
shahn added a commit to shahn/bytes that referenced this issue Mar 31, 2024
Since Rust 1.65, which introduced LLVM 15, codegen for both versions of
ptr_map is the same. The release happened in Nov 2022, which should mean
the majority of users have upgraded.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants