Skip to content

Commit 532aa3c

Browse files
committed
cbe: work around some miscompilations
The changes to `codegen.c` are blatant hacks, but the problem they work around isn't a regression: it's an existing miscompilation. This branch happened to *expose* that miscompilation in more cases by changing how an incorrect result is *used*.
1 parent 5df5e2e commit 532aa3c

File tree

3 files changed

+54
-3
lines changed

3 files changed

+54
-3
lines changed

src/Type.zig

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3556,7 +3556,7 @@ pub fn packedStructFieldPtrInfo(
35563556
} else .{
35573557
switch (zcu.comp.getZigBackend()) {
35583558
else => (running_bits + 7) / 8,
3559-
.stage2_x86_64 => @intCast(struct_ty.abiSize(zcu)),
3559+
.stage2_x86_64, .stage2_c => @intCast(struct_ty.abiSize(zcu)),
35603560
},
35613561
bit_offset,
35623562
};

src/codegen/c.zig

Lines changed: 52 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3801,6 +3801,24 @@ fn airAlloc(f: *Function, inst: Air.Inst.Index) !CValue {
38013801
});
38023802
log.debug("%{d}: allocated unfreeable t{d}", .{ inst, local.new_local });
38033803
try f.allocs.put(zcu.gpa, local.new_local, true);
3804+
3805+
switch (elem_ty.zigTypeTag(zcu)) {
3806+
.@"struct", .@"union" => switch (elem_ty.containerLayout(zcu)) {
3807+
.@"packed" => {
3808+
// For packed aggregates, we zero-initialize to try and work around a design flaw
3809+
// related to how `packed`, `undefined`, and RLS interact. See comment in `airStore`
3810+
// for details.
3811+
const w = &f.object.code.writer;
3812+
try w.print("memset(&t{d}, 0x00, sizeof(", .{local.new_local});
3813+
try f.renderType(w, elem_ty);
3814+
try w.writeAll("));");
3815+
try f.object.newline();
3816+
},
3817+
.auto, .@"extern" => {},
3818+
},
3819+
else => {},
3820+
}
3821+
38043822
return .{ .local_ref = local.new_local };
38053823
}
38063824
@@ -3820,6 +3838,24 @@ fn airRetPtr(f: *Function, inst: Air.Inst.Index) !CValue {
38203838
});
38213839
log.debug("%{d}: allocated unfreeable t{d}", .{ inst, local.new_local });
38223840
try f.allocs.put(zcu.gpa, local.new_local, true);
3841+
3842+
switch (elem_ty.zigTypeTag(zcu)) {
3843+
.@"struct", .@"union" => switch (elem_ty.containerLayout(zcu)) {
3844+
.@"packed" => {
3845+
// For packed aggregates, we zero-initialize to try and work around a design flaw
3846+
// related to how `packed`, `undefined`, and RLS interact. See comment in `airStore`
3847+
// for details.
3848+
const w = &f.object.code.writer;
3849+
try w.print("memset(&t{d}, 0x00, sizeof(", .{local.new_local});
3850+
try f.renderType(w, elem_ty);
3851+
try w.writeAll("));");
3852+
try f.object.newline();
3853+
},
3854+
.auto, .@"extern" => {},
3855+
},
3856+
else => {},
3857+
}
3858+
38233859
return .{ .local_ref = local.new_local };
38243860
}
38253861
@@ -4098,9 +4134,24 @@ fn airStore(f: *Function, inst: Air.Inst.Index, safety: bool) !CValue {
40984134
if (val_is_undef) {
40994135
try reap(f, inst, &.{ bin_op.lhs, bin_op.rhs });
41004136
if (safety and ptr_info.packed_offset.host_size == 0) {
4137+
// If the thing we're initializing is a packed struct/union, we set to 0 instead of
4138+
// 0xAA. This is a hack to work around a problem with partially-undefined packed
4139+
// aggregates. If we used 0xAA here, then a later initialization through RLS would
4140+
// not zero the high padding bits (for a packed type which is not 8/16/32/64/etc bits),
4141+
// so we would get a miscompilation. Using 0x00 here avoids this bug in some cases. It
4142+
// is *not* a correct fix; for instance it misses any case where packed structs are
4143+
// nested in other aggregates. A proper fix for this will involve changing the language,
4144+
// such as to remove RLS. This just prevents miscompilations in *some* common cases.
4145+
const byte_str: []const u8 = switch (src_ty.zigTypeTag(zcu)) {
4146+
else => "0xaa",
4147+
.@"struct", .@"union" => switch (src_ty.containerLayout(zcu)) {
4148+
.auto, .@"extern" => "0xaa",
4149+
.@"packed" => "0x00",
4150+
},
4151+
};
41014152
try w.writeAll("memset(");
41024153
try f.writeCValue(w, ptr_val, .FunctionArgument);
4103-
try w.writeAll(", 0xaa, sizeof(");
4154+
try w.print(", {s}, sizeof(", .{byte_str});
41044155
try f.renderType(w, .fromInterned(ptr_info.child));
41054156
try w.writeAll("));");
41064157
try f.object.newline();

test/behavior/union.zig

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1547,7 +1547,7 @@ test "packed union field pointer has correct alignment" {
15471547

15481548
const host_size = switch (builtin.zig_backend) {
15491549
else => comptime std.math.divCeil(comptime_int, @bitSizeOf(S), 8) catch unreachable,
1550-
.stage2_x86_64 => @sizeOf(S),
1550+
.stage2_x86_64, .stage2_c => @sizeOf(S),
15511551
};
15521552
comptime assert(@TypeOf(ap) == *align(4:2:host_size) u20);
15531553
comptime assert(@TypeOf(bp) == *align(1:2:host_size) u20);

0 commit comments

Comments
 (0)