Conversation
Ports tests/unit/test_x86.c from the unicorn submodule (pin 681bd32) to
crates/unicorn/src/tests/x86.rs, following the existing per-arch test
modules. x86 was the only architecture whose upstream test file had no
Rust counterpart, despite being the largest one (71 KB of C).
50 of the 56 upstream tests are ported. The remaining 6 need bindings
that do not exist yet:
test_x86_hook_cpuid, test_x86_hook_insn_rdtsc, test_x86_hook_insn_rdtscp
need an instruction hook whose callback returns a value
(uc_cb_insn_cpuid_t); add_insn_sys_hook takes FnMut(..) -> ().
test_x86_mmu, test_x86_read_virtual
need uc_x86_msr read/write; MSR is not covered by value_size_x86 and
reg_read only returns u64.
test_x86_segmentation
needs a uc_x86_mmr accessor to write GDTR.
test_x86_unaligned_access and test_x86_64_unaligned_access are gated on
the same host-arch condition upstream uses (TARGET_READ_INLINED is set
for aarch64 and ppc targets).
Signed-off-by: KeyCode17 <m.daffa.karyudi@gmail.com>
Contributor
|
I don't think there is any benefit in copy the unit test from unicorn. The rust test should test the rust api. If unicorn is working correct is tested by unicorn. |
Member
|
Same feeling. This does not make too much sense and it will be out of sync easily. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
x86 is the only architecture whose upstream unit-test file has no Rust counterpart, even though it is by far the largest one. Every other arch was ported:
tests/unit/test_x86.ctest_arm64.ctests/arm64.rstest_arm.ctests/arm.rstest_riscv.ctests/riscv.rstest_mem.ctests/mem.rstest_ctl.ctests/ctl.rstest_mips/ppc/m68k/s390xx86 appears incidentally in
ctl.rsandmem.rs, but nothing covers the x86-specific behaviour thattest_x86.cexercises: SMC, MMIO, TCG-op hooks, nestedemu_start, TB-cache invalidation, x87fnstenv,fxsaveFPIP,bswap/REX prefix handling, unaligned access logging.What's in it
crates/unicorn/src/tests/x86.rs— 50 tests ported fromtests/unit/test_x86.cat the current submodule pin (681bd32), following the existing per-arch module conventions (uc_common_setup, state vianew_with_data+get_data_mut,assert_eq!overTEST_CHECK).crates/unicorn/src/tests/mod.rs— registers the module behind#[cfg(feature = "arch_x86")], matching the other arch gates.No library code is touched.
Not ported (6 of 56), and why
These need bindings that don't exist yet. I deliberately left them out rather than adding API in a test PR — happy to follow up with the bindings if you want them.
test_x86_hook_cpuid,test_x86_hook_insn_rdtsc,test_x86_hook_insn_rdtscpuc_cb_insn_cpuid_t).add_insn_sys_hooktakesFnMut(..) -> (), so the "skip the instruction" return can't be expressed.test_x86_mmu,test_x86_read_virtualuc_x86_msrread/write.MSRisn't invalue_size_x86, andreg_readonly yieldsu64.test_x86_segmentationuc_x86_mmraccessor to writeGDTR.One judgement call worth flagging
test_x86_unaligned_access/test_x86_64_unaligned_accesssit behind#if !defined(TARGET_READ_INLINED) && defined(BOOST_LITTLE_ENDIAN)upstream (CMake setsTARGET_READ_INLINEDfor aarch64 and ppc). I mirrored that guard with acfg. Both tests actually pass on aarch64 here when I remove the guard — so it can be dropped if you'd rather have the coverage on Apple Silicon / arm64 CI. I kept it to match upstream's stated constraint rather than silently diverging.Verification
cargo test— 154 passed, 0 failed (106 before, +48 on this aarch64 host; the two guarded tests bring it to +50 on x86_64).cargo fmt --all --check— clean.cargo clippy --workspace --all-targets— no new hits from this file. (There are 3 pre-existing-D warningserrors incrates/unicorn-sys/build.rsunder clippy 1.96 —collapsible_if×2 andunnecessary_debug_formatting. Untouched here; can send a separate PR if you'd like.)fnstenvandmem_hooks_pc_guaranteecallbacks actually fire, and confirmed a deliberately broken expectation does fail.