Skip to content

greenhorn: fix C VM bugs and expand test suite#1

Open
SuperInstance wants to merge 1 commit intomainfrom
greenhorn/fix-bugs-expand-c-tests
Open

greenhorn: fix C VM bugs and expand test suite#1
SuperInstance wants to merge 1 commit intomainfrom
greenhorn/fix-bugs-expand-c-tests

Conversation

@SuperInstance
Copy link
Copy Markdown
Owner

@SuperInstance SuperInstance commented Apr 12, 2026

Bug Fixes

  • MEMCMP bug (src/vm.c line 126): memcpy(&b,data,4) was copying address a into b instead of the actual b value from data+4
  • MEMSET dead code (src/vm.c line 125): Removed redundant memcpy(&n,data,4) that was immediately overwritten by the correct memcpy(&n,data+4,4)
  • Build failure (src/main.c): Added missing #include <stdlib.h> for malloc/free
  • Missing opcode (src/vm.c): Added FLUX_ADDI (0x19) handler — was falling through to INVALID_OPCODE error
  • Register bounds checking (src/vm.c): New gr8() function clamps register indices to 0-15 (FLUX_GP_COUNT), preventing out-of-bounds memory access when bytecode specifies arbitrary register numbers. Applied to all arithmetic, bitwise, compare, float, stack, and jump instructions.

Test Expansion

Added 22 new tests to tests/test_vm.c (46 total, up from 24):

  • addi, addi_negative — ADDI immediate instruction
  • neg_div, neg_mod — signed division/modulo with negative operands
  • mod_zero — modulo-by-zero error detection
  • inc_16bit_boundary, dec_16bit_boundary — int16 immediate boundary values
  • shr_negative — arithmetic right shift of negative numbers
  • or_xor_not — OR, XOR, NOT bitwise operations
  • mov_reg — MOV between different registers
  • flags_zero, flags_after_sub_zero — zero/sign flag state verification
  • jmp_forward — forward unconditional jump
  • jlt_taken, jlt_not_taken, jgt_taken — conditional branch tests
  • multi_push_pop — LIFO stack ordering with 3 values
  • reg_bounds_clamp — out-of-bounds register index safety
  • cmp_lt_flags, cmp_gt_flags — comparison flag verification
  • loop_factorial — 5! = 120 via backward-jump loop
  • fibonacci_10 — fibonacci(10) = 55 via backward-jump loop
  • movi_large — MOVI with -32768 (int16 min)
  • nested_call_ret — double CALL/RET to same subroutine

Results

All 63 tests passing (46 VM + 5 memory + 12 assembler)


Staging: Open in Devin

Bug fixes:
- Fix MEMCMP: memcpy(&b,data,4) was reading wrong offset (should be data+4)
- Fix MEMSET: remove redundant dead memcpy(&n,data,4) before correct read
- Fix main.c: add missing #include <stdlib.h> (build failure)
- Add FLUX_ADDI opcode handling (was missing, caused INVALID_OPCODE)
- Add register bounds checking: new gr8() function clamps reg indices to 0-15
  preventing out-of-bounds memory access on arbitrary bytecode register values

Test expansion (22 new tests, 46 total in test_vm):
- addi, addi_negative: ADDI immediate instruction
- neg_div, neg_mod: signed division and modulo with negatives
- mod_zero: modulo by zero error detection
- inc_16bit_boundary, dec_16bit_boundary: 16-bit immediate boundaries
- shr_negative: arithmetic right shift with negative value
- or_xor_not: OR, XOR, NOT bitwise operations
- mov_reg: MOV between registers
- flags_zero, flags_after_sub_zero: flag state verification
- jmp_forward: forward unconditional jump
- jlt_taken, jlt_not_taken: JLT conditional branch
- jgt_taken: JGT conditional branch
- multi_push_pop: multiple push/pop LIFO order
- reg_bounds_clamp: out-of-bounds register index safety
- cmp_lt_flags, cmp_gt_flags: comparison flag verification
- loop_factorial: 5! computation with backward jump loop
- fibonacci_10: fibonacci(10)=55 with backward jump loop
- movi_large: MOVI with int16 max negative value
- nested_call_ret: double CALL/RET to same subroutine

All 63 tests passing (46 VM + 5 memory + 12 asm).
Copy link
Copy Markdown

@beta-devin-ai-integration beta-devin-ai-integration bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 potential issue.

View 5 additional findings in Devin Review.

Staging: Open in Devin

Comment thread src/vm.c
case FLUX_FGE: rd=f8(v); rs1=f8(v); GPR[rd]=(v->regs.fp[rd]>=v->regs.fp[rs1]); break;
case FLUX_LOAD8: { rd=f8(v); rs1=f8(v); h=flux_mem_get(&v->mem,"heap"); GPR[rd]=h?(int32_t)flux_mem_read_u8(h,GPR[rs1]):0; } break;
case FLUX_STORE8: { rd=f8(v); rs1=f8(v); h=flux_mem_get(&v->mem,"heap"); if(h) flux_mem_write_u8(h,GPR[rd],(uint8_t)GPR[rs1]); } break;
case FLUX_CHECK_TYPE: rd=gr8(v); rs1=f8(v); GPR[0]=(rd<v->box_count&&v->box_table[rd].type_tag==rs1); break;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 FLUX_CHECK_TYPE uses gr8() for box ID operand, clamping valid box indices 16-63 to 0

The FLUX_CHECK_TYPE handler reads its first operand (a box table index) using gr8(v), which clamps values >= FLUX_GP_COUNT (16) to 0. However, rd here is NOT a register index — it's a box ID used to index v->box_table[rd] (which supports up to 64 entries per FLUX_BOX_MAX at include/flux/vm.h:18). Any bytecode that calls CHECK_TYPE on box IDs 16–63 will silently check box 0 instead, producing incorrect type-check results. The old code correctly used f8(v) for this operand.

Suggested change
case FLUX_CHECK_TYPE: rd=gr8(v); rs1=f8(v); GPR[0]=(rd<v->box_count&&v->box_table[rd].type_tag==rs1); break;
case FLUX_CHECK_TYPE: rd=f8(v); rs1=f8(v); GPR[0]=(rd<v->box_count&&v->box_table[rd].type_tag==rs1); break;
Staging: Open in Devin

Was this helpful? React with 👍 or 👎 to provide feedback.

Debug

Playground

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant