Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical logic bug in the XCR0 (Extended Control Register 0) validation code for handling the XSETBV instruction in virtual machine execution. The bug was introduced in commit 5f6eaf4 where operator precedence caused the validation condition to always evaluate to true, making it ineffective.
Changes:
- Fixed the XCR0 AVX-512 state validation logic by properly grouping conditions with parentheses and using AND instead of chained ORs
- Improved error reporting by changing from
ok_ortook_or_elsewith a detailed error message showing the invalid xcr0 value
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| || !x.contains(Xcr0::XCR0_OPMASK_STATE) | ||
| || !x.contains(Xcr0::XCR0_ZMM_HI256_STATE) | ||
| || !x.contains(Xcr0::XCR0_HI16_ZMM_STATE) | ||
| || !x.contains(Xcr0::XCR0_HI16_ZMM_STATE)) |
There was a problem hiding this comment.
The indentation on this line appears inconsistent with the previous lines in the same logical group. It should align with lines 1227-1228 to maintain consistent formatting.
| || !x.contains(Xcr0::XCR0_HI16_ZMM_STATE)) | |
| || !x.contains(Xcr0::XCR0_HI16_ZMM_STATE)) |
| } | ||
|
|
||
| if x.contains(Xcr0::XCR0_OPMASK_STATE) | ||
| if (x.contains(Xcr0::XCR0_OPMASK_STATE) |
There was a problem hiding this comment.
Could you please add some comments here to describe why these combinations are invalid
There was a problem hiding this comment.
Also maybe we can extract these ifs into a new validator function?
No description provided.