From c86ba0f9d0d6d1421c93fa09a5172c796110258b Mon Sep 17 00:00:00 2001 From: Xavier Bouchoux Date: Sun, 8 Oct 2023 11:36:53 +0200 Subject: [PATCH 1/6] test: add behaviour test for casting accross 32-bits boundary --- test/behavior/cast_int.zig | 93 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 93 insertions(+) diff --git a/test/behavior/cast_int.zig b/test/behavior/cast_int.zig index 989f3d3aa1..62238bdaec 100644 --- a/test/behavior/cast_int.zig +++ b/test/behavior/cast_int.zig @@ -1,6 +1,7 @@ const builtin = @import("builtin"); const std = @import("std"); const expect = std.testing.expect; +const expectEqual = std.testing.expectEqual; const maxInt = std.math.maxInt; test "@intCast i32 to u7" { @@ -28,3 +29,95 @@ test "coerce i8 to i32 and @intCast back" { var y2: i8 = -5; try expect(y2 == @as(i8, @intCast(x2))); } + +test "coerce non byte-sized integers accross 32bits boundary" { + if (builtin.zig_backend == .stage2_wasm) return error.SkipZigTest; // TODO + { + var v: u21 = 6417; + const a: u32 = v; + const b: u64 = v; + const c: u64 = a; + var w: u64 = 0x1234567812345678; + const d: u21 = @truncate(w); + const e: u60 = d; + try expectEqual(@as(u32, 6417), a); + try expectEqual(@as(u64, 6417), b); + try expectEqual(@as(u64, 6417), c); + try expectEqual(@as(u21, 0x145678), d); + try expectEqual(@as(u60, 0x145678), e); + } + + { + var v: u10 = 234; + const a: u32 = v; + const b: u64 = v; + const c: u64 = a; + var w: u64 = 0x1234567812345678; + const d: u10 = @truncate(w); + const e: u60 = d; + try expectEqual(@as(u32, 234), a); + try expectEqual(@as(u64, 234), b); + try expectEqual(@as(u64, 234), c); + try expectEqual(@as(u21, 0x278), d); + try expectEqual(@as(u60, 0x278), e); + } + { + var v: u7 = 11; + const a: u32 = v; + const b: u64 = v; + const c: u64 = a; + var w: u64 = 0x1234567812345678; + const d: u7 = @truncate(w); + const e: u60 = d; + try expectEqual(@as(u32, 11), a); + try expectEqual(@as(u64, 11), b); + try expectEqual(@as(u64, 11), c); + try expectEqual(@as(u21, 0x78), d); + try expectEqual(@as(u60, 0x78), e); + } + + { + var v: i21 = -6417; + const a: i32 = v; + const b: i64 = v; + const c: i64 = a; + var w: i64 = -12345; + const d: i21 = @intCast(w); + const e: i60 = d; + try expectEqual(@as(i32, -6417), a); + try expectEqual(@as(i64, -6417), b); + try expectEqual(@as(i64, -6417), c); + try expectEqual(@as(i21, -12345), d); + try expectEqual(@as(i60, -12345), e); + } + + { + var v: i10 = -234; + const a: i32 = v; + const b: i64 = v; + const c: i64 = a; + var w: i64 = -456; + const d: i10 = @intCast(w); + const e: i60 = d; + try expectEqual(@as(i32, -234), a); + try expectEqual(@as(i64, -234), b); + try expectEqual(@as(i64, -234), c); + try expectEqual(@as(i10, -456), d); + try expectEqual(@as(i60, -456), e); + } + { + var v: i7 = -11; + const a: i32 = v; + const b: i64 = v; + const c: i64 = a; + var w: i64 = -42; + const d: i7 = @intCast(w); + const e: i60 = d; + try expectEqual(@as(i32, -11), a); + try expectEqual(@as(i64, -11), b); + try expectEqual(@as(i64, -11), c); + try expectEqual(@as(i7, -42), d); + try expectEqual(@as(i60, -42), e); + } +} + From 85315bb535a98cb08b4eb8bad6254ef3f5d9714f Mon Sep 17 00:00:00 2001 From: Xavier Bouchoux Date: Sun, 8 Oct 2023 11:10:27 +0200 Subject: [PATCH 2/6] codegen/wasm: fix intcast accross 32-bits boundary --- src/arch/wasm/CodeGen.zig | 16 +++++++++++++--- test/behavior/cast_int.zig | 1 - 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/src/arch/wasm/CodeGen.zig b/src/arch/wasm/CodeGen.zig index 49754f03f6..f382a7770c 100644 --- a/src/arch/wasm/CodeGen.zig +++ b/src/arch/wasm/CodeGen.zig @@ -4353,9 +4353,21 @@ fn intcast(func: *CodeGen, operand: WValue, given: Type, wanted: Type) InnerErro if (op_bits > 32 and op_bits <= 64 and wanted_bits == 32) { try func.emitWValue(operand); try func.addTag(.i32_wrap_i64); + if (given.isSignedInt(mod) and wanted_bitsize < 32) + return func.wrapOperand(.{ .stack = {} }, wanted) + else + return WValue{ .stack = {} }; } else if (op_bits == 32 and wanted_bits > 32 and wanted_bits <= 64) { - try func.emitWValue(operand); + const operand32 = if (given_bitsize < 32 and wanted.isSignedInt(mod)) + try func.signExtendInt(operand, given) + else + operand; + try func.emitWValue(operand32); try func.addTag(if (wanted.isSignedInt(mod)) .i64_extend_i32_s else .i64_extend_i32_u); + if (given.isSignedInt(mod) and wanted_bitsize < 64) + return func.wrapOperand(.{ .stack = {} }, wanted) + else + return WValue{ .stack = {} }; } else if (wanted_bits == 128) { // for 128bit integers we store the integer in the virtual stack, rather than a local const stack_ptr = try func.allocStack(wanted); @@ -4381,8 +4393,6 @@ fn intcast(func: *CodeGen, operand: WValue, given: Type, wanted: Type) InnerErro } return stack_ptr; } else return func.load(operand, wanted, 0); - - return WValue{ .stack = {} }; } fn airIsNull(func: *CodeGen, inst: Air.Inst.Index, opcode: wasm.Opcode, op_kind: enum { value, ptr }) InnerError!void { diff --git a/test/behavior/cast_int.zig b/test/behavior/cast_int.zig index 62238bdaec..400fbd0462 100644 --- a/test/behavior/cast_int.zig +++ b/test/behavior/cast_int.zig @@ -31,7 +31,6 @@ test "coerce i8 to i32 and @intCast back" { } test "coerce non byte-sized integers accross 32bits boundary" { - if (builtin.zig_backend == .stage2_wasm) return error.SkipZigTest; // TODO { var v: u21 = 6417; const a: u32 = v; From 6bb10c72712377fb38764a2df0c7abfebcebf3ad Mon Sep 17 00:00:00 2001 From: Jacob Young Date: Sat, 29 Jul 2023 02:07:52 -0400 Subject: [PATCH 3/6] llvm: fix load of packed struct that was initialized through pointers --- src/codegen/llvm.zig | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/src/codegen/llvm.zig b/src/codegen/llvm.zig index 4355ac1191..148d83bdef 100644 --- a/src/codegen/llvm.zig +++ b/src/codegen/llvm.zig @@ -10378,7 +10378,21 @@ pub const FuncGen = struct { if (isByRef(elem_ty, mod)) { return self.loadByRef(ptr, elem_ty, ptr_alignment, access_kind); } - return self.wip.load(access_kind, try o.lowerType(elem_ty), ptr, ptr_alignment, ""); + const llvm_elem_ty = try o.lowerType(elem_ty); + const llvm_load_ty = if (elem_ty.isAbiInt(mod)) + try o.builder.intType(@intCast(elem_ty.abiSize(mod) * 8)) + else + llvm_elem_ty; + const loaded = try self.wip.load(access_kind, llvm_load_ty, ptr, ptr_alignment, ""); + const shifted = if (llvm_elem_ty != llvm_load_ty and o.target.cpu.arch.endian() == .Big) + try self.wip.bin(.lshr, loaded, try o.builder.intValue( + llvm_load_ty, + (elem_ty.abiSize(mod) - (std.math.divCeil(u64, elem_ty.bitSize(mod), 8) catch + unreachable)) * 8, + ), "") + else + loaded; + return self.wip.conv(.unneeded, shifted, llvm_elem_ty, ""); } const containing_int_ty = try o.builder.intType(@intCast(info.packed_offset.host_size * 8)); From 370662c565ba541157e3c69d12625c28787d642e Mon Sep 17 00:00:00 2001 From: Xavier Bouchoux Date: Sun, 8 Oct 2023 11:35:33 +0200 Subject: [PATCH 4/6] codegen/llvm: truncate padding bits when loading a non-byte-sized value --- src/codegen/llvm.zig | 57 +++++++++++------- test/behavior/cast_int.zig | 103 ++++++++++++++++++++++++++++++++ test/behavior/packed-struct.zig | 2 +- 3 files changed, 139 insertions(+), 23 deletions(-) diff --git a/src/codegen/llvm.zig b/src/codegen/llvm.zig index 148d83bdef..85ac706d7c 100644 --- a/src/codegen/llvm.zig +++ b/src/codegen/llvm.zig @@ -6179,7 +6179,6 @@ pub const FuncGen = struct { const elem_alignment = elem_ty.abiAlignment(mod).toLlvm(); return self.loadByRef(elem_ptr, elem_ty, elem_alignment, .normal); } else { - const elem_llvm_ty = try o.lowerType(elem_ty); if (Air.refToIndex(bin_op.lhs)) |lhs_index| { if (self.air.instructions.items(.tag)[lhs_index] == .load) { const load_data = self.air.instructions.items(.data)[lhs_index]; @@ -6201,7 +6200,7 @@ pub const FuncGen = struct { &indices, "", ); - return self.wip.load(.normal, elem_llvm_ty, gep, .default, ""); + return self.loadTruncate(.normal, elem_ty, gep, .default); }, else => {}, } @@ -6210,7 +6209,7 @@ pub const FuncGen = struct { } const elem_ptr = try self.wip.gep(.inbounds, array_llvm_ty, array_llvm_val, &indices, ""); - return self.wip.load(.normal, elem_llvm_ty, elem_ptr, .default, ""); + return self.loadTruncate(.normal, elem_ty, elem_ptr, .default); } } @@ -6378,13 +6377,12 @@ pub const FuncGen = struct { const payload_index = @intFromBool(layout.tag_align.compare(.gte, layout.payload_align)); const field_ptr = try self.wip.gepStruct(union_llvm_ty, struct_llvm_val, payload_index, ""); - const llvm_field_ty = try o.lowerType(field_ty); const payload_alignment = layout.payload_align.toLlvm(); if (isByRef(field_ty, mod)) { if (canElideLoad(self, body_tail)) return field_ptr; return self.loadByRef(field_ptr, field_ty, payload_alignment, .normal); } else { - return self.wip.load(.normal, llvm_field_ty, field_ptr, payload_alignment, ""); + return self.loadTruncate(.normal, field_ty, field_ptr, payload_alignment); } }, else => unreachable, @@ -10219,8 +10217,7 @@ pub const FuncGen = struct { return fg.loadByRef(payload_ptr, payload_ty, payload_alignment, .normal); } - const payload_llvm_ty = try o.lowerType(payload_ty); - return fg.wip.load(.normal, payload_llvm_ty, payload_ptr, payload_alignment, ""); + return fg.loadTruncate(.normal, payload_ty, payload_ptr, payload_alignment); } assert(!isByRef(payload_ty, mod)); @@ -10321,6 +10318,36 @@ pub const FuncGen = struct { } } + /// Load a value and, if needed, mask out padding bits for non byte-sized integer values. + fn loadTruncate( + fg: *FuncGen, + access_kind: Builder.MemoryAccessKind, + payload_ty: Type, + payload_ptr: Builder.Value, + payload_alignment: Builder.Alignment, + ) !Builder.Value { + // from https://llvm.org/docs/LangRef.html#load-instruction : + // "When loading a value of a type like i20 with a size that is not an integral number of bytes, the result is undefined if the value was not originally written using a store of the same type. " + // => so load the byte aligned value and trunc the unwanted bits. + + const o = fg.dg.object; + const mod = o.module; + const payload_llvm_ty = try o.lowerType(payload_ty); + const load_llvm_ty = if (payload_ty.isAbiInt(mod)) + try o.builder.intType(@intCast(payload_ty.abiSize(mod) * 8)) + else + payload_llvm_ty; + const loaded = try fg.wip.load(access_kind, load_llvm_ty, payload_ptr, payload_alignment, ""); + const shifted = if (payload_llvm_ty != load_llvm_ty and o.target.cpu.arch.endian() == .Big) + try fg.wip.bin(.lshr, loaded, try o.builder.intValue( + load_llvm_ty, + (payload_ty.abiSize(mod) - (std.math.divCeil(u64, payload_ty.bitSize(mod), 8) catch unreachable)) * 8, + ), "") + else + loaded; + return fg.wip.conv(.unneeded, shifted, payload_llvm_ty, ""); + } + /// Load a by-ref type by constructing a new alloca and performing a memcpy. fn loadByRef( fg: *FuncGen, @@ -10378,21 +10405,7 @@ pub const FuncGen = struct { if (isByRef(elem_ty, mod)) { return self.loadByRef(ptr, elem_ty, ptr_alignment, access_kind); } - const llvm_elem_ty = try o.lowerType(elem_ty); - const llvm_load_ty = if (elem_ty.isAbiInt(mod)) - try o.builder.intType(@intCast(elem_ty.abiSize(mod) * 8)) - else - llvm_elem_ty; - const loaded = try self.wip.load(access_kind, llvm_load_ty, ptr, ptr_alignment, ""); - const shifted = if (llvm_elem_ty != llvm_load_ty and o.target.cpu.arch.endian() == .Big) - try self.wip.bin(.lshr, loaded, try o.builder.intValue( - llvm_load_ty, - (elem_ty.abiSize(mod) - (std.math.divCeil(u64, elem_ty.bitSize(mod), 8) catch - unreachable)) * 8, - ), "") - else - loaded; - return self.wip.conv(.unneeded, shifted, llvm_elem_ty, ""); + return self.loadTruncate(access_kind, elem_ty, ptr, ptr_alignment); } const containing_int_ty = try o.builder.intType(@intCast(info.packed_offset.host_size * 8)); diff --git a/test/behavior/cast_int.zig b/test/behavior/cast_int.zig index 400fbd0462..aacb06b40d 100644 --- a/test/behavior/cast_int.zig +++ b/test/behavior/cast_int.zig @@ -120,3 +120,106 @@ test "coerce non byte-sized integers accross 32bits boundary" { } } +const Piece = packed struct { + color: Color, + type: Type, + + const Type = enum { KING, QUEEN, BISHOP, KNIGHT, ROOK, PAWN }; + const Color = enum { WHITE, BLACK }; + + fn charToPiece(c: u8) !@This() { + return .{ + .type = try charToPieceType(c), + .color = if (std.ascii.isUpper(c)) Color.WHITE else Color.BLACK, + }; + } + + fn charToPieceType(c: u8) !Type { + return switch (std.ascii.toLower(c)) { + 'p' => .PAWN, + 'k' => .KING, + 'q' => .QUEEN, + 'b' => .BISHOP, + 'n' => .KNIGHT, + 'r' => .ROOK, + else => error.UnexpectedCharError, + }; + } +}; + +test "load non byte-sized optional value" { + // Originally reported at https://github.com/ziglang/zig/issues/14200 + // note: this bug is triggered by the == operator, expectEqual will hide it + var opt: ?Piece = try Piece.charToPiece('p'); + try expect(opt.?.type == .PAWN); + try expect(opt.?.color == .BLACK); + + var p: Piece = undefined; + @as(*u8, @ptrCast(&p)).* = 0b11111011; + try expect(p.type == .PAWN); + try expect(p.color == .BLACK); +} + +test "load non byte-sized value in struct" { + if (builtin.cpu.arch.endian() != .Little) return error.SkipZigTest; // packed struct TODO + + // note: this bug is triggered by the == operator, expectEqual will hide it + // using ptrCast not to depend on unitialised memory state + + var struct0: struct { + p: Piece, + int: u8, + } = undefined; + @as(*u8, @ptrCast(&struct0.p)).* = 0b11111011; + try expect(struct0.p.type == .PAWN); + try expect(struct0.p.color == .BLACK); + + var struct1: packed struct { + p0: Piece, + p1: Piece, + pad: u1, + p2: Piece, + } = undefined; + @as(*u8, @ptrCast(&struct1.p0)).* = 0b11111011; + struct1.p1 = try Piece.charToPiece('p'); + struct1.p2 = try Piece.charToPiece('p'); + try expect(struct1.p0.type == .PAWN); + try expect(struct1.p0.color == .BLACK); + try expect(struct1.p1.type == .PAWN); + try expect(struct1.p1.color == .BLACK); + try expect(struct1.p2.type == .PAWN); + try expect(struct1.p2.color == .BLACK); +} + +test "load non byte-sized value in union" { + 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; + if (builtin.zig_backend == .stage2_x86_64) return error.SkipZigTest; + if (builtin.zig_backend == .stage2_wasm) return error.SkipZigTest; + + // note: this bug is triggered by the == operator, expectEqual will hide it + // using ptrCast not to depend on unitialised memory state + + var union0: packed union { + p: Piece, + int: u8, + } = .{ .int = 0 }; + union0.int = 0b11111011; + try expect(union0.p.type == .PAWN); + try expect(union0.p.color == .BLACK); + + var union1: union { + p: Piece, + int: u8, + } = .{ .p = .{ .color = .WHITE, .type = .KING } }; + @as(*u8, @ptrCast(&union1.p)).* = 0b11111011; + try expect(union1.p.type == .PAWN); + try expect(union1.p.color == .BLACK); + + var pieces: [3]Piece = undefined; + @as(*u8, @ptrCast(&pieces[1])).* = 0b11111011; + try expect(pieces[1].type == .PAWN); + try expect(pieces[1].color == .BLACK); +} diff --git a/test/behavior/packed-struct.zig b/test/behavior/packed-struct.zig index aa91edc4c4..6240814df7 100644 --- a/test/behavior/packed-struct.zig +++ b/test/behavior/packed-struct.zig @@ -991,7 +991,7 @@ test "bitcast back and forth" { test "field access of packed struct smaller than its abi size inside struct initialized with rls" { // Originally reported at https://github.com/ziglang/zig/issues/14200 - if (builtin.zig_backend == .stage2_llvm and builtin.cpu.arch == .arm) return error.SkipZigTest; + const S = struct { ps: packed struct { x: i2, y: i2 }, From 40528b8a9348fc615cc36a85eebd8cc6ee184333 Mon Sep 17 00:00:00 2001 From: Xavier Bouchoux Date: Sun, 8 Oct 2023 11:35:33 +0200 Subject: [PATCH 5/6] codegen/llvm: add workarounds to loadTruncate() for llvm codegen bugs for wasm, as a heuritic, only enable truncation for values smaller than 32bits. -> the bug is no longer triggered in most use cases (or at least the test suite...) as for powerpc, adding a redundant `and mask` produces working code. --- src/codegen/llvm.zig | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/src/codegen/llvm.zig b/src/codegen/llvm.zig index 85ac706d7c..5203965f8e 100644 --- a/src/codegen/llvm.zig +++ b/src/codegen/llvm.zig @@ -10333,8 +10333,20 @@ pub const FuncGen = struct { const o = fg.dg.object; const mod = o.module; const payload_llvm_ty = try o.lowerType(payload_ty); + const abi_size = payload_ty.abiSize(mod); + + // llvm bug workarounds: + const workaround_explicit_mask = o.target.cpu.arch == .powerpc and abi_size >= 4; + const workaround_disable_truncate = o.target.cpu.arch == .wasm32 and abi_size >= 4; + + if (workaround_disable_truncate) { + // see https://github.com/llvm/llvm-project/issues/64222 + // disable the truncation codepath for larger that 32bits value - with this heuristic, the backend passes the test suite. + return try fg.wip.load(access_kind, payload_llvm_ty, payload_ptr, payload_alignment, ""); + } + const load_llvm_ty = if (payload_ty.isAbiInt(mod)) - try o.builder.intType(@intCast(payload_ty.abiSize(mod) * 8)) + try o.builder.intType(@intCast(abi_size * 8)) else payload_llvm_ty; const loaded = try fg.wip.load(access_kind, load_llvm_ty, payload_ptr, payload_alignment, ""); @@ -10345,7 +10357,15 @@ pub const FuncGen = struct { ), "") else loaded; - return fg.wip.conv(.unneeded, shifted, payload_llvm_ty, ""); + + const anded = if (workaround_explicit_mask and payload_llvm_ty != load_llvm_ty) blk: { + // this is rendundant with llvm.trunc. But without it, llvm17 emits invalid code for powerpc. + var mask_val = try o.builder.intConst(payload_llvm_ty, -1); + mask_val = try o.builder.castConst(.zext, mask_val, load_llvm_ty); + break :blk try fg.wip.bin(.@"and", shifted, mask_val.toValue(), ""); + } else shifted; + + return fg.wip.conv(.unneeded, anded, payload_llvm_ty, ""); } /// Load a by-ref type by constructing a new alloca and performing a memcpy. From 703fe0f121df9df44721250528e508762e161780 Mon Sep 17 00:00:00 2001 From: Xavier Bouchoux Date: Sun, 8 Oct 2023 11:35:33 +0200 Subject: [PATCH 6/6] test: add a pair of cases from bug reports --- test/behavior/packed-struct.zig | 29 +++++++++++++++++++++++++++++ test/behavior/packed-union.zig | 28 ++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+) diff --git a/test/behavior/packed-struct.zig b/test/behavior/packed-struct.zig index 6240814df7..de98b3e27d 100644 --- a/test/behavior/packed-struct.zig +++ b/test/behavior/packed-struct.zig @@ -1036,3 +1036,32 @@ test "modify nested packed struct aligned field" { try std.testing.expectEqual(@as(u8, 1), opts.pretty_print.indent); try std.testing.expect(!opts.baz); } + +test "assigning packed struct inside another packed struct" { + // Originally reported at https://github.com/ziglang/zig/issues/9674 + + const S = struct { + const Inner = packed struct { + bits: u3, + more_bits: u6, + }; + + const Outer = packed struct { + padding: u5, + inner: Inner, + }; + fn t(inner: Inner) void { + r.inner = inner; + } + + var mem: Outer = undefined; + var r: *volatile Outer = &mem; + }; + + const val = S.Inner{ .bits = 1, .more_bits = 11 }; + S.mem.padding = 0; + S.t(val); + + try expectEqual(val, S.mem.inner); + try expect(S.mem.padding == 0); +} diff --git a/test/behavior/packed-union.zig b/test/behavior/packed-union.zig index da62acdd04..ab14bf38e7 100644 --- a/test/behavior/packed-union.zig +++ b/test/behavior/packed-union.zig @@ -85,3 +85,31 @@ test "flags in packed union at offset" { try expectEqual(false, test_bits.adv_flags.adv.flags.enable_1); try expectEqual(false, test_bits.adv_flags.adv.flags.enable_2); } + +test "packed union in packed struct" { + // Originally reported at https://github.com/ziglang/zig/issues/16581 + + const ReadRequest = packed struct { key: i32 }; + const RequestType = enum { + read, + insert, + }; + const RequestUnion = packed union { + read: ReadRequest, + }; + + const Request = packed struct { + active_type: RequestType, + request: RequestUnion, + const Self = @This(); + + fn init(read: ReadRequest) Self { + return .{ + .active_type = .read, + .request = RequestUnion{ .read = read }, + }; + } + }; + + try std.testing.expectEqual(RequestType.read, Request.init(.{ .key = 3 }).active_type); +}