wokwi / rp2040js

A Raspberry Pi Pico Emulator in JavaScript
MIT License
389 stars 40 forks source link

STR instruction fails when using a negative offset #43

Closed urish closed 3 years ago

urish commented 3 years ago

The STR instruction has a form where you can specify base register + offset register, e.g.

str r0, [r1, r2]

It's possible to use a negative offset value (e.g. -4), and the target address will still be correct as the result should wrap at 32 bits. This doesn't work correctly in the current implementation. The following test case passes on the physical hardware but fails in the emulator:

  it('should execute a `str r2, [r3, r1]` instruction where r1 + r3 > 32 bits', async () => {
    await cpu.setPC(0x20000000);
    await cpu.writeUint16(0x20000000, opcodeSTRreg(r2, r1, r3));
    await cpu.setRegisters({ r1: -4, r3: 0x20041e50, r2: 0x4201337 });
    await cpu.singleStep();
    const registers = await cpu.readRegisters();
    expect(await cpu.readUint32(0x20041e4c)).toEqual(0x4201337);
    expect(registers.pc).toEqual(0x20000002);
  });

I suspect that a similar bug exists for other instructions that accept a memory address in the form of base register + offset register, e.g. STRH, STRB, LDR, LDRB, LDRH, LDRS, LDRSB.

urish commented 3 years ago

One effect of this bug, found by @sutaburosu:

Running MicroPython and typing in the following code will result in a weird error:

>>> def factorial(x):
...     if x == 0:
...         return 1
...     r = x * factorial(x-1)
...     return r
...
...
>>> factorial(5)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 4, in factorial
  File "<stdin>", line 4, in factorial
  File "<stdin>", line 4, in factorial
  File "<stdin>", line 4, in factorial
  File "<stdin>", line 5, in factorial
NameError: local variable referenced before assignment
>>>
sutaburosu commented 3 years ago

Can confirm the currently deployed changes fix this issue. Thanks, Uri.