cvut / qtrvsim

RISC-V CPU simulator for education purposes
GNU General Public License v3.0
466 stars 56 forks source link

Machine: fix instruction to_str #132

Closed trdthg closed 3 months ago

trdthg commented 4 months ago

negative hex should use '-' in the str, or it will be treated as uint

jal x1, -0xFF:

str::asHex((int32_t)-0xff) -> s: 0xffffffffffffff01
str::asHex((uint32_t)-0xff) -> s: 0xffffff01

'-' lost after to_str()

ppisa commented 3 months ago

I am not sure if this is a good idea. It is common to show constants in instructions either as decimal number where negative values are converted as minus sign and absolute value or as hexadecimal where raw value is shown unmodified.

So I would vote to not convert such values to opposite number and minus sign. The correct solution is to show all values as 64 bit unsigned numbers in such case. But I agree that it would overflow available space for instruction. When size is well known (i.e. 12 bits) and is multiple of nibble, then the second complement value printed to three digits without sign is appropriate. Where where is problem with too long prints on wires etc... I would vote to some shortcuts in form like 0x..f123849 or even ...f77898 so basically replace sign extension by ellipsis or something similar. But meaning of hexadecimal number even with 31 or 63 bit set can be mask or something similar where switch to minus plus absolute value can lead to more misleading than keeping complete value.

Please, try to elaborate places where are the problems, what was the output and what do you think that is the optimal option to be readable.

jdupak commented 3 months ago

The asHex function is generic and I agree with @trdthg that it makes more sense to render minus sign if signed type is used (this is decided by the caller).

As for the usage in disassembler, I would love to see negative constants as hex. I find the mixture of hex and decimal very confusing during teaching.

trdthg commented 3 months ago

Currently asHex is only used for Instruction::to_str() . I've rechecked the PR, and I've got some new ideas

where are the problems

First of all, I found this problem when I was doing some testing, and I had previously written the test cases in hexadecimal (with negative values added to the symbols)

{0xfe0793e3, “bne x15, x0, -0x1a”, “bnez x15, -0x1a”}

According to the above test case, the input command bnez x15, -0x1a, convert it to code, after converting it back to string, it should become bne x15, x0, -0x1a.

But in the test, we found that the output result is hexadecimal unsigned

Actual (Instruction(code_to_string[i].code).to_str()): “bne x15, x0, 0xffffffffffffffffe6”
Expected (code_to_string[i].str) : “bne x15, x0, -0x1a”

So I tried to get asHex to display negative values as carry symbols to satisfy the test requirements

These are my previous thoughts

what was the output

But now I realize that, if I follow your request, negative values should be displayed as decimal (carrying a negative sign), not 0xffffffffffffffffe6

What I should change is the following code

image

what do you think that is the optimal option to be readable

It is common to show constants in instructions either as decimal number where negative values are converted as minus sign and absolute value or as hexadecimal where raw value is shown unmodified.

I must admit I didn't know this rule before, so I modified the asHex function directly.

I've just checked the output of the gcc disassembler, and it does match

image

If this is indeed the most common practice, I agree with you @ppisa, it would be better to be consistent with gcc.

But I must admit, I was confused :D @jdupak.

Anyway, I think we should revert this PR change, and change the location mentioned above.


Update

'p', 'a' are address offset, the calculated address should never < 0, it's fine to keep 0xffffffffffffffffe6 and I should change my test case.

just revert this PR change if you're in agreement.