cranelift-assembler-x64: add Intel XED as a fuzzer disassembler oracle - #13915
Conversation
|
Hi, looking to restart work on this issue #12157 to enable APX. I am using the plan at the issue as reference but also just trying to enable an end to end example (assembler to file testing) of an APX instruction (#13916). W.r.t. fuzzing, capstone does not support APX but xed does. This patch allows an alternative oracle to capstone regardless of if APX is ever implemented, but is also necessary for APX fuzzing. The upstream xed sys crate needs to be updated as the APX spec has likely evolved for some corner cases not covered in the current spec, but before pursuing that I wanted to see if this plan is supported and if we can merge support for what is upstream today. |
github-actions
Bot
added
the
cranelift
|
Thanks! Could the xed paths be tested in CI? If you want them fuzzed as well they'll either need to be on-by-default or enabled explicitly in oss-fuzz. Also, there's a lot of updates to the lock file here -- could that be reset and then regenerated? (a stray |
fb75da1 to
3d467c6
Compare
2d78ea2 to
74e9a69
Compare
github-actions
Bot
added
the
fuzzing
Subscribe to Label Actioncc @fitzgen DetailsThis issue or pull request has been labeled: "cranelift", "fuzzing"Thus the following users have been cc'd because of the following labels:
To subscribe or unsubscribe from this label, edit the |
|
There's a huge amount of heavy lifting here going on in the comparison of xed's output to what this assembler outputs, and while that's not too bad since this is only related to fuzzing I'm also wondering if this is something where there might be a better way of handling this. One possible way of doing this is that instructions are stringifed with Do you have a sense if that sould help encapsulate the differences here? Alternatively maybe it'd make sense to just output the xed syntax and switch to that as the oracle in fuzzing? Capstone is also a big C dependency so building a big C project isn't necessarily the end of the world. Overall I'm mostly just wary of how there's such a large volume of code necessary to bridge capstone/xed disassembly. ~100 lines would be fine but ~700 feels like it's pushing things a bit |
The roundtrip fuzzer compares assembled instructions against Capstone, which cannot decode newer encodings such as APX. Add Intel XED as an optional second oracle: * Refactor the roundtrip core into `roundtrip_with(...)` so the Capstone path and a new XED path share one implementation (Capstone logic is unchanged, just extracted). * Implement `disassemble_xed` and `xed_matches` (normalizing whitespace, immediates, SIB scale-of-1, and operand-size/vector-length suffixes). * Gate XED behind a new off-by-default `fuzz-xed` feature, since it builds XED from source, and add a matching `roundtrip-xed` libFuzzer target. * Add the `smoke_xed` test, `#[ignore]`d while normalization (e.g. condition-code aliases) is still a work in progress. * Vet: exempt `xed-sys` and its `target-lexicon` requirement.
The `smoke_xed` test was `#[ignore]`d because XED and the assembler agreed on decoding but printed instructions differently. Reconcile the remaining differences in `xed_matches` and enable the test by default. New cases handled: displacement/branch-target hex-vs-decimal, the reverse operand-size suffix, legacy prefixes, condition-code aliases, the AT&T/Intel convert and move-with-extension mnemonics, `movabs`, implicit operands (blend `%xmm0` mask, shift-by-one), the indirect-branch `*`, and the SSE/AVX compare pseudo-ops. All behind `fuzz-xed`, so the default and Capstone paths are unchanged.
Rather than adding a `roundtrip-xed` fuzz target, add an `assembler_roundtrip_xed` case to the existing `misc` fuzzer next to the Capstone-based `assembler_roundtrip`. That avoids spending a separate OSS-Fuzz target budget on what is really just a second oracle for the same instruction stream. The new case is appended to the end of the `run_fuzzers!` list so the input-byte discriminants of the existing fuzzers are unchanged, and it is listed unconditionally, with only its body compiled out, so those discriminants do not shift with feature selection either. XED is built from source and needs a C compiler and Python, so the new `fuzz-xed` feature is off by default.
Add `cargo test -p cranelift-assembler-x64 --features fuzz-xed` to the nightly test job so the XED oracle is actually exercised rather than merely compiled. That job is already triggered by changes to paths matching `fuzz`, which covers both the assembler's `fuzz` module and the `misc` fuzz target. XED is built from source, but the C compiler and Python that needs are already available on the runner, so no extra packages are required.
`cargo test -p cranelift-assembler-x64 --features fuzz-xed` exercises the XED comparison logic, but it never compiles `misc.rs`, so a change that broke the `assembler_roundtrip_xed` wiring would go unnoticed. Pass `--features fuzz-xed` when checking the top-level fuzz targets so that wiring is built too.
The XED oracle previously compared XED's disassembly against the
assembler's own syntax and reconciled the two afterwards, which took
~500 lines of string rewriting. Instead, teach the assembler to print
the XED dialect directly: `format!("{inst}")` is unchanged, while
`format!("{inst:#}")` renders what XED would.
XED's spelling differs from ours in a few systematic ways, all derived at
build time in `dsl::Inst::xed_mnemonics` rather than annotated per
instruction:
- it drops the AT&T operand-size suffix we carry (`addl` -> `add`);
- it prefers a different condition-code alias for six of the sixteen
conditions (`cmovae` -> `cmovnb`);
- it appends a width marker when an operand is memory (`addsd` ->
`addsdq`), which is why the mnemonic is chosen at runtime from the
`r/m` operand.
Operand rendering follows the same flag: memory operands drop the spaces
after commas, always state the SIB scale, and use hexadecimal
displacements. The `;; implicit:` and trap annotations are ours alone, so
the alternate form omits them, as it does the fixed `%xmm0` blend mask.
This leaves a single residual difference -- XED pads the mnemonic field
with extra spaces -- so `xed_matches` is now a one-line comparison.
Verified by fuzzing the whole instruction set: zero mnemonic and zero
operand disagreements.
74e9a69 to
fa2c941
Compare
|
Hi @alexcrichton, good suggestion to use Also, about making XED the default, I think one day that may be desired but would hope to vet it out as an alternative first. It will be good to see if there are any discrepancies between the two. Let me know what you think. |
|
@alexcrichton, merge failed due to a build error on Linux s390x. The xed-sys crate build xed from sources that only build on x86 and aarch64 hosts. The update here recognizes that and gates building on x86_64 only. I guess this needs another review and merge initiation? |
The `fuzz-xed` feature builds Intel XED from C source, and `ci/run-tests.py` runs `cargo test --workspace --all-features`, which enabled the feature on every target in the test matrix. XED's build system is particular about its environment and failed in two different ways there: - on s390x it rejects the host outright with `Unknown cpu s390x`; - on macOS it selects `llvm-ar` as the archiver (because the compiler is clang) and aborts with `FileNotFoundError: 'llvm-ar'`, since that binary is not on the runner's PATH. The Windows jobs were cancelled before they ran in both cases, so whether XED builds there is unknown. The oracle only ever runs on x86_64 Linux -- the `fuzz-xed` CI job and OSS-Fuzz -- so restrict the dependency to that target and gate the corresponding code paths to match. Capstone remains the oracle everywhere else and the crate's other tests continue to run across the full matrix.
3a94bc9 to
23fa9d9
Compare
|
@alexcrichton sorry for the churn. There was a follow-up failure from merge tests for a similar reason of unsupported build/configuration. This time it was on macOS x86-64 where the underlying hardware was supported but the building for XED failed because it is looking for a toolchain that supports llvm-ar which is not default on this platform. I resolved the issue by just focusing on building on x86-64 linux. I ran with 'git commit --allow-empty -m "prtest:full"' to assure no other issues, and all tests passed except the onyx/windows issues which is known and which @rahulchaphalkar has root caused #13880. I assume this is good to go now. |
|
👍 thanks for tracking that down! |
d5657d4
2 participants
The roundtrip fuzzer compares assembled instructions against Capstone, which cannot decode newer encodings such as APX. Add Intel XED as an optional second oracle:
roundtrip_with(...)so the Capstone path and a new XED path share one implementation (Capstone logic is unchanged, just extracted).disassemble_xedandxed_matches(normalizing whitespace, immediates, SIB scale-of-1, and operand-size/vector-length suffixes).fuzz-xedfeature, since it builds XED from source, and add a matchingroundtrip-xedlibFuzzer target.smoke_xedtest,#[ignore]d while normalization (e.g. condition-code aliases) is still a work in progress.xed-sysand itstarget-lexiconrequirement.