fix(debug): Implement reference counter and thread safety in DbgHelpLoader to allow for independent users

Core/Libraries/Source/WWVegas/WWLib/DbgHelpGuard.cpp

@@ -22,7 +22,7 @@
 
 
 DbgHelpGuard::DbgHelpGuard()
-	: m_hasLoaded(false)
+	: m_needsUnload(false)
 {
 	activate();
 }
@@ -34,26 +34,16 @@ DbgHelpGuard::~DbgHelpGuard()
 
 void DbgHelpGuard::activate()
 {
-	if (DbgHelpLoader::isLoadedFromSystem())
-	{
-		// This is ok. Do nothing.
-	}
-	else if (DbgHelpLoader::isLoaded())
-	{
-		// This is maybe not ok. But do nothing until this becomes a user facing problem.
-	}
-	else
-	{
-		// Front load the DLL now to prevent other code from loading the potentially wrong DLL.
-		m_hasLoaded = DbgHelpLoader::load();
-	}
+	// Front load the DLL now to prevent other code from loading the potentially wrong DLL.
+	DbgHelpLoader::load();
+	m_needsUnload = true;
 }
 
 void DbgHelpGuard::deactivate()
 {
-	if (m_hasLoaded)
+	if (m_needsUnload)
 	{
 		DbgHelpLoader::unload();
-		m_hasLoaded = false;
+		m_needsUnload = false;
 	}
 }

Core/Libraries/Source/WWVegas/WWLib/DbgHelpGuard.h

@@ -40,5 +40,5 @@ class DbgHelpGuard
 
 private:
 
-	bool m_hasLoaded;
+	bool m_needsUnload;
 };

Core/Libraries/Source/WWVegas/WWLib/DbgHelpLoader.cpp

@@ -20,6 +20,7 @@
 
 
 DbgHelpLoader* DbgHelpLoader::Inst = NULL;
+CriticalSectionClass DbgHelpLoader::CriticalSection;
 
 DbgHelpLoader::DbgHelpLoader()
 	: m_symInitialize(NULL)
@@ -33,6 +34,7 @@ DbgHelpLoader::DbgHelpLoader()
 	, m_symFunctionTableAccess(NULL)
 	, m_stackWalk(NULL)
 	, m_dllModule(HMODULE(0))
