wokwi / rp2040js

A Raspberry Pi Pico Emulator in JavaScript
MIT License
400 stars 44 forks source link

LSLS wrong result and flags when shift >=32 #62

Closed Turro75 closed 3 years ago

Turro75 commented 3 years ago

another gdbdiff bug found, this is the failed test it('should execute a lsls r3, r4 instruction', async () => { await cpu.setPC(0x20000000); await cpu.writeUint16(0x20000000, opcodeLSLSreg(r3, r4)); await cpu.setRegisters({ r3: 1, r4: 0x20, C: true }); await cpu.singleStep(); const registers = await cpu.readRegisters(); expect(registers.r3).toEqual(0); expect(registers.pc).toEqual(0x20000002); expect(registers.N).toEqual(false); expect(registers.C).toEqual(true); expect(registers.Z).toEqual(true); });

this is the current implementation // LSLS (register) else if (opcode >> 6 === 0b0100000010) { const Rm = (opcode >> 3) & 0x7; const Rdn = opcode & 0x7; const input = this.registers[Rdn]; const shiftCount = this.registers[Rm] & 0xff; const result = input << shiftCount; this.registers[Rdn] = result; this.N = !!(result & 0x80000000); this.Z = result === 0; this.C = shiftCount ? !!(input & (1 << (32 - shiftCount))) : this.C; }

as bitwise operations are limited to 32bit when the shift is >= 32 it fails. changing the result assignment as this const result = shiftCount >= 0x20 ? 0 : input << shiftCount; fixes

do we have to inspect all shifts?

urish commented 3 years ago

I see that in LSRS we already handle this case:

   const result = shiftAmount < 32 ? input >>> shiftAmount : 0; 

(I usually prefer to write the "happy path" first, and the "special" case last)

but in ASRS we don't handle this:

    const result = this.registers[Rdn] >> shiftN;

My guess is that ASRS with 32 bits will just return 0xffffffff or 0, depending on the highest bit (MSB) of the input.

Thoughts?

Turro75 commented 3 years ago

I already wrote a fix and test for both asrs and lsls, I'll PR later Today. I don't see any other instruction that would be affected

urish commented 3 years ago

Thank you!