Skip to content

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

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

Merged
merged 1 commit into from
May 14, 2024

Conversation

ptersilie
Copy link
Contributor

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
Copy link
Contributor Author

ptersilie commented May 13, 2024

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
Copy link
Owner

CensoredUsername commented May 13, 2024

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
Copy link
Contributor Author

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
Copy link
Owner

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
Copy link
Contributor Author

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.

When incorrectly implementing an x86_64 instruction the compiler error
message currently makes wrong suggestions about what the correct 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` and `shr` require the shift amount to be in the `CL`
register. This commit fixes the ordering of the register array used by
the error formatting to make it match the ordering in the AMD64 manual.
@CensoredUsername CensoredUsername changed the base branch from master to dev May 14, 2024 11:14
@CensoredUsername CensoredUsername merged commit 0823e46 into CensoredUsername:dev May 14, 2024
1 check passed
@ptersilie ptersilie deleted the fix_format branch May 14, 2024 11:25
@CensoredUsername
Copy link
Owner

Thanks!

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 this pull request may close these issues.

2 participants