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

Machine: fix instruction to_str #132

Merged
merged 1 commit into from
May 30, 2024
Merged

Machine: fix instruction to_str #132

merged 1 commit into from
May 30, 2024

Conversation

trdthg
Copy link
Contributor

@trdthg trdthg commented May 7, 2024

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()

@trdthg trdthg marked this pull request as ready for review May 7, 2024 05:11
@trdthg trdthg mentioned this pull request May 7, 2024
13 tasks
@jdupak jdupak added this pull request to the merge queue May 30, 2024
Merged via the queue into cvut:master with commit db1cabc May 30, 2024
8 checks passed
@ppisa
Copy link
Member

ppisa commented May 30, 2024

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

jdupak commented May 30, 2024

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

trdthg commented May 31, 2024

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.

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.

None yet

3 participants