Skip to content

[GEN][ZH] Fix static initialization order for StatDumpClass and LogClass #1003

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

Merged
merged 6 commits into from
Jun 13, 2025

Conversation

Caball009
Copy link

@Caball009 Caball009 commented Jun 3, 2025

Static / global variable:


StatDumpClass::StatDumpClass( const char *fname )
{
char buffer[ _MAX_PATH ];
GetModuleFileName( NULL, buffer, sizeof( buffer ) );
char *pEnd = buffer + strlen( buffer );
while( pEnd != buffer )
{
if( *pEnd == '\\' )
{
*pEnd = 0;
break;
}
pEnd--;
}
AsciiString fullPath;
fullPath.format( "%s\\%s", buffer, fname );
m_fp = fopen( fullPath.str(), "wt" );
}

I ran into an issue where the initialization of global TheStatDump happened before the initialization of global TheDynamicMemoryAllocator which is used for AsciiString (aka static initialization order fiasco). This PR fixes a couple of places where that may happen, though I only noticed it for TheStatDump.

I've also added a couple of DEBUG_ASSERTCRASH. They'll work better with #1000 accepted, which was created exactly for assertions like these.

EDIT: I can't seem to find how the issue surfaced for me naturally, but it's possible to force the issue to occur.

TheDynamicMemoryAllocator is part of the game engine and TheStatDump is part of the game engine device. You can force the issue to occur when you modify the dependency list of z_generals:
Go to Properties - Configuration Properties - Linker - Input - Additional Dependencies and reverse the order of:
..\GameEngineDevice\RelWithDebInfo\z_gameenginedevice.lib
..\GameEngine\RelWithDebInfo\z_gameengine.lib

@Caball009
Copy link
Author

Caball009 commented Jun 3, 2025

It's not entirely clear to me how severe a bug needs to be to warrant a "TheSuperHackers" comment. I can change it if it's not necessary for this change.

@xezon
Copy link

xezon commented Jun 6, 2025

On Windows it would be possible to use TLS static initialization in XLB segment to be the first to initialize.

https://stackoverflow.com/questions/14538159/tls-callback-in-windows

@xezon xezon added Minor Severity: Minor < Major < Critical < Blocker Fix Is fixing something, but is not user facing Stability Concerns stability of the runtime labels Jun 6, 2025
@xezon xezon added this to the Stability fixes milestone Jun 6, 2025
@xezon xezon merged commit 68d4db3 into TheSuperHackers:main Jun 13, 2025
18 checks passed
@Caball009 Caball009 deleted the fix_static_initialization_order branch June 13, 2025 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fix Is fixing something, but is not user facing Minor Severity: Minor < Major < Critical < Blocker Stability Concerns stability of the runtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants