From 8f514f8d0eb3a26a7154cc322bf08a82ba6114de Mon Sep 17 00:00:00 2001 From: Kevin Ferrare Date: Thu, 4 Jul 2024 00:13:07 +0200 Subject: [PATCH] Added an ASM test for self modifying code. Made the Test fail after 100k cycles to prevent infinite loop. --- .../CPU/CfgCpu/Feeder/CurrentInstructions.cs | 16 ++++++------- .../CPU/CfgCpu/Feeder/InstructionsFeeder.cs | 2 +- .../CPU/CfgCpu/Feeder/PreviousInstructions.cs | 2 +- src/Spice86.Core/Emulator/ProgramExecutor.cs | 10 ++++---- src/Spice86.Core/Emulator/VM/EmulationLoop.cs | 8 ++----- tests/Spice86.Tests/MachineTest.cs | 19 +++++++++++++-- .../Resources/cpuTests/asmsrc/selfmodify.asm | 22 ++++++++++++++++++ .../Resources/cpuTests/selfmodify.bin | Bin 0 -> 65536 bytes 8 files changed, 54 insertions(+), 25 deletions(-) create mode 100644 tests/Spice86.Tests/Resources/cpuTests/asmsrc/selfmodify.asm create mode 100644 tests/Spice86.Tests/Resources/cpuTests/selfmodify.bin diff --git a/src/Spice86.Core/Emulator/CPU/CfgCpu/Feeder/CurrentInstructions.cs b/src/Spice86.Core/Emulator/CPU/CfgCpu/Feeder/CurrentInstructions.cs index 4bbad94245..8c90b98bd9 100644 --- a/src/Spice86.Core/Emulator/CPU/CfgCpu/Feeder/CurrentInstructions.cs +++ b/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); } diff --git a/src/Spice86.Core/Emulator/CPU/CfgCpu/Feeder/InstructionsFeeder.cs b/src/Spice86.Core/Emulator/CPU/CfgCpu/Feeder/InstructionsFeeder.cs index 4a059566fb..d355e1f22f 100644 --- a/src/Spice86.Core/Emulator/CPU/CfgCpu/Feeder/InstructionsFeeder.cs +++ b/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; diff --git a/src/Spice86.Core/Emulator/CPU/CfgCpu/Feeder/PreviousInstructions.cs b/src/Spice86.Core/Emulator/CPU/CfgCpu/Feeder/PreviousInstructions.cs index 280f92e00b..5c9b5f7f89 100644 --- a/src/Spice86.Core/Emulator/CPU/CfgCpu/Feeder/PreviousInstructions.cs +++ b/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? previousInstructionsAtAddress)) { return _memoryInstructionMatcher.MatchExistingInstructionWithMemory(previousInstructionsAtAddress); diff --git a/src/Spice86.Core/Emulator/ProgramExecutor.cs b/src/Spice86.Core/Emulator/ProgramExecutor.cs index b638a1c6b8..f56861e321 100644 --- a/src/Spice86.Core/Emulator/ProgramExecutor.cs +++ b/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; - /// /// Initializes a new instance of /// @@ -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); } /// @@ -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; diff --git a/src/Spice86.Core/Emulator/VM/EmulationLoop.cs b/src/Spice86.Core/Emulator/VM/EmulationLoop.cs index 8f4b0103fa..67e7274b43 100644 --- a/src/Spice86.Core/Emulator/VM/EmulationLoop.cs +++ b/src/Spice86.Core/Emulator/VM/EmulationLoop.cs @@ -44,18 +44,16 @@ public class EmulationLoop { /// The emulated CPU, so the emulation loop can call ExecuteNextInstruction(). /// The emulated CPU State, so that we know when to stop. /// The timer device, so the emulation loop can call Tick() - /// Whether we react to breakpoints in the emulation loop. /// The class that stores emulation breakpoints. /// The DMA Controller, to start the DMA loop thread. /// The GDB Command Handler, used to trigger a GDB breakpoint on pause. - 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(); diff --git a/tests/Spice86.Tests/MachineTest.cs b/tests/Spice86.Tests/MachineTest.cs index aa30f90dcc..11c0ca17ea 100644 --- a/tests/Spice86.Tests/MachineTest.cs +++ b/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,7 +343,7 @@ 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; } @@ -341,10 +351,15 @@ private ProgramExecutor CreateProgramExecutor(string binName, bool recordData = 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; } diff --git a/tests/Spice86.Tests/Resources/cpuTests/asmsrc/selfmodify.asm b/tests/Spice86.Tests/Resources/cpuTests/asmsrc/selfmodify.asm new file mode 100644 index 0000000000..e0b1b5c792 --- /dev/null +++ b/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 diff --git a/tests/Spice86.Tests/Resources/cpuTests/selfmodify.bin b/tests/Spice86.Tests/Resources/cpuTests/selfmodify.bin new file mode 100644 index 0000000000000000000000000000000000000000..3ac1d956c6a6052ca8c804a54da8f7d621a186d0 GIT binary patch literal 65536 zcmeIuu?;`~6h+Y|5sAhqHn4+X%wZmdUTY7HLjToPkaIVUlfB*pk1PBlyK!5-N0t5&UAV7cs0RjXF5FkK+009C72oNAZfB*pk1PBlyK!5-N z0t5&UAV7cs0RjXF5FkK+009C72oNAZfB*pk1PBlyK!5-N0t5&UAV7cs0RjXF5FkK+ z009C72oNAZfB*pk1PBlyK!5-N0t5&UAV7cs0RjXF5FkK+009C72oNAZfB*pk1PBly zK!5-N0t5&UAV7cs0RjXF5FkK+009C72oNAZfB*pk1PBlyK!5-N0t5&UAV7cs0RjXF z5FkK+009C72oNAZfB*pk1PBlyK!5-N0t5&UAV7cs0RjXF5FkK+009C72oNAZfB*pk j1PBlyK!5-N0t5&UAV7cs0RjXF5FkK+z@G)K;d56nCY%jz literal 0 HcmV?d00001