Skip to content
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

Cranelift: x64: fuzzbug: vbroadcastss AVX instruction emitted when AVX not enabled #6059

Closed
cfallin opened this issue Mar 17, 2023 · 0 comments · Fixed by #6060
Closed

Cranelift: x64: fuzzbug: vbroadcastss AVX instruction emitted when AVX not enabled #6059

cfallin opened this issue Mar 17, 2023 · 0 comments · Fixed by #6060

Comments

@cfallin
Copy link
Member

cfallin commented Mar 17, 2023

Regression from #6025: when building with fuzz input

ICAgICAgICAg////IP8gICAgICAgICAgICAgICAAIP8gICAgICAgICAgICAgICAgICAgICAg/yAg
ICAgICAgICAgICAgICAgICAgICAgICAgICAgICD/ICAgICAgICAgICAgICAgICAgIAAg///2RA==

to the fuzz target differential, we see

thread '<unnamed>' panicked at 'Cannot emit inst 'vbroadcastss %v130, %v143' for target; failed to match ISA requirements: [AVX]', cranelift/codegen/src/isa/x64/inst/emit.rs:138:9

cc @alexcrichton

(OSS-Fuzz bug 57200 when public)

alexcrichton added a commit to alexcrichton/wasmtime that referenced this issue Mar 18, 2023
This commit fixes a corner case in the emission of the
`vbroadcasts{s,d}` instructions. The memory-to-xmm form of these
instructions was available with the AVX instruction set, but the
xmm-to-xmm form of these instructions wasn't available until AVX2.
The instruction requirement for these are listed as AVX but the lowering
rules are appropriately annotated to use either AVX2 or AVX when
appropriate.

While this should work in practice this didn't work for the assertion
about enabled features for each instruction. The `vbroadcastss`
instruction was listed as requiring AVX but could get emitted when AVX2
was enabled (due to the reg-to-reg form being available). This caused an
issue for the fuzzer where AVX2 was enabled but AVX was disabled.

One possible fix would be to add more opcodes, one for reg-to-reg and
one for mem-to-reg. That seemed like somewhat overkill for a pretty
niche situation that shouldn't actually come up in practice anywhere.
Instead this commit changes all the `has_avx` accessors to the
`use_avx_simd` predicate already available in the target flags. The
`use_avx2_simd` predicate was then updated to additionally require
`has_avx`, so if AVX2 is enabled and AVX is disabled then the
`vbroadcastss` instruction won't get emitted any more.

Closes bytecodealliance#6059
alexcrichton added a commit that referenced this issue Mar 18, 2023
* x64: Fix vbroadcastss with AVX2 and without AVX

This commit fixes a corner case in the emission of the
`vbroadcasts{s,d}` instructions. The memory-to-xmm form of these
instructions was available with the AVX instruction set, but the
xmm-to-xmm form of these instructions wasn't available until AVX2.
The instruction requirement for these are listed as AVX but the lowering
rules are appropriately annotated to use either AVX2 or AVX when
appropriate.

While this should work in practice this didn't work for the assertion
about enabled features for each instruction. The `vbroadcastss`
instruction was listed as requiring AVX but could get emitted when AVX2
was enabled (due to the reg-to-reg form being available). This caused an
issue for the fuzzer where AVX2 was enabled but AVX was disabled.

One possible fix would be to add more opcodes, one for reg-to-reg and
one for mem-to-reg. That seemed like somewhat overkill for a pretty
niche situation that shouldn't actually come up in practice anywhere.
Instead this commit changes all the `has_avx` accessors to the
`use_avx_simd` predicate already available in the target flags. The
`use_avx2_simd` predicate was then updated to additionally require
`has_avx`, so if AVX2 is enabled and AVX is disabled then the
`vbroadcastss` instruction won't get emitted any more.

Closes #6059

* Pass `enable_simd` on a few more files
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant