Implement closure values for Pascal closures #1618
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (captured_env) { | ||
| if (captured_env->slot_count != proc_symbol->upvalue_count) { | ||
| releaseClosureEnv(captured_env); | ||
| runtimeError(vm, "VM Error: Closure environment mismatch for '%s'.", | ||
| proc_symbol->name ? proc_symbol->name : "<anonymous>"); | ||
| return INTERPRET_RUNTIME_ERROR; | ||
| } |
There was a problem hiding this comment.
When CALL_INDIRECT sees a closure whose captured slots do not match the callee’s upvalue_count, it reports a runtime error but returns without undoing the earlier vm->frameCount++. The VM now believes there is one more active frame than actually exists, so later unwinding or cleanup will release uninitialized pointers and can corrupt memory. The frame count should be decremented before returning on this error path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Summary
- Ensured CALL_USER_PROC and CALL unwind the frame counter when their parent frame lookup fails, preventing phantom frames after runtime errors. src/vm/vm.cL6487-L6510 src/vm/vm.cL6581-L6603
- Balanced the frame stack for CALL_INDIRECT and PROC_CALL_INDIRECT by decrementing the frame counter on closure environment mismatches or missing parent frames while still releasing retained environments. src/vm/vm.cL6703-L6739 src/vm/vm.cL6967-L6999
Testing
- ✅
cmake --build build --target clike
| if (captured_env) { | ||
| if (captured_env->slot_count != proc_symbol->upvalue_count) { | ||
| releaseClosureEnv(captured_env); | ||
| runtimeError(vm, "VM Error: Closure environment mismatch for '%s'.", | ||
| proc_symbol->name ? proc_symbol->name : "<anonymous>"); | ||
| return INTERPRET_RUNTIME_ERROR; | ||
| } |
There was a problem hiding this comment.
The same imbalance occurs in the PROC_CALL_INDIRECT handler: after allocating a CallFrame, a closure environment mismatch triggers an error and immediate return, but the incremented vm->frameCount is never rolled back. Subsequent interpreter logic will process a phantom frame and may read or free garbage. Decrement vm->frameCount before returning here as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Summary
- Rolled back
vm->frameCountwhenCALL_USER_PROCcannot find an enclosing frame to avoid leaving phantom frames behind. src/vm/vm.cL6490-L6510 - Ensured direct
CALLframes initialize ownership/closure fields and undo the frame increment if the parent frame lookup fails. src/vm/vm.cL6573-L6605 - Decremented
vm->frameCounton closure environment mismatches and missing parents inCALL_INDIRECTandPROC_CALL_INDIRECT, keeping the frame stack balanced. src/vm/vm.cL6705-L6741 src/vm/vm.cL6965-L7000
Testing
- ✅
cmake -S . -B build - ✅
cmake --build build --target clike
|
@codex please fix... [ 25%] Building C object CMakeFiles/clike.dir/src/core/version.c.o |
|
Summary
Testing
|
|
@codex please address the failed tests. Ran 100 Pascal test(s); 3 failure(s); 3 skipped 2/4 Test #2: clike_tests ...................... Passed 12.77 sec Ran 55 Rea test(s); 1 failure(s); 6 skipped 4/4 Test #4: json2bc_tests .................... Passed 0.09 sec 50% tests passed, 2 tests failed out of 4 Total Test time (real) = 72.06 sec The following tests FAILED: |
|
Summary
Testing
|
|
@codex please fix... |
|
I’m sorry, but I wasn’t able to complete these changes. |
|
@codex please fix... |
|
Summary
Testing
|
Summary
Testing
https://chatgpt.com/codex/tasks/task_b_6901c7d44770832996d91ea1e0099c40