the "right" timing for savestates?

I wonder... Eugene has found an obscure bug in my emulator regarding savestates during a screen transition. The game is Battletoads & Double Dragon.

The bug occurs when a savestate is saved right during a screen transition (in this case, when you go from a sublevel to another, like 2-2 to 2-3, and the screen fades to black). Loading the state before the new level "reappears", the emulator can crash (invalid opcode) or display glitched graphics. The result depends of the timing.

Is there a right time for saving/loading a state? I mean, in mid-instruction or only after the entire instruction be completed..? What if an NMI or IRQ is pending? Should I do the NMI/IRQ, then poll the keys? I need some advice.

If you're getting a crash, there has to be some state that is necessary to restore that you're not restoring. There's almost nothing you can safely leave at a default value when restoring a save state.

You don't have to resolve the NMI/IRQ before saving; you just have to make sure you're saving all the state necessary.

The problem is very specific. If I save a state when the game is loading the next level, then restore it, the bug appears.

Why not trace log this particular moment once when saving the state and then again after loading so you can compare the two and see where things go wrong? That might give you some insight on which specific bit of state is not being restored properly.

It could be any number of issues:

1. Failure to persist the full state of the emulator.
2. Failure to pause the emulation during state capture causing a rolling-shutter-like bug.
3. Failure to resume properly after loading the state.
4. Just some stupid coding bug in save state logic.

Resuming from a save state is actually a bit challenging. If the emulator were a text-book state machine, then the state could be easily persisted and restored. But, more often than not, little bits of state end up all over the place and the execution thread needs to jump to exactly where it left off.

I was working on something like this for RetroArch, a savestate validation system.

What I'm doing:
Run two different instances of the emulator, give them the exact same input.
Each frame, do this instead:

Instance A: Run Frame, Save State
Instance B: Run Frame, Save State, Run Frame (optional), Load State

Then you compare the contents of the savestates. If they match, it's all good. If they don't, there's an error.
If they don't match, you can run the emulator in Trace logging mode and see where they differ.

For RetroArch, I was also capturing the sound and video output and comparing that as well.

Sounds like either a bug in the save state restoration code, *or* (and this is more likely I'd think) there's something you're not storing in a save state to begin with that's necessary to ensure reliable restoration.

Usually it's just MMIO contents, register contents, RAM, and some general ROM state (PRG bank, etc.). But I bet there's some internal PPU state that's not being saved/restored. 100% speculative on my part.

So... what exactly do you store in your emulators' save state file? Maybe by disclosing that we can tell you what might be missing that needs to be saved.

(Same question applies to any/all emulator authors. Maybe Sour can explain what he saves in Mesen's save states and you two can compare.)

None of you got the point, so I changed the topic slightly.
I'm asking if it's OK to save during mid-instruction, or if I should wait the current instruction to be completed, then poll keyboard (for saving/loading).

We did answer that.

We said: Are you saving all the state for whatever you choose?

Okay, I'll give my answer, given the subject change + edit applied to the initial post (substantially more clear to me, thanks!):

My advice would be to only save state after the running 6502 instruction has executed.

Can anyone think of a case where waiting for a 6502 instruction to finish (before doing the actual save state) would result a bad UX (user experience)? Example: user press keyboard shortcut to save state (or GUI option via mouse), followed by a long delay (greater than, say, 0.5 seconds) before the user got actual confirmation that the state was saved to the file? I can't think of one off the top of my head.

If you want to allow saving state at any point in time, then you need to save all of the data necessary to fully represent that point in time - e.g. if you're saving mid-scanline, you need the entire state of the sprite state machine to ensure that sprite 0 hit and/or sprite overflow will occur correctly.

If, on the other hand, you enforce an arbitrary rule that e.g. savestates can only be captured during VBLANK, then you can safely discard a lot of that data because it'll get reinitialized from scratch before it would need to be used. My own emulator requires savestates to be saved during scanline 240 (the post-render scanline before NMI), which made things a whole lot simpler, though I suppose it also opens the possibility of a malicious program that causes the emulator to hang if you try to save its state (e.g. by initiating a Sprite DMA at scanline 239).

For what it's worth, I can't think of any reason why you would even want to allow pausing emulation mid-instruction, let alone creating a savestate.

In my code, there's a function to poll keys right before the VBlank (line 240, cycle 0). Such function is called inside ppu_clock() which is called at every CPU read/write (1 CPU cycle = 3 PPU cycles). So, if polling keys is inside ppu_clock() it may be called in mid-instruction or even during an NMI/IRQ.

So, this way, I suppose it's NOT correct at all. So, waiting for the current instruction to finish (+ any pending interruptions) seems to be the right timing for it.

Sometimes timing can be sensitive that if you're off by a cycle or subcycle, if can ruin consistency or crash the game. I know that resetting Super Mario Bros 3 at the wrong time can cause it to crash.

Are you sure the save time is exactly the same as the restore time? It doesn't matter if it's in the middle of an instruction if even the instruction state is saved, restored at the same point in process.

My suggestion is to just keep it simple and consistent where and when the state save/load is made.

Good point.

It's true only for the PPU timing, but not for the CPU. I believe to have fixed such issue, as koitsu pointed out.

From what I've read, it does seem like a variable not being restored properly.

I've never personally implemented savestates for WedNESday, so I've no experience with them. But shouldn't an emulator wait for the entire frame to finish before saving the savestate?

Nope, it wasn't. Just the wrong timing - must be saved at the end of the current instruction. Period.

That's what I always did - scanline 240, right before the VBlank. It doesn't matter if it's saved before or after VBlank though.

I'm of the opinion that being able to save on a specific instruction is useful for debugging.