From bd3173e344912d0b2031c46407e2f18cf6a059fd Mon Sep 17 00:00:00 2001 From: Pokechu22 Date: Mon, 23 May 2022 14:10:49 -0700 Subject: [PATCH] DSPAssembler: Rework errors and warnings Among other things, this trims trailing newline characters. Before (on windows) the \r would corrupt the output and make them very hard to understand (as the error message would be drawn over the code line, but part of the code line would peek out from behind it). --- Source/Core/Core/DSP/DSPAssembler.cpp | 248 ++++++++++++++------------ Source/Core/Core/DSP/DSPAssembler.h | 26 ++- Source/Core/Core/DSP/DSPCodeUtil.cpp | 8 +- 3 files changed, 156 insertions(+), 126 deletions(-) diff --git a/Source/Core/Core/DSP/DSPAssembler.cpp b/Source/Core/Core/DSP/DSPAssembler.cpp index 67077f8675..ed181b7a3c 100644 --- a/Source/Core/Core/DSP/DSPAssembler.cpp +++ b/Source/Core/Core/DSP/DSPAssembler.cpp @@ -26,6 +26,42 @@ #include "Core/DSP/DSPDisassembler.h" #include "Core/DSP/DSPTables.h" +template <> +struct fmt::formatter +{ + constexpr auto parse(format_parse_context& ctx) { return ctx.begin(); } + template + auto format(const DSP::DSPAssembler::LocationContext& loc, FormatContext& ctx) const + { + // Trim trailing newlines + // TODO: Ideally, we shouldn't be getting any newlines at all here, but + // DSPTool uses File::ReadFileToString, which opens the file in binary mode, + // so we get \r endings on Windows. This leads to bad results when printing the string. + std::string trimmed_line = loc.line_text; + // while (trimmed_line.ends_with('\r') || trimmed_line.ends_with('\n')) + while (!trimmed_line.empty() && (trimmed_line.back() == '\r' || trimmed_line.back() == '\n')) + trimmed_line.pop_back(); + + auto out = fmt::format_to(ctx.out(), "At line {}", loc.line_num); + + if (loc.included_file_path.has_value()) + out = fmt::format_to(out, " of included file {}", loc.included_file_path.value()); + + if (loc.opcode_type.has_value()) + { + if (loc.opcode_type.value() == DSP::DSPAssembler::OpcodeType::Primary) + out = fmt::format_to(out, ", main opcode"); + else if (loc.opcode_type.value() == DSP::DSPAssembler::OpcodeType::Extension) + out = fmt::format_to(out, ", extension opcode"); + + if (loc.opcode_param_number.has_value()) + out = fmt::format_to(out, " parameter {}", loc.opcode_param_number.value()); + } + + return fmt::format_to(out, ":\n{}", trimmed_line); + } +}; + namespace DSP { static const char* err_string[] = {"", @@ -88,31 +124,39 @@ bool DSPAssembler::Assemble(const std::string& text, std::vector& code, return true; } -void DSPAssembler::ShowError(AssemblerError err_code, const char* extra_info) +void DSPAssembler::ShowError(AssemblerError err_code) { if (!m_settings.force) m_failed = true; - std::string error = fmt::format("{} : {} ", m_code_line, m_cur_line); - if (!extra_info) - extra_info = "-"; - - const char* const error_string = err_string[static_cast(err_code)]; - - if (m_current_param == 0) - { - error += fmt::format("ERROR: {} Line: {} : {}\n", error_string, m_code_line, extra_info); - } - else - { - error += fmt::format("ERROR: {} Line: {} Param: {} : {}\n", error_string, m_code_line, - m_current_param, extra_info); - } - - m_last_error_str = std::move(error); + m_last_error_str = + fmt::format("{}\nERROR: {}\n\n", m_location, err_string[static_cast(err_code)]); + fmt::print(stderr, "{}", m_last_error_str); m_last_error = err_code; } +template +void DSPAssembler::ShowError(AssemblerError err_code, fmt::format_string format, + Args&&... args) +{ + if (!m_settings.force) + m_failed = true; + + const auto msg = fmt::format(format, std::forward(args)...); + + m_last_error_str = fmt::format("{}\nERROR: {}: {}\n\n", m_location, + err_string[static_cast(err_code)], msg); + fmt::print(stderr, "{}", m_last_error_str); + m_last_error = err_code; +} + +template +void DSPAssembler::ShowWarning(fmt::format_string format, Args&&... args) +{ + const auto msg = fmt::format(format, std::forward(args)...); + fmt::print(stderr, "{}\nWARNING: {}\n\n", m_location, msg); +} + static char* skip_spaces(char* ptr) { while (*ptr == ' ') @@ -147,7 +191,7 @@ s32 DSPAssembler::ParseValue(const char* str) if (ptr[i] >= '0' && ptr[i] <= '9') val += ptr[i] - '0'; else - ShowError(AssemblerError::IncorrectDecimal, str); + ShowError(AssemblerError::IncorrectDecimal, "{}", str); } } else @@ -165,7 +209,7 @@ s32 DSPAssembler::ParseValue(const char* str) else if (ptr[i] >= '0' && ptr[i] <= '9') val += (ptr[i] - '0'); else - ShowError(AssemblerError::IncorrectHex, str); + ShowError(AssemblerError::IncorrectHex, "{}", str); } break; case '\'': // binary @@ -175,7 +219,7 @@ s32 DSPAssembler::ParseValue(const char* str) if (ptr[i] >= '0' && ptr[i] <= '1') val += ptr[i] - '0'; else - ShowError(AssemblerError::IncorrectBinary, str); + ShowError(AssemblerError::IncorrectBinary, "{}", str); } break; default: @@ -196,7 +240,7 @@ s32 DSPAssembler::ParseValue(const char* str) if (ptr[i] >= '0' && ptr[i] <= '9') val += ptr[i] - '0'; else - ShowError(AssemblerError::IncorrectDecimal, str); + ShowError(AssemblerError::IncorrectDecimal, "{}", str); } } else // Everything else is a label. @@ -206,7 +250,7 @@ s32 DSPAssembler::ParseValue(const char* str) return *value; if (m_cur_pass == 2) - ShowError(AssemblerError::UnknownLabel, str); + ShowError(AssemblerError::UnknownLabel, "{}", str); } } if (negative) @@ -332,10 +376,10 @@ u32 DSPAssembler::ParseExpression(const char* ptr) val = ParseExpression(d_buffer) - ParseExpression(pbuf + 1); if (val < 0) { + ShowWarning("Number Underflow: {}", val); val = 0x10000 + (val & 0xffff); // ATTENTION: avoid a terrible bug!!! number cannot write with '-' in sprintf - fprintf(stderr, "WARNING: Number Underflow at Line: %d \n", m_code_line); } sprintf(d_buffer, "%d", val); } @@ -472,7 +516,7 @@ bool DSPAssembler::VerifyParams(const DSPOPCTemplate* opc, param_t* par, size_t { for (size_t i = 0; i < count; i++) { - const size_t current_param = i + 1; // just for display. + m_location.opcode_param_number = i + 1; if (opc->params[i].type != par[i].type || (par[i].type & P_REG)) { if (par[i].type == P_VAL && @@ -496,10 +540,6 @@ bool DSPAssembler::VerifyParams(const DSPOPCTemplate* opc, param_t* par, size_t if ((int)par[i].val < value || (int)par[i].val > value + get_mask_shifted_down(opc->params[i].mask)) { - if (type == OpcodeType::Extension) - fprintf(stderr, "(ext) "); - - fprintf(stderr, "%s (param %zu)", m_cur_line.c_str(), current_param); ShowError(AssemblerError::InvalidRegister); } break; @@ -507,34 +547,19 @@ bool DSPAssembler::VerifyParams(const DSPOPCTemplate* opc, param_t* par, size_t case P_PRG: if ((int)par[i].val < DSP_REG_AR0 || (int)par[i].val > DSP_REG_AR3) { - if (type == OpcodeType::Extension) - fprintf(stderr, "(ext) "); - - fprintf(stderr, "%s (param %zu)", m_cur_line.c_str(), current_param); ShowError(AssemblerError::InvalidRegister); } break; case P_ACC: if ((int)par[i].val < DSP_REG_ACC0_FULL || (int)par[i].val > DSP_REG_ACC1_FULL) { - if (type == OpcodeType::Extension) - fprintf(stderr, "(ext) "); - if (par[i].val >= DSP_REG_ACM0 && par[i].val <= DSP_REG_ACM1) { - fprintf(stderr, "%i : %s ", m_code_line, m_cur_line.c_str()); - fprintf(stderr, - "WARNING: $ACM%d register used instead of $ACC%d register Line: %d " - "Param: %zu Ext: %d\n", - (par[i].val & 1), (par[i].val & 1), m_code_line, current_param, - static_cast(type)); + ShowWarning("$ACM{0} register used instead of $ACC{0} register", (par[i].val & 1)); } else if (par[i].val >= DSP_REG_ACL0 && par[i].val <= DSP_REG_ACL1) { - fprintf( - stderr, - "WARNING: $ACL%d register used instead of $ACC%d register Line: %d Param: %zu\n", - (par[i].val & 1), (par[i].val & 1), m_code_line, current_param); + ShowWarning("$ACL{0} register used instead of $ACC{0} register", (par[i].val & 1)); } else { @@ -545,22 +570,13 @@ bool DSPAssembler::VerifyParams(const DSPOPCTemplate* opc, param_t* par, size_t case P_ACCM: if ((int)par[i].val < DSP_REG_ACM0 || (int)par[i].val > DSP_REG_ACM1) { - if (type == OpcodeType::Extension) - fprintf(stderr, "(ext) "); - if (par[i].val >= DSP_REG_ACL0 && par[i].val <= DSP_REG_ACL1) { - fprintf( - stderr, - "WARNING: $ACL%d register used instead of $ACM%d register Line: %d Param: %zu\n", - (par[i].val & 1), (par[i].val & 1), m_code_line, current_param); + ShowWarning("$ACL{0} register used instead of $ACCM{0} register", (par[i].val & 1)); } else if (par[i].val >= DSP_REG_ACC0_FULL && par[i].val <= DSP_REG_ACC1_FULL) { - fprintf( - stderr, - "WARNING: $ACC%d register used instead of $ACM%d register Line: %d Param: %zu\n", - (par[i].val & 1), (par[i].val & 1), m_code_line, current_param); + ShowWarning("$ACC{0} register used instead of $ACM{0} register", (par[i].val & 1)); } else { @@ -572,24 +588,13 @@ bool DSPAssembler::VerifyParams(const DSPOPCTemplate* opc, param_t* par, size_t case P_ACCL: if ((int)par[i].val < DSP_REG_ACL0 || (int)par[i].val > DSP_REG_ACL1) { - if (type == OpcodeType::Extension) - fprintf(stderr, "(ext) "); - if (par[i].val >= DSP_REG_ACM0 && par[i].val <= DSP_REG_ACM1) { - fprintf(stderr, "%s ", m_cur_line.c_str()); - fprintf( - stderr, - "WARNING: $ACM%d register used instead of $ACL%d register Line: %d Param: %zu\n", - (par[i].val & 1), (par[i].val & 1), m_code_line, current_param); + ShowWarning("$ACM{0} register used instead of $ACL{0} register", (par[i].val & 1)); } else if (par[i].val >= DSP_REG_ACC0_FULL && par[i].val <= DSP_REG_ACC1_FULL) { - fprintf(stderr, "%s ", m_cur_line.c_str()); - fprintf( - stderr, - "WARNING: $ACC%d register used instead of $ACL%d register Line: %d Param: %zu\n", - (par[i].val & 1), (par[i].val & 1), m_code_line, current_param); + ShowWarning("$ACC{0} register used instead of $ACL{0} register", (par[i].val & 1)); } else { @@ -604,23 +609,15 @@ bool DSPAssembler::VerifyParams(const DSPOPCTemplate* opc, param_t* par, size_t switch (par[i].type & (P_REG | 7)) { case P_REG: - if (type == OpcodeType::Extension) - fprintf(stderr, "(ext) "); ShowError(AssemblerError::ExpectedParamReg); break; case P_MEM: - if (type == OpcodeType::Extension) - fprintf(stderr, "(ext) "); ShowError(AssemblerError::ExpectedParamMem); break; case P_VAL: - if (type == OpcodeType::Extension) - fprintf(stderr, "(ext) "); ShowError(AssemblerError::ExpectedParamVal); break; case P_IMM: - if (type == OpcodeType::Extension) - fprintf(stderr, "(ext) "); ShowError(AssemblerError::ExpectedParamImm); break; } @@ -634,40 +631,49 @@ bool DSPAssembler::VerifyParams(const DSPOPCTemplate* opc, param_t* par, size_t unsigned int valueu = 0xffff & ~(value >> 1); if ((int)par[i].val < 0) { - if (value == 7) // value 7 por sbclr/sbset + if (value == 7) // value 7 for sbclr/sbset { - fprintf(stderr, "Value must be from 0x0 to 0x%x\n", value); - ShowError(AssemblerError::NumberOutOfRange); + ShowError(AssemblerError::NumberOutOfRange, "Value must be from 0x0 to {:#x}, was {:#x}", + value, (int)par[i].val); } else if (opc->params[i].type == P_MEM) { if (value < 256) - fprintf(stderr, "Address value must be from 0x%x to 0x%x\n", valueu, (value >> 1)); + { + ShowError(AssemblerError::NumberOutOfRange, + "Address value must be from {:#x} to {:#x}, was {:#x}", valueu, (value >> 1), + (int)par[i].val); + } else - fprintf(stderr, "Address value must be from 0x0 to 0x%x\n", value); - - ShowError(AssemblerError::NumberOutOfRange); + { + ShowError(AssemblerError::NumberOutOfRange, + "Address value must be from 0x0 to {:#x}, was {:#x}", value, (int)par[i].val); + } } else if ((int)par[i].val < -((value >> 1) + 1)) { if (value < 128) - fprintf(stderr, "Value must be from -0x%x to 0x%x, is %i\n", (value >> 1) + 1, - value >> 1, par[i].val); + { + ShowError(AssemblerError::NumberOutOfRange, + "Value must be from {:#x} to {:#x}, was {:#x}", -((value >> 1) + 1), + value >> 1, (int)par[i].val); + } else - fprintf(stderr, "Value must be from -0x%x to 0x%x or 0x0 to 0x%x, is %i\n", - (value >> 1) + 1, value >> 1, value, par[i].val); - - ShowError(AssemblerError::NumberOutOfRange); + { + ShowError(AssemblerError::NumberOutOfRange, + "Value must be from {:#x} to {:#x} or 0x0 to {:#x}, was {:#x}", + -((value >> 1) + 1), value >> 1, value, (int)par[i].val); + } } } else { - if (value == 7) // value 7 por sbclr/sbset + if (value == 7) // value 7 for sbclr/sbset { if (par[i].val > (unsigned)value) { - fprintf(stderr, "Value must be from 0x%x to 0x%x, is %i\n", valueu, value, par[i].val); - ShowError(AssemblerError::NumberOutOfRange); + ShowError(AssemblerError::NumberOutOfRange, + "Value must be from {:#x} to {:#x}, was {:#x}\n", valueu, value, par[i].val); } } else if (opc->params[i].type == P_MEM) @@ -678,11 +684,17 @@ bool DSPAssembler::VerifyParams(const DSPOPCTemplate* opc, param_t* par, size_t (par[i].val < valueu || par[i].val > (unsigned)0xffff)) { if (value < 256) - fprintf(stderr, "Address value must be from 0x%x to 0x%x, is %04x\n", valueu, value, - par[i].val); + { + ShowError(AssemblerError::NumberOutOfRange, + "Address value must be from {:#x} to {:#x}, was {:04x}\n", valueu, value, + par[i].val); + } else - fprintf(stderr, "Address value must be minor of 0x%x\n", value + 1); - ShowError(AssemblerError::NumberOutOfRange); + { + ShowError(AssemblerError::NumberOutOfRange, + "Address value must be less than {:#x}, was {:04x}\n", value + 1, + par[i].val); + } } } else @@ -692,18 +704,23 @@ bool DSPAssembler::VerifyParams(const DSPOPCTemplate* opc, param_t* par, size_t if (par[i].val > (unsigned)value) { if (value < 64) - fprintf(stderr, "Value must be from -0x%x to 0x%x, is %i\n", (value + 1), value, - par[i].val); + { + ShowError(AssemblerError::NumberOutOfRange, + "Value must be from {:#x} to {:#x}, was {:#x}\n", -(value + 1), value, + par[i].val); + } else - fprintf(stderr, "Value must be minor of 0x%x, is %i\n", value + 1, par[i].val); - ShowError(AssemblerError::NumberOutOfRange); + { + ShowError(AssemblerError::NumberOutOfRange, + "Value must be less than {:#x}, was {:#x}\n", value + 1, par[i].val); + } } } } continue; } } - m_current_param = 0; + m_location.opcode_param_number = std::nullopt; return true; } @@ -755,7 +772,7 @@ bool DSPAssembler::AssemblePass(const std::string& text, int pass) std::istringstream fsrc(text); - m_code_line = 0; + m_location.line_num = 0; m_cur_pass = pass; #define LINEBUF_SIZE 1024 @@ -767,8 +784,8 @@ bool DSPAssembler::AssemblePass(const std::string& text, int pass) if (fsrc.fail()) break; - m_cur_line = line; - m_code_line++; + m_location.line_text = line; + m_location.line_num++; param_t params[10] = {{0, P_NONE, nullptr}}; param_t params_ext[10] = {{0, P_NONE, nullptr}}; @@ -905,23 +922,22 @@ bool DSPAssembler::AssemblePass(const std::string& text, int pass) { if (params[0].type == P_STR) { - std::string include_file_path; - const u32 this_code_line = m_code_line; + const auto old_location = m_location; if (m_include_dir.empty()) { - include_file_path = params[0].str; + m_location.included_file_path = params[0].str; } else { - include_file_path = m_include_dir + '/' + params[0].str; + m_location.included_file_path = m_include_dir + '/' + params[0].str; } std::string included_text; - File::ReadFileToString(include_file_path, included_text); + File::ReadFileToString(m_location.included_file_path.value(), included_text); AssemblePass(included_text, pass); - m_code_line = this_code_line; + m_location = old_location; } else { @@ -959,9 +975,8 @@ bool DSPAssembler::AssemblePass(const std::string& text, int pass) { if (m_cur_addr > params[0].val) { - const std::string msg = fmt::format("WARNPC at 0x{:04x}, expected 0x{:04x} or less", - m_cur_addr, params[0].val); - ShowError(AssemblerError::PCOutOfRange, msg.c_str()); + ShowError(AssemblerError::PCOutOfRange, "WARNPC at 0x{:04x}, expected 0x{:04x} or less", + m_cur_addr, params[0].val); } } else @@ -987,6 +1002,7 @@ bool DSPAssembler::AssemblePass(const std::string& text, int pass) continue; } + m_location.opcode_type = OpcodeType::Primary; const DSPOPCTemplate* opc = FindOpcode(opcode, params_count, OpcodeType::Primary); if (!opc) opc = &cw; @@ -997,6 +1013,7 @@ bool DSPAssembler::AssemblePass(const std::string& text, int pass) const DSPOPCTemplate* opc_ext = nullptr; // Check for opcode extensions. + m_location.opcode_type = OpcodeType::Extension; if (opc->extended) { if (opcode_ext) @@ -1016,6 +1033,7 @@ bool DSPAssembler::AssemblePass(const std::string& text, int pass) if (params_count_ext) ShowError(AssemblerError::ExtensionParamsOnNonExtendableOpcode); } + m_location.opcode_type = std::nullopt; if (pass == 2) { diff --git a/Source/Core/Core/DSP/DSPAssembler.h b/Source/Core/Core/DSP/DSPAssembler.h index 4c1cdbcd94..29ccaeb557 100644 --- a/Source/Core/Core/DSP/DSPAssembler.h +++ b/Source/Core/Core/DSP/DSPAssembler.h @@ -6,9 +6,13 @@ #include #include +#include #include +#include #include +#include + #include "Common/CommonTypes.h" #include "Core/DSP/DSPDisassembler.h" @@ -85,6 +89,17 @@ private: Extension }; + struct LocationContext + { + u32 line_num = 0; + std::string line_text; + std::optional opcode_type; + std::optional opcode_param_number; + std::optional included_file_path; + }; + + friend struct ::fmt::formatter; + // Utility functions s32 ParseValue(const char* str); u32 ParseExpression(const char* ptr); @@ -94,7 +109,11 @@ private: void InitPass(int pass); bool AssemblePass(const std::string& text, int pass); - void ShowError(AssemblerError err_code, const char* extra_info = nullptr); + void ShowError(AssemblerError err_code); + template + void ShowError(AssemblerError err_code, fmt::format_string format, Args&&... args); + template + void ShowWarning(fmt::format_string format, Args&&... args); char* FindBrackets(char* src, char* dst); const DSPOPCTemplate* FindOpcode(std::string name, size_t par_count, OpcodeType type); @@ -104,7 +123,6 @@ private: std::vector m_output_buffer; std::string m_include_dir; - std::string m_cur_line; u32 m_cur_addr = 0; int m_total_size = 0; @@ -112,7 +130,6 @@ private: LabelMap m_labels; - u32 m_code_line = 0; bool m_failed = false; std::string m_last_error_str; AssemblerError m_last_error = AssemblerError::OK; @@ -122,7 +139,8 @@ private: segment_t m_cur_segment = SEGMENT_CODE; u32 m_segment_addr[SEGMENT_MAX] = {}; - int m_current_param = 0; const AssemblerSettings m_settings; + + LocationContext m_location; }; } // namespace DSP diff --git a/Source/Core/Core/DSP/DSPCodeUtil.cpp b/Source/Core/Core/DSP/DSPCodeUtil.cpp index 98e8c44d59..9a33edc9ba 100644 --- a/Source/Core/Core/DSP/DSPCodeUtil.cpp +++ b/Source/Core/Core/DSP/DSPCodeUtil.cpp @@ -35,13 +35,7 @@ bool Assemble(const std::string& text, std::vector& code, bool force) // TODO: fix the terrible api of the assembler. DSPAssembler assembler(settings); - if (!assembler.Assemble(text, code)) - { - std::cerr << assembler.GetErrorString() << std::endl; - return false; - } - - return true; + return assembler.Assemble(text, code); } bool Disassemble(const std::vector& code, bool line_numbers, std::string& text)