From 2193c8964ecb9750f445ef3f523896ea6cf175f4 Mon Sep 17 00:00:00 2001 From: Pokechu22 Date: Mon, 13 Jun 2022 16:23:44 -0700 Subject: [PATCH 01/17] DSPTool: Remove moved files from VS project file These were moved into UnitTests in #5449. --- Source/DSPTool/DSPTool.vcxproj | 6 ------ 1 file changed, 6 deletions(-) diff --git a/Source/DSPTool/DSPTool.vcxproj b/Source/DSPTool/DSPTool.vcxproj index 846377f990..20401a4ddb 100644 --- a/Source/DSPTool/DSPTool.vcxproj +++ b/Source/DSPTool/DSPTool.vcxproj @@ -21,12 +21,6 @@ Console - - - - - - From 3cb0976367205aff969a6eda1c3b601e40c7f24e Mon Sep 17 00:00:00 2001 From: Pokechu22 Date: Mon, 13 Jun 2022 17:03:19 -0700 Subject: [PATCH 02/17] UnitTests: Use hermes.s as part of an actual test Before, the file just existed as the source code for HermesBinary.cpp, but we can test that things assemble correctly too (compare DSPTestBinary.cpp and DSPTestText.cpp). A bit of jank is needed due to MSVC limitations (see https://docs.microsoft.com/en-us/cpp/error-messages/compiler-errors-1/compiler-error-c2026?view=msvc-170). --- Source/UnitTests/Core/CMakeLists.txt | 1 + Source/UnitTests/Core/DSP/DSPAssemblyTest.cpp | 15 ++++++--------- .../Core/DSP/{hermes.s => HermesText.cpp} | 10 ++++++++-- Source/UnitTests/Core/DSP/HermesText.h | 8 ++++++++ Source/UnitTests/UnitTests.vcxproj | 2 ++ 5 files changed, 25 insertions(+), 11 deletions(-) rename Source/UnitTests/Core/DSP/{hermes.s => HermesText.cpp} (97%) create mode 100644 Source/UnitTests/Core/DSP/HermesText.h diff --git a/Source/UnitTests/Core/CMakeLists.txt b/Source/UnitTests/Core/CMakeLists.txt index 134f7da78b..1e24489239 100644 --- a/Source/UnitTests/Core/CMakeLists.txt +++ b/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) diff --git a/Source/UnitTests/Core/DSP/DSPAssemblyTest.cpp b/Source/UnitTests/Core/DSP/DSPAssemblyTest.cpp index dd3c96e194..ecd543d168 100644 --- a/Source/UnitTests/Core/DSP/DSPAssemblyTest.cpp +++ b/Source/UnitTests/Core/DSP/DSPAssemblyTest.cpp @@ -8,6 +8,7 @@ #include "DSPTestBinary.h" #include "DSPTestText.h" #include "HermesBinary.h" +#include "HermesText.h" #include @@ -129,6 +130,11 @@ 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)); @@ -143,12 +149,3 @@ TEST(DSPAssembly, DSPTestBinary) { ASSERT_TRUE(RoundTrip(s_dsp_test_bin)); } - -/* - -if (File::ReadFileToString("C:/devkitPro/examples/wii/asndlib/dsptest/dsp_test.ds", &dsp_test)) - 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. -*/ diff --git a/Source/UnitTests/Core/DSP/hermes.s b/Source/UnitTests/Core/DSP/HermesText.cpp similarity index 97% rename from Source/UnitTests/Core/DSP/hermes.s rename to Source/UnitTests/Core/DSP/HermesText.cpp index d5418a578a..1b643a0c9b 100644 --- a/Source/UnitTests/Core/DSP/hermes.s +++ b/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 **/ /********************************/ @@ -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 */ @@ -1077,4 +1083,4 @@ polla_loca: clr $ACC0 jmp recv_cmd - +)"; diff --git a/Source/UnitTests/Core/DSP/HermesText.h b/Source/UnitTests/Core/DSP/HermesText.h new file mode 100644 index 0000000000..99c16686e2 --- /dev/null +++ b/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 + +extern const char s_hermes_text[21370]; diff --git a/Source/UnitTests/UnitTests.vcxproj b/Source/UnitTests/UnitTests.vcxproj index b13cab4d0d..da461087e9 100644 --- a/Source/UnitTests/UnitTests.vcxproj +++ b/Source/UnitTests/UnitTests.vcxproj @@ -28,6 +28,7 @@ + @@ -60,6 +61,7 @@ + From dd66dac5c166e2e4fb9195a914f10cdd2f9eb3c4 Mon Sep 17 00:00:00 2001 From: Pokechu22 Date: Mon, 13 Jun 2022 17:05:24 -0700 Subject: [PATCH 03/17] UnitTests: Fix typo in DSPAssemblyTest --- Source/UnitTests/Core/DSP/DSPAssemblyTest.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Source/UnitTests/Core/DSP/DSPAssemblyTest.cpp b/Source/UnitTests/Core/DSP/DSPAssemblyTest.cpp index ecd543d168..fabdc73c3c 100644 --- a/Source/UnitTests/Core/DSP/DSPAssemblyTest.cpp +++ b/Source/UnitTests/Core/DSP/DSPAssemblyTest.cpp @@ -12,7 +12,7 @@ #include -static bool RoundTrippableDissassemble(const std::vector& code, std::string& text) +static bool RoundTrippableDisassemble(const std::vector& code, std::string& text) { DSP::AssemblerSettings settings; settings.ext_separator = '\''; @@ -32,7 +32,7 @@ static bool RoundTrip(const std::vector& code1) { std::vector code2; std::string text; - if (!RoundTrippableDissassemble(code1, text)) + if (!RoundTrippableDisassemble(code1, text)) { printf("RoundTrip: Disassembly failed.\n"); return false; @@ -63,7 +63,7 @@ static bool SuperTrip(const char* asm_code) } printf("First assembly: %i words\n", (int)code1.size()); - if (!RoundTrippableDissassemble(code1, text)) + if (!RoundTrippableDisassemble(code1, text)) { printf("SuperTrip: Disassembly failed\n"); return false; From 8fac249581c3d344dd956a536d4b109ab587faa4 Mon Sep 17 00:00:00 2001 From: Pokechu22 Date: Mon, 13 Jun 2022 17:20:13 -0700 Subject: [PATCH 04/17] UnitTests: Use fmt::print in PageFaultTest --- Source/UnitTests/Core/PageFaultTest.cpp | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/Source/UnitTests/Core/PageFaultTest.cpp b/Source/UnitTests/Core/PageFaultTest.cpp index e340ca7646..e873a28e73 100644 --- a/Source/UnitTests/Core/PageFaultTest.cpp +++ b/Source/UnitTests/Core/PageFaultTest.cpp @@ -2,6 +2,7 @@ // SPDX-License-Identifier: GPL-2.0-or-later #include +#include #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) \ - ((unsigned long long)std::chrono::duration_cast(diff).count()) + auto difference_in_nanoseconds = [](auto start, auto end) { + return std::chrono::duration_cast(end - start).count(); + }; EMM::UninstallExceptionHandler(); JitInterface::SetJit(nullptr); - printf("page fault timing:\n"); - printf("start->HandleFault %llu ns\n", AS_NS(pfjit.m_pre_unprotect_time - start)); - printf("UnWriteProtectMemory %llu ns\n", - AS_NS(pfjit.m_post_unprotect_time - pfjit.m_pre_unprotect_time)); - printf("HandleFault->end %llu ns\n", AS_NS(end - pfjit.m_post_unprotect_time)); - printf("total %llu ns\n", AS_NS(end - start)); + fmt::print("page fault timing:\n"); + fmt::print("start->HandleFault {} ns\n", + difference_in_nanoseconds(start, pfjit.m_pre_unprotect_time)); + fmt::print("UnWriteProtectMemory {} ns\n", + difference_in_nanoseconds(pfjit.m_pre_unprotect_time, pfjit.m_post_unprotect_time)); + 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)); } From d8803a129849f087e670a4f29892e769190f6b77 Mon Sep 17 00:00:00 2001 From: Pokechu22 Date: Mon, 13 Jun 2022 17:23:23 -0700 Subject: [PATCH 05/17] UnitTests: Use fmt::print in DSPAssemblyTest --- Source/UnitTests/Core/DSP/DSPAssemblyTest.cpp | 20 ++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/Source/UnitTests/Core/DSP/DSPAssemblyTest.cpp b/Source/UnitTests/Core/DSP/DSPAssemblyTest.cpp index fabdc73c3c..c9d21bb9a0 100644 --- a/Source/UnitTests/Core/DSP/DSPAssemblyTest.cpp +++ b/Source/UnitTests/Core/DSP/DSPAssemblyTest.cpp @@ -1,6 +1,8 @@ // Copyright 2017 Dolphin Emulator Project // SPDX-License-Identifier: GPL-2.0-or-later +#include + #include "Common/FileUtil.h" #include "Core/DSP/DSPCodeUtil.h" #include "Core/DSP/DSPDisassembler.h" @@ -34,18 +36,18 @@ static bool RoundTrip(const std::vector& code1) std::string 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); - printf("%s", text.c_str()); + fmt::print("{}", text); } return true; } @@ -58,25 +60,25 @@ 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 (!RoundTrippableDisassemble(code1, text)) { - printf("SuperTrip: Disassembly failed\n"); + fmt::print("SuperTrip: Disassembly failed\n"); return false; } else { - printf("Disassembly:\n"); - printf("%s", text.c_str()); + fmt::print("Disassembly:\n"); + fmt::print("{}", text); } if (!DSP::Assemble(text, code2)) { - printf("SuperTrip: Second assembly failed\n"); + fmt::print("SuperTrip: Second assembly failed\n"); return false; } return true; From 693a29f8ceeb1166658d9c0752b6e84660077374 Mon Sep 17 00:00:00 2001 From: Pokechu22 Date: Mon, 13 Jun 2022 17:55:43 -0700 Subject: [PATCH 06/17] DSPCodeUtil: Use fmt::print instead of logging in DSP::Compare --- Source/Core/Core/DSP/DSPCodeUtil.cpp | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/Source/Core/Core/DSP/DSPCodeUtil.cpp b/Source/Core/Core/DSP/DSPCodeUtil.cpp index 9a33edc9ba..f7d358e710 100644 --- a/Source/Core/Core/DSP/DSPCodeUtil.cpp +++ b/Source/Core/Core/DSP/DSPCodeUtil.cpp @@ -57,10 +57,12 @@ bool Disassemble(const std::vector& 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& code1, const std::vector& 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(std::min(code1.size(), code2.size())); @@ -79,23 +81,23 @@ bool Compare(const std::vector& code1, const std::vector& code2) disassembler.DisassembleOpcode(&code1[0], &pc, line1); pc = i; disassembler.DisassembleOpcode(&code2[0], &pc, line2); - WARN_LOG_FMT(AUDIO, "!! {:04x} : {:04x} vs {:04x} - {} vs {}\n", i, code1[i], code2[i], - line1, line2); + fmt::print("!! {:04x} : {:04x} vs {:04x} - {} vs {}\n", i, code1[i], 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& 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); - DEBUG_LOG_FMT(AUDIO, "!! {}\n", line); + fmt::print("!! {}\n", 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; } From dc353ed84d780c0a7bd2c6d98e893a6db304933e Mon Sep 17 00:00:00 2001 From: Pokechu22 Date: Mon, 13 Jun 2022 17:56:49 -0700 Subject: [PATCH 07/17] DSPTool: Exit with status 1 if binary comparison fails --- Source/DSPTool/DSPTool.cpp | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/Source/DSPTool/DSPTool.cpp b/Source/DSPTool/DSPTool.cpp index 3c4b350479..41299771ce 100644 --- a/Source/DSPTool/DSPTool.cpp +++ b/Source/DSPTool/DSPTool.cpp @@ -128,7 +128,7 @@ static std::string CodesToHeader(const std::vector>& 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 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 0; + return PerformBinaryComparison(input_name, output_name) ? 0 : 1; } if (print_results) From cad9801ded56ecee0922ebb8d304cacb6c69541d Mon Sep 17 00:00:00 2001 From: Pokechu22 Date: Mon, 13 Jun 2022 18:34:17 -0700 Subject: [PATCH 08/17] DSPDisassembler: Fix out-of-bounds read when the last word is an instruction with a large immediate For instance, ending with 0x009e (which you can do with CW 0x009e) indicates a LRI $ac0.m instruction, but there is no immediate value to load, so before whatever garbage in memory existed after the end of the file was used. The bounds-checking also previously assumed that IRAM or IROM was being used, both of which were exactly 0x1000 long. --- Source/Core/Core/DSP/DSPCodeUtil.cpp | 6 ++--- Source/Core/Core/DSP/DSPDisassembler.cpp | 31 ++++++++++++++++++----- Source/Core/Core/DSP/DSPDisassembler.h | 6 +++-- Source/Core/Core/HW/DSPLLE/DSPSymbols.cpp | 4 ++- 4 files changed, 34 insertions(+), 13 deletions(-) diff --git a/Source/Core/Core/DSP/DSPCodeUtil.cpp b/Source/Core/Core/DSP/DSPCodeUtil.cpp index f7d358e710..b825c15776 100644 --- a/Source/Core/Core/DSP/DSPCodeUtil.cpp +++ b/Source/Core/Core/DSP/DSPCodeUtil.cpp @@ -78,9 +78,9 @@ bool Compare(const std::vector& code1, const std::vector& 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); fmt::print("!! {:04x} : {:04x} vs {:04x} - {} vs {}\n", i, code1[i], code2[i], line1, line2); } @@ -93,7 +93,7 @@ bool Compare(const std::vector& code1, const std::vector& code2) { u16 pc = i; std::string line; - disassembler.DisassembleOpcode(&longest[0], &pc, line); + disassembler.DisassembleOpcode(longest, &pc, line); fmt::print("!! {}\n", line); } } diff --git a/Source/Core/Core/DSP/DSPDisassembler.cpp b/Source/Core/Core/DSP/DSPDisassembler.cpp index 01817c0780..91f9b8f593 100644 --- a/Source/Core/Core/DSP/DSPDisassembler.cpp +++ b/Source/Core/Core/DSP/DSPDisassembler.cpp @@ -33,9 +33,10 @@ bool DSPDisassembler::Disassemble(const std::vector& code, std::string& tex for (u16 pc = 0; pc < code.size();) { - if (!DisassembleOpcode(code.data(), &pc, text)) - return false; + bool failed = !DisassembleOpcode(code, &pc, text); text.append("\n"); + if (failed) + return false; } return true; } @@ -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& 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); } diff --git a/Source/Core/Core/DSP/DSPDisassembler.h b/Source/Core/Core/DSP/DSPDisassembler.h index 37a556050c..294cc8df52 100644 --- a/Source/Core/Core/DSP/DSPDisassembler.h +++ b/Source/Core/Core/DSP/DSPDisassembler.h @@ -34,8 +34,10 @@ public: bool Disassemble(const std::vector& code, std::string& text); - // Warning - this one is trickier to use right. - bool DisassembleOpcode(const u16* binbuf, u16* pc, std::string& dest); + // Disassembles the given opcode at pc and increases pc by the opcode's size. + // The PC is wrapped such that 0x0000 and 0x8000 both point to the start of the buffer. + bool DisassembleOpcode(const std::vector& 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); diff --git a/Source/Core/Core/HW/DSPLLE/DSPSymbols.cpp b/Source/Core/Core/HW/DSPLLE/DSPSymbols.cpp index ab1ff3ce0b..8f190f1ebe 100644 --- a/Source/Core/Core/HW/DSPLLE/DSPSymbols.cpp +++ b/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; From 087d89225c351bd45c8ccd4550e9dc4861f6312d Mon Sep 17 00:00:00 2001 From: Pokechu22 Date: Mon, 13 Jun 2022 18:49:59 -0700 Subject: [PATCH 09/17] DSPCodeUtil: Give better output when comparing instructions with large immediates --- Source/Core/Core/DSP/DSPCodeUtil.cpp | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/Source/Core/Core/DSP/DSPCodeUtil.cpp b/Source/Core/Core/DSP/DSPCodeUtil.cpp index b825c15776..d196a5a5b5 100644 --- a/Source/Core/Core/DSP/DSPCodeUtil.cpp +++ b/Source/Core/Core/DSP/DSPCodeUtil.cpp @@ -83,6 +83,27 @@ bool Compare(const std::vector& code1, const std::vector& code2) disassembler.DisassembleOpcode(code2, &pc, line2); fmt::print("!! {:04x} : {:04x} vs {:04x} - {} vs {}\n", i, code1[i], code2[i], line1, 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()) From 2d774010c3db58af0601684dcb0e348cf3f638e5 Mon Sep 17 00:00:00 2001 From: Pokechu22 Date: Mon, 13 Jun 2022 18:50:42 -0700 Subject: [PATCH 10/17] DSPCodeUtil: Include the PC and hex in the "Extra code words" section It's included in the section before, so it's helpful to supply here too. --- Source/Core/Core/DSP/DSPCodeUtil.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Source/Core/Core/DSP/DSPCodeUtil.cpp b/Source/Core/Core/DSP/DSPCodeUtil.cpp index d196a5a5b5..074dd86c3a 100644 --- a/Source/Core/Core/DSP/DSPCodeUtil.cpp +++ b/Source/Core/Core/DSP/DSPCodeUtil.cpp @@ -115,7 +115,7 @@ bool Compare(const std::vector& code1, const std::vector& code2) u16 pc = i; std::string line; disassembler.DisassembleOpcode(longest, &pc, line); - fmt::print("!! {}\n", line); + fmt::print("!! {:04x} : {:04x} - {}\n", i, longest[i], line); } } fmt::print("Equal instruction words: {} / {}\n", count_equal, min_size); From 6a2ec825a225d07208d61aeeaf62c3c5525745f0 Mon Sep 17 00:00:00 2001 From: Pokechu22 Date: Mon, 13 Jun 2022 19:00:15 -0700 Subject: [PATCH 11/17] UnitTests: Fail DSPAssemblyTest if the assembled code doesn't match the expected result This reveals that both HermesText and HermesBinary fail. HermesBinary would have failed on master, too, if this had been implemented. --- Source/UnitTests/Core/DSP/DSPAssemblyTest.cpp | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/Source/UnitTests/Core/DSP/DSPAssemblyTest.cpp b/Source/UnitTests/Core/DSP/DSPAssemblyTest.cpp index c9d21bb9a0..17e4d70b3d 100644 --- a/Source/UnitTests/Core/DSP/DSPAssemblyTest.cpp +++ b/Source/UnitTests/Core/DSP/DSPAssemblyTest.cpp @@ -46,8 +46,8 @@ static bool RoundTrip(const std::vector& code1) } if (!DSP::Compare(code1, code2)) { - DSP::Disassemble(code1, true, text); - fmt::print("{}", text); + fmt::print("RoundTrip: Assembled code does not match input code\n"); + return false; } return true; } @@ -81,6 +81,12 @@ static bool SuperTrip(const char* asm_code) 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; } From 41939eeaf9850f8c0859292720ed3d8101e0e70c Mon Sep 17 00:00:00 2001 From: Pokechu22 Date: Mon, 13 Jun 2022 19:24:37 -0700 Subject: [PATCH 12/17] DSPDisassembler: Fix disassembly of LSR and ASR Before, both 1441 and 147f would disassemble as `lsr $acc0, #1`, when the second should be `lsr $acc0, #-1`, and both 14c1 and 14ff would be `asr $acc0, #1` when the second should be `asr $acc0, #-1`. I'm not entirely sure whether the minus signs actually make sense here, but this change is consistent with the assembler so that's an improvement at least. devkitPro previously changed the formatting to not require negative signs for lsr and asr; this is probably something we should do in the future: https://github.com/devkitPro/gamecube-tools/commit/8a65c85c9b4748ed3dab3c0c85aeb0df95d58cfb This fixes the HermesText and HermesBinary tests (HermesText already wrote `lsr $ACC0, #-5`, so this is consistent with what it used before.) --- Source/Core/Core/DSP/DSPDisassembler.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Source/Core/Core/DSP/DSPDisassembler.cpp b/Source/Core/Core/DSP/DSPDisassembler.cpp index 91f9b8f593..6e833cdf16 100644 --- a/Source/Core/Core/DSP/DSPDisassembler.cpp +++ b/Source/Core/Core/DSP/DSPDisassembler.cpp @@ -108,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 { From d52528a6f0f3c06d38d31447bf861d8773f9bb1b Mon Sep 17 00:00:00 2001 From: Pokechu22 Date: Mon, 13 Jun 2022 19:52:40 -0700 Subject: [PATCH 13/17] UnitTests: Add tests for assembling DSP code to expected binary We already have the data for this, so this seems like a useful thing to do. However, neither of the new tests currently pass... --- Source/UnitTests/Core/DSP/DSPAssemblyTest.cpp | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/Source/UnitTests/Core/DSP/DSPAssemblyTest.cpp b/Source/UnitTests/Core/DSP/DSPAssemblyTest.cpp index 17e4d70b3d..8f99fd83d3 100644 --- a/Source/UnitTests/Core/DSP/DSPAssemblyTest.cpp +++ b/Source/UnitTests/Core/DSP/DSPAssemblyTest.cpp @@ -90,6 +90,29 @@ static bool SuperTrip(const char* asm_code) return true; } +// Assembles asm_code, and verifies that it matches code1. +static bool AssembleAndCompare(const char* asm_code, const std::vector& code1) +{ + std::vector 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; +} + // Let's start out easy - a trivial instruction.. TEST(DSPAssembly, TrivialInstruction) { @@ -148,6 +171,11 @@ 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)); @@ -157,3 +185,8 @@ TEST(DSPAssembly, DSPTestBinary) { ASSERT_TRUE(RoundTrip(s_dsp_test_bin)); } + +TEST(DSPAssembly, DSPTestAssemble) +{ + ASSERT_TRUE(AssembleAndCompare(s_dsp_test_text, s_dsp_test_bin)); +} From 36769017c0417138b77e2a4012dffd6add7cc2cd Mon Sep 17 00:00:00 2001 From: Pokechu22 Date: Mon, 13 Jun 2022 21:46:03 -0700 Subject: [PATCH 14/17] UnitTests: Update DSP test text for old renames This change makes assembling DSPTestText match DSPTestBinary, though HermesText doesn't yet match HermesBinary. The test data was originally added on April 18, 2009 in e7e4ef4481d1c5dcce6463414d815843ae15b8e2. Then, set16 and set40 were swapped on April 22, 2009 89178f411cf1e8ef61b005197545618e53a0d4e8, which updated the DSPSpy version of dsp_code, but not the version in DSPTool used for testing. So, when the test was made, the assembled data matched the text, but a few days after it no longer did. Similarly, on Jul 7, 2009 in 1654c582ab9a6323a31802c4b241c0cffc9d3bca the conditional instructions were adjusted, and 0x1706 was changed from JRL to JRNC and 0x0297 was changed from JGE to JC. For what it's worth, devkitPro made the same changes on May 31, 2010 in https://github.com/devkitPro/gamecube-tools/commit/8a65c85c9b4748ed3dab3c0c85aeb0df95d58cfb and updated their version of the asnd ucode (which is this ucode) on June 11, 2011 in https://github.com/devkitpro/libogc/commit/b1b8ecab3af3745c8df0b401abd512bdf5fcc011 (though this update also includes other feature changes). Note that at the time, they didn't reassemble the ucode unless they made changes to it; the assembled was stored in the repo until https://github.com/devkitPro/libogc/compare/bfb705fe1607a3031d18b65d603975b68a1cffd4~...d20f9bdcfb43260c6c759f4fb98d724931443f93. This fixes the following failures with Hermes: !! 0015 : 8e00 vs 8f00 - set16 vs set40 !! 016f : 8e00 vs 8f00 - set16 vs set40 and with Hermes: !! 0014 : 8e00 vs 8f00 - set16 vs set40 !! 0063 : 8e00 vs 8f00 - set16 vs set40 !! 019b : 1706 vs 1701 - jrnc $AR0 vs jrl $AR0 !! 01bf : 0297 vs 0290 - jc 0x01dc vs jge 0x01dc !! 01d2 : 0297 vs 0290 - jc 0x01dc vs jge 0x01dc Hermes has the remaining failures: !! 027b : 03c0 vs 03a0 - andcf $AC1.M, #0x8000 vs andf $AC1.M, #0x8000 !! 027d : 029d vs 0294 - jlz 0x027a vs jnz 0x027a --- Source/UnitTests/Core/DSP/DSPTestText.cpp | 6 +++--- Source/UnitTests/Core/DSP/HermesText.cpp | 10 +++++----- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/Source/UnitTests/Core/DSP/DSPTestText.cpp b/Source/UnitTests/Core/DSP/DSPTestText.cpp index 2e1f60a450..bc02ef1ba2 100644 --- a/Source/UnitTests/Core/DSP/DSPTestText.cpp +++ b/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 diff --git a/Source/UnitTests/Core/DSP/HermesText.cpp b/Source/UnitTests/Core/DSP/HermesText.cpp index 1b643a0c9b..fd59677d35 100644 --- a/Source/UnitTests/Core/DSP/HermesText.cpp +++ b/Source/UnitTests/Core/DSP/HermesText.cpp @@ -175,7 +175,7 @@ MEM_SND: equ data_end ; it need 2048 words (4096 bytes) lri $CONFIG, #0xff lri $SR,#0 - s40 + s16 clr15 m0 @@ -256,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 @@ -661,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 @@ -717,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 @@ -747,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 From 75ff89e8c7ff2befd77d512818d70aa032d73db6 Mon Sep 17 00:00:00 2001 From: Pokechu22 Date: Mon, 13 Jun 2022 23:07:14 -0700 Subject: [PATCH 15/17] UnitTests: Edit wait_for_dsp_mail in HermesText to match HermesBinary I don't know what happened here, unfortunately. The version of dsp_mixer.s added to libogc on Nov 14, 2008 in https://github.com/devkitPro/libogc/commit/c76d8b851fafc11b0a5debc0b40842929d5a5825 uses andcf and jlz here, and the version we have matches the one from Feb 5, 2009 in https://github.com/devkitPro/libogc/commit/ae5c3a5fb5aa4ccf1e1374b6e4f6be1aac118693 exactly (prior to the fixes in my previous commit). I can't see any reason why wait_for_dsp_mail would be changed like this. ANDCF and ANDF were previously swapped and JNE/JEQ/JZR/JNZ became JNZ/JZ/JLNZ/JLZ on Apr 3, 2009 in 7c4e6542533f7cef929ce86117b156c714820618, corresponding to a change Hermes made on Nov 10, 2008 in https://github.com/devkitPro/gamecube-tools/commit/2cea6d99ad518c963e24c3e28834473ff5312e69. But these predate the test being added. The only other information I can find is that ASNDLIB 1.0 released on November 11, 2008, at https://web.archive.org/web/20120326145022/http://www.entuwii.net/foro/viewtopic.php?f=6&t=87 (but there aren't any surviving links from there). --- Source/UnitTests/Core/DSP/HermesText.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Source/UnitTests/Core/DSP/HermesText.cpp b/Source/UnitTests/Core/DSP/HermesText.cpp index fd59677d35..f7f83a9737 100644 --- a/Source/UnitTests/Core/DSP/HermesText.cpp +++ b/Source/UnitTests/Core/DSP/HermesText.cpp @@ -967,8 +967,8 @@ wait_dma: wait_for_dsp_mail: lrs $ACM1, @DMBH - andf $ACM1, #0x8000 - jnz wait_for_dsp_mail + andcf $ACM1, #0x8000 + jlz wait_for_dsp_mail ret wait_for_cpu_mail: From 5ea3efaedf0e319c6f4011970911a20246c8927b Mon Sep 17 00:00:00 2001 From: Pokechu22 Date: Mon, 13 Jun 2022 23:27:31 -0700 Subject: [PATCH 16/17] UnitTests: Fix license for HermesBinary.cpp This is an assembled version of HermesText.cpp, so the same license applies to it. --- Source/UnitTests/Core/DSP/HermesBinary.cpp | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/Source/UnitTests/Core/DSP/HermesBinary.cpp b/Source/UnitTests/Core/DSP/HermesBinary.cpp index 299f50e16c..a7f485d0a0 100644 --- a/Source/UnitTests/Core/DSP/HermesBinary.cpp +++ b/Source/UnitTests/Core/DSP/HermesBinary.cpp @@ -1,5 +1,13 @@ -// Copyright 2017 Dolphin Emulator Project -// SPDX-License-Identifier: GPL-2.0-or-later +/* DSP_MIXER -> PCM VOICE SOFTWARE PROCESSOR (8-16 Bits Mono/Stereo Voices) + +// Thanks to Duddie for you hard work and documentation + +Copyright (c) 2008 Hermes +All rights reserved. + +SPDX-License-Identifier: BSD-3-Clause + +*/ #include "HermesBinary.h" From dec48ed7de1e9710dc2aad5690e3a32b8ee8ed13 Mon Sep 17 00:00:00 2001 From: Pokechu22 Date: Tue, 14 Jun 2022 13:07:14 -0700 Subject: [PATCH 17/17] UnitTests: Remove unused include from DSPAssemblyTest --- Source/UnitTests/Core/DSP/DSPAssemblyTest.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/Source/UnitTests/Core/DSP/DSPAssemblyTest.cpp b/Source/UnitTests/Core/DSP/DSPAssemblyTest.cpp index 8f99fd83d3..44e4cde30e 100644 --- a/Source/UnitTests/Core/DSP/DSPAssemblyTest.cpp +++ b/Source/UnitTests/Core/DSP/DSPAssemblyTest.cpp @@ -3,7 +3,6 @@ #include -#include "Common/FileUtil.h" #include "Core/DSP/DSPCodeUtil.h" #include "Core/DSP/DSPDisassembler.h"