-
Notifications
You must be signed in to change notification settings - Fork 104
feat(crashdump): Add crash dump functionality for fatal errors #1594
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, tested dump generation with a 2022 build.
Another approach I realized after publishing could be to move the GameMemory allocations from using GlobalAlloc to instead create a separate GameMemory heap and use HeapAlloc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good overall. Just a bunch of small comments.
{ | ||
// Find the full path to the dbghelp.dll file in the system32 dir | ||
GetSystemDirectory(m_sysDbgHelpPath, MAX_PATH); | ||
strlcat(m_sysDbgHelpPath, "\\dbghelp.dll", MAX_PATH); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, I'll hold off on this one (but push changes for the review comments) until after #1066 is merged, then replace the loading functionality here with DbgHelpLoader and extend it with the MiniDumpWriteDump function.
#ifdef RTS_ENABLE_CRASHDUMP | ||
#include "Common/MiniDumper.h" | ||
|
||
MiniDumper TheMiniDumper = MiniDumper(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think = MiniDumper()
is not necessary.
#ifdef RTS_ENABLE_CRASHDUMP | ||
#include "Common/MiniDumper.h" | ||
|
||
MiniDumper TheMiniDumper = MiniDumper(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe move this into MiniDumper.cpp
? This is how EA does it.
Win32Mouse *TheWin32Mouse= NULL; ///< for the WndProc() only | ||
DWORD TheMessageTime = 0; ///< For getting the time that a message was posted from Windows. | ||
#ifdef RTS_ENABLE_CRASHDUMP | ||
extern MiniDumper TheMiniDumper; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe move this into MiniDumper.h (or Debug.h) ?
class MiniDumper | ||
{ | ||
public: | ||
MiniDumper() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function body can go to cpp.
Win32Mouse *TheWin32Mouse= NULL; ///< for the WndProc() only | ||
DWORD TheMessageTime = 0; ///< For getting the time that a message was posted from Windows. | ||
#ifdef RTS_ENABLE_CRASHDUMP | ||
extern MiniDumper TheMiniDumper; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And maybe make this MiniDumper* TheMiniDumper
? This way we can control its lifetime better.
AllocationRangeIterator operator++(int) { AllocationRangeIterator tmp = *this; ++(*this); return tmp; } | ||
|
||
friend bool operator== (const AllocationRangeIterator& a, const AllocationRangeIterator& b) { return a.m_currentBlobInPool == b.m_currentBlobInPool; }; | ||
friend bool operator!= (const AllocationRangeIterator& a, const AllocationRangeIterator& b) { return a.m_currentBlobInPool != b.m_currentBlobInPool; }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const
m_stacktrace[0] = NULL; | ||
} | ||
#endif | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this end of scope correctly placed? Looks strange.
} | ||
void DynamicMemoryAllocator::fillAllocationRangeForRawBlockN(const Int n, MemoryPoolAllocatedRange& allocationRange) const | ||
{ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove blank line
|
||
void MiniDumper::CleanupResources() | ||
{ | ||
// NOTE: This method should not be called unless the dump thread is confirmed to not be running anymore. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe can put an assert here for this assumption?
This change introduces the option to generate crash dumps, aka. mini dumps, on fatal errors.
The main minidump functionality is done by explicitly loading the
dbghelp.dll
from the system directory, as the dbghelp.dll that is bundled with the game is an older version that does not include this functionality. There is an option to create small dumps or extended info dumps, currently both are created.Small dumps
These mostly contain stacks for the process threads and some stack variables, or to create dumps with extended info. The use case for these is to quickly determine where a crash occured, the type of crash, if it was already fixed etc. In addition, if the memory allocation structures are corrupted enough, an extended info dump might not succeed while the small dump should. The size of these dumps are typically on the order of 250kB.
Extended info
These contain global values, along with the memory regions allocated via the memory pool factory and the dynamic memory allocator. This makes all in-game objects available to the person debugging the crash dump, so for example
dt generalszh!TheWritableGlobalData
in WinDbg will show the state at the time the dump was created.An alternative option could be to not traverse the memory structures "manually" to get to the allocations and instead just specify the
MiniDumpWithFullMemory
flag toMiniDumpWriteDump
, but that increases the file size considerably.As an example, dump of the generalszh process in the main menu with the shell map in the background yields a ~140MB dump when traversing and ~420MB with
MiniDumpWithFullMemory
. Beyond that, the ~140MB file compresses to ~20MB with 7Z, so should be relatively easily transferable.Storage Location
Crash dumps are stored in a new folder called 'CrashDumps' under the userDir ("Documents\Command and Conquer Generals Zero Hour Data"), and on startup it will create this directory if it doesn't exist and delete any older dumps so only the 10 newest small and 2 newest extended info dumps are left. This is to preserve disk space, as the extended info files can be several hundred MB.
Integration points
For VS2022 builds, unhandled exceptions end up in the
UnhandledExceptionFilter
in WinMain, which then get a reference to the actual exception that occurred and includes that in the dump.For VC6 builds, unhandled exceptions are caught in the
catch(...)
blocks ofGameEngine::execute
which then calls RELEASE_CRASH. As there is no exception data available in this case to populate _EXCEPTION_POINTERS from, an intentional exception is triggered to get the trace of the current thread. This makes the stack traces for VC6 a bit more cryptic than VS2022 builds as the C++ exception handling gets included in the trace.Limitations
In the longer run we'll probably want to replace this code with a more mature solution, like CrashPad, but that currently depends on a newer compiler than VC6.
As the code is intended to be temporary, it's kept behind a new CMake feature so it can be easily removed. There are also some other decisions made with this in mind: