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

arm64: cs_arm64_op sys field can be arm64_sys_op OR arm64_reg but is defined as arm64_sys_op #1881

Closed
tmfink opened this issue May 10, 2022 · 4 comments · Fixed by #2026
Closed

Comments

@tmfink
Copy link
Contributor

tmfink commented May 10, 2022

The cs_arm64_op field sys can be either arm64_sysreg OR arm64_reg but the field is defined as arm64_sys_op.

arm64_sys_op sys; ///< IC/DC/AT/TLBI operation (see arm64_ic_op, arm64_dc_op, arm64_at_op, arm64_tlbi_op)

The current state makes it harder to:

This may also be related to #1760.

arm64_sys_op

$ ./cstool -d arm64 "00 78 08 d5"
 0  00 78 08 d5  at     s1e1r, x0
        ID: 948 (at)
        op_count: 2
                operands[0].type: SYS = 0x4f
                operands[1].type: REG = x0

1st operand "s1e1r" is arm64_sys_op value ARM64_AT_S1E1R.

arm64_reg

$ ./cstool -d arm64 "090038d5"
 0  09 00 38 d5  mrs    x9, midr_el1
        ID: 495 (mrs)
        op_count: 2
                operands[0].type: REG = x9
                operands[0].access: READ | WRITE
                operands[1].type: SYS = 0xc000
                operands[1].access: READ | WRITE
        Registers read: x9
        Registers modified: x9
        Groups: privilege 

The 2nd operand "midr_el1" is arm64_sysreg value ARM64_SYSREG_MIDR_EL1.

@adamjseitz
Copy link
Contributor

adamjseitz commented May 10, 2022

It looks like for MRS and MSR, the operand type should really be ARM64_OP_REG_MRS and ARM64_OP_REG_MSR, not ARM64_OP_SYS.

I think the confusion arises from the "SYS" instruction (for which AT / DC / IC / TLBI are aliases) and MSR / MRS refer to "system registers".

This is a regression since 4.x, check out old printMRSSystemRegister, which sets the type as ARM64_OP_REG_MRS:

https://github.com/capstone-engine/capstone/blob/master/arch/AArch64/AArch64InstPrinter.c#L1603

but now sets the type to ARM64_OP_SYS:

https://github.com/capstone-engine/capstone/blob/next/arch/AArch64/AArch64InstPrinter.c#L2026

tmfink added a commit to tmfink/capstone-rs that referenced this issue Jul 11, 2022
Update pre-generated bindings and changelog.

Various system register enums have been merged into `Arm64SysOp`.

Comments out part of test case due to upstream capstone bug:
capstone-engine/capstone#1881
tmfink added a commit to tmfink/capstone-rs that referenced this issue Oct 16, 2022
Update pre-generated bindings and changelog.

Various system register enums have been merged into `Arm64SysOp`.

Comments out part of test case due to upstream capstone bug:
capstone-engine/capstone#1881
@tmfink
Copy link
Contributor Author

tmfink commented Oct 16, 2022

@adamjseitz nice analysis. Here are the permalinks:

Old way (v4 branch):

MI->flat_insn->detail->arm64.operands[MI->flat_insn->detail->arm64.op_count].type = ARM64_OP_REG_MRS;

New way (next branch currently):

MI->flat_insn->detail->arm64.operands[MI->flat_insn->detail->arm64.op_count].type = ARM64_OP_SYS;

Currently in the next branch, if the operand is ARM64_SYSREG_DBGDTRRX_EL0, ARM64_SYSREG_VSCTLR_EL2, or a readable register, then the type is set to ARM64_OP_SYS. Otherwise, the type is set to ARM64_OP_REG_MRS.

tmfink added a commit to capstone-rust/capstone-rs that referenced this issue Oct 16, 2022
Update pre-generated bindings and changelog.

Various system register enums have been merged into `Arm64SysOp`.

Comments out part of test case due to upstream capstone bug:
capstone-engine/capstone#1881
@XVilka
Copy link
Contributor

XVilka commented Jun 29, 2023

@Rot127 take a look at this one too please

@Rot127
Copy link
Collaborator

Rot127 commented Jun 29, 2023

Will be fixed with #2026

System operands are described in way more detail (just as LLVM defines them).

typedef enum {
    [...]

    // Different system operands.
    AArch64_OP_SVCR = CS_OP_SPECIAL + 4,
    AArch64_OP_AT = CS_OP_SPECIAL + 5,
    AArch64_OP_DB = CS_OP_SPECIAL + 6,
    AArch64_OP_DC = CS_OP_SPECIAL + 7,
    AArch64_OP_ISB = CS_OP_SPECIAL + 8,
    AArch64_OP_TSB = CS_OP_SPECIAL + 9,
    AArch64_OP_PRFM = CS_OP_SPECIAL + 10,
    AArch64_OP_SVEPRFM = CS_OP_SPECIAL + 11,
    AArch64_OP_RPRFM = CS_OP_SPECIAL + 12,
    AArch64_OP_PSTATEIMM0_15 = CS_OP_SPECIAL + 13,
    AArch64_OP_PSTATEIMM0_1 = CS_OP_SPECIAL + 14,
    AArch64_OP_PSB = CS_OP_SPECIAL + 15,
    AArch64_OP_BTI = CS_OP_SPECIAL + 16,
    AArch64_OP_SVEPREDPAT = CS_OP_SPECIAL + 17,
    AArch64_OP_SVEVECLENSPECIFIER = CS_OP_SPECIAL + 18,
} aarch64_op_type;

typedef union {
    aarch64_sysreg sysreg;
    aarch64_tlbi tlbi;
    aarch64_ic ic;
} aarch64_sysop_reg;


typedef union {
    aarch64_dbnxs dbnxs;
    aarch64_exactfpimm exactfpimm;
} aarch64_sysop_imm;


typedef union {
    aarch64_svcr svcr;
    aarch64_at at;
    aarch64_db db;
    aarch64_dc dc;
    aarch64_isb isb;
    aarch64_tsb tsb;
    aarch64_prfm prfm;
    aarch64_sveprfm sveprfm;
    aarch64_rprfm rprfm;
    aarch64_pstateimm0_15 pstateimm0_15;
    aarch64_pstateimm0_1 pstateimm0_1;
    aarch64_psb psb;
    aarch64_bti bti;
    aarch64_svepredpat svepredpat;
    aarch64_sveveclenspecifier sveveclenspecifier;
} aarch64_sysop_alias;


typedef union {
    aarch64_sysop_reg reg;
    aarch64_sysop_imm imm;
    aarch64_sysop_alias alias;
} aarch64_sysop;

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.

4 participants