Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

incorrect order of stack operations #1257

Closed
peterferrie opened this issue Oct 31, 2023 · 14 comments
Closed

incorrect order of stack operations #1257

peterferrie opened this issue Oct 31, 2023 · 14 comments
Labels
Milestone

Comments

@peterferrie
Copy link

peterferrie commented Oct 31, 2023

A tester had this code:

$0178  A2 7D     LDX #$7D
$017A  9A        TXS
$017B  20 55 13  JSR $1355

with the expected result that the JSR's $13 gets overwritten with $01 before the $13 is fetched, resulting in $155 being executed instead of the current $1355.

Tester followed up with the verified cycles:

So for $017B 20 55 13 JSR $1355

  1. read $20 (JSR) then inc PC to $017C
  2. read ADL from $017C as $55, then inc PC to $017D
  3. internal stuff
  4. overwrite $017D's $13 with PCH $01 then dec S
  5. overwrite $017C's $55 with PCL $7D then dec S
  6. read ADH from $017D as $01, set PC with ADH/ADL as $0155.
@tomcw
Copy link
Contributor

tomcw commented Oct 31, 2023

Thanks Peter.

You can see where the implementation has got it wrong (for this edge case)...

			case 0x20: ABS        JSR  CYC(6)  break;

#define ABS	 addr = *(LPWORD)(mem+regs.pc);	 regs.pc += 2;

#define JSR	 --regs.pc;						    \
		 PUSH(regs.pc >> 8)					    \
		 PUSH(regs.pc & 0xFF)					    \
		 regs.pc = addr;

#define PUSH(a)	 *(mem+regs.sp--) = (a);				    \
		 if (regs.sp < 0x100)					    \
		   regs.sp = 0x1FF;

@tomcw tomcw added the bug label Oct 31, 2023
@peterferrie
Copy link
Author

peterferrie commented Nov 1, 2023 via email

@sicklittlemonkey
Copy link
Contributor

Wow, that's a nice edge case! Only JSR because it's the only multi-byte instruction that affects the stack.

Kinda surprised it hasn't popped up already for obfuscation in a protection routine.

I've often wondered how much slower a "microcode" cycle accurate 6502 emulation would be.

@xotmatrix
Copy link

This is neat. My emulated CPU handles it thanks to single-cycle instructions. It may be a little bit slower but it simplifies other things like memory-mapped I/O oddities and timing.

@peterferrie
Copy link
Author

peterferrie commented Nov 1, 2023 via email

@univta0001
Copy link

univta0001 commented Nov 7, 2023

The JSR bug is tested on some of the Apple 2 emulators.

The emulator is tested using the following DSK image (jsr_stack.dsk) attached in the ZIP file jsr_stack.zip

Here are the results.

  1. MAME (Apple 2e and below) - OK
  2. MAME (Apple 2GS) - Not OK (Expected for 65816)
  3. Apple2JS (https://github.com/whscullin/apple2js)- OK
  4. Jace (https://github.com/badvision/jace) - Not OK
  5. Epple2 (https://cmosher01.github.io/Epple-II/) - OK
  6. emu6502 (https://github.com/univta0001/emu6502) - OK
  7. microM8 (https://paleotronic.com/software/microm8/) - Not OK
  8. Accurapple (https://gitlab.com/wiz21/accurapple) - OK
  9. KEGS (https://kegs.sourceforge.net/) - Not OK (Expected for 65816)
  10. MII Emulator (https://github.com/buserror/mii_emu) - Not OK

On way to fix AppleWin implementation is as follows

#define JSR	 --regs.pc;						    \
		 PUSH(regs.pc >> 8)					    \
		 PUSH(regs.pc & 0xFF)					    \
		 regs.pc = (addr & 0xff) | (*(LPWORD)(mem+regs.pc)) << 8

@ksherlock
Copy link

The 65816 cycle order is different so KEGS and MAME (IIgs) are correct.

image

@buserror
Copy link

buserror commented Nov 7, 2023

MII https://github.com/buserror/mii_emu also works

@univta0001
Copy link

MII https://github.com/buserror/mii_emu also works

Have tested your emulator. It is having the incorrect order of stack operations too.

@buserror
Copy link

buserror commented Nov 8, 2023

MII https://github.com/buserror/mii_emu also works

Have tested your emulator. It is having the incorrect order of stack operations too.

How?


  PC:0178 A:01 X:00 Y:00 S:ff #1 I AD:0178 D:a2 R czidbRvn 0178: A2 7D     LDX #$7D

  PC:017A A:01 X:00 Y:00 S:ff #1   AD:0179 D:7d R czidbRvn

  PC:017A A:01 X:7D Y:00 S:ff #2 I AD:017A D:9a R czidbRvn 017A: 9A        TXS 

  PC:017B A:01 X:7D Y:00 S:7d #2 I AD:017B D:20 R czidbRvn 017B: 20 55 13  JSR $1355

  PC:017D A:01 X:7D Y:00 S:7d #1   AD:017C D:55 R czidbRvn

  PC:017E A:01 X:7D Y:00 S:7d #2   AD:017D D:13 R czidbRvn

  PC:017E A:01 X:7D Y:00 S:7d #3   AD:1355 D:00 R czidbRvn

  PC:017E A:01 X:7D Y:00 S:7c #4   AD:017D D:01 W czidbRvn

  PC:017E A:01 X:7D Y:00 S:7b #5   AD:017C D:7d W czidbRvn

  PC:1355 A:01 X:7D Y:00 S:7b #6 I AD:1355 D:00 R czidbRvn 1355: 00        BRK 

  TEST Test of JSR *in* the stack              : PASS


@xotmatrix
Copy link

@buserror

How?

It should be landing on $0155, not $1355. It looks like you are fetching the high-order byte in your step 2 instead of your step 5.

Also, I believe the discarded read you have in your step 3 should be reading from the stack, the same address you push to in the next step. That normally wouldn't matter but it could cause an issue with memory-mapped hardware.

firefox_2023-11-08_03-36-57

@sicklittlemonkey
Copy link
Contributor

Run MAME and see for yourself. :-)

I just had a look. It makes sense to generate the code with the Python script.
Hard to get a sense of the execution without compiling and debugging it.
It's been a long time, and the code isn't much friendlier. ; - )

I see the floating bus code I donated 20 years ago is still there unchanged.

@peterferrie
Copy link
Author

peterferrie commented Nov 9, 2023 via email

tomcw added a commit that referenced this issue Mar 3, 2024
@tomcw tomcw added this to the 1.30.18 milestone Mar 3, 2024
@tomcw
Copy link
Contributor

tomcw commented Mar 3, 2024

Implemented a fix in commit 40bf9cd.
I also tested the following boundary cases on a real 65C02 (and have added all 3 as CPU unit-tests).

S = 7C

155:00
178:A2 7C 9A 20 55 13 00
1355:00

Expect break at 1355, with return address (7D 01) overwriting the JSR opcode at $17B.
(NB. The BRK trashes the original code)

S = 7E

155:00
178:A2 7E 9A 20 55 13 00
7D55:00

Expect break at 7D55, with return address (7D 01) overwriting the JSR operand2 at $17D.
(NB. The BRK trashes the original code)

@tomcw tomcw closed this as completed Mar 3, 2024
@tomcw tomcw mentioned this issue Mar 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants