tmfink
commented
11 months ago Currently, for many supported architectures, their
RegIdandInsnGroupId(some architectures even theInsnId) are simply defined as a bunch ofu32values inside a mod rather than a dedicated enum (e.g. ARM, ARM64, x86, etc.). We’d better re-define them to be dedicated enums so that definingArchTag::RegIdandArchTag::InsnGroupIdis meaningful.
I like your change which uses newtype_enum instead of the constified enum module! It's already a big improvement.
Bindgen didn't supported them when I first did this work. In fact, I contributed the "constified enum module" feature to bindgen (https://github.com/rust-lang/rust-bindgen/pull/741) to avoid the less ergonomic constified enum.
However, I think we may be able to do even better (although it might be more annoying). Ideally, we would expose actual Rust enums in the high-level bindings. That way, users could take advantage of the exhaustive pattern matching with match. The Capstone C code makes sure that 0 is always an "invalid" value such as ARM_SYSREG_INVALID. When converting from the C value to the Rust value, we could map any invalid value to an "invalid" variant of the enum OR always return an Option<T> when converting and return None if we get an invalid value.
// Have internal "invalid" variant
enum ArmReg {
Invalid = 0,
APSR,
APSR_NZCV,
// ...
}// No invalid variant, so converting gives `Option<ArmReg>`
enum ArmReg {
APSR = 1,
APSR_NZCV,
// ...
}You don't need to implement this "Rust enum" change as a part of this PR--I just thought I'd let you know what I was thinking.
Lancern
codecov[bot]
This PR resolves #118, which proposes compile-time target architecture tags. It contains a series of commits that implement the proposal. Problems may arise as I author these commits so I'm opening this PR in an early stage to make sure that we can agree on the specific designs in the end.
Here's a list of unresolved problems:
CapstoneBuilderthat buildsCapstoneinstances whose target architectures are "really" unknown at compile-time. When we call functions likex86, we have explicitly tell that the target architecture isx86. We should implement a new builder that sets the target architecture via a runtimeArchvalue.RegIdandInsnGroupId(some architectures even theInsnId) are simply defined as a bunch ofu32values inside a mod rather than a dedicated enum (e.g. ARM, ARM64, x86, etc.). We’d better re-define them to be dedicated enums so that definingArchTag::RegIdandArchTag::InsnGroupIdis meaningful.OptionorResultany more as they should always succeed given the exact target architecture. But forDynamicArchTagthese operations may still fail so we need to keep their return types as it is now. How do we resolve the conflict?~Here's a TODO list to make this PR fully ready: