-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Fail when VEX.L is set in SSE instructions (AVX is not supported) #1658
Conversation
Follow up: I do not have a real CPU that doesn't support AVX (I only have the first i3 model from 2011 that introduced AVX) so I wasn't able to figure out the exception thrown. Regardless I think it's better to fail loudly than to execute instructions incorrectly. |
@@ -47,12 +47,37 @@ static void test_vmovdqu(void) | |||
OK(uc_close(uc)); | |||
} | |||
|
|||
/* https://github.com/unicorn-engine/unicorn/issues/1656 */ | |||
static void test_vex_l(void) |
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.
Thanks for your contribution. The test case under tests/regress
is not built & run with CI and should have been moved to tests/unit
. Could you move this test to tests/unit/test_x86.c
?
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.
I enabled “Allow edits and access to secrets by maintainers“, feel free to edit as you see fit!
Agree-ed. I think |
Merged & fixed. |
Awesome, thanks! |
Closes #1656
According to https://gitlab.com/qemu-project/qemu/-/issues/132 YMM isn't supported in qemu upstream at all. I still have to test on a real CPU what exception is thrown, but other checks on
vex_l
seem to indicate it isillegal_op
. An alternative would be to add the check tovex_l
to every opcode, but since YMM isn't supported at all I think this is reasonable.