Fix xsave segfaults #1934
Fix xsave segfaults #1934
Conversation
|
r? @folkertdev rustbot has assigned @folkertdev. Use |
|
something still seems wrong, the segfaulting rate is now 100% lol, imma try it again in #1933 |
sayantn
force-pushed
the
fix-xsave
branch
2 times, most recently
from
f60757a to
22f89b4
Compare
|
i'm just dumb, made the |
| #[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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
There were asserts before, they were removed for some reason, can't remember why. I'll check if I can add them back
There was a problem hiding this comment.
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
3 participants
It turns out that MSVC was right, and we were wrong (such a weird thing to say).
Our
xsavetests used a fixed-size 2560 byte buffer for saving all register data. But the problem is that the required size for xsave is not fixed, it depends on the CPU. This piece of code is ancient, and when it was written the authors calculated this limit based on the typical size. But currently, the maximum size required for xsave can be as high as 11892 bytes (heck on my laptop it is 2696 bytes, if anyone wants to try it out the code is in playground), for our SDE run it is 11008 bytes (which also explains why these tests were disabled in thex86_64-unknown-linux-gnurun, due to such a huge discrepancy in size, it always generated a segfault). So in a way, MSVC helped us detect this bug.See #1933 for some more context
As for why we are only seeing this now, if the CPU only supports up to AVX, 2560 bytes is enough space, but my guess is that GH introduced some better (windows?) runners, with AVX512 or AMX support, and both of those are capable of skyrocketing the XSAVE area size.
I will still rerun the CI a few times to ensure no weird random behavior is snooping around.