CensoredUsername / dynasm-rs

A dynasm-like tool for rust.
https://censoredusername.github.io/dynasm-rs/language/index.html
Mozilla Public License 2.0
705 stars 52 forks source link

Fix "argument type/size mismatch" error. #96

Closed ptersilie closed 1 month ago

ptersilie commented 1 month ago

When incorrectly implementing an x86_64 instruction the compiler error message currently makes wrong suggestions about what the correc input should be. For example,

dynasm!(self.asm; shl Rq(Rq::RDX.code()), Rq::RAX.code()))

gives the following error:

error: 'shl': argument type/size mismatch, expected one of the following forms:
>>> shl reg/mem8, dl
>>> shl reg/mem8, imm8
>>> shl reg/mem64, dl
>>> shl reg/mem16, dl
...

However, shl/shr require the shift amount to be in the CL register, which is also how it is implement in dynasm-rs. This commit fixes the error message to report the correct register.

ptersilie commented 1 month ago

I'm not sure this is the correct fix. I first tried fixing the arguments in gen_opmap.rs, but that lead to other issues, so it appears the compiler implements this correctly and it is merely the error message that is reporting it wrong.

CensoredUsername commented 1 month ago

Ah, nice catch. Thanks for the report.

I don't think this solution is correct, but I think i found what the root cause is. Check line 54 of debug.rs

x64 registers are encoded as 0-7 = rax, rcx, rdx, rbx, rsp, rbp, rsi, rdi.

So it should be "a", "c", "d", "b" instead of "a", "d", "c", "b".

ptersilie commented 1 month ago

x64 registers are encoded as 0-7 = rax, rcx, rdx, rbx, rsp, rbp, rsi, rdi.

Is this how they are encoded in dynasm-rs? Because the DWARF encoding goes rax, rdx, rcx, rbx, rsi, rdi, rbp, rsp. So maybe that's where the error stems from?

Do you want me to update the PR or do the fix yourself? I'm happy either way. :-)

CensoredUsername commented 1 month ago

Is this how they are encoded in dynasm-rs? Because the DWARF encoding goes rax, rdx, rcx, rbx, rsi, rdi, rbp, rsp. So maybe that's where the error stems from?

It's how they're encoded in the ISA according to the AMD64 manual: afbeelding

I'm not sure why DWARF uses that encoding. Either way I must've gotten confused somewhere in that code. rsp/rbp also got swapped in the error message output.

Feel free to update the PR.

ptersilie commented 1 month ago

Ah fair enough! I'm working a lot with LLVM which uses DWARF so that's what I'm used to.

I force pushed the new fix and updated the commit message.

CensoredUsername commented 1 month ago

Thanks!