From 57afd05bf4154a60aaa032ffa9d9a4cde15a8e51 Mon Sep 17 00:00:00 2001 From: Martin Kedmenec Date: Thu, 24 Apr 2025 14:30:07 +0200 Subject: [PATCH 1/2] Fix #167. Fix bytecode corruption in nested conditionals --- src/parser.c | 64 ++++++++++++++++++++++++++++++++++++------ tests/lexer_test.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++ tests/lexer_test.h | 1 + tests/main.c | 1 + 4 files changed, 128 insertions(+), 8 deletions(-) diff --git a/src/parser.c b/src/parser.c index 3fabb40..215a721 100644 --- a/src/parser.c +++ b/src/parser.c @@ -16,21 +16,56 @@ #include "vm.h" #ifdef __CC65__ -enum { kIdentifierNameLength = 16, kSymbolTableSize = 16 }; +enum { + kIdentifierNameLength = 16, + kSymbolTableSize = 16, + kJumpPatchStackSize = 4 +}; #else static constexpr int kIdentifierNameLength = 16; static constexpr int kSymbolTableSize = 16; +static constexpr int kJumpPatchStackSize = 4; #endif +/// Pushes a value onto the jump patch stack. +/// Defined as a macro since cc65 doesn't support passing structs to functions +/// by value for regular functions. +#define PushJumpPatch(jump_patch) \ + do { \ + if (kJumpPatchStackSize <= jump_patch_stack_index) { \ + puts("Error: Too many nested conditionals."); \ + } else { \ + jump_patch_stack[jump_patch_stack_index++] = (jump_patch); \ + } \ + } while (0) + +/// Pops a value from the jump patch stack. +/// Defined as a macro since cc65 doesn't support passing structs to functions +/// by value for regular functions. +#define PopJumpPatch() \ + (0 == jump_patch_stack_index \ + ? (puts("Error: Jump patch stack underflow."), kEmptyPendingJumpPatch) \ + : jump_patch_stack[--jump_patch_stack_index]) + typedef struct SymbolTableEntry { char name[kIdentifierNameLength]; VariableType type; size_t index; } SymbolTableEntry; +typedef struct PendingJumpPatch { + size_t condition_patch_slot; + size_t exit_patch_slot; +} PendingJumpPatch; + +static const PendingJumpPatch kEmptyPendingJumpPatch = {}; + // NOLINTBEGIN(cppcoreguidelines-avoid-non-const-global-variables) static SymbolTableEntry symbol_table[kSymbolTableSize]; +static PendingJumpPatch jump_patch_stack[kJumpPatchStackSize]; +static size_t jump_patch_stack_index = 0; + static char local_names[kSymbolTableSize][kIdentifierNameLength]; static size_t local_count = 0; @@ -377,8 +412,9 @@ static void ParsePrintStatement() { // NOLINTNEXTLINE(misc-no-recursion) static void ParseIfStatement() { - size_t if_jump_address = 0; - size_t else_jump_address = 0; + size_t condition_patch_slot = 0; + size_t exit_patch_slot = 0; + PendingJumpPatch pending_jump_patch = {}; if (!ExpectToken(1, kTokenLeftParenthesis)) { return; @@ -392,38 +428,50 @@ static void ParseIfStatement() { EmitByte(kOpJumpIfFalse); - if_jump_address = instruction_address; + condition_patch_slot = instruction_address; EmitByte(0); + pending_jump_patch.condition_patch_slot = condition_patch_slot; + + PushJumpPatch(pending_jump_patch); + while (kTokenEndif != token.type && kTokenElse != token.type && kTokenEof != token.type) { ParseStatement(); } + // No else if (kTokenElse != token.type) { - instructions[if_jump_address] = instruction_address; + pending_jump_patch = PopJumpPatch(); + + instructions[pending_jump_patch.condition_patch_slot] = instruction_address; ExpectToken(1, kTokenEndif); return; } + // If-else EmitByte(kOpJump); - else_jump_address = instruction_address; + exit_patch_slot = instruction_address; EmitByte(0); - instructions[if_jump_address] = instruction_address; + pending_jump_patch = PopJumpPatch(); + + instructions[pending_jump_patch.condition_patch_slot] = instruction_address; + pending_jump_patch.exit_patch_slot = exit_patch_slot; + // Parse else body. ConsumeNextToken(); while (kTokenEndif != token.type && kTokenEof != token.type) { ParseStatement(); } - instructions[else_jump_address] = instruction_address; + instructions[pending_jump_patch.exit_patch_slot] = instruction_address; ExpectToken(1, kTokenEndif); } diff --git a/tests/lexer_test.c b/tests/lexer_test.c index 7ab097d..5cc7e23 100644 --- a/tests/lexer_test.c +++ b/tests/lexer_test.c @@ -189,6 +189,76 @@ void TestIf() { TEST_ASSERT_EQUAL_INT(kTokenEndif, token.type); } +void TestNestedIf() { + FillProgramBuffer( + "if(true)\nif(false)\nprint(\"foo\")\nelse\nprint(\"bar\")" + "\nendif\nendif"); + + ConsumeNextToken(); // if + TEST_ASSERT_EQUAL_INT(kTokenIf, token.type); + + ConsumeNextToken(); // ( + TEST_ASSERT_EQUAL_INT(kTokenLeftParenthesis, token.type); + + ConsumeNextToken(); // true + TEST_ASSERT_EQUAL_INT(kTokenBoolean, token.type); + TEST_ASSERT_EQUAL_INT(1, token.value.number); + + ConsumeNextToken(); // ) + TEST_ASSERT_EQUAL_INT(kTokenRightParenthesis, token.type); + + ConsumeNextToken(); // if + TEST_ASSERT_EQUAL_INT(kTokenIf, token.type); + + ConsumeNextToken(); // ( + TEST_ASSERT_EQUAL_INT(kTokenLeftParenthesis, token.type); + + ConsumeNextToken(); // false + TEST_ASSERT_EQUAL_INT(kTokenBoolean, token.type); + TEST_ASSERT_EQUAL_INT(0, token.value.number); + + ConsumeNextToken(); // ) + TEST_ASSERT_EQUAL_INT(kTokenRightParenthesis, token.type); + + ConsumeNextToken(); // print + TEST_ASSERT_EQUAL_INT(kTokenPrint, token.type); + + ConsumeNextToken(); // ( + TEST_ASSERT_EQUAL_INT(kTokenLeftParenthesis, token.type); + + ConsumeNextToken(); // foo + TEST_ASSERT_EQUAL_INT(kTokenString, token.type); + TEST_ASSERT_EQUAL_STRING("foo", token.value.text); + + ConsumeNextToken(); // ) + TEST_ASSERT_EQUAL_INT(kTokenRightParenthesis, token.type); + + ConsumeNextToken(); // else + TEST_ASSERT_EQUAL_INT(kTokenElse, token.type); + + ConsumeNextToken(); // print + TEST_ASSERT_EQUAL_INT(kTokenPrint, token.type); + + ConsumeNextToken(); // ( + TEST_ASSERT_EQUAL_INT(kTokenLeftParenthesis, token.type); + + ConsumeNextToken(); // bar + TEST_ASSERT_EQUAL_INT(kTokenString, token.type); + TEST_ASSERT_EQUAL_STRING("bar", token.value.text); + + ConsumeNextToken(); // ) + TEST_ASSERT_EQUAL_INT(kTokenRightParenthesis, token.type); + + ConsumeNextToken(); // endif + TEST_ASSERT_EQUAL_INT(kTokenEndif, token.type); + + ConsumeNextToken(); // endif + TEST_ASSERT_EQUAL_INT(kTokenEndif, token.type); + + ConsumeNextToken(); // EOF + TEST_ASSERT_EQUAL_INT(kTokenEof, token.type); +} + void TestFor() { FillProgramBuffer("for(true)print(\"Hello, world!\")endfor"); diff --git a/tests/lexer_test.h b/tests/lexer_test.h index b8d181c..b39fe4f 100644 --- a/tests/lexer_test.h +++ b/tests/lexer_test.h @@ -16,6 +16,7 @@ void TestGreaterThanOrEqualTo(); void TestLessThanOrEqualTo(); void TestBooleanLiteral(); void TestIf(); +void TestNestedIf(); void TestFor(); void TestNot(); void TestIntVariableDeclaration(); diff --git a/tests/main.c b/tests/main.c index f9cc577..cdedc67 100644 --- a/tests/main.c +++ b/tests/main.c @@ -30,6 +30,7 @@ int main() { RUN_TEST(TestLessThanOrEqualTo); RUN_TEST(TestBooleanLiteral); RUN_TEST(TestIf); + RUN_TEST(TestNestedIf); RUN_TEST(TestFor); RUN_TEST(TestNot); RUN_TEST(TestIntVariableDeclaration); From 8ca5a7a17f20209c476b32908cdd43ee06b36645 Mon Sep 17 00:00:00 2001 From: Martin Kedmenec Date: Thu, 24 Apr 2025 14:34:52 +0200 Subject: [PATCH 2/2] Fix #163. Improve VM stack push/pop logic --- src/vm.c | 30 +++++++++++++++++++++++------- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/src/vm.c b/src/vm.c index db61d29..22d9c60 100644 --- a/src/vm.c +++ b/src/vm.c @@ -27,14 +27,23 @@ static constexpr int kFunctionPoolSize = 16; #endif /// Pushes a value onto the stack. -/// Is defined as a macro since cc65 doesn't support passing structs to -/// functions by value for regular functions. -#define Push(value) (stack[stack_index++] = (value)) +/// Defined as a macro since cc65 doesn't support passing structs to functions +/// by value for regular functions. +#define Push(value) \ + do { \ + if (kStackSize <= stack_index) { \ + puts("Error: Stack overflow."); \ + } else { \ + stack[stack_index++] = (value); \ + } \ + } while (0) /// Pops a value from the stack. -/// Is defined as a macro since cc65 doesn't support passing structs to -/// functions by value for regular functions. -#define Pop() (stack[--stack_index]) +/// Defined as a macro since cc65 doesn't support passing structs to functions +/// by value for regular functions. +#define Pop() \ + (0 == stack_index ? (puts("Error: Stack underflow."), kEmptyStackValue) \ + : stack[--stack_index]) typedef struct Constants { const void* pointer[kConstantsSize]; @@ -49,14 +58,21 @@ typedef struct Function { } Function; typedef struct StackValue { - VariableType type; union as { int number; char* string; Function* function; } as; + VariableType type; +#ifdef __CC65__ + unsigned char padding; ///< Used to add 1 byte padding to the struct, so that + ///< the whole struct has a size of exactly 4 bytes. + ///< See https://cc65.github.io/doc/cc65.html#s4 +#endif } StackValue; +static const StackValue kEmptyStackValue = {}; + typedef struct CallFrame { size_t return_address[kCallFrameTableSize]; size_t stack_offset[kCallFrameTableSize];