Repository: BurntSushi/byteorder (language: rust)

Issue:
Don't mask NaN's, or make it a configuration option?
Noticed this while looking at the codegen for deserializing a 4x4 f32 matrix with bincode, which otherwise *should* be a memcopy.

Having gone through the history of how this happened, the LLVM concerns are all bogus, it handles sNaN fine. I don't have much of an opinion on the platform-specific stuff, but at very least this seems like a reasonable feature-flag, or even target cfg thing. Certainly for my use-case, this mask does nothing.


Discussion / hints:
You can't just say "the LLVM concerns are bogus." Can you show it?

On Nov 12, 2017 21:36, "Alexis Beingessner" <notifications@github.com>
wrote:

Noticed this while looking at the codegen for deserializing a 4x4 f32
matrix with bincode, which otherwise *should* be a memcopy.

Having gone through the history of how this happened, the LLVM concerns are
all bogus, it handles sNaN fine. I don't have much of an opinion on the
platform-specific stuff, but at very least this seems like a reasonable
feature-flag, or even target cfg thing. Certainly for my use-case, this
mask does nothing.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<https://github.com/BurntSushi/byteorder/issues/103>, or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAb34ssDC0YLCwaKPvgT9KcNLmxo7HH8ks5s16tIgaJpZM4QbK6e>
.

I discussed it with @regehr and some others who work on llvm, and they all agreed it was just bad docs using ancient C-semantics rather than llvm semantics (C vaguely tries to support non-IEEE floats, but LLVM doesn't). The same docs also say `fdiv %X, 0` is Undefined Behaviour, which is obviously false.

I am working on upstreaming a patch to change the problematic docs (which are just trying to explain undef/UB, and actually have nothing to do with defining fdiv): https://github.com/Gankro/llvm/commit/850f0aedfc3ac8a4b986afc35c60f12530aa2c9b

The docs that caused the scare, for reference:

<img width="1255" alt="screen shot 2017-11-10 at 10 40 39 pm" src="https://user-images.githubusercontent.com/1136864/32707850-c15c96c8-c7f4-11e7-8f30-1dda92b91627.png">

And the *actual* docs for fdiv indicate there's no inputs that cause trouble:

<img width="777" alt="screen shot 2017-11-12 at 10 00 56 pm" src="https://user-images.githubusercontent.com/1136864/32707919-2b680570-c7f5-11e7-8411-d2d30193c68d.png">

If there were any issues, the Semantics section would note them, as the udiv instruction does:

<img width="866" alt="screen shot 2017-11-12 at 10 05 53 pm" src="https://user-images.githubusercontent.com/1136864/32708027-c307d57c-c7f5-11e7-929f-c002d248a9dd.png">

This should also be brought up with std as well, since from_bits has the
same masking behavior.

On Nov 12, 2017 22:05, "Alexis Beingessner" <notifications@github.com>
wrote:

> I discussed it with @regehr <https://github.com/regehr> and some others
> who work on llvm, and they all agreed it was just bad docs using ancient
> C-semantics rather than llvm semantics (C vaguely tries to support non-IEEE
> floats, but LLVM doesn't). The same docs also say fdiv %X, 0 is Undefined
> Behaviour, which is obviously false.
>
> I am working on upstreaming a patch to change the problematic docs (which
> are just trying to explain undef/UB, and actually have nothing to do with
> defining fdiv): Gankro/llvm@850f0ae
> <https://github.com/Gankro/llvm/commit/850f0aedfc3ac8a4b986afc35c60f12530aa2c9b>
>
> The docs that caused the scare, for reference:
>
> [image: screen shot 2017-11-10 at 10 40 39 pm]
> <https://user-images.githubusercontent.com/1136864/32707850-c15c96c8-c7f4-11e7-8f30-1dda92b91627.png>
>
> And the *actual* docs for fdiv indicate there's no inputs that cause
> trouble:
>
> [image: screen shot 2017-11-12 at 10 00 56 pm]
> <https://user-images.githubusercontent.com/1136864/32707919-2b680570-c7f5-11e7-8411-d2d30193c68d.png>
>
> If there were any issues, the Semantics section would note them, as the
> udiv instruction does:
>
> [image: screen shot 2017-11-12 at 10 03 13 pm]
> <https://user-images.githubusercontent.com/1136864/32707973-788c4f14-c7f5-11e7-8fc5-fad49e3bb8ae.png>
>
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub
> <https://github.com/BurntSushi/byteorder/issues/103#issuecomment-343797738>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAb34rfZU3c6YfjiuBWgBmkapwE0HPHqks5s17HkgaJpZM4QbK6e>
> .
>

Ah, I wasn't going to try to fix std since from_bits was stabilized documenting this behaviour, but I see now that it was a "may", so we can just change the impl without touching the documented contract.
Exactly. That "may" was very intentional because we were very unsure of
ourselves, so we took the conservative route to leave it open to being
fixed for good. :)

On Nov 12, 2017 22:34, "Alexis Beingessner" <notifications@github.com>
wrote:

> Ah, I wasn't going to try to fix std since from_bits was stabilized
> documenting this behaviour, but I see now that it was a "may", so we can
> just change the impl without touching the documented contract.
>
> —
> You are receiving this because you commented.
> Reply to this email directly, view it on GitHub
> <https://github.com/BurntSushi/byteorder/issues/103#issuecomment-343801131>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AAb34iP_zkcV1sLQOPvfLdDKA1U-mbh_ks5s17iugaJpZM4QbK6e>
> .
>


Fix patch:
diff --git a/src/io.rs b/src/io.rs
index 208596a..a4c4fb4 100644
--- a/src/io.rs
+++ b/src/io.rs
@@ -509,7 +509,7 @@ pub trait ReadBytesExt: io::Read {
     #[inline]
     fn read_u16_into<T: ByteOrder>(&mut self, dst: &mut [u16]) -> Result<()> {
         {
-            let mut buf = unsafe { slice_to_u8_mut(dst) };
+            let buf = unsafe { slice_to_u8_mut(dst) };
             try!(self.read_exact(buf));
         }
         T::from_slice_u16(dst);
@@ -544,7 +544,7 @@ pub trait ReadBytesExt: io::Read {
     #[inline]
     fn read_u32_into<T: ByteOrder>(&mut self, dst: &mut [u32]) -> Result<()> {
         {
-            let mut buf = unsafe { slice_to_u8_mut(dst) };
+            let buf = unsafe { slice_to_u8_mut(dst) };
             try!(self.read_exact(buf));
         }
         T::from_slice_u32(dst);
@@ -582,7 +582,7 @@ pub trait ReadBytesExt: io::Read {
     #[inline]
     fn read_u64_into<T: ByteOrder>(&mut self, dst: &mut [u64]) -> Result<()> {
         {
-            let mut buf = unsafe { slice_to_u8_mut(dst) };
+            let buf = unsafe { slice_to_u8_mut(dst) };
             try!(self.read_exact(buf));
         }
         T::from_slice_u64(dst);
@@ -659,7 +659,7 @@ pub trait ReadBytesExt: io::Read {
     #[inline]
     fn read_i16_into<T: ByteOrder>(&mut self, dst: &mut [i16]) -> Result<()> {
         {
-            let mut buf = unsafe { slice_to_u8_mut(dst) };
+            let buf = unsafe { slice_to_u8_mut(dst) };
             try!(self.read_exact(buf));
         }
         T::from_slice_i16(dst);
@@ -694,7 +694,7 @@ pub trait ReadBytesExt: io::Read {
     #[inline]
     fn read_i32_into<T: ByteOrder>(&mut self, dst: &mut [i32]) -> Result<()> {
         {
-            let mut buf = unsafe { slice_to_u8_mut(dst) };
+            let buf = unsafe { slice_to_u8_mut(dst) };
             try!(self.read_exact(buf));
         }
         T::from_slice_i32(dst);
@@ -732,7 +732,7 @@ pub trait ReadBytesExt: io::Read {
     #[inline]
     fn read_i64_into<T: ByteOrder>(&mut self, dst: &mut [i64]) -> Result<()> {
         {
-            let mut buf = unsafe { slice_to_u8_mut(dst) };
+            let buf = unsafe { slice_to_u8_mut(dst) };
             try!(self.read_exact(buf));
         }
         T::from_slice_i64(dst);
@@ -826,7 +826,7 @@ pub trait ReadBytesExt: io::Read {
         dst: &mut [f32],
     ) -> Result<()> {
         {
-            let mut buf = slice_to_u8_mut(dst);
+            let buf = slice_to_u8_mut(dst);
             try!(self.read_exact(buf));
         }
         T::from_slice_f32(dst);
@@ -878,7 +878,7 @@ pub trait ReadBytesExt: io::Read {
         dst: &mut [f64],
     ) -> Result<()> {
         {
-            let mut buf = slice_to_u8_mut(dst);
+            let buf = slice_to_u8_mut(dst);
             try!(self.read_exact(buf));
         }
         T::from_slice_f64(dst);
diff --git a/src/lib.rs b/src/lib.rs
index 085e5d4..2051f9b 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -40,7 +40,7 @@ assert_eq!(wtr, vec![5, 2, 0, 3]);
 #![cfg_attr(not(feature = "std"), no_std)]
 #![cfg_attr(feature = "i128", feature(i128_type))]
 #![cfg_attr(all(feature = "i128", test), feature(i128))]
-#![doc(html_root_url = "https://docs.rs/byteorder/1.0.0")]
+#![doc(html_root_url = "https://docs.rs/byteorder/1.1.0")]
 
 #[cfg(feature = "std")]
 extern crate core;
@@ -611,9 +611,6 @@ pub trait ByteOrder
 
     /// Reads a IEEE754 single-precision (4 bytes) floating point number.
     ///
-    /// The return value is always defined; signaling NaN's may be turned into
-    /// quiet NaN's.
-    ///
     /// # Panics
     ///
     /// Panics when `buf.len() < 4`.
@@ -632,14 +629,11 @@ pub trait ByteOrder
     /// ```
     #[inline]
     fn read_f32(buf: &[u8]) -> f32 {
-        safe_u32_bits_to_f32(Self::read_u32(buf))
+        unsafe { transmute(Self::read_u32(buf)) }
     }
 
     /// Reads a IEEE754 double-precision (8 bytes) floating point number.
     ///
-    /// The return value is always defined; signaling NaN's may be turned into
-    /// quiet NaN's.
-    ///
     /// # Panics
     ///
     /// Panics when `buf.len() < 8`.
@@ -658,7 +652,7 @@ pub trait ByteOrder
     /// ```
     #[inline]
     fn read_f64(buf: &[u8]) -> f64 {
-        safe_u64_bits_to_f64(Self::read_u64(buf))
+        unsafe { transmute(Self::read_u64(buf)) }
     }
 
     /// Writes a signed 16 bit integer `n` to `buf`.
@@ -1066,12 +1060,6 @@ pub trait ByteOrder
     /// Reads IEEE754 single-precision (4 bytes) floating point numbers from
     /// `src` into `dst`.
     ///
-    /// Note that this does not perform any checks on the floating point
-    /// conversion. In particular, if the `src` data encodes an undefined
-    /// floating point value for your environment, then the result may be
-    /// undefined behavior. For example, this function may produce signaling
-    /// NaN floating point values.
-    ///
     /// # Panics
     ///
     /// Panics when `src.len() != 4*dst.len()`.
@@ -1088,25 +1076,17 @@ pub trait ByteOrder
     /// LittleEndian::write_f32_into(&numbers_given, &mut bytes);
     ///
     /// let mut numbers_got = [0.0; 4];
-    /// unsafe {
-    ///     LittleEndian::read_f32_into_unchecked(&bytes, &mut numbers_got);
-    /// }
+    /// LittleEndian::read_f32_into_unchecked(&bytes, &mut numbers_got);
     /// assert_eq!(numbers_given, numbers_got);
     /// ```
     #[inline]
-    unsafe fn read_f32_into_unchecked(src: &[u8], dst: &mut [f32]) {
-        Self::read_u32_into(src, transmute(dst));
+    fn read_f32_into_unchecked(src: &[u8], dst: &mut [f32]) {
+        Self::read_u32_into(src, unsafe { transmute(dst) });
     }
 
     /// Reads IEEE754 single-precision (4 bytes) floating point numbers from
     /// `src` into `dst`.
     ///
-    /// Note that this does not perform any checks on the floating point
-    /// conversion. In particular, if the `src` data encodes an undefined
-    /// floating point value for your environment, then the result may be
-    /// undefined behavior. For example, this function may produce signaling
-    /// NaN floating point values.
-    ///
     /// # Panics
     ///
     /// Panics when `src.len() != 8*dst.len()`.
@@ -1123,14 +1103,12 @@ pub trait ByteOrder
     /// LittleEndian::write_f64_into(&numbers_given, &mut bytes);
     ///
     /// let mut numbers_got = [0.0; 4];
-    /// unsafe {
-    ///     LittleEndian::read_f64_into_unchecked(&bytes, &mut numbers_got);
-    /// }
+    /// LittleEndian::read_f64_into_unchecked(&bytes, &mut numbers_got);
     /// assert_eq!(numbers_given, numbers_got);
     /// ```
     #[inline]
-    unsafe fn read_f64_into_unchecked(src: &[u8], dst: &mut [f64]) {
-        Self::read_u64_into(src, transmute(dst));
+    fn read_f64_into_unchecked(src: &[u8], dst: &mut [f64]) {
+        Self::read_u64_into(src, unsafe { transmute(dst) });
     }
 
     /// Writes unsigned 16 bit integers from `src` into `dst`.
@@ -1590,10 +1568,6 @@ pub trait ByteOrder
     ///
     /// If the endianness matches the endianness of the host platform, then
     /// this is a no-op.
-    ///
-    /// Note that the results of this operation are guaranteed to be defined.
-    /// In particular, this method may replace signaling NaN values with
-    /// quiet NaN values.
     fn from_slice_f32(numbers: &mut [f32]);
 
     /// Converts the given slice of IEEE754 double-precision (8 bytes) floating
@@ -1601,10 +1575,6 @@ pub trait ByteOrder
     ///
     /// If the endianness matches the endianness of the host platform, then
     /// this is a no-op.
-    ///
-    /// Note that the results of this operation are guaranteed to be defined.
-    /// In particular, this method may replace signaling NaN values with
-    /// quiet NaN values.
     fn from_slice_f64(numbers: &mut [f64]);
 }
 
@@ -1964,7 +1934,7 @@ impl ByteOrder for BigEndian {
         if cfg!(target_endian = "little") {
             for n in numbers {
                 let int: u32 = unsafe { transmute(*n) };
-                *n = safe_u32_bits_to_f32(int.to_be());
+                *n = unsafe { transmute(int.to_be()) };
             }
         }
     }
@@ -1974,7 +1944,7 @@ impl ByteOrder for BigEndian {
         if cfg!(target_endian = "little") {
             for n in numbers {
                 let int: u64 = unsafe { transmute(*n) };
-                *n = safe_u64_bits_to_f64(int.to_be());
+                *n = unsafe { transmute(int.to_be()) };
             }
         }
     }
@@ -2167,7 +2137,7 @@ impl ByteOrder for LittleEndian {
         if cfg!(target_endian = "big") {
             for n in numbers {
                 let int: u32 = unsafe { transmute(*n) };
-                *n = safe_u32_bits_to_f32(int.to_le());
+                *n = unsafe { transmute(int.to_le()) };
             }
         }
     }
@@ -2177,22 +2147,12 @@ impl ByteOrder for LittleEndian {
         if cfg!(target_endian = "big") {
             for n in numbers {
                 let int: u64 = unsafe { transmute(*n) };
-                *n = safe_u64_bits_to_f64(int.to_le());
+                *n = unsafe { transmute(int.to_le()) };
             }
         }
     }
 }
 
-#[inline]
-fn safe_u32_bits_to_f32(u: u32) -> f32 {
-    unsafe { transmute(u) }
-}
-
-#[inline]
-fn safe_u64_bits_to_f64(u: u64) -> f64 {
-    unsafe { transmute(u) }
-}
-
 #[cfg(test)]
 mod test {
     extern crate quickcheck;


Test patch: