insou22 / mipsy

Education-focused MIPS Emulator written in Rust.
86 stars 13 forks source link

bug when using la with register addressing #286

Closed XavierCooney closed 1 year ago

XavierCooney commented 1 year ago

The following program prints 32767, as would be expected:

main:
    li  $a0, 0
    la  $a0, 32767($a0)
    li  $v0, 1
    syscall

    li  $v0, 0
    jr  $ra

but this one prints 65536 (double the expected answer):

main:
    li  $a0, 0
    la  $a0, 32768($a0)
    li  $v0, 1
    syscall

    li  $v0, 0
    jr  $ra

Probably something similar going on to #283, since this only seems to occur when the two registers in the la instruction are the same.

Dylan-Brotherston commented 1 year ago

Assuming the first example is supposed to use something like $t0 instead of $a0

Looks like there is a typo here https://github.com/insou22/mipsy/blob/030526ec5200cdece7293c9e56f22e9edd18eadc/mips.yaml#L2457 And $Rs is used twice instead of $Rt

XavierCooney commented 1 year ago

The first example is still meant to be $a0, the bug is only triggered with offsets too large to fit in signed 16 bits.

I think the issue occurs when $Rs is the same as $Rt, in which case the lui and ori will overwrite $Rt so the subsequent addu is using the wrong value of $Rt, and so ends up just doubling $Rt.

Dylan-Brotherston commented 1 year ago

indeed image


la  $a0, 32767($a0)

becomes

addi $a0, $a0, 32767

in SPIM

and

addiu  $a0, $a0, 32767

in MIPSY


where as

la  $a0, 32768($a0)

becomes

ori $at, $zero, -32768
add $a0, $a0, $at

in SPIM

and

lui    $a0, 0
ori    $a0, $a0, 32768
addu   $a0, $a0, $a0

in MIPSY

clearly just doubling the value.


lui    $at, 0
ori    $at, $at, 32768
addu   $a0, $a0, $at

would be the correct expansion here

Dylan-Brotherston commented 1 year ago

That seems to have done the job image

diff --git a/mips.yaml b/mips.yaml
index aa4f815..a1c719b 100644
--- a/mips.yaml
+++ b/mips.yaml
@@ -2441,11 +2441,11 @@ pseudoinstructions:
       format: [Rs, Off32Rt]
     expand:
       - inst: LUI
-        data: [$Rs, $Off32uHi]
+        data: [$At, $Off32uHi]
       - inst: ORI
-        data: [$Rs, $Rs, $Off32uLo]
+        data: [$At, $At, $Off32uLo]
       - inst: ADDU
-        data: [$Rs, $Rs, $Rt]
+        data: [$Rs, $Rs, $At]

   - name: NOP
     desc_short: No-Operation - doesn't do anything
Dylan-Brotherston commented 1 year ago

Fixed by #288