Memory hook improvements

[VelpaChallenger]

For BSNES core, signatures were updated and data value was passed to the read callback after reading from the bus. For genplus-gx, since all callbacks (read/write/exec) use the same signature from mem_cb, I had to pass the value to all three of them. And since this could potentially cause some unwanted behavior, I also updated the values passed to the write and exec callbacks inside the core (I divided each update in a separate commit so it's easier to follow and understand).

Check if completed:

• I have run any relevant test suites
• I, the committer, have read the licensing terms for contributors (last updated 2022-07-15) and am compliant

resolves #3813

[Morilli]

Changes look good from what I can see without actually testing them.

[VelpaChallenger]

Something important to keep in mind is that there's a trade-off to these changes, and that is that a memory.writebyte inside a memory read callback function will not write the value before the reading by the core, so if someone wanted to try to force a value to be read by the game (as a form of stronger freezing if you will), it won't be possible anymore, since now the callback is called after the bus is read (albeit still before the game reads it). I don't know how many people actually do this, but just as a heads-up.

[Morilli]

That's actually a valid point, and it reinforces the point that there IS a "before" and "after" state when it comes to reading values, and the decision on where the callback is called has an actual impact on how that callback can be used.

Quoting myself from earlier:

I'd definitely appreciate an update to the docs to clarify for both bizhawk devs and lua devs how exactly those functions should work.

@YoshiRulz I do believe the "before" part for the callback is correct and matches expectations, we just need to decide what to do with the val parameter and how it should get populated:

BizHawk/src/BizHawk.Client.Common/lua/LuaHelperLibs/EventsLuaLibrary.cs

Line 206 in 6e77c73

[LuaMethod("on_bus_read", "Fires immediately before the given address is read by the core. Your callback can have 3 parameters {{(addr, val, flags)}}. {{val}} is the value read. If no address is given, it will fire on every memory read.")]

[YoshiRulz]

I'd definitely appreciate an update to the docs to clarify for both bizhawk devs and lua devs how exactly those functions should work.

[...] we just need to decide what to do with the val parameter and how it should get populated

I don't understand your question.

Fires immediately before the given address is read by the core. [...] val is the value read.

...seems pretty clear to me. Are you saying we should make that even clearer, maybe "val is the value to be read"? Or is it that existing implementations don't match the docs?

---

so if someone wanted to try to force a value to be read by the game (as a form of stronger freezing if you will), it won't be possible anymore

In our discussions on Discord re: possible implementations, this never came up. Freezing needs to remain functional—and I believe if you've broken it for Lua then it must also be broken for cheats.

[Morilli]

Are you saying we should make that even clearer, maybe "val is the value to be read"? Or is it that existing implementations don't match the docs?

Both, probably. First of all yes, val is the value to be read is the only way that makes sense to me.
Also, I assume no core will actually implement both conditions "correctly" at the moment, aka call the callback before the value is read (and allow for manipulation of the value to be read before it IS read), and also provide the value to be read in the callback.

Making it clearer in the docs what's important would help here.

[VelpaChallenger]

In our discussions on Discord re: possible implementations, this never came up. Freezing needs to remain functional—and I believe if you've broken it for Lua then it must also be broken for cheats.

I think there's a misunderstanding: freezing still works as usual and hasn't been affected in any way. What I meant with the wording "stronger freezing" is that when you freeze a memory address using cheats (traditional way), you are not actually freezing the memory address in the strict sense of the word, or at least not the way I'd expect. Whenever a frame advances, if the game's code updates the memory address, it will be updated. Say for example that you freeze 0xC676 when it has stored a 0xC value, if there's a line like move.b D0, ($C676), and then there's another line move.b ($C676), ($C284), then 0xC284 is not going to be populated with the value 0xC, rather it's going to be populated with whatever value was in D0. That's what I mean when I say it's not a freeze in the strict sense of the word. It's only going to get updated to 0xC at the end of the frame, but during it, freezing has no effect. However, when using memory callbacks, you can do something like memory.writebyte(0xC676, 0xC) inside your callback and then when move.b ($C676), ($C284) is executed, the value 0xC is going to be passed to 0xC284 because that is the value that is going to be read at 0xC676.

Also, the way I see it, there are 3 key moments to have in mind:

1. The moment the bus is read
2. The moment the game reads the value
3. The moment the callback is run

The moment the bus is read is for BSNES when this line auto data = bus.read(address, r.mdr); is executed. The moment the game reads the value is after the function used for accessing memory values returns (for BSNES, that would be when CPU::read returns). And the moment the callback is run is, well, when it's called by the core.

My point being that as I mentioned in the issue comments, while I agree with Morilli that the decision on where the callback is called has an actual impact on how that callback can be used, I still don't think there is really a "before" and an "after" state. The value still hasn't been sent when the callback is run, in fact, one possible way to go around this is to call bus.read again after calling the callback and then send that as the return value of CPU::read (for BSNES). That would be possible because the game's code still didn't receive the value at that point in time, so it's still before.

I do believe before the given address is read by the core should be changed to before the given address is read by the game's code, though. val is the value to be read sounds more accurate, but it's still not fully accurate given what we are discussing here and how memory callbacks cannot be used for the stronger freezing I was talking about.

Also, speaking of which. This detail slipped my mind earlier, but for the existing implementations, like here:

BizHawk/src/BizHawk.Emulation.Cores/Consoles/Nintendo/NES/NES.Core.cs

Lines 1077 to 1084 in b98942d

	if (MemoryCallbacks.HasReads)
	{
	uint flags = (uint)(MemoryCallbackFlags.CPUZero | MemoryCallbackFlags.AccessRead);
	MemoryCallbacks.CallMemoryCallbacks(addr, ret, flags, "System Bus");
	}
	DB = ret;
	return ret;

memory callbacks already cannot be used for the stronger freezing. So in that sense, nothing to really worry about.
(As a positive of this implementation, you can know how many bytes are actually being read by the game (so val will be 1 byte if just 1 byte is being read, 2 bytes if 2 bytes are being read, etc.). This has been very useful to me for reverse engineering.)

[YoshiRulz]

This discussion is continuing on Discord, but I want to apologise for my earlier assertion that you'd broken freezing. Freezing is, of course, already "broken" in that sense, and has been forever since it's implemented as pokes.

BizHawk/src/BizHawk.Client.Common/tools/Cheat.cs

Lines 168 to 176 in bb0d8f2

	case WatchSize.Byte:
	_watch.Poke(((ByteWatch)_watch).FormatValue((byte)_val));
	break;
	case WatchSize.Word:
	_watch.Poke(((WordWatch)_watch).FormatValue((ushort)_val));
	break;
	case WatchSize.DWord:
	_watch.Poke(((DWordWatch)_watch).FormatValue((uint)_val));
	break;

(This method is called twice in StepRunLoop_Core, both before and after FrameAdvance.)

[YoshiRulz]

Please rebase. Since GPGX got a big update, it may not be trivial to recreate those changes, in which case you can drop them and we'll leave the linked issue open to track that. Or maybe it was magically fixed in the update? (GPGX also now includes a stronger version of freezing, if you were interested in that.)

[VelpaChallenger]

Definitely interested, thanks for letting me know. Will check the changes and see if I can recreate what I did in this PR (if still necessary).

[VelpaChallenger]

Maybe things changed in this last month, but still, to make the PR discussion easy to track, I'd like to include what we recently talked on Discord, namely that after bringing in the changes from gpgx upstream, it was in fact sort of "magically fixed", because now the order is the same as it is in this PR (first read the value, then call the callback). The problem is the same as before and that is that this way, deep freezing is not possible using on_bus_read. Maybe once deep freezing is stable enough (such that it can replace freezing by means of on_bus_read), we can leave the order of this PR and then replicate it to the BSNES and other cores. Though there was also discussion on updating the documentation on the expected behavior: first read, then callback with the value read, or first callback, just before the read, and then read. Once that is decided, I think I could move on updating the PR accordingly.

[vadosnaprimer]

The problem is the same as before and that is that this way, deep freezing is not possible using on_bus_read.

While we were working on deep freeze for gpgx I pondered a few approaches. The current one is simply freezing individual bytes by restoring their values after every write. We discussed changing the value to be written right before it's written, also from inside the write callback.

The problem with changing it while inside the write callback is different widths of written values. If in a given scenario we only need to freeze 1 byte because we know the game uses that address that way, we won't be able to easily overwrite it because callbacks can access that address using a different width, and we don't want to freeze adjacent addresses, so we'll only need to change a part of the bigger value that goes to that one byte address. Or we may be freezing a 32-bit value, but the game may be writing to it using smaller sizes.

And overwriting it right after it's written while inside the write callback, changes the order of things, which may be problematic due to inconsistency.