From 8e00f476851a509d554fe25d08544cebccc91c1d 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] 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 cec888fba9..ae687906a4 100644 --- a/src/codegen/llvm.zig +++ b/src/codegen/llvm.zig @@ -1736,8 +1736,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); @@ -1828,6 +1826,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); @@ -5006,6 +5015,15 @@ pub const FuncGen = struct { sync_scope: Builder.SyncScope, + /// 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), @@ -6567,6 +6585,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); } @@ -7112,10 +7133,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); } @@ -7185,10 +7210,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); } @@ -7590,6 +7620,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.*) { @@ -7696,10 +7728,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; @@ -7881,7 +7914,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 { @@ -7908,6 +7940,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, "") @@ -7964,6 +7999,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, "") @@ -8015,6 +8052,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; @@ -8028,6 +8067,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); @@ -8118,12 +8160,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, ""); @@ -8143,11 +8190,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); @@ -8368,6 +8419,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)); @@ -9720,6 +9773,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, @@ -9734,6 +9789,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; @@ -9776,6 +9833,9 @@ pub const FuncGen = struct { if (!canElideLoad(fg, body_tail)) break :elide; return ptr; } + + fg.maybeMarkAllowZeroAccess(ptr_info); + return fg.load(ptr, ptr_ty); } @@ -9835,6 +9895,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, @@ -9886,6 +9948,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( @@ -9949,6 +10013,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( @@ -9998,6 +10064,9 @@ pub const FuncGen = struct { "", ); } + + self.maybeMarkAllowZeroAccess(ptr_ty.ptrInfo(zcu)); + try self.store(ptr, ptr_ty, element, ordering); return .none; } @@ -10015,6 +10084,8 @@ pub const FuncGen = struct { const access_kind: Builder.MemoryAccessKind = if (ptr_ty.isVolatilePtr(zcu)) .@"volatile" else .normal; + self.maybeMarkAllowZeroAccess(ptr_ty.ptrInfo(zcu)); + // Any WebAssembly runtime will trap when the destination pointer is out-of-bounds, regardless // of the length. This means we need to emit a check where we skip the memset when the length // is 0 as we allow for undefined pointers in 0-sized slices. @@ -10171,6 +10242,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)); + // When bulk-memory is enabled, this will be lowered to WebAssembly's memory.copy instruction. // This instruction will trap on an invalid address, regardless of the length. // For this reason we must add a check for 0-sized slices as its pointer field can be undefined. @@ -10218,6 +10292,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) {