Skip to content

Conversation

@Xearty
Copy link

@Xearty Xearty commented Feb 4, 2023

What

This PR changes the type of byte from int8_t to uint8_t, this is necessarry in order for the vm to work correctly when the arguments size is 8 bytes e. g. when one of the arguments to an instruction is a floating-point number.
The problem is due to the way the instruction data is packed. it is a single byte, the bottom 6 bits represent the opcode and the most significant 2 bits represent the power of 2 which is the size of the arguments:
0 is for one byte
1 is for two bytes
2 is for four bytes
3 is for eight bytes
In the case of 3, the instruction byte is of the form 11xx xxxx where the x's are the opcode. To obtain the size of the arguments we would have to shift the instruction by 6 to the right. However, for a signed number this would result in all bits being 1 which in two's complement is -1. Later in read_integer we would pass this size of -1 and it would get cast to a size_t and evaluate to 2^64 - 1 and it would assert with assert(false && "not reached").
With this fix 11xx xxxx >> 6 would evaluate to 3 which is what we want.

Who

3MI0800077

@Xearty
Copy link
Author

Xearty commented Feb 4, 2023

We could just do *reinterpret_cast<const uint8_t*>(&instruction) >> 6 but I think it makes more sense for a byte type to be unsigned in order to prevent such mistakes in the first place.

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.

1 participant