Merge pull request #9480 from leoetlino/saturating-cast

MathUtil: Add SaturatingCast to cast floats more safely

Source/Core/Common/MathUtil.h

@@ -6,6 +6,7 @@
 
 #include <algorithm>
 #include <cmath>
+#include <limits>
 #include <type_traits>
 #include <vector>
 
@@ -30,6 +31,48 @@ constexpr auto Lerp(const T& x, const T& y, const F& a) -> decltype(x + (y - x)
   return x + (y - x) * a;
 }
 
+// Casts the specified value to a Dest. The value will be clamped to fit in the destination type.
+// Warning: The result of SaturatingCast(NaN) is undefined.
+template <typename Dest, typename T>
+constexpr Dest SaturatingCast(T value)
+{
+  static_assert(std::is_integral<Dest>());
+
+  constexpr Dest lo = std::numeric_limits<Dest>::lowest();
+  constexpr Dest hi = std::numeric_limits<Dest>::max();
+
+  // T being a signed integer and Dest unsigned is a problematic case because the value will
+  // be converted into an unsigned integer, and u32(...) < 0 is always false.
+  if constexpr (std::is_integral<T>() && std::is_signed<T>() && std::is_unsigned<Dest>())
+  {
+    static_assert(lo == 0);
+    if (value < 0)
+      return lo;
+    // Now that we got rid of negative values, we can safely cast value to an unsigned T
+    // since unsigned T can represent any positive value signed T could represent.
+    // The compiler will then promote the LHS or the RHS if necessary.
+    if (std::make_unsigned_t<T>(value) > hi)
+      return hi;
+  }
+  else if constexpr (std::is_integral<T>() && std::is_unsigned<T>() && std::is_signed<Dest>())
+  {
+    // value and hi will never be negative, and hi is representable as an unsigned Dest.
+    if (value > std::make_unsigned_t<Dest>(hi))
+      return hi;
+  }
+  else
+  {
+    // Do not use std::clamp or a similar function here to avoid overflow.
+    // For example, if Dest = s64 and T = int, we want integer promotion to convert value to a s64
+    // instead of changing lo or hi into an int.
+    if (value < lo)
+      return lo;
+    if (value > hi)
+      return hi;
+  }
+  return static_cast<Dest>(value);
+}
+
 template <typename T>
 constexpr bool IsPow2(T imm)
 {

Source/Core/DiscIO/WIACompression.cpp

@@ -19,6 +19,7 @@
 
 #include "Common/Assert.h"
 #include "Common/CommonTypes.h"
+#include "Common/MathUtil.h"
 #include "Common/Swap.h"
 #include "DiscIO/LaggedFibonacciGenerator.h"
 
@@ -166,18 +167,13 @@ bool Bzip2Decompressor::Decompress(const DecompressionBuffer& in, DecompressionB
     m_started = true;
   }
 
-  constexpr auto clamped_cast = [](size_t x) {
-    return static_cast<unsigned int>(
-        std::min<size_t>(std::numeric_limits<unsigned int>().max(), x));
-  };
-
   char* const in_ptr = reinterpret_cast<char*>(const_cast<u8*>(in.data.data() + *in_bytes_read));
   m_stream.next_in = in_ptr;
-  m_stream.avail_in = clamped_cast(in.bytes_written - *in_bytes_read);
+  m_stream.avail_in = MathUtil::SaturatingCast<u32>(in.bytes_written - *in_bytes_read);
 
   char* const out_ptr = reinterpret_cast<char*>(out->data.data() + out->bytes_written);
   m_stream.next_out = out_ptr;
-  m_stream.avail_out = clamped_cast(out->data.size() - out->bytes_written);
+  m_stream.avail_out = MathUtil::SaturatingCast<u32>(out->data.size() - out->bytes_written);
 
   const int result = BZ2_bzDecompress(&m_stream);
 

Source/UnitTests/Common/MathUtilTest.cpp

@@ -26,3 +26,45 @@ TEST(MathUtil, NextPowerOf2)
   EXPECT_EQ(8U, MathUtil::NextPowerOf2(6));
   EXPECT_EQ(0x40000000U, MathUtil::NextPowerOf2(0x23456789));
 }
+
+TEST(MathUtil, SaturatingCast)
+{
+  // Cast from an integer type to a smaller type
+  EXPECT_EQ(255u, (MathUtil::SaturatingCast<u8, int>(1000)));
+  EXPECT_EQ(255u, (MathUtil::SaturatingCast<u8, u16>(1000u)));
+  EXPECT_EQ(255u, (MathUtil::SaturatingCast<u8, std::size_t>(1000)));
+
+  // Cast from a signed integer type
+  EXPECT_EQ(0u, (MathUtil::SaturatingCast<u8, int>(-1)));
+  EXPECT_EQ(0u, (MathUtil::SaturatingCast<u8, int>(-1000)));
+  EXPECT_EQ(0u, (MathUtil::SaturatingCast<u32, int>(-1)));
+  EXPECT_EQ(-1000, (MathUtil::SaturatingCast<s16, int>(-1000)));
+  EXPECT_EQ(-1000, (MathUtil::SaturatingCast<int, int>(-1000)));
+  EXPECT_EQ(-1000, (MathUtil::SaturatingCast<s64, int>(-1000)));
+
+  // Cast from an unsigned integer type to a smaller integer type
+  EXPECT_EQ(0x7fff, (MathUtil::SaturatingCast<s16, u32>(0xffffffffu)));
+  EXPECT_EQ(0x7fffffff, (MathUtil::SaturatingCast<int, u32>(0xffffffffu)));
+
+  // Cast from a floating point type to an integer type
+  EXPECT_EQ(255u, MathUtil::SaturatingCast<u8>(1234.0));
+  EXPECT_EQ(0u, MathUtil::SaturatingCast<u8>(-1234.0));
+  EXPECT_EQ(127, MathUtil::SaturatingCast<s8>(5678.0));
+  EXPECT_EQ(-128, MathUtil::SaturatingCast<s8>(-5678.0));
+  EXPECT_EQ(65535u, MathUtil::SaturatingCast<u16>(999999.0));
+
+  // Negative zero
+  EXPECT_EQ(0u, MathUtil::SaturatingCast<u8>(0.0));
+  EXPECT_EQ(0u, MathUtil::SaturatingCast<u8>(-0.0));
+  EXPECT_EQ(0, MathUtil::SaturatingCast<s8>(0.0));
+  EXPECT_EQ(0, MathUtil::SaturatingCast<s8>(-0.0));
+
+  // Edge cases
+  EXPECT_EQ(std::numeric_limits<s32>::max(),
+            MathUtil::SaturatingCast<s32>(std::numeric_limits<float>::infinity()));
+  EXPECT_EQ(std::numeric_limits<s32>::min(),
+            MathUtil::SaturatingCast<s32>(-std::numeric_limits<float>::infinity()));
+  // 16777217 = 2^24 + 1 is the first integer that cannot be represented correctly with a f32.
+  EXPECT_EQ(16777216, MathUtil::SaturatingCast<s32>(float(16777216)));
+  EXPECT_EQ(16777216, MathUtil::SaturatingCast<s32>(float(16777217)));
+}

Source/UnitTests/VideoCommon/VertexLoaderTest.cpp

@@ -12,6 +12,7 @@
 
 #include "Common/BitUtils.h"
 #include "Common/Common.h"
+#include "Common/MathUtil.h"
 #include "VideoCommon/CPMemory.h"
 #include "VideoCommon/DataReader.h"
 #include "VideoCommon/OpcodeDecoding.h"
@@ -148,7 +149,7 @@ TEST_P(VertexLoaderParamTest, PositionAll)
       -0x8000,
       -0x80,
       -1,
-      -0,
+      -0.0,
       0,
       1,
       123,
@@ -180,16 +181,16 @@ TEST_P(VertexLoaderParamTest, PositionAll)
     switch (format)
     {
     case ComponentFormat::UByte:
-      Input((u8)value);
+      Input(MathUtil::SaturatingCast<u8>(value));
       break;
     case ComponentFormat::Byte:
-      Input((s8)value);
+      Input(MathUtil::SaturatingCast<s8>(value));
       break;
     case ComponentFormat::UShort:
-      Input((u16)value);
+      Input(MathUtil::SaturatingCast<u16>(value));
       break;
     case ComponentFormat::Short:
-      Input((s16)value);
+      Input(MathUtil::SaturatingCast<s16>(value));
       break;
     case ComponentFormat::Float:
       Input(value);
@@ -206,20 +207,20 @@ TEST_P(VertexLoaderParamTest, PositionAll)
     switch (format)
     {
     case ComponentFormat::UByte:
-      f = (u8)*iter++;
-      g = (u8)*iter++;
+      f = MathUtil::SaturatingCast<u8>(*iter++);
+      g = MathUtil::SaturatingCast<u8>(*iter++);
       break;
     case ComponentFormat::Byte:
-      f = (s8)*iter++;
-      g = (s8)*iter++;
+      f = MathUtil::SaturatingCast<s8>(*iter++);
+      g = MathUtil::SaturatingCast<s8>(*iter++);
       break;
     case ComponentFormat::UShort:
-      f = (u16)*iter++;
-      g = (u16)*iter++;
+      f = MathUtil::SaturatingCast<u16>(*iter++);
+      g = MathUtil::SaturatingCast<u16>(*iter++);
       break;
     case ComponentFormat::Short:
-      f = (s16)*iter++;
-      g = (s16)*iter++;
+      f = MathUtil::SaturatingCast<s16>(*iter++);
+      g = MathUtil::SaturatingCast<s16>(*iter++);
       break;
     case ComponentFormat::Float:
       f = *iter++;