From 070e3ea37dd51c2a2080941a056c455f70232148 Mon Sep 17 00:00:00 2001 From: Andrew Kelley Date: Thu, 14 Jul 2022 19:30:56 -0700 Subject: [PATCH 1/5] LLVM: insert debug logging when LLVM ABI size is wrong --- src/codegen/llvm.zig | 18 ++++++++++++++++++ src/codegen/llvm/bindings.zig | 6 ++++++ 2 files changed, 24 insertions(+) diff --git a/src/codegen/llvm.zig b/src/codegen/llvm.zig index fe35620d38..714ac16c54 100644 --- a/src/codegen/llvm.zig +++ b/src/codegen/llvm.zig @@ -2424,6 +2424,24 @@ pub const DeclGen = struct { } fn lowerType(dg: *DeclGen, t: Type) Allocator.Error!*const llvm.Type { + const llvm_ty = try lowerTypeInner(dg, t); + if (std.debug.runtime_safety) { + if (t.zigTypeTag() != .Opaque and t.hasRuntimeBits() and + !llvm_ty.isOpaqueStruct().toBool()) + { + const zig_size = t.abiSize(dg.module.getTarget()); + const llvm_size = dg.object.target_data.abiSizeOfType(llvm_ty); + if (llvm_size != zig_size) { + log.err("when lowering {}, Zig ABI size = {d} but LLVM ABI size = {d}", .{ + t.fmt(dg.module), zig_size, llvm_size, + }); + } + } + } + return llvm_ty; + } + + fn lowerTypeInner(dg: *DeclGen, t: Type) Allocator.Error!*const llvm.Type { const gpa = dg.gpa; const target = dg.module.getTarget(); switch (t.zigTypeTag()) { diff --git a/src/codegen/llvm/bindings.zig b/src/codegen/llvm/bindings.zig index 89905e4a29..3279f052b2 100644 --- a/src/codegen/llvm/bindings.zig +++ b/src/codegen/llvm/bindings.zig @@ -301,6 +301,9 @@ pub const Type = opaque { pub const countStructElementTypes = LLVMCountStructElementTypes; extern fn LLVMCountStructElementTypes(StructTy: *const Type) c_uint; + + pub const isOpaqueStruct = LLVMIsOpaqueStruct; + extern fn LLVMIsOpaqueStruct(StructTy: *const Type) Bool; }; pub const Module = opaque { @@ -1032,6 +1035,9 @@ pub const TargetData = opaque { pub const abiAlignmentOfType = LLVMABIAlignmentOfType; extern fn LLVMABIAlignmentOfType(TD: *const TargetData, Ty: *const Type) c_uint; + + pub const abiSizeOfType = LLVMABISizeOfType; + extern fn LLVMABISizeOfType(TD: *const TargetData, Ty: *const Type) c_ulonglong; }; pub const CodeModel = enum(c_int) { From 9c136be78fcc44d4dd4902605de6af14358c67a3 Mon Sep 17 00:00:00 2001 From: Andrew Kelley Date: Thu, 14 Jul 2022 20:05:55 -0700 Subject: [PATCH 2/5] LLVM: add padding to optional types when lowering If the LLVM ABI size does not agree with the Zig ABI size. --- src/codegen/llvm.zig | 42 +++++++++++++++++++++++++++++++----------- 1 file changed, 31 insertions(+), 11 deletions(-) diff --git a/src/codegen/llvm.zig b/src/codegen/llvm.zig index 714ac16c54..fd26a603dd 100644 --- a/src/codegen/llvm.zig +++ b/src/codegen/llvm.zig @@ -2537,10 +2537,22 @@ pub const DeclGen = struct { return payload_llvm_ty; } + comptime assert(optional_layout_version == 1); const fields: [2]*const llvm.Type = .{ - payload_llvm_ty, dg.context.intType(1), + payload_llvm_ty, + dg.context.intType(1), }; - return dg.context.structType(&fields, fields.len, .False); + const llvm_ty = dg.context.structType(&fields, fields.len, .False); + const llvm_size = dg.object.target_data.abiSizeOfType(llvm_ty); + const zig_size = t.abiSize(target); + const padding = @intCast(c_uint, zig_size - llvm_size); + if (padding == 0) return llvm_ty; + const padded_fields: [3]*const llvm.Type = .{ + payload_llvm_ty, + dg.context.intType(1), + dg.context.intType(8).arrayType(padding), + }; + return dg.context.structType(&padded_fields, padded_fields.len, .False); }, .ErrorUnion => { const payload_ty = t.errorUnionPayload(); @@ -3101,6 +3113,7 @@ pub const DeclGen = struct { else => unreachable, }, .Optional => { + comptime assert(optional_layout_version == 1); var buf: Type.Payload.ElemType = undefined; const payload_ty = tv.ty.optionalChild(&buf); const llvm_i1 = dg.context.intType(1); @@ -3109,25 +3122,30 @@ pub const DeclGen = struct { if (!payload_ty.hasRuntimeBitsIgnoreComptime()) { return non_null_bit; } + const llvm_ty = try dg.lowerType(tv.ty); if (tv.ty.optionalReprIsPayload()) { if (tv.val.castTag(.opt_payload)) |payload| { return dg.lowerValue(.{ .ty = payload_ty, .val = payload.data }); } else if (is_pl) { return dg.lowerValue(.{ .ty = payload_ty, .val = tv.val }); } else { - const llvm_ty = try dg.lowerType(tv.ty); return llvm_ty.constNull(); } } assert(payload_ty.zigTypeTag() != .Fn); - const fields: [2]*const llvm.Value = .{ - try dg.lowerValue(.{ - .ty = payload_ty, - .val = if (tv.val.castTag(.opt_payload)) |pl| pl.data else Value.initTag(.undef), - }), - non_null_bit, - }; - return dg.context.constStruct(&fields, fields.len, .False); + + const llvm_field_count = llvm_ty.countStructElementTypes(); + var fields_buf: [3]*const llvm.Value = undefined; + fields_buf[0] = try dg.lowerValue(.{ + .ty = payload_ty, + .val = if (tv.val.castTag(.opt_payload)) |pl| pl.data else Value.initTag(.undef), + }); + fields_buf[1] = non_null_bit; + if (llvm_field_count > 2) { + assert(llvm_field_count == 3); + fields_buf[2] = llvm_ty.structGetTypeAtIndex(2).getUndef(); + } + return dg.context.constStruct(&fields_buf, llvm_field_count, .False); }, .Fn => { const fn_decl_index = switch (tv.val.tag()) { @@ -5865,6 +5883,7 @@ pub const FuncGen = struct { const ty_op = self.air.instructions.items(.data)[inst].ty_op; const payload_ty = self.air.typeOf(ty_op.operand); const non_null_bit = self.context.intType(1).constAllOnes(); + comptime assert(optional_layout_version == 1); if (!payload_ty.hasRuntimeBitsIgnoreComptime()) return non_null_bit; const operand = try self.resolveInst(ty_op.operand); const optional_ty = self.air.typeOfIndex(inst); @@ -9318,6 +9337,7 @@ fn intrinsicsAllowed(scalar_ty: Type, target: std.Target) bool { /// We can do this because for all types, Zig ABI alignment >= LLVM ABI /// alignment. const struct_layout_version = 2; +const optional_layout_version = 1; /// We use the least significant bit of the pointer address to tell us /// whether the type is fully resolved. Types that are only fwd declared From 8c14d170b579e078ff961973e4d28a4946df36d7 Mon Sep 17 00:00:00 2001 From: Andrew Kelley Date: Thu, 14 Jul 2022 21:22:01 -0700 Subject: [PATCH 3/5] Revert "stage2 llvm: Use unpacked struct for unions and arrays" This reverts commit 2eaef84ebe968224b0cf25206abf12ea1c5e0f5a. Here is a motivating example: ```zig const E = union(enum) { A: [9]u8, B: u64, }; ``` ```llvm %test2.E = type { { i64, [1 x i8] }, i1, [6 x i8] } ``` ``` error(codegen): when lowering test2.E, Zig ABI size = 16 but LLVM ABI size = 24 ``` --- src/codegen/llvm.zig | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/codegen/llvm.zig b/src/codegen/llvm.zig index fd26a603dd..d7bdbd1062 100644 --- a/src/codegen/llvm.zig +++ b/src/codegen/llvm.zig @@ -2734,7 +2734,7 @@ pub const DeclGen = struct { llvm_aligned_field_ty, dg.context.intType(8).arrayType(padding_len), }; - break :t dg.context.structType(&fields, fields.len, .False); + break :t dg.context.structType(&fields, fields.len, .True); }; if (layout.tag_size == 0) { @@ -3050,7 +3050,7 @@ pub const DeclGen = struct { return dg.context.constStruct( llvm_elems.ptr, @intCast(c_uint, llvm_elems.len), - .False, + .True, ); } else { const llvm_elem_ty = try dg.lowerType(elem_ty); @@ -3087,7 +3087,7 @@ pub const DeclGen = struct { return dg.context.constStruct( llvm_elems.ptr, @intCast(c_uint, llvm_elems.len), - .False, + .True, ); } else { const llvm_elem_ty = try dg.lowerType(elem_ty); @@ -3104,7 +3104,7 @@ pub const DeclGen = struct { const llvm_elems: [1]*const llvm.Value = .{sentinel}; const need_unnamed = dg.isUnnamedType(elem_ty, llvm_elems[0]); if (need_unnamed) { - return dg.context.constStruct(&llvm_elems, llvm_elems.len, .False); + return dg.context.constStruct(&llvm_elems, llvm_elems.len, .True); } else { const llvm_elem_ty = try dg.lowerType(elem_ty); return llvm_elem_ty.constArray(&llvm_elems, llvm_elems.len); @@ -3397,7 +3397,7 @@ pub const DeclGen = struct { const fields: [2]*const llvm.Value = .{ field, dg.context.intType(8).arrayType(padding_len).getUndef(), }; - break :p dg.context.constStruct(&fields, fields.len, .False); + break :p dg.context.constStruct(&fields, fields.len, .True); }; if (layout.tag_size == 0) { From 040cb585e88583cba8a15f30eea07bfd2f135515 Mon Sep 17 00:00:00 2001 From: Andrew Kelley Date: Thu, 14 Jul 2022 23:24:57 -0700 Subject: [PATCH 4/5] LLVM: fix ABI size of optional and error union types Previously, the Zig ABI size and LLVM ABI size of these types disagreed sometimes. This code also corrects the logging messages to not trigger LLVM assertions. --- src/codegen/llvm.zig | 106 ++++++++++++++++++++++------------ src/codegen/llvm/bindings.zig | 3 + src/type.zig | 3 + 3 files changed, 75 insertions(+), 37 deletions(-) diff --git a/src/codegen/llvm.zig b/src/codegen/llvm.zig index d7bdbd1062..6608920dfa 100644 --- a/src/codegen/llvm.zig +++ b/src/codegen/llvm.zig @@ -2425,17 +2425,17 @@ pub const DeclGen = struct { fn lowerType(dg: *DeclGen, t: Type) Allocator.Error!*const llvm.Type { const llvm_ty = try lowerTypeInner(dg, t); - if (std.debug.runtime_safety) { - if (t.zigTypeTag() != .Opaque and t.hasRuntimeBits() and - !llvm_ty.isOpaqueStruct().toBool()) - { - const zig_size = t.abiSize(dg.module.getTarget()); - const llvm_size = dg.object.target_data.abiSizeOfType(llvm_ty); - if (llvm_size != zig_size) { - log.err("when lowering {}, Zig ABI size = {d} but LLVM ABI size = {d}", .{ - t.fmt(dg.module), zig_size, llvm_size, - }); - } + if (std.debug.runtime_safety) check: { + if (t.zigTypeTag() == .Opaque) break :check; + if (!t.hasRuntimeBits()) break :check; + if (!llvm_ty.isSized().toBool()) break :check; + + const zig_size = t.abiSize(dg.module.getTarget()); + const llvm_size = dg.object.target_data.abiSizeOfType(llvm_ty); + if (llvm_size != zig_size) { + log.err("when lowering {}, Zig ABI size = {d} but LLVM ABI size = {d}", .{ + t.fmt(dg.module), zig_size, llvm_size, + }); } } return llvm_ty; @@ -2537,22 +2537,18 @@ pub const DeclGen = struct { return payload_llvm_ty; } - comptime assert(optional_layout_version == 1); - const fields: [2]*const llvm.Type = .{ - payload_llvm_ty, - dg.context.intType(1), + comptime assert(optional_layout_version == 2); + var fields_buf: [3]*const llvm.Type = .{ + payload_llvm_ty, dg.context.intType(1), undefined, }; - const llvm_ty = dg.context.structType(&fields, fields.len, .False); - const llvm_size = dg.object.target_data.abiSizeOfType(llvm_ty); - const zig_size = t.abiSize(target); - const padding = @intCast(c_uint, zig_size - llvm_size); - if (padding == 0) return llvm_ty; - const padded_fields: [3]*const llvm.Type = .{ - payload_llvm_ty, - dg.context.intType(1), - dg.context.intType(8).arrayType(padding), - }; - return dg.context.structType(&padded_fields, padded_fields.len, .False); + const offset = child_ty.abiSize(target) + 1; + const abi_size = t.abiSize(target); + const padding = @intCast(c_uint, abi_size - offset); + if (padding == 0) { + return dg.context.structType(&fields_buf, 2, .False); + } + fields_buf[2] = dg.context.intType(8).arrayType(padding); + return dg.context.structType(&fields_buf, 3, .False); }, .ErrorUnion => { const payload_ty = t.errorUnionPayload(); @@ -2564,12 +2560,37 @@ pub const DeclGen = struct { const payload_align = payload_ty.abiAlignment(target); const error_align = Type.anyerror.abiAlignment(target); + + const payload_size = payload_ty.abiSize(target); + const error_size = Type.anyerror.abiSize(target); + + var fields_buf: [3]*const llvm.Type = undefined; if (error_align > payload_align) { - const fields: [2]*const llvm.Type = .{ llvm_error_type, llvm_payload_type }; - return dg.context.structType(&fields, fields.len, .False); + fields_buf[0] = llvm_error_type; + fields_buf[1] = llvm_payload_type; + const payload_end = + std.mem.alignForwardGeneric(u64, error_size, payload_align) + + payload_size; + const abi_size = std.mem.alignForwardGeneric(u64, payload_end, error_align); + const padding = @intCast(c_uint, abi_size - payload_end); + if (padding == 0) { + return dg.context.structType(&fields_buf, 2, .False); + } + fields_buf[2] = dg.context.intType(8).arrayType(padding); + return dg.context.structType(&fields_buf, 3, .False); } else { - const fields: [2]*const llvm.Type = .{ llvm_payload_type, llvm_error_type }; - return dg.context.structType(&fields, fields.len, .False); + fields_buf[0] = llvm_payload_type; + fields_buf[1] = llvm_error_type; + const error_end = + std.mem.alignForwardGeneric(u64, payload_size, error_align) + + error_size; + const abi_size = std.mem.alignForwardGeneric(u64, error_end, payload_align); + const padding = @intCast(c_uint, abi_size - error_end); + if (padding == 0) { + return dg.context.structType(&fields_buf, 2, .False); + } + fields_buf[2] = dg.context.intType(8).arrayType(padding); + return dg.context.structType(&fields_buf, 3, .False); } }, .ErrorSet => return dg.context.intType(16), @@ -3113,7 +3134,7 @@ pub const DeclGen = struct { else => unreachable, }, .Optional => { - comptime assert(optional_layout_version == 1); + comptime assert(optional_layout_version == 2); var buf: Type.Payload.ElemType = undefined; const payload_ty = tv.ty.optionalChild(&buf); const llvm_i1 = dg.context.intType(1); @@ -3191,12 +3212,23 @@ pub const DeclGen = struct { .ty = payload_type, .val = if (tv.val.castTag(.eu_payload)) |pl| pl.data else Value.initTag(.undef), }); + var fields_buf: [3]*const llvm.Value = undefined; + + const llvm_ty = try dg.lowerType(tv.ty); + const llvm_field_count = llvm_ty.countStructElementTypes(); + if (llvm_field_count > 2) { + assert(llvm_field_count == 3); + fields_buf[2] = llvm_ty.structGetTypeAtIndex(2).getUndef(); + } + if (error_align > payload_align) { - const fields: [2]*const llvm.Value = .{ llvm_error_value, llvm_payload_value }; - return dg.context.constStruct(&fields, fields.len, .False); + fields_buf[0] = llvm_error_value; + fields_buf[1] = llvm_payload_value; + return dg.context.constStruct(&fields_buf, llvm_field_count, .False); } else { - const fields: [2]*const llvm.Value = .{ llvm_payload_value, llvm_error_value }; - return dg.context.constStruct(&fields, fields.len, .False); + fields_buf[0] = llvm_payload_value; + fields_buf[1] = llvm_error_value; + return dg.context.constStruct(&fields_buf, llvm_field_count, .False); } }, .Struct => { @@ -5883,7 +5915,7 @@ pub const FuncGen = struct { const ty_op = self.air.instructions.items(.data)[inst].ty_op; const payload_ty = self.air.typeOf(ty_op.operand); const non_null_bit = self.context.intType(1).constAllOnes(); - comptime assert(optional_layout_version == 1); + comptime assert(optional_layout_version == 2); if (!payload_ty.hasRuntimeBitsIgnoreComptime()) return non_null_bit; const operand = try self.resolveInst(ty_op.operand); const optional_ty = self.air.typeOfIndex(inst); @@ -9337,7 +9369,7 @@ fn intrinsicsAllowed(scalar_ty: Type, target: std.Target) bool { /// We can do this because for all types, Zig ABI alignment >= LLVM ABI /// alignment. const struct_layout_version = 2; -const optional_layout_version = 1; +const optional_layout_version = 2; /// We use the least significant bit of the pointer address to tell us /// whether the type is fully resolved. Types that are only fwd declared diff --git a/src/codegen/llvm/bindings.zig b/src/codegen/llvm/bindings.zig index 3279f052b2..6cd5e41b10 100644 --- a/src/codegen/llvm/bindings.zig +++ b/src/codegen/llvm/bindings.zig @@ -304,6 +304,9 @@ pub const Type = opaque { pub const isOpaqueStruct = LLVMIsOpaqueStruct; extern fn LLVMIsOpaqueStruct(StructTy: *const Type) Bool; + + pub const isSized = LLVMTypeIsSized; + extern fn LLVMTypeIsSized(Ty: *const Type) Bool; }; pub const Module = opaque { diff --git a/src/type.zig b/src/type.zig index 0744a50579..59f0668770 100644 --- a/src/type.zig +++ b/src/type.zig @@ -2305,6 +2305,9 @@ pub const Type = extern union { /// true if and only if the type takes up space in memory at runtime. /// There are two reasons a type will return false: /// * the type is a comptime-only type. For example, the type `type` itself. + /// - note, however, that a struct can have mixed fields and only the non-comptime-only + /// fields will count towards the ABI size. For example, `struct {T: type, x: i32}` + /// hasRuntimeBits()=true and abiSize()=4 /// * the type has only one possible value, making its ABI size 0. /// When `ignore_comptime_only` is true, then types that are comptime only /// may return false positives. From 9329b93b8896e552812d68a542b296cb897584d8 Mon Sep 17 00:00:00 2001 From: Andrew Kelley Date: Thu, 14 Jul 2022 23:26:32 -0700 Subject: [PATCH 5/5] LLVM: disable the ABI size safety check There are many more instances of this check being tripped that we need to fix before we can enable this. --- src/codegen/llvm.zig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/codegen/llvm.zig b/src/codegen/llvm.zig index 6608920dfa..d688ff6dc6 100644 --- a/src/codegen/llvm.zig +++ b/src/codegen/llvm.zig @@ -2425,7 +2425,7 @@ pub const DeclGen = struct { fn lowerType(dg: *DeclGen, t: Type) Allocator.Error!*const llvm.Type { const llvm_ty = try lowerTypeInner(dg, t); - if (std.debug.runtime_safety) check: { + if (std.debug.runtime_safety and false) check: { if (t.zigTypeTag() == .Opaque) break :check; if (!t.hasRuntimeBits()) break :check; if (!llvm_ty.isSized().toBool()) break :check;