From 6748bb4f131c848012131b900bebf35d4799b63e Mon Sep 17 00:00:00 2001 From: Xavier Bouchoux Date: Sat, 29 Jul 2023 20:08:08 +0200 Subject: [PATCH 1/2] add nested packed struct/union behavior test. --- test/behavior/packed-struct.zig | 89 +++++++++++++++++++++++++++++++-- test/behavior/packed-union.zig | 47 +++++++++++++++++ 2 files changed, 131 insertions(+), 5 deletions(-) diff --git a/test/behavior/packed-struct.zig b/test/behavior/packed-struct.zig index e1520e0c40ca..c83e33985c6e 100644 --- a/test/behavior/packed-struct.zig +++ b/test/behavior/packed-struct.zig @@ -354,6 +354,44 @@ test "byte-aligned field pointer offsets" { try comptime S.doTheTest(); } +test "nested packed struct field pointers" { + if (builtin.zig_backend == .stage2_aarch64) return error.SkipZigTest; + if (builtin.zig_backend == .stage2_arm) return error.SkipZigTest; + if (builtin.zig_backend == .stage2_sparc64) return error.SkipZigTest; // TODO + if (builtin.zig_backend == .stage2_spirv64) return error.SkipZigTest; + if (builtin.zig_backend == .stage2_c) return error.SkipZigTest; // ubsan unaligned pointer access + if (native_endian != .Little) return error.SkipZigTest; // Byte aligned packed struct field pointers have not been implemented yet + + const S2 = packed struct { + base: u8, + p0: packed struct { + a: u4, + b: u4, + c: u8, + }, + bit: u1, + p1: packed struct { + a: u7, + b: u8, + }, + + var s: @This() = .{ .base = 1, .p0 = .{ .a = 2, .b = 3, .c = 4 }, .bit = 0, .p1 = .{ .a = 5, .b = 6 } }; + }; + + const ptr_base = &S2.s.base; + const ptr_p0_a = &S2.s.p0.a; + const ptr_p0_b = &S2.s.p0.b; + const ptr_p0_c = &S2.s.p0.c; + const ptr_p1_a = &S2.s.p1.a; + //const ptr_p1_b = &S2.s.p1.b; + try expectEqual(@as(u8, 1), ptr_base.*); + try expectEqual(@as(u4, 2), ptr_p0_a.*); + try expectEqual(@as(u4, 3), ptr_p0_b.*); + try expectEqual(@as(u8, 4), ptr_p0_c.*); + try expectEqual(@as(u7, 5), ptr_p1_a.*); + // try expectEqual(@as(u8,6), ptr_p1_b.*); https://github.com/ziglang/zig/issues/16748 +} + test "load pointer from packed struct" { if (builtin.zig_backend == .stage2_aarch64) return error.SkipZigTest; if (builtin.zig_backend == .stage2_arm) return error.SkipZigTest; @@ -382,11 +420,8 @@ test "@intFromPtr on a packed struct field" { if (builtin.zig_backend == .stage2_spirv64) return error.SkipZigTest; const S = struct { - const P = packed struct { - x: u8, - y: u8, - z: u32, - }; + const P = packed struct { x: u8, y: u8, z: u32 }; + var p0: P = P{ .x = 1, .y = 2, @@ -394,6 +429,50 @@ test "@intFromPtr on a packed struct field" { }; }; try expect(@intFromPtr(&S.p0.z) - @intFromPtr(&S.p0.x) == 2); + + const S2 = packed struct { + base: u8, + p0: packed struct { + a: u4, + b: u4, + c: u8, + }, + bit: u1, + p1: packed struct { + a: u7, + b: u8, + }, + + var s: @This() = .{ .base = 1, .p0 = .{ .a = 2, .b = 3, .c = 4 }, .bit = 0, .p1 = .{ .a = 5, .b = 6 } }; + }; + try expect(@intFromPtr(&S2.s.base) - @intFromPtr(&S2.s) == 0); + try expect(@intFromPtr(&S2.s.p0.a) - @intFromPtr(&S2.s) == 1); + try expect(@intFromPtr(&S2.s.p0.b) - @intFromPtr(&S2.s) == 1); + try expect(@intFromPtr(&S2.s.p0.c) - @intFromPtr(&S2.s) == 2); + try expect(@intFromPtr(&S2.s.bit) - @intFromPtr(&S2.s) == 0); + try expect(@intFromPtr(&S2.s.p1.a) - @intFromPtr(&S2.s) == 0); + // try expect(@intFromPtr(&S2.s.p1.b) - @intFromPtr(&S2.s) == 4); https://github.com/ziglang/zig/issues/16748 +} + +test "packed struct fields modification" { + const Small = packed struct { + val: u8 = 0, + lo: u4 = 0, + hi: u4 = 0, + + var p: @This() = undefined; + }; + Small.p = .{ + .val = 0x12, + .lo = 3, + .hi = 4, + }; + try expect(@as(u16, @bitCast(Small.p)) == 0x4312); + + Small.p.val -= Small.p.lo; + Small.p.val += Small.p.hi; + Small.p.hi -= Small.p.lo; + try expect(@as(u16, @bitCast(Small.p)) == 0x1313); } test "optional pointer in packed struct" { diff --git a/test/behavior/packed-union.zig b/test/behavior/packed-union.zig index ceb22e1ba9db..da62acdd045d 100644 --- a/test/behavior/packed-union.zig +++ b/test/behavior/packed-union.zig @@ -38,3 +38,50 @@ test "flags in packed union" { try expectEqual(true, test_bits.enable_1); try expectEqual(false, test_bits.other_flags.flags.enable_1); } + +test "flags in packed union at offset" { + if (builtin.zig_backend == .stage2_aarch64) return error.SkipZigTest; + if (builtin.zig_backend == .stage2_arm) return error.SkipZigTest; + if (builtin.zig_backend == .stage2_sparc64) return error.SkipZigTest; + if (builtin.zig_backend == .stage2_spirv64) return error.SkipZigTest; + + const FlagBits = packed union { + base_flags: packed union { + flags: packed struct(u4) { + enable_1: bool = true, + enable_2: bool = false, + enable_3: bool = false, + enable_4: bool = false, + }, + bits: u4, + }, + adv_flags: packed struct(u12) { + pad: u8 = 0, + adv: packed union { + flags: packed struct(u4) { + enable_1: bool = true, + enable_2: bool = false, + enable_3: bool = false, + enable_4: bool = false, + }, + bits: u4, + }, + }, + }; + var test_bits: FlagBits = .{ .adv_flags = .{ .adv = .{ .flags = .{} } } }; + + try expectEqual(@as(u8, 0), test_bits.adv_flags.pad); + try expectEqual(true, test_bits.adv_flags.adv.flags.enable_1); + try expectEqual(false, test_bits.adv_flags.adv.flags.enable_2); + + test_bits.adv_flags.adv.flags.enable_1 = false; + test_bits.adv_flags.adv.flags.enable_2 = true; + try expectEqual(@as(u8, 0), test_bits.adv_flags.pad); + try expectEqual(false, test_bits.adv_flags.adv.flags.enable_1); + try expectEqual(true, test_bits.adv_flags.adv.flags.enable_2); + + test_bits.adv_flags.adv.bits = 12; + try expectEqual(@as(u8, 0), test_bits.adv_flags.pad); + try expectEqual(false, test_bits.adv_flags.adv.flags.enable_1); + try expectEqual(false, test_bits.adv_flags.adv.flags.enable_2); +} From b316d458d5f0e92db71369ca0c8cc622d2c4e209 Mon Sep 17 00:00:00 2001 From: Xavier Bouchoux Date: Mon, 7 Aug 2023 08:18:33 +0000 Subject: [PATCH 2/2] codegen/llvm: fix wrong field offset in packed structs Fields smaller that the abisize, even when aligned to a byte boundary, are accessed via a packed_offset decorated ptr. This was not always considered by the code, and the offset was applied once to the pointer (as if it would be used as an undecorated ptr), and again when dereferencing it (by the packed_offset decoration) resolves https://github.com/ziglang/zig/issues/16615 and probaly https://github.com/ziglang/zig/issues/16621 --- src/arch/wasm/CodeGen.zig | 5 ++++- src/codegen.zig | 11 ++++------- src/codegen/llvm.zig | 15 +++++++++------ src/type.zig | 36 ++++++++++++++++++++++++++++++++++++ 4 files changed, 53 insertions(+), 14 deletions(-) diff --git a/src/arch/wasm/CodeGen.zig b/src/arch/wasm/CodeGen.zig index 04ac8c41315b..adeb7e34ace1 100644 --- a/src/arch/wasm/CodeGen.zig +++ b/src/arch/wasm/CodeGen.zig @@ -3015,7 +3015,10 @@ fn lowerParentPtr(func: *CodeGen, ptr_val: Value, offset: u32) InnerError!WValue const field_offset = switch (parent_ty.zigTypeTag(mod)) { .Struct => switch (parent_ty.containerLayout(mod)) { - .Packed => parent_ty.packedStructFieldByteOffset(@as(usize, @intCast(field.index)), mod), + .Packed => if (parent_ty.packedStructFieldByteAligned(@intCast(field.index), mod)) + parent_ty.packedStructFieldByteOffset(@as(usize, @intCast(field.index)), mod) + else + 0, else => parent_ty.structFieldOffset(@as(usize, @intCast(field.index)), mod), }, .Union => switch (parent_ty.containerLayout(mod)) { diff --git a/src/codegen.zig b/src/codegen.zig index a2d0f225bce9..d02366386198 100644 --- a/src/codegen.zig +++ b/src/codegen.zig @@ -696,13 +696,10 @@ fn lowerParentPtr( mod, )), .Packed => if (mod.typeToStruct(base_type.toType())) |struct_obj| - math.divExact(u16, struct_obj.packedFieldBitOffset( - mod, - @intCast(field.index), - ), 8) catch |err| switch (err) { - error.UnexpectedRemainder => 0, - error.DivisionByZero => unreachable, - } + if (base_type.toType().packedStructFieldByteAligned(@intCast(field.index), mod)) + @divExact(struct_obj.packedFieldBitOffset(mod, @intCast(field.index)), 8) + else + 0 else 0, }, diff --git a/src/codegen/llvm.zig b/src/codegen/llvm.zig index 7a46472c62bd..d50107578152 100644 --- a/src/codegen/llvm.zig +++ b/src/codegen/llvm.zig @@ -3702,7 +3702,7 @@ pub const Object = struct { .opt_payload, .elem, .field, - => try o.lowerParentPtr(val, ty.ptrInfo(mod).packed_offset.bit_offset % 8 == 0), + => try o.lowerParentPtr(val), .comptime_field => unreachable, }; switch (ptr.len) { @@ -4137,14 +4137,14 @@ pub const Object = struct { return o.lowerDeclRefValue(ptr_ty, decl_index); } - fn lowerParentPtr(o: *Object, ptr_val: Value, byte_aligned: bool) Allocator.Error!Builder.Constant { + fn lowerParentPtr(o: *Object, ptr_val: Value) Allocator.Error!Builder.Constant { const mod = o.module; return switch (mod.intern_pool.indexToKey(ptr_val.toIntern()).ptr.addr) { .decl => |decl| o.lowerParentPtrDecl(decl), .mut_decl => |mut_decl| o.lowerParentPtrDecl(mut_decl.decl), .int => |int| try o.lowerIntAsPtr(int), .eu_payload => |eu_ptr| { - const parent_ptr = try o.lowerParentPtr(eu_ptr.toValue(), true); + const parent_ptr = try o.lowerParentPtr(eu_ptr.toValue()); const eu_ty = mod.intern_pool.typeOf(eu_ptr).toType().childType(mod); const payload_ty = eu_ty.errorUnionPayload(mod); @@ -4161,7 +4161,7 @@ pub const Object = struct { }); }, .opt_payload => |opt_ptr| { - const parent_ptr = try o.lowerParentPtr(opt_ptr.toValue(), true); + const parent_ptr = try o.lowerParentPtr(opt_ptr.toValue()); const opt_ty = mod.intern_pool.typeOf(opt_ptr).toType().childType(mod); const payload_ty = opt_ty.optionalChild(mod); @@ -4179,7 +4179,7 @@ pub const Object = struct { }, .comptime_field => unreachable, .elem => |elem_ptr| { - const parent_ptr = try o.lowerParentPtr(elem_ptr.base.toValue(), true); + const parent_ptr = try o.lowerParentPtr(elem_ptr.base.toValue()); const elem_ty = mod.intern_pool.typeOf(elem_ptr.base).toType().elemType2(mod); return o.builder.gepConst(.inbounds, try o.lowerType(elem_ty), parent_ptr, null, &.{ @@ -4187,7 +4187,7 @@ pub const Object = struct { }); }, .field => |field_ptr| { - const parent_ptr = try o.lowerParentPtr(field_ptr.base.toValue(), byte_aligned); + const parent_ptr = try o.lowerParentPtr(field_ptr.base.toValue()); const parent_ty = mod.intern_pool.typeOf(field_ptr.base).toType().childType(mod); const field_index: u32 = @intCast(field_ptr.index); @@ -4213,7 +4213,10 @@ pub const Object = struct { }, .Struct => { if (parent_ty.containerLayout(mod) == .Packed) { + const ptr_ty = mod.intern_pool.typeOf(ptr_val.ip_index).toType(); + const byte_aligned = ptr_ty.isPackedStructFieldPtrByteAligned(mod); if (!byte_aligned) return parent_ptr; + const llvm_usize = try o.lowerType(Type.usize); const base_addr = try o.builder.castConst(.ptrtoint, parent_ptr, llvm_usize); diff --git a/src/type.zig b/src/type.zig index 32d207bfbf68..bbfd75ef142d 100644 --- a/src/type.zig +++ b/src/type.zig @@ -3113,6 +3113,42 @@ pub const Type = struct { }; } + // value can be accessed through a normal ptr (without packed_offset related masking) + pub fn isPackedStructFieldPtrByteAligned(ptr_ty: Type, mod: *Module) bool { + const ptr_info = ptr_ty.ptrInfo(mod); + if (ptr_info.packed_offset.host_size == 0) return true; + const elem_ty = ptr_ty.childType(mod); + const elem_abi_size = elem_ty.abiSize(mod); + const elem_size_bits = elem_ty.bitSize(mod); + const bit_offset = ptr_info.packed_offset.bit_offset; + + return (bit_offset % 8 == 0 and elem_abi_size * 8 == elem_size_bits); + } + + pub fn packedStructFieldByteAligned(ty: Type, field_index: usize, mod: *Module) bool { + const struct_type = mod.intern_pool.indexToKey(ty.toIntern()).struct_type; + const struct_obj = mod.structPtrUnwrap(struct_type.index).?; + assert(struct_obj.layout == .Packed); + comptime assert(Type.packed_struct_layout_version == 2); + + var bit_offset: u16 = undefined; + var elem_abi_size: u64 = 0; + var elem_size_bits: u16 = undefined; + var running_bits: u16 = 0; + for (struct_obj.fields.values(), 0..) |f, i| { + if (!f.ty.hasRuntimeBits(mod)) continue; + + const field_bits = @as(u16, @intCast(f.ty.bitSize(mod))); + if (i == field_index) { + bit_offset = running_bits; + elem_size_bits = field_bits; + elem_abi_size = f.ty.abiSize(mod); + } + running_bits += field_bits; + } + return (bit_offset % 8 == 0 and elem_abi_size * 8 == elem_size_bits); + } + pub fn packedStructFieldByteOffset(ty: Type, field_index: usize, mod: *Module) u32 { const struct_type = mod.intern_pool.indexToKey(ty.toIntern()).struct_type; const struct_obj = mod.structPtrUnwrap(struct_type.index).?;