From e40de8608273d50afe4c2d278c34be142af57562 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20R=C3=B8nne=20Petersen?= Date: Thu, 31 Oct 2024 12:45:49 +0100 Subject: [PATCH 01/16] riscv64: Get rid of some trailing whitespace. --- src/arch/riscv64/CodeGen.zig | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/arch/riscv64/CodeGen.zig b/src/arch/riscv64/CodeGen.zig index 77efade41f..6c2befb98a 100644 --- a/src/arch/riscv64/CodeGen.zig +++ b/src/arch/riscv64/CodeGen.zig @@ -1460,7 +1460,7 @@ fn genBody(func: *Func, body: []const Air.Inst.Index) InnerError!void { .mul, .mul_wrap, - .div_trunc, + .div_trunc, .div_exact, .rem, @@ -1478,13 +1478,13 @@ fn genBody(func: *Func, body: []const Air.Inst.Index) InnerError!void { .max, => try func.airBinOp(inst, tag), - + .ptr_add, .ptr_sub => try func.airPtrArithmetic(inst, tag), .mod, - .div_float, - .div_floor, + .div_float, + .div_floor, => return func.fail("TODO: {s}", .{@tagName(tag)}), .sqrt, @@ -1639,7 +1639,7 @@ fn genBody(func: *Func, body: []const Air.Inst.Index) InnerError!void { .ptr_slice_ptr_ptr => try func.airPtrSlicePtrPtr(inst), .array_elem_val => try func.airArrayElemVal(inst), - + .slice_elem_val => try func.airSliceElemVal(inst), .slice_elem_ptr => try func.airSliceElemPtr(inst), From 61ae9a2f4547ec2fd0024e79addfa2d38b4c1239 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20R=C3=B8nne=20Petersen?= Date: Fri, 1 Nov 2024 04:04:31 +0100 Subject: [PATCH 02/16] riscv64: Add missing fence for seq_cst atomic_store. --- src/arch/riscv64/CodeGen.zig | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/arch/riscv64/CodeGen.zig b/src/arch/riscv64/CodeGen.zig index 6c2befb98a..9c96b2c19f 100644 --- a/src/arch/riscv64/CodeGen.zig +++ b/src/arch/riscv64/CodeGen.zig @@ -7729,7 +7729,7 @@ fn airAtomicLoad(func: *Func, inst: Air.Inst.Index) !void { const ptr_mcv = try func.resolveInst(atomic_load.ptr); const bit_size = elem_ty.bitSize(zcu); - if (bit_size > 64) return func.fail("TODO: airAtomicStore > 64 bits", .{}); + if (bit_size > 64) return func.fail("TODO: airAtomicLoad > 64 bits", .{}); const result_mcv = try func.allocRegOrMem(elem_ty, inst, true); assert(result_mcv == .register); // should be less than 8 bytes @@ -7793,6 +7793,17 @@ fn airAtomicStore(func: *Func, inst: Air.Inst.Index, order: std.builtin.AtomicOr } try func.store(ptr_mcv, val_mcv, ptr_ty); + + if (order == .seq_cst) { + _ = try func.addInst(.{ + .tag = .fence, + .data = .{ .fence = .{ + .pred = .rw, + .succ = .rw, + } }, + }); + } + return func.finishAir(inst, .unreach, .{ bin_op.lhs, bin_op.rhs, .none }); } From d4ca9804f8a546c45451a58c8bde20bc2299d4a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20R=C3=B8nne=20Petersen?= Date: Sat, 2 Nov 2024 08:59:14 +0100 Subject: [PATCH 03/16] riscv64: Handle writes to the zero register sensibly in result bookkeeping. --- src/arch/riscv64/CodeGen.zig | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/arch/riscv64/CodeGen.zig b/src/arch/riscv64/CodeGen.zig index 9c96b2c19f..d4b96ae3f3 100644 --- a/src/arch/riscv64/CodeGen.zig +++ b/src/arch/riscv64/CodeGen.zig @@ -1769,8 +1769,15 @@ fn finishAirBookkeeping(func: *Func) void { fn finishAirResult(func: *Func, inst: Air.Inst.Index, result: MCValue) void { if (func.liveness.isUnused(inst)) switch (result) { .none, .dead, .unreach => {}, - else => unreachable, // Why didn't the result die? + // Why didn't the result die? + .register => |r| if (r != .zero) unreachable, + else => unreachable, } else { + switch (result) { + .register => |r| if (r == .zero) unreachable, // Why did we discard a used result? + else => {}, + } + tracking_log.debug("%{d} => {} (birth)", .{ inst, result }); func.inst_tracking.putAssumeCapacityNoClobber(inst, InstTracking.init(result)); // In some cases, an operand may be reused as the result. From 4c36a403a895bd3e7dc7b4dd308f2b82a3dab60a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20R=C3=B8nne=20Petersen?= Date: Thu, 31 Oct 2024 09:00:02 +0100 Subject: [PATCH 04/16] Air: Fix mustLower() for atomic_load with inter-thread ordering. --- src/Air.zig | 5 ++++- src/arch/riscv64/CodeGen.zig | 14 ++++++++------ src/arch/x86_64/CodeGen.zig | 28 +++++++++++++++++----------- 3 files changed, 29 insertions(+), 18 deletions(-) diff --git a/src/Air.zig b/src/Air.zig index 3b1dfc644a..4379d5dde1 100644 --- a/src/Air.zig +++ b/src/Air.zig @@ -1889,7 +1889,10 @@ pub fn mustLower(air: Air, inst: Air.Inst.Index, ip: *const InternPool) bool { }, .load => air.typeOf(data.ty_op.operand, ip).isVolatilePtrIp(ip), .slice_elem_val, .ptr_elem_val => air.typeOf(data.bin_op.lhs, ip).isVolatilePtrIp(ip), - .atomic_load => air.typeOf(data.atomic_load.ptr, ip).isVolatilePtrIp(ip), + .atomic_load => switch (data.atomic_load.order) { + .unordered, .monotonic => air.typeOf(data.atomic_load.ptr, ip).isVolatilePtrIp(ip), + else => true, // Stronger memory orderings have inter-thread side effects. + }, }; } diff --git a/src/arch/riscv64/CodeGen.zig b/src/arch/riscv64/CodeGen.zig index d4b96ae3f3..7ed8352b5d 100644 --- a/src/arch/riscv64/CodeGen.zig +++ b/src/arch/riscv64/CodeGen.zig @@ -7738,7 +7738,10 @@ fn airAtomicLoad(func: *Func, inst: Air.Inst.Index) !void { const bit_size = elem_ty.bitSize(zcu); if (bit_size > 64) return func.fail("TODO: airAtomicLoad > 64 bits", .{}); - const result_mcv = try func.allocRegOrMem(elem_ty, inst, true); + const result_mcv: MCValue = if (func.liveness.isUnused(inst)) + .{ .register = .zero } + else + try func.allocRegOrMem(elem_ty, inst, true); assert(result_mcv == .register); // should be less than 8 bytes if (order == .seq_cst) { @@ -7754,11 +7757,10 @@ fn airAtomicLoad(func: *Func, inst: Air.Inst.Index) !void { try func.load(result_mcv, ptr_mcv, ptr_ty); switch (order) { - // Don't guarnetee other memory operations to be ordered after the load. - .unordered => {}, - .monotonic => {}, - // Make sure all previous reads happen before any reading or writing accurs. - .seq_cst, .acquire => { + // Don't guarantee other memory operations to be ordered after the load. + .unordered, .monotonic => {}, + // Make sure all previous reads happen before any reading or writing occurs. + .acquire, .seq_cst => { _ = try func.addInst(.{ .tag = .fence, .data = .{ .fence = .{ diff --git a/src/arch/x86_64/CodeGen.zig b/src/arch/x86_64/CodeGen.zig index 3ba38ccd7b..f0478d7e23 100644 --- a/src/arch/x86_64/CodeGen.zig +++ b/src/arch/x86_64/CodeGen.zig @@ -106219,23 +106219,29 @@ fn airAtomicRmw(self: *CodeGen, inst: Air.Inst.Index) !void { fn airAtomicLoad(self: *CodeGen, inst: Air.Inst.Index) !void { const atomic_load = self.air.instructions.items(.data)[@intFromEnum(inst)].atomic_load; + const result: MCValue = result: { + const ptr_ty = self.typeOf(atomic_load.ptr); + const ptr_mcv = try self.resolveInst(atomic_load.ptr); + const ptr_lock = switch (ptr_mcv) { + .register => |reg| self.register_manager.lockRegAssumeUnused(reg), + else => null, + }; + defer if (ptr_lock) |lock| self.register_manager.unlockReg(lock); - const ptr_ty = self.typeOf(atomic_load.ptr); - const ptr_mcv = try self.resolveInst(atomic_load.ptr); - const ptr_lock = switch (ptr_mcv) { - .register => |reg| self.register_manager.lockRegAssumeUnused(reg), - else => null, - }; - defer if (ptr_lock) |lock| self.register_manager.unlockReg(lock); + const unused = self.liveness.isUnused(inst); - const dst_mcv = - if (self.reuseOperand(inst, atomic_load.ptr, 0, ptr_mcv)) + const dst_mcv: MCValue = if (unused) + .{ .register = try self.register_manager.allocReg(null, self.regSetForType(ptr_ty.childType(self.pt.zcu))) } + else if (self.reuseOperand(inst, atomic_load.ptr, 0, ptr_mcv)) ptr_mcv else try self.allocRegOrMem(inst, true); - try self.load(dst_mcv, ptr_ty, ptr_mcv); - return self.finishAir(inst, dst_mcv, .{ atomic_load.ptr, .none, .none }); + try self.load(dst_mcv, ptr_ty, ptr_mcv); + + break :result if (unused) .unreach else dst_mcv; + }; + return self.finishAir(inst, result, .{ atomic_load.ptr, .none, .none }); } fn airAtomicStore(self: *CodeGen, inst: Air.Inst.Index, order: std.builtin.AtomicOrder) !void { From 47aaaec6ea32754ad9d7f9f25e874d63439aead8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20R=C3=B8nne=20Petersen?= Date: Thu, 31 Oct 2024 09:50:14 +0100 Subject: [PATCH 05/16] Air: Always return true for inline assembly in mustLower(). AstGen requires inline assembly to either have outputs or be marked volatile, so there doesn't appear to be any point in doing these checks. --- src/Air.zig | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/src/Air.zig b/src/Air.zig index 4379d5dde1..f7d8699b73 100644 --- a/src/Air.zig +++ b/src/Air.zig @@ -1673,6 +1673,7 @@ pub fn mustLower(air: Air, inst: Air.Inst.Index, ip: *const InternPool) bool { const data = air.instructions.items(.data)[@intFromEnum(inst)]; return switch (air.instructions.items(.tag)[@intFromEnum(inst)]) { .arg, + .assembly, .block, .loop, .repeat, @@ -1879,14 +1880,6 @@ pub fn mustLower(air: Air, inst: Air.Inst.Index, ip: *const InternPool) bool { .work_group_id, => false, - .assembly => { - const extra = air.extraData(Air.Asm, data.ty_pl.payload); - const is_volatile = @as(u1, @truncate(extra.data.flags >> 31)) != 0; - return is_volatile or if (extra.data.outputs_len == 1) - @as(Air.Inst.Ref, @enumFromInt(air.extra[extra.end])) != .none - else - extra.data.outputs_len > 1; - }, .load => air.typeOf(data.ty_op.operand, ip).isVolatilePtrIp(ip), .slice_elem_val, .ptr_elem_val => air.typeOf(data.bin_op.lhs, ip).isVolatilePtrIp(ip), .atomic_load => switch (data.atomic_load.order) { From 5c2e300f427efd76f52aefdaa8ad7fb85bb1d2d5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20R=C3=B8nne=20Petersen?= Date: Thu, 31 Oct 2024 09:55:31 +0100 Subject: [PATCH 06/16] Air: Fix mustLower() to consider volatile for a handful of instructions. These can all potentially operate on volatile pointers. --- src/Air.zig | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/Air.zig b/src/Air.zig index f7d8699b73..a1247496e3 100644 --- a/src/Air.zig +++ b/src/Air.zig @@ -1817,12 +1817,8 @@ pub fn mustLower(air: Air, inst: Air.Inst.Index, ip: *const InternPool) bool { .cmp_vector_optimized, .is_null, .is_non_null, - .is_null_ptr, - .is_non_null_ptr, .is_err, .is_non_err, - .is_err_ptr, - .is_non_err_ptr, .bool_and, .bool_or, .fptrunc, @@ -1835,7 +1831,6 @@ pub fn mustLower(air: Air, inst: Air.Inst.Index, ip: *const InternPool) bool { .unwrap_errunion_payload, .unwrap_errunion_err, .unwrap_errunion_payload_ptr, - .unwrap_errunion_err_ptr, .wrap_errunion_payload, .wrap_errunion_err, .struct_field_ptr, @@ -1880,7 +1875,8 @@ pub fn mustLower(air: Air, inst: Air.Inst.Index, ip: *const InternPool) bool { .work_group_id, => false, - .load => air.typeOf(data.ty_op.operand, ip).isVolatilePtrIp(ip), + .is_non_null_ptr, .is_null_ptr, .is_non_err_ptr, .is_err_ptr => air.typeOf(data.un_op, ip).isVolatilePtrIp(ip), + .load, .unwrap_errunion_err_ptr => air.typeOf(data.ty_op.operand, ip).isVolatilePtrIp(ip), .slice_elem_val, .ptr_elem_val => air.typeOf(data.bin_op.lhs, ip).isVolatilePtrIp(ip), .atomic_load => switch (data.atomic_load.order) { .unordered, .monotonic => air.typeOf(data.atomic_load.ptr, ip).isVolatilePtrIp(ip), From e95e7651ecd988d4955a46c7adfea29c0996c62f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20R=C3=B8nne=20Petersen?= Date: Thu, 31 Oct 2024 09:18:46 +0100 Subject: [PATCH 07/16] llvm: Set null_pointer_is_valid attribute when accessing allowzero pointers. This informs optimization passes that they shouldn't assume that a load from a null pointer invokes undefined behavior. Closes #15816. --- src/codegen/llvm.zig | 91 ++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 84 insertions(+), 7 deletions(-) diff --git a/src/codegen/llvm.zig b/src/codegen/llvm.zig index 891cc0dc52..0c6aaa66fb 100644 --- a/src/codegen/llvm.zig +++ b/src/codegen/llvm.zig @@ -1419,8 +1419,6 @@ pub const Object = struct { } } - function_index.setAttributes(try attributes.finish(&o.builder), &o.builder); - const file, const subprogram = if (!wip.strip) debug_info: { const file = try o.getDebugFile(file_scope); @@ -1517,6 +1515,17 @@ pub const Object = struct { else => |e| return e, }; + // If we saw any loads or stores involving `allowzero` pointers, we need to mark the whole + // function as considering null pointers valid so that LLVM's optimizers don't remove these + // operations on the assumption that they're undefined behavior. + if (fg.allowzero_access) { + try attributes.addFnAttr(.null_pointer_is_valid, &o.builder); + } else { + _ = try attributes.removeFnAttr(.null_pointer_is_valid); + } + + function_index.setAttributes(try attributes.finish(&o.builder), &o.builder); + if (fg.fuzz) |*f| { { const array_llvm_ty = try o.builder.arrayType(f.pcs.items.len, .i8); @@ -4667,6 +4676,15 @@ pub const FuncGen = struct { disable_intrinsics: bool, + /// Have we seen loads or stores involving `allowzero` pointers? + allowzero_access: bool = false, + + pub fn maybeMarkAllowZeroAccess(self: *FuncGen, info: InternPool.Key.PtrType) void { + // LLVM already considers null pointers to be valid in non-generic address spaces, so avoid + // pessimizing optimization for functions with accesses to such pointers. + if (info.flags.address_space == .generic and info.flags.is_allowzero) self.allowzero_access = true; + } + const Fuzz = struct { counters_variable: Builder.Variable.Index, pcs: std.ArrayListUnmanaged(Builder.Constant), @@ -6206,6 +6224,9 @@ pub const FuncGen = struct { const body: []const Air.Inst.Index = @ptrCast(self.air.extra[extra.end..][0..extra.data.body_len]); const err_union_ty = self.typeOf(extra.data.ptr).childType(zcu); const is_unused = self.liveness.isUnused(inst); + + self.maybeMarkAllowZeroAccess(self.typeOf(extra.data.ptr).ptrInfo(zcu)); + return lowerTry(self, err_union_ptr, body, err_union_ty, true, true, is_unused, err_cold); } @@ -6751,10 +6772,14 @@ pub const FuncGen = struct { if (self.canElideLoad(body_tail)) return ptr; + self.maybeMarkAllowZeroAccess(slice_ty.ptrInfo(zcu)); + const elem_alignment = elem_ty.abiAlignment(zcu).toLlvm(); return self.loadByRef(ptr, elem_ty, elem_alignment, .normal); } + self.maybeMarkAllowZeroAccess(slice_ty.ptrInfo(zcu)); + return self.load(ptr, slice_ty); } @@ -6824,10 +6849,15 @@ pub const FuncGen = struct { &.{rhs}, ""); if (isByRef(elem_ty, zcu)) { if (self.canElideLoad(body_tail)) return ptr; + + self.maybeMarkAllowZeroAccess(ptr_ty.ptrInfo(zcu)); + const elem_alignment = elem_ty.abiAlignment(zcu).toLlvm(); return self.loadByRef(ptr, elem_ty, elem_alignment, .normal); } + self.maybeMarkAllowZeroAccess(ptr_ty.ptrInfo(zcu)); + return self.load(ptr, ptr_ty); } @@ -7235,6 +7265,8 @@ pub const FuncGen = struct { }), } + self.maybeMarkAllowZeroAccess(output_ty.ptrInfo(zcu)); + // Pass any non-return outputs indirectly, if the constraint accepts a memory location is_indirect.* = constraintAllowsMemory(constraint); if (is_indirect.*) { @@ -7341,10 +7373,11 @@ pub const FuncGen = struct { // In the case of indirect inputs, LLVM requires the callsite to have // an elementtype() attribute. - llvm_param_attrs[llvm_param_i] = if (constraint[0] == '*') - try o.lowerPtrElemTy(if (is_by_ref) arg_ty else arg_ty.childType(zcu)) - else - .none; + llvm_param_attrs[llvm_param_i] = if (constraint[0] == '*') blk: { + if (!is_by_ref) self.maybeMarkAllowZeroAccess(arg_ty.ptrInfo(zcu)); + + break :blk try o.lowerPtrElemTy(if (is_by_ref) arg_ty else arg_ty.childType(zcu)); + } else .none; llvm_param_i += 1; total_i += 1; @@ -7530,7 +7563,6 @@ pub const FuncGen = struct { if (output != .none) { const output_ptr = try self.resolveInst(output); const output_ptr_ty = self.typeOf(output); - const alignment = output_ptr_ty.ptrAlignment(zcu).toLlvm(); _ = try self.wip.store(.normal, output_value, output_ptr, alignment); } else { @@ -7557,6 +7589,9 @@ pub const FuncGen = struct { const optional_ty = if (operand_is_ptr) operand_ty.childType(zcu) else operand_ty; const optional_llvm_ty = try o.lowerType(optional_ty); const payload_ty = optional_ty.optionalChild(zcu); + + if (operand_is_ptr) self.maybeMarkAllowZeroAccess(operand_ty.ptrInfo(zcu)); + if (optional_ty.optionalReprIsPayload(zcu)) { const loaded = if (operand_is_ptr) try self.wip.load(.normal, optional_llvm_ty, operand, .default, "") @@ -7613,6 +7648,8 @@ pub const FuncGen = struct { return val.toValue(); } + if (operand_is_ptr) self.maybeMarkAllowZeroAccess(operand_ty.ptrInfo(zcu)); + if (!payload_ty.hasRuntimeBitsIgnoreComptime(zcu)) { const loaded = if (operand_is_ptr) try self.wip.load(.normal, try o.lowerType(err_union_ty), operand, .default, "") @@ -7664,6 +7701,8 @@ pub const FuncGen = struct { const payload_ty = optional_ty.optionalChild(zcu); const non_null_bit = try o.builder.intValue(.i8, 1); if (!payload_ty.hasRuntimeBitsIgnoreComptime(zcu)) { + self.maybeMarkAllowZeroAccess(self.typeOf(ty_op.operand).ptrInfo(zcu)); + // We have a pointer to a i8. We need to set it to 1 and then return the same pointer. _ = try self.wip.store(.normal, non_null_bit, operand, .default); return operand; @@ -7677,6 +7716,9 @@ pub const FuncGen = struct { // First set the non-null bit. const optional_llvm_ty = try o.lowerType(optional_ty); const non_null_ptr = try self.wip.gepStruct(optional_llvm_ty, operand, 1, ""); + + self.maybeMarkAllowZeroAccess(self.typeOf(ty_op.operand).ptrInfo(zcu)); + // TODO set alignment on this store _ = try self.wip.store(.normal, non_null_bit, non_null_ptr, .default); @@ -7767,12 +7809,17 @@ pub const FuncGen = struct { const payload_ty = err_union_ty.errorUnionPayload(zcu); if (!payload_ty.hasRuntimeBitsIgnoreComptime(zcu)) { if (!operand_is_ptr) return operand; + + self.maybeMarkAllowZeroAccess(operand_ty.ptrInfo(zcu)); + return self.wip.load(.normal, error_type, operand, .default, ""); } const offset = try errUnionErrorOffset(payload_ty, pt); if (operand_is_ptr or isByRef(err_union_ty, zcu)) { + if (operand_is_ptr) self.maybeMarkAllowZeroAccess(operand_ty.ptrInfo(zcu)); + const err_union_llvm_ty = try o.lowerType(err_union_ty); const err_field_ptr = try self.wip.gepStruct(err_union_llvm_ty, operand, offset, ""); return self.wip.load(.normal, error_type, err_field_ptr, .default, ""); @@ -7792,11 +7839,15 @@ pub const FuncGen = struct { const payload_ty = err_union_ty.errorUnionPayload(zcu); const non_error_val = try o.builder.intValue(try o.errorIntType(), 0); if (!payload_ty.hasRuntimeBitsIgnoreComptime(zcu)) { + self.maybeMarkAllowZeroAccess(self.typeOf(ty_op.operand).ptrInfo(zcu)); + _ = try self.wip.store(.normal, non_error_val, operand, .default); return operand; } const err_union_llvm_ty = try o.lowerType(err_union_ty); { + self.maybeMarkAllowZeroAccess(self.typeOf(ty_op.operand).ptrInfo(zcu)); + const err_int_ty = try pt.errorIntType(); const error_alignment = err_int_ty.abiAlignment(zcu).toLlvm(); const error_offset = try errUnionErrorOffset(payload_ty, pt); @@ -8017,6 +8068,8 @@ pub const FuncGen = struct { const index = try self.resolveInst(extra.lhs); const operand = try self.resolveInst(extra.rhs); + self.maybeMarkAllowZeroAccess(vector_ptr_ty.ptrInfo(zcu)); + const access_kind: Builder.MemoryAccessKind = if (vector_ptr_ty.isVolatilePtr(zcu)) .@"volatile" else .normal; const elem_llvm_ty = try o.lowerType(vector_ptr_ty.childType(zcu)); @@ -9482,6 +9535,8 @@ pub const FuncGen = struct { return .none; } + self.maybeMarkAllowZeroAccess(ptr_info); + const len = try o.builder.intValue(try o.lowerType(Type.usize), operand_ty.abiSize(zcu)); _ = try self.wip.callMemSet( dest_ptr, @@ -9497,6 +9552,8 @@ pub const FuncGen = struct { return .none; } + self.maybeMarkAllowZeroAccess(ptr_ty.ptrInfo(zcu)); + const src_operand = try self.resolveInst(bin_op.rhs); try self.store(dest_ptr, ptr_ty, src_operand, .none); return .none; @@ -9539,6 +9596,9 @@ pub const FuncGen = struct { if (!canElideLoad(fg, body_tail)) break :elide; return ptr; } + + fg.maybeMarkAllowZeroAccess(ptr_info); + return fg.load(ptr, ptr_ty); } @@ -9598,6 +9658,8 @@ pub const FuncGen = struct { new_value = try self.wip.conv(signedness, new_value, llvm_abi_ty, ""); } + self.maybeMarkAllowZeroAccess(ptr_ty.ptrInfo(zcu)); + const result = try self.wip.cmpxchg( kind, if (ptr_ty.isVolatilePtr(zcu)) .@"volatile" else .normal, @@ -9649,6 +9711,8 @@ pub const FuncGen = struct { if (ptr_ty.isVolatilePtr(zcu)) .@"volatile" else .normal; const ptr_alignment = ptr_ty.ptrAlignment(zcu).toLlvm(); + self.maybeMarkAllowZeroAccess(ptr_ty.ptrInfo(zcu)); + if (llvm_abi_ty != .none) { // operand needs widening and truncating or bitcasting. return self.wip.cast(if (is_float) .bitcast else .trunc, try self.wip.atomicrmw( @@ -9712,6 +9776,8 @@ pub const FuncGen = struct { if (info.flags.is_volatile) .@"volatile" else .normal; const elem_llvm_ty = try o.lowerType(elem_ty); + self.maybeMarkAllowZeroAccess(info); + if (llvm_abi_ty != .none) { // operand needs widening and truncating const loaded = try self.wip.loadAtomic( @@ -9761,6 +9827,9 @@ pub const FuncGen = struct { "", ); } + + self.maybeMarkAllowZeroAccess(ptr_ty.ptrInfo(zcu)); + try self.store(ptr, ptr_ty, element, ordering); return .none; } @@ -9778,6 +9847,8 @@ pub const FuncGen = struct { const access_kind: Builder.MemoryAccessKind = if (ptr_ty.isVolatilePtr(zcu)) .@"volatile" else .normal; + self.maybeMarkAllowZeroAccess(ptr_ty.ptrInfo(zcu)); + if (try self.air.value(bin_op.rhs, pt)) |elem_val| { if (elem_val.isUndefDeep(zcu)) { // Even if safety is disabled, we still emit a memset to undefined since it conveys @@ -9916,6 +9987,9 @@ pub const FuncGen = struct { const access_kind: Builder.MemoryAccessKind = if (src_ptr_ty.isVolatilePtr(zcu) or dest_ptr_ty.isVolatilePtr(zcu)) .@"volatile" else .normal; + self.maybeMarkAllowZeroAccess(dest_ptr_ty.ptrInfo(zcu)); + self.maybeMarkAllowZeroAccess(src_ptr_ty.ptrInfo(zcu)); + _ = try self.wip.callMemCpy( dest_ptr, dest_ptr_ty.ptrAlignment(zcu).toLlvm(), @@ -9962,6 +10036,9 @@ pub const FuncGen = struct { const un_ty = self.typeOf(bin_op.lhs).childType(zcu); const layout = un_ty.unionGetLayout(zcu); if (layout.tag_size == 0) return .none; + + self.maybeMarkAllowZeroAccess(self.typeOf(bin_op.lhs).ptrInfo(zcu)); + const union_ptr = try self.resolveInst(bin_op.lhs); const new_tag = try self.resolveInst(bin_op.rhs); if (layout.payload_size == 0) { From e9ae9a5fc45a6ee85d988372a4d150032f52ef4c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20R=C3=B8nne=20Petersen?= Date: Thu, 31 Oct 2024 09:00:46 +0100 Subject: [PATCH 08/16] llvm: Don't set nonnull attribute on allowzero slices. --- src/codegen/llvm.zig | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/codegen/llvm.zig b/src/codegen/llvm.zig index 0c6aaa66fb..0bbc3aee46 100644 --- a/src/codegen/llvm.zig +++ b/src/codegen/llvm.zig @@ -1341,7 +1341,7 @@ pub const Object = struct { try attributes.addParamAttr(llvm_arg_i, .@"noalias", &o.builder); } } - if (param_ty.zigTypeTag(zcu) != .optional) { + if (param_ty.zigTypeTag(zcu) != .optional and !ptr_info.flags.is_allowzero) { try attributes.addParamAttr(llvm_arg_i, .nonnull, &o.builder); } if (ptr_info.flags.is_const) { @@ -5410,7 +5410,7 @@ pub const FuncGen = struct { try attributes.addParamAttr(llvm_arg_i, .@"noalias", &o.builder); } } - if (param_ty.zigTypeTag(zcu) != .optional) { + if (param_ty.zigTypeTag(zcu) != .optional and !ptr_info.flags.is_allowzero) { try attributes.addParamAttr(llvm_arg_i, .nonnull, &o.builder); } if (ptr_info.flags.is_const) { From 427810f3edea1beba77e3f84dd695dd0a8c35f00 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20R=C3=B8nne=20Petersen?= Date: Thu, 31 Oct 2024 09:25:16 +0100 Subject: [PATCH 09/16] llvm: Don't set nonnull attribute on pointers in non-generic address spaces. LLVM considers null pointers to be valid for such address spaces. --- src/codegen/llvm.zig | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/codegen/llvm.zig b/src/codegen/llvm.zig index 0bbc3aee46..c74f54b461 100644 --- a/src/codegen/llvm.zig +++ b/src/codegen/llvm.zig @@ -1341,7 +1341,10 @@ pub const Object = struct { try attributes.addParamAttr(llvm_arg_i, .@"noalias", &o.builder); } } - if (param_ty.zigTypeTag(zcu) != .optional and !ptr_info.flags.is_allowzero) { + if (param_ty.zigTypeTag(zcu) != .optional and + !ptr_info.flags.is_allowzero and + ptr_info.flags.address_space == .generic) + { try attributes.addParamAttr(llvm_arg_i, .nonnull, &o.builder); } if (ptr_info.flags.is_const) { @@ -4358,7 +4361,10 @@ pub const Object = struct { try attributes.addParamAttr(llvm_arg_i, .@"noalias", &o.builder); } } - if (!param_ty.isPtrLikeOptional(zcu) and !ptr_info.flags.is_allowzero) { + if (!param_ty.isPtrLikeOptional(zcu) and + !ptr_info.flags.is_allowzero and + ptr_info.flags.address_space == .generic) + { try attributes.addParamAttr(llvm_arg_i, .nonnull, &o.builder); } switch (fn_info.cc) { @@ -5410,7 +5416,10 @@ pub const FuncGen = struct { try attributes.addParamAttr(llvm_arg_i, .@"noalias", &o.builder); } } - if (param_ty.zigTypeTag(zcu) != .optional and !ptr_info.flags.is_allowzero) { + if (param_ty.zigTypeTag(zcu) != .optional and + !ptr_info.flags.is_allowzero and + ptr_info.flags.address_space == .generic) + { try attributes.addParamAttr(llvm_arg_i, .nonnull, &o.builder); } if (ptr_info.flags.is_const) { From 44bf64a7099a825394e686a78abd3d465c7ec3f7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20R=C3=B8nne=20Petersen?= Date: Sat, 2 Nov 2024 05:44:19 +0100 Subject: [PATCH 10/16] llvm: Fix a bunch of volatile semantics violations. Also fix some cases where we were being overzealous in applying volatile. --- src/codegen/llvm.zig | 108 +++++++++++++++++++++++++++++-------------- 1 file changed, 74 insertions(+), 34 deletions(-) diff --git a/src/codegen/llvm.zig b/src/codegen/llvm.zig index c74f54b461..050e47b68a 100644 --- a/src/codegen/llvm.zig +++ b/src/codegen/llvm.zig @@ -5546,7 +5546,7 @@ pub const FuncGen = struct { ptr_ty.ptrAlignment(zcu).toLlvm(), try o.builder.intValue(.i8, 0xaa), len, - if (ptr_ty.isVolatilePtr(zcu)) .@"volatile" else .normal, + .normal, self.disable_intrinsics, ); const owner_mod = self.ng.ownerModule(); @@ -5781,8 +5781,8 @@ pub const FuncGen = struct { // of optionals that are not pointers. const is_by_ref = isByRef(scalar_ty, zcu); const opt_llvm_ty = try o.lowerType(scalar_ty); - const lhs_non_null = try self.optCmpNull(.ne, opt_llvm_ty, lhs, is_by_ref); - const rhs_non_null = try self.optCmpNull(.ne, opt_llvm_ty, rhs, is_by_ref); + const lhs_non_null = try self.optCmpNull(.ne, opt_llvm_ty, lhs, is_by_ref, .normal); + const rhs_non_null = try self.optCmpNull(.ne, opt_llvm_ty, rhs, is_by_ref, .normal); const llvm_i2 = try o.builder.intType(2); const lhs_non_null_i2 = try self.wip.cast(.zext, lhs_non_null, llvm_i2, ""); const rhs_non_null_i2 = try self.wip.cast(.zext, rhs_non_null, llvm_i2, ""); @@ -6259,10 +6259,13 @@ pub const FuncGen = struct { if (!err_union_ty.errorUnionSet(zcu).errorSetIsEmpty(zcu)) { const loaded = loaded: { + const access_kind: Builder.MemoryAccessKind = + if (err_union_ty.isVolatilePtr(zcu)) .@"volatile" else .normal; + if (!payload_has_bits) { // TODO add alignment to this load break :loaded if (operand_is_ptr) - try fg.wip.load(.normal, error_type, err_union, .default, "") + try fg.wip.load(access_kind, error_type, err_union, .default, "") else err_union; } @@ -6272,7 +6275,7 @@ pub const FuncGen = struct { try fg.wip.gepStruct(err_union_llvm_ty, err_union, err_field_index, ""); // TODO add alignment to this load break :loaded try fg.wip.load( - .normal, + if (operand_is_ptr) access_kind else .normal, error_type, err_field_ptr, .default, @@ -6784,7 +6787,7 @@ pub const FuncGen = struct { self.maybeMarkAllowZeroAccess(slice_ty.ptrInfo(zcu)); const elem_alignment = elem_ty.abiAlignment(zcu).toLlvm(); - return self.loadByRef(ptr, elem_ty, elem_alignment, .normal); + return self.loadByRef(ptr, elem_ty, elem_alignment, if (slice_ty.isVolatilePtr(zcu)) .@"volatile" else .normal); } self.maybeMarkAllowZeroAccess(slice_ty.ptrInfo(zcu)); @@ -6862,7 +6865,7 @@ pub const FuncGen = struct { self.maybeMarkAllowZeroAccess(ptr_ty.ptrInfo(zcu)); const elem_alignment = elem_ty.abiAlignment(zcu).toLlvm(); - return self.loadByRef(ptr, elem_ty, elem_alignment, .normal); + return self.loadByRef(ptr, elem_ty, elem_alignment, if (ptr_ty.isVolatilePtr(zcu)) .@"volatile" else .normal); } self.maybeMarkAllowZeroAccess(ptr_ty.ptrInfo(zcu)); @@ -7409,7 +7412,13 @@ pub const FuncGen = struct { llvm_param_types[llvm_param_i] = llvm_rw_val.typeOfWip(&self.wip); } else { const alignment = rw_ty.abiAlignment(zcu).toLlvm(); - const loaded = try self.wip.load(.normal, llvm_elem_ty, llvm_rw_val, alignment, ""); + const loaded = try self.wip.load( + if (rw_ty.isVolatilePtr(zcu)) .@"volatile" else .normal, + llvm_elem_ty, + llvm_rw_val, + alignment, + "", + ); llvm_param_values[llvm_param_i] = loaded; llvm_param_types[llvm_param_i] = llvm_elem_ty; } @@ -7573,7 +7582,12 @@ pub const FuncGen = struct { const output_ptr = try self.resolveInst(output); const output_ptr_ty = self.typeOf(output); const alignment = output_ptr_ty.ptrAlignment(zcu).toLlvm(); - _ = try self.wip.store(.normal, output_value, output_ptr, alignment); + _ = try self.wip.store( + if (output_ptr_ty.isVolatilePtr(zcu)) .@"volatile" else .normal, + output_value, + output_ptr, + alignment, + ); } else { ret_val = output_value; } @@ -7599,11 +7613,14 @@ pub const FuncGen = struct { const optional_llvm_ty = try o.lowerType(optional_ty); const payload_ty = optional_ty.optionalChild(zcu); + const access_kind: Builder.MemoryAccessKind = + if (operand_is_ptr and operand_ty.isVolatilePtr(zcu)) .@"volatile" else .normal; + if (operand_is_ptr) self.maybeMarkAllowZeroAccess(operand_ty.ptrInfo(zcu)); if (optional_ty.optionalReprIsPayload(zcu)) { const loaded = if (operand_is_ptr) - try self.wip.load(.normal, optional_llvm_ty, operand, .default, "") + try self.wip.load(access_kind, optional_llvm_ty, operand, .default, "") else operand; if (payload_ty.isSlice(zcu)) { @@ -7621,14 +7638,14 @@ pub const FuncGen = struct { if (!payload_ty.hasRuntimeBitsIgnoreComptime(zcu)) { const loaded = if (operand_is_ptr) - try self.wip.load(.normal, optional_llvm_ty, operand, .default, "") + try self.wip.load(access_kind, optional_llvm_ty, operand, .default, "") else operand; return self.wip.icmp(cond, loaded, try o.builder.intValue(.i8, 0), ""); } const is_by_ref = operand_is_ptr or isByRef(optional_ty, zcu); - return self.optCmpNull(cond, optional_llvm_ty, operand, is_by_ref); + return self.optCmpNull(cond, optional_llvm_ty, operand, is_by_ref, access_kind); } fn airIsErr( @@ -7648,6 +7665,9 @@ pub const FuncGen = struct { const error_type = try o.errorIntType(); const zero = try o.builder.intValue(error_type, 0); + const access_kind: Builder.MemoryAccessKind = + if (operand_is_ptr and operand_ty.isVolatilePtr(zcu)) .@"volatile" else .normal; + if (err_union_ty.errorUnionSet(zcu).errorSetIsEmpty(zcu)) { const val: Builder.Constant = switch (cond) { .eq => .true, // 0 == 0 @@ -7661,7 +7681,7 @@ pub const FuncGen = struct { if (!payload_ty.hasRuntimeBitsIgnoreComptime(zcu)) { const loaded = if (operand_is_ptr) - try self.wip.load(.normal, try o.lowerType(err_union_ty), operand, .default, "") + try self.wip.load(access_kind, try o.lowerType(err_union_ty), operand, .default, "") else operand; return self.wip.icmp(cond, loaded, zero, ""); @@ -7673,7 +7693,7 @@ pub const FuncGen = struct { const err_union_llvm_ty = try o.lowerType(err_union_ty); const err_field_ptr = try self.wip.gepStruct(err_union_llvm_ty, operand, err_field_index, ""); - break :loaded try self.wip.load(.normal, error_type, err_field_ptr, .default, ""); + break :loaded try self.wip.load(access_kind, error_type, err_field_ptr, .default, ""); } else try self.wip.extractValue(operand, &.{err_field_index}, ""); return self.wip.icmp(cond, loaded, zero, ""); } @@ -7706,14 +7726,19 @@ pub const FuncGen = struct { const zcu = pt.zcu; const ty_op = self.air.instructions.items(.data)[@intFromEnum(inst)].ty_op; const operand = try self.resolveInst(ty_op.operand); - const optional_ty = self.typeOf(ty_op.operand).childType(zcu); + const optional_ptr_ty = self.typeOf(ty_op.operand); + const optional_ty = optional_ptr_ty.childType(zcu); const payload_ty = optional_ty.optionalChild(zcu); const non_null_bit = try o.builder.intValue(.i8, 1); + + const access_kind: Builder.MemoryAccessKind = + if (optional_ptr_ty.isVolatilePtr(zcu)) .@"volatile" else .normal; + if (!payload_ty.hasRuntimeBitsIgnoreComptime(zcu)) { - self.maybeMarkAllowZeroAccess(self.typeOf(ty_op.operand).ptrInfo(zcu)); + self.maybeMarkAllowZeroAccess(optional_ptr_ty.ptrInfo(zcu)); // We have a pointer to a i8. We need to set it to 1 and then return the same pointer. - _ = try self.wip.store(.normal, non_null_bit, operand, .default); + _ = try self.wip.store(access_kind, non_null_bit, operand, .default); return operand; } if (optional_ty.optionalReprIsPayload(zcu)) { @@ -7726,10 +7751,10 @@ pub const FuncGen = struct { const optional_llvm_ty = try o.lowerType(optional_ty); const non_null_ptr = try self.wip.gepStruct(optional_llvm_ty, operand, 1, ""); - self.maybeMarkAllowZeroAccess(self.typeOf(ty_op.operand).ptrInfo(zcu)); + self.maybeMarkAllowZeroAccess(optional_ptr_ty.ptrInfo(zcu)); // TODO set alignment on this store - _ = try self.wip.store(.normal, non_null_bit, non_null_ptr, .default); + _ = try self.wip.store(access_kind, non_null_bit, non_null_ptr, .default); // Then return the payload pointer (only if it's used). if (self.liveness.isUnused(inst)) return .none; @@ -7815,13 +7840,16 @@ pub const FuncGen = struct { } } + const access_kind: Builder.MemoryAccessKind = + if (operand_is_ptr and operand_ty.isVolatilePtr(zcu)) .@"volatile" else .normal; + const payload_ty = err_union_ty.errorUnionPayload(zcu); if (!payload_ty.hasRuntimeBitsIgnoreComptime(zcu)) { if (!operand_is_ptr) return operand; self.maybeMarkAllowZeroAccess(operand_ty.ptrInfo(zcu)); - return self.wip.load(.normal, error_type, operand, .default, ""); + return self.wip.load(access_kind, error_type, operand, .default, ""); } const offset = try errUnionErrorOffset(payload_ty, pt); @@ -7831,7 +7859,7 @@ pub const FuncGen = struct { const err_union_llvm_ty = try o.lowerType(err_union_ty); const err_field_ptr = try self.wip.gepStruct(err_union_llvm_ty, operand, offset, ""); - return self.wip.load(.normal, error_type, err_field_ptr, .default, ""); + return self.wip.load(access_kind, error_type, err_field_ptr, .default, ""); } return self.wip.extractValue(operand, &.{offset}, ""); @@ -7843,26 +7871,31 @@ pub const FuncGen = struct { const zcu = pt.zcu; const ty_op = self.air.instructions.items(.data)[@intFromEnum(inst)].ty_op; const operand = try self.resolveInst(ty_op.operand); - const err_union_ty = self.typeOf(ty_op.operand).childType(zcu); + const err_union_ptr_ty = self.typeOf(ty_op.operand); + const err_union_ty = err_union_ptr_ty.childType(zcu); const payload_ty = err_union_ty.errorUnionPayload(zcu); const non_error_val = try o.builder.intValue(try o.errorIntType(), 0); - if (!payload_ty.hasRuntimeBitsIgnoreComptime(zcu)) { - self.maybeMarkAllowZeroAccess(self.typeOf(ty_op.operand).ptrInfo(zcu)); - _ = try self.wip.store(.normal, non_error_val, operand, .default); + const access_kind: Builder.MemoryAccessKind = + if (err_union_ptr_ty.isVolatilePtr(zcu)) .@"volatile" else .normal; + + if (!payload_ty.hasRuntimeBitsIgnoreComptime(zcu)) { + self.maybeMarkAllowZeroAccess(err_union_ptr_ty.ptrInfo(zcu)); + + _ = try self.wip.store(access_kind, non_error_val, operand, .default); return operand; } const err_union_llvm_ty = try o.lowerType(err_union_ty); { - self.maybeMarkAllowZeroAccess(self.typeOf(ty_op.operand).ptrInfo(zcu)); + self.maybeMarkAllowZeroAccess(err_union_ptr_ty.ptrInfo(zcu)); const err_int_ty = try pt.errorIntType(); const error_alignment = err_int_ty.abiAlignment(zcu).toLlvm(); const error_offset = try errUnionErrorOffset(payload_ty, pt); // First set the non-error value. const non_null_ptr = try self.wip.gepStruct(err_union_llvm_ty, operand, error_offset, ""); - _ = try self.wip.store(.normal, non_error_val, non_null_ptr, error_alignment); + _ = try self.wip.store(access_kind, non_error_val, non_null_ptr, error_alignment); } // Then return the payload pointer (only if it is used). if (self.liveness.isUnused(inst)) return .none; @@ -8079,6 +8112,8 @@ pub const FuncGen = struct { self.maybeMarkAllowZeroAccess(vector_ptr_ty.ptrInfo(zcu)); + // TODO: Emitting a load here is a violation of volatile semantics. Not fixable in general. + // https://github.com/ziglang/zig/issues/18652#issuecomment-2452844908 const access_kind: Builder.MemoryAccessKind = if (vector_ptr_ty.isVolatilePtr(zcu)) .@"volatile" else .normal; const elem_llvm_ty = try o.lowerType(vector_ptr_ty.childType(zcu)); @@ -10042,23 +10077,27 @@ pub const FuncGen = struct { const pt = o.pt; const zcu = pt.zcu; const bin_op = self.air.instructions.items(.data)[@intFromEnum(inst)].bin_op; - const un_ty = self.typeOf(bin_op.lhs).childType(zcu); + const un_ptr_ty = self.typeOf(bin_op.lhs); + const un_ty = un_ptr_ty.childType(zcu); const layout = un_ty.unionGetLayout(zcu); if (layout.tag_size == 0) return .none; - self.maybeMarkAllowZeroAccess(self.typeOf(bin_op.lhs).ptrInfo(zcu)); + const access_kind: Builder.MemoryAccessKind = + if (un_ptr_ty.isVolatilePtr(zcu)) .@"volatile" else .normal; + + self.maybeMarkAllowZeroAccess(un_ptr_ty.ptrInfo(zcu)); const union_ptr = try self.resolveInst(bin_op.lhs); const new_tag = try self.resolveInst(bin_op.rhs); if (layout.payload_size == 0) { // TODO alignment on this store - _ = try self.wip.store(.normal, new_tag, union_ptr, .default); + _ = try self.wip.store(access_kind, new_tag, union_ptr, .default); return .none; } const tag_index = @intFromBool(layout.tag_align.compare(.lt, layout.payload_align)); const tag_field_ptr = try self.wip.gepStruct(try o.lowerType(un_ty), union_ptr, tag_index, ""); // TODO alignment on this store - _ = try self.wip.store(.normal, new_tag, tag_field_ptr, .default); + _ = try self.wip.store(access_kind, new_tag, tag_field_ptr, .default); return .none; } @@ -10955,12 +10994,13 @@ pub const FuncGen = struct { opt_llvm_ty: Builder.Type, opt_handle: Builder.Value, is_by_ref: bool, + access_kind: Builder.MemoryAccessKind, ) Allocator.Error!Builder.Value { const o = self.ng.object; const field = b: { if (is_by_ref) { const field_ptr = try self.wip.gepStruct(opt_llvm_ty, opt_handle, 1, ""); - break :b try self.wip.load(.normal, .i8, field_ptr, .default, ""); + break :b try self.wip.load(access_kind, .i8, field_ptr, .default, ""); } break :b try self.wip.extractValue(opt_handle, &.{1}, ""); }; @@ -11269,7 +11309,7 @@ pub const FuncGen = struct { const vec_elem_ty = try o.lowerType(elem_ty); const vec_ty = try o.builder.vectorType(.normal, info.packed_offset.host_size, vec_elem_ty); - const loaded_vector = try self.wip.load(access_kind, vec_ty, ptr, ptr_alignment, ""); + const loaded_vector = try self.wip.load(.normal, vec_ty, ptr, ptr_alignment, ""); const modified_vector = try self.wip.insertElement(loaded_vector, elem, index_u32, ""); @@ -11282,7 +11322,7 @@ pub const FuncGen = struct { const containing_int_ty = try o.builder.intType(@intCast(info.packed_offset.host_size * 8)); assert(ordering == .none); const containing_int = - try self.wip.load(access_kind, containing_int_ty, ptr, ptr_alignment, ""); + try self.wip.load(.normal, containing_int_ty, ptr, ptr_alignment, ""); const elem_bits = ptr_ty.childType(zcu).bitSize(zcu); const shift_amt = try o.builder.intConst(containing_int_ty, info.packed_offset.bit_offset); // Convert to equally-sized integer type in order to perform the bit From af55b27d5a6fd7af37892eebc01f1c6e9dbaafdd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20R=C3=B8nne=20Petersen?= Date: Thu, 28 Nov 2024 15:11:18 +0100 Subject: [PATCH 11/16] test: Fix incorrect interpretation of -Dtest-filter=... for test-debugger. --- test/src/Debugger.zig | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/test/src/Debugger.zig b/test/src/Debugger.zig index ef82af90d4..ff0f2ae69c 100644 --- a/test/src/Debugger.zig +++ b/test/src/Debugger.zig @@ -2442,8 +2442,10 @@ fn addTest( db_argv2: []const []const u8, expected_output: []const []const u8, ) void { - for (db.options.test_filters) |test_filter| { - if (std.mem.indexOf(u8, name, test_filter)) |_| return; + if (db.options.test_filters.len > 0) { + for (db.options.test_filters) |test_filter| { + if (std.mem.indexOf(u8, name, test_filter) != null) break; + } else return; } if (db.options.test_target_filters.len > 0) { const triple_txt = target.resolved.result.zigTriple(db.b.allocator) catch @panic("OOM"); From fe5dbc247430cc0b7bce4fe7d01ef6c425db0bf2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20R=C3=B8nne=20Petersen?= Date: Thu, 28 Nov 2024 16:15:07 +0100 Subject: [PATCH 12/16] std.Build: Change Step.Compile.no_builtin from bool to ?bool. To be in line with other, similar options. --- lib/std/Build/Step/Compile.zig | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/std/Build/Step/Compile.zig b/lib/std/Build/Step/Compile.zig index e436865108..04e68065c3 100644 --- a/lib/std/Build/Step/Compile.zig +++ b/lib/std/Build/Step/Compile.zig @@ -229,7 +229,7 @@ is_linking_libc: bool = false, /// Computed during make(). is_linking_libcpp: bool = false, -no_builtin: bool = false, +no_builtin: ?bool = null, /// Populated during the make phase when there is a long-lived compiler process. /// Managed by the build runner, not user build script. @@ -1646,8 +1646,8 @@ fn getZigArgs(compile: *Compile, fuzz: bool) ![][]const u8 { } } - if (compile.no_builtin) { - try zig_args.append("-fno-builtin"); + if (compile.no_builtin) |enabled| { + try zig_args.append(if (enabled) "-fbuiltin" else "-fno-builtin"); } if (b.sysroot) |sysroot| { From e63e3f7a7cabb10e86e94682835fe99669a404bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20R=C3=B8nne=20Petersen?= Date: Thu, 28 Nov 2024 16:17:02 +0100 Subject: [PATCH 13/16] test: Add test-llvm-ir step and harness for testing generated LLVM IR. --- build.zig | 5 ++ test/llvm_ir.zig | 6 ++ test/src/LlvmIr.zig | 132 ++++++++++++++++++++++++++++++++++++++++++++ test/tests.zig | 21 +++++++ 4 files changed, 164 insertions(+) create mode 100644 test/llvm_ir.zig create mode 100644 test/src/LlvmIr.zig diff --git a/build.zig b/build.zig index c7987cb677..00c1254f38 100644 --- a/build.zig +++ b/build.zig @@ -550,6 +550,11 @@ pub fn build(b: *std.Build) !void { .skip_non_native = skip_non_native, .skip_libc = skip_libc, })) |test_debugger_step| test_step.dependOn(test_debugger_step); + if (tests.addLlvmIrTests(b, .{ + .enable_llvm = enable_llvm, + .test_filters = test_filters, + .test_target_filters = test_target_filters, + })) |test_llvm_ir_step| test_step.dependOn(test_llvm_ir_step); try addWasiUpdateStep(b, version); diff --git a/test/llvm_ir.zig b/test/llvm_ir.zig new file mode 100644 index 0000000000..127a5f93d4 --- /dev/null +++ b/test/llvm_ir.zig @@ -0,0 +1,6 @@ +pub fn addCases(cases: *tests.LlvmIrContext) void { + _ = cases; +} + +const std = @import("std"); +const tests = @import("tests.zig"); diff --git a/test/src/LlvmIr.zig b/test/src/LlvmIr.zig new file mode 100644 index 0000000000..dfc993b4b5 --- /dev/null +++ b/test/src/LlvmIr.zig @@ -0,0 +1,132 @@ +b: *std.Build, +options: Options, +root_step: *std.Build.Step, + +pub const Options = struct { + enable_llvm: bool, + test_filters: []const []const u8, + test_target_filters: []const []const u8, +}; + +const TestCase = struct { + name: []const u8, + source: []const u8, + check: union(enum) { + matches: []const []const u8, + exact: []const u8, + }, + params: Params, + + pub const Params = struct { + code_model: std.builtin.CodeModel = .default, + dll_export_fns: ?bool = null, + dwarf_format: ?std.dwarf.Format = null, + error_tracing: ?bool = null, + no_builtin: ?bool = null, + omit_frame_pointer: ?bool = null, + // For most cases, we want to test the LLVM IR that we output; we don't want to be in the + // business of testing LLVM's optimization passes. `Debug` gets us the closest to that as it + // disables the vast majority of passes in LLVM. + optimize: std.builtin.OptimizeMode = .Debug, + pic: ?bool = null, + pie: ?bool = null, + red_zone: ?bool = null, + sanitize_thread: ?bool = null, + single_threaded: ?bool = null, + stack_check: ?bool = null, + stack_protector: ?bool = null, + strip: ?bool = null, + target: std.Target.Query = .{}, + unwind_tables: ?std.builtin.UnwindTables = null, + valgrind: ?bool = null, + }; +}; + +pub fn addMatches( + self: *LlvmIr, + name: []const u8, + source: []const u8, + matches: []const []const u8, + params: TestCase.Params, +) void { + self.addCase(.{ + .name = name, + .source = source, + .check = .{ .matches = matches }, + .params = params, + }); +} + +pub fn addExact( + self: *LlvmIr, + name: []const u8, + source: []const u8, + expected: []const []const u8, + params: TestCase.Params, +) void { + self.addCase(.{ + .name = name, + .source = source, + .check = .{ .exact = expected }, + .params = params, + }); +} + +pub fn addCase(self: *LlvmIr, case: TestCase) void { + const target = self.b.resolveTargetQuery(case.params.target); + if (self.options.test_target_filters.len > 0) { + const triple_txt = target.result.zigTriple(self.b.allocator) catch @panic("OOM"); + for (self.options.test_target_filters) |filter| { + if (std.mem.indexOf(u8, triple_txt, filter) != null) break; + } else return; + } + + const name = std.fmt.allocPrint(self.b.allocator, "check llvm-ir {s}", .{case.name}) catch @panic("OOM"); + if (self.options.test_filters.len > 0) { + for (self.options.test_filters) |filter| { + if (std.mem.indexOf(u8, name, filter) != null) break; + } else return; + } + + const obj = self.b.addObject(.{ + .name = "test", + .root_source_file = self.b.addWriteFiles().add("test.zig", case.source), + .use_llvm = true, + + .code_model = case.params.code_model, + .error_tracing = case.params.error_tracing, + .omit_frame_pointer = case.params.omit_frame_pointer, + .optimize = case.params.optimize, + .pic = case.params.pic, + .sanitize_thread = case.params.sanitize_thread, + .single_threaded = case.params.single_threaded, + .strip = case.params.strip, + .target = target, + .unwind_tables = case.params.unwind_tables, + }); + + obj.dll_export_fns = case.params.dll_export_fns; + obj.pie = case.params.pie; + obj.no_builtin = case.params.no_builtin; + + obj.root_module.dwarf_format = case.params.dwarf_format; + obj.root_module.red_zone = case.params.red_zone; + obj.root_module.stack_check = case.params.stack_check; + obj.root_module.stack_protector = case.params.stack_protector; + obj.root_module.valgrind = case.params.valgrind; + + // This is not very sophisticated at the moment. Eventually, we should move towards something + // like LLVM's `FileCheck` utility (https://llvm.org/docs/CommandGuide/FileCheck.html), though + // likely a more simplified version as we probably don't want a full-blown regex engine in the + // standard library... + const check = self.b.addCheckFile(obj.getEmittedLlvmIr(), switch (case.check) { + .matches => |m| .{ .expected_matches = m }, + .exact => |e| .{ .expected_exact = e }, + }); + check.setName(name); + + self.root_step.dependOn(&check.step); +} + +const LlvmIr = @This(); +const std = @import("std"); diff --git a/test/tests.zig b/test/tests.zig index e6401551da..02446ff7eb 100644 --- a/test/tests.zig +++ b/test/tests.zig @@ -11,6 +11,7 @@ const stack_traces = @import("stack_traces.zig"); const assemble_and_link = @import("assemble_and_link.zig"); const translate_c = @import("translate_c.zig"); const run_translated_c = @import("run_translated_c.zig"); +const llvm_ir = @import("llvm_ir.zig"); // Implementations pub const TranslateCContext = @import("src/TranslateC.zig"); @@ -18,6 +19,7 @@ pub const RunTranslatedCContext = @import("src/RunTranslatedC.zig"); pub const CompareOutputContext = @import("src/CompareOutput.zig"); pub const StackTracesContext = @import("src/StackTrace.zig"); pub const DebuggerContext = @import("src/Debugger.zig"); +pub const LlvmIrContext = @import("src/LlvmIr.zig"); const TestTarget = struct { linkage: ?std.builtin.LinkMode = null, @@ -2125,3 +2127,22 @@ pub fn addIncrementalTests(b: *std.Build, test_step: *Step) !void { test_step.dependOn(&run.step); } } + +pub fn addLlvmIrTests(b: *std.Build, options: LlvmIrContext.Options) ?*Step { + const step = b.step("test-llvm-ir", "Run the LLVM IR tests"); + + if (!options.enable_llvm) { + step.dependOn(&b.addFail("test-llvm-ir requires -Denable-llvm").step); + return null; + } + + var context: LlvmIrContext = .{ + .b = b, + .options = options, + .root_step = step, + }; + + llvm_ir.addCases(&context); + + return step; +} From 74f55175d52d68e80688522f41938ee013c56d71 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20R=C3=B8nne=20Petersen?= Date: Thu, 28 Nov 2024 16:30:49 +0100 Subject: [PATCH 14/16] test: Add some basic LLVM IR tests for atomics, volatile, and allowzero. --- test/llvm_ir.zig | 118 ++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 117 insertions(+), 1 deletion(-) diff --git a/test/llvm_ir.zig b/test/llvm_ir.zig index 127a5f93d4..32221d8287 100644 --- a/test/llvm_ir.zig +++ b/test/llvm_ir.zig @@ -1,5 +1,121 @@ pub fn addCases(cases: *tests.LlvmIrContext) void { - _ = cases; + cases.addMatches("nonnull ptr load", + \\export fn entry(ptr: *i16) i16 { + \\ return ptr.*; + \\} + , &.{ + "ptr nonnull", + "load i16, ptr %0", + }, .{}); + + cases.addMatches("nonnull ptr store", + \\export fn entry(ptr: *i16) void { + \\ ptr.* = 42; + \\} + , &.{ + "ptr nonnull", + "store i16 42, ptr %0", + }, .{}); + + cases.addMatches("unused acquire atomic ptr load", + \\export fn entry(ptr: *i16) void { + \\ _ = @atomicLoad(i16, ptr, .acquire); + \\} + , &.{ + "load atomic i16, ptr %0 acquire", + }, .{}); + + cases.addMatches("unused unordered atomic volatile ptr load", + \\export fn entry(ptr: *volatile i16) void { + \\ _ = @atomicLoad(i16, ptr, .unordered); + \\} + , &.{ + "load atomic volatile i16, ptr %0 unordered", + }, .{}); + + cases.addMatches("unused volatile ptr load", + \\export fn entry(ptr: *volatile i16) void { + \\ _ = ptr.*; + \\} + , &.{ + "load volatile i16, ptr %0", + }, .{}); + + cases.addMatches("dead volatile ptr store", + \\export fn entry(ptr: *volatile i16) void { + \\ ptr.* = 123; + \\ ptr.* = 321; + \\} + , &.{ + "store volatile i16 123, ptr %0", + "store volatile i16 321, ptr %0", + }, .{}); + + cases.addMatches("unused volatile slice load", + \\export fn entry(ptr: *volatile i16) void { + \\ entry2(ptr[0..1]); + \\} + \\fn entry2(ptr: []volatile i16) void { + \\ _ = ptr[0]; + \\} + , &.{ + "load volatile i16, ptr", + }, .{}); + + cases.addMatches("dead volatile slice store", + \\export fn entry(ptr: *volatile i16) void { + \\ entry2(ptr[0..1]); + \\} + \\fn entry2(ptr: []volatile i16) void { + \\ ptr[0] = 123; + \\ ptr[0] = 321; + \\} + , &.{ + "store volatile i16 123, ptr", + "store volatile i16 321, ptr", + }, .{}); + + cases.addMatches("allowzero ptr load", + \\export fn entry(ptr: *allowzero i16) i16 { + \\ return ptr.*; + \\} + , &.{ + "null_pointer_is_valid", + "load i16, ptr %0", + }, .{}); + + cases.addMatches("allowzero ptr store", + \\export fn entry(ptr: *allowzero i16) void { + \\ ptr.* = 42; + \\} + , &.{ + "null_pointer_is_valid", + "store i16 42, ptr %0", + }, .{}); + + cases.addMatches("allowzero slice load", + \\export fn entry(ptr: *allowzero i16) i16 { + \\ return entry2(ptr[0..1]); + \\} + \\fn entry2(ptr: []allowzero i16) i16 { + \\ return ptr[0]; + \\} + , &.{ + "null_pointer_is_valid", + "load i16, ptr", + }, .{}); + + cases.addMatches("allowzero slice store", + \\export fn entry(ptr: *allowzero i16) void { + \\ entry2(ptr[0..1]); + \\} + \\fn entry2(ptr: []allowzero i16) void { + \\ ptr[0] = 42; + \\} + , &.{ + "null_pointer_is_valid", + "store i16 42, ptr", + }, .{}); } const std = @import("std"); From aa7c6dcac1c87d892156daf210620f999d9838f3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20R=C3=B8nne=20Petersen?= Date: Wed, 2 Apr 2025 14:37:30 +0200 Subject: [PATCH 15/16] main: List -f(no-)builtin as per-module options. Contributes to #23424. --- src/main.zig | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main.zig b/src/main.zig index c196f1a4fd..4ced84d469 100644 --- a/src/main.zig +++ b/src/main.zig @@ -471,8 +471,6 @@ const usage_build_generic = \\ -fno-dll-export-fns Force-disable marking exported functions as DLL exports \\ -freference-trace[=num] Show num lines of reference trace per compile error \\ -fno-reference-trace Disable reference trace - \\ -fbuiltin Enable implicit builtin knowledge of functions - \\ -fno-builtin Disable implicit builtin knowledge of functions \\ -ffunction-sections Places each function in a separate section \\ -fno-function-sections All functions go into same section \\ -fdata-sections Places each data in a separate section @@ -534,6 +532,8 @@ const usage_build_generic = \\ -fno-sanitize-thread Disable Thread Sanitizer \\ -ffuzz Enable fuzz testing instrumentation \\ -fno-fuzz Disable fuzz testing instrumentation + \\ -fbuiltin Enable implicit builtin knowledge of functions + \\ -fno-builtin Disable implicit builtin knowledge of functions \\ -funwind-tables Always produce unwind table entries for all functions \\ -fasync-unwind-tables Always produce asynchronous unwind table entries for all functions \\ -fno-unwind-tables Never produce unwind table entries From 9d8adb38a18169a16707acac2812dd6850de99be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alex=20R=C3=B8nne=20Petersen?= Date: Wed, 2 Apr 2025 14:36:21 +0200 Subject: [PATCH 16/16] std.Build: Make no_builtin a property of Module instead of Step.Compile. This reflects how the compiler actually treats it. Closes #23424. --- lib/std/Build/Module.zig | 4 ++++ lib/std/Build/Step/Compile.zig | 6 ------ test/src/LlvmIr.zig | 2 +- test/tests.zig | 2 +- 4 files changed, 6 insertions(+), 8 deletions(-) diff --git a/lib/std/Build/Module.zig b/lib/std/Build/Module.zig index d95da55efa..757f51f933 100644 --- a/lib/std/Build/Module.zig +++ b/lib/std/Build/Module.zig @@ -33,6 +33,7 @@ omit_frame_pointer: ?bool, error_tracing: ?bool, link_libc: ?bool, link_libcpp: ?bool, +no_builtin: ?bool, /// Symbols to be exported when compiling to WebAssembly. export_symbol_names: []const []const u8 = &.{}, @@ -268,6 +269,7 @@ pub const CreateOptions = struct { /// more difficult to obtain stack traces. Has target-dependent effects. omit_frame_pointer: ?bool = null, error_tracing: ?bool = null, + no_builtin: ?bool = null, }; pub const Import = struct { @@ -314,6 +316,7 @@ pub fn init( .omit_frame_pointer = options.omit_frame_pointer, .error_tracing = options.error_tracing, .export_symbol_names = &.{}, + .no_builtin = options.no_builtin, }; m.import_table.ensureUnusedCapacity(allocator, options.imports.len) catch @panic("OOM"); @@ -564,6 +567,7 @@ pub fn appendZigProcessFlags( try addFlag(zig_args, m.valgrind, "-fvalgrind", "-fno-valgrind"); try addFlag(zig_args, m.pic, "-fPIC", "-fno-PIC"); try addFlag(zig_args, m.red_zone, "-mred-zone", "-mno-red-zone"); + try addFlag(zig_args, m.no_builtin, "-fno-builtin", "-fbuiltin"); if (m.sanitize_c) |sc| switch (sc) { .off => try zig_args.append("-fno-sanitize-c"), diff --git a/lib/std/Build/Step/Compile.zig b/lib/std/Build/Step/Compile.zig index 04e68065c3..066167905d 100644 --- a/lib/std/Build/Step/Compile.zig +++ b/lib/std/Build/Step/Compile.zig @@ -229,8 +229,6 @@ is_linking_libc: bool = false, /// Computed during make(). is_linking_libcpp: bool = false, -no_builtin: ?bool = null, - /// Populated during the make phase when there is a long-lived compiler process. /// Managed by the build runner, not user build script. zig_process: ?*Step.ZigProcess, @@ -1646,10 +1644,6 @@ fn getZigArgs(compile: *Compile, fuzz: bool) ![][]const u8 { } } - if (compile.no_builtin) |enabled| { - try zig_args.append(if (enabled) "-fbuiltin" else "-fno-builtin"); - } - if (b.sysroot) |sysroot| { try zig_args.appendSlice(&[_][]const u8{ "--sysroot", sysroot }); } diff --git a/test/src/LlvmIr.zig b/test/src/LlvmIr.zig index dfc993b4b5..f6d39505b9 100644 --- a/test/src/LlvmIr.zig +++ b/test/src/LlvmIr.zig @@ -107,9 +107,9 @@ pub fn addCase(self: *LlvmIr, case: TestCase) void { obj.dll_export_fns = case.params.dll_export_fns; obj.pie = case.params.pie; - obj.no_builtin = case.params.no_builtin; obj.root_module.dwarf_format = case.params.dwarf_format; + obj.root_module.no_builtin = case.params.no_builtin; obj.root_module.red_zone = case.params.red_zone; obj.root_module.stack_check = case.params.stack_check; obj.root_module.stack_protector = case.params.stack_protector; diff --git a/test/tests.zig b/test/tests.zig index 02446ff7eb..39c898cbad 100644 --- a/test/tests.zig +++ b/test/tests.zig @@ -1827,7 +1827,7 @@ pub fn addModuleTests(b: *std.Build, options: ModuleTestOptions) *Step { .zig_lib_dir = b.path("lib"), }); these_tests.linkage = test_target.linkage; - if (options.no_builtin) these_tests.no_builtin = true; + if (options.no_builtin) these_tests.root_module.no_builtin = false; if (options.build_options) |build_options| { these_tests.root_module.addOptions("build_options", build_options); }