Skip to content

Commit

Permalink
Fix #19443
Browse files Browse the repository at this point in the history 
Reset the line number to its previous state after genBodyDebugScope, so
that instructions between its end and the next dbg_stmt do not wander
inside the block. This core fix is very simple, but surfaces a secondary
issue: the LLVM codegen generates blocks in a different order than Clang
would for the same CFG in C, which ultimately results in GDB skipping
the loop header altogether. So we need to reorder some basic blocks too.
  • Loading branch information
@tau-dev
tau-dev committed Jul 8, 2024
1 parent 854e86c commit 3ed994c
Show file tree
Hide file tree
Showing 2 changed files with 70 additions and 21 deletions.
41 changes: 32 additions & 9 deletions src/codegen/llvm.zig
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5150,13 +5150,25 @@ pub const FuncGen = struct {
const old_inlined = self.inlined;
const old_base_line = self.base_line;
const old_scope = self.scope;
defer if (maybe_inline_func) |_| {
self.wip.debug_location = self.inlined;
self.file = old_file;
self.inlined = old_inlined;
self.base_line = old_base_line;
};
defer self.scope = old_scope;
const old_line = self.prev_dbg_line;
const old_column = self.prev_dbg_column;

defer {
if (maybe_inline_func) |_| {
self.wip.debug_location = self.inlined;
self.file = old_file;
self.inlined = old_inlined;
self.base_line = old_base_line;
}
self.scope = old_scope;
// In some conditions, such as on the exit branch of a loop
// header, the body is not immediately followed by a
// dbg_stmt. In this case, it makes more sense to associate
// the following instructions with the statement before the
// scope than with last statement inside the scope.
self.prev_dbg_line = old_line;
self.prev_dbg_column = old_column;
}

if (maybe_inline_func) |inline_func| {
const o = self.dg.object;
Expand Down Expand Up @@ -5224,7 +5236,11 @@ pub const FuncGen = struct {
.no_location => {},
}
defer switch (self.wip.debug_location) {
.location => |*l| l.scope = old_scope,
.location => |*l| {
l.line = old_line;
l.column = old_column;
l.scope = old_scope;
},
.no_location => {},
};

Expand Down Expand Up @@ -5941,7 +5957,13 @@ pub const FuncGen = struct {
var breaks: BreakList = if (have_block_result) .{ .list = .{} } else .{ .len = 0 };
defer if (have_block_result) breaks.list.deinit(self.gpa);

const parent_bb = try self.wip.block(0, "Block");
// GDB is sensitive to the ordering of blocks. In a loop, for
// example, the update step will go into a parent_bb; if that
// block were placed here, its instructions would immediately
// follow the loop header and be covered by the same line
// number info. GDB would then never stop on the loop header
// while trying to step by line (cf. gdb/infrun.c:7682).
const parent_bb = try self.wip.unplacedBlock(0, "Block");
try self.blocks.putNoClobber(self.gpa, inst, .{
.parent_bb = parent_bb,
.breaks = &breaks,
Expand All @@ -5950,6 +5972,7 @@ pub const FuncGen = struct {

try self.genBodyDebugScope(maybe_inline_func, body);

try self.wip.placeBlock(parent_bb);
self.wip.cursor = .{ .block = parent_bb };

// Create a phi node only if the block returns a value.
Expand Down
50 changes: 38 additions & 12 deletions src/codegen/llvm/Builder.zig
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3988,6 +3988,8 @@ pub const Function = struct {

pub const Block = struct {
instruction: Instruction.Index,
// Reverses the block_order mapping.
remapped_index: u32,

pub const Index = WipFunction.Block.Index;
};
Expand Down Expand Up @@ -4922,6 +4924,10 @@ pub const Function = struct {
return argument_index.toValue();
}

fn remapBlock(self: @This(), blk: Block.Index) u32 {
return self.blocks[@intFromEnum(blk)].remapped_index;
}

const ExtraDataTrail = struct {
index: Instruction.ExtraIndex,

Expand Down Expand Up @@ -5008,6 +5014,7 @@ pub const WipFunction = struct {
debug_location: DebugLocation,
cursor: Cursor,
blocks: std.ArrayListUnmanaged(Block),
block_order: std.ArrayListUnmanaged(u32),
instructions: std.MultiArrayList(Instruction),
names: std.ArrayListUnmanaged(String),
strip: bool,
Expand Down Expand Up @@ -5054,6 +5061,7 @@ pub const WipFunction = struct {
.debug_location = .no_location,
.cursor = undefined,
.blocks = .{},
.block_order = .{},
.instructions = .{},
.names = .{},
.strip = options.strip,
Expand Down Expand Up @@ -5089,6 +5097,13 @@ pub const WipFunction = struct {
}

pub fn block(self: *WipFunction, incoming: u32, name: []const u8) Allocator.Error!Block.Index {
try self.block_order.ensureUnusedCapacity(self.builder.gpa, 1);
const res = try self.unplacedBlock(incoming, name);
self.block_order.appendAssumeCapacity(@intFromEnum(res));
return res;
}

pub fn unplacedBlock(self: *WipFunction, incoming: u32, name: []const u8) Allocator.Error!Block.Index {
try self.blocks.ensureUnusedCapacity(self.builder.gpa, 1);

const index: Block.Index = @enumFromInt(self.blocks.items.len);
Expand All @@ -5101,6 +5116,10 @@ pub const WipFunction = struct {
return index;
}

pub fn placeBlock(self: *WipFunction, blk: Block.Index) Allocator.Error!void {
try self.block_order.append(self.builder.gpa, @intFromEnum(blk));
}

pub fn ret(self: *WipFunction, val: Value) Allocator.Error!Instruction.Index {
assert(val.typeOfWip(self) == self.function.typeOf(self.builder).functionReturn(self.builder));
try self.ensureUnusedExtraCapacity(1, NoExtra, 0);
Expand Down Expand Up @@ -5973,6 +5992,7 @@ pub const WipFunction = struct {
const function = self.function.ptr(self.builder);
const params_len = self.function.typeOf(self.builder).functionParameters(self.builder).len;
const final_instructions_len = self.blocks.items.len + self.instructions.len;
assert(self.blocks.items.len == self.block_order.items.len); // All blocks should be placed once.

const blocks = try gpa.alloc(Function.Block, self.blocks.items.len);
errdefer gpa.free(blocks);
Expand Down Expand Up @@ -6075,15 +6095,19 @@ pub const WipFunction = struct {
instructions.items[param_index] = final_instruction_index;
final_instruction_index = @enumFromInt(@intFromEnum(final_instruction_index) + 1);
}
for (blocks, self.blocks.items) |*final_block, current_block| {
for (self.block_order.items) |i| {
const current_block = self.blocks.items[i];
assert(current_block.incoming == current_block.branches);
final_block.instruction = final_instruction_index;
blocks[i].instruction = final_instruction_index;
final_instruction_index = @enumFromInt(@intFromEnum(final_instruction_index) + 1);
for (current_block.instructions.items) |instruction| {
instructions.items[@intFromEnum(instruction)] = final_instruction_index;
final_instruction_index = @enumFromInt(@intFromEnum(final_instruction_index) + 1);
}
}
for (self.block_order.items, 0..) |blk_idx, llvm_idx| {
blocks[blk_idx].remapped_index = @intCast(llvm_idx);
}
}

var wip_name: struct {
Expand Down Expand Up @@ -6150,7 +6174,8 @@ pub const WipFunction = struct {
debug_values[index] = new_argument_index;
}
}
for (self.blocks.items) |current_block| {
for (self.block_order.items) |i| {
const current_block = self.blocks.items[i];
const new_block_index: Instruction.Index = @enumFromInt(function.instructions.len);
value_indices[function.instructions.len] = value_index;
function.instructions.appendAssumeCapacity(.{
Expand Down Expand Up @@ -6494,6 +6519,7 @@ pub const WipFunction = struct {
self.instructions.deinit(self.builder.gpa);
for (self.blocks.items) |*b| b.instructions.deinit(self.builder.gpa);
self.blocks.deinit(self.builder.gpa);
self.block_order.deinit(self.builder.gpa);
self.* = undefined;
}

Expand Down Expand Up @@ -13654,7 +13680,7 @@ pub fn toBitcode(self: *Builder, allocator: Allocator) bitcode_writer.Error![]co
try constants_block.writeAbbrev(Constants.BlockAddress{
.type_id = extra.function.typeOf(self),
.function = constant_adapter.getConstantIndex(extra.function.toConst(self)),
.block = @intFromEnum(extra.block),
.block = extra.function.ptrConst(self).remapBlock(extra.block),
});
},
.dso_local_equivalent,
Expand Down Expand Up @@ -14618,14 +14644,14 @@ pub fn toBitcode(self: *Builder, allocator: Allocator) bitcode_writer.Error![]co
},
.br => {
try function_block.writeAbbrev(FunctionBlock.BrUnconditional{
.block = datas[instr_index],
.block = func.remapBlock(@enumFromInt(datas[instr_index])),
});
},
.br_cond => {
const extra = func.extraData(Function.Instruction.BrCond, datas[instr_index]);
try function_block.writeAbbrev(FunctionBlock.BrConditional{
.then_block = @intFromEnum(extra.then),
.else_block = @intFromEnum(extra.@"else"),
.then_block = func.remapBlock(extra.then),
.else_block = func.remapBlock(extra.@"else"),
.condition = adapter.getOffsetValueIndex(extra.cond),
});
},
Expand All @@ -14641,13 +14667,13 @@ pub fn toBitcode(self: *Builder, allocator: Allocator) bitcode_writer.Error![]co
record.appendAssumeCapacity(adapter.getOffsetValueIndex(extra.data.val));

// Default block
record.appendAssumeCapacity(@intFromEnum(extra.data.default));
record.appendAssumeCapacity(func.remapBlock(extra.data.default));

const vals = extra.trail.next(extra.data.cases_len, Constant, &func);
const blocks = extra.trail.next(extra.data.cases_len, Function.Block.Index, &func);
for (vals, blocks) |val, block| {
record.appendAssumeCapacity(adapter.constant_adapter.getConstantIndex(val));
record.appendAssumeCapacity(@intFromEnum(block));
record.appendAssumeCapacity(func.remapBlock(block));
}

try function_block.writeUnabbrev(12, record.items);
Expand All @@ -14661,7 +14687,7 @@ pub fn toBitcode(self: *Builder, allocator: Allocator) bitcode_writer.Error![]co
});
},
.phi,
.@"phi fast",
.@"phi fast",
=> |kind| {
var extra = func.extraDataTrail(Function.Instruction.Phi, datas[instr_index]);
const vals = extra.trail.next(block_incoming_len, Value, &func);
Expand All @@ -14679,7 +14705,7 @@ pub fn toBitcode(self: *Builder, allocator: Allocator) bitcode_writer.Error![]co
const abs_value: u32 = @intCast(@abs(offset_value));
const signed_vbr = if (offset_value > 0) abs_value << 1 else ((abs_value << 1) | 1);
record.appendAssumeCapacity(signed_vbr);
record.appendAssumeCapacity(@intFromEnum(block));
record.appendAssumeCapacity(func.remapBlock(block));
}

if (kind == .@"phi fast") record.appendAssumeCapacity(@as(u8, @bitCast(FastMath{})));
Expand Down Expand Up @@ -14762,7 +14788,7 @@ pub fn toBitcode(self: *Builder, allocator: Allocator) bitcode_writer.Error![]co
if (name == .none or name == .empty) continue;

try value_symtab_block.writeAbbrev(ValueSymbolTable.BlockEntry{
.value_id = @intCast(block_index),
.value_id = func.remapBlock(@enumFromInt(block_index)),
.string = name.slice(self).?,
});
}
Expand Down

0 comments on commit 3ed994c

Please sign in to comment.