Added an ASM test for self modifying code. Made the Test fail after 100k cycles to prevent infinite loop.

src/Spice86.Core/Emulator/CPU/CfgCpu/Feeder/CurrentInstructions.cs

@@ -64,20 +64,18 @@ private void CreateBreakpointsForInstruction(CfgInstruction instruction) {
              byteAddress++) {
             // When reached the breakpoint will clear the cache and the other breakpoints for the instruction
             AddressBreakPoint breakPoint = new AddressBreakPoint(BreakPointType.WRITE, byteAddress,
-                b => { OnBreakPointReached(b, instruction); }, false);
+                b => { OnBreakPointReached((AddressBreakPoint)b, instruction); }, false);
             breakpoints.Add(breakPoint);
             _machineBreakpoints.ToggleBreakPoint(breakPoint, true);
         }
     }
 
-    private void OnBreakPointReached(BreakPoint breakPoint, CfgInstruction instruction) {
-        if (breakPoint is AddressBreakPoint addressBreakPoint) {
-            // Check that value is effectively beeing modified
-            byte current = _memory.UInt8[addressBreakPoint.Address];
-            byte newValue = _memory.CurrentlyWritingByte;
-            if (current == newValue) {
-                return;
-            }
+    private void OnBreakPointReached(AddressBreakPoint addressBreakPoint, CfgInstruction instruction) {
+        // Check that value is effectively beeing modified
+        byte current = _memory.UInt8[addressBreakPoint.Address];
+        byte newValue = _memory.CurrentlyWritingByte;
+        if (current == newValue) {
+            return;
         }
         ClearCurrentInstruction(instruction);
     }

src/Spice86.Core/Emulator/CPU/CfgCpu/Feeder/InstructionsFeeder.cs

@@ -37,7 +37,7 @@ public CfgInstruction GetInstructionFromMemory(SegmentedAddress address) {
         // First try to see if it has been encountered before at this address instead of re-parsing.
         // Reason is we don't want several versions of the same instructions hanging around in the graph,
         // this would be bad for successors / predecessors management and self modifying code detection.
-        CfgInstruction? previousMatching = _previousInstructions.GetAtAddress(address);
+        CfgInstruction? previousMatching = _previousInstructions.GetAtAddressIfMatchesMemory(address);
         if (previousMatching != null) {
             _currentInstructions.SetAsCurrent(previousMatching);
             return previousMatching;

src/Spice86.Core/Emulator/CPU/CfgCpu/Feeder/PreviousInstructions.cs

@@ -19,7 +19,7 @@ public PreviousInstructions(IMemory memory) {
         _memoryInstructionMatcher = new MemoryInstructionMatcher(memory);
     }
 
-    public CfgInstruction? GetAtAddress(SegmentedAddress address) {
+    public CfgInstruction? GetAtAddressIfMatchesMemory(SegmentedAddress address) {
         if (_previousInstructionsAtAddress.TryGetValue(address,
                 out HashSet<CfgInstruction>? previousInstructionsAtAddress)) {
             return _memoryInstructionMatcher.MatchExistingInstructionWithMemory(previousInstructionsAtAddress);

src/Spice86.Core/Emulator/ProgramExecutor.cs

@@ -30,8 +30,6 @@ public sealed class ProgramExecutor : IProgramExecutor {
     private readonly GdbServer? _gdbServer;
     private readonly EmulationLoop _emulationLoop;
 
-    private bool ListensToBreakpoints => _configuration.GdbPort != null || _configuration.DumpDataOnExit is not false;
-
     /// <summary>
     /// Initializes a new instance of <see cref="ProgramExecutor"/>
     /// </summary>
@@ -43,7 +41,7 @@ public ProgramExecutor(Configuration configuration, ILoggerService loggerService
         _loggerService = loggerService;
         Machine = CreateMachine(gui);
         _gdbServer = CreateGdbServer(gui);
-        _emulationLoop = new(loggerService, Machine.Cpu, Machine.CfgCpu, Machine.CpuState, Machine.Timer, ListensToBreakpoints, Machine.MachineBreakpoints, Machine.DmaController, _gdbServer?.GdbCommandHandler);
+        _emulationLoop = new(loggerService, Machine.Cpu, Machine.CfgCpu, Machine.CpuState, Machine.Timer, Machine.MachineBreakpoints, Machine.DmaController, _gdbServer?.GdbCommandHandler);
     }
 
     /// <summary>
@@ -55,7 +53,7 @@ public ProgramExecutor(Configuration configuration, ILoggerService loggerService
     public void Run() {
         _gdbServer?.StartServerAndWait();
         _emulationLoop.Run();
-        if (ListensToBreakpoints) {
+        if (_configuration.DumpDataOnExit is not false) {
             DumpEmulatorStateToDirectory(_configuration.RecordedDataDirectory);
         }
     }
@@ -150,10 +148,10 @@ private ExecutableFileLoader CreateExecutableFileLoader(Configuration configurat
     private Machine CreateMachine(IGui? gui) {
         CounterConfigurator counterConfigurator = new CounterConfigurator(_configuration, _loggerService);
         RecordedDataReader reader = new RecordedDataReader(_configuration.RecordedDataDirectory, _loggerService);
-        ExecutionFlowRecorder executionFlowRecorder = reader.ReadExecutionFlowRecorderFromFileOrCreate(ListensToBreakpoints);
+        ExecutionFlowRecorder executionFlowRecorder = reader.ReadExecutionFlowRecorderFromFileOrCreate(_configuration.DumpDataOnExit is not false);
         State cpuState = new();
         IOPortDispatcher ioPortDispatcher = new IOPortDispatcher(cpuState, _loggerService, _configuration.FailOnUnhandledPort);
-        Machine = new Machine(gui, cpuState, ioPortDispatcher, _loggerService, counterConfigurator, executionFlowRecorder, _configuration, ListensToBreakpoints);
+        Machine = new Machine(gui, cpuState, ioPortDispatcher, _loggerService, counterConfigurator, executionFlowRecorder, _configuration, _configuration.DumpDataOnExit is not false);
         ExecutableFileLoader loader = CreateExecutableFileLoader(_configuration);
         if (_configuration.InitializeDOS is null) {
             _configuration.InitializeDOS = loader.DosInitializationNeeded;

src/Spice86.Core/Emulator/VM/EmulationLoop.cs

@@ -44,18 +44,16 @@ public class EmulationLoop {
     /// <param name="cpu">The emulated CPU, so the emulation loop can call ExecuteNextInstruction().</param>
     /// <param name="cpuState">The emulated CPU State, so that we know when to stop.</param>
     /// <param name="timer">The timer device, so the emulation loop can call Tick()</param>
-    /// <param name="listensToBreakpoints">Whether we react to breakpoints in the emulation loop.</param>
     /// <param name="machineBreakpoints">The class that stores emulation breakpoints.</param>
     /// <param name="dmaController">The DMA Controller, to start the DMA loop thread.</param>
     /// <param name="gdbCommandHandler">The GDB Command Handler, used to trigger a GDB breakpoint on pause.</param>
-    public EmulationLoop(ILoggerService loggerService, Cpu cpu, CfgCpu cfgCpu, State cpuState, Devices.Timer.Timer timer, bool listensToBreakpoints, MachineBreakpoints machineBreakpoints,
+    public EmulationLoop(ILoggerService loggerService, Cpu cpu, CfgCpu cfgCpu, State cpuState, Devices.Timer.Timer timer, MachineBreakpoints machineBreakpoints,
         DmaController dmaController, GdbCommandHandler? gdbCommandHandler) {
         _loggerService = loggerService;
         _cpu = cpu;
         _cfgCpu = cfgCpu;
         _cpuState = cpuState;
         _timer = timer;
-        _listensToBreakpoints = listensToBreakpoints;
         _machineBreakpoints = machineBreakpoints;
         _dmaController = dmaController;
         _gdbCommandHandler = gdbCommandHandler;
@@ -104,9 +102,7 @@ private void RunLoop() {
         _stopwatch.Start();
         while (_cpuState.IsRunning) {
             PauseIfAskedTo();
-            if (_listensToBreakpoints) {
-                _machineBreakpoints.CheckBreakPoint();
-            }
+            _machineBreakpoints.CheckBreakPoint();
             _cpu.ExecuteNextInstruction();
             //_cfgCpu.ExecuteNext();
             _timer.Tick();

tests/Spice86.Tests/MachineTest.cs

@@ -268,6 +268,16 @@ public void TestSub() {
         TestOneBin("sub");
     }
 
+    [Fact]
+    public void TestSelfModify() {
+        byte[] expected = new byte[4];
+        expected[0x00] = 0x01;
+        expected[0x01] = 0x00;
+        expected[0x02] = 0xff;
+        expected[0x03] = 0xff;
+        TestOneBin("selfmodify", expected);
+    }
+
     [Theory]
     [InlineData(0b0011110000000000, 0b0010000000000001, 0, 0b0011110000000000, true, true)] // result is same as dest, flags unaffected
     [InlineData(0b0000000000000001, 0b0000000000000000, 1, 0b0000000000000010, false, false)] // shift one bit 
@@ -333,18 +343,23 @@ private Machine TestOneBin(string binName) {
     private Machine TestOneBin(string binName, byte[] expected) {
         Machine machine = Execute(binName);
         IMemory memory = machine.Memory;
-        CompareMemoryWithExpected(memory, expected, 0, expected.Length - 1);
+        CompareMemoryWithExpected(memory, expected, 0, expected.Length);
         return machine;
     }
 
     private ProgramExecutor CreateProgramExecutor(string binName, bool recordData = false) {
         return new MachineCreator().CreateProgramExecutorFromBinName(binName, recordData);
     }
 
+    [AssertionMethod]
     private Machine Execute(string binName) {
         using ProgramExecutor programExecutor = CreateProgramExecutor(binName);
+        // Add a breakpoint after a million cycles to ensure no infinite loop can lock the tests
+        programExecutor.Machine.MachineBreakpoints.ToggleBreakPoint(new AddressBreakPoint(BreakPointType.CYCLES, 100000L,
+            (breakpoint) => {
+                Assert.Fail($"Test ran for {((AddressBreakPoint)breakpoint).Address} cycles, something is wrong.");
+            }, true), true);
         programExecutor.Run();
-        //new StringsOverrides(new(), programExecutor.Machine).entry_F000_FFF0_FFFF0(0);
         return programExecutor.Machine;
     }
 

tests/Spice86.Tests/Resources/cpuTests/asmsrc/selfmodify.asm

@@ -0,0 +1,22 @@
+;00: 01 00 ff ff
+; compile it with fasm
+use16
+start:
+mov ax,0
+mov ss,ax
+mov sp,4
+selmodifiedmov:
+mov ax,0ffffh
+push ax
+mov word[cs:selmodifiedmov+1],0001h ; modify value to put in AX next time
+; loop back to mov if AX is 0ffffh
+cmp ax,word 0ffffh
+je selmodifiedmov
+;stack should be 0100ffff
+hlt
+
+; bios entry point at offset fff0
+rb 65520-$
+jmp start
+rb 65535-$
+db 0ffh