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

Handle Win64 builtins ABI change in LLVM 14 #455

Merged
merged 1 commit into from
Feb 16, 2022

Conversation

nikic
Copy link
Contributor

@nikic nikic commented Feb 13, 2022

As of https://reviews.llvm.org/D110413, these no longer use the unadjusted ABI (and use normal C ABI instead, passing i128 indirectly and returning it as <2 x i64>).

To support both LLVM 14 and older versions, rustc will expose a "llvm14-builtins-abi" target feature, based on which compiler-builtins can chose the appropriate ABI.

This is needed for rust-lang/rust#93577.

@Mark-Simulacrum
Copy link
Member

Doesn't this need to be cfg gated somehow to retain support for older llvms?

@nikic
Copy link
Contributor Author

nikic commented Feb 13, 2022

@Mark-Simulacrum Possibly. I assumed this doesn't matter for Windows, where distros linking against system LLVM are not a thing.

@nikic
Copy link
Contributor Author

nikic commented Feb 13, 2022

If we want this gated on LLVM version, would it be acceptable to call $RUSTC -vV from build.rs and parse the LLVM version from there?

@Mark-Simulacrum
Copy link
Member

Hm. Probably not, unfortunately. The RUSTC available to build scripts may differ from that used to actually compile the code, particularly for std dependencies (since build scripts link to std themselves, we have to be a little fancy there).

We could expose an unstable target feature like "float-abi-llvm-14" or something that is used here, though, from rustc? Presumably if we cfg_attr on that, then we get the right behavior without needing build script hacks or similar, but also not exposing any new stable surface area from rustc. Once we bump min LLVM to 14, it could be dropped.

@mati865
Copy link

mati865 commented Feb 13, 2022

@Mark-Simulacrum Possibly. I assumed this doesn't matter for Windows, where distros linking against system LLVM are not a thing.

It's niche but does exist, at MSYS2 we link to LLVM 13 right now but plan to move to LLVM 14 as soon as it's released.
Another use case I can think of is cross compiling, Fedora does build multiple targets (including mingw-w64) for their system Rust package.

@nikic nikic changed the title Remove #[unadjusted_on_win64] from __float functions Handle Win64 builtins ABI change in LLVM 14 Feb 15, 2022
// use Win64 ABI rather than unadjusted ABI. Pick the correct ABI based on the
// llvm14-builtins-abi target feature.

#[cfg(target_feature = "llvm14-builtins-abi")]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this target feature be behind a feature gate?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently, we don't feature-gate #[cfg(target_feature)] (though we do gate #[target_feature]). cfg(target_feature) exposes all unstable target features on nightly builds.

@nikic
Copy link
Contributor Author

nikic commented Feb 15, 2022

@Mark-Simulacrum Is this plus rust-lang/rust@28423ca on the rustc side what you had in mind?

@Mark-Simulacrum
Copy link
Member

Yeah, this seems to match my expectation.

One thought I do have: since out-of-tree users of compiler-builtins (https://crates.io/crates/compiler_builtins/reverse_dependencies, at least) are using this crate directly, there's an unfortunate situation where we're basically forced to leave this cfg gate in indefinitely unless we bump the MSRV for the crate to a new enough rustc -- I'm not sure what our policies there look like.

As of https://reviews.llvm.org/D110413, these no longer use the
unadjusted ABI (and use normal C ABI instead, passing i128
indirectly and returning it as <2 x i64>).

To support both LLVM 14 and older versions, rustc will expose a
"llvm14-builtins-abi" target feature, based on which
compiler-builtins can chose the appropriate ABI.

This is needed for rust-lang/rust#93577.
@nikic
Copy link
Contributor Author

nikic commented Feb 15, 2022

@Mark-Simulacrum From the few I looked at, it looks like direct users of compiler_builtins are due to rustc-dep-of-std.

@Amanieu
Copy link
Member

Amanieu commented Feb 15, 2022

Also compiler-builtins only supports nightly Rust anyways.

@nikic
Copy link
Contributor Author

nikic commented Feb 15, 2022

The msvc try build with this compiler-builtins succeeded, so this is ready for review now.

@Amanieu Amanieu merged commit 4f114dc into rust-lang:master Feb 16, 2022
@Amanieu
Copy link
Member

Amanieu commented Feb 16, 2022

Published in compiler-builtins 0.1.70.

nikic added a commit to nikic/rust that referenced this pull request Feb 16, 2022
This pulls in rust-lang/compiler-builtins#455,
which exports __float/__fix builtins with the expected Win64 ABI
on LLVM 14.
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

Successfully merging this pull request may close these issues.

5 participants