From 5b594139d93bb94642a94928adf00506f30da746 Mon Sep 17 00:00:00 2001 From: mlugg Date: Sat, 10 Jun 2023 01:23:17 +0100 Subject: [PATCH 1/3] Sema: copy pointer alignment to union field pointers This implements the semantics as discussed in today's compiler meeting, where the alignment of pointers to fields of default-layout unions cannot exceed the field's alignment. Resolves: #15878 --- src/Sema.zig | 13 +++-- test/behavior/union.zig | 109 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 119 insertions(+), 3 deletions(-) diff --git a/src/Sema.zig b/src/Sema.zig index a638c4d04b..4403df671a 100644 --- a/src/Sema.zig +++ b/src/Sema.zig @@ -26060,6 +26060,7 @@ fn unionFieldPtr( assert(unresolved_union_ty.zigTypeTag(mod) == .Union); const union_ptr_ty = sema.typeOf(union_ptr); + const union_ptr_info = union_ptr_ty.ptrInfo(mod); const union_ty = try sema.resolveTypeFields(unresolved_union_ty); const union_obj = mod.typeToUnion(union_ty).?; const field_index = try sema.unionFieldIndex(block, union_ty, field_name, field_name_src); @@ -26067,10 +26068,16 @@ fn unionFieldPtr( const ptr_field_ty = try mod.ptrType(.{ .child = field.ty.toIntern(), .flags = .{ - .is_const = !union_ptr_ty.ptrIsMutable(mod), - .is_volatile = union_ptr_ty.isVolatilePtr(mod), - .address_space = union_ptr_ty.ptrAddressSpace(mod), + .is_const = union_ptr_info.flags.is_const, + .is_volatile = union_ptr_info.flags.is_volatile, + .address_space = union_ptr_info.flags.address_space, + .alignment = if (union_obj.layout == .Auto) blk: { + const union_align = union_ptr_info.flags.alignment.toByteUnitsOptional() orelse try sema.typeAbiAlignment(union_ty); + const field_align = try sema.unionFieldAlignment(field); + break :blk InternPool.Alignment.fromByteUnits(@min(union_align, field_align)); + } else union_ptr_info.flags.alignment, }, + .packed_offset = union_ptr_info.packed_offset, }); const enum_field_index = @as(u32, @intCast(union_obj.tag_ty.enumFieldIndex(field_name, mod).?)); diff --git a/test/behavior/union.zig b/test/behavior/union.zig index 33cf1198ad..f7a481e311 100644 --- a/test/behavior/union.zig +++ b/test/behavior/union.zig @@ -1583,3 +1583,112 @@ test "coerce enum literal to union in result loc" { try U.doTest(true); try comptime U.doTest(true); } + +test "defined-layout union field pointer has correct alignment" { + if (builtin.zig_backend == .stage2_c) return error.SkipZigTest; // TODO + if (builtin.zig_backend == .stage2_wasm) return error.SkipZigTest; // TODO + if (builtin.zig_backend == .stage2_x86_64) return error.SkipZigTest; // TODO + if (builtin.zig_backend == .stage2_arm) return error.SkipZigTest; // TODO + if (builtin.zig_backend == .stage2_aarch64) return error.SkipZigTest; // TODO + if (builtin.zig_backend == .stage2_sparc64) return error.SkipZigTest; // TODO + if (builtin.zig_backend == .stage2_spirv64) return error.SkipZigTest; // TODO + + const S = struct { + fn doTheTest(comptime U: type) !void { + var a: U = .{ .x = 123 }; + var b: U align(1) = .{ .x = 456 }; + var c: U align(64) = .{ .x = 789 }; + + const ap = &a.x; + const bp = &b.x; + const cp = &c.x; + + comptime assert(@TypeOf(ap) == *u32); + comptime assert(@TypeOf(bp) == *align(1) u32); + comptime assert(@TypeOf(cp) == *align(64) u32); + + try expectEqual(@as(u32, 123), ap.*); + try expectEqual(@as(u32, 456), bp.*); + try expectEqual(@as(u32, 789), cp.*); + } + }; + + const U1 = extern union { x: u32 }; + const U2 = packed union { x: u32 }; + + try S.doTheTest(U1); + try S.doTheTest(U2); + try comptime S.doTheTest(U1); + try comptime S.doTheTest(U2); +} + +test "undefined-layout union field pointer has correct alignment" { + if (builtin.zig_backend == .stage2_c) return error.SkipZigTest; // TODO + if (builtin.zig_backend == .stage2_wasm) return error.SkipZigTest; // TODO + if (builtin.zig_backend == .stage2_x86_64) return error.SkipZigTest; // TODO + if (builtin.zig_backend == .stage2_arm) return error.SkipZigTest; // TODO + if (builtin.zig_backend == .stage2_aarch64) return error.SkipZigTest; // TODO + if (builtin.zig_backend == .stage2_sparc64) return error.SkipZigTest; // TODO + if (builtin.zig_backend == .stage2_spirv64) return error.SkipZigTest; // TODO + + const S = struct { + fn doTheTest(comptime U: type) !void { + var a: U = .{ .x = 123 }; + var b: U align(1) = .{ .x = 456 }; + var c: U align(64) = .{ .x = 789 }; + + const ap = &a.x; + const bp = &b.x; + const cp = &c.x; + + comptime assert(@TypeOf(ap) == *u32); + comptime assert(@TypeOf(bp) == *align(1) u32); + comptime assert(@TypeOf(cp) == *u32); // undefined layout so does not inherit larger aligns + + try expectEqual(@as(u32, 123), ap.*); + try expectEqual(@as(u32, 456), bp.*); + try expectEqual(@as(u32, 789), cp.*); + } + }; + + const U1 = union { x: u32 }; + const U2 = union(enum) { x: u32 }; + + try S.doTheTest(U1); + try S.doTheTest(U2); + try comptime S.doTheTest(U1); + try comptime S.doTheTest(U2); +} + +test "packed union field pointer has correct alignment" { + if (builtin.zig_backend == .stage2_c) return error.SkipZigTest; // TODO + if (builtin.zig_backend == .stage2_wasm) return error.SkipZigTest; // TODO + if (builtin.zig_backend == .stage2_x86_64) return error.SkipZigTest; // TODO + if (builtin.zig_backend == .stage2_arm) return error.SkipZigTest; // TODO + if (builtin.zig_backend == .stage2_aarch64) return error.SkipZigTest; // TODO + if (builtin.zig_backend == .stage2_sparc64) return error.SkipZigTest; // TODO + if (builtin.zig_backend == .stage2_spirv64) return error.SkipZigTest; // TODO + + const U = packed union { x: u20 }; + const S = packed struct(u24) { a: u2, u: U, b: u2 }; + + var a: S = undefined; + var b: S align(1) = undefined; + var c: S align(64) = undefined; + + const ap = &a.u.x; + const bp = &b.u.x; + const cp = &c.u.x; + + comptime assert(@TypeOf(ap) == *align(4:2:3) u20); + comptime assert(@TypeOf(bp) == *align(1:2:3) u20); + comptime assert(@TypeOf(cp) == *align(64:2:3) u20); + + a.u = .{ .x = 123 }; + b.u = .{ .x = 456 }; + c.u = .{ .x = 789 }; + + try expectEqual(@as(u20, 123), ap.*); + try expectEqual(@as(u20, 456), bp.*); + try expectEqual(@as(u20, 789), cp.*); +} From 730f2e040770b9f537922c18d95d8bef0b2603d7 Mon Sep 17 00:00:00 2001 From: mlugg Date: Fri, 16 Jun 2023 23:08:33 +0100 Subject: [PATCH 2/3] Sema: copy pointer alignment to struct field pointers --- src/Module.zig | 1 + src/Sema.zig | 34 ++++++++++++++++-- test/behavior/struct.zig | 77 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 109 insertions(+), 3 deletions(-) diff --git a/src/Module.zig b/src/Module.zig index 6d04b8f894..16f1c4f641 100644 --- a/src/Module.zig +++ b/src/Module.zig @@ -991,6 +991,7 @@ pub const Struct = struct { is_comptime: bool, /// Returns the field alignment. If the struct is packed, returns 0. + /// Keep implementation in sync with `Sema.structFieldAlignment`. pub fn alignment( field: Field, mod: *Module, diff --git a/src/Sema.zig b/src/Sema.zig index 4403df671a..16b3b5c710 100644 --- a/src/Sema.zig +++ b/src/Sema.zig @@ -25842,6 +25842,9 @@ fn structFieldPtrByIndex( const target = mod.getTarget(); + const parent_align = struct_ptr_ty_info.flags.alignment.toByteUnitsOptional() orelse + try sema.typeAbiAlignment(struct_ptr_ty_info.child.toType()); + if (struct_obj.layout == .Packed) { comptime assert(Type.packed_struct_layout_version == 2); @@ -25863,8 +25866,6 @@ fn structFieldPtrByIndex( ptr_ty_data.packed_offset.bit_offset += struct_ptr_ty_info.packed_offset.bit_offset; } - const parent_align = struct_ptr_ty_info.flags.alignment.toByteUnitsOptional() orelse - struct_ptr_ty_info.child.toType().abiAlignment(mod); ptr_ty_data.flags.alignment = Alignment.fromByteUnits(parent_align); // If the field happens to be byte-aligned, simplify the pointer type. @@ -25888,8 +25889,13 @@ fn structFieldPtrByIndex( ptr_ty_data.packed_offset = .{ .host_size = 0, .bit_offset = 0 }; } } + } else if (struct_obj.layout == .Extern and field_index == 0) { + // This is the first field in memory, so can inherit the struct alignment + ptr_ty_data.flags.alignment = Alignment.fromByteUnits(parent_align); } else { - ptr_ty_data.flags.alignment = field.abi_align; + // Our alignment is capped at the field alignment + const field_align = try sema.structFieldAlignment(field, struct_obj.layout); + ptr_ty_data.flags.alignment = Alignment.fromByteUnits(@min(field_align, parent_align)); } const ptr_field_ty = try mod.ptrType(ptr_ty_data); @@ -35952,6 +35958,28 @@ fn unionFieldAlignment(sema: *Sema, field: Module.Union.Field) !u32 { field.abi_align.toByteUnitsOptional() orelse try sema.typeAbiAlignment(field.ty))); } +/// Keep implementation in sync with `Module.Struct.Field.alignment`. +fn structFieldAlignment(sema: *Sema, field: Module.Struct.Field, layout: std.builtin.Type.ContainerLayout) !u32 { + const mod = sema.mod; + if (field.abi_align.toByteUnitsOptional()) |a| { + assert(layout != .Packed); + return @as(u32, @intCast(a)); + } + switch (layout) { + .Packed => return 0, + .Auto => if (mod.getTarget().ofmt != .c) { + return sema.typeAbiAlignment(field.ty); + }, + .Extern => {}, + } + // extern + const ty_abi_align = try sema.typeAbiAlignment(field.ty); + if (field.ty.isAbiInt(mod) and field.ty.intInfo(mod).bits >= 128) { + return @max(ty_abi_align, 16); + } + return ty_abi_align; +} + /// Synchronize logic with `Type.isFnOrHasRuntimeBits`. pub fn fnHasRuntimeBits(sema: *Sema, ty: Type) CompileError!bool { const mod = sema.mod; diff --git a/test/behavior/struct.zig b/test/behavior/struct.zig index 95b2718efd..08c4c37eee 100644 --- a/test/behavior/struct.zig +++ b/test/behavior/struct.zig @@ -1,6 +1,7 @@ const std = @import("std"); const builtin = @import("builtin"); const native_endian = builtin.target.cpu.arch.endian(); +const assert = std.debug.assert; const expect = std.testing.expect; const expectEqual = std.testing.expectEqual; const expectEqualSlices = std.testing.expectEqualSlices; @@ -1637,3 +1638,79 @@ test "instantiate struct with comptime field" { comptime std.debug.assert(things.foo == 1); } } + +test "struct field pointer has correct alignment" { + if (builtin.zig_backend == .stage2_c) return error.SkipZigTest; // TODO + if (builtin.zig_backend == .stage2_wasm) return error.SkipZigTest; // TODO + if (builtin.zig_backend == .stage2_x86_64) return error.SkipZigTest; // TODO + if (builtin.zig_backend == .stage2_arm) return error.SkipZigTest; // TODO + if (builtin.zig_backend == .stage2_aarch64) return error.SkipZigTest; // TODO + if (builtin.zig_backend == .stage2_sparc64) return error.SkipZigTest; // TODO + if (builtin.zig_backend == .stage2_spirv64) return error.SkipZigTest; // TODO + + const S = struct { + fn doTheTest() !void { + var a: struct { x: u32 } = .{ .x = 123 }; + var b: struct { x: u32 } align(1) = .{ .x = 456 }; + var c: struct { x: u32 } align(64) = .{ .x = 789 }; + + const ap = &a.x; + const bp = &b.x; + const cp = &c.x; + + comptime assert(@TypeOf(ap) == *u32); + comptime assert(@TypeOf(bp) == *align(1) u32); + comptime assert(@TypeOf(cp) == *u32); // undefined layout, cannot inherit larger alignment + + try expectEqual(@as(u32, 123), ap.*); + try expectEqual(@as(u32, 456), bp.*); + try expectEqual(@as(u32, 789), cp.*); + } + }; + + try S.doTheTest(); + try comptime S.doTheTest(); +} + +test "extern struct field pointer has correct alignment" { + if (builtin.zig_backend == .stage2_c) return error.SkipZigTest; // TODO + if (builtin.zig_backend == .stage2_wasm) return error.SkipZigTest; // TODO + if (builtin.zig_backend == .stage2_x86_64) return error.SkipZigTest; // TODO + if (builtin.zig_backend == .stage2_arm) return error.SkipZigTest; // TODO + if (builtin.zig_backend == .stage2_aarch64) return error.SkipZigTest; // TODO + if (builtin.zig_backend == .stage2_sparc64) return error.SkipZigTest; // TODO + if (builtin.zig_backend == .stage2_spirv64) return error.SkipZigTest; // TODO + + const S = struct { + fn doTheTest() !void { + var a: extern struct { x: u32, y: u32 } = .{ .x = 1, .y = 2 }; + var b: extern struct { x: u32, y: u32 } align(1) = .{ .x = 3, .y = 4 }; + var c: extern struct { x: u32, y: u32 } align(64) = .{ .x = 5, .y = 6 }; + + const axp = &a.x; + const bxp = &b.x; + const cxp = &c.x; + const ayp = &a.y; + const byp = &b.y; + const cyp = &c.y; + + comptime assert(@TypeOf(axp) == *u32); + comptime assert(@TypeOf(bxp) == *align(1) u32); + comptime assert(@TypeOf(cxp) == *align(64) u32); // first field, inherits larger alignment + comptime assert(@TypeOf(ayp) == *u32); + comptime assert(@TypeOf(byp) == *align(1) u32); + comptime assert(@TypeOf(cyp) == *u32); + + try expectEqual(@as(u32, 1), axp.*); + try expectEqual(@as(u32, 3), bxp.*); + try expectEqual(@as(u32, 5), cxp.*); + + try expectEqual(@as(u32, 2), ayp.*); + try expectEqual(@as(u32, 4), byp.*); + try expectEqual(@as(u32, 6), cyp.*); + } + }; + + try S.doTheTest(); + try comptime S.doTheTest(); +} From 402b958e19e2ae584903fde9576f89370558558c Mon Sep 17 00:00:00 2001 From: mlugg Date: Sun, 25 Jun 2023 12:31:22 +0100 Subject: [PATCH 3/3] std.pdb: fix incorrect field alignment in ptrCast --- lib/std/pdb.zig | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/std/pdb.zig b/lib/std/pdb.zig index 4d71ce2103..0f77279677 100644 --- a/lib/std/pdb.zig +++ b/lib/std/pdb.zig @@ -776,8 +776,8 @@ pub const Pdb = struct { } else 0; const found_line_index = start_line_index + line_entry_idx * @sizeOf(LineNumberEntry); - const line_num_entry = @as(*align(1) LineNumberEntry, @ptrCast(&subsect_info[found_line_index])); - const flags = @as(*LineNumberEntry.Flags, @ptrCast(&line_num_entry.Flags)); + const line_num_entry: *align(1) LineNumberEntry = @ptrCast(&subsect_info[found_line_index]); + const flags: *align(1) LineNumberEntry.Flags = @ptrCast(&line_num_entry.Flags); return debug.LineInfo{ .file_name = source_file_name,