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

Compile test_num_f128 conditionally on reliable_f128_math config #132696

Merged
merged 1 commit into from
Nov 8, 2024
Merged

Compile test_num_f128 conditionally on reliable_f128_math config #132696

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
22 changes: 20 additions & 2 deletions library/std/src/f128/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@
#![cfg(reliable_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.

This doesn't seem to work. I disregarded that as I wanted to have a minimal PR. Should I remove this, move the conditional compilation to the mod tests; statement (but that's even easier for developers to miss), or use a mod statement within this tests module with a proper single conditional compilation statement?

Copy link
Contributor

@tgross35 tgross35 Nov 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

That config is set here

rust/library/std/build.rs

Lines 120 to 143 in 4d215e2

let has_reliable_f128 = match (target_arch.as_str(), target_os.as_str()) {
// We can always enable these in Miri as that is not affected by codegen bugs.
_ if is_miri => true,
// Unsupported <https://github.com/llvm/llvm-project/issues/94434>
("arm64ec", _) => false,
// Selection bug <https://github.com/llvm/llvm-project/issues/96432>
("mips64" | "mips64r6", _) => false,
// Selection bug <https://github.com/llvm/llvm-project/issues/95471>
("nvptx64", _) => false,
// ABI bugs <https://github.com/rust-lang/rust/issues/125109> et al. (full
// list at <https://github.com/rust-lang/rust/issues/116909>)
("powerpc" | "powerpc64", _) => false,
// ABI unsupported <https://github.com/llvm/llvm-project/issues/41838>
("sparc", _) => false,
// Stack alignment bug <https://github.com/llvm/llvm-project/issues/77401>. NB: tests may
// not fail if our compiler-builtins is linked.
("x86", _) => false,
// MinGW ABI bugs <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115054>
("x86_64", "windows") if target_env == "gnu" && target_abi != "llvm" => false,
// There are no known problems on other platforms, so the only requirement is that symbols
// are available. `compiler-builtins` provides all symbols required for core `f128`
// support, so this should work for everything else.
_ => true,
};
and at this point should really only cover platforms where LLVM crashes or ABI problems lead to buggy results. reliable_f128_math is the gate that should cover anything that relies on libm symbols. I think we just didn't catch this because fmodl is available on many platforms even when the rest of the math symbols might not be.

So moving only calls to functions that rely on f128 libm to reliable_f128_math should be the needed correction (suggested by @beetrees below), everything else should continue to work on SGX and other platforms as long as it isn't in the linked list.


use crate::f128::consts;
use crate::num::{FpCategory as Fp, *};
use crate::num::FpCategory as Fp;
#[cfg(reliable_f128_math)]
use crate::ops::Rem;
use crate::ops::{Add, Div, Mul, Sub};

// Note these tolerances make sense around zero, but not for more extreme exponents.

Expand Down Expand Up @@ -53,7 +56,22 @@ macro_rules! assert_f128_biteq {

#[test]
fn test_num_f128() {
test_num(10f128, 2f128);
// FIXME(f16_f128): replace with a `test_num` call once the required `fmodl`/`fmodf128`
// function is available on all platforms.
let ten = 10f128;
let two = 2f128;
assert_eq!(ten.add(two), ten + two);
assert_eq!(ten.sub(two), ten - two);
assert_eq!(ten.mul(two), ten * two);
assert_eq!(ten.div(two), ten / two);
}

#[test]
#[cfg(reliable_f128_math)]
fn test_num_f128_rem() {
let ten = 10f128;
raoulstrackx marked this conversation as resolved.
Show resolved Hide resolved
let two = 2f128;
assert_eq!(ten.rem(two), ten % two);
}

#[test]
Expand Down
Loading