Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

x86_64: fix packedStore miscomp by spilling EFLAGS #23256

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

xtexx
Copy link
Contributor

@xtexx xtexx commented Mar 15, 2025

Fixes #20113. Fixes #20581.

AND instructions in packedStore clobbers EFLAGS.

Tested with the repro from #20113 and #20581.

xtex@xtex1 ~/s/z/zig (master)> build-host/out/zig run repro.zig -fno-llvm
ret: repro.Data{ .rd = 1, .rl = true }, true, true
xtex@xtex1 ~/s/z/zig (master)> zig version
0.15.0-dev.56+d0911786c
xtex@xtex1 ~/s/z/zig (master)> zig run repro.zig -fno-llvm
ret: repro.Data{ .rd = 1, .rl = false }, false, true
xtex@xtex1 ~/s/z/zig (master)> zig run repro.zig -fllvm
ret: repro.Data{ .rd = 1, .rl = true }, true, true

Behavior test: 1951 passed; 143 skipped; 0 failed.

Copy link
Member

@jacobly0 jacobly0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we keep it simple with something like this, to reduce the impact on the soon-to-be-rewritten-anyway code:

--- a/src/arch/x86_64/CodeGen.zig
+++ b/src/arch/x86_64/CodeGen.zig
@@ -91810,12 +91810,15 @@ fn airStore(self: *CodeGen, inst: Air.Inst.Index, safety: bool) !void {
         const reg_locks = self.register_manager.lockRegsAssumeUnused(3, .{ .rdi, .rsi, .rcx });
         defer for (reg_locks) |lock| self.register_manager.unlockReg(lock);
 
+        const ptr_ty = self.typeOf(bin_op.lhs);
+        const ptr_info = ptr_ty.ptrInfo(zcu);
+        const is_packed = ptr_info.flags.vector_index != .none or ptr_info.packed_offset.host_size > 0;
+        if (is_packed) try self.spillEflagsIfOccupied();
+
         const src_mcv = try self.resolveInst(bin_op.rhs);
         const ptr_mcv = try self.resolveInst(bin_op.lhs);
-        const ptr_ty = self.typeOf(bin_op.lhs);
 
-        const ptr_info = ptr_ty.ptrInfo(zcu);
-        if (ptr_info.flags.vector_index != .none or ptr_info.packed_offset.host_size > 0) {
+        if (is_packed) {
             try self.packedStore(ptr_ty, ptr_mcv, src_mcv);
         } else {
             try self.store(ptr_ty, ptr_mcv, src_mcv, .{ .safety = safety });

This change also requires regression test coverage for the linked issues (in test/behavior, but not in x86_64) to be able to merge and close them.

@ifreund can you check the impact on #22790 when you get a chance to rule out any issues caused by this bug.

Fixes ziglang#20113 and ziglang#20581.

AND instructions in packedStore clobbers EFLAGS.

Bug: ziglang#20113
Bug: ziglang#20581
Signed-off-by: Bingwu Zhang <xtex@aosc.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

x86 backend miscompilation w/ packed structs and bitwise operation x86_64 backend miscomp
2 participants