-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
x86_64: fix packedStore miscomp by spilling EFLAGS #23256
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
Conversation
There was a problem hiding this 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.
88aadce
to
29d9b16
Compare
What happened in the aarch64 CI? The aarch64 debug build succeeded. |
Can you add a test that doesn't have the |
Fixes ziglang#20113 and ziglang#20581. AND instructions in packedStore clobbers EFLAGS. Bug: ziglang#20113 Bug: ziglang#20581 Signed-off-by: Bingwu Zhang <[email protected]>
Done and rebased. |
Last nit I swear, since test names need to be unique, it should be something more like |
x86_64: fix packedStore miscomp by spilling EFLAGS
Fixes #20113. Fixes #20581. Fixes #22790.
AND instructions in packedStore clobbers EFLAGS.
Tested with the repro from #20113 and #20581.
Behavior test:
1951 passed; 143 skipped; 0 failed.