From 50a8124f45cd0994b52af3fd166dd3bda48f4b99 Mon Sep 17 00:00:00 2001 From: LemonBoy Date: Sun, 25 Apr 2021 16:40:41 +0200 Subject: [PATCH 1/5] stage1: Change how the Frame alignment is computed The code would previously assume every function would start at addresses being multiples of 16, this is not true beside some specific cases. Moreover LLVM picks different alignment values depending on whether it's trying to generate dense or fast code. Let's use the minimum guaranteed alignment as base value, computed according to how big the opcodes are. The alignment of function pointers is always 1, a safe value that won't cause any error at runtime. Note that this was already the case before this commit, here we're making this choice explicit. Let the 'alignment' field for TypeInfo of fn types reflect the ABI alignment used by the compiler, make this field behave similarly to the 'alignment' one for pointers. --- src/stage1/analyze.cpp | 15 ++++++-------- src/stage1/ir.cpp | 4 ++-- src/stage1/target.cpp | 33 +++++++++++++++++++++++++++++- src/stage1/target.hpp | 1 + test/stage1/behavior/type_info.zig | 2 +- 5 files changed, 42 insertions(+), 13 deletions(-) diff --git a/src/stage1/analyze.cpp b/src/stage1/analyze.cpp index 21ad55b8f7a8d1223a3ea58be45000a9d712e15a..e1e0c496f6c6bd8ba78391d3203816a8d4ec85c4 100644 --- a/src/stage1/analyze.cpp +++ b/src/stage1/analyze.cpp @@ -4770,10 +4770,10 @@ Error type_is_nonnull_ptr2(CodeGen *g, ZigType *type, bool *result) { } static uint32_t get_async_frame_align_bytes(CodeGen *g) { - uint32_t a = g->pointer_size_bytes * 2; - // promises have at least alignment 8 so that we can have 3 extra bits when doing atomicrmw - if (a < 8) a = 8; - return a; + // Due to how the frame structure is built the minimum alignment is the one + // of a usize (or pointer). + // label (grep this): [fn_frame_struct_layout] + return max(g->builtin_types.entry_usize->abi_align, target_fn_align(g->zig_target)); } uint32_t get_ptr_align(CodeGen *g, ZigType *type) { @@ -4789,11 +4789,8 @@ uint32_t get_ptr_align(CodeGen *g, ZigType *type) { return (ptr_type->data.pointer.explicit_alignment == 0) ? get_abi_alignment(g, ptr_type->data.pointer.child_type) : ptr_type->data.pointer.explicit_alignment; } else if (ptr_type->id == ZigTypeIdFn) { - // I tried making this use LLVMABIAlignmentOfType but it trips this assertion in LLVM: - // "Cannot getTypeInfo() on a type that is unsized!" - // when getting the alignment of `?fn() callconv(.C) void`. - // See http://lists.llvm.org/pipermail/llvm-dev/2018-September/126142.html - return (ptr_type->data.fn.fn_type_id.alignment == 0) ? 1 : ptr_type->data.fn.fn_type_id.alignment; + return (ptr_type->data.fn.fn_type_id.alignment == 0) ? + target_fn_ptr_align(g->zig_target) : ptr_type->data.fn.fn_type_id.alignment; } else if (ptr_type->id == ZigTypeIdAnyFrame) { return get_async_frame_align_bytes(g); } else { diff --git a/src/stage1/ir.cpp b/src/stage1/ir.cpp index c59f63399c23f8ce62b8d4f2432b8de9845f9f37..176d50dc72c01f77c1ebb884203a84a46d24da09 100644 --- a/src/stage1/ir.cpp +++ b/src/stage1/ir.cpp @@ -26079,11 +26079,11 @@ static Error ir_make_type_info_value(IrAnalyze *ira, IrInst* source_instr, ZigTy fields[0]->special = ConstValSpecialStatic; fields[0]->type = get_builtin_type(ira->codegen, "CallingConvention"); bigint_init_unsigned(&fields[0]->data.x_enum_tag, type_entry->data.fn.fn_type_id.cc); - // alignment: u29 + // alignment: comptime_int ensure_field_index(result->type, "alignment", 1); fields[1]->special = ConstValSpecialStatic; fields[1]->type = ira->codegen->builtin_types.entry_num_lit_int; - bigint_init_unsigned(&fields[1]->data.x_bigint, type_entry->data.fn.fn_type_id.alignment); + bigint_init_unsigned(&fields[1]->data.x_bigint, get_ptr_align(ira->codegen, type_entry)); // is_generic: bool ensure_field_index(result->type, "is_generic", 2); bool is_generic = type_entry->data.fn.is_generic; diff --git a/src/stage1/target.cpp b/src/stage1/target.cpp index 028f6e5fa87eaef75aeca78b2f5340633edf5e55..6aa3cfcbd0960675c53815ba01424c5b24a0a757 100644 --- a/src/stage1/target.cpp +++ b/src/stage1/target.cpp @@ -1253,6 +1253,37 @@ bool target_is_ppc(const ZigTarget *target) { target->arch == ZigLLVM_ppc64le; } +// Returns the minimum alignment for every function pointer on the given +// architecture. +unsigned target_fn_ptr_align(const ZigTarget *target) { + // TODO This is a pessimization but is always correct. + return 1; +} + +// Returns the minimum alignment for every function on the given architecture. unsigned target_fn_align(const ZigTarget *target) { - return 16; + switch (target->arch) { + case ZigLLVM_riscv32: + case ZigLLVM_riscv64: + // TODO If the C extension is not present the value is 4. + return 2; + case ZigLLVM_ppc: + case ZigLLVM_ppcle: + case ZigLLVM_ppc64: + case ZigLLVM_ppc64le: + case ZigLLVM_aarch64: + case ZigLLVM_aarch64_be: + case ZigLLVM_aarch64_32: + case ZigLLVM_sparc: + case ZigLLVM_sparcel: + case ZigLLVM_sparcv9: + case ZigLLVM_mips: + case ZigLLVM_mipsel: + case ZigLLVM_mips64: + case ZigLLVM_mips64el: + return 4; + + default: + return 1; + } } diff --git a/src/stage1/target.hpp b/src/stage1/target.hpp index 099c3c1e7898526b57754bed8af112c368fc793f..362d63eb32c81018729a765253e6aac913828099 100644 --- a/src/stage1/target.hpp +++ b/src/stage1/target.hpp @@ -98,6 +98,7 @@ size_t target_libc_count(void); void target_libc_enum(size_t index, ZigTarget *out_target); bool target_libc_needs_crti_crtn(const ZigTarget *target); +unsigned target_fn_ptr_align(const ZigTarget *target); unsigned target_fn_align(const ZigTarget *target); #endif diff --git a/test/stage1/behavior/type_info.zig b/test/stage1/behavior/type_info.zig index 6dec7ca4d2fde133046a25468bc3d96278e5195e..f944b7904c70381f4785bc4f3c725ca61ef9b0c0 100644 --- a/test/stage1/behavior/type_info.zig +++ b/test/stage1/behavior/type_info.zig @@ -306,7 +306,7 @@ test "type info: function type info" { fn testFunction() void { const fn_info = @typeInfo(@TypeOf(foo)); expect(fn_info == .Fn); - expect(fn_info.Fn.alignment == 0); + expect(fn_info.Fn.alignment > 0); expect(fn_info.Fn.calling_convention == .C); expect(!fn_info.Fn.is_generic); expect(fn_info.Fn.args.len == 2); -- 2.54.0 From 19cec0db1e8a9fefd8295c81edb03d631c624e62 Mon Sep 17 00:00:00 2001 From: LemonBoy Date: Sun, 25 Apr 2021 16:49:21 +0200 Subject: [PATCH 2/5] std: Make met.alignment work on more types Make it return the correct value for function types and optional pointers. --- lib/std/meta.zig | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/lib/std/meta.zig b/lib/std/meta.zig index c6f800ca2cc314af834d3799ea7402c6f6d0101a..9bb02013e6c48a81e58d74ae4166e867274ae440 100644 --- a/lib/std/meta.zig +++ b/lib/std/meta.zig @@ -117,10 +117,21 @@ test "std.meta.bitCount" { testing.expect(bitCount(f32) == 32); } +/// Returns the alignment of type T. +/// Note that if T is a pointer or function type the result is different than +/// the one returned by @alignOf(T). +/// If T is a pointer type the alignment of the type it points to is returned. +/// If T is a function type the alignment a target-dependent value is returned. pub fn alignment(comptime T: type) comptime_int { - //@alignOf works on non-pointer types - const P = if (comptime trait.is(.Pointer)(T)) T else *T; - return @typeInfo(P).Pointer.alignment; + return switch (@typeInfo(T)) { + .Optional => |info| switch (@typeInfo(info.child)) { + .Pointer, .Fn => alignment(info.child), + else => @alignOf(T), + }, + .Pointer => |info| info.alignment, + .Fn => |info| info.alignment, + else => @alignOf(T), + }; } test "std.meta.alignment" { @@ -129,6 +140,8 @@ test "std.meta.alignment" { testing.expect(alignment(*align(2) u8) == 2); testing.expect(alignment([]align(1) u8) == 1); testing.expect(alignment([]align(2) u8) == 2); + testing.expect(alignment(fn () void) > 0); + testing.expect(alignment(fn () align(128) void) == 128); } pub fn Child(comptime T: type) type { -- 2.54.0 From ae15022406e5d59787195130cfd5261c6336f41c Mon Sep 17 00:00:00 2001 From: LemonBoy Date: Sun, 25 Apr 2021 16:50:41 +0200 Subject: [PATCH 3/5] translate-c: Fix casting of function pointers The @ptrCast(X, @alignCast(@alignOf(T), Y)) pattern is only correct if T is not a function type or a pointer, in that case the @alignOf refers to the pointer itself and not to the pointee type. --- src/translate_c.zig | 2 +- src/translate_c/ast.zig | 12 +++++++++++- test/translate_c.zig | 18 +++++++++--------- 3 files changed, 21 insertions(+), 11 deletions(-) diff --git a/src/translate_c.zig b/src/translate_c.zig index 60b3961e6ef99ca33e2044d8da88b82a5405329b..0a722ab3b4ef758a99b62b91a0140cd5f4d92a50 100644 --- a/src/translate_c.zig +++ b/src/translate_c.zig @@ -3539,7 +3539,7 @@ fn transCPtrCast( expr else blk: { const child_type_node = try transQualType(c, scope, child_type, loc); - const alignof = try Tag.alignof.create(c.arena, child_type_node); + const alignof = try Tag.std_meta_alignment.create(c.arena, child_type_node); const align_cast = try Tag.align_cast.create(c.arena, .{ .lhs = alignof, .rhs = expr }); break :blk align_cast; }; diff --git a/src/translate_c/ast.zig b/src/translate_c/ast.zig index 61d28bb22daa43d70b8993b51e379eb3c0a5fca3..2e75bf11822c0ad68caf33590a16237ab8d0dfdc 100644 --- a/src/translate_c/ast.zig +++ b/src/translate_c/ast.zig @@ -120,8 +120,11 @@ pub const Node = extern union { std_math_Log2Int, /// @intCast(lhs, rhs) int_cast, - /// @rem(lhs, rhs) + /// @import("std").meta.promoteIntLiteral(value, type, radix) std_meta_promoteIntLiteral, + /// @import("std").meta.alignment(value) + std_meta_alignment, + /// @rem(lhs, rhs) rem, /// @divTrunc(lhs, rhs) div_trunc, @@ -260,6 +263,7 @@ pub const Node = extern union { .switch_else, .block_single, .std_meta_sizeof, + .std_meta_alignment, .bool_to_int, .sizeof, .alignof, @@ -876,6 +880,11 @@ fn renderNode(c: *Context, node: Node) Allocator.Error!NodeIndex { const import_node = try renderStdImport(c, "meta", "promoteIntLiteral"); return renderCall(c, import_node, &.{ payload.type, payload.value, payload.radix }); }, + .std_meta_alignment => { + const payload = node.castTag(.std_meta_alignment).?.data; + const import_node = try renderStdImport(c, "meta", "alignment"); + return renderCall(c, import_node, &.{payload}); + }, .std_meta_sizeof => { const payload = node.castTag(.std_meta_sizeof).?.data; const import_node = try renderStdImport(c, "meta", "sizeof"); @@ -2144,6 +2153,7 @@ fn renderNodeGrouped(c: *Context, node: Node) !NodeIndex { .typeof, .typeinfo, .std_meta_sizeof, + .std_meta_alignment, .std_meta_cast, .std_meta_promoteIntLiteral, .std_meta_vector, diff --git a/test/translate_c.zig b/test/translate_c.zig index 846eec0d624e6f05e199ccbe524444ce8181c40e..4021210b2972d85eaf09253c300e3cfedcc5c9bf 100644 --- a/test/translate_c.zig +++ b/test/translate_c.zig @@ -1363,7 +1363,7 @@ pub fn addCases(cases: *tests.TranslateCContext) void { , &[_][]const u8{ \\pub export fn ptrcast() [*c]f32 { \\ var a: [*c]c_int = undefined; - \\ return @ptrCast([*c]f32, @alignCast(@alignOf(f32), a)); + \\ return @ptrCast([*c]f32, @alignCast(@import("std").meta.alignment(f32), a)); \\} }); @@ -1387,16 +1387,16 @@ pub fn addCases(cases: *tests.TranslateCContext) void { \\pub export fn test_ptr_cast() void { \\ var p: ?*c_void = undefined; \\ { - \\ var to_char: [*c]u8 = @ptrCast([*c]u8, @alignCast(@alignOf(u8), p)); - \\ var to_short: [*c]c_short = @ptrCast([*c]c_short, @alignCast(@alignOf(c_short), p)); - \\ var to_int: [*c]c_int = @ptrCast([*c]c_int, @alignCast(@alignOf(c_int), p)); - \\ var to_longlong: [*c]c_longlong = @ptrCast([*c]c_longlong, @alignCast(@alignOf(c_longlong), p)); + \\ var to_char: [*c]u8 = @ptrCast([*c]u8, @alignCast(@import("std").meta.alignment(u8), p)); + \\ var to_short: [*c]c_short = @ptrCast([*c]c_short, @alignCast(@import("std").meta.alignment(c_short), p)); + \\ var to_int: [*c]c_int = @ptrCast([*c]c_int, @alignCast(@import("std").meta.alignment(c_int), p)); + \\ var to_longlong: [*c]c_longlong = @ptrCast([*c]c_longlong, @alignCast(@import("std").meta.alignment(c_longlong), p)); \\ } \\ { - \\ var to_char: [*c]u8 = @ptrCast([*c]u8, @alignCast(@alignOf(u8), p)); - \\ var to_short: [*c]c_short = @ptrCast([*c]c_short, @alignCast(@alignOf(c_short), p)); - \\ var to_int: [*c]c_int = @ptrCast([*c]c_int, @alignCast(@alignOf(c_int), p)); - \\ var to_longlong: [*c]c_longlong = @ptrCast([*c]c_longlong, @alignCast(@alignOf(c_longlong), p)); + \\ var to_char: [*c]u8 = @ptrCast([*c]u8, @alignCast(@import("std").meta.alignment(u8), p)); + \\ var to_short: [*c]c_short = @ptrCast([*c]c_short, @alignCast(@import("std").meta.alignment(c_short), p)); + \\ var to_int: [*c]c_int = @ptrCast([*c]c_int, @alignCast(@import("std").meta.alignment(c_int), p)); + \\ var to_longlong: [*c]c_longlong = @ptrCast([*c]c_longlong, @alignCast(@import("std").meta.alignment(c_longlong), p)); \\ } \\} }); -- 2.54.0 From 0340ed3a9e3fd62e5f8021f2755a26fb3497123e Mon Sep 17 00:00:00 2001 From: LemonBoy Date: Sun, 25 Apr 2021 17:02:48 +0200 Subject: [PATCH 4/5] build: Re-add test-translate-c and test-run-translated-c --- ci/drone/linux_script_test | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/ci/drone/linux_script_test b/ci/drone/linux_script_test index d59a2008ba16c463f7e3b09a6d46c8435adfd937..a8e497a619850981b929871fe92938d48a6764cb 100755 --- a/ci/drone/linux_script_test +++ b/ci/drone/linux_script_test @@ -22,7 +22,9 @@ case "$1" in steps="\ test-compiler-rt \ test-minilibc \ - test-compare-output" + test-compare-output \ + test-translate-c \ + test-run-translated-c" ;; '') echo "error: expecting test group argument" -- 2.54.0 From 82f1d592fae021fcfc737e3cb6c107b325fcf1ee Mon Sep 17 00:00:00 2001 From: LemonBoy Date: Sun, 25 Apr 2021 19:42:59 +0200 Subject: [PATCH 5/5] stage1: Use correct alignment for asyncCall frame --- src/stage1/analyze.cpp | 2 +- src/stage1/analyze.hpp | 1 + src/stage1/ir.cpp | 8 ++++++-- test/compile_errors.zig | 4 +++- 4 files changed, 11 insertions(+), 4 deletions(-) diff --git a/src/stage1/analyze.cpp b/src/stage1/analyze.cpp index e1e0c496f6c6bd8ba78391d3203816a8d4ec85c4..f76ea85ca90af88c4d7824377a7544ef29afd822 100644 --- a/src/stage1/analyze.cpp +++ b/src/stage1/analyze.cpp @@ -4769,7 +4769,7 @@ Error type_is_nonnull_ptr2(CodeGen *g, ZigType *type, bool *result) { return ErrorNone; } -static uint32_t get_async_frame_align_bytes(CodeGen *g) { +uint32_t get_async_frame_align_bytes(CodeGen *g) { // Due to how the frame structure is built the minimum alignment is the one // of a usize (or pointer). // label (grep this): [fn_frame_struct_layout] diff --git a/src/stage1/analyze.hpp b/src/stage1/analyze.hpp index cea48b893c2cb7434cd4c601590575153dfe2444..2815274f63bc3f3d5a1033254086df722b472442 100644 --- a/src/stage1/analyze.hpp +++ b/src/stage1/analyze.hpp @@ -47,6 +47,7 @@ ZigType *get_test_fn_type(CodeGen *g); ZigType *get_any_frame_type(CodeGen *g, ZigType *result_type); bool handle_is_ptr(CodeGen *g, ZigType *type_entry); Error emit_error_unless_callconv_allowed_for_target(CodeGen *g, AstNode *source_node, CallingConvention cc); +uint32_t get_async_frame_align_bytes(CodeGen *g); bool type_has_bits(CodeGen *g, ZigType *type_entry); Error type_has_bits2(CodeGen *g, ZigType *type_entry, bool *result); diff --git a/src/stage1/ir.cpp b/src/stage1/ir.cpp index 176d50dc72c01f77c1ebb884203a84a46d24da09..7edfe37c92786250815059b09cb6ce951e171fdb 100644 --- a/src/stage1/ir.cpp +++ b/src/stage1/ir.cpp @@ -20659,8 +20659,12 @@ static IrInstGen *analyze_casted_new_stack(IrAnalyze *ira, IrInst* source_instr, get_fn_frame_type(ira->codegen, fn_entry), false); return ir_implicit_cast(ira, new_stack, needed_frame_type); } else { + // XXX The stack alignment is hardcoded to 16 here and in + // std.Target.stack_align. + const uint32_t required_align = is_async_call_builtin ? + get_async_frame_align_bytes(ira->codegen) : 16; ZigType *u8_ptr = get_pointer_to_type_extra(ira->codegen, ira->codegen->builtin_types.entry_u8, - false, false, PtrLenUnknown, target_fn_align(ira->codegen->zig_target), 0, 0, false); + false, false, PtrLenUnknown, required_align, 0, 0, false); ZigType *u8_slice = get_slice_type(ira->codegen, u8_ptr); ira->codegen->need_frame_size_prefix_data = true; return ir_implicit_cast2(ira, new_stack_src, new_stack, u8_slice); @@ -30095,7 +30099,7 @@ static IrInstGen *ir_align_cast(IrAnalyze *ira, IrInstGen *target, uint32_t alig fn_type_id.alignment = align_bytes; result_type = get_fn_type(ira->codegen, &fn_type_id); } else if (target_type->id == ZigTypeIdAnyFrame) { - if (align_bytes >= target_fn_align(ira->codegen->zig_target)) { + if (align_bytes >= get_async_frame_align_bytes(ira->codegen)) { result_type = target_type; } else { ir_add_error(ira, &target->base, buf_sprintf("sub-aligned anyframe not allowed")); diff --git a/test/compile_errors.zig b/test/compile_errors.zig index bfa9b592b438194da499a4d70a7992f652615d66..5b36027248094ecbd397c11ae0e6aacfa6f94c97 100644 --- a/test/compile_errors.zig +++ b/test/compile_errors.zig @@ -2136,7 +2136,9 @@ pub fn addCases(cases: *tests.CompileErrorContext) void { \\} \\fn func() callconv(.Async) void {} , &[_][]const u8{ - "tmp.zig:4:21: error: expected type '[]align(16) u8', found '*[64]u8'", + // Split the check in two as the alignment value is target dependent. + "tmp.zig:4:21: error: expected type '[]align(", + ") u8', found '*[64]u8'", }); cases.add("atomic orderings of fence Acquire or stricter", -- 2.54.0