dcpu16
dcpu16 copied to clipboard
swetland
Fix dcpu_skip() bugs
PC increment must be preincrement in order to look up the correct instruction. Additionally, skiptable lookups must be masked or else garbage will be read, which can cause skips of more than one per operand.
Fixes several bugs in my Forth compiler's output when generating IFE instructions.
In hex:
7C51
D000
8453
7001
9002
00D1
7DC1
000D
8452
6061
6071
0000
0000
85A1
620C
A1C2
8453
7001
9002
00D1
7DC1
000D
8452
9DC2
8453
7001
9002
00D1
7DC1
000D
8452
35C1
Disassembled:
SET Z, 0xd000 ; 0000: 7c51 d000
SUB Z, 0x01 ; 0002: 8453
SET A, PC ; 0003: 7001
ADD A, 0x04 ; 0004: 9002
SET [Z], A ; 0005: 00d1
SET PC, 0x0d ; 0006: 7dc1 000d
ADD Z, 0x01 ; 0008: 8452
SET I, POP ; 0009: 6061
SET J, POP ; 000a: 6071
None
None
SET PUSH, 0x01 ; 000d: 85a1
IFE 0x00, POP ; 000e: 620c
ADD PC, 0x08 ; 000f: a1c2
SUB Z, 0x01 ; 0010: 8453
SET A, PC ; 0011: 7001
ADD A, 0x04 ; 0012: 9002
SET [Z], A ; 0013: 00d1
SET PC, 0x0d ; 0014: 7dc1 000d
ADD Z, 0x01 ; 0016: 8452
ADD PC, 0x07 ; 0017: 9dc2
SUB Z, 0x01 ; 0018: 8453
SET A, PC ; 0019: 7001
ADD A, 0x04 ; 001a: 9002
SET [Z], A ; 001b: 00d1
SET PC, 0x0d ; 001c: 7dc1 000d
ADD Z, 0x01 ; 001e: 8452
SET PC, [Z] ; 001f: 35c1
Ugh, sorry, GH apparently eats formatting and comments in this browser.
Anyway, the point is that at 0x000e, the IFE should only go to 0x000f or 0x0010, but instead it goes off to lala-land without my patch.
I thought about the pre/post-increment, and I agreed with you at first, but then I stepped through with GDB and realized that it needs to be pre-increment in order to examine the correct word.
There are two changes here:
- change skip to pre-increment PC instead of post-increment. Comment: like swetland, I think post-increment is correct - the caller takes care of pointing PC at the operand. However, I ran MostAwesomeDude's test code, and I do see it crashing. Another implemention I'm working on (system verilog) is executing the test code correctly, so I think there is a bug here. I'll take some time later today to have a closer look.
- Mask (op >> 10) with 31. The code extracts the top 6 bits of 16-bit op, and uses it to index into a 32-location table. Masking with 31 ensures we don't index outside of the table, but because op is an unsigned 16-bit quantity, the mask should be irrelevant (not harmful, but not needed). MostAwesomeDude, can you check that your compiler properly handles the u16 typedef? You mentioned a Forth compiler, but I'm not sure if that comes into play here.
Well, TBH I no longer use this emulator because Cauliflower and goforth are both at the advanced stage where they require I/O to be implemented to be emulated correctly.
Anyway, if you look at the code I posted, and read through it, and agree that it should behave in a certain way, then this patch does make it behave in that way.
I should point out that this patch was before the massive sequence of refactors which recently happened, so it's totally possible that PC is in a more consistent state coming into dcpu_skip() now. The reason for the mask is because without it, op >> 10 is a 6-bit quantity, which can range from 0x0 (zero) to 0x3f (63) which means that, in certain cases (like my test case!) it's possible for it to overflow the table and either segfault or look up some other data to figure out how many words to skip, jumping into lala-land.
Re: the mask value: 0x1f (31): this is needed to avoid overflowing skiptable, just as you said. I was wrong when I said the unsigned shift would take care of things.
But, masking with 0x1F leads to its own problems: now literal values (operand codes 0x20 - 0x3F) alias down to indices 0x0 - 0x1F in skiptable; some of those literal values are then incorrectly identified as values which imply an additional instruction word. I've got another fix which avoids this problem as well, and a testcase to show failure before the fix, correct function after. I've just pull-requested it, have a look if you like, comments welcome.