fix(codegen): guard generated rule dispatch against native stack overflow - #194
Merged
Merged
Conversation
…flow Generated recursive-descent rule methods mapped grammar-rule nesting directly onto native call depth: the CEL grammar walks ~9 rules per `[`, so ~850 nesting levels aborted the process on an 8 MiB stack while the interpreted path (recognize_state_fast, #147) and the tree walker were already segmented-stack safe. Emit a capacity probe at the shared parse_generated_rule_N_dispatch boundary, sampled once per 8 rule-context frames, growing onto a segmented stack via the same red-zone constants recognize_state_fast uses. Deeply nested input now parses (or reports a syntax error) instead of aborting. Fixes #193
📝 WalkthroughWalkthroughGenerated recursive rule dispatch now checks stack capacity and invokes a stack-growth wrapper when sampling indicates it is due. The runtime helper is publicly exported, and an end-to-end test validates generated parsing of 10,000 nested brackets. ChangesGenerated parser stack growth
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Test
participant antlr4-rust-gen
participant GeneratedParser
participant BaseParser
participant StackGrowth
Test->>antlr4-rust-gen: generate parser from Nest.g4
antlr4-rust-gen-->>Test: generated parser with stack guard
Test->>GeneratedParser: parse 10,000 nested brackets
GeneratedParser->>BaseParser: check generated_rule_stack_check_due()
BaseParser-->>GeneratedParser: stack check result
GeneratedParser->>StackGrowth: grow stack around rule call when due
StackGrowth-->>GeneratedParser: return parsed rule result
GeneratedParser-->>Test: parse tree with rule node
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 1
🤖 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 `@tests/antlr4_rust_gen_cli.rs`:
- Around line 1572-1578: Strengthen the assertion in the generated parser test
to count dispatch method declarations and require a nonzero count of
`antlr4_runtime::grow_generated_rule_stack` calls equal to that declaration
count. Replace the single `parser.contains` check while preserving the existing
failure context and generated-parser validation.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7968553f-4362-4c54-abda-ca71a5712986
📒 Files selected for processing (5)
src/bin/antlr4-rust-gen.rssrc/lib.rssrc/parser.rstests/antlr4_rust_gen_cli.rstests/fixtures/antlr4-rust-gen/deep-nesting/Nest.g4
📊 Source Code Metrics (this PR vs
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Copy/Paste DetectionFound 15 duplication(s) across 4 changed Rust file(s) (threshold: 100 tokens). Show duplicationsFound a 27 line (145 tokens) duplication in the following files:
fn generated_match_token_recovers_missing_token_from_context_follow() {
let atn = generated_match_recovery_atn();
let data = RecognizerData::new(
"Mini.g4",
Vocabulary::new(
[None, Some("'X'"), Some("'Y'")],
[None, Some("X"), Some("Y")],
[None::<&str>, None, None],
),
);
let mut parser = BaseParser::new(
CommonTokenStream::new(Source {
tokens: vec![TestToken::eof("parser-test", 3, 1, 3)],
index: 0,
}),
data,
);
parser.rule_context_stack = vec![
RuleContextFrame {
rule_index: 0,
invoking_state: 0,
},
RuleContextFrame {
rule_index: 1,
invoking_state: 1,
},
];
```rust
---
Found a 22 line (132 tokens) duplication in the following files:
* Starting at line 1100 of tests/antlr4_rust_gen_cli.rs
* Starting at line 1200 of tests/antlr4_rust_gen_cli.rs
```rust
"parser grammar Delegate;\ndelegated: {isTypeName()}? ID;\n",
)
.expect("delegate grammar should be writable");
fs::write(&tokens, "lexer grammar Tokens;\nID: [a-z]+;\n")
.expect("token grammar should be writable");
let output = run_antlr4_rust_gen(&[
root.as_os_str(),
tokens.as_os_str(),
OsStr::new("-I"),
temp.path().as_os_str(),
OsStr::new("--out-dir"),
out.as_os_str(),
]);
assert!(
output.status.success(),
"stdout: {}\nstderr: {}",
utf8(&output.stdout),
utf8(&output.stderr)
);
let parser = fs::read_to_string(out.join("root.rs")).expect("parser should be emitted");
assert!(parser.contains("pub trait RootHooks"), "{parser}");Found a 27 line (127 tokens) duplication in the following files:
let mut atn = ParserAtnBuilder::new(2);
assert_eq!(
atn.add_state(AtnStateKind::RuleStart, Some(0))
.expect("state")
.index(),
0
);
assert_eq!(
atn.add_state(AtnStateKind::BlockStart, Some(0))
.expect("state")
.index(),
1
);
assert_eq!(
atn.add_state(AtnStateKind::Basic, Some(0))
.expect("state")
.index(),
2
);
assert_eq!(
atn.add_state(AtnStateKind::Basic, Some(0))
.expect("state")
.index(),
3
);
assert_eq!(
atn.add_state(AtnStateKind::BlockEnd, Some(0))
```rust
---
Found a 22 line (125 tokens) duplication in the following files:
* Starting at line 14375 of src/parser.rs
* Starting at line 14447 of src/parser.rs
```rust
atn.add_state(AtnStateKind::BlockEnd, Some(0))
.expect("state")
.index(),
4
);
assert_eq!(
atn.add_state(AtnStateKind::RuleStop, Some(0))
.expect("state")
.index(),
5
);
atn.set_rule_to_start_state(vec![0])
.expect("rule start states");
atn.set_rule_to_stop_state(vec![5])
.expect("rule stop states");
atn.add_decision_state(1).expect("decision state");
atn.add_transition(0, ParserTransitionSpec::Epsilon { target: 1 })
.expect("transition");
atn.add_transition(
1,
ParserTransitionSpec::Atom {
target: 2,Found a 34 line (119 tokens) duplication in the following files:
outcomes.extend(
self.recognize_state(
atn,
RecognizeRequest {
state_number: *target,
stop_state,
index,
rule_start_index,
decision_start_index: next_decision_start_index,
init_action_rules,
predicates,
semantics,
rule_args,
member_actions,
return_actions,
local_int_arg,
member_values: member_values.clone(),
return_values: return_values.clone(),
rule_alt_number: next_alt_number,
track_alt_numbers,
consumed_eof,
committed_decision: transition_committed,
precedence,
depth: depth + 1,
recovery_symbols: epsilon_recovery_symbols.clone(),
recovery_state: epsilon_recovery_state,
},
visiting,
memo,
expected,
)
.into_iter()
.map(|mut outcome| {
prepend_decision(&mut outcome, decision);
```rust
---
Found a 15 line (113 tokens) duplication in the following files:
* Starting at line 15449 of src/parser.rs
* Starting at line 15725 of src/parser.rs
```rust
fn generated_match_token_counts_single_token_deletion_recovery() {
let atn = generated_match_recovery_atn();
let data = RecognizerData::new(
"Mini.g4",
Vocabulary::new(
[None, Some("'X'"), Some("'Y'"), Some("'Z'")],
[None, Some("X"), Some("Y"), Some("Z")],
[None::<&str>, None, None, None],
),
);
let mut parser = BaseParser::new(
CommonTokenStream::new(Source {
tokens: vec![
TestToken::new(3).with_text("z"),
TestToken::new(2).with_text("y"),Found a 12 line (112 tokens) duplication in the following files:
let mut atn = ParserAtnBuilder::new(1);
for (state, kind, rule) in [
(0, AtnStateKind::RuleStart, 0),
(1, AtnStateKind::StarLoopEntry, 0),
(2, AtnStateKind::Basic, 0), // ops hub
(3, AtnStateKind::Basic, 0), // shift prec
(4, AtnStateKind::Basic, 0), // shift first >
(5, AtnStateKind::Basic, 0), // shift second >
(6, AtnStateKind::Basic, 0), // rel prec
(7, AtnStateKind::Basic, 0), // rel >
(8, AtnStateKind::LoopEnd, 0),
(9, AtnStateKind::RuleStop, 0),
```rust
---
Found a 22 line (112 tokens) duplication in the following files:
* Starting at line 14786 of src/parser.rs
* Starting at line 14987 of src/parser.rs
```rust
fn predicate_after_token_atn() -> Atn {
let mut atn = ParserAtnBuilder::new(2);
assert_eq!(
atn.add_state(AtnStateKind::RuleStart, Some(0))
.expect("state")
.index(),
0
);
assert_eq!(
atn.add_state(AtnStateKind::Basic, Some(0))
.expect("state")
.index(),
1
);
assert_eq!(
atn.add_state(AtnStateKind::Basic, Some(0))
.expect("state")
.index(),
2
);
assert_eq!(
atn.add_state(AtnStateKind::Basic, Some(0))Found a 22 line (111 tokens) duplication in the following files:
fn left_recursive_loop_with_caller_follow_atn(caller_symbol: i32) -> Atn {
let mut atn = ParserAtnBuilder::new(2);
assert_eq!(
atn.add_state(AtnStateKind::RuleStart, Some(0))
.expect("state")
.index(),
0
);
assert_eq!(
atn.add_state(AtnStateKind::Basic, Some(0))
.expect("state")
.index(),
1
);
assert_eq!(
atn.add_state(AtnStateKind::Basic, Some(0))
.expect("state")
.index(),
2
);
assert_eq!(
atn.add_state(AtnStateKind::RuleStart, Some(1))
```rust
---
Found a 14 line (110 tokens) duplication in the following files:
* Starting at line 15120 of src/parser.rs
* Starting at line 17876 of src/parser.rs
```rust
fn parser_matches_token_and_reports_mismatch() {
let source = Source {
tokens: vec![
TestToken::new(1).with_text("x"),
TestToken::eof("parser-test", 1, 1, 1),
],
index: 0,
};
let data = RecognizerData::new(
"Mini.g4",
Vocabulary::new([None, Some("'x'")], [None, Some("X")], [None::<&str>, None]),
);
let mut parser = BaseParser::new(CommonTokenStream::new(source), data);
let matched = parser.match_token(1).expect("token 1 should match");Found a 13 line (109 tokens) duplication in the following files:
fn parser_matches_token_and_reports_mismatch() {
let source = Source {
tokens: vec![
TestToken::new(1).with_text("x"),
TestToken::eof("parser-test", 1, 1, 1),
],
index: 0,
};
let data = RecognizerData::new(
"Mini.g4",
Vocabulary::new([None, Some("'x'")], [None, Some("X")], [None::<&str>, None]),
);
let mut parser = BaseParser::new(CommonTokenStream::new(source), data);
```rust
---
Found a 22 line (108 tokens) duplication in the following files:
* Starting at line 7578 of src/parser.rs
* Starting at line 7969 of src/parser.rs
```rust
) -> Option<RecognizeOutcome> {
let (error_index, message) = self.expected_error_message(rule_index, start_index, expected);
let diagnostic = diagnostic_for_token(self.token_at(error_index), message);
let mut next_index = error_index;
loop {
let symbol = self.token_type_at(next_index);
if sync_symbols.contains(&symbol) {
if next_index == error_index {
return None;
}
break;
}
if symbol == TOKEN_EOF {
break;
}
let after = self.consume_index(next_index, symbol);
if after == next_index {
break;
}
next_index = after;
}
let mut nodes = NodeSeqId::EMPTY;Found a 15 line (108 tokens) duplication in the following files:
fn outcome_ties_keep_later_non_recursive_alternative() {
let arena = RecognitionArena::default();
let first = RecognizeOutcome {
index: 1,
consumed_eof: false,
alt_number: 0,
member_values: BTreeMap::new(),
return_values: BTreeMap::new(),
diagnostics: DiagnosticSeqId::EMPTY,
decisions: Vec::new(),
actions: vec![ParserAction::new(1, 0, 0, None)],
nodes: NodeSeqId::EMPTY,
};
let second = RecognizeOutcome {
actions: vec![ParserAction::new(2, 0, 0, None)],
```rust
---
Found a 16 line (105 tokens) duplication in the following files:
* Starting at line 6717 of src/parser.rs
* Starting at line 7337 of src/parser.rs
```rust
let start_state = atn.rule_to_start_state().get(rule_index).ok_or_else(|| {
AntlrError::Unsupported(format!("rule {rule_index} has no start state"))
})?;
let stop_state = atn
.rule_to_stop_state()
.get(rule_index)
.filter(|state| *state != usize::MAX)
.ok_or_else(|| {
AntlrError::Unsupported(format!("rule {rule_index} has no stop state"))
})?;
let start_index = self.current_visible_index();
self.clear_prediction_diagnostics();
self.reset_per_parse_caches();
self.reset_recognition_arena();
let caller_follow_state = self.pending_invoking_follow_state(atn);Found a 13 line (100 tokens) duplication in the following files:
let mut expected = BTreeSet::new();
for index in (1..self.rule_context_stack.len()).rev() {
let invoking_state = self.rule_context_stack[index].invoking_state;
let Ok(state_number) = usize::try_from(invoking_state) else {
continue;
};
let Some(Transition::Rule { follow_state, .. }) = atn
.state(state_number)
.and_then(|state| state.transitions().first())
.map(ParserTransition::data)
else {
continue;
};
```rust
</details> |
|
Claude finished @tinovyatkin's task in 14m 18s —— View job Code review — PR #194No blocking issues found. Checked for bugs and CLAUDE.md compliance across the full diff (5 files, +140/−13). Reviewed post-merge against
What was verifiedThe core claim — that the guard actually closes the hole — was validated empirically rather than by inspection:
The emitted code expands correctly through the Non-blocking observations1. Deep nesting now trends toward OOM rather than abort. The generated path has no ceiling analogous to 2. Test assertion is weaker than its comment (also raised by CodeRabbit). The comment says "Every generated dispatch method must carry the stack guard" but 3. Doc-comment phrasing. Upstream parityNothing to flag. ANTLR's Java runtime maps rule nesting onto JVM call depth and simply throws Reviewed by 4 independent agents (2× CLAUDE.md compliance, 2× bug/logic); every candidate finding was empirically validated or dropped. |
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.
Fixes #193.
What
Generated recursive-descent rule methods mapped grammar-rule nesting directly onto native call depth — the CEL grammar walks ~9 rules per
[, so ~850 nesting levels aborted the process on an 8 MiB stack. The interpreted path (recognize_state_fast, #147) and the tree walker were already segmented-stack safe; the generated parse path was the last unguarded recursion.The generator now emits a stack-capacity probe at the shared
parse_generated_rule_N_dispatchboundary:generated_rule_stack_check_duesamples once per 8 rule-context frames (rule_context_stack.len() % 8 == 0— no new state), andgrow_generated_rule_stackre-usesrecognize_state_fast's red-zone constants (1 MiB red zone, 4 MiB segments) viastacker::maybe_grow.Verification
New e2e test:
deep-nesting/Nest.g4(CEL-shaped 6-rule chain) parses 10 000 nesting levels on the default 2 MiB test-thread stack. Before the fix that input aborts the process.CEL spike: the exact bug(codegen): generated recursive-descent rules overflow the native stack on deeply nested input #193 reproducer now parses at depth 100 000 (previously aborted at ~850).
Conformance: full sweep 357 passed, 0 failed, 0 skipped.
Perf: Kotlin-parity dumper, min of 3×30 iters, quiet machine, baseline = origin/main runtime + codegen:
Within noise (±1%). Parse trees byte-identical pre/post on all snippets.
cargo test --locked(unit + doctests) andcargo clippy --locked --all-targets --all-features -- -D warningsclean.Summary by CodeRabbit
Bug Fixes
Tests