Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upAdd functions to safely transmute float to int #39271
Conversation
rust-highfive
assigned
BurntSushi
Jan 24, 2017
This comment has been minimized.
This comment has been minimized.
|
r? @BurntSushi (rust_highfive has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
|
@alexcrichton I think we briefly talked about this at one point and you mentioned there were possible safety issues related to |
BurntSushi
added
the
T-libs
label
Jan 24, 2017
This comment has been minimized.
This comment has been minimized.
|
My only worry here would be related to "signaling nans" where some bit patterns of nan may cause hardware traps in some floating point operations (I believe). IIRC we don't generate them normally, so I don't think we've brushed up against them yet (I may be wrong). That being said I think it has to do with weird processor mode features so I'd be fine having a feature like this. |
This comment has been minimized.
This comment has been minimized.
|
Signaling NaNs are only a concern when going from int to float. So I'd be totally down with safe bitcasting from float to int, but I'm totally opposed to safe bitcasting from int to float. |
This comment has been minimized.
This comment has been minimized.
|
@retep998 Can you explain the issue in more detail for people unfamiliar with signaling NaNs and under what circumstances they violate safety? |
This comment has been minimized.
This comment has been minimized.
|
A signalling NaN is a floating point number with a certain bit pattern that causes the CPU to trap when attempting to perform any operations with it, which means it'll raise an interrupt or signal or exception (similar to what division by zero does). As a result, LLVM defines operations on signalling NaNs to be undefined behavior, so if you can create a signalling NaN in safe code then you can create undefined behavior which definitely violates Rust's rules. Being able to bitcast an int to a float allows you to get any bit pattern of float you desire, including signalling NaNs, hence it should not be allowed in safe code. |
This comment has been minimized.
This comment has been minimized.
|
Hmm, what about making |
This comment has been minimized.
This comment has been minimized.
|
@BurntSushi In principle code like this
should cause this "Invalid Operation" exception
If segfault is bad enough to be considered unsafety, then sigfpe is bad too? ... I guess? |
This comment has been minimized.
This comment has been minimized.
|
Well, I don't think segfaults directly imply unsafety, they are just highly correlated, no? For example, if using a signaling NaN wasn't undefined behavior and always caused your program to abort, then I wouldn't consider that a violation of safety. (Similar to, say, a |
This comment has been minimized.
This comment has been minimized.
Making it panic seems like a fine solution to me (if signaling NaN's are indeed unsafe). |
This comment has been minimized.
This comment has been minimized.
|
I think returning Err is better because the most likely use case is in deserialisation code, and here panicing on well crafted input is not a very good thing to do. You can still unwrap or expect if you want to panic in such a case, but if the code paniced instead of returning an Err, you'd have to duplicate checks. |
est31
force-pushed the
est31:add_float_bits
branch
from
6bf2273
to
dc85965
Jan 24, 2017
frewsxcv
reviewed
Jan 24, 2017
| /// | ||
| /// Note that this function is distinct from casting. | ||
| /// | ||
| /// ``` |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Looks good to me! |
This comment has been minimized.
This comment has been minimized.
|
What are the primary uses of floats transmuted from integers, by the way? I can imagine two scenarios:
|
This comment has been minimized.
This comment has been minimized.
|
To restate my concerns here:
|
This comment has been minimized.
This comment has been minimized.
valarauca
commented
Feb 3, 2017
•
|
Duplicating my comment from reddit b/c I don't expect people to read reddit C11 standard says the behavior of hard/soft The ISO/IEC 60559 states [2]
So I believe panicking on [1] ISO/IEC 9899/201X Standard: Link Section: 5.2.4.2.2 Bullet Point 3, annotation 22 [2] ISO/IEC 60559 Standard: Link Section 12 first-ish paragraph. |
This comment has been minimized.
This comment has been minimized.
|
@valarauca Can Rust diverge from C11 and make it well defined? (Is that even a coherent question?) |
This comment has been minimized.
This comment has been minimized.
valarauca
commented
Feb 3, 2017
•
|
@BurntSushi I think Rust diverging from C to make undefined behavior well defined is one of the core missions of Rust (?) My recommendation is panicking is fine. |
This comment has been minimized.
This comment has been minimized.
|
Sure, I just don't know the details between "make it well defined" and "make sure this is consistent with how we're using LLVM." |
This comment has been minimized.
This comment has been minimized.
|
I've updated my signaling NaN example in #39271 (comment). Now it outputs
on Linux. A possible solution:
If FP exceptions are disabled, then signaling NaNs are converted into quiet NaNs by arithmetic operations, so this is completely safe. The behavior is controlled by IEEE 754-2008, not implementations. |
This comment has been minimized.
This comment has been minimized.
valarauca
commented
Feb 3, 2017
What ever is done will have to be encoded into the Rust standard library and fixing various bugs will be a continuous effort. The LLVM doesn't handle The real issue is platforms. For GPU's, ARMv7, ARMv8, and NEON subnormals are flushed to zero (which isn't IEEE754 or ISO/IEC 60559 conformant) so there really isn't any The easiest way to do this is have a something like: fn is_nan(&self) -> bool {
*self != Nanf32 //or Nanf64 for 64bit
}This is what IEEE and ISO/IEC suggest.
This doesn't 100% work. If I guess the proper way to catch both Quiet and Signal NAN, IEEE and ISO/IEC encode Quiet/Signal NAN's the same, but IDFK all platforms do. @petrochenkov example seems to imply that x86/x87/x64 treats anything with an 0xFF or 0x7FF exponent as a NaN which is non-standard... So a quick (untested) example: const F32_NAN: u32 = 0x7F800000u32;
const F32_NAN_MASK: u32 = 0x7FFFFFFFu32;
unsafe fn is_signal_nan(x: *const f32) -> bool {
let ptr: *const u32 = mem::transmute(x);
let mut val: u32 = read_volatile(ptr);
(val & F32_NAN_MASK) == F32_NAN
}
const F64NAN: 7FF0000000000000u64;
const F64_NAN_MASK: 7FFFFFFFFFFFFFFFu64;
unsafe fn is_signal_nan(x: *const f64) -> bool {
let ptr: *const u64 = mem::transmute(x);
let mut val: u64 = read_volatile(ptr);
(val & F64_NAN_MASK) == F64NAN
}The type cast/volatile read is to avoid the compiler/platform doing any magic even in This of course can still fail on some platforms is unsafe fn is_nan(x: f32) -> bool {
let val: u32 = mem::transmute(x);
....
}One can hit a register to register copies and incur the same magic. There is no idiomatic way to do this. I think the best approach is the stdlib will have to implement a solid attempt at NAN checking, and this function will grow and get more specialized as bug reports come in. Even well tested implementation will likely have a lot of holes in it as different processor models/modes will have different semantics. |
This comment has been minimized.
This comment has been minimized.
|
[Comment is moved to https://github.com/rust-lang/rust/pull/39271#issuecomment-277264729] |
This comment has been minimized.
This comment has been minimized.
|
Discussed during libs triage today our conclusions were:
|
tshepang
reviewed
Mar 13, 2017
| /// | ||
| /// ``` | ||
| /// #![feature(float_bits_conv)] | ||
| /// assert!((1f32).to_bits() != 1f32 as u32); // to_bits() is not casting! |
This comment has been minimized.
This comment has been minimized.
est31
force-pushed the
est31:add_float_bits
branch
from
adfdee7
to
8827e9b
Mar 13, 2017
This comment has been minimized.
This comment has been minimized.
|
Cc me I'll have comments tonight. |
added some commits
Mar 29, 2017
est31
force-pushed the
est31:add_float_bits
branch
from
35b07d4
to
0c14815
Apr 18, 2017
This comment has been minimized.
This comment has been minimized.
|
Tidy is fixed now. Had to rebase. r? @BurntSushi |
This comment has been minimized.
This comment has been minimized.
|
@bors r=BurntSushi |
This comment has been minimized.
This comment has been minimized.
|
|
This comment has been minimized.
This comment has been minimized.
bors
added a commit
that referenced
this pull request
Apr 18, 2017
This comment has been minimized.
This comment has been minimized.
|
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
bors
added a commit
that referenced
this pull request
Apr 18, 2017
This comment has been minimized.
This comment has been minimized.
bors
added a commit
that referenced
this pull request
Apr 18, 2017
This comment has been minimized.
This comment has been minimized.
|
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
bors
added a commit
that referenced
this pull request
Apr 18, 2017
This comment has been minimized.
This comment has been minimized.
|
|
est31 commentedJan 24, 2017
•
edited
The safe subset of Rust tries to be as powerful as possible. While it is very powerful already, its currently impossible to safely transmute integers to floats. While crates exist that provide a safe interface, most prominently the
iee754crate (which also inspired naming of the added functions), they themselves only use the unsafemem::transmutefunction to accomplish this task.Also, including an entire crate for just two lines of unsafe code seems quite wasteful.
That's why this PR adds functions to safely transmute integers to floats and vice versa, currently gated by the newly added
float_bits_convfeature.The functions added are no niche case. Not just
ieee754currently implements float to int transmutation via unsafe code but also the very popularbyteordercrate. This functionality of byteorder is in turn used by higher level crates. I only give two examples out of many: chor and bincode.One alternative would be to manually use functions like pow or multiplication by 1 to get a similar result, but they only work in the int -> float direction, and are not bit exact, and much slower (also, most likely the optimizer will never optimize it to a transmute because the conversion is not bit exact while the transmute is).
Tracking issue: #40470