Merge pull request #21882 from alexrp/compiler-fixes

compiler: Fix some real and theoretical miscompilations with `allowzero` and `volatile`

Author: alexrp

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);
 

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"),

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 = false,
-
 /// 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) {
-        try zig_args.append("-fno-builtin");
-    }
-
     if (b.sysroot) |sysroot| {
         try zig_args.appendSlice(&[_][]const u8{ "--sysroot", sysroot });
     }

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,
@@ -1816,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,
@@ -1834,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,
@@ -1879,17 +1875,13 @@ 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),
+        .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 => 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.
+        },
     };
 }
 

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),
 
@@ -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.
@@ -7729,9 +7736,12 @@ 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);
+    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) {
@@ -7747,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 = .{
@@ -7793,6 +7802,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 });
 }
 

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 {