From 380af2d15f35c270cfbdf51ef80df723f5e8d838 Mon Sep 17 00:00:00 2001 From: Guilherme Lampert Date: Tue, 20 Oct 2026 12:32:15 +1100 Subject: [PATCH 1/4] Tag errors with "error:", as warnings are tagged "warning:" Errors were printed as `file(line) : message` and warnings as `file(line) : warning: message`, so searching a build log for "error:" found nothing, and a failed VU build was easy to miss. Errors now read `file(line) : error: message`, or `openvcl: error: message` when they have no location. Co-Authored-By: Claude Opus 5.5 --- src/Error.cpp | 3 +- test/CMakeLists.txt | 1 + test/integration/test_diagnostics.cpp | 69 +++++++++++++++++++++++++++ 3 files changed, 71 insertions(+), 2 deletions(-) create mode 100644 test/integration/test_diagnostics.cpp diff --git a/src/Error.cpp b/src/Error.cpp index 3f5cfc3..1b30ead 100644 --- a/src/Error.cpp +++ b/src/Error.cpp @@ -109,8 +109,7 @@ std::string Error::toString() const s << "openvcl: "; } - if( m_warning ) - s << "warning: "; + s << (m_warning ? "warning: " : "error: "); s << m_string; diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt index e1a162b..a1a5459 100644 --- a/test/CMakeLists.txt +++ b/test/CMakeLists.txt @@ -69,6 +69,7 @@ set(OPENVCL_TEST_SOURCES ${CMAKE_CURRENT_SOURCE_DIR}/unit/test_vsm_cost_analyzer.cpp ${CMAKE_CURRENT_SOURCE_DIR}/integration/openvcl_runner.cpp ${CMAKE_CURRENT_SOURCE_DIR}/integration/test_cli.cpp + ${CMAKE_CURRENT_SOURCE_DIR}/integration/test_diagnostics.cpp ${CMAKE_CURRENT_SOURCE_DIR}/integration/test_init_vf_range.cpp ${CMAKE_CURRENT_SOURCE_DIR}/integration/test_clip_validation.cpp ${CMAKE_CURRENT_SOURCE_DIR}/integration/test_clip_implicit_w.cpp diff --git a/test/integration/test_diagnostics.cpp b/test/integration/test_diagnostics.cpp new file mode 100644 index 0000000..2d162b4 --- /dev/null +++ b/test/integration/test_diagnostics.cpp @@ -0,0 +1,69 @@ +// Regression tests: openvcl's diagnostics are easy to find and act on. +// +// Errors were printed as "file(line) : message", without the "error:" tag +// warnings already had, so searching a build log for "error:" found +// nothing. + +#include "test_harness.h" +#include "openvcl_runner.h" + +#include +#include + +namespace +{ + bool contains(const std::string& text, const std::string& needle) + { + return text.find(needle) != std::string::npos; + } + + std::string program(const std::string& directives, const std::string& body) + { + return directives + + "\t.syntax new\n" + "\t.name vsmDiagnostics\n" + "\t--enter\n" + "\t--endenter\n" + + body + + "\t--exit\n" + "\t--endexit\n"; + } + + const char* kAllRegisters = "\t.init_vf_all\n\t.init_vi_all\n"; + + // Line 8 passes clipw a field other than W. + std::string invalidProgram() + { + return program(kAllRegisters, + "\tlq fA, 1(vi00)\n" + "\tclipw.xyz fA, fA[x]\n" + "\tfcand vi01, 0x3f\n" + "\tisw.x vi01, 2(vi00)\n"); + } + + std::string validProgram() + { + return program(kAllRegisters, + "\tlq fA, 1(vi00)\n" + "\tsq fA, 2(vi00)\n"); + } +} + +TEST_CASE("openvcl: an error at a source line is tagged error:") +{ + ::test::RunResult r = ::test::run_openvcl(std::vector(), invalidProgram()); + + CHECK(r.exit_code != 0); + CHECK(contains(r.stderr_data, "(8) : error: Invalid argument (clipw.xyz fA, >>> fA[x] <<<")); +} + +TEST_CASE("openvcl: an error without a location is tagged error:") +{ + std::vector args; + args.push_back("-o"); + args.push_back("/nonexistent-openvcl-dir/out.vsm"); + ::test::RunResult r = ::test::run_openvcl(args, validProgram()); + + CHECK(r.exit_code != 0); + CHECK(contains(r.stderr_data, "openvcl: error: Could not open output file")); +} From 358c6f39b7eb8f6a1d4de18e0b6269438d88b262 Mon Sep 17 00:00:00 2001 From: Guilherme Lampert Date: Tue, 20 Oct 2026 12:33:25 +1100 Subject: [PATCH 2/4] Name the variable that ran out of registers, and where it is set Running out of registers printed only `openvcl: Register allocation ran out of registers`, with no variable, register type or location, so finding the pressure point meant bisecting the source. The error now names the register type and how many registers .init_vf/.init_vi leave to the allocator, and points at the first line that uses the variable it could not place: x.vcl(9) : error: Register allocation ran out of VI registers (2 available) for iC (ilw.x >>> iC <<<, 2(vi00)) Co-Authored-By: Claude Opus 5.5 --- src/RegisterAllocator.cpp | 51 +++++++++++++++++++++++++-- src/RegisterAllocator.h | 3 +- test/integration/test_diagnostics.cpp | 34 +++++++++++++++++- 3 files changed, 83 insertions(+), 5 deletions(-) diff --git a/src/RegisterAllocator.cpp b/src/RegisterAllocator.cpp index cf6d79b..4d0507e 100644 --- a/src/RegisterAllocator.cpp +++ b/src/RegisterAllocator.cpp @@ -269,9 +269,10 @@ bool RegisterAllocator::process( std::list& tokens ) if( m_aliases.size() > 0 ) { - if( !processAliases() ) + Alias* unallocated = NULL; + if( !processAliases( unallocated ) ) { - Error::Display( Error( "Register allocation ran out of registers" ) ); + reportUnallocatedAlias( tokens, unallocated ); return false; } } @@ -559,7 +560,7 @@ bool RegisterAllocator::collectLabels( std::list::iterator start, std::li /////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// -bool RegisterAllocator::processAliases() +bool RegisterAllocator::processAliases( Alias*& unallocated ) { // allocate registers that do not overlap usage @@ -798,6 +799,7 @@ bool RegisterAllocator::processAliases() std::cerr << "Failed to allocate " << (dest->type() == Alias::FLOAT ? "FLOAT" : "INTEGER") << " register for alias" << std::endl; } + unallocated = dest; return false; } //assert( dest->allocatedRegister() ); @@ -808,6 +810,49 @@ bool RegisterAllocator::processAliases() /////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// +void RegisterAllocator::reportUnallocatedAlias( const std::list& tokens, const Alias* alias ) const +{ + if( !alias ) + { + Error::Display( Error( "Register allocation ran out of registers" ) ); + return; + } + + // the registers .init_vf/.init_vi left to the allocator + + const bool isFloat = (alias->type() == Alias::FLOAT); + unsigned int available = 0; + for( unsigned int i = 0; i < (isFloat ? 32u : 16u); ++i ) + { + if( (isFloat ? m_floats[i] : m_integers[i]).available() ) + ++available; + } + + std::stringstream message; + message << "Register allocation ran out of " << (isFloat ? "VF" : "VI") + << " registers (" << available << " available)"; + + // name the variable, where it first appears + + for( std::list::const_iterator i = tokens.begin(); i != tokens.end(); ++i ) + { + for( std::list::const_iterator a = (*i).arguments().begin(); a != (*i).arguments().end(); ++a ) + { + if( ((*a).content() == Token::Argument::ALIAS) && (*a).dependency() + && ((*a).dependency()->alias() == alias) ) + { + message << " for " << (*a).alias(); + Error::Display( Error( message.str(), *i, *a ) ); + return; + } + } + } + + Error::Display( Error( message.str() ) ); +} + +/////////////////////////////////////////////////////////////////////////////////////////////////////////////////////// + namespace { void extendContinuationBlockLiveRanges( std::list::iterator blockStart, std::list::iterator blockEnd ) diff --git a/src/RegisterAllocator.h b/src/RegisterAllocator.h index 738fca5..0da00da 100644 --- a/src/RegisterAllocator.h +++ b/src/RegisterAllocator.h @@ -88,7 +88,8 @@ class RegisterAllocator bool processCommonDirective( Token& token ); - bool processAliases(); + bool processAliases( Alias*& unallocated ); + void reportUnallocatedAlias( const std::list& tokens, const Alias* alias ) const; // Walks all emittable tokens and records, for each physical register, // the line numbers where it appears as a *literal* (non-alias) operand. diff --git a/test/integration/test_diagnostics.cpp b/test/integration/test_diagnostics.cpp index 2d162b4..5f90eb8 100644 --- a/test/integration/test_diagnostics.cpp +++ b/test/integration/test_diagnostics.cpp @@ -2,7 +2,8 @@ // // Errors were printed as "file(line) : message", without the "error:" tag // warnings already had, so searching a build log for "error:" found -// nothing. +// nothing. Running out of registers said only "Register allocation ran out +// of registers", with no variable, register type or location. #include "test_harness.h" #include "openvcl_runner.h" @@ -67,3 +68,34 @@ TEST_CASE("openvcl: an error without a location is tagged error:") CHECK(r.exit_code != 0); CHECK(contains(r.stderr_data, "openvcl: error: Could not open output file")); } + +TEST_CASE("openvcl: running out of VI registers names the variable and where it is set") +{ + // only two VI registers for three values live at once; line 9 sets the third + ::test::RunResult r = ::test::run_openvcl(std::vector(), program( + "\t.init_vf_all\n\t.init_vi vi01, vi02\n", + "\tilw.x iA, 0(vi00)\n" + "\tilw.x iB, 1(vi00)\n" + "\tilw.x iC, 2(vi00)\n" + "\tiadd iD, iA, iB\n" + "\tiadd iE, iD, iC\n" + "\tisw.x iE, 3(vi00)\n")); + + CHECK(r.exit_code != 0); + CHECK(contains(r.stderr_data, "(9) : error: Register allocation ran out of VI registers (2 available) for iC (ilw.x >>> iC <<<, 2(vi00))")); +} + +TEST_CASE("openvcl: running out of VF registers names the variable and where it is set") +{ + ::test::RunResult r = ::test::run_openvcl(std::vector(), program( + "\t.init_vf vf01-vf02\n\t.init_vi_all\n", + "\tlq fA, 0(vi00)\n" + "\tlq fB, 1(vi00)\n" + "\tlq fC, 2(vi00)\n" + "\tadd fD, fA, fB\n" + "\tadd fE, fD, fC\n" + "\tsq fE, 3(vi00)\n")); + + CHECK(r.exit_code != 0); + CHECK(contains(r.stderr_data, "(9) : error: Register allocation ran out of VF registers (2 available) for fC (lq >>> fC <<<")); +} From 0fccde4f436e22a349cd38519fdbf40b28e033bc Mon Sep 17 00:00:00 2001 From: Guilherme Lampert Date: Tue, 20 Oct 2026 12:34:18 +1100 Subject: [PATCH 3/4] Write no output after an error When tokenizing reported an error (an invalid operand, say), openvcl exited with failure but still wrote the .vsm, minus the bad instruction. The fresh file then looked up to date to make, so the next build used it, and a VU change could seem not to take effect. Now nothing is written once an error has been reported, to a file or to stdout, and an existing output file is left as it was. Co-Authored-By: Claude Opus 5.5 --- src/Parser.cpp | 6 +++++ test/integration/test_diagnostics.cpp | 35 ++++++++++++++++++++++++++- 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/src/Parser.cpp b/src/Parser.cpp index b8b1946..9e3e21c 100644 --- a/src/Parser.cpp +++ b/src/Parser.cpp @@ -2101,6 +2101,12 @@ bool Parser::generateCode() bool Parser::writeOutput() { + // write nothing once an error has been reported: a .vsm left behind + // would look up to date to make, and the next build would use it + + if( Error::HasErrors() ) + return false; + if( m_cmdLine.output() == "-" ) return writeOutputStream( std::cout ); diff --git a/test/integration/test_diagnostics.cpp b/test/integration/test_diagnostics.cpp index 5f90eb8..d6a3237 100644 --- a/test/integration/test_diagnostics.cpp +++ b/test/integration/test_diagnostics.cpp @@ -3,12 +3,18 @@ // Errors were printed as "file(line) : message", without the "error:" tag // warnings already had, so searching a build log for "error:" found // nothing. Running out of registers said only "Register allocation ran out -// of registers", with no variable, register type or location. +// of registers", with no variable, register type or location. And a failed +// build still wrote its output file, which then looked up to date to make. #include "test_harness.h" #include "openvcl_runner.h" +#include +#include +#include +#include #include +#include #include namespace @@ -99,3 +105,30 @@ TEST_CASE("openvcl: running out of VF registers names the variable and where it CHECK(r.exit_code != 0); CHECK(contains(r.stderr_data, "(9) : error: Register allocation ran out of VF registers (2 available) for fC (lq >>> fC <<<")); } + +TEST_CASE("openvcl: a failed build writes no output") +{ + char path[] = "/tmp/openvcl_diag_XXXXXX.vsm"; + int fd = mkstemps(path, 4); + REQUIRE(fd >= 0); + const char stale[] = "stale output from an earlier build\n"; + REQUIRE(write(fd, stale, sizeof(stale) - 1) == static_cast(sizeof(stale) - 1)); + close(fd); + + std::vector args; + args.push_back("-o"); + args.push_back(path); + ::test::RunResult r = ::test::run_openvcl(args, invalidProgram()); + + std::ifstream f(path); + std::stringstream contents; + contents << f.rdbuf(); + std::remove(path); + + CHECK(r.exit_code != 0); + CHECK(contents.str() == stale); + + ::test::RunResult toStdout = ::test::run_openvcl(std::vector(), invalidProgram()); + CHECK(toStdout.exit_code != 0); + CHECK(toStdout.stdout_data.empty()); +} From 5d0ab4b8c6a12de803c963553c3e1a233b0ab943 Mon Sep 17 00:00:00 2001 From: Guilherme Lampert Date: Tue, 20 Oct 2026 12:35:50 +1100 Subject: [PATCH 4/4] Fix two undefined behaviours found by UBSan - Tokenizer's constructor left m_newSyntax uninitialized. Parser always sets it before tokenizing, but the Tokenizer unit tests read it first (UBSan: "load of value 78, which is not a valid value for type 'bool'"). It now starts false, the default without -n. - The gasp temp-file name computed ((getpid() + rand()) << 8) in int, which overflows. It is now computed unsigned, which gives the same bits, so the same names, without the undefined behaviour. Found by building with -fsanitize=address,undefined and running the test suite and the PS2 examples; both reports are gone. Co-Authored-By: Claude Opus 5.5 --- src/Parser.cpp | 4 +++- src/Tokenizer.cpp | 1 + 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/src/Parser.cpp b/src/Parser.cpp index 9e3e21c..055d259 100644 --- a/src/Parser.cpp +++ b/src/Parser.cpp @@ -2177,7 +2177,9 @@ std::string Parser::tempFilename() { std::stringstream buffer; - buffer << "/tmp/vcl" << std::hex << std::setfill('0') << std::setw(8) << ((getpid()+rand()) << 8) + m_tempCounter++; + // unsigned, so the sum and the shift wrap instead of overflowing an int + buffer << "/tmp/vcl" << std::hex << std::setfill('0') << std::setw(8) + << ((static_cast(getpid()) + static_cast(rand())) << 8) + m_tempCounter++; std::string temp = buffer.str(); std::ofstream file(temp.c_str(),std::ifstream::out); diff --git a/src/Tokenizer.cpp b/src/Tokenizer.cpp index c906fd6..8dce64c 100644 --- a/src/Tokenizer.cpp +++ b/src/Tokenizer.cpp @@ -24,6 +24,7 @@ Tokenizer::Tokenizer() { setVsmMode( false ); setComment( false ); + setNewSyntax( false ); m_availableFloats = 0; m_availableIntegers = 0;