Conversation
📝 WalkthroughWalkthrough
ChangesReturn and binary expression pipeline
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Source
participant Lexer
participant Parser
participant LLVM
Source->>Lexer: Read return and + tokens
Lexer->>Parser: Emit token stream
Parser->>LLVM: Generate binary addition and return instructions
LLVM-->>Source: Produce generated function output
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
stage1.c (5)
852-896: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
parser()is bounded by buffer capacity, not actual token count.Line 881 passes
alloc_tokens_main(the fixed 32-slot buffer capacity) toparser(), whilenum_tokens_main(the actual count returned bylexer()at line 873) is exactly the value that should bound parsing. Any bound checks inside the parser (e.g.parse_expr'sidx+1 < alloc_tokens) end up validating against unused/uninitialized token slots rather than the real end of the token stream.🛡️ Proposed fix
- int parsed = parser(&tokens_main, alloc_tokens_main, parse_tree_main); + int parsed = parser(&tokens_main, num_tokens_main, parse_tree_main);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stage1.c` around lines 852 - 896, Update the parser invocation in main to pass num_tokens_main, the token count returned by lexer(), instead of alloc_tokens_main, the allocation capacity. Ensure parser and its bound checks use the actual token stream length so parsing never reads unused token slots.
766-782: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRemoving the fallback
LLVMBuildRetleaves blocks without an explicitreturnunterminated.Every LLVM basic block requires a terminator. Now that the unconditional fallback return is gone, any parsed block lacking an explicit
returnstatement produces a block with no terminator, andLLVMVerifyModule(..., LLVMAbortProcessAction, ...)(line 804) willabort()the whole compiler process rather than failing gracefully. This works today only becauseexample.zrhappens to always include areturn.Consider re-adding a fallback, guarded so it doesn't double-terminate a block that already ended in a
return:if (LLVMGetBasicBlockTerminator(entry) == NULL) { LLVMBuildRet(*builder, LLVMConstInt(LLVMInt32Type(), 0, 0)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stage1.c` around lines 766 - 782, Update test_gen_fn after gen_stmt to ensure the entry block is terminated: use LLVMGetBasicBlockTerminator(entry) and emit the existing integer-zero fallback return only when no terminator exists, preserving explicit return statements without double-terminating the block.
453-463: 🗄️ Data Integrity & Integration | 🔴 Critical | ⚡ Quick winMissing
PARSER_RETURNcase leaks the return expression's AST node.
free_parsergained aPARSER_BINARY_OPcase but noPARSER_RETURNcase. SincePARSER_RETURN'svalis a malloc'dGRAMMAR_T*(set at parse_stmt line 397 whenever a return expression exists), it's never freed — this leaks on the normal/successful path too (example.zr'sreturn test2;triggers it).🐛 Proposed fix
case PARSER_LIST: { free_parser_list((GRAMMAR_LIST*)(out->val)); } break; + case PARSER_RETURN: + { + free_parser((GRAMMAR_T*)(out->val)); + } + break; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stage1.c` around lines 453 - 463, Add a PARSER_RETURN branch to free_parser alongside the other parser-node cases. When the return node has an expression value, free its malloc'd GRAMMAR_T* AST through the existing recursive cleanup path, then release the return node itself; preserve safe handling for returns without an expression.
660-710: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUndefined identifier → uninitialized
LLVMValueRef/LLVMTypeReffed toLLVMBuildLoad2.The new
TOK_IDENTIFIERpath (671-680) callsget_valueand immediately usesptr/typinLLVMBuildLoad2without checking whether the identifier was actually found. Combined withget_valuenever signaling failure (see comment on lines 626-640), referencing an undeclared identifier passes garbage stack memory into the LLVM C API — undefined behavior, likely a crash or corrupt IR.🐛 Proposed fix
case TOK_IDENTIFIER: { LLVMTypeRef typ; LLVMValueRef ptr; - get_value(tok->val, vl, &ptr, &typ); + if (!get_value(tok->val, vl, &ptr, &typ)) { + char error[128]; + snprintf(error, 128, "undefined identifier: %s\n", (char*)tok->val); + quit(1, error); + } char name[64];(Requires
get_valueto actually return a success/failure indicator.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stage1.c` around lines 660 - 710, Update get_value and its caller in gen_expr’s TOK_IDENTIFIER case to return and check a success/failure indicator before using ptr or typ. For an undeclared identifier, stop expression generation through the existing error-handling path instead of calling LLVMBuildLoad2 with uninitialized values; retain the current load behavior for successfully resolved identifiers.
626-640: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSilent lookup failure leaves
*out/*typunset.
get_valuealways returns0and, on a miss (current == 0), leaves*outand*typuntouched instead of returning an error signal. Callers (seegen_expr'sTOK_IDENTIFIERcase) have no way to detect a failed lookup, so an undefined identifier flows through as uninitializedLLVMValueRef/LLVMTypeRef.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@stage1.c` around lines 626 - 640, Update get_value to return a distinct failure status when the name is not found, and have the TOK_IDENTIFIER path in gen_expr check that status before using *out or *typ. Preserve the successful lookup assignments while ensuring undefined identifiers cannot continue with unset LLVMValueRef or LLVMTypeRef values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@stage1.c`:
- Around line 388-407: Guard the lookahead in the TOK_RETURN branch before
dereferencing tokens[0][idx+1].tok. Use alloc_tokens to ensure idx+1 is within
the token buffer, treating an out-of-bounds lookahead as the no-expression
return case while preserving the existing semicolon validation and parse_expr
behavior for valid tokens.
- Around line 825-848: Initialize the block AST node’s val field immediately
when the PARSER_LIST node is allocated in parse_stmt(), before entering the
recursive `{ ... }` parsing loop. Ensure quit() can safely call free_parser() at
any point during nested parsing without dereferencing an indeterminate val
pointer.
---
Outside diff comments:
In `@stage1.c`:
- Around line 852-896: Update the parser invocation in main to pass
num_tokens_main, the token count returned by lexer(), instead of
alloc_tokens_main, the allocation capacity. Ensure parser and its bound checks
use the actual token stream length so parsing never reads unused token slots.
- Around line 766-782: Update test_gen_fn after gen_stmt to ensure the entry
block is terminated: use LLVMGetBasicBlockTerminator(entry) and emit the
existing integer-zero fallback return only when no terminator exists, preserving
explicit return statements without double-terminating the block.
- Around line 453-463: Add a PARSER_RETURN branch to free_parser alongside the
other parser-node cases. When the return node has an expression value, free its
malloc'd GRAMMAR_T* AST through the existing recursive cleanup path, then
release the return node itself; preserve safe handling for returns without an
expression.
- Around line 660-710: Update get_value and its caller in gen_expr’s
TOK_IDENTIFIER case to return and check a success/failure indicator before using
ptr or typ. For an undeclared identifier, stop expression generation through the
existing error-handling path instead of calling LLVMBuildLoad2 with
uninitialized values; retain the current load behavior for successfully resolved
identifiers.
- Around line 626-640: Update get_value to return a distinct failure status when
the name is not found, and have the TOK_IDENTIFIER path in gen_expr check that
status before using *out or *typ. Preserve the successful lookup assignments
while ensuring undefined identifiers cannot continue with unset LLVMValueRef or
LLVMTypeRef values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 94ec54d1-85d7-4bb5-8d5b-dec32482e20b
📒 Files selected for processing (2)
example.zrstage1.c
| } | ||
| if (current.tok == TOK_RETURN) { | ||
| out->typ = PARSER_RETURN; | ||
| out->val = 0; | ||
| GRAMMAR_T val; | ||
| val.typ = PARSER_TOKEN; | ||
| val.val = 0; | ||
| int len = 1; | ||
| if (tokens[0][idx+1].tok != TOK_SEMICOLON) { | ||
| out->val = malloc(sizeof(GRAMMAR_T)); | ||
| len += parse_expr(tokens, alloc_tokens, (GRAMMAR_T*)(out->val), idx+1); | ||
| if (tokens[0][idx+len].tok != TOK_SEMICOLON) { | ||
| quit(1, "missing semicolon at end of return"); | ||
| } | ||
| len++; | ||
| } | ||
| return len; | ||
|
|
||
|
|
||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unbounded array access before checking for +1.
tokens[0][idx+1].tok (line 396) is dereferenced before any bound check, unlike parse_expr's idx+1 < alloc_tokens && ... guard (line 294). If a return token lands at the last allocated slot, this reads past the token buffer.
🛡️ Proposed fix
- if (tokens[0][idx+1].tok != TOK_SEMICOLON) {
+ if (idx+1 < alloc_tokens && tokens[0][idx+1].tok != TOK_SEMICOLON) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } | |
| if (current.tok == TOK_RETURN) { | |
| out->typ = PARSER_RETURN; | |
| out->val = 0; | |
| GRAMMAR_T val; | |
| val.typ = PARSER_TOKEN; | |
| val.val = 0; | |
| int len = 1; | |
| if (tokens[0][idx+1].tok != TOK_SEMICOLON) { | |
| out->val = malloc(sizeof(GRAMMAR_T)); | |
| len += parse_expr(tokens, alloc_tokens, (GRAMMAR_T*)(out->val), idx+1); | |
| if (tokens[0][idx+len].tok != TOK_SEMICOLON) { | |
| quit(1, "missing semicolon at end of return"); | |
| } | |
| len++; | |
| } | |
| return len; | |
| } | |
| } | |
| if (current.tok == TOK_RETURN) { | |
| out->typ = PARSER_RETURN; | |
| out->val = 0; | |
| GRAMMAR_T val; | |
| val.typ = PARSER_TOKEN; | |
| val.val = 0; | |
| int len = 1; | |
| if (idx+1 < alloc_tokens && tokens[0][idx+1].tok != TOK_SEMICOLON) { | |
| out->val = malloc(sizeof(GRAMMAR_T)); | |
| len += parse_expr(tokens, alloc_tokens, (GRAMMAR_T*)(out->val), idx+1); | |
| if (tokens[0][idx+len].tok != TOK_SEMICOLON) { | |
| quit(1, "missing semicolon at end of return"); | |
| } | |
| len++; | |
| } | |
| return len; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@stage1.c` around lines 388 - 407, Guard the lookahead in the TOK_RETURN
branch before dereferencing tokens[0][idx+1].tok. Use alloc_tokens to ensure
idx+1 is within the token buffer, treating an out-of-bounds lookahead as the
no-expression return case while preserving the existing semicolon validation and
parse_expr behavior for valid tokens.
| GRAMMAR_T* parse_tree_main = 0; | ||
| TOKEN * tokens_main = 0; | ||
| int alloc_tokens_main; | ||
| int num_tokens_main = 0; | ||
|
|
||
| /* | ||
| * exit the program | ||
| */ | ||
| void quit(int code, char * reason) { | ||
| if (code != 0) { | ||
| printf("Program crashed! Reason: %s\n", reason); | ||
| } | ||
|
|
||
| if (parse_tree_main != 0) free_parser(parse_tree_main); | ||
| if (tokens_main != 0) { | ||
| for (int i = 0; i < num_tokens_main; i++) { | ||
| free(tokens_main[i].val); | ||
| } | ||
| free(tokens_main); | ||
| } | ||
|
|
||
| exit(code); | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate relevant symbols and read the surrounding code in manageable slices.
grep -nE '^(void quit|GRAMMAR_T\* parse_tree_main|TOKEN \* tokens_main|int num_tokens_main|void free_parser|void free_parser_list|.*parse_stmt|.*TOK_OPEN_CURLY|.*missing semicolon at end of return|.*free_parser\()' stage1.c || true
echo '--- parse_stmt / block handling ---'
sed -n '330,430p' stage1.c
echo '--- free_parser / list handling ---'
sed -n '1,220p' stage1.c
echo '--- quit and globals ---'
sed -n '810,870p' stage1.cRepository: zirco-lang/openzrc
Length of output: 9408
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find exact locations if the first slices are insufficient.
grep -nE 'free_parser_list|free_parser\(|parse_tree_main|quit\(' stage1.c || trueRepository: zirco-lang/openzrc
Length of output: 1143
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Print only the relevant function bodies and constructor patterns.
awk '
BEGIN{p=0}
/^void free_parser_list\(/ {p=1}
p{print}
p && /^}/ {exit}
' stage1.c
echo '---'
awk '
BEGIN{p=0}
/^void free_parser\(/ {p=1}
p{print}
p && /^}/ {exit}
' stage1.c
echo '---'
awk '
BEGIN{p=0}
/^void quit\(/ {p=1}
p{print}
p && /^}/ {exit}
' stage1.cRepository: zirco-lang/openzrc
Length of output: 925
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show every assignment/use of parse_tree_main and the relevant parser functions.
grep -nE 'parse_tree_main\s*=|free_parser\(|free_parser_list\(|quit\(' stage1.c | sed -n '1,200p'
echo '--- parse_stmt block area ---'
sed -n '350,420p' stage1.c
echo '--- free_parser area ---'
sed -n '120,220p' stage1.c
echo '--- quit area ---'
sed -n '820,860p' stage1.cRepository: zirco-lang/openzrc
Length of output: 5688
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Small targeted slices only.
nl -ba stage1.c | sed -n '350,405p'
echo '---'
nl -ba stage1.c | sed -n '540,620p'
echo '---'
nl -ba stage1.c | sed -n '820,850p'Repository: zirco-lang/openzrc
Length of output: 196
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect free_parser body and the remainder of free_parser_list.
sed -n '416,475p' stage1.cRepository: zirco-lang/openzrc
Length of output: 1435
Initialize block AST nodes before recursive parsing
parse_stmt() leaves a PARSER_LIST node’s val unset until after the { ... } loop finishes. If a nested statement calls quit(1, ...) before that assignment, quit() unwinds through free_parser(), and the PARSER_LIST cleanup path can dereference an indeterminate pointer. Set out->val as soon as the list head is allocated, or avoid freeing the AST on fatal parse errors.
🧰 Tools
🪛 Clang (14.0.6)
[warning] 838-838: statement should be inside braces
(readability-braces-around-statements)
🪛 Cppcheck (2.21.0)
[style] 833-833: The function 'quit' should have static linkage since it is not used outside of its translation unit.
(staticFunction)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@stage1.c` around lines 825 - 848, Initialize the block AST node’s val field
immediately when the PARSER_LIST node is allocated in parse_stmt(), before
entering the recursive `{ ... }` parsing loop. Ensure quit() can safely call
free_parser() at any point during nested parsing without dereferencing an
indeterminate val pointer.
1 participant
Summary by CodeRabbit
New Features
returnstatements, including returns with or without values.Bug Fixes