From 259f784241fb44e0a1b570daaf31ba2b9f164106 Mon Sep 17 00:00:00 2001 From: Andrew Kelley Date: Tue, 3 May 2022 23:44:32 -0700 Subject: [PATCH] stage2: improve `@sizeOf` and `@alignOf` integers Prior to this commit, the logic for ABI size and ABI alignment for integers was naive and incorrect. This results in wasted hardware as well as undefined behavior in the LLVM backend when we memset an incorrect number of bytes to 0xaa due to disagreeing with LLVM about the ABI size of integers. This commit introduces a "max int align" value which is different per Target. This value is used to derive the ABI size and alignment of all integers. This commit makes an interesting change from stage1, which treats 128-bit integers as 16-bytes aligned for x86_64-linux. stage1 is incorrect. The maximum integer alignment on this system is only 8 bytes. This change breaks the behavior test called "128-bit cmpxchg" because on that target, 128-bit cmpxchg does require a 16-bytes aligned pointer to a 128 bit integer. However, this alignment property does not belong on *all* 128 bit integers - only on the pointer type in the `@cmpxchg` builtin function prototype. The user can then use an alignment override annotation on a 128-bit integer variable or struct field to obtain such a pointer. --- lib/std/target.zig | 77 ++++++++++++++++++++++ src/codegen/llvm.zig | 2 +- src/type.zig | 38 ++++++----- test/behavior/align.zig | 130 ++++++++++++++++++++++++++++++-------- test/behavior/bitcast.zig | 15 +---- test/behavior/struct.zig | 9 +-- 6 files changed, 211 insertions(+), 60 deletions(-) diff --git a/lib/std/target.zig b/lib/std/target.zig index c23a42c9637a63a696357d77f92d68c67bbef52a..777b0f0ec0420840ed1c5f15a562c6b8ad7fc6e7 100644 --- a/lib/std/target.zig +++ b/lib/std/target.zig @@ -1773,6 +1773,83 @@ pub const Target = struct { else => false, }; } + + pub inline fn maxIntAlignment(target: Target) u16 { + return switch (target.cpu.arch) { + .avr => 1, + .msp430 => 2, + .xcore => 4, + + .arm, + .armeb, + .thumb, + .thumbeb, + .x86_64, + .hexagon, + .mips, + .mipsel, + .mips64, + .mips64el, + .powerpc, + .powerpcle, + .powerpc64, + .powerpc64le, + .r600, + .amdgcn, + .riscv32, + .riscv64, + .sparc, + .sparcv9, + .sparcel, + .s390x, + .lanai, + .wasm32, + .wasm64, + => 8, + + .i386 => return switch (target.os.tag) { + .windows => 8, + else => 4, + }, + .aarch64, + .aarch64_be, + .aarch64_32, + .bpfel, + .bpfeb, + .nvptx, + .nvptx64, + => 16, + + // Below this comment are unverified and I have chosen a number + // based on ptrBitWidth. + + .spu_2 => 2, + + .csky, + .arc, + .m68k, + .tce, + .tcele, + .le32, + .amdil, + .hsail, + .spir, + .kalimba, + .renderscript32, + .spirv32, + .shave, + => 4, + + .le64, + .amdil64, + .hsail64, + .spir64, + .renderscript64, + .ve, + .spirv64, + => 8, + }; + } }; test { diff --git a/src/codegen/llvm.zig b/src/codegen/llvm.zig index 6c74fc8223345066960d78976e3cc065f2aef9f0..068067d7654c6b2d2bfdbfd70e44959f0cc495fc 100644 --- a/src/codegen/llvm.zig +++ b/src/codegen/llvm.zig @@ -7583,7 +7583,7 @@ pub const FuncGen = struct { const size_bytes = elem_ty.abiSize(target); _ = self.builder.buildMemCpy( self.builder.buildBitCast(ptr, llvm_ptr_u8, ""), - ptr_ty.ptrAlignment(target), + ptr_alignment, self.builder.buildBitCast(elem, llvm_ptr_u8, ""), elem_ty.abiAlignment(target), self.context.intType(Type.usize.intInfo(target).bits).constInt(size_bytes, .False), diff --git a/src/type.zig b/src/type.zig index c96c512d9a09075f74d2402871feef112e75a0c6..44432b95f6d69921df46a0eedf2320f0271c18b9 100644 --- a/src/type.zig +++ b/src/type.zig @@ -2788,11 +2788,6 @@ pub const Type = extern union { return AbiAlignmentAdvanced{ .scalar = target_util.defaultFunctionAlignment(target) }; }, - .i16, .u16 => return AbiAlignmentAdvanced{ .scalar = 2 }, - .i32, .u32 => return AbiAlignmentAdvanced{ .scalar = 4 }, - .i64, .u64 => return AbiAlignmentAdvanced{ .scalar = 8 }, - .u128, .i128 => return AbiAlignmentAdvanced{ .scalar = 16 }, - .isize, .usize, .single_const_pointer_to_comptime_int, @@ -2865,14 +2860,15 @@ pub const Type = extern union { // ABI alignment of vectors? .vector => return AbiAlignmentAdvanced{ .scalar = 16 }, + .i16, .u16 => return AbiAlignmentAdvanced{ .scalar = intAbiAlignment(16, target) }, + .i32, .u32 => return AbiAlignmentAdvanced{ .scalar = intAbiAlignment(32, target) }, + .i64, .u64 => return AbiAlignmentAdvanced{ .scalar = intAbiAlignment(64, target) }, + .u128, .i128 => return AbiAlignmentAdvanced{ .scalar = intAbiAlignment(128, target) }, + .int_signed, .int_unsigned => { const bits: u16 = ty.cast(Payload.Bits).?.data; if (bits == 0) return AbiAlignmentAdvanced{ .scalar = 0 }; - if (bits <= 8) return AbiAlignmentAdvanced{ .scalar = 1 }; - if (bits <= 16) return AbiAlignmentAdvanced{ .scalar = 2 }; - if (bits <= 32) return AbiAlignmentAdvanced{ .scalar = 4 }; - if (bits <= 64) return AbiAlignmentAdvanced{ .scalar = 8 }; - return AbiAlignmentAdvanced{ .scalar = 16 }; + return AbiAlignmentAdvanced{ .scalar = intAbiAlignment(bits, target) }; }, .optional => { @@ -3113,10 +3109,6 @@ pub const Type = extern union { assert(elem_size >= payload.elem_type.abiAlignment(target)); return (payload.len + 1) * elem_size; }, - .i16, .u16 => return 2, - .i32, .u32 => return 4, - .i64, .u64 => return 8, - .u128, .i128 => return 16, .isize, .usize, @@ -3189,10 +3181,14 @@ pub const Type = extern union { .error_set_merged, => return 2, // TODO revisit this when we have the concept of the error tag type + .i16, .u16 => return intAbiSize(16, target), + .i32, .u32 => return intAbiSize(32, target), + .i64, .u64 => return intAbiSize(64, target), + .u128, .i128 => return intAbiSize(128, target), .int_signed, .int_unsigned => { const bits: u16 = self.cast(Payload.Bits).?.data; if (bits == 0) return 0; - return std.math.ceilPowerOfTwoPromote(u16, (bits + 7) / 8); + return intAbiSize(bits, target); }, .optional => { @@ -3234,6 +3230,18 @@ pub const Type = extern union { }; } + fn intAbiSize(bits: u16, target: Target) u64 { + const alignment = intAbiAlignment(bits, target); + return std.mem.alignForwardGeneric(u64, (bits + 7) / 8, alignment); + } + + fn intAbiAlignment(bits: u16, target: Target) u32 { + return @minimum( + std.math.ceilPowerOfTwoPromote(u16, (bits + 7) / 8), + target.maxIntAlignment(), + ); + } + /// Asserts the type has the bit size already resolved. pub fn bitSize(ty: Type, target: Target) u64 { return switch (ty.tag()) { diff --git a/test/behavior/align.zig b/test/behavior/align.zig index 3a35d1fcca5b932533a3159802e46c2d9de9e229..9d7ca9958a8332efc9cde22b05af95f80a67ad41 100644 --- a/test/behavior/align.zig +++ b/test/behavior/align.zig @@ -47,41 +47,121 @@ fn expects4(x: *align(4) u32) void { x.* += 1; } -test "alignment of structs" { +test "alignment of struct with pointer has same alignment as usize" { try expect(@alignOf(struct { a: i32, b: *i32, }) == @alignOf(usize)); } -test "alignment of >= 128-bit integer type" { - try expect(@alignOf(u128) == 16); - try expect(@alignOf(u129) == 16); -} - -test "alignment of struct with 128-bit field" { - try expect(@alignOf(struct { +test "alignment and size of structs with 128-bit fields" { + const A = struct { x: u128, - }) == 16); - - comptime { - try expect(@alignOf(struct { - x: u128, - }) == 16); - } -} - -test "size of extern struct with 128-bit field" { - try expect(@sizeOf(extern struct { + }; + const B = extern struct { x: u128, y: u8, - }) == 32); + }; + const expected = switch (builtin.cpu.arch) { + .arm, + .armeb, + .thumb, + .thumbeb, + .x86_64, + .hexagon, + .mips, + .mipsel, + .mips64, + .mips64el, + .powerpc, + .powerpcle, + .powerpc64, + .powerpc64le, + .r600, + .amdgcn, + .riscv32, + .riscv64, + .sparc, + .sparcv9, + .sparcel, + .s390x, + .lanai, + .wasm32, + .wasm64, + => .{ + .a_align = 8, + .a_size = 16, + .b_align = 8, + .b_size = 24, + + .u128_align = 8, + .u128_size = 16, + .u129_align = 8, + .u129_size = 24, + }, + + .i386 => switch (builtin.os.tag) { + .windows => .{ + .a_align = 8, + .a_size = 16, + + .b_align = 8, + .b_size = 24, + + .u128_align = 8, + .u128_size = 16, + .u129_align = 8, + .u129_size = 24, + }, + else => .{ + .a_align = 4, + .a_size = 16, + + .b_align = 4, + .b_size = 20, + + .u128_align = 4, + .u128_size = 16, + .u129_align = 4, + .u129_size = 20, + }, + }, + + .aarch64, + .aarch64_be, + .aarch64_32, + .bpfel, + .bpfeb, + .nvptx, + .nvptx64, + => .{ + .a_align = 16, + .a_size = 16, + + .b_align = 16, + .b_size = 32, + + .u128_align = 16, + .u128_size = 16, + .u129_align = 16, + .u129_size = 32, + }, + + else => return error.SkipZigTest, + }; comptime { - try expect(@sizeOf(extern struct { - x: u128, - y: u8, - }) == 32); + std.debug.assert(@alignOf(A) == expected.a_align); + std.debug.assert(@sizeOf(A) == expected.a_size); + + std.debug.assert(@alignOf(B) == expected.b_align); + std.debug.assert(@sizeOf(B) == expected.b_size); + + std.debug.assert(@alignOf(u128) == expected.u128_align); + std.debug.assert(@sizeOf(u128) == expected.u128_size); + + std.debug.assert(@alignOf(u129) == expected.u129_align); + std.debug.assert(@sizeOf(u129) == expected.u129_size); } } @@ -328,7 +408,6 @@ test "read 128-bit field from default aligned struct in stack memory" { .nevermind = 1, .badguy = 12, }; - try expect((@ptrToInt(&default_aligned.badguy) % 16) == 0); try expect(12 == default_aligned.badguy); } @@ -345,7 +424,6 @@ test "read 128-bit field from default aligned struct in global memory" { if (builtin.zig_backend == .stage2_x86_64) return error.SkipZigTest; if (builtin.zig_backend == .stage2_arm) return error.SkipZigTest; - try expect((@ptrToInt(&default_aligned_global.badguy) % 16) == 0); try expect(12 == default_aligned_global.badguy); } diff --git a/test/behavior/bitcast.zig b/test/behavior/bitcast.zig index 6be3cc15c43cae25105aaf55d767d2b7ef3d9d52..e74a4c44f48d9e7dc8d07a04c49fd7e8246887bf 100644 --- a/test/behavior/bitcast.zig +++ b/test/behavior/bitcast.zig @@ -138,18 +138,9 @@ test "@bitCast packed structs at runtime and comptime" { fn doTheTest() !void { var full = Full{ .number = 0x1234 }; var two_halves = @bitCast(Divided, full); - switch (native_endian) { - .Big => { - try expect(two_halves.half1 == 0x12); - try expect(two_halves.quarter3 == 0x3); - try expect(two_halves.quarter4 == 0x4); - }, - .Little => { - try expect(two_halves.half1 == 0x34); - try expect(two_halves.quarter3 == 0x2); - try expect(two_halves.quarter4 == 0x1); - }, - } + try expect(two_halves.half1 == 0x34); + try expect(two_halves.quarter3 == 0x2); + try expect(two_halves.quarter4 == 0x1); } }; try S.doTheTest(); diff --git a/test/behavior/struct.zig b/test/behavior/struct.zig index b05126db622595c72d22c0f41946ca09ede46d59..ed75268f7d5fdea30662d31e575b2249d0e48b33 100644 --- a/test/behavior/struct.zig +++ b/test/behavior/struct.zig @@ -499,17 +499,14 @@ const Bitfields = packed struct { f7: u8, }; -test "native bit field understands endianness" { - if (builtin.zig_backend == .stage2_aarch64) return error.SkipZigTest; +test "packed struct fields are ordered from LSB to MSB" { + if (builtin.zig_backend == .stage2_aarch64) return error.SkipZigTest; // TODO if (builtin.zig_backend == .stage2_wasm) return error.SkipZigTest; // TODO if (builtin.zig_backend == .stage2_c) return error.SkipZigTest; // TODO if (builtin.zig_backend == .stage2_arm) return error.SkipZigTest; // TODO if (builtin.zig_backend == .stage2_x86_64) return error.SkipZigTest; // TODO - var all: u64 = if (native_endian != .Little) - 0x1111222233445677 - else - 0x7765443322221111; + var all: u64 = 0x7765443322221111; var bytes: [8]u8 = undefined; @memcpy(&bytes, @ptrCast([*]u8, &all), 8); var bitfields = @ptrCast(*Bitfields, &bytes).*; -- 2.54.0