Casting u128::MAX to f32 is undefined
est31 opened this issue · 22 comments
Given this code (playpen):
#![feature(i128_type,i128)]
fn cast(src: f32) -> Result<u128, &'static str> {
use std::{u128, f32};
Err(if src != src {
"NaN"
} else if src < u128::MIN as f32 {
"Underflow"
} else if src > u128::MAX as f32 {
"Overflow"
} else {
return Ok(src as u128);
})
}
fn main() {
let f = 2742605.0f32;
println!("{:?}", cast(f));
}It should print Ok(2742605). In release mode, that happens, but in debug mode, it prints Err("Overflow").
The value is mostly irrelevant, it would also work for 42.0.
u128::MAX as f32 is undefined. It ends up being as float undef in LLVM. This issue is in a similar vein as the float to integer casts. All we really need is a decision on how to handle this. For u128->f32 it seems pretty obvious to simply produce an infinity.
u64::MAX to f32 converts just fine.
Yes, because the maximum value of a u64 fits in a f32 fine.
Hi! Sorry if this is a stupid suggestion… but as someone who uses u128 (in several cryptographic libraries) and doesn't use floats, I wonder how many u128 users actually need safe casting to floats? Is this something one would do in graphics development? If there isn't much use for it, perhaps a simple fix would be to disallow 42u128 as f32 casts, i.e. they should have to first cast to u64.
If a new as-cast rule is going to be introduced, please make it generic so that it can be naturally extensible to something like mini-floats or GMP types
@isislovecruft Rust is a general purpose programming language and it already has a syntax for doing conversions that one could expect to work on all the primitive types, so it not supporting as-casts for only this particular conversion would make the language inconsistent.
Furthermore, consider a code like this:
macro_rules! foo {
($x: ident) = {
/* some stuff */
$x as f32
/* rest of the stuff */
}
}
Now this macro works just fine for every single integer and float combo. Forbidding it for i/u128 would extend the discrepancy to macros as well. I hope that’s enough of demonstration of a need to keep functionality consistent.
@nagisa The consistency issue, esp. w.r.t. macros, makes perfect sense. Thanks for taking the time to point that out.
u128 as f32 currently generates this LLVM IR:
%1 = uitofp i128 %0 to floatwhich, on x86_64, i686 and aarch64, becomes a call to the compiler_rt function __floatuntisf
call __floatuntisf@PLTDirectly calling the function, instead of generating the uitofp instruction, seems to be able to hide the UB?
The current implementation casts u128::MAX to +∞.
It makes no sense to do that over just adding the relevant branches into the IR, because it inhibits pretty much every optimisation with the values involved.
@nagisa Yes we could do this
if a >= 0xffffff80000000000000000000000000_u128 {
f32::INFINITY
} else {
a as f32
}producing
%1 = icmp ugt i128 %0, -10141204801825835211973625643009
%2 = uitofp i128 %0 to float
%_0.0 = select i1 %1, float 0x7FF0000000000000, float %2producing (unfortunately the function will always be called, and the cold branch check remains)
mov rbx, rsi
shr rbx, 39
call __floatuntisf@PLT
cmp rbx, 33554430 ; 0x1fffffe
jbe .LBB0_2
movss xmm0, dword ptr [rip + .LCPI0_0]
.LBB0_2:
ret
.LCPI0_0:
.long 2139095040 ; 0x7f800000u128::MAX as f32 gives undef during LLVM-level constant-folding with LLVMConstUIToFP. Could the range check placed there as a first step, to prevent a naked undef being generated?
Or just add a lint/error if MirConstContext::trans() produces an undef Const? Or wait for miri?
Could the range check placed there as a first step, to prevent a naked undef being generated?
LLVM will want some solution that’s more general and we want to avoid having custom patches in our LLVM, so I do not think that would work.
Or just add a lint/error if MirConstContext::trans() produces an undef Const?
Interesting option, but does not help with code like
target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"
define internal i128 @foo() unnamed_addr {
start:
ret i128 -1
}
define float @bar() unnamed_addr {
start:
%0 = call i128 @foo()
%1 = uitofp i128 %0 to float
ret float %1
}which is still very UB, even if it does not produce a literal undef at the moment after the optimisations.
Ignoring LLVM and C/C++ for the moment, IEEE 754 section 5.4.1 "Arithmetic operations" defines convertFromInt and says:
If the converted value is not exactly representable in the destination format, the result is determined according to the applicable rounding-direction attribute [...].
Integer to floating-point conversions typically use "to nearest" rounding. IEEE 754 section 4.3.1 "Rounding-direction attributes to nearest" defines "no nearest" and says:
[...] an infinitely precise result with magnitude at least bemax(b − ½b1−p) shall round to ∞ with no change in sign [...].
u128::MAX fits that condition when converted to f32. So by IEEE 754 rules, for what that's worth, the result is defined and has the value ∞.
Marking P-medium to match other similar bugs.
#45205 has been merged, so there's now an opt-in solution to this issue (-Z saturating-float-casts). Presumably we'll want to make this behavior the default at some point in the future – does anyone have opinions on when that should be and what prerequisites there are (if any)?
@rkruppe I'd say we should do a public call, asking people to test the feature and report back. Then if there are no major issues after idk, 2-3 weeks (some people only read the weekly newsletter), we could flip the defaults and allow an opt out.
It could stay this way on the nightly channel and if there continue to be no issues, we can let it ride the trains and include it in the next beta to be added to the next stable (without an opt out, as it would require a -C option and you won't be able to remove the opt-out again). If we hear about issues while the feature is in the beta channel, we can disable it on the beta channel as well.
@est31 Huh, that's a bit more scrutiny than I would have expected. To be clear, in the context of this issue I am only talking about u128 -> f32 casts, the float -> int direction covered by #10184 can and should be rolled out independently. With that in mind, do you think the change to u128 -> f32 casts is more likely to cause regressions than the average bug fix?
I've file a PR (#45900) to just make the u128->f32 behavior the default, refocusing the -Z flag to just cover float->int casts. As with any other change, we'll have up to six weeks in nightly and six weeks of beta to find and fix any regressions there may be.
FYI overflowing s/uitofp casts are no longer considered undefined as of https://reviews.llvm.org/D47807. So we should be able to drop the extra range checking code somewhere in the future.