Merge pull request #10750 from Pokechu22/hermes-test

Enhance DSPAssemblyTest, and fix various DSPTool bugs discovered while doing so

Source/Core/Core/DSP/DSPCodeUtil.cpp

@@ -57,10 +57,12 @@ bool Disassemble(const std::vector<u16>& code, bool line_numbers, std::string& t
   return success;
 }
 
+// NOTE: This code is called from DSPTool and UnitTests, which do not use the logging system.
+// Thus, fmt::print is used instead of the log system.
 bool Compare(const std::vector<u16>& code1, const std::vector<u16>& code2)
 {
   if (code1.size() != code2.size())
-    WARN_LOG_FMT(AUDIO, "Size difference! 1={} 2={}\n", code1.size(), code2.size());
+    fmt::print("Size difference! 1={} 2={}\n", code1.size(), code2.size());
   u32 count_equal = 0;
   const u16 min_size = static_cast<u16>(std::min(code1.size(), code2.size()));
 
@@ -76,26 +78,47 @@ bool Compare(const std::vector<u16>& code1, const std::vector<u16>& code2)
     {
       std::string line1, line2;
       u16 pc = i;
-      disassembler.DisassembleOpcode(&code1[0], &pc, line1);
+      disassembler.DisassembleOpcode(code1, &pc, line1);
       pc = i;
-      disassembler.DisassembleOpcode(&code2[0], &pc, line2);
+      disassembler.DisassembleOpcode(code2, &pc, line2);
-      WARN_LOG_FMT(AUDIO, "!! {:04x} : {:04x} vs {:04x} - {}  vs  {}\n", i, code1[i], code2[i],
+      fmt::print("!! {:04x} : {:04x} vs {:04x} - {}  vs  {}\n", i, code1[i], code2[i], line1,
-                   line1, line2);
+                 line2);
+
+      // Also do a comparison one word back if the previous word corresponded to an instruction with
+      // a large immediate. (Compare operates on individual words, so both the main word and the
+      // immediate following it are compared separately; we don't use DisassembleOpcode's ability to
+      // increment pc by 2 for two-word instructions because code1 may have a 1-word instruction
+      // where code2 has a 2-word instruction.)
+      if (i >= 1 && code1[i - 1] == code2[i - 1])
+      {
+        const DSPOPCTemplate* opc = FindOpInfoByOpcode(code1[i - 1]);
+        if (opc != nullptr && opc->size == 2)
+        {
+          line1.clear();
+          line2.clear();
+          pc = i - 1;
+          disassembler.DisassembleOpcode(code1, &pc, line1);
+          pc = i - 1;
+          disassembler.DisassembleOpcode(code2, &pc, line2);
+          fmt::print("   (or {:04x} : {:04x} {:04x} vs {:04x} {:04x} - {}  vs  {})\n", i - 1,
+                     code1[i - 1], code1[i], code2[i - 1], code2[i], line1, line2);
+        }
+      }
     }
   }
   if (code2.size() != code1.size())
   {
-    DEBUG_LOG_FMT(AUDIO, "Extra code words:\n");
+    fmt::print("Extra code words:\n");
     const std::vector<u16>& longest = code1.size() > code2.size() ? code1 : code2;
     for (u16 i = min_size; i < longest.size(); i++)
     {
       u16 pc = i;
       std::string line;
-      disassembler.DisassembleOpcode(&longest[0], &pc, line);
+      disassembler.DisassembleOpcode(longest, &pc, line);
-      DEBUG_LOG_FMT(AUDIO, "!! {}\n", line);
+      fmt::print("!! {:04x} : {:04x} - {}\n", i, longest[i], line);
     }
   }
-  DEBUG_LOG_FMT(AUDIO, "Equal instruction words: {} / {}\n", count_equal, min_size);
+  fmt::print("Equal instruction words: {} / {}\n", count_equal, min_size);
   return code1.size() == code2.size() && code1.size() == count_equal;
 }
 

Source/Core/Core/DSP/DSPDisassembler.cpp

