capstone icon indicating copy to clipboard operation
capstone copied to clipboard

Incorrect operands data for RISC-V ret

Open KelvinChung2000 opened this issue 1 year ago • 9 comments

Work environment

Questions Answers
OS/arch/bits Pop os
Architecture x86
Source of Capstone git clone
Version/git commit v5.0.1 5ba4ca4ba6b9ba7edb41243a036f973cb056d143

Expected behavior

When accessing CsInsn.operands should return 3 RISCVOp

Actual behavior

It return empty list

Steps to reproduce the behavior

from capstone import Cs
from capstone import CS_ARCH_RISCV, CS_MODE_RISCV64

CODE = b"\x67\x80\x00\x00"
md = Cs(CS_ARCH_RISCV, CS_MODE_RISCV64)
md.detail = True
for insn in md.disasm(CODE, 0):
    print(insn.bytes)
    print(f"{insn.mnemonic} {insn.op_str}")
    print(insn.operands)

Additional Logs, screenshots, source code, configuration dump, ...

The ret is the shorthand for jalr x0, 0(x1), so it should return 3 ops. But the disassembly itself is correct.

image

KelvinChung2000 avatar Jul 24 '24 20:07 KelvinChung2000

fyi, the next branch has support for access information of operands as well. You might want to check, if it works there.

Rot127 avatar Jul 25 '24 09:07 Rot127

I am on the next already, but not working for me

KelvinChung2000 avatar Jul 25 '24 10:07 KelvinChung2000

cc @wxrdnx At the latest I'm going to fix it in the https://github.com/capstone-engine/capstone/pull/2384 PR. But if you find time before, I would be grateful if you could take a look at it as well.

Rot127 avatar Jul 25 '24 11:07 Rot127

I have looked into #2393. Before fixing access, I think we need to rethink how it should be implemented. I think it is better to classify them based on the instruction format. For example, all R-type instructions with the OP opcode group will have an access pattern of write, read, and read for the operands.

KelvinChung2000 avatar Jul 25 '24 12:07 KelvinChung2000

The RISC-V module is far from perfect. We currently have a huge Capstone modernization effort going on. For the RISC-V module specifically, there is a generator in progress, which uses the SAIL definitions. It will replace the current module.

https://github.com/capstone-engine/capstone/pull/2393 was just a quick and dirty addition, because @wxrdnx needed it, and had not enough time to work on the generator. If you are interested in joining the effort to build the RISC-V module generator, it would be very welcome of course :)

Rot127 avatar Jul 25 '24 13:07 Rot127

The access is not on my critical path. It is nice to have, but I can hack around it. I will see what I can do when the generator is in place.

KelvinChung2000 avatar Jul 25 '24 13:07 KelvinChung2000

Feel free to also fix the access if you find wrong ones. The table was created by hand. So there might be a few mistakes.

Rot127 avatar Jul 25 '24 14:07 Rot127

This issue exists before my commit I got the same result from commit 9c5b48b5 (The commit before my pr is merged)

$ cstool -d riscv64 67800000 
 0  67 80 00 00  ret	
	ID: 207 (jalr)

	Groups: call

The same thing happens with other pseudo-instructions:

$ cstool -d riscv64 13000000
 0  13 00 00 00  nop	
	ID: 2 (addi)
$ cstool -d riscv64 13840400
 0  13 84 04 00  mv	s0, s1
	ID: 2 (addi)
	op_count: 2
		operands[0].type: REG = s0
		operands[1].type: REG = s1

I think it has something to do with the way RISC-V pseudo-instructions are handled by Capstone. Capstone seems to discard operands when converting base instructions back to pseudo-instructions.

wxrdnx avatar Jul 25 '24 19:07 wxrdnx

cc @moste00

XVilka avatar Jul 25 '24 23:07 XVilka