From e72cc4d0d522bee83faabde4899f01589e0ff4dc Mon Sep 17 00:00:00 2001
From: Matthew Budd <mjbudd77@gmail.com>
Date: Tue, 3 Nov 2020 06:36:30 -0500
Subject: [PATCH] Bug fix for issue #217. The Qt hex editor memory reads are
 now synchronized with emulation thread execution. This ensures that calls to
 GetMem will not improperly interfere with certain memory mapped registers
 while the emulation thread is executing. Reading at an inappropriate time
 from controller registers mapped at addresses $4016 and $4017 can cause the
 emulator to miss button presses. Thread synchronization fixes this.

---
 src/drivers/Qt/ConsoleDebugger.cpp |  8 +++++-
 src/drivers/Qt/HexEditor.cpp       | 45 +++++++++++++++++++++++++++++-
 src/drivers/Qt/HexEditor.h         |  1 +
 src/drivers/Qt/fceuWrapper.cpp     |  2 ++
 4 files changed, 54 insertions(+), 2 deletions(-)

diff --git a/src/drivers/Qt/ConsoleDebugger.cpp b/src/drivers/Qt/ConsoleDebugger.cpp
index db4439e2..111aa93c 100644
--- a/src/drivers/Qt/ConsoleDebugger.cpp
+++ b/src/drivers/Qt/ConsoleDebugger.cpp
@@ -1427,9 +1427,9 @@ void ConsoleDebugger::reloadSymbolsCB(void)
 {
 	fceuWrapperLock();
 	debugSymbolTable.loadGameSymbols();
-	fceuWrapperUnLock();
 
 	asmView->updateAssemblyView();
+	fceuWrapperUnLock();
 }
 //----------------------------------------------------------------------------
 void ConsoleDebugger::debugRunCB(void)
@@ -2812,7 +2812,9 @@ void QAsmView::setDisplayROMoffsets( bool value )
 	{
 		displayROMoffsets = value;
 
+		fceuWrapperLock();
 		updateAssemblyView();
+		fceuWrapperUnLock();
 	}
 }
 //----------------------------------------------------------------------------
@@ -2822,7 +2824,9 @@ void QAsmView::setSymbolDebugEnable( bool value )
 	{
 		symbolicDebugEnable = value;
 
+		fceuWrapperLock();
 		updateAssemblyView();
+		fceuWrapperUnLock();
 	}
 }
 //----------------------------------------------------------------------------
@@ -2832,7 +2836,9 @@ void QAsmView::setRegisterNameEnable( bool value )
 	{
 		registerNameEnable = value;
 
+		fceuWrapperLock();
 		updateAssemblyView();
+		fceuWrapperUnLock();
 	}
 }
 //----------------------------------------------------------------------------
diff --git a/src/drivers/Qt/HexEditor.cpp b/src/drivers/Qt/HexEditor.cpp
index 11a58326..d9663e12 100644
--- a/src/drivers/Qt/HexEditor.cpp
+++ b/src/drivers/Qt/HexEditor.cpp
@@ -41,6 +41,7 @@
 #include "Qt/ConsoleUtilities.h"
 #include "Qt/ConsoleWindow.h"
 
+static bool memNeedsCheck = false;
 static HexBookMarkManager_t hbm;
 static std::list <HexEditorDialog_t*> winList;
 static const char *memViewNames[] = { "RAM", "PPU", "OAM", "ROM", NULL };
@@ -724,7 +725,12 @@ HexEditorDialog_t::HexEditorDialog_t(QWidget *parent)
 
 	periodicTimer->start( 100 ); // 10hz
 
+	// Lock the mutex before adding a new window to the list,
+	// we want to be sure that the emulator is not iterating the list
+	// when we change it.
+	fceuWrapperLock();
 	winList.push_back(this);
+	fceuWrapperUnLock();
 
 	populateBookmarkMenu();
 
@@ -737,6 +743,11 @@ HexEditorDialog_t::~HexEditorDialog_t(void)
 	printf("Hex Editor Deleted\n");
 	periodicTimer->stop();
 
+	// Lock the emulation thread mutex to ensure
+	// that the emulator is not attempting to update memory values
+	// for window while we are destroying it or editing the window list.
+	fceuWrapperLock();
+
 	for (it = winList.begin(); it != winList.end(); it++)
 	{
 		if ( (*it) == this )
@@ -746,6 +757,7 @@ HexEditorDialog_t::~HexEditorDialog_t(void)
 			break;
 		}
 	}
+	fceuWrapperUnLock();
 }
 //----------------------------------------------------------------------------
 void HexEditorDialog_t::setWindowTitle(void)
@@ -1103,7 +1115,18 @@ void HexEditorDialog_t::updatePeriodic(void)
 {
 	//printf("Update Periodic\n");
 
-	editor->checkMemActivity();
+	if ( fceuWrapperTryLock(0) )
+	{
+		memNeedsCheck = false;
+
+		editor->checkMemActivity();
+
+		fceuWrapperUnLock();
+	}
+	else
+	{
+		memNeedsCheck = true;
+	}
 
 	editor->memModeUpdate();
 
@@ -2009,6 +2032,9 @@ void QHexEdit::jumpToROM(void)
 	setAddr( jumpToRomValue );
 }
 //----------------------------------------------------------------------------
+// Calling of checkMemActivity must always be synchronized with the emulation
+// thread as calling GetMem while the emulation is executing can mess up certain
+// registers (especially controller registers $4016 and $4017)
 int QHexEdit::checkMemActivity(void)
 {
 	int c;
@@ -2442,3 +2468,20 @@ int hexEditorOpenFromDebugger( int mode, int addr )
 	return 0;
 }
 //----------------------------------------------------------------------------
+// This function must be called from within the emulation thread
+void hexEditorUpdateMemoryValues(void)
+{
+	std::list <HexEditorDialog_t*>::iterator it;
+
+	if ( !memNeedsCheck )
+	{
+		return;
+	}
+
+	for (it = winList.begin(); it != winList.end(); it++)
+	{
+		(*it)->editor->checkMemActivity();
+	}
+	memNeedsCheck = false;
+}
+//----------------------------------------------------------------------------
diff --git a/src/drivers/Qt/HexEditor.h b/src/drivers/Qt/HexEditor.h
index d0fb5b9d..0ae73b7a 100644
--- a/src/drivers/Qt/HexEditor.h
+++ b/src/drivers/Qt/HexEditor.h
@@ -248,6 +248,7 @@ class HexEditorDialog_t : public QDialog
 };
 
 int hexEditorNumWindows(void);
+void hexEditorUpdateMemoryValues(void);
 void hexEditorLoadBookmarks(void);
 void hexEditorSaveBookmarks(void);
 int hexEditorOpenFromDebugger( int mode, int addr );
diff --git a/src/drivers/Qt/fceuWrapper.cpp b/src/drivers/Qt/fceuWrapper.cpp
index 4ab56df4..e025641a 100644
--- a/src/drivers/Qt/fceuWrapper.cpp
+++ b/src/drivers/Qt/fceuWrapper.cpp
@@ -1043,6 +1043,8 @@ int  fceuWrapperUpdate( void )
 	{
 		DoFun(frameskip, periodic_saves);
 	
+		hexEditorUpdateMemoryValues();
+
 		fceuWrapperUnLock();
 
 		emulatorHasMutux = 0;