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.

Sign up for GitHub

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

Jump to bottom

Enable f16 tests on non-GNU Windows #130959

Merged
merged 1 commit into from
Oct 1, 2024
Merged

Enable f16 tests on non-GNU Windows #130959

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions library/std/build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ fn main() {
// Unsupported <https://github.com/llvm/llvm-project/issues/94434>
("arm64ec", _) => false,
// MinGW ABI bugs <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115054>
("x86_64", "windows") => false,
("x86_64", "windows") if target_env == "gnu" => false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't it also check for target_abi == "" or target_abi != "llvm" to enable it for gnullvm targets since it works fine with Clang?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do gnullvm targets avoid linking MinGW/GCC libraries at all? I'm honestly not positive how those works.

If you are suggesting updating the gate to if target_env == "gnu" && target_abi == "" => false I can do that after figuring out what is currently failing.

Copy link
Contributor

Choose a reason for hiding this comment

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

gnullvm targets are based on mingw-w64 just like regular gnu targets but built using the Clang / LLVM toolchain. In https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115054 you've said Clang is using proper ABI, so this feature should be fine. Doesn't have to be done in this PR though.
The same for f128.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clang uses the correct ABI, but the problem is it can link symbols from libgcc which means things wind up broken. Maybe this isn't the case for gnullvm for some reason?

In any case I'll plan to leave this PR as-is for now unless you are able to check that things work on gnullvm (or I'll try to do that at some point after this merges).

Copy link
Contributor

Choose a reason for hiding this comment

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

gnullvm targets don't use anything from GCC/GNU, so this is not a problem. FWIW libgcc is replaced by compiler-rt and libgcc_{s,eh} is replaced by libunwind.

I'm trying to tests that, but I have troubles cross-compiling full toolchain because workspace resolver = "2" exposed Cargo bug:

Building tool rustdoc (stage1 -> stage2, x86_64-pc-windows-gnullvm)
thread 'main' panicked at src/cargo\core\resolver\features.rs:323:14:
activated_features for invalid package: features did not find PackageId { name: "windows_x86_64_gnullvm", version: "0.52.6", source: "registry `crates-io`" } HostDep
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

That might get a fix soon: rust-lang/cargo#14593
Otherwise I'll work around it another way, so I'll take care of that change then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh that's really interesting. compiler-rt may have been part of the problem too since GCC was being used to build it - but I don't really remember exactly what I was testing, and sounds like that might not be a problem anyway.

Anyway, totally your discretion since you're probably the main one testing this target :) feel free to r? me if you wind up putting up a followup PR.

Copy link
Contributor

@mati865 mati865 Sep 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

LLVM mingw-w64 toolchains are purely LLVM + mingw-w64 based, no GNU tools like GCC or Binutils are used at any point.

EDIT: feel free to resolve this discussion, my button has disappeared.

// Infinite recursion <https://github.com/llvm/llvm-project/issues/97981>
("csky", _) => false,
("hexagon", _) => false,
Expand Down Expand Up @@ -129,10 +129,10 @@ fn main() {
// ABI unsupported <https://github.com/llvm/llvm-project/issues/41838>
("sparc", _) => false,
// MinGW ABI bugs <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115054>
("x86_64", "windows") => false,
("x86_64", "windows") if target_env == "gnu" => false,
// 64-bit Linux is about the only platform to have f128 symbols by default
(_, "linux") if target_pointer_width == 64 => true,
// Same as for f16, except MacOS is also missing f128 symbols.
// Almost all OSs are missing symbol. compiler-builtins will have to add them.
_ => false,
};

Expand Down
Loading