Repository: RetroAchievements/RAIntegration
Pull request #863 (2022-01-30 20:42:45)

use correct address for notification event when not on first memory block
Fixes issue where a frozen memory address on a memory block other than the first (usually labelled as something other than System RAM) could not be modified.

The address being modified was mutated to reflect the offset within the appropriate block. This was then passed as the modified address to the event handlers, which were expecting the original address. As a result, the bookmark was not immediately updated and the frozen value was written back into memory on the next frame.


Patch:
diff --git a/src/data/context/EmulatorContext.cpp b/src/data/context/EmulatorContext.cpp
index 39875dce..1de5ea7a 100644
--- a/src/data/context/EmulatorContext.cpp
+++ b/src/data/context/EmulatorContext.cpp
@@ -790,14 +790,15 @@ uint32_t EmulatorContext::ReadMemory(ra::ByteAddress nAddress, MemSize nSize) co
 
 void EmulatorContext::WriteMemoryByte(ra::ByteAddress nAddress, uint8_t nValue) const
 {
+    auto nBlockAddress = nAddress;
     for (const auto& pBlock : m_vMemoryBlocks)
     {
-        if (nAddress < pBlock.size)
+        if (nBlockAddress < pBlock.size)
         {
             if (!pBlock.write)
                 return;
 
-            pBlock.write(nAddress, nValue);
+            pBlock.write(nBlockAddress, nValue);
             m_bMemoryModified = true;
 
             // create a copy of the list of pointers in case it's modified by one of the callbacks
@@ -811,7 +812,7 @@ void EmulatorContext::WriteMemoryByte(ra::ByteAddress nAddress, uint8_t nValue)
             return;
         }
 
-        nAddress -= gsl::narrow_cast<ra::ByteAddress>(pBlock.size);
+        nBlockAddress -= gsl::narrow_cast<ra::ByteAddress>(pBlock.size);
     }
 }
 


Test patch:
diff --git a/tests/data/context/EmulatorContext_Tests.cpp b/tests/data/context/EmulatorContext_Tests.cpp
index df15e4f8..264ef890 100644
--- a/tests/data/context/EmulatorContext_Tests.cpp
+++ b/tests/data/context/EmulatorContext_Tests.cpp
@@ -82,6 +82,19 @@ private:
         }
     };
 
+    class EmulatorContextNotifyHarness : public EmulatorContext::NotifyTarget
+    {
+    public:
+        void OnByteWritten(ra::ByteAddress nAddress, uint8_t nValue) noexcept override
+        {
+            nLastByteWrittenAddress = nAddress;
+            nLastByteWritten = nValue;
+        }
+
+        ra::ByteAddress nLastByteWrittenAddress = 0xFFFFFFFFU;
+        uint8_t nLastByteWritten = 0xFF;
+    };
+
 public:
     TEST_METHOD(TestClientName)
     {
@@ -1342,6 +1355,42 @@ public:
             Assert::AreEqual(i, memory.at(i));
     }
 
+    TEST_METHOD(TestWriteMemoryByteEvents)
+    {
+        InitializeMemory();
+
+        EmulatorContextHarness emulator;
+        emulator.AddMemoryBlock(0, 20, &ReadMemory0, &WriteMemory0);
+        emulator.AddMemoryBlock(1, 10, &ReadMemory2, &WriteMemory2);
+        Assert::AreEqual({ 30U }, emulator.TotalMemorySize());
+
+        EmulatorContextNotifyHarness notify;
+        emulator.AddNotifyTarget(notify);
+
+        // write to first block
+        uint8_t nByte = 0xCE;
+        emulator.WriteMemoryByte(6U, nByte);
+        Assert::AreEqual(6U, notify.nLastByteWrittenAddress);
+        Assert::AreEqual(nByte, notify.nLastByteWritten);
+
+        // write to second block
+        nByte = 0xEE;
+        emulator.WriteMemoryByte(26U, nByte);
+        Assert::AreEqual(26U, notify.nLastByteWrittenAddress);
+        Assert::AreEqual(nByte, notify.nLastByteWritten);
+
+        // write to invalid address
+        emulator.WriteMemoryByte(36U, nByte + 3);
+        Assert::AreEqual(26U, notify.nLastByteWrittenAddress);
+        Assert::AreEqual(nByte, notify.nLastByteWritten);
+
+        // write partial value
+        nByte = 0xC4;
+        emulator.WriteMemory(6U, MemSize::Nibble_Lower, 0x04);
+        Assert::AreEqual(6U, notify.nLastByteWrittenAddress);
+        Assert::AreEqual(nByte, notify.nLastByteWritten);
+    }
+
     TEST_METHOD(TestWriteMemory)
     {
         memory.at(4) = 0xA8;