+	, m_referenceCount(0)
 	, m_failed(false)
 	, m_loadedFromSystem(false)
 {
@@ -44,27 +46,47 @@ DbgHelpLoader::~DbgHelpLoader()
 
 bool DbgHelpLoader::isLoaded()
 {
+	CriticalSectionClass::LockClass lock(CriticalSection);
+
 	return Inst != NULL && Inst->m_dllModule != HMODULE(0);
 }
 
 bool DbgHelpLoader::isLoadedFromSystem()
 {
-	return Inst != NULL && Inst->m_loadedFromSystem;
+	CriticalSectionClass::LockClass lock(CriticalSection);
+
+	return isLoaded() && Inst->m_loadedFromSystem;
+}
+
+bool DbgHelpLoader::isFailed()
+{
+	CriticalSectionClass::LockClass lock(CriticalSection);
+
+	return Inst != NULL && Inst->m_failed;
 }
 
 bool DbgHelpLoader::load()
 {
+	CriticalSectionClass::LockClass lock(CriticalSection);
+
 	if (Inst == NULL)
 	{
 		// Cannot use new/delete here when this is loaded during game memory initialization.
 		void* p = GlobalAlloc(GMEM_FIXED, sizeof(DbgHelpLoader));
 		Inst = new (p) DbgHelpLoader();
 	}
 
+	// Always increment the reference count.
+	++Inst->m_referenceCount;
+
 	// Optimization: return early if it failed before.
 	if (Inst->m_failed)
 		return false;
 
+	// Return early if someone else already loaded it.
+	if (Inst->m_referenceCount > 1)
+		return true;
+
 	// Try load dbghelp.dll from the system directory first.
 	char dllFilename[MAX_PATH];
 	::GetSystemDirectoryA(dllFilename, ARRAY_SIZE(dllFilename));
@@ -99,25 +121,35 @@ bool DbgHelpLoader::load()
 
 	if (Inst->m_symInitialize == NULL || Inst->m_symCleanup == NULL)
 	{
-		unload();
+		freeResources();
 		Inst->m_failed = true;
 		return false;
 	}
 
 	return true;
 }
 
-bool DbgHelpLoader::reload()
-{
-	unload();
-	return load();
-}
-
 void DbgHelpLoader::unload()
 {
+	CriticalSectionClass::LockClass lock(CriticalSection);
+
 	if (Inst == NULL)
 		return;
 
+	if (--Inst->m_referenceCount != 0)
+		return;
+
+	freeResources();
+
+	Inst->~DbgHelpLoader();
+	GlobalFree(Inst);
+	Inst = NULL;
+}
+
+void DbgHelpLoader::freeResources()
+{
+	// Is private. Needs no locking.
+
 	while (!Inst->m_initializedProcesses.empty())
 	{
 		symCleanup(*Inst->m_initializedProcesses.begin());
@@ -129,16 +161,27 @@ void DbgHelpLoader::unload()
 		Inst->m_dllModule = HMODULE(0);
 	}
 
-	Inst->~DbgHelpLoader();
-	GlobalFree(Inst);
-	Inst = NULL;
+	Inst->m_symInitialize = NULL;
+	Inst->m_symCleanup = NULL;
+	Inst->m_symLoadModule = NULL;
+	Inst->m_symUnloadModule = NULL;
+	Inst->m_symGetModuleBase = NULL;
+	Inst->m_symGetSymFromAddr = NULL;
+	Inst->m_symGetLineFromAddr = NULL;
+	Inst->m_symSetOptions = NULL;
+	Inst->m_symFunctionTableAccess = NULL;
+	Inst->m_stackWalk = NULL;
+
+	Inst->m_loadedFromSystem = false;
 }
 
 BOOL DbgHelpLoader::symInitialize(
 	HANDLE hProcess,
 	LPSTR UserSearchPath,
 	BOOL fInvadeProcess)
 {
+	CriticalSectionClass::LockClass lock(CriticalSection);
+
 	if (Inst == NULL)
 		return FALSE;
 
@@ -164,6 +207,8 @@ BOOL DbgHelpLoader::symInitialize(
 BOOL DbgHelpLoader::symCleanup(
 	HANDLE hProcess)
 {
+	CriticalSectionClass::LockClass lock(CriticalSection);
+
 	if (Inst == NULL)
 		return FALSE;
 
@@ -186,6 +231,8 @@ BOOL DbgHelpLoader::symLoadModule(
 	DWORD BaseOfDll,
 	DWORD SizeOfDll)
 {
+	CriticalSectionClass::LockClass lock(CriticalSection);
+
 	if (Inst != NULL && Inst->m_symLoadModule)
 		return Inst->m_symLoadModule(hProcess, hFile, ImageName, ModuleName, BaseOfDll, SizeOfDll);
 
@@ -196,6 +243,8 @@ DWORD DbgHelpLoader::symGetModuleBase(
 	HANDLE hProcess,
 	DWORD dwAddr)
 {
+	CriticalSectionClass::LockClass lock(CriticalSection);
+
 	if (Inst != NULL && Inst->m_symGetModuleBase)
 		return Inst->m_symGetModuleBase(hProcess, dwAddr);
 
@@ -206,6 +255,8 @@ BOOL DbgHelpLoader::symUnloadModule(
 	HANDLE hProcess,
 	DWORD BaseOfDll)
 {
+	CriticalSectionClass::LockClass lock(CriticalSection);
+
 	if (Inst != NULL && Inst->m_symUnloadModule)
 		return Inst->m_symUnloadModule(hProcess, BaseOfDll);
 
@@ -218,6 +269,8 @@ BOOL DbgHelpLoader::symGetSymFromAddr(
 	LPDWORD Displacement,
 	PIMAGEHLP_SYMBOL Symbol)
 {
+	CriticalSectionClass::LockClass lock(CriticalSection);
+
 	if (Inst != NULL && Inst->m_symGetSymFromAddr)
 		return Inst->m_symGetSymFromAddr(hProcess, Address, Displacement, Symbol);
 
@@ -230,6 +283,8 @@ BOOL DbgHelpLoader::symGetLineFromAddr(
 	PDWORD pdwDisplacement,
 	PIMAGEHLP_LINE Line)
 {
+	CriticalSectionClass::LockClass lock(CriticalSection);
+
 	if (Inst != NULL && Inst->m_symGetLineFromAddr)
 		return Inst->m_symGetLineFromAddr(hProcess, dwAddr, pdwDisplacement, Line);
 
@@ -239,6 +294,8 @@ BOOL DbgHelpLoader::symGetLineFromAddr(
 DWORD DbgHelpLoader::symSetOptions(
 	DWORD SymOptions)
 {
+	CriticalSectionClass::LockClass lock(CriticalSection);
+
 	if (Inst != NULL && Inst->m_symSetOptions)
 		return Inst->m_symSetOptions(SymOptions);
 
@@ -249,6 +306,8 @@ LPVOID DbgHelpLoader::symFunctionTableAccess(
 	HANDLE hProcess,
 	DWORD AddrBase)
 {
+	CriticalSectionClass::LockClass lock(CriticalSection);
+
 	if (Inst != NULL && Inst->m_symFunctionTableAccess)
 		return Inst->m_symFunctionTableAccess(hProcess, AddrBase);
 
@@ -266,6 +325,8 @@ BOOL DbgHelpLoader::stackWalk(
 	PGET_MODULE_BASE_ROUTINE GetModuleBaseRoutine,
 	PTRANSLATE_ADDRESS_ROUTINE TranslateAddress)
 {
+	CriticalSectionClass::LockClass lock(CriticalSection);
+
 	if (Inst != NULL && Inst->m_stackWalk)
 		return Inst->m_stackWalk(MachineType, hProcess, hThread, StackFrame, ContextRecord, ReadMemoryRoutine, FunctionTableAccessRoutine, GetModuleBaseRoutine, TranslateAddress);
 

Core/Libraries/Source/WWVegas/WWLib/DbgHelpLoader.h

@@ -24,16 +24,18 @@
 #include <imagehlp.h> // Must be included after Windows.h
 #include <set>
 
+#include "mutex.h"
 #include "SystemAllocator.h"
 
-// This static class can load and unload dbghelp.dll
+// This static class can load, unload and use dbghelp.dll. Is thread-safe.
 // Internally it must not use new and delete because it can be created during game memory initialization.
 
 class DbgHelpLoader
 {
 private:
 
 	static DbgHelpLoader* Inst; // Is singleton class
+	static CriticalSectionClass CriticalSection; // Required because dbg help is not thread safe for the most part
 
 	DbgHelpLoader();
 	~DbgHelpLoader();
@@ -46,8 +48,11 @@ class DbgHelpLoader
 	// Returns whether dbghelp.dll is loaded from the system directory
 	static bool isLoadedFromSystem();
 
+	// Returns whether dbghelp.dll was attempted to be loaded but failed
+	static bool isFailed();
+
+	// Every call to load needs a paired call to unload, no matter if the load was successful
 	static bool load();
-	static bool reload();
 	static void unload();
 
 	static BOOL WINAPI symInitialize(
@@ -106,6 +111,8 @@ class DbgHelpLoader
 
 private:
 
+	static void freeResources();
+
 	typedef BOOL (WINAPI *SymInitialize_t) (
 		HANDLE hProcess,
 		LPSTR UserSearchPath,
@@ -175,6 +182,8 @@ class DbgHelpLoader
 
 	Processes m_initializedProcesses;
 	HMODULE m_dllModule;
+	int m_referenceCount;
+	CriticalSectionClass m_criticalSection;
 	bool m_failed;
 	bool m_loadedFromSystem;
 };

Core/Libraries/Source/WWVegas/WWLib/WWCommon.h

@@ -38,6 +38,8 @@ enum
 
 #if defined(_MSC_VER) && _MSC_VER < 1300
 typedef unsigned MemValueType;
+typedef long Interlocked32; // To use with Interlocked functions
 #else
 typedef unsigned long long MemValueType;
+typedef volatile long Interlocked32; // To use with Interlocked functions
 #endif

Generals/Code/GameEngine/Source/Common/System/StackDump.cpp

@@ -37,7 +37,6 @@
 //	Prototypes
 //*****************************************************************************
 BOOL InitSymbolInfo(void);
-void UninitSymbolInfo(void);
 void MakeStackTrace(DWORD myeip,DWORD myesp,DWORD myebp, int skipFrames, void (*callback)(const char*));
 void GetFunctionDetails(void *pointer, char*name, char*filename, unsigned int* linenumber, unsigned int* address);
 void WriteStackLine(void*address, void (*callback)(const char*));
@@ -109,10 +108,14 @@ BOOL InitSymbolInfo()
 	if (DbgHelpLoader::isLoaded())
 		return TRUE;
 
-	if (!DbgHelpLoader::load())
+	if (DbgHelpLoader::isFailed())
 		return FALSE;
 
-	atexit(UninitSymbolInfo);
+	if (!DbgHelpLoader::load())
+	{
+		atexit(DbgHelpLoader::unload);
+		return FALSE;
+	}
 
 	char pathname[_MAX_PATH+1];
 	char drive[10];
@@ -140,24 +143,16 @@ BOOL InitSymbolInfo()
 		if(DbgHelpLoader::symLoadModule(process, NULL, pathname, NULL, 0, 0))
 		{
 				//Load any other relevant modules (ie dlls) here
+				atexit(DbgHelpLoader::unload);
 				return TRUE;
 		}
 	}
 
 	DbgHelpLoader::unload();
-	return(FALSE);
+	return FALSE;
 }
 
 
-//*****************************************************************************
-//*****************************************************************************
-void UninitSymbolInfo(void)
-{
-	DbgHelpLoader::unload();
-}
-
-
-
 //*****************************************************************************
 //*****************************************************************************
 void MakeStackTrace(DWORD myeip,DWORD myesp,DWORD myebp, int skipFrames, void (*callback)(const char*))