From 8e96922f3118601dd30da23ba7db1440066784ab Mon Sep 17 00:00:00 2001 From: LemonBoy Date: Thu, 16 Apr 2020 20:26:38 +0200 Subject: [PATCH] stage1: Fix several bugs in constant generation The codegen would sometimes change the LLVM type for some constants to an unnamed structure in order to accomodate extra padding. This is fine as long as the alignment of each field is still respected and it was not the case for structure types, leading to ill-formed constants being generated. Optional types suffer from this to a lower extent as their layout is quite lucky, the only missing piece was the tail padding. Closes #4530 Closes #4594 Closes #4295 Closes my will to live --- src/analyze.cpp | 2 + src/codegen.cpp | 82 +++++++++++++++++++++---------- test/stage1/behavior/optional.zig | 37 +++++++++++++- test/stage1/behavior/struct.zig | 16 ++++++ 4 files changed, 111 insertions(+), 26 deletions(-) diff --git a/src/analyze.cpp b/src/analyze.cpp index c3580e35e..d7064ed59 100644 --- a/src/analyze.cpp +++ b/src/analyze.cpp @@ -858,10 +858,12 @@ ZigType *get_slice_type(CodeGen *g, ZigType *ptr_type) { entry->data.structure.fields[slice_ptr_index]->type_entry = ptr_type; entry->data.structure.fields[slice_ptr_index]->src_index = slice_ptr_index; entry->data.structure.fields[slice_ptr_index]->gen_index = 0; + entry->data.structure.fields[slice_ptr_index]->offset = 0; entry->data.structure.fields[slice_len_index]->name = len_field_name; entry->data.structure.fields[slice_len_index]->type_entry = g->builtin_types.entry_usize; entry->data.structure.fields[slice_len_index]->src_index = slice_len_index; entry->data.structure.fields[slice_len_index]->gen_index = 1; + entry->data.structure.fields[slice_len_index]->offset = ptr_type->abi_size; entry->data.structure.fields_by_name.put(ptr_field_name, entry->data.structure.fields[slice_ptr_index]); entry->data.structure.fields_by_name.put(len_field_name, entry->data.structure.fields[slice_len_index]); diff --git a/src/codegen.cpp b/src/codegen.cpp index 1a091584b..ea4d698cd 100644 --- a/src/codegen.cpp +++ b/src/codegen.cpp @@ -7140,12 +7140,12 @@ static LLVMValueRef gen_const_val(CodeGen *g, ZigValue *const_val, const char *n ZigType *type_entry = const_val->type; assert(type_has_bits(g, type_entry)); -check: switch (const_val->special) { + if (const_val->special == ConstValSpecialLazy && + (err = ir_resolve_lazy(g, nullptr, const_val))) + codegen_report_errors_and_exit(g); + + switch (const_val->special) { case ConstValSpecialLazy: - if ((err = ir_resolve_lazy(g, nullptr, const_val))) { - codegen_report_errors_and_exit(g); - } - goto check; case ConstValSpecialRuntime: zig_unreachable(); case ConstValSpecialUndef: @@ -7222,12 +7222,26 @@ check: switch (const_val->special) { make_unnamed_struct = false; } + LLVMValueRef fields[] = { child_val, maybe_val, + nullptr, }; if (make_unnamed_struct) { - return LLVMConstStruct(fields, 2, false); + LLVMValueRef result = LLVMConstStruct(fields, 2, false); + uint64_t last_field_offset = LLVMOffsetOfElement(g->target_data_ref, LLVMTypeOf(result), 1); + uint64_t end_offset = last_field_offset + + LLVMStoreSizeOfType(g->target_data_ref, LLVMTypeOf(fields[1])); + uint64_t expected_sz = LLVMABISizeOfType(g->target_data_ref, get_llvm_type(g, type_entry)); + unsigned pad_sz = expected_sz - end_offset; + if (pad_sz != 0) { + fields[2] = LLVMGetUndef(LLVMArrayType(LLVMInt8Type(), pad_sz)); + result = LLVMConstStruct(fields, 3, false); + } + uint64_t actual_sz = LLVMStoreSizeOfType(g->target_data_ref, LLVMTypeOf(result)); + assert(actual_sz == expected_sz); + return result; } else { return LLVMConstNamedStruct(get_llvm_type(g, type_entry), fields, 2); } @@ -7316,32 +7330,50 @@ check: switch (const_val->special) { continue; } ZigValue *field_val = const_val->data.x_struct.fields[i]; - assert(field_val->type != nullptr); - if ((err = ensure_const_val_repr(nullptr, g, nullptr, field_val, - type_struct_field->type_entry))) - { + ZigType *field_type = field_val->type; + assert(field_type != nullptr); + if ((err = ensure_const_val_repr(nullptr, g, nullptr, field_val, field_type))) { zig_unreachable(); } LLVMValueRef val = gen_const_val(g, field_val, ""); - fields[type_struct_field->gen_index] = val; - make_unnamed_struct = make_unnamed_struct || is_llvm_value_unnamed_type(g, field_val->type, val); + make_unnamed_struct = make_unnamed_struct || is_llvm_value_unnamed_type(g, field_type, val); - size_t end_pad_gen_index = (i + 1 < src_field_count) ? - type_entry->data.structure.fields[i + 1]->gen_index : - type_entry->data.structure.gen_field_count; - size_t next_offset = (i + 1 < src_field_count) ? - type_entry->data.structure.fields[i + 1]->offset : type_entry->abi_size; - if (end_pad_gen_index != SIZE_MAX) { - for (size_t gen_i = type_struct_field->gen_index + 1; gen_i < end_pad_gen_index; - gen_i += 1) - { - size_t pad_bytes = next_offset - - (type_struct_field->offset + type_struct_field->type_entry->abi_size); - LLVMTypeRef llvm_array_type = LLVMArrayType(LLVMInt8Type(), pad_bytes); - fields[gen_i] = LLVMGetUndef(llvm_array_type); + // Find the next runtime field + size_t next_rt_gen_index = type_entry->data.structure.gen_field_count; + size_t next_offset = type_entry->abi_size; + for (size_t j = i + 1; j < src_field_count; j++) { + const size_t index = type_entry->data.structure.fields[j]->gen_index; + const size_t offset = type_entry->data.structure.fields[j]->offset; + + if (index != SIZE_MAX) { + next_rt_gen_index = index; + next_offset = offset; + break; } } + + // How much padding is needed to reach the next field + const size_t pad_bytes = next_offset - + (type_struct_field->offset + LLVMABISizeOfType(g->target_data_ref, LLVMTypeOf(val))); + // Catch underflow + assert((ssize_t)pad_bytes >= 0); + + if (type_struct_field->gen_index + 1 != next_rt_gen_index) { + // If there's a hole between this field and the next + // we have an alignment gap to fill + fields[type_struct_field->gen_index] = val; + fields[type_struct_field->gen_index + 1] = LLVMGetUndef(LLVMArrayType(LLVMInt8Type(), pad_bytes)); + } else if (pad_bytes != 0) { + LLVMValueRef padded_val[] = { + val, + LLVMGetUndef(LLVMArrayType(LLVMInt8Type(), pad_bytes)), + }; + fields[type_struct_field->gen_index] = LLVMConstStruct(padded_val, 2, true); + make_unnamed_struct = true; + } else { + fields[type_struct_field->gen_index] = val; + } } } if (make_unnamed_struct) { diff --git a/test/stage1/behavior/optional.zig b/test/stage1/behavior/optional.zig index 7dd48769d..ffd37e59f 100644 --- a/test/stage1/behavior/optional.zig +++ b/test/stage1/behavior/optional.zig @@ -1,4 +1,7 @@ -const expect = @import("std").testing.expect; +const std = @import("std"); +const testing = std.testing; +const expect = testing.expect; +const expectEqual = testing.expectEqual; pub const EmptyStruct = struct {}; @@ -198,3 +201,35 @@ test "0-bit child type coerced to optional" { S.doTheTest(); comptime S.doTheTest(); } + +test "array of optional unaligned types" { + const Enum = enum { one, two, three }; + + const SomeUnion = union(enum) { + Num: Enum, + Other: u32, + }; + + const values = [_]?SomeUnion{ + SomeUnion{ .Num = .one }, + SomeUnion{ .Num = .two }, + SomeUnion{ .Num = .three }, + SomeUnion{ .Num = .one }, + SomeUnion{ .Num = .two }, + SomeUnion{ .Num = .three }, + }; + + // The index must be a runtime value + var i: usize = 0; + expectEqual(Enum.one, values[i].?.Num); + i += 1; + expectEqual(Enum.two, values[i].?.Num); + i += 1; + expectEqual(Enum.three, values[i].?.Num); + i += 1; + expectEqual(Enum.one, values[i].?.Num); + i += 1; + expectEqual(Enum.two, values[i].?.Num); + i += 1; + expectEqual(Enum.three, values[i].?.Num); +} diff --git a/test/stage1/behavior/struct.zig b/test/stage1/behavior/struct.zig index 1203399a2..affbc4b4b 100644 --- a/test/stage1/behavior/struct.zig +++ b/test/stage1/behavior/struct.zig @@ -825,3 +825,19 @@ test "self-referencing struct via array member" { x = T{ .children = .{&x} }; expect(x.children[0] == &x); } + +test "struct with union field" { + const Value = struct { + ref: u32 = 2, + kind: union(enum) { + None: usize, + Bool: bool, + }, + }; + + var True = Value{ + .kind = .{ .Bool = true }, + }; + expectEqual(@as(u32, 2), True.ref); + expectEqual(true, True.kind.Bool); +}