Memory Hook Improvements (Return value) (#4283)

* new callback system with callback return values

If the lua callback returns a value, the core will update the addr with it. Otherwise, the old value sent by the core will be used unmodified

* update MemoryCallbackDelegate return value to uint?

* throw NotSupportedException for GBA memory callbacks

* docs: return value of MemoryCallbackDelegate and CallMemoryCallbacks

src/BizHawk.Client.Common/BreakpointList.cs

@@ -166,7 +166,7 @@ namespace BizHawk.Client.Common
 			_core.MemoryCallbacks.Remove(DoCallback);
 		}
 
-		private void DoCallback(uint address, uint value, uint flags)
+		private uint? DoCallback(uint address, uint value, uint flags)
 			=> Callback(address, value, flags);
 
 		public void ResetCallback()

src/BizHawk.Client.Common/lua/NamedLuaFunction.cs

@@ -60,12 +60,13 @@ namespace BizHawk.Client.Common
 			{
 				try
 				{
-					_function.Call(args);
+					return _function.Call(args);
 				}
 				catch (Exception ex)
 				{
 					logCallback($"error running function attached by the event {Event}\nError message: {ex.Message}");
 				}
+				return null;
 			};
 			InputCallback = () =>
 			{
@@ -84,7 +85,7 @@ namespace BizHawk.Client.Common
 				LuaLibraries.IsInInputOrMemoryCallback = true;
 				try
 				{
-					Callback(new object[] { addr, val, flags });
+					return Callback([ addr, val, flags ]) is [ long n ] ? unchecked((uint) n) : null;
 				}
 				finally
 				{
@@ -119,7 +120,7 @@ namespace BizHawk.Client.Common
 
 		public string Event { get; }
 
-		private Action<object[]> Callback { get; }
+		private Func<object[], object[]> Callback { get; }
 
 		public Action InputCallback { get; }
 

src/BizHawk.Client.EmuHawk/tools/Debugger/BreakpointControl.cs

@@ -53,14 +53,15 @@ namespace BizHawk.Client.EmuHawk
 				: Color.White;
 		}
 
-		private void BreakpointCallback(uint addr, uint value, uint flags)
+		private uint? BreakpointCallback(uint addr, uint value, uint flags)
 		{
 			MainForm.PauseEmulator();
 			ParentDebugger.UpdateForBreakpointHit();
 			MainForm.AddOnScreenMessage("Breakpoint hit");
+			return null;
 		}
 
-		private void SeekCallback(uint addr, uint value, uint flags)
+		private uint? SeekCallback(uint addr, uint value, uint flags)
 		{
 			BreakpointCallback(addr, value, flags);
 
@@ -73,6 +74,7 @@ namespace BizHawk.Client.EmuHawk
 			}
 
 			ParentDebugger.DisableCancelSeekBtn();
+			return null;
 		}
 
 		public void UpdateValues()

src/BizHawk.Emulation.Common/Base Implementations/CallbackBasedTraceBuffer.cs

@@ -42,7 +42,7 @@ namespace BizHawk.Emulation.Common
 
 		protected readonly List<TraceInfo> Buffer = new List<TraceInfo>();
 
-		protected abstract void TraceFromCallback(uint addr, uint value, uint flags);
+		protected abstract uint? TraceFromCallback(uint addr, uint value, uint flags);
 
 		private ITraceSink? _sink;
 

src/BizHawk.Emulation.Common/Base Implementations/MemoryBasedInputCallbackSystem.cs

@@ -34,12 +34,13 @@ namespace BizHawk.Emulation.Common
 			}
 		}
 
-		private void MemoryCallback(uint address, uint value, uint flags)
+		private uint? MemoryCallback(uint address, uint value, uint flags)
 		{
 			foreach (var action in _inputCallbacks)
 			{
 				action.Invoke();
 			}
+			return null;
 		}
 
 		public IEnumerator<Action> GetEnumerator() => _inputCallbacks.GetEnumerator();

src/BizHawk.Emulation.Common/Base Implementations/MemoryCallbackSystem.cs

@@ -66,29 +66,31 @@ namespace BizHawk.Emulation.Common
 			}
 		}
 
-		private static void Call(MemoryCallbackCollection cbs, uint addr, uint value, uint flags, string scope)
+		private static uint Call(MemoryCallbackCollection cbs, uint addr, uint value, uint flags, string scope)
 		{
+			uint cbReturn = value; //By default, if no callback is called, it will return the value sent by the core unmodified.
 			foreach (var cb in cbs)
 			{
 				if (!cb.Address.HasValue || (cb.Scope == scope && cb.Address == (addr & cb.AddressMask)))
 				{
-					cb.Callback(addr, value, flags);
+					if (cb.Callback(addr, value, flags) is uint n) cbReturn = n; //If many callbacks are registered to the same address, no matter the order, if one of them overrides the original value, that new value will be sent to the core, even if a second, third etc. callback doesn't return anything. If many callbacks try to override, only the last will be sent to the core.
 				}
 			}
+			return cbReturn;
 		}
 
-		public void CallMemoryCallbacks(uint addr, uint value, uint flags, string scope)
+		public uint CallMemoryCallbacks(uint addr, uint value, uint flags, string scope)
 		{
 			if (!_hasAny)
 			{
-				return;
+				return value;
 			}
 
 			if (HasReads)
 			{
 				if ((flags & (uint) MemoryCallbackFlags.AccessRead) != 0)
 				{
-					Call(_reads, addr, value, flags, scope);
+					value = Call(_reads, addr, value, flags, scope);
 				}
 			}
 
@@ -96,7 +98,7 @@ namespace BizHawk.Emulation.Common
 			{
 				if ((flags & (uint) MemoryCallbackFlags.AccessWrite) != 0)
 				{
-					Call(_writes, addr, value, flags, scope);
+					value = Call(_writes, addr, value, flags, scope);
 				}
 			}
 
@@ -104,9 +106,10 @@ namespace BizHawk.Emulation.Common
 			{
 				if ((flags & (uint) MemoryCallbackFlags.AccessExecute) != 0)
 				{
-					Call(_execs, addr, value, flags, scope);
+					value = Call(_execs, addr, value, flags, scope);
 				}
 			}
+			return value;
 		}
 
 		public bool HasReads { get; private set; }

src/BizHawk.Emulation.Common/Interfaces/IMemoryCallbackSystem.cs

@@ -5,7 +5,10 @@ using System.Collections.Generic;
 namespace BizHawk.Emulation.Common
 {
 	/// <param name="value">For reads/execs, the value read/executed; for writes, the value to be written. Cores may pass the default <c>0</c> if write/exec is partially implemented.</param>
-	public delegate void MemoryCallbackDelegate(uint address, uint value, uint flags);
+	/// <returns>
+	/// NULL if we should leave the value sent by the core as it is; the value to override with otherwise.
+	/// </returns>
+	public delegate uint? MemoryCallbackDelegate(uint address, uint value, uint flags);
 
 	/// <summary>
 	/// This is a property of <see cref="IDebuggable"/>, and defines the means by which a client
@@ -70,7 +73,10 @@ namespace BizHawk.Emulation.Common
 		/// <param name="value">For reads/execs, the value read/executed; for writes, the value to be written. Cores may pass the default <c>0</c> if write/exec is partially implemented.</param>
 		/// <param name="flags">The callback flags relevant to this access</param>
 		/// <param name="scope">The scope that the address pertains to. Must be a value in <see cref="AvailableScopes"/></param>
-		void CallMemoryCallbacks(uint addr, uint value, uint flags, string scope);
+		/// <returns>
+		/// The value to send back to the core. Will be the original untouched if MemoryCallbackDelegate returned NULL, the new value to override with otherwise.
+		/// </returns>
+		uint CallMemoryCallbacks(uint addr, uint value, uint flags, string scope);
 
 		/// <summary>
 		/// Removes the given callback from the list

src/BizHawk.Emulation.Cores/Consoles/Nintendo/GBA/MGBAMemoryCallbackSystem.cs

@@ -166,8 +166,9 @@ namespace BizHawk.Emulation.Cores.Nintendo.GBA
 		IEnumerator IEnumerable.GetEnumerator()
 			=> GetEnumerator();
 
-		public void CallMemoryCallbacks(uint addr, uint value, uint flags, string scope)
+		public uint CallMemoryCallbacks(uint addr, uint value, uint flags, string scope)
 		{
+			throw new NotSupportedException("Memory callbacks not supported.");
 			// Not a thing in this implementation
 		}
 

src/BizHawk.Emulation.Cores/Consoles/Sega/gpgx64/GPGX.IDebuggable.cs

@@ -77,24 +77,27 @@ namespace BizHawk.Emulation.Cores.Consoles.Sega.gpgx
 				if (MemoryCallbacks.HasExecutes)
 				{
 					const uint flags = (uint)MemoryCallbackFlags.AccessExecute;
-					MemoryCallbacks.CallMemoryCallbacks(addr, val, flags, "M68K BUS");
+					val = MemoryCallbacks.CallMemoryCallbacks(addr, val, flags, "M68K BUS");
 				}
+				return val;
 			};
 			ReadCallback = (addr, val) =>
 			{
 				if (MemoryCallbacks.HasReads)
 				{
 					const uint flags = (uint)MemoryCallbackFlags.AccessRead;
-					MemoryCallbacks.CallMemoryCallbacks(addr, val, flags, "M68K BUS");
+					val = MemoryCallbacks.CallMemoryCallbacks(addr, val, flags, "M68K BUS");
 				}
+				return val;
 			};
 			WriteCallback = (addr, val) =>
 			{
 				if (MemoryCallbacks.HasWrites)
 				{
 					const uint flags = (uint)MemoryCallbackFlags.AccessWrite;
-					MemoryCallbacks.CallMemoryCallbacks(addr, val, flags, "M68K BUS");
+					val = MemoryCallbacks.CallMemoryCallbacks(addr, val, flags, "M68K BUS");
 				}
+				return val;
 			};
 			_memoryCallbacks.ActiveChanged += RefreshMemCallbacks;
 		}

src/BizHawk.Emulation.Cores/Consoles/Sega/gpgx64/GPGX.ITraceable.cs

@@ -17,7 +17,7 @@ namespace BizHawk.Emulation.Cores.Consoles.Sega.gpgx
 		{
 			private const string TRACE_HEADER = "M68K: PC, machine code, mnemonic, operands, registers (A0-A7, D0-D7, SR, USP), flags (XNZVC)";
 
-			protected override void TraceFromCallback(uint addr, uint value, uint flags)
+			protected override uint? TraceFromCallback(uint addr, uint value, uint flags)
 			{
 				var regs = DebuggableCore.GetCpuFlagsAndRegisters();
 				var pc = (uint)regs["M68K PC"].Value & 0xFFFFFF;
@@ -47,6 +47,7 @@ namespace BizHawk.Emulation.Cores.Consoles.Sega.gpgx
 					(sr &  1) > 0 ? "C" : "c"));
 
 				this.Put(new(disassembly: $"{pc:X6}:  {disasm}".PadRight(50), registerInfo: sb.ToString().Trim()));
+				return null;
 			}
 		}
 	}

src/BizHawk.Emulation.Cores/Consoles/Sega/gpgx64/LibGPGX.cs

@@ -194,7 +194,7 @@ namespace BizHawk.Emulation.Cores.Consoles.Sega.gpgx
 		public abstract void gpgx_set_input_callback(input_cb cb);
 
 		[UnmanagedFunctionPointer(CallingConvention.Cdecl)]
-		public delegate void mem_cb(uint addr, uint value); //value MUST be uint, since the value will be trimmed if the type is byte (8-bit) or ushort (16-bit) and the value read/written/executed is bigger than that (i.e 32 bits).
+		public delegate uint mem_cb(uint addr, uint value); //value MUST be uint, since the value will be trimmed if the type is byte (8-bit) or ushort (16-bit) and the value read/written/executed is bigger than that (i.e 32 bits).
 
 		[UnmanagedFunctionPointer(CallingConvention.Cdecl)]
 		public delegate void CDCallback(int addr, CDLog_AddrType addrtype, CDLog_Flags flags);

src/BizHawk.Tests/Emulation.Common/Base Implementations/MemoryCallbackSystemTests.cs

@@ -206,6 +206,7 @@ namespace BizHawk.Tests.Emulation.Common
 				callback1invoked = true;
 				_memoryCallbackSystem.Add(callback2);
 				_memoryCallbackSystem.Add(callback3);
+				return null;
 			};
 
 			MemoryCallback callback1 = new(ScopeA, MemoryCallbackType.Read, "Callback 1", callback, null, null);
@@ -230,6 +231,7 @@ namespace BizHawk.Tests.Emulation.Common
 			{
 				callback2invoked = true;
 				_memoryCallbackSystem.Remove(callback1.Callback);
+				return null;
 			};
 
 			MemoryCallback callback2 = new(ScopeA, MemoryCallbackType.Read, "Callback 2", callback, null, null);
@@ -261,6 +263,7 @@ namespace BizHawk.Tests.Emulation.Common
 			{
 				callback2invoked = true;
 				_memoryCallbackSystem.Remove(callback2!.Callback);
+				return null;
 			};
 
 			callback2 = new(ScopeA, MemoryCallbackType.Read, "Callback 2", callback, null, null);
@@ -289,15 +292,20 @@ namespace BizHawk.Tests.Emulation.Common
 
 			public List<(uint Address, uint Value, uint Flags)> Callback3Invocations { get; } = new();
 
-			public void Callback1(uint address, uint value, uint flags)
-				=> Callback1Invocations.Add((address, value, flags));
+			public uint? Callback1(uint address, uint value, uint flags) {
+				Callback1Invocations.Add((address, value, flags));
+				return null;
+			}
 
-			public void Callback2(uint address, uint value, uint flags)
-				=> Callback2Invocations.Add((address, value, flags));
-
-			public void Callback3(uint address, uint value, uint flags)
-				=> Callback3Invocations.Add((address, value, flags));
+			public uint? Callback2(uint address, uint value, uint flags) {
+				Callback2Invocations.Add((address, value, flags));
+				return null;
+			}
 
+			public uint? Callback3(uint address, uint value, uint flags) {
+				Callback3Invocations.Add((address, value, flags));
+				return null;
+			}
 			public void Clear()
 			{
 				Callback1Invocations.Clear();

waterbox/gpgx/Genesis-Plus-GX

@@ -1 +1 @@
-Subproject commit 9fb7e671240d9a1e63b4effbf46b7c0abe3c6b91
+Subproject commit 79c243dee53291d4a004406200275d11c9875473

waterbox/gpgx/cinterface/callbacks.h

@@ -7,9 +7,9 @@
 
 typedef ECL_ENTRY void (*CDCallback)(int32 addr, int32 addrtype, int32 flags);
 
-extern ECL_ENTRY void (*biz_execcb)(unsigned addr, unsigned value);
-extern ECL_ENTRY void (*biz_readcb)(unsigned addr, unsigned value);
-extern ECL_ENTRY void (*biz_writecb)(unsigned addr, unsigned value);
+extern ECL_ENTRY unsigned (*biz_execcb)(unsigned addr, unsigned value);
+extern ECL_ENTRY unsigned (*biz_readcb)(unsigned addr, unsigned value);
+extern ECL_ENTRY unsigned (*biz_writecb)(unsigned addr, unsigned value);
 extern CDCallback biz_cdcb;
 
 extern ECL_ENTRY void (*cdd_readcallback)(int lba, void *dest, int subcode);

waterbox/gpgx/cinterface/cinterface.c

@@ -61,9 +61,9 @@ static uint8_t brm_format[0x40] =
 	0x52,0x41,0x4d,0x5f,0x43,0x41,0x52,0x54,0x52,0x49,0x44,0x47,0x45,0x5f,0x5f,0x5f
 };
 
-ECL_ENTRY void (*biz_execcb)(unsigned addr, unsigned int value);
-ECL_ENTRY void (*biz_readcb)(unsigned addr, unsigned int value);
-ECL_ENTRY void (*biz_writecb)(unsigned addr, unsigned int value);
+ECL_ENTRY unsigned (*biz_execcb)(unsigned addr, unsigned value);
+ECL_ENTRY unsigned (*biz_readcb)(unsigned addr, unsigned value);
+ECL_ENTRY unsigned (*biz_writecb)(unsigned addr, unsigned value);
 CDCallback biz_cdcb = NULL;
 ECL_ENTRY void (*cdd_readcallback)(int lba, void *dest, int subcode);
 uint8 *tempsram;
@@ -770,7 +770,7 @@ void CDLog68k(uint addr, uint flags)
 	}
 }
 
-void bk_cpu_hook(hook_type_t type, int width, unsigned int address, unsigned int value)
+unsigned bk_cpu_hook(hook_type_t type, int width, unsigned address, unsigned value)
 {
 	switch (type)
 	{
@@ -791,7 +791,7 @@ void bk_cpu_hook(hook_type_t type, int width, unsigned int address, unsigned int
 		case HOOK_M68K_R:
 		{
 			if (biz_readcb)
-				biz_readcb(address, value);
+				return biz_readcb(address, value);
 
 			break;
 		}
@@ -799,13 +799,14 @@ void bk_cpu_hook(hook_type_t type, int width, unsigned int address, unsigned int
 		case HOOK_M68K_W:
 		{
 			if (biz_writecb)
-				biz_writecb(address, value);
+				return biz_writecb(address, value);
 
 			break;
 		}
 
 		default: break;
 	}
+	return value;
 }
 
 #endif // USE_BIZHAWK_CALLBACKS
@@ -1022,7 +1023,7 @@ GPGX_EX void gpgx_reset(int hard)
 		gen_reset(0);
 }
 
-GPGX_EX void gpgx_set_mem_callback(ECL_ENTRY void (*read)(unsigned, unsigned), ECL_ENTRY void (*write)(unsigned, unsigned), ECL_ENTRY void (*exec)(unsigned, unsigned))
+GPGX_EX void gpgx_set_mem_callback(ECL_ENTRY unsigned (*read)(unsigned, unsigned), ECL_ENTRY unsigned (*write)(unsigned, unsigned), ECL_ENTRY unsigned (*exec)(unsigned, unsigned))
 {
 	biz_readcb = read;
 	biz_writecb = write;