Merge pull request #8525 from Techjar/axwii-ub

HW/DSPHLE: Fix struct aliasing undefined behavior in AX ucode

Source/Core/Common/BitUtils.h

@@ -4,6 +4,7 @@
 
 #pragma once
 
+#include <array>
 #include <climits>
 #include <cstddef>
 #include <cstring>
@@ -240,6 +241,53 @@ inline auto BitCastPtr(PtrType* ptr) noexcept -> BitCastPtrType<T, PtrType>
   return BitCastPtrType<T, PtrType>{ptr};
 }
 
+// Similar to BitCastPtr, but specifically for aliasing structs to arrays.
+template <typename ArrayType, typename T,
+          typename Container = std::array<ArrayType, sizeof(T) / sizeof(ArrayType)>>
+inline auto BitCastToArray(const T& obj) noexcept -> Container
+{
+  static_assert(sizeof(T) % sizeof(ArrayType) == 0,
+                "Size of array type must be a factor of size of source type.");
+  static_assert(std::is_trivially_copyable<T>(),
+                "BitCastToArray source type must be trivially copyable.");
+  static_assert(std::is_trivially_copyable<Container>(),
+                "BitCastToArray array type must be trivially copyable.");
+
+  Container result;
+  std::memcpy(result.data(), &obj, sizeof(T));
+  return result;
+}
+
+template <typename ArrayType, typename T,
+          typename Container = std::array<ArrayType, sizeof(T) / sizeof(ArrayType)>>
+inline void BitCastFromArray(const Container& array, T& obj) noexcept
+{
+  static_assert(sizeof(T) % sizeof(ArrayType) == 0,
+                "Size of array type must be a factor of size of destination type.");
+  static_assert(std::is_trivially_copyable<Container>(),
+                "BitCastFromArray array type must be trivially copyable.");
+  static_assert(std::is_trivially_copyable<T>(),
+                "BitCastFromArray destination type must be trivially copyable.");
+
+  std::memcpy(&obj, array.data(), sizeof(T));
+}
+
+template <typename ArrayType, typename T,
+          typename Container = std::array<ArrayType, sizeof(T) / sizeof(ArrayType)>>
+inline auto BitCastFromArray(const Container& array) noexcept -> T
+{
+  static_assert(sizeof(T) % sizeof(ArrayType) == 0,
+                "Size of array type must be a factor of size of destination type.");
+  static_assert(std::is_trivially_copyable<Container>(),
+                "BitCastFromArray array type must be trivially copyable.");
+  static_assert(std::is_trivially_copyable<T>(),
+                "BitCastFromArray destination type must be trivially copyable.");
+
+  T obj;
+  std::memcpy(&obj, array.data(), sizeof(T));
+  return obj;
+}
+
 template <typename T>
 void SetBit(T& value, size_t bit_number, bool bit_value)
 {

Source/Core/Core/HW/DSPHLE/UCodes/AX.cpp

@@ -279,21 +279,6 @@ void AXUCode::HandleCommandList()
   }
 }
 
-void AXUCode::ApplyUpdatesForMs(int curr_ms, u16* pb, u16* num_updates, u16* updates)
-{
-  u32 start_idx = 0;
-  for (int i = 0; i < curr_ms; ++i)
-    start_idx += num_updates[i];
-
-  for (u32 i = start_idx; i < start_idx + num_updates[curr_ms]; ++i)
-  {
-    u16 update_off = Common::swap16(updates[2 * i]);
-    u16 update_val = Common::swap16(updates[2 * i + 1]);
-
-    pb[update_off] = update_val;
-  }
-}
-
 AXMixControl AXUCode::ConvertMixerControl(u32 mixer_control)
 {
   u32 ret = 0;
@@ -428,7 +413,7 @@ void AXUCode::ProcessPBList(u32 pb_addr)
 {
   // Samples per millisecond. In theory DSP sampling rate can be changed from
   // 32KHz to 48KHz, but AX always process at 32KHz.
-  const u32 spms = 32;
+  constexpr u32 spms = 32;
 
   AXPB pb;
 
@@ -445,7 +430,7 @@ void AXUCode::ProcessPBList(u32 pb_addr)
 
     for (int curr_ms = 0; curr_ms < 5; ++curr_ms)
     {
-      ApplyUpdatesForMs(curr_ms, (u16*)&pb, pb.updates.num_updates, updates);
+      ApplyUpdatesForMs(curr_ms, pb, pb.updates.num_updates, updates);
 
       ProcessVoice(pb, buffers, spms, ConvertMixerControl(pb.mixer_control),
                    m_coeffs_available ? m_coeffs : nullptr);

Source/Core/Core/HW/DSPHLE/UCodes/AX.h

@@ -12,7 +12,9 @@
 
 #pragma once
 
+#include "Common/BitUtils.h"
 #include "Common/CommonTypes.h"
+#include "Common/Swap.h"
 #include "Core/HW/DSPHLE/UCodes/UCodes.h"
 
 namespace DSP::HLE
@@ -108,7 +110,25 @@ class AXUCode : public UCodeInterface
   AXMixControl ConvertMixerControl(u32 mixer_control);
 
   // Apply updates to a PB. Generic, used in AX GC and AX Wii.
-  void ApplyUpdatesForMs(int curr_ms, u16* pb, u16* num_updates, u16* updates);
+  template <typename PBType>
+  void ApplyUpdatesForMs(int curr_ms, PBType& pb, u16* num_updates, u16* updates)
+  {
+    auto pb_mem = Common::BitCastToArray<u16>(pb);
+
+    u32 start_idx = 0;
+    for (int i = 0; i < curr_ms; ++i)
+      start_idx += num_updates[i];
+
+    for (u32 i = start_idx; i < start_idx + num_updates[curr_ms]; ++i)
+    {
+      u16 update_off = Common::swap16(updates[2 * i]);
+      u16 update_val = Common::swap16(updates[2 * i + 1]);
+
+      pb_mem[update_off] = update_val;
+    }
+
+    Common::BitCastFromArray<u16>(pb_mem, pb);
+  }
 
   virtual void HandleCommandList();
   void SignalWorkEnd();

Source/Core/Core/HW/DSPHLE/UCodes/AXWii.cpp

@@ -385,13 +385,13 @@ void AXWiiUCode::GenerateVolumeRamp(u16* output, u16 vol1, u16 vol2, size_t nval
 bool AXWiiUCode::ExtractUpdatesFields(AXPBWii& pb, u16* num_updates, u16* updates,
                                       u32* updates_addr)
 {
-  u16* pb_mem = (u16*)&pb;
+  auto pb_mem = Common::BitCastToArray<u16>(pb);
 
   if (!m_old_axwii)
     return false;
 
   // Copy the num_updates field.
-  memcpy(num_updates, pb_mem + 41, 6);
+  memcpy(num_updates, &pb_mem[41], 6);
 
   // Get the address of the updates data
   u16 addr_hi = pb_mem[44];
@@ -417,28 +417,36 @@ bool AXWiiUCode::ExtractUpdatesFields(AXPBWii& pb, u16* num_updates, u16* update
   }
 
   // Remove the updates data from the PB
-  memmove(pb_mem + 41, pb_mem + 46, sizeof(pb) - 2 * 46);
+  memmove(&pb_mem[41], &pb_mem[46], sizeof(pb) - 2 * 46);
+
+  Common::BitCastFromArray<u16>(pb_mem, pb);
 
   return true;
 }
 
 void AXWiiUCode::ReinjectUpdatesFields(AXPBWii& pb, u16* num_updates, u32 updates_addr)
 {
-  u16* pb_mem = (u16*)&pb;
+  auto pb_mem = Common::BitCastToArray<u16>(pb);
 
   // Make some space
-  memmove(pb_mem + 46, pb_mem + 41, sizeof(pb) - 2 * 46);
+  memmove(&pb_mem[46], &pb_mem[41], sizeof(pb) - 2 * 46);
 
   // Reinsert previous values
   pb_mem[41] = num_updates[0];
   pb_mem[42] = num_updates[1];
   pb_mem[43] = num_updates[2];
   pb_mem[44] = updates_addr >> 16;
   pb_mem[45] = updates_addr & 0xFFFF;
+
+  Common::BitCastFromArray<u16>(pb_mem, pb);
 }
 
 void AXWiiUCode::ProcessPBList(u32 pb_addr)
 {
+  // Samples per millisecond. In theory DSP sampling rate can be changed from
+  // 32KHz to 48KHz, but AX always process at 32KHz.
+  constexpr u32 spms = 32;
+
   AXPBWii pb;
 
   while (pb_addr)
@@ -460,13 +468,13 @@ void AXWiiUCode::ProcessPBList(u32 pb_addr)
     {
       for (int curr_ms = 0; curr_ms < 3; ++curr_ms)
       {
-        ApplyUpdatesForMs(curr_ms, (u16*)&pb, num_updates, updates);
-        ProcessVoice(pb, buffers, 32, ConvertMixerControl(HILO_TO_32(pb.mixer_control)),
+        ApplyUpdatesForMs(curr_ms, pb, num_updates, updates);
+        ProcessVoice(pb, buffers, spms, ConvertMixerControl(HILO_TO_32(pb.mixer_control)),
                      m_coeffs_available ? m_coeffs : nullptr);
 
         // Forward the buffers
         for (auto& ptr : buffers.ptrs)
-          ptr += 32;
+          ptr += spms;
       }
       ReinjectUpdatesFields(pb, num_updates, updates_addr);
     }