Skip to content

Fix xsave segfaults #1934

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Oct 5, 2025
Merged

Fix xsave segfaults #1934

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions ci/docker/x86_64-unknown-linux-gnu/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,5 +14,3 @@ RUN tar -xJf sde.tar.xz --strip-components=1 -C intel-sde
ENV CARGO_TARGET_X86_64_UNKNOWN_LINUX_GNU_RUNNER="/intel-sde/sde64 \
-cpuid-in /checkout/ci/docker/x86_64-unknown-linux-gnu/cpuid.def \
-rtm-mode full -tsx --"
# These tests fail with SDE as it doesn't support saving register data
ENV STDARCH_TEST_SKIP_FUNCTION="xsave,xsaveopt,xsave64,xsaveopt64"
34 changes: 22 additions & 12 deletions crates/core_arch/src/x86/xsave.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,29 +159,39 @@ pub unsafe fn _xrstors(mem_addr: *const u8, rs_mask: u64) {
xrstors(mem_addr, (rs_mask >> 32) as u32, rs_mask as u32);
}

#[cfg(test)]
pub(crate) use tests::XsaveArea;

#[cfg(test)]
mod tests {
use std::{fmt, prelude::v1::*};
use std::boxed::Box;

use crate::core_arch::x86::*;
use stdarch_test::simd_test;

#[repr(align(64))]
#[derive(Debug)]
struct XsaveArea {
// max size for 256-bit registers is 800 bytes:
// see https://software.intel.com/en-us/node/682996
// max size for 512-bit registers is 2560 bytes:
// FIXME: add source
data: [u8; 2560],
pub(crate) struct XsaveArea {
data: Box<[AlignedArray]>,
}

#[repr(align(64))]
#[derive(Copy, Clone, Debug)]
struct AlignedArray([u8; 64]);

impl XsaveArea {
fn new() -> XsaveArea {
XsaveArea { data: [0; 2560] }
#[target_feature(enable = "xsave")]
pub(crate) fn new() -> XsaveArea {
// `CPUID.(EAX=0DH,ECX=0):ECX` contains the size required to hold all supported xsave
// components. `EBX` contains the size required to hold all xsave components currently
// enabled in `XCR0`. We are using `ECX` to ensure enough space in all scenarios
let CpuidResult { ecx, .. } = unsafe { __cpuid(0x0d) };

XsaveArea {
data: vec![AlignedArray([0; 64]); ecx.div_ceil(64) as usize].into_boxed_slice(),
}
}
fn ptr(&mut self) -> *mut u8 {
self.data.as_mut_ptr()
pub(crate) fn ptr(&mut self) -> *mut u8 {
self.data.as_mut_ptr().cast()
}
}

Expand Down
41 changes: 11 additions & 30 deletions crates/core_arch/src/x86_64/xsave.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,39 +126,20 @@ pub unsafe fn _xrstors64(mem_addr: *const u8, rs_mask: u64) {

#[cfg(test)]
mod tests {
use crate::core_arch::x86_64::xsave;
use std::fmt;
use crate::core_arch::x86::*;
use crate::core_arch::x86_64::*;
use stdarch_test::simd_test;

#[repr(align(64))]
#[derive(Debug)]
struct XsaveArea {
// max size for 256-bit registers is 800 bytes:
// see https://software.intel.com/en-us/node/682996
// max size for 512-bit registers is 2560 bytes:
// FIXME: add source
data: [u8; 2560],
}

impl XsaveArea {
fn new() -> XsaveArea {
XsaveArea { data: [0; 2560] }
}
fn ptr(&mut self) -> *mut u8 {
self.data.as_mut_ptr()
}
}

#[simd_test(enable = "xsave")]
#[cfg_attr(miri, ignore)] // Register saving/restoring is not supported in Miri
unsafe fn test_xsave64() {
let m = 0xFFFFFFFFFFFFFFFF_u64; //< all registers
let mut a = XsaveArea::new();
let mut b = XsaveArea::new();

xsave::_xsave64(a.ptr(), m);
xsave::_xrstor64(a.ptr(), m);
xsave::_xsave64(b.ptr(), m);
_xsave64(a.ptr(), m);
_xrstor64(a.ptr(), m);
_xsave64(b.ptr(), m);
Comment on lines 133 to +142
Copy link
Contributor

Choose a reason for hiding this comment

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

you're re-enabling 4 functions xsave,xsaveopt,xsave64,xsaveopt64, but there are only 3 tests here. Also shouldn't there be some sort of assert? or is this just a test that the functions work at all?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah looking at it again all 4 functions do occur in these tests. Still, I'm not sure what this is actually testing beyond that these functions don't just segfault/sigill.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There were asserts before, they were removed for some reason, can't remember why. I'll check if I can add them back

Copy link
Contributor Author

Choose a reason for hiding this comment

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

looking back on #1672, I don't think adding back the asserts is a good idea. The compiler can insert some other calls in between _xrstor and the second _xsave, which can change the register states. These tests should just be there to ensure the intrinsics don't segfault/sigill

}

#[simd_test(enable = "xsave,xsaveopt")]
Expand All @@ -168,9 +149,9 @@ mod tests {
let mut a = XsaveArea::new();
let mut b = XsaveArea::new();

xsave::_xsaveopt64(a.ptr(), m);
xsave::_xrstor64(a.ptr(), m);
xsave::_xsaveopt64(b.ptr(), m);
_xsaveopt64(a.ptr(), m);
_xrstor64(a.ptr(), m);
_xsaveopt64(b.ptr(), m);
}

#[simd_test(enable = "xsave,xsavec")]
Expand All @@ -180,8 +161,8 @@ mod tests {
let mut a = XsaveArea::new();
let mut b = XsaveArea::new();

xsave::_xsavec64(a.ptr(), m);
xsave::_xrstor64(a.ptr(), m);
xsave::_xsavec64(b.ptr(), m);
_xsavec64(a.ptr(), m);
_xrstor64(a.ptr(), m);
_xsavec64(b.ptr(), m);
}
}