Skip to content

Commit c4e1123

Browse files
jgarzikjgarzik
committed
[cc] review fixes
1 parent 4811648 commit c4e1123

File tree

3 files changed

+56
-84
lines changed

3 files changed

+56
-84
lines changed

‎cc/ir/linearize.rs‎

Lines changed: 29 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -470,6 +470,20 @@ impl<'a> Linearizer<'a> {
470470
false
471471
}
472472

473+
/// Link current block to merge block if not terminated.
474+
/// Used after linearizing then/else branches to connect to merge block.
475+
fn link_to_merge_if_needed(&mut self, merge_bb: BasicBlockId) {
476+
if self.is_terminated() {
477+
return;
478+
}
479+
if let Some(current) = self.current_bb {
480+
// If the block isn't terminated, it needs a branch to the merge block.
481+
// Any unreachable blocks will be cleaned up by dead code elimination.
482+
self.emit(Instruction::br(merge_bb));
483+
self.link_bb(current, merge_bb);
484+
}
485+
}
486+
473487
/// Link two basic blocks (parent -> child)
474488
fn link_bb(&mut self, from: BasicBlockId, to: BasicBlockId) {
475489
let func = self.current_func.as_mut().unwrap();
@@ -1747,50 +1761,13 @@ impl<'a> Linearizer<'a> {
17471761
// Then block
17481762
self.switch_bb(then_bb);
17491763
self.linearize_stmt(then_stmt);
1750-
if !self.is_terminated() {
1751-
// After linearize_stmt, current_bb may be different from then_bb
1752-
// (e.g., if then_stmt contains nested control flow). Link the
1753-
// CURRENT block to merge_bb, but only if it's reachable.
1754-
if let Some(current) = self.current_bb {
1755-
// A block is reachable if it's the original block we switched to
1756-
// (which has an incoming edge from the condition), or if it has
1757-
// any parents from previous links.
1758-
let is_reachable = current == then_bb
1759-
|| self
1760-
.current_func
1761-
.as_ref()
1762-
.and_then(|f| f.get_block(current))
1763-
.map(|bb| !bb.parents.is_empty())
1764-
.unwrap_or(false);
1765-
if is_reachable {
1766-
self.emit(Instruction::br(merge_bb));
1767-
self.link_bb(current, merge_bb);
1768-
}
1769-
}
1770-
}
1764+
self.link_to_merge_if_needed(merge_bb);
17711765

17721766
// Else block
17731767
if let Some(else_s) = else_stmt {
17741768
self.switch_bb(else_bb);
17751769
self.linearize_stmt(else_s);
1776-
if !self.is_terminated() {
1777-
// After linearize_stmt, current_bb may be different from else_bb
1778-
// (e.g., if else_stmt is a nested if-else chain). Link the
1779-
// CURRENT block to merge_bb, but only if it's reachable.
1780-
if let Some(current) = self.current_bb {
1781-
let is_reachable = current == else_bb
1782-
|| self
1783-
.current_func
1784-
.as_ref()
1785-
.and_then(|f| f.get_block(current))
1786-
.map(|bb| !bb.parents.is_empty())
1787-
.unwrap_or(false);
1788-
if is_reachable {
1789-
self.emit(Instruction::br(merge_bb));
1790-
self.link_bb(current, merge_bb);
1791-
}
1792-
}
1793-
}
1770+
self.link_to_merge_if_needed(merge_bb);
17941771
}
17951772

17961773
// Merge block
@@ -4472,20 +4449,22 @@ impl<'a> Linearizer<'a> {
44724449
OffsetOfPath::Field(field_id) => {
44734450
// Look up the field in the current struct type
44744451
let struct_type = self.resolve_struct_type(current_type);
4475-
if let Some(member_info) =
4476-
self.types.find_member(struct_type, *field_id)
4477-
{
4478-
offset += member_info.offset as u64;
4479-
current_type = member_info.typ;
4480-
}
4452+
let member_info = self
4453+
.types
4454+
.find_member(struct_type, *field_id)
4455+
.expect("offsetof: field not found in struct type");
4456+
offset += member_info.offset as u64;
4457+
current_type = member_info.typ;
44814458
}
44824459
OffsetOfPath::Index(index) => {
44834460
// Array indexing: offset += index * sizeof(element)
4484-
if let Some(elem_type) = self.types.base_type(current_type) {
4485-
let elem_size = self.types.size_bytes(elem_type);
4486-
offset += (*index as u64) * (elem_size as u64);
4487-
current_type = elem_type;
4488-
}
4461+
let elem_type = self
4462+
.types
4463+
.base_type(current_type)
4464+
.expect("offsetof: array index on non-array type");
4465+
let elem_size = self.types.size_bytes(elem_type);
4466+
offset += (*index as u64) * (elem_size as u64);
4467+
current_type = elem_type;
44894468
}
44904469
}
44914470
}

‎cc/ir/test_linearize.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -327,8 +327,8 @@ fn test_simple_array_element_store() {
327327
#[test]
328328
fn test_nested_if_cfg_linking() {
329329
// Test: if (outer) { if (inner) { x = 1; } else { x = 2; } } else { x = 3; }
330-
// After nested if-else, current_bb points to inner merge block, not outer then_bb.
331-
// The fix ensures the inner merge block is correctly linked to outer merge block.
330+
// Verifies that after a nested if-else in the then branch, the inner merge block
331+
// is correctly linked to the outer merge block in the resulting control-flow graph.
332332
let mut strings = StringTable::new();
333333
let types = TypeTable::new(64);
334334
let test_id = strings.intern("test");

‎cc/parse/parser.rs‎

Lines changed: 25 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -2512,8 +2512,8 @@ impl<'a> Parser<'a> {
25122512

25132513
self.expect_special(b',')?;
25142514

2515-
// Parse member-designator: field, .field, [index], or chains
2516-
// First component is a field name (no leading dot required)
2515+
// Parse member-designator starting with field name (no dot prefix for first field)
2516+
// Subsequent components use .field or [index] syntax
25172517
let mut path = Vec::new();
25182518

25192519
// Expect identifier for first member
@@ -2530,9 +2530,16 @@ impl<'a> Parser<'a> {
25302530
self.advance();
25312531
// Parse constant expression for index
25322532
let index_expr = self.parse_conditional_expr()?;
2533+
let index_pos = index_expr.pos;
25332534
self.expect_special(b']')?;
2534-
// Evaluate as constant
2535-
let index_val = self.eval_const_expr(&index_expr).unwrap_or(0);
2535+
// Evaluate as constant - offsetof requires compile-time constant
2536+
let index_val =
2537+
self.eval_const_expr(&index_expr).ok_or_else(|| {
2538+
ParseError::new(
2539+
"array index in offsetof must be a constant expression",
2540+
index_pos,
2541+
)
2542+
})?;
25362543
path.push(OffsetOfPath::Index(index_val));
25372544
} else {
25382545
break;
@@ -3205,12 +3212,8 @@ impl Parser<'_> {
32053212
/// Blocks create their own scope for local declarations. This enters a
32063213
/// new scope, parses the block, binds any declarations, then leaves
32073214
/// the scope.
3208-
fn parse_block_stmt(&mut self) -> ParseResult<Stmt> {
3209-
self.expect_special(b'{')?;
3210-
3211-
// Enter block scope
3212-
self.symbols.enter_scope();
3213-
3215+
/// Parse block items (declarations and statements) until closing brace
3216+
fn parse_block_items(&mut self) -> ParseResult<Vec<BlockItem>> {
32143217
let mut items = Vec::new();
32153218
while !self.is_special(b'}') && !self.is_eof() {
32163219
if self.is_declaration_start() {
@@ -3221,6 +3224,16 @@ impl Parser<'_> {
32213224
items.push(BlockItem::Statement(stmt));
32223225
}
32233226
}
3227+
Ok(items)
3228+
}
3229+
3230+
fn parse_block_stmt(&mut self) -> ParseResult<Stmt> {
3231+
self.expect_special(b'{')?;
3232+
3233+
// Enter block scope
3234+
self.symbols.enter_scope();
3235+
3236+
let items = self.parse_block_items()?;
32243237

32253238
// Leave block scope
32263239
self.symbols.leave_scope();
@@ -3235,18 +3248,7 @@ impl Parser<'_> {
32353248
/// by the function parsing code (to include parameters in scope).
32363249
fn parse_block_stmt_no_scope(&mut self) -> ParseResult<Stmt> {
32373250
self.expect_special(b'{')?;
3238-
3239-
let mut items = Vec::new();
3240-
while !self.is_special(b'}') && !self.is_eof() {
3241-
if self.is_declaration_start() {
3242-
let decl = self.parse_declaration_and_bind()?;
3243-
items.push(BlockItem::Declaration(decl));
3244-
} else {
3245-
let stmt = self.parse_statement()?;
3246-
items.push(BlockItem::Statement(stmt));
3247-
}
3248-
}
3249-
3251+
let items = self.parse_block_items()?;
32503252
self.expect_special(b'}')?;
32513253
Ok(Stmt::Block(items))
32523254
}
@@ -3260,16 +3262,7 @@ impl Parser<'_> {
32603262
// Enter block scope for the statement expression
32613263
self.symbols.enter_scope();
32623264

3263-
let mut items = Vec::new();
3264-
while !self.is_special(b'}') && !self.is_eof() {
3265-
if self.is_declaration_start() {
3266-
let decl = self.parse_declaration_and_bind()?;
3267-
items.push(BlockItem::Declaration(decl));
3268-
} else {
3269-
let stmt = self.parse_statement()?;
3270-
items.push(BlockItem::Statement(stmt));
3271-
}
3272-
}
3265+
let mut items = self.parse_block_items()?;
32733266

32743267
self.expect_special(b'}')?;
32753268
self.expect_special(b')')?;

0 commit comments

Comments
 (0)