Conversation
- Add CLAUDE.md with build commands and architecture overview - Rename basic_compiler_design_spec.md to design.md - Update README.md link to design.md Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Fix stack misalignment in _rt_instr: add sub/add rsp,8 around memcmp calls (6 pushes left rsp%16=8, now properly aligned) - Fix stack misalignment in _rt_strcat: replace push/pop rax with 16-byte aligned temp storage for saving malloc result across memcpy calls - Fix function return type handling: use type-aware load instead of always using movsd xmm0 (now correctly handles Integer, Long, Single, Double, and String return types) - Fix procedure stack frame sizing: replace hardcoded 64-byte allocation with dynamic sizing using STACK_RESERVE_PROC placeholder - Remove macOS Homebrew job from CI workflow Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR implements stack alignment fixes for the runtime library and adds dynamic stack size calculation with type-aware function return value handling in the code generator.
Changes:
- Fixed stack alignment in runtime assembly functions (
_rt_instrand_rt_strcat) to comply with System V AMD64 ABI 16-byte alignment requirements - Replaced fixed 64-byte stack allocation with dynamic sizing that's calculated after processing function body and patched into the prologue
- Added type-specific return value loading for INTEGER, LONG, SINGLE, DOUBLE, and STRING function return types
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/runtime/string.s | Added stack alignment adjustments in _rt_instr and _rt_strcat; replaced push/pop with aligned memory operations |
| src/codegen.rs | Implemented dynamic stack size calculation with placeholder patching; added type-aware function return value handling |
| design.md | Added comprehensive design specification document (new file) |
| README.md | Updated design specification link to point to design.md |
| CLAUDE.md | Added Claude Code guidance document (new file) |
| .github/workflows/TestingCI.yml | Removed macOS homebrew CI job |
Comments suppressed due to low confidence (1)
src/runtime/string.s:257
- Out-of-bounds read in
_rt_instrdue to uncheckedstartposition. The_rt_instrruntime function takes a 1-basedstartindex inr8(moved torbx), decrements it, and then adjusts the haystack pointer and remaining length withdec rbx; add r12, rbx; sub r13, rbxwithout validating thatstartis within[1, haystack_len]. The code generator forINSTR(src/codegen.rs) passes the user-suppliedstartargument directly (converted from integer/float) intor8without clamping, so a BASIC program can supplystart <= 0orstart > LEN(haystack), causingr12to point before or beyond the actual string buffer whiler13underflows to a very large value. The subsequent loop usesmemcmp(r12, needle, needle_len)and advancesr12one byte at a time whiler13remains large (cmp r13, r15; jb .Linstr_not_found), somemcmpwill read outside the haystack buffer into adjacent memory, invoking undefined behavior, with realistic consequences including reading other in-process data (information disclosure) or segfaulting. This is especially dangerous if the runtime is ever used to execute untrusted BASIC code in a long-lived process (e.g., as an embedded scripting engine), since a malicious script can deliberately choose out-of-rangestartvalues to scan memory beyond its logical string bounds via repeated equality tests, potentially exposing sensitive data or destabilizing the process.
Severity: HIGH. Confidence: 9
add r12, rbx # advance haystack ptr
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Single-pass solution that tracks DIM declarations and emits Expr::ArrayAccess for known arrays vs Expr::FnCall for functions. Removes dead_code annotation from ArrayAccess variant as it's now properly used. Arrays are distinguished from functions at parse time by checking if the identifier was previously declared with DIM. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
The documentation said [rbp - offset] but the code consistently uses [rbp + offset] where offset is negative (e.g., -8, -16). Fixed the comment to match the actual implementation. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.