@@ -33,9 +33,10 @@ bool DSPDisassembler::Disassemble(const std::vector<u16>& code, std::string& tex
 
   for (u16 pc = 0; pc < code.size();)
   {
-    if (!DisassembleOpcode(code.data(), &pc, text))
+    bool failed = !DisassembleOpcode(code, &pc, text);
-      return false;
     text.append("\n");
+    if (failed)
+      return false;
   }
   return true;
 }
@@ -107,7 +108,7 @@ std::string DSPDisassembler::DisassembleParameters(const DSPOPCTemplate& opc, u1
         {
           // Left and right shifts function essentially as a single shift by a 7-bit signed value,
           // but are split into two intructions for clarity.
-          buf += fmt::format("#{}", (val & 0x20) != 0 ? (64 - val) : val);
+          buf += fmt::format("#{}", (val & 0x20) != 0 ? (int(val) - 64) : int(val));
         }
         else
         {
@@ -139,16 +140,23 @@ std::string DSPDisassembler::DisassembleParameters(const DSPOPCTemplate& opc, u1
   return buf;
 }
 
-bool DSPDisassembler::DisassembleOpcode(const u16* binbuf, u16* pc, std::string& dest)
+bool DSPDisassembler::DisassembleOpcode(const std::vector<u16>& code, u16* pc, std::string& dest)
 {
-  if ((*pc & 0x7fff) >= 0x1000)
+  return DisassembleOpcode(code.data(), code.size(), pc, dest);
+}
+
+bool DSPDisassembler::DisassembleOpcode(const u16* binbuf, size_t binbuf_size, u16* pc,
+                                        std::string& dest)
+{
+  const u16 wrapped_pc = (*pc & 0x7fff);
+  if (wrapped_pc >= binbuf_size)
   {
     ++pc;
     dest.append("; outside memory");
     return false;
   }
 
-  const u16 op1 = binbuf[*pc & 0x0fff];
+  const u16 op1 = binbuf[wrapped_pc];
 
   // Find main opcode
   const DSPOPCTemplate* opc = FindOpInfoByOpcode(op1);
@@ -179,14 +187,23 @@ bool DSPDisassembler::DisassembleOpcode(const u16* binbuf, u16* pc, std::string&
   // printing
 
   if (settings_.show_pc)
-    dest += fmt::format("{:04x} ", *pc);
+    dest += fmt::format("{:04x} ", wrapped_pc);
 
   u16 op2;
 
   // Size 2 - the op has a large immediate.
   if (opc->size == 2)
   {
-    op2 = binbuf[(*pc + 1) & 0x0fff];
+    if (wrapped_pc + 1 >= binbuf_size)
+    {
+      if (settings_.show_hex)
+        dest += fmt::format("{:04x} ???? ", op1);
+      dest += fmt::format("; Insufficient data for large immediate");
+      *pc += opc->size;
+      return false;
+    }
+
+    op2 = binbuf[wrapped_pc + 1];
     if (settings_.show_hex)
       dest += fmt::format("{:04x} {:04x} ", op1, op2);
   }

Source/Core/Core/DSP/DSPDisassembler.h

@@ -34,8 +34,10 @@ public:
 
   bool Disassemble(const std::vector<u16>& code, std::string& text);
 
-  // Warning - this one is trickier to use right.
+  // Disassembles the given opcode at pc and increases pc by the opcode's size.
-  bool DisassembleOpcode(const u16* binbuf, u16* pc, std::string& dest);
+  // The PC is wrapped such that 0x0000 and 0x8000 both point to the start of the buffer.
+  bool DisassembleOpcode(const std::vector<u16>& code, u16* pc, std::string& dest);
+  bool DisassembleOpcode(const u16* binbuf, size_t binbuf_size, u16* pc, std::string& dest);
 
 private:
   std::string DisassembleParameters(const DSPOPCTemplate& opc, u16 op1, u16 op2);

Source/Core/Core/HW/DSPLLE/DSPSymbols.cpp

@@ -77,13 +77,15 @@ void AutoDisassembly(const SDSP& dsp, u16 start_addr, u16 end_addr)
 
   u16 addr = start_addr;
   const u16* ptr = (start_addr >> 15) != 0 ? dsp.irom : dsp.iram;
+  constexpr size_t size = DSP_IROM_SIZE;
+  static_assert(size == DSP_IRAM_SIZE);
   while (addr < end_addr)
   {
     line_to_addr[line_counter] = addr;
     addr_to_line[addr] = line_counter;
 
     std::string buf;
-    if (!disasm.DisassembleOpcode(ptr, &addr, buf))
+    if (!disasm.DisassembleOpcode(ptr, size, &addr, buf))
     {
       ERROR_LOG_FMT(DSPLLE, "disasm failed at {:04x}", addr);
       break;

Source/DSPTool/DSPTool.cpp

@@ -128,7 +128,7 @@ static std::string CodesToHeader(const std::vector<std::vector<u16>>& codes,
   return header;
 }
 
-static void PerformBinaryComparison(const std::string& lhs, const std::string& rhs)
+static bool PerformBinaryComparison(const std::string& lhs, const std::string& rhs)
 {
   std::string binary_code;
 
@@ -138,7 +138,7 @@ static void PerformBinaryComparison(const std::string& lhs, const std::string& r
   File::ReadFileToString(rhs, binary_code);
   const std::vector<u16> code2 = DSP::BinaryStringBEToCode(binary_code);
 
-  DSP::Compare(code1, code2);
+  return DSP::Compare(code1, code2);
 }
 
 static void PrintResults(const std::string& input_name, const std::string& output_name,
@@ -482,8 +482,7 @@ int main(int argc, const char* argv[])
 
   if (compare)
   {
-    PerformBinaryComparison(input_name, output_name);
+    return PerformBinaryComparison(input_name, output_name) ? 0 : 1;
-    return 0;
   }
 
   if (print_results)

Source/DSPTool/DSPTool.vcxproj

@@ -21,12 +21,6 @@
       <SubSystem>Console</SubSystem>
     </Link>
   </ItemDefinitionGroup>
-  <ItemGroup>
-    <None Include="Testdata\dsp_test.bin" />
-    <None Include="Testdata\dsp_test.S" />
-    <None Include="Testdata\hermes.bin" />
-    <None Include="Testdata\hermes.s" />
-  </ItemGroup>
   <ItemGroup>
     <ClCompile Include="DSPTool.cpp" />
     <ClCompile Include="StubHost.cpp" />

Source/UnitTests/Core/CMakeLists.txt

@@ -8,6 +8,7 @@ add_dolphin_test(DSPAssemblyTest
   DSP/DSPTestBinary.cpp
   DSP/DSPTestText.cpp
   DSP/HermesBinary.cpp
+  DSP/HermesText.cpp
 )
 
 add_dolphin_test(ESFormatsTest IOS/ES/FormatsTest.cpp)

Source/UnitTests/Core/DSP/DSPAssemblyTest.cpp

@@ -1,17 +1,19 @@
 // Copyright 2017 Dolphin Emulator Project
 // SPDX-License-Identifier: GPL-2.0-or-later
 
-#include "Common/FileUtil.h"
+#include <fmt/format.h>
+
 #include "Core/DSP/DSPCodeUtil.h"
 #include "Core/DSP/DSPDisassembler.h"
 
 #include "DSPTestBinary.h"
 #include "DSPTestText.h"
 #include "HermesBinary.h"
+#include "HermesText.h"
 
 #include <gtest/gtest.h>
 
-static bool RoundTrippableDissassemble(const std::vector<u16>& code, std::string& text)
+static bool RoundTrippableDisassemble(const std::vector<u16>& code, std::string& text)
 {
   DSP::AssemblerSettings settings;
   settings.ext_separator = '\'';
@@ -31,20 +33,20 @@ static bool RoundTrip(const std::vector<u16>& code1)
 {
   std::vector<u16> code2;
   std::string text;
-  if (!RoundTrippableDissassemble(code1, text))
+  if (!RoundTrippableDisassemble(code1, text))
   {
-    printf("RoundTrip: Disassembly failed.\n");
+    fmt::print("RoundTrip: Disassembly failed.\n");
     return false;
   }
   if (!DSP::Assemble(text, code2))
   {
-    printf("RoundTrip: Assembly failed.\n");
+    fmt::print("RoundTrip: Assembly failed.\n");
     return false;
   }
   if (!DSP::Compare(code1, code2))
   {
-    DSP::Disassemble(code1, true, text);
+    fmt::print("RoundTrip: Assembled code does not match input code\n");
-    printf("%s", text.c_str());
+    return false;
   }
   return true;
 }
@@ -57,25 +59,54 @@ static bool SuperTrip(const char* asm_code)
   std::string text;
   if (!DSP::Assemble(asm_code, code1))
   {
-    printf("SuperTrip: First assembly failed\n");
+    fmt::print("SuperTrip: First assembly failed\n");
     return false;
   }
-  printf("First assembly: %i words\n", (int)code1.size());
+  fmt::print("First assembly: {} words\n", code1.size());
 
-  if (!RoundTrippableDissassemble(code1, text))
+  if (!RoundTrippableDisassemble(code1, text))
   {
-    printf("SuperTrip: Disassembly failed\n");
+    fmt::print("SuperTrip: Disassembly failed\n");
     return false;
   }
   else
   {
-    printf("Disassembly:\n");
+    fmt::print("Disassembly:\n");
-    printf("%s", text.c_str());
+    fmt::print("{}", text);
   }
 
   if (!DSP::Assemble(text, code2))
   {
-    printf("SuperTrip: Second assembly failed\n");
+    fmt::print("SuperTrip: Second assembly failed\n");
+    return false;
+  }
+
+  if (!DSP::Compare(code1, code2))
+  {
+    fmt::print("SuperTrip: Assembled code does not match between passes\n");
+    return false;
+  }
+  return true;
+}
+
+// Assembles asm_code, and verifies that it matches code1.
+static bool AssembleAndCompare(const char* asm_code, const std::vector<u16>& code1)
+{
+  std::vector<u16> code2;
+  if (!DSP::Assemble(asm_code, code2))
+  {
+    fmt::print("AssembleAndCompare: Assembly failed\n");
+    return false;
+  }
+
+  fmt::print("AssembleAndCompare: Produced {} words; padding to {} words\n", code2.size(),
+             code1.size());
+  while (code2.size() < code1.size())
+    code2.push_back(0);
+
+  if (!DSP::Compare(code1, code2))
+  {
+    fmt::print("AssembleAndCompare: Assembled code does not match expected code\n");
     return false;
   }
   return true;
@@ -129,11 +160,21 @@ TEST(DSPAssembly, ExtendedInstructions)
                         "   ADDAXL'MV   $ACC1, $AX1.L : $AX1.H, $AC1.M\n"));
 }
 
+TEST(DSPAssembly, HermesText)
+{
+  ASSERT_TRUE(SuperTrip(s_hermes_text));
+}
+
 TEST(DSPAssembly, HermesBinary)
 {
   ASSERT_TRUE(RoundTrip(s_hermes_bin));
 }
 
+TEST(DSPAssembly, HermesAssemble)
+{
+  ASSERT_TRUE(AssembleAndCompare(s_hermes_text, s_hermes_bin));
+}
+
 TEST(DSPAssembly, DSPTestText)
 {
   ASSERT_TRUE(SuperTrip(s_dsp_test_text));
@@ -144,11 +185,7 @@ TEST(DSPAssembly, DSPTestBinary)
   ASSERT_TRUE(RoundTrip(s_dsp_test_bin));
 }
 
-/*
+TEST(DSPAssembly, DSPTestAssemble)
-
+{
-if (File::ReadFileToString("C:/devkitPro/examples/wii/asndlib/dsptest/dsp_test.ds", &dsp_test))
+  ASSERT_TRUE(AssembleAndCompare(s_dsp_test_text, s_dsp_test_bin));
-  SuperTrip(dsp_test.c_str());
+}
-
-//.File::ReadFileToString("C:/devkitPro/trunk/libogc/libasnd/dsp_mixer/dsp_mixer.s", &dsp_test);
-// This is CLOSE to working. Sorry about the local path btw. This is preliminary code.
-*/

Source/UnitTests/Core/DSP/DSPTestText.cpp

@@ -89,7 +89,7 @@ MEM_LO:			equ		0x0f7F
 	CW		0x1305
 	CW		0x1306
 
-	s40
+	s16
 	lri		$r12, #0x00ff
 
 main:
@@ -469,7 +469,7 @@ irq4:
 	jmp		irq
 irq5:
 ;	jmp finale
-	s40
+	s16
 	mrr		$r0d, $r1c
 	mrr		$r0d, $r1e
 	clr		$acc0
@@ -609,7 +609,7 @@ dma_copy:
 
 	ret
 
-send_back_16:
+send_back_40:
 
 	cw		0x8e00
 	call	send_back

Source/UnitTests/Core/DSP/HermesBinary.cpp

@@ -1,5 +1,13 @@
-// Copyright 2017 Dolphin Emulator Project
+/* DSP_MIXER -> PCM VOICE SOFTWARE PROCESSOR (8-16 Bits Mono/Stereo Voices)
-// SPDX-License-Identifier: GPL-2.0-or-later
+
+// Thanks to Duddie for you hard work and documentation
+
+Copyright (c) 2008 Hermes <www.entuwii.net>
+All rights reserved.
+
+SPDX-License-Identifier: BSD-3-Clause
+
+*/
 
 #include "HermesBinary.h"
 

Source/UnitTests/Core/DSP/HermesText.cpp

@@ -9,7 +9,9 @@ SPDX-License-Identifier: BSD-3-Clause
 
 */
 
+#include "HermesText.h"
 
+const char s_hermes_text[21370] = R"(
 /********************************/
 /**      REGISTER NAMES        **/
 /********************************/
@@ -173,7 +175,7 @@ MEM_SND:	equ	data_end ; it need 2048 words (4096 bytes)
 
 	lri     $CONFIG, #0xff
 	lri	$SR,#0
-	s40         
+	s16         
 	clr15       
 	m0         
 
@@ -254,7 +256,7 @@ sys_command:
 	jmp		recv_cmd
 	
 run_nexttask:
-	s40
+	s16
 	call		wait_for_cpu_mail
 	lrs			$29,@CMBL
 	call		wait_for_cpu_mail
@@ -542,7 +544,11 @@ no_delay:
 /////////////////////////////////////
 // end of delay time section
 /////////////////////////////////////
-
+)"  // Work around C2026 on MSVC, which allows at most 16380 single-byte characters in a single
+    // non-concatenated string literal (but you can concatenate multiple shorter string literals to
+    // produce a longer string just fine).  (This comment is not part of the actual test program,
+    // and instead there is a single blank line at this location.)
+                                  R"(
 /* bucle de generacion de samples */
 
 	
@@ -655,7 +661,7 @@ left_skip2:
 
 	cmp
 
-	jrl	$AR0 //get_sample or get_sample2 method
+	jrnc	$AR0 //get_sample or get_sample2 method
 
 	sr	@COUNTERH_SMP, $ACH1 
         sr	@COUNTERL_SMP, $ACM1
@@ -711,7 +717,7 @@ get_sample2: // slow method
 
 // if addr>addr end get a new buffer (if you uses double buffer)
 
-	jge	get_new_buffer
+	jc	get_new_buffer
 
 // load samples from dma, return $ar2 with the addr to get the samples and return using $ar0 to the routine to process 8-16bits Mono/Stereo
 
@@ -741,7 +747,7 @@ get_sample: // fast method
 // compares if the current address is >= end address to change the buffer or stops
 
 	cmp
-	jge	get_new_buffer
+	jc	get_new_buffer
 
 // load the new sample from the buffer
 
@@ -961,8 +967,8 @@ wait_dma:
 wait_for_dsp_mail:
 
 	lrs	$ACM1, @DMBH
-	andf	$ACM1, #0x8000
+	andcf	$ACM1, #0x8000
-	jnz	wait_for_dsp_mail
+	jlz	wait_for_dsp_mail
 	ret
 
 wait_for_cpu_mail:
@@ -1077,4 +1083,4 @@ polla_loca:
 	clr	$ACC0
 	jmp	recv_cmd
 
-
+)";

Source/UnitTests/Core/DSP/HermesText.h

@@ -0,0 +1,8 @@
+// Copyright 2022 Dolphin Emulator Project
+// SPDX-License-Identifier: GPL-2.0-or-later
+
+#pragma once
+
+#include <string>
+
+extern const char s_hermes_text[21370];

Source/UnitTests/Core/PageFaultTest.cpp

@@ -2,6 +2,7 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 
 #include <chrono>
+#include <fmt/format.h>
 
 #include "Common/CommonTypes.h"
 #include "Common/Timer.h"
@@ -79,16 +80,19 @@ TEST(PageFault, PageFault)
   perform_invalid_access(data);
   auto end = std::chrono::high_resolution_clock::now();
 
-#define AS_NS(diff)                                                                                \
+  auto difference_in_nanoseconds = [](auto start, auto end) {
-  ((unsigned long long)std::chrono::duration_cast<std::chrono::nanoseconds>(diff).count())
+    return std::chrono::duration_cast<std::chrono::nanoseconds>(end - start).count();
+  };
 
   EMM::UninstallExceptionHandler();
   JitInterface::SetJit(nullptr);
 
-  printf("page fault timing:\n");
+  fmt::print("page fault timing:\n");
-  printf("start->HandleFault     %llu ns\n", AS_NS(pfjit.m_pre_unprotect_time - start));
+  fmt::print("start->HandleFault     {} ns\n",
-  printf("UnWriteProtectMemory   %llu ns\n",
+             difference_in_nanoseconds(start, pfjit.m_pre_unprotect_time));
-         AS_NS(pfjit.m_post_unprotect_time - pfjit.m_pre_unprotect_time));
+  fmt::print("UnWriteProtectMemory   {} ns\n",
-  printf("HandleFault->end       %llu ns\n", AS_NS(end - pfjit.m_post_unprotect_time));
+             difference_in_nanoseconds(pfjit.m_pre_unprotect_time, pfjit.m_post_unprotect_time));
-  printf("total                  %llu ns\n", AS_NS(end - start));
+  fmt::print("HandleFault->end       {} ns\n",
+             difference_in_nanoseconds(pfjit.m_post_unprotect_time, end));
+  fmt::print("total                  {} ns\n", difference_in_nanoseconds(start, end));
 }

Source/UnitTests/UnitTests.vcxproj

@@ -28,6 +28,7 @@
     <ClInclude Include="Core\DSP\DSPTestBinary.h" />
     <ClInclude Include="Core\DSP\DSPTestText.h" />
     <ClInclude Include="Core\DSP\HermesBinary.h" />
+    <ClInclude Include="Core\DSP\HermesText.h" />
     <ClInclude Include="Core\IOS\ES\TestBinaryData.h" />
     <ClInclude Include="Core\PowerPC\TestValues.h" />
   </ItemGroup>
@@ -60,6 +61,7 @@
     <ClCompile Include="Core\DSP\DSPTestBinary.cpp" />
     <ClCompile Include="Core\DSP\DSPTestText.cpp" />
     <ClCompile Include="Core\DSP\HermesBinary.cpp" />
+    <ClCompile Include="Core\DSP\HermesText.cpp" />
     <ClCompile Include="Core\IOS\ES\FormatsTest.cpp" />
     <ClCompile Include="Core\IOS\FS\FileSystemTest.cpp" />
     <ClCompile Include="Core\MMIOTest.cpp" />