-
Notifications
You must be signed in to change notification settings - Fork 26
IGVM crate affected by apparent compiler regression in nightly optimized builds #80
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
Comments
I'd be happy to take a PR if you can create a minimal testcase. We do try to run with miri so I guess we were missing this test coverage? |
I think perhaps also it might be worth standing up occasional nightly runs, since otherwise CI doesn't run very often. |
I can look at adding coverage for write_binary_header with SnpVpContext. It doesn't look like miri saw any problems since it was a bug in the optimizer. |
Adds test that the SnpVpContext/IGVM_VHS_VP_CONTEXT/IGVM_VHT_VP_CONTEXT type is serialized and deserialized correctly. Adds regression test for #80 (compiler bug, so no code changes needed to fix). Signed-off-by: Adam Dunlap <[email protected]>
Closing this one out since it was tracking adding the test for this specific regression. I'll see about standing up some nightly runs on a regular cadence. |
This is mostly a FYI, but an apparent rust compiler regression caused some code in this crate to start crashing with SIGILL. The function IgvmDirectiveHeader::write_binary_header can crash this way when given a IgvmDirectiveHeader::SnpVpContext variant. It only happens when the opt-level is higher than 1 and only with the rust nightly compiler. This is tracked in rust-lang/rust#136361, and the issue contains some more details about exactly which versions of rust may be affected.
There's no indication that any of code in this crate is buggy since it's just in safe code, but there's a possibility the zerocopy dependency is doing something unsound with unsafe code.
This issue wasn't found in this crate's tests, it was only found in some of our integration tests, so it might make sense to increase test coverage of this part of IgvmDirectiveHeader::write_binary_header. While narrowing down our failing test it was pretty easy to reproduce the crash so I think even weak coverage would be helpful.
The text was updated successfully, but these errors were encountered: