From cc16ac9314752681300c7e72a4989aeba2ba2579 Mon Sep 17 00:00:00 2001 From: mparadinha Date: Sun, 30 Jan 2022 02:39:37 +0000 Subject: [PATCH 1/4] implement storing to MCValue.memory --- src/arch/x86_64/CodeGen.zig | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/src/arch/x86_64/CodeGen.zig b/src/arch/x86_64/CodeGen.zig index 4d3f899cd5..a60a407beb 100644 --- a/src/arch/x86_64/CodeGen.zig +++ b/src/arch/x86_64/CodeGen.zig @@ -1762,8 +1762,19 @@ fn store(self: *Self, ptr: MCValue, value: MCValue, ptr_ty: Type, value_ty: Type }, } }, - .memory => { - return self.fail("TODO implement storing to MCValue.memory", .{}); + .memory => |addr| { + const reg = try self.copyToTmpRegister(ptr_ty, .{ .memory = addr }); + // mov reg, [reg] + _ = try self.addInst(.{ + .tag = .mov, + .ops = (Mir.Ops{ + .reg1 = reg.to64(), + .reg2 = reg.to64(), + .flags = 0b10, + }).encode(), + .data = .{ .imm = 0 }, + }); + return self.store(.{ .register = reg }, value, ptr_ty, value_ty); }, .stack_offset => { return self.fail("TODO implement storing to MCValue.stack_offset", .{}); From b67b89025cc12785f78a7908e321f8ed0b83afc6 Mon Sep 17 00:00:00 2001 From: mparadinha Date: Sun, 30 Jan 2022 16:02:18 +0000 Subject: [PATCH 2/4] implement store for 8 byte immediates --- src/arch/x86_64/CodeGen.zig | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/src/arch/x86_64/CodeGen.zig b/src/arch/x86_64/CodeGen.zig index a60a407beb..9356cf49ac 100644 --- a/src/arch/x86_64/CodeGen.zig +++ b/src/arch/x86_64/CodeGen.zig @@ -1740,6 +1740,24 @@ fn store(self: *Self, ptr: MCValue, value: MCValue, ptr_ty: Type, value_ty: Type .data = .{ .payload = payload }, }); }, + 8 => { + // TODO: optimization: if the imm is only using the lower + // 4 bytes and can be sign extended we can use a normal mov + // with indirect addressing (mov [reg64], imm32). + + // movabs does not support indirect register addressing + // so we need an extra register and an extra mov. + const tmp_reg = try self.copyToTmpRegister(value_ty, value); + _ = try self.addInst(.{ + .tag = .mov, + .ops = (Mir.Ops{ + .reg1 = reg.to64(), + .reg2 = tmp_reg.to64(), + .flags = 0b10, + }).encode(), + .data = .{ .imm = 0 }, + }); + }, else => { return self.fail("TODO implement set pointee with immediate of ABI size {d}", .{abi_size}); }, From ef4c54ba3836ecb4962f64f10b0b5c12188c9ff4 Mon Sep 17 00:00:00 2001 From: mparadinha Date: Wed, 2 Feb 2022 08:48:00 +0000 Subject: [PATCH 3/4] need to go through the GOT, and use a temporary register --- src/arch/x86_64/CodeGen.zig | 62 ++++++++++++++++++++++++++++++++++--- 1 file changed, 57 insertions(+), 5 deletions(-) diff --git a/src/arch/x86_64/CodeGen.zig b/src/arch/x86_64/CodeGen.zig index 9356cf49ac..e926968e5b 100644 --- a/src/arch/x86_64/CodeGen.zig +++ b/src/arch/x86_64/CodeGen.zig @@ -1781,18 +1781,70 @@ fn store(self: *Self, ptr: MCValue, value: MCValue, ptr_ty: Type, value_ty: Type } }, .memory => |addr| { - const reg = try self.copyToTmpRegister(ptr_ty, .{ .memory = addr }); + // TODO: in case the address fits in an imm32 we can use [ds:imm32] + // instead of wasting an instruction copying the address to a register + + const addr_reg = try self.copyToTmpRegister(ptr_ty, .{ .immediate = addr }); + // to get the actual address of the value we want to modify we have to go through the GOT // mov reg, [reg] _ = try self.addInst(.{ .tag = .mov, .ops = (Mir.Ops{ - .reg1 = reg.to64(), - .reg2 = reg.to64(), - .flags = 0b10, + .reg1 = addr_reg.to64(), + .reg2 = addr_reg.to64(), + .flags = 0b01, }).encode(), .data = .{ .imm = 0 }, }); - return self.store(.{ .register = reg }, value, ptr_ty, value_ty); + + const abi_size = value_ty.abiSize(self.target.*); + switch (value) { + .immediate => |imm| { + const payload = try self.addExtra(Mir.ImmPair{ + .dest_off = 0, + .operand = @intCast(u32, imm), + }); + _ = try self.addInst(.{ + .tag = .mov_mem_imm, + .ops = (Mir.Ops{ + .reg1 = addr_reg.to64(), + .flags = switch (abi_size) { + 1 => 0b00, + 2 => 0b01, + 4 => 0b10, + 8 => flag: { + const top_bits: u32 = @intCast(u32, imm >> 32); + const can_extend = if (value_ty.isUnsignedInt()) + (top_bits == 0) and (imm & 0x8000_0000) == 0 + else + top_bits == 0xffff_ffff; + + if (!can_extend) { + return self.fail("TODO imm64 would get incorrectly sign extended", .{}); + } + break :flag 0b11; + }, + else => { + return self.fail("TODO saving imm to memory for abi_size {}", .{abi_size}); + }, + }, + }).encode(), + .data = .{ .payload = payload }, + }); + }, + .register => |reg| { + _ = try self.addInst(.{ + .tag = .mov, + .ops = (Mir.Ops{ + .reg1 = addr_reg.to64(), + .reg2 = reg, + .flags = 0b10, + }).encode(), + .data = .{ .imm = 0 }, + }); + }, + else => return self.fail("TODO implement storing {} to MCValue.memory", .{value}), + } }, .stack_offset => { return self.fail("TODO implement storing to MCValue.stack_offset", .{}); From f4e0641450032004b9200a4d5fa754e189a123f3 Mon Sep 17 00:00:00 2001 From: Jakub Konka Date: Thu, 3 Feb 2022 14:31:16 +0100 Subject: [PATCH 4/4] x64: use freeze/unfreeze api; TODO for PIE --- src/arch/x86_64/CodeGen.zig | 57 ++++++++++++++++++++++--------------- test/behavior/bugs/1486.zig | 3 -- 2 files changed, 34 insertions(+), 26 deletions(-) diff --git a/src/arch/x86_64/CodeGen.zig b/src/arch/x86_64/CodeGen.zig index c6a2d3f23b..14aebc3724 100644 --- a/src/arch/x86_64/CodeGen.zig +++ b/src/arch/x86_64/CodeGen.zig @@ -1685,6 +1685,7 @@ fn airLoad(self: *Self, inst: Air.Inst.Index) !void { fn store(self: *Self, ptr: MCValue, value: MCValue, ptr_ty: Type, value_ty: Type) InnerError!void { _ = ptr_ty; + const abi_size = value_ty.abiSize(self.target.*); switch (ptr) { .none => unreachable, .undef => unreachable, @@ -1705,6 +1706,9 @@ fn store(self: *Self, ptr: MCValue, value: MCValue, ptr_ty: Type, value_ty: Type return self.fail("TODO implement storing to MCValue.embedded_in_code", .{}); }, .register => |reg| { + self.register_manager.freezeRegs(&.{reg}); + defer self.register_manager.unfreezeRegs(&.{reg}); + switch (value) { .none => unreachable, .undef => unreachable, @@ -1713,7 +1717,6 @@ fn store(self: *Self, ptr: MCValue, value: MCValue, ptr_ty: Type, value_ty: Type .compare_flags_unsigned => unreachable, .compare_flags_signed => unreachable, .immediate => |imm| { - const abi_size = value_ty.abiSize(self.target.*); switch (abi_size) { 1, 2, 4 => { // TODO this is wasteful! @@ -1760,7 +1763,6 @@ fn store(self: *Self, ptr: MCValue, value: MCValue, ptr_ty: Type, value_ty: Type } }, .register => |src_reg| { - const abi_size = value_ty.abiSize(self.target.*); _ = try self.addInst(.{ .tag = .mov, .ops = (Mir.Ops{ @@ -1777,9 +1779,16 @@ fn store(self: *Self, ptr: MCValue, value: MCValue, ptr_ty: Type, value_ty: Type } }, .memory => |addr| { + if (self.bin_file.options.pie) { + return self.fail("TODO implement storing to memory when targeting PIE", .{}); + } + // TODO: in case the address fits in an imm32 we can use [ds:imm32] // instead of wasting an instruction copying the address to a register + if (value.isRegister()) self.register_manager.freezeRegs(&.{value.register}); + defer if (value.isRegister()) self.register_manager.unfreezeRegs(&.{value.register}); + const addr_reg = try self.copyToTmpRegister(ptr_ty, .{ .immediate = addr }); // to get the actual address of the value we want to modify we have to go through the GOT // mov reg, [reg] @@ -1793,37 +1802,39 @@ fn store(self: *Self, ptr: MCValue, value: MCValue, ptr_ty: Type, value_ty: Type .data = .{ .imm = 0 }, }); - const abi_size = value_ty.abiSize(self.target.*); switch (value) { .immediate => |imm| { + if (abi_size > 8) { + return self.fail("TODO saving imm to memory for abi_size {}", .{abi_size}); + } + const payload = try self.addExtra(Mir.ImmPair{ .dest_off = 0, .operand = @intCast(u32, imm), }); + const flags: u2 = switch (abi_size) { + 1 => 0b00, + 2 => 0b01, + 4 => 0b10, + 8 => 0b11, + else => unreachable, + }; + if (flags == 0b11) { + const top_bits: u32 = @intCast(u32, imm >> 32); + const can_extend = if (value_ty.isUnsignedInt()) + (top_bits == 0) and (imm & 0x8000_0000) == 0 + else + top_bits == 0xffff_ffff; + + if (!can_extend) { + return self.fail("TODO imm64 would get incorrectly sign extended", .{}); + } + } _ = try self.addInst(.{ .tag = .mov_mem_imm, .ops = (Mir.Ops{ .reg1 = addr_reg.to64(), - .flags = switch (abi_size) { - 1 => 0b00, - 2 => 0b01, - 4 => 0b10, - 8 => flag: { - const top_bits: u32 = @intCast(u32, imm >> 32); - const can_extend = if (value_ty.isUnsignedInt()) - (top_bits == 0) and (imm & 0x8000_0000) == 0 - else - top_bits == 0xffff_ffff; - - if (!can_extend) { - return self.fail("TODO imm64 would get incorrectly sign extended", .{}); - } - break :flag 0b11; - }, - else => { - return self.fail("TODO saving imm to memory for abi_size {}", .{abi_size}); - }, - }, + .flags = flags, }).encode(), .data = .{ .payload = payload }, }); diff --git a/test/behavior/bugs/1486.zig b/test/behavior/bugs/1486.zig index 53309f5553..8f954a3600 100644 --- a/test/behavior/bugs/1486.zig +++ b/test/behavior/bugs/1486.zig @@ -1,13 +1,10 @@ const std = @import("std"); -const builtin = @import("builtin"); const expect = std.testing.expect; const ptr = &global; var global: usize = 123; test "constant pointer to global variable causes runtime load" { - if (builtin.zig_backend == .stage2_x86_64) return error.SkipZigTest; - global = 1234; try expect(&global == ptr); try expect(ptr.* == 1234);