From e96f65db7735ef80e19253ec25a1ce31751650f1 Mon Sep 17 00:00:00 2001 From: Jacob Young Date: Sun, 18 Dec 2022 01:25:19 -0500 Subject: llvm: fix lowering pointer to final zero-width field of a comptime value * Handle a `null` return from `llvmFieldIndex`. * Add a behavior test to test this code path. * Reword this test name, which incorrectly described how pointers to zero-bit fields behave, and instead describe the actual test. --- src/codegen/llvm.zig | 22 +++++++++++++++------- test/behavior/struct.zig | 36 +++++++++++++++++++++++------------- 2 files changed, 38 insertions(+), 20 deletions(-) diff --git a/src/codegen/llvm.zig b/src/codegen/llvm.zig index 7af987f4d6..39c797e12e 100644 --- a/src/codegen/llvm.zig +++ b/src/codegen/llvm.zig @@ -4033,16 +4033,24 @@ pub const DeclGen = struct { const final_llvm_ty = (try dg.lowerType(ptr_child_ty)).pointerType(0); break :blk field_addr.constIntToPtr(final_llvm_ty); } - bitcast_needed = !field_ty.eql(ptr_child_ty, dg.module); var ty_buf: Type.Payload.Pointer = undefined; - const llvm_field_index = llvmFieldIndex(parent_ty, field_index, target, &ty_buf).?; - const indices: [2]*llvm.Value = .{ - llvm_u32.constInt(0, .False), - llvm_u32.constInt(llvm_field_index, .False), - }; + const parent_llvm_ty = try dg.lowerType(parent_ty); - break :blk parent_llvm_ty.constInBoundsGEP(parent_llvm_ptr, &indices, indices.len); + if (llvmFieldIndex(parent_ty, field_index, target, &ty_buf)) |llvm_field_index| { + bitcast_needed = !field_ty.eql(ptr_child_ty, dg.module); + const indices: [2]*llvm.Value = .{ + llvm_u32.constInt(0, .False), + llvm_u32.constInt(llvm_field_index, .False), + }; + break :blk parent_llvm_ty.constInBoundsGEP(parent_llvm_ptr, &indices, indices.len); + } else { + bitcast_needed = !parent_ty.eql(ptr_child_ty, dg.module); + const indices: [1]*llvm.Value = .{ + llvm_u32.constInt(1, .False), + }; + break :blk parent_llvm_ty.constInBoundsGEP(parent_llvm_ptr, &indices, indices.len); + } }, .Pointer => { assert(parent_ty.isSlice()); diff --git a/test/behavior/struct.zig b/test/behavior/struct.zig index db7092ab82..0984f7d1e4 100644 --- a/test/behavior/struct.zig +++ b/test/behavior/struct.zig @@ -1359,23 +1359,33 @@ test "under-aligned struct field" { try expect(result == 1234); } -test "address of zero-bit field is equal to address of only field" { +test "fieldParentPtr of a zero-bit field" { if (builtin.zig_backend == .stage2_x86_64) return error.SkipZigTest; // TODO if (builtin.zig_backend == .stage2_aarch64) return error.SkipZigTest; // TODO if (builtin.zig_backend == .stage2_arm) return error.SkipZigTest; // TODO - { - const A = struct { b: void = {}, u: u8 }; - var a = A{ .u = 0 }; - const a_ptr = @fieldParentPtr(A, "b", &a.b); - try std.testing.expectEqual(&a, a_ptr); - } - { - const A = struct { u: u8, b: void = {} }; - var a = A{ .u = 0 }; - const a_ptr = @fieldParentPtr(A, "b", &a.b); - try std.testing.expectEqual(&a, a_ptr); - } + const S = struct { + fn testOneType(comptime A: type) !void { + { + const a = A{ .u = 0 }; + const b_ptr = &a.b; + const a_ptr = @fieldParentPtr(A, "b", b_ptr); + try std.testing.expectEqual(&a, a_ptr); + } + { + var a = A{ .u = 0 }; + const b_ptr = &a.b; + const a_ptr = @fieldParentPtr(A, "b", b_ptr); + try std.testing.expectEqual(&a, a_ptr); + } + } + fn doTheTest() !void { + try testOneType(struct { b: void = {}, u: u8 }); + try testOneType(struct { u: u8, b: void = {} }); + } + }; + try S.doTheTest(); + comptime try S.doTheTest(); } test "struct field has a pointer to an aligned version of itself" { -- cgit v1.2.3 From 18f05664dcf9c3a307d269dda4f146bafbca19e9 Mon Sep 17 00:00:00 2001 From: Jacob Young Date: Sun, 18 Dec 2022 21:53:49 -0500 Subject: llvm: avoid creating an invalid llvm type Fixes the following assertion: ``` zig: llvm/lib/IR/Type.cpp:729: static llvm::PointerType* llvm::PointerType::get(llvm::Type*, unsigned int): Assertion `isValidElementType(EltTy) && "Invalid type for pointer element!"' failed. ``` --- 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 39c797e12e..5641793d9e 100644 --- a/src/codegen/llvm.zig +++ b/src/codegen/llvm.zig @@ -4125,7 +4125,7 @@ pub const DeclGen = struct { else => unreachable, }; if (bitcast_needed) { - return llvm_ptr.constBitCast((try dg.lowerType(ptr_child_ty)).pointerType(0)); + return llvm_ptr.constBitCast((try dg.lowerPtrElemTy(ptr_child_ty)).pointerType(0)); } else { return llvm_ptr; } -- cgit v1.2.3 From 52e5c6602550788cab96957d1a177bc7952d7a09 Mon Sep 17 00:00:00 2001 From: Jacob Young Date: Sun, 18 Dec 2022 21:57:18 -0500 Subject: llvm: fix use of invalid alignment * Initialize `big_align` with 1 as 0 is not a valid alignment. * Add an assert to `alignForwardGeneric` to catch this issue earlier. * Refactor valid alignment checks to call a more descriptive function. --- lib/std/mem.zig | 17 ++++++++++++----- src/codegen/llvm.zig | 2 +- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/lib/std/mem.zig b/lib/std/mem.zig index 17281390a8..a392335a70 100644 --- a/lib/std/mem.zig +++ b/lib/std/mem.zig @@ -3291,7 +3291,7 @@ pub fn nativeToBig(comptime T: type, x: T) T { /// - The delta required to align the pointer is not a multiple of the pointee's /// type. pub fn alignPointerOffset(ptr: anytype, align_to: usize) ?usize { - assert(align_to != 0 and @popCount(align_to) == 1); + assert(isValidAlign(align_to)); const T = @TypeOf(ptr); const info = @typeInfo(T); @@ -3751,6 +3751,7 @@ pub fn alignForwardLog2(addr: usize, log2_alignment: u8) usize { /// The alignment must be a power of 2 and greater than 0. /// Asserts that rounding up the address does not cause integer overflow. pub fn alignForwardGeneric(comptime T: type, addr: T, alignment: T) T { + assert(isValidAlignGeneric(T, alignment)); return alignBackwardGeneric(T, addr + (alignment - 1), alignment); } @@ -3846,7 +3847,7 @@ test "alignForward" { /// Round an address down to the previous (or current) aligned address. /// Unlike `alignBackward`, `alignment` can be any positive number, not just a power of 2. pub fn alignBackwardAnyAlign(i: usize, alignment: usize) usize { - if (@popCount(alignment) == 1) + if (isValidAlign(alignment)) return alignBackward(i, alignment); assert(alignment != 0); return i - @mod(i, alignment); @@ -3861,7 +3862,7 @@ pub fn alignBackward(addr: usize, alignment: usize) usize { /// Round an address down to the previous (or current) aligned address. /// The alignment must be a power of 2 and greater than 0. pub fn alignBackwardGeneric(comptime T: type, addr: T, alignment: T) T { - assert(@popCount(alignment) == 1); + assert(isValidAlignGeneric(T, alignment)); // 000010000 // example alignment // 000001111 // subtract 1 // 111110000 // binary not @@ -3871,11 +3872,17 @@ pub fn alignBackwardGeneric(comptime T: type, addr: T, alignment: T) T { /// Returns whether `alignment` is a valid alignment, meaning it is /// a positive power of 2. pub fn isValidAlign(alignment: usize) bool { - return @popCount(alignment) == 1; + return isValidAlignGeneric(usize, alignment); +} + +/// Returns whether `alignment` is a valid alignment, meaning it is +/// a positive power of 2. +pub fn isValidAlignGeneric(comptime T: type, alignment: T) bool { + return alignment > 0 and std.math.isPowerOfTwo(alignment); } pub fn isAlignedAnyAlign(i: usize, alignment: usize) bool { - if (@popCount(alignment) == 1) + if (isValidAlign(alignment)) return isAligned(i, alignment); assert(alignment != 0); return 0 == @mod(i, alignment); diff --git a/src/codegen/llvm.zig b/src/codegen/llvm.zig index 5641793d9e..43ffcb0aba 100644 --- a/src/codegen/llvm.zig +++ b/src/codegen/llvm.zig @@ -2969,7 +2969,7 @@ pub const DeclGen = struct { comptime assert(struct_layout_version == 2); var offset: u64 = 0; - var big_align: u32 = 0; + var big_align: u32 = 1; var any_underaligned_fields = false; for (struct_obj.fields.values()) |field| { -- cgit v1.2.3 From 0e3feebb042a538405153e4bc75a3bb73ee2e63c Mon Sep 17 00:00:00 2001 From: Jacob Young Date: Sun, 18 Dec 2022 22:09:13 -0500 Subject: codegen: fix taking the address of a zero-bit field in a zero-bit struct Normally when we want a pointer to the end of a struct we just add 1 to the struct pointer. However, when it is a zero-bit struct, the pointer type being used during lowering is often a dummy pointer type that actually points to a non-zero-bit type, so we actually want to add 0 instead, since a zero-bit struct begins and ends at the same address. --- src/codegen/c.zig | 4 +++- src/codegen/llvm.zig | 9 ++++----- test/behavior/struct.zig | 26 +++++++++++++++++++++++--- 3 files changed, 30 insertions(+), 9 deletions(-) diff --git a/src/codegen/c.zig b/src/codegen/c.zig index a7a2b2cf2a..9488fa420b 100644 --- a/src/codegen/c.zig +++ b/src/codegen/c.zig @@ -5131,7 +5131,9 @@ fn structFieldPtr(f: *Function, inst: Air.Inst.Index, struct_ptr_ty: Type, struc .begin, .end => { try writer.writeByte('('); try f.writeCValue(writer, struct_ptr, .Other); - try writer.print(")[{}]", .{@boolToInt(field_loc == .end)}); + try writer.print(")[{}]", .{ + @boolToInt(field_loc == .end and struct_ty.hasRuntimeBitsIgnoreComptime()), + }); }, .field => |field| if (extra_name != .none) { try f.writeCValueDerefMember(writer, struct_ptr, extra_name); diff --git a/src/codegen/llvm.zig b/src/codegen/llvm.zig index 43ffcb0aba..3b180b4c50 100644 --- a/src/codegen/llvm.zig +++ b/src/codegen/llvm.zig @@ -4046,9 +4046,8 @@ pub const DeclGen = struct { break :blk parent_llvm_ty.constInBoundsGEP(parent_llvm_ptr, &indices, indices.len); } else { bitcast_needed = !parent_ty.eql(ptr_child_ty, dg.module); - const indices: [1]*llvm.Value = .{ - llvm_u32.constInt(1, .False), - }; + const llvm_index = llvm_u32.constInt(@boolToInt(parent_ty.hasRuntimeBitsIgnoreComptime()), .False); + const indices: [1]*llvm.Value = .{llvm_index}; break :blk parent_llvm_ty.constInBoundsGEP(parent_llvm_ptr, &indices, indices.len); } }, @@ -9774,8 +9773,8 @@ pub const FuncGen = struct { // end of the struct. Treat our struct pointer as an array of two and get // the index to the element at index `1` to get a pointer to the end of // the struct. - const llvm_usize = try self.dg.lowerType(Type.usize); - const llvm_index = llvm_usize.constInt(1, .False); + const llvm_u32 = self.dg.context.intType(32); + const llvm_index = llvm_u32.constInt(@boolToInt(struct_ty.hasRuntimeBitsIgnoreComptime()), .False); const indices: [1]*llvm.Value = .{llvm_index}; return self.builder.buildInBoundsGEP(struct_llvm_ty, struct_ptr, &indices, indices.len, ""); } diff --git a/test/behavior/struct.zig b/test/behavior/struct.zig index 0984f7d1e4..b6567b14c3 100644 --- a/test/behavior/struct.zig +++ b/test/behavior/struct.zig @@ -1365,7 +1365,7 @@ test "fieldParentPtr of a zero-bit field" { if (builtin.zig_backend == .stage2_arm) return error.SkipZigTest; // TODO const S = struct { - fn testOneType(comptime A: type) !void { + fn testStruct(comptime A: type) !void { { const a = A{ .u = 0 }; const b_ptr = &a.b; @@ -1379,9 +1379,29 @@ test "fieldParentPtr of a zero-bit field" { try std.testing.expectEqual(&a, a_ptr); } } + fn testNestedStruct(comptime A: type) !void { + { + const a = A{ .u = 0 }; + const c_ptr = &a.b.c; + const b_ptr = @fieldParentPtr(@TypeOf(a.b), "c", c_ptr); + try std.testing.expectEqual(&a.b, b_ptr); + const a_ptr = @fieldParentPtr(A, "b", b_ptr); + try std.testing.expectEqual(&a, a_ptr); + } + { + var a = A{ .u = 0 }; + const c_ptr = &a.b.c; + const b_ptr = @fieldParentPtr(@TypeOf(a.b), "c", c_ptr); + try std.testing.expectEqual(&a.b, b_ptr); + const a_ptr = @fieldParentPtr(A, "b", b_ptr); + try std.testing.expectEqual(&a, a_ptr); + } + } fn doTheTest() !void { - try testOneType(struct { b: void = {}, u: u8 }); - try testOneType(struct { u: u8, b: void = {} }); + try testStruct(struct { b: void = {}, u: u8 }); + try testStruct(struct { u: u8, b: void = {} }); + try testNestedStruct(struct { b: struct { c: void = {} } = .{}, u: u8 }); + try testNestedStruct(struct { u: u8, b: struct { c: void = {} } = .{} }); } }; try S.doTheTest(); -- cgit v1.2.3 From 202e8a0589b4e02dac0694e3eb30ac0b51ec3f6c Mon Sep 17 00:00:00 2001 From: Jacob Young Date: Sun, 18 Dec 2022 23:38:37 -0500 Subject: cbe: fix type passed to renderParentPtr --- src/codegen/c.zig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/codegen/c.zig b/src/codegen/c.zig index 9488fa420b..2cd24f69b3 100644 --- a/src/codegen/c.zig +++ b/src/codegen/c.zig @@ -653,7 +653,7 @@ pub const DeclGen = struct { } try writer.print("{ }", .{fmtIdent(field_info.name)}); } else { - try dg.renderParentPtr(writer, field_ptr.container_ptr, field_info.ty); + try dg.renderParentPtr(writer, field_ptr.container_ptr, container_ptr_ty); } }, .elem_ptr => { -- cgit v1.2.3 From 0768115b01f01ab1c75da3e42ffcdc99078eaad2 Mon Sep 17 00:00:00 2001 From: Jacob Young Date: Mon, 19 Dec 2022 05:49:15 -0500 Subject: behavior: disable failing test Also add an assert to catch this issue earlier. For future reference, the decl without a type and value is the string literal "GET". --- src/value.zig | 12 ++++++++++-- test/behavior/bugs/3742.zig | 2 ++ 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/src/value.zig b/src/value.zig index 839b3d7580..2d676f51d3 100644 --- a/src/value.zig +++ b/src/value.zig @@ -2915,8 +2915,16 @@ pub const Value = extern union { .field_ptr => val.castTag(.field_ptr).?.data.container_ptr.isVariable(mod), .eu_payload_ptr => val.castTag(.eu_payload_ptr).?.data.container_ptr.isVariable(mod), .opt_payload_ptr => val.castTag(.opt_payload_ptr).?.data.container_ptr.isVariable(mod), - .decl_ref => mod.declPtr(val.castTag(.decl_ref).?.data).val.isVariable(mod), - .decl_ref_mut => mod.declPtr(val.castTag(.decl_ref_mut).?.data.decl_index).val.isVariable(mod), + .decl_ref => { + const decl = mod.declPtr(val.castTag(.decl_ref).?.data); + assert(decl.has_tv); + return decl.val.isVariable(mod); + }, + .decl_ref_mut => { + const decl = mod.declPtr(val.castTag(.decl_ref_mut).?.data.decl_index); + assert(decl.has_tv); + return decl.val.isVariable(mod); + }, .variable => true, else => false, diff --git a/test/behavior/bugs/3742.zig b/test/behavior/bugs/3742.zig index ab5ee92023..a984f0d8e4 100644 --- a/test/behavior/bugs/3742.zig +++ b/test/behavior/bugs/3742.zig @@ -39,5 +39,7 @@ test "fixed" { if (builtin.zig_backend == .stage2_aarch64) return error.SkipZigTest; if (builtin.zig_backend == .stage2_x86_64) return error.SkipZigTest; if (builtin.zig_backend == .stage2_sparc64) return error.SkipZigTest; // TODO + if (builtin.zig_backend == .stage2_llvm and + builtin.cpu.arch == .aarch64 and builtin.os.tag == .windows) return error.SkipZigTest; ArgSerializer.serializeCommand(GET.init("banana")); } -- cgit v1.2.3