From 7fdea0c4b7acfec81d764dc637a9d07ba2a0ee7a Mon Sep 17 00:00:00 2001 From: Daniel Gibson Date: Sun, 6 Sep 2020 04:28:43 +0200 Subject: [PATCH 01/33] MSVC: Shut up Compiler warning in Matrix.h if asserts are disabled --- idlib/math/Matrix.h | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/idlib/math/Matrix.h b/idlib/math/Matrix.h index 785ac8f..4a8935a 100644 --- a/idlib/math/Matrix.h +++ b/idlib/math/Matrix.h @@ -34,6 +34,12 @@ If you have questions concerning this license or the applicable additional terms #include #endif +#ifdef _MSC_VER // DG: I don't care if matrix code has some unused r variable only used for assertions, shut up VS +#pragma warning( push ) +#pragma warning( disable : 4189 ) +#endif + + /* =============================================================================== @@ -2946,4 +2952,8 @@ ID_INLINE float *idMatX::ToFloatPtr( void ) { return mat; } +#ifdef _MSC_VER // DG: re-enable warning 4189 +#pragma warning( pop ) +#endif + #endif /* !__MATH_MATRIX_H__ */ From 88cafada23000769bb7886c14116321d38a4d2b1 Mon Sep 17 00:00:00 2001 From: Daniel Gibson Date: Sun, 6 Sep 2020 04:36:40 +0200 Subject: [PATCH 02/33] ID_MAYBE_INLINE for not-forced inlining On Windows, ID_INLINE does __forceinline, which is bad if the function calls alloca() and is called in a loop.. So use just __inline there so the compiler can choose not to inline (if called in a loop). This didn't cause actual stack overflows as far as I know, but it could (and MSVC warns about it). (This includes "Fix ID_MAYBE_INLINE on non-Windows platforms") --- idlib/math/Curve.h | 4 ++-- idlib/math/Lcp.cpp | 2 +- idlib/math/Matrix.h | 2 +- sys/platform.h | 7 +++++++ 4 files changed, 11 insertions(+), 4 deletions(-) diff --git a/idlib/math/Curve.h b/idlib/math/Curve.h index c8f9979..5c9e804 100644 --- a/idlib/math/Curve.h +++ b/idlib/math/Curve.h @@ -204,7 +204,7 @@ idCurve::RombergIntegral ==================== */ template< class type > -ID_INLINE float idCurve::RombergIntegral( const float t0, const float t1, const int order ) const { +ID_MAYBE_INLINE float idCurve::RombergIntegral(const float t0, const float t1, const int order) const { int i, j, k, m, n; float sum, delta; float *temp[2]; @@ -242,7 +242,7 @@ idCurve::GetLengthBetweenKnots ==================== */ template< class type > -ID_INLINE float idCurve::GetLengthBetweenKnots( const int i0, const int i1 ) const { +ID_MAYBE_INLINE float idCurve::GetLengthBetweenKnots(const int i0, const int i1) const { float length = 0.0f; for ( int i = i0; i < i1; i++ ) { length += RombergIntegral( times[i], times[i+1], 5 ); diff --git a/idlib/math/Lcp.cpp b/idlib/math/Lcp.cpp index 13ad756..f61c86a 100644 --- a/idlib/math/Lcp.cpp +++ b/idlib/math/Lcp.cpp @@ -340,7 +340,7 @@ idLCP_Square::CalcForceDelta modifies this->delta_f ============ */ -ID_INLINE void idLCP_Square::CalcForceDelta( int d, float dir ) { +ID_MAYBE_INLINE void idLCP_Square::CalcForceDelta(int d, float dir) { int i; float *ptr; diff --git a/idlib/math/Matrix.h b/idlib/math/Matrix.h index 4a8935a..c6cb187 100644 --- a/idlib/math/Matrix.h +++ b/idlib/math/Matrix.h @@ -2390,7 +2390,7 @@ ID_INLINE void idMatX::Clamp( float min, float max ) { } } -ID_INLINE idMatX &idMatX::SwapRows( int r1, int r2 ) { +ID_MAYBE_INLINE idMatX &idMatX::SwapRows(int r1, int r2) { float *ptr; ptr = (float *) _alloca16( numColumns * sizeof( float ) ); diff --git a/sys/platform.h b/sys/platform.h index 2c4ce98..d9a9a5b 100644 --- a/sys/platform.h +++ b/sys/platform.h @@ -85,6 +85,9 @@ If you have questions concerning this license or the applicable additional terms #define ALIGN16( x ) __declspec(align(16)) x #define PACKED #define ID_INLINE __forceinline +// DG: alternative to forced inlining of ID_INLINE for functions that do alloca() +// and are called in a loop so inlining them might cause stack overflow +#define ID_MAYBE_INLINE __inline #define ID_STATIC_TEMPLATE static #define assertmem( x, y ) assert( _CrtIsValidPointer( x, y, true ) ) #else @@ -161,6 +164,10 @@ If you have questions concerning this license or the applicable additional terms #endif +#ifndef ID_MAYBE_INLINE +// for MSVC it's __inline, otherwise just inline should work +#define ID_MAYBE_INLINE inline +#endif // ID_MAYBE_INLINE #ifdef __GNUC__ #define id_attribute(x) __attribute__(x) From db0fc8b25acac09c8f81b47893c6866ccca70ddc Mon Sep 17 00:00:00 2001 From: Daniel Gibson Date: Sun, 6 Sep 2020 04:42:33 +0200 Subject: [PATCH 03/33] Fix handling of paths with dots in dir names, fix #299, #301 idStr::StripFileExtension() (and SetFileExtension() which uses it) and others didn't work correctly if there was a dot in a directory name, because they just searched from last to first char for '.', so if the current filename didn't have an extension to cut off, they'd just cut off at any other '.' they found. So D:\dev\doom3.data\base\maps\bla could turn into D:\dev\doom3 (or, for SetFileExtension(), D:\dev\doom3.map) While at it, I made most of the idStr code that explicitly checked for '\\' and '/' (and maybe ':' for AROS) use a little "bool isDirSeparator(int c)" function so we don't have the #ifdefs for different platforms all over the place. --- idlib/Str.cpp | 85 ++++++++++++++++++++++++++------------------------- 1 file changed, 44 insertions(+), 41 deletions(-) diff --git a/idlib/Str.cpp b/idlib/Str.cpp index b37e1a9..4ccfb96 100644 --- a/idlib/Str.cpp +++ b/idlib/Str.cpp @@ -753,6 +753,26 @@ idStr &idStr::SetFileExtension( const char *extension ) { return *this; } +// DG: helper-function that returns true if the character c is a directory separator +// on the current platform +static ID_INLINE bool isDirSeparator( int c ) +{ + if ( c == '/' ) { + return true; + } +#ifdef _WIN32 + if ( c == '\\' ) { + return true; + } +#elif defined(__AROS__) + if ( c == ':' ) { + return true; + } +#endif + return false; +} +// DG end + /* ============ idStr::StripFileExtension @@ -762,6 +782,10 @@ idStr &idStr::StripFileExtension( void ) { int i; for ( i = len-1; i >= 0; i-- ) { + // DG: we're at a directory separator, nothing to strip at filename + if ( isDirSeparator( data[i] ) ) { + break; + } // DG end if ( data[i] == '.' ) { data[i] = '\0'; len = i; @@ -778,7 +802,9 @@ idStr::StripAbsoluteFileExtension */ idStr &idStr::StripAbsoluteFileExtension( void ) { int i; - + // FIXME DG: seems like this is unused, but it probably doesn't do what's expected + // (if you wanna strip .tar.gz this will fail with dots in path, + // if you indeed wanna strip the first dot in *path* (even in some directory) this is right) for ( i = 0; i < len; i++ ) { if ( data[i] == '.' ) { data[i] = '\0'; @@ -800,6 +826,10 @@ idStr &idStr::DefaultFileExtension( const char *extension ) { // do nothing if the string already has an extension for ( i = len-1; i >= 0; i-- ) { + // DG: we're at a directory separator, there was no file extension + if ( isDirSeparator( data[i] ) ) { + break; + } // DG end if ( data[i] == '.' ) { return *this; } @@ -817,11 +847,7 @@ idStr::DefaultPath ================== */ idStr &idStr::DefaultPath( const char *basepath ) { -#if defined(__AROS__) - if ( ( ( *this )[ 0 ] == '/' ) || ( ( *this )[ 0 ] == '\\' ) || ( ( *this )[ 0 ] == ':' ) ) { -#else - if ( ( ( *this )[ 0 ] == '/' ) || ( ( *this )[ 0 ] == '\\' ) ) { -#endif + if ( isDirSeparator( ( *this )[ 0 ] ) ) { // absolute path location return *this; } @@ -844,19 +870,12 @@ void idStr::AppendPath( const char *text ) { EnsureAlloced( len + strlen( text ) + 2 ); if ( pos ) { -#if defined(__AROS__) - if (( data[ pos-1 ] != '/' ) || ( data[ pos-1 ] != ':' )) { -#else - if ( data[ pos-1 ] != '/' ) { -#endif + if ( !isDirSeparator( data[ pos-1 ] ) ) { data[ pos++ ] = '/'; } } -#if defined(__AROS__) - if (( text[i] == '/' ) || ( text[i] == ':' )) { -#else - if ( text[i] == '/' ) { -#endif + + if ( isDirSeparator( text[ i ] ) ) { i++; } @@ -881,11 +900,7 @@ idStr &idStr::StripFilename( void ) { int pos; pos = Length() - 1; -#if defined(__AROS__) - while( ( pos > 0 ) && ( ( *this )[ pos ] != '/' ) && ( ( *this )[ pos ] != '\\' ) && ( ( *this )[ pos ] != ':' ) ) { -#else - while( ( pos > 0 ) && ( ( *this )[ pos ] != '/' ) && ( ( *this )[ pos ] != '\\' ) ) { -#endif + while( ( pos > 0 ) && !isDirSeparator( ( *this )[ pos ] ) ) { pos--; } @@ -906,11 +921,7 @@ idStr &idStr::StripPath( void ) { int pos; pos = Length(); -#if defined(__AROS__) - while( ( pos > 0 ) && ( ( *this )[ pos - 1 ] != '/' ) && ( ( *this )[ pos - 1 ] != '\\' ) && ( ( *this )[ pos - 1 ] != ':' ) ) { -#else - while( ( pos > 0 ) && ( ( *this )[ pos - 1 ] != '/' ) && ( ( *this )[ pos - 1 ] != '\\' ) ) { -#endif + while( ( pos > 0 ) && !isDirSeparator( ( *this )[ pos - 1 ] ) ) { pos--; } @@ -930,11 +941,7 @@ void idStr::ExtractFilePath( idStr &dest ) const { // back up until a \ or the start // pos = Length(); -#if defined(__AROS__) - while( ( pos > 0 ) && ( ( *this )[ pos - 1 ] != '/' ) && ( ( *this )[ pos - 1 ] != '\\' ) && ( ( *this )[ pos - 1 ] != ':' ) ) { -#else - while( ( pos > 0 ) && ( ( *this )[ pos - 1 ] != '/' ) && ( ( *this )[ pos - 1 ] != '\\' ) ) { -#endif + while( ( pos > 0 ) && !isDirSeparator( ( *this )[ pos - 1 ] ) ) { pos--; } @@ -953,11 +960,7 @@ void idStr::ExtractFileName( idStr &dest ) const { // back up until a \ or the start // pos = Length() - 1; -#if defined(__AROS__) - while( ( pos > 0 ) && ( ( *this )[ pos - 1 ] != '/' ) && ( ( *this )[ pos - 1 ] != '\\' ) && ( ( *this )[ pos - 1 ] != ':' ) ) { -#else - while( ( pos > 0 ) && ( ( *this )[ pos - 1 ] != '/' ) && ( ( *this )[ pos - 1 ] != '\\' ) ) { -#endif + while( ( pos > 0 ) && !isDirSeparator( ( *this )[ pos - 1 ] ) ) { pos--; } @@ -977,11 +980,7 @@ void idStr::ExtractFileBase( idStr &dest ) const { // back up until a \ or the start // pos = Length() - 1; -#if defined(__AROS__) - while( ( pos > 0 ) && ( ( *this )[ pos - 1 ] != '/' ) && ( ( *this )[ pos - 1 ] != '\\' ) && ( ( *this )[ pos - 1 ] != ':' ) ) { -#else - while( ( pos > 0 ) && ( ( *this )[ pos - 1 ] != '/' ) && ( ( *this )[ pos - 1 ] != '\\' ) ) { -#endif + while( ( pos > 0 ) && !isDirSeparator( ( *this )[ pos - 1 ] ) ) { pos--; } @@ -1007,6 +1006,10 @@ void idStr::ExtractFileExtension( idStr &dest ) const { pos = Length() - 1; while( ( pos > 0 ) && ( ( *this )[ pos - 1 ] != '.' ) ) { pos--; + if( isDirSeparator( ( *this )[ pos ] ) ) { // DG: check for directory separator + // no extension in the whole filename + dest.Empty(); + } // DG end } if ( !pos ) { From dde6fe462a92366890336105c44a4518c0b1cda8 Mon Sep 17 00:00:00 2001 From: Daniel Gibson Date: Sun, 6 Sep 2020 04:49:18 +0200 Subject: [PATCH 04/33] Fix lingering messages in HUD after loading savegame If you save, you get a message like "Game Saved..." which goes away after a few seconds. This happens at the very end of idPlayer::Save(): if ( hud ) { hud->SetStateString( "message", /* .. left out .. */ ); hud->HandleNamedEvent( "Message" ); } And handled in hud.gui, "onNamedEvent Message { ..." However, if you save again before it's gone, it'll be shown after loading the savegame and not go away until you save again.. This works around that issue by setting an empty message after loading a savegame. The underlying problem (which is not fixed!) seems to be that the transition GUI command (that's executed when hud.gui handles the "Message" event that's used to show this message) is probably not properly saved/restored so fading out the message isn't continued after loading. --- d3xp/Player.cpp | 7 +++++++ game/Player.cpp | 7 +++++++ 2 files changed, 14 insertions(+) diff --git a/d3xp/Player.cpp b/d3xp/Player.cpp index bea0802..783e62c 100644 --- a/d3xp/Player.cpp +++ b/d3xp/Player.cpp @@ -2821,6 +2821,13 @@ void idPlayer::Restore( idRestoreGame *savefile ) { savefile->ReadFloat( bloomSpeed ); savefile->ReadFloat( bloomIntensity ); #endif + + // DG: workaround for lingering messages that are shown forever after loading a savegame + // (one way to get them is saving again, while the message from first save is still + // shown, and then load) + if ( hud ) { + hud->SetStateString( "message", "" ); + } } /* diff --git a/game/Player.cpp b/game/Player.cpp index d0cf95b..be594d3 100644 --- a/game/Player.cpp +++ b/game/Player.cpp @@ -2355,6 +2355,13 @@ void idPlayer::Restore( idRestoreGame *savefile ) { // create combat collision hull for exact collision detection SetCombatModel(); + + // DG: workaround for lingering messages that are shown forever after loading a savegame + // (one way to get them is saving again, while the message from first save is still + // shown, and then load) + if ( hud ) { + hud->SetStateString( "message", "" ); + } } /* From c472d371dc44b0ab83555a43d9a73654b79f2a8f Mon Sep 17 00:00:00 2001 From: Daniel Gibson Date: Sun, 6 Sep 2020 04:54:51 +0200 Subject: [PATCH 05/33] Disable broken idSIMD_SSE::UpSampleOGGTo44kHz() It corrupted the stack when called with buffers allocated on the stack and numSamples that are not a multiple of four, apparently, by writing 4 floats too many, at least in the 22KHz Stereo case.. This caused the crash described in https://github.com/dhewm/dhewm3/issues/303#issuecomment-678809662 Now it just uses the generic C code, like all platforms besides MSVC/x86 already do. --- idlib/math/Simd_SSE.cpp | 5 +++++ idlib/math/Simd_SSE.h | 3 ++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/idlib/math/Simd_SSE.cpp b/idlib/math/Simd_SSE.cpp index 4c00500..609cb59 100644 --- a/idlib/math/Simd_SSE.cpp +++ b/idlib/math/Simd_SSE.cpp @@ -17164,6 +17164,10 @@ void idSIMD_SSE::UpSamplePCMTo44kHz( float *dest, const short *src, const int nu } } + +// DG: at least in the 22KHz Stereo OGG case with numSamples % 4 != 0 this is broken (writes 4 floats too much which can destroy the stack, see #303), +// so let's just not use it anymore its MSVC+32bit only anyway and I doubt it gets noticeable speedups, so I don't feel like trying to understand and fix it.. +#if 0 /* ============ SSE_UpSample11kHzMonoOGGTo44kHz @@ -17474,6 +17478,7 @@ void idSIMD_SSE::UpSampleOGGTo44kHz( float *dest, const float * const *ogg, cons assert( 0 ); } } +#endif // 0 (DG: commenting out all the OGG-related SSE code) /* ============ diff --git a/idlib/math/Simd_SSE.h b/idlib/math/Simd_SSE.h index 3c66a55..385bac8 100644 --- a/idlib/math/Simd_SSE.h +++ b/idlib/math/Simd_SSE.h @@ -135,7 +135,8 @@ class idSIMD_SSE : public idSIMD_MMX { virtual int VPCALL CreateVertexProgramShadowCache( idVec4 *vertexCache, const idDrawVert *verts, const int numVerts ); virtual void VPCALL UpSamplePCMTo44kHz( float *dest, const short *pcm, const int numSamples, const int kHz, const int numChannels ); - virtual void VPCALL UpSampleOGGTo44kHz( float *dest, const float * const *ogg, const int numSamples, const int kHz, const int numChannels ); + // DG: the following is broken at least in the 22KHz Stereo case with numSamples % 4 != 0 (writes 4 floats too much which can destroy the stack), so let's not use it anymore. + //virtual void VPCALL UpSampleOGGTo44kHz( float *dest, const float * const *ogg, const int numSamples, const int kHz, const int numChannels ); virtual void VPCALL MixSoundTwoSpeakerMono( float *mixBuffer, const float *samples, const int numSamples, const float lastV[2], const float currentV[2] ); virtual void VPCALL MixSoundTwoSpeakerStereo( float *mixBuffer, const float *samples, const int numSamples, const float lastV[2], const float currentV[2] ); virtual void VPCALL MixSoundSixSpeakerMono( float *mixBuffer, const float *samples, const int numSamples, const float lastV[6], const float currentV[6] ); From 72b4def6f8407a259fa5a3d9ae39a1965e6a1877 Mon Sep 17 00:00:00 2001 From: Daniel Gibson Date: Sun, 6 Sep 2020 04:56:53 +0200 Subject: [PATCH 06/33] Fix "t->c->value.argSize == func->parmTotal" Assertion in Scripts, #303 If a "class" (object) in a Script has multiple member function prototypes, and one function implementation later calls another before that is implemented, there was an assertion when the script was parsed (at map start), because the size of function arguments at the call-site didn't match what the function expected - because the function hadn't calculated that size yet, that only happened once its own implementation was parsed. Now it's calculated (and stored) when the prototype/declaration is parsed to prevent this issue, which seems to be kinda common with Mods, esp. Script-only mods, as the release game DLLs had Assertions disabled. --- d3xp/script/Script_Compiler.cpp | 53 ++++++++++++++++++++------------- game/script/Script_Compiler.cpp | 53 ++++++++++++++++++++------------- 2 files changed, 66 insertions(+), 40 deletions(-) diff --git a/d3xp/script/Script_Compiler.cpp b/d3xp/script/Script_Compiler.cpp index 7620c60..6fac0a6 100644 --- a/d3xp/script/Script_Compiler.cpp +++ b/d3xp/script/Script_Compiler.cpp @@ -2144,34 +2144,47 @@ void idCompiler::ParseFunctionDef( idTypeDef *returnType, const char *name ) { } } - // check if this is a prototype or declaration - if ( !CheckToken( "{" ) ) { - // it's just a prototype, so get the ; and move on - ExpectToken( ";" ); - return; - } + // DG: make sure parmSize gets calculated when parsing prototype (not just when parsing + // implementation) so calling this function/method before implementation has been parsed + // works without getting Assertions in IdInterpreter::Execute() and ::LeaveFunction() + // ("st->c->value.argSize == func->parmTotal", "localstackUsed == localstackBase", see #303) // calculate stack space used by parms numParms = type->NumParameters(); - func->parmSize.SetNum( numParms ); - for( i = 0; i < numParms; i++ ) { - parmType = type->GetParmType( i ); - if ( parmType->Inherits( &type_object ) ) { - func->parmSize[ i ] = type_object.Size(); - } else { - func->parmSize[ i ] = parmType->Size(); + if( numParms != func->parmSize.Num() ) { // DG: make sure not to do this twice + + // if it hasn't been parsed yet, parmSize.Num() should be 0.. + assert( func->parmSize.Num() == 0 && "function had different number of arguments before?!" ); + + func->parmSize.SetNum( numParms ); + for( i = 0; i < numParms; i++ ) { + parmType = type->GetParmType( i ); + if ( parmType->Inherits( &type_object ) ) { + func->parmSize[ i ] = type_object.Size(); + } else { + func->parmSize[ i ] = parmType->Size(); + } + func->parmTotal += func->parmSize[ i ]; } - func->parmTotal += func->parmSize[ i ]; - } - // define the parms - for( i = 0; i < numParms; i++ ) { - if ( gameLocal.program.GetDef( type->GetParmType( i ), type->GetParmName( i ), def ) ) { - Error( "'%s' defined more than once in function parameters", type->GetParmName( i ) ); + // define the parms + for( i = 0; i < numParms; i++ ) { + if ( gameLocal.program.GetDef( type->GetParmType( i ), type->GetParmName( i ), def ) ) { + Error( "'%s' defined more than once in function parameters", type->GetParmName( i ) ); + } + gameLocal.program.AllocDef( type->GetParmType( i ), type->GetParmName( i ), def, false ); } - gameLocal.program.AllocDef( type->GetParmType( i ), type->GetParmName( i ), def, false ); } + // DG: moved this down here so parmSize also gets calculated when parsing prototype + // check if this is a prototype or declaration + if ( !CheckToken( "{" ) ) { + // it's just a prototype, so get the ; and move on + ExpectToken( ";" ); + return; + } + // DG end + oldscope = scope; scope = def; diff --git a/game/script/Script_Compiler.cpp b/game/script/Script_Compiler.cpp index 1757233..1ad2693 100644 --- a/game/script/Script_Compiler.cpp +++ b/game/script/Script_Compiler.cpp @@ -2144,34 +2144,47 @@ void idCompiler::ParseFunctionDef( idTypeDef *returnType, const char *name ) { } } - // check if this is a prototype or declaration - if ( !CheckToken( "{" ) ) { - // it's just a prototype, so get the ; and move on - ExpectToken( ";" ); - return; - } + // DG: make sure parmSize gets calculated when parsing prototype (not just when parsing + // implementation) so calling this function/method before implementation has been parsed + // works without getting Assertions in IdInterpreter::Execute() and ::LeaveFunction() + // ("st->c->value.argSize == func->parmTotal", "localstackUsed == localstackBase", see #303) // calculate stack space used by parms numParms = type->NumParameters(); - func->parmSize.SetNum( numParms ); - for( i = 0; i < numParms; i++ ) { - parmType = type->GetParmType( i ); - if ( parmType->Inherits( &type_object ) ) { - func->parmSize[ i ] = type_object.Size(); - } else { - func->parmSize[ i ] = parmType->Size(); + if( numParms != func->parmSize.Num() ) { // DG: make sure not to do this twice + + // if it hasn't been parsed yet, parmSize.Num() should be 0.. + assert( func->parmSize.Num() == 0 && "function had different number of arguments before?!" ); + + func->parmSize.SetNum( numParms ); + for( i = 0; i < numParms; i++ ) { + parmType = type->GetParmType( i ); + if ( parmType->Inherits( &type_object ) ) { + func->parmSize[ i ] = type_object.Size(); + } else { + func->parmSize[ i ] = parmType->Size(); + } + func->parmTotal += func->parmSize[ i ]; } - func->parmTotal += func->parmSize[ i ]; - } - // define the parms - for( i = 0; i < numParms; i++ ) { - if ( gameLocal.program.GetDef( type->GetParmType( i ), type->GetParmName( i ), def ) ) { - Error( "'%s' defined more than once in function parameters", type->GetParmName( i ) ); + // define the parms + for( i = 0; i < numParms; i++ ) { + if ( gameLocal.program.GetDef( type->GetParmType( i ), type->GetParmName( i ), def ) ) { + Error( "'%s' defined more than once in function parameters", type->GetParmName( i ) ); + } + gameLocal.program.AllocDef( type->GetParmType( i ), type->GetParmName( i ), def, false ); } - gameLocal.program.AllocDef( type->GetParmType( i ), type->GetParmName( i ), def, false ); } + // DG: moved this down here so parmSize also gets calculated when parsing prototype + // check if this is a prototype or declaration + if ( !CheckToken( "{" ) ) { + // it's just a prototype, so get the ; and move on + ExpectToken( ";" ); + return; + } + // DG end + oldscope = scope; scope = def; From 4e9a6a71dad713ed18cdaac691542e469dd09f89 Mon Sep 17 00:00:00 2001 From: Daniel Gibson Date: Wed, 8 Jan 2025 02:59:57 +0100 Subject: [PATCH 07/33] Add information about dhewm3 build to savegames like the dhewm3 version and the OS and architecture of the dhewm3 version that created the savegame. Also added an internalSavegameVersion so be independent of BUILD_NUMBER fixes #344 --- d3xp/Game_local.cpp | 41 +++++++++++++++++++++++++++++++++++++++ d3xp/Game_local.h | 1 + d3xp/gamesys/SaveGame.cpp | 1 + d3xp/gamesys/SaveGame.h | 15 ++++++++++++++ game/Game_local.cpp | 41 +++++++++++++++++++++++++++++++++++++++ game/Game_local.h | 1 + game/gamesys/SaveGame.cpp | 1 + game/gamesys/SaveGame.h | 15 ++++++++++++++ 8 files changed, 116 insertions(+) diff --git a/d3xp/Game_local.cpp b/d3xp/Game_local.cpp index aeef441..e6b8b1d 100644 --- a/d3xp/Game_local.cpp +++ b/d3xp/Game_local.cpp @@ -26,6 +26,7 @@ If you have questions concerning this license or the applicable additional terms =========================================================================== */ +#include "sys/Stub_SDL_endian.h" #include "sys/platform.h" #include "idlib/LangDict.h" #include "idlib/Timer.h" @@ -33,6 +34,7 @@ If you have questions concerning this license or the applicable additional terms #include "framework/BuildVersion.h" #include "framework/DeclEntityDef.h" #include "framework/FileSystem.h" +#include "framework/Licensee.h" #include "renderer/ModelManager.h" #include "gamesys/SysCvar.h" @@ -539,6 +541,15 @@ void idGameLocal::SaveGame( idFile *f ) { savegame.WriteBuildNumber( BUILD_NUMBER ); + // DG: add some more information to savegame to make future quirks easier + savegame.WriteInt( INTERNAL_SAVEGAME_VERSION ); // to be independent of BUILD_NUMBER + savegame.WriteString( D3_OSTYPE ); // operating system - from CMake + savegame.WriteString( D3_ARCH ); // CPU architecture (e.g. "x86" or "x86_64") - from CMake + savegame.WriteString( ENGINE_VERSION ); + savegame.WriteShort( (short)sizeof(void*) ); // tells us if it's from a 32bit (4) or 64bit system (8) + savegame.WriteShort( SDL_BYTEORDER ) ; // SDL_LIL_ENDIAN or SDL_BIG_ENDIAN + // DG end + // go through all entities and threads and add them to the object list for( i = 0; i < MAX_GENTITIES; i++ ) { ent = entities[i]; @@ -1575,6 +1586,36 @@ bool idGameLocal::InitFromSaveGame( const char *mapName, idRenderWorld *renderWo savegame.ReadBuildNumber(); + // DG: I enhanced the information in savegames a bit for dhewm3 1.5.1 + // for which I bumped th BUILD_NUMBER to 1305 + if( savegame.GetBuildNumber() >= 1305 ) + { + savegame.ReadInternalSavegameVersion(); + if( savegame.GetInternalSavegameVersion() > INTERNAL_SAVEGAME_VERSION ) { + Warning( "Savegame from newer dhewm3 version, don't know how to load! (its version is %d, only up to %d supported)", + savegame.GetInternalSavegameVersion(), INTERNAL_SAVEGAME_VERSION ); + return false; + } + idStr osType; + idStr cpuArch; + idStr engineVersion; + short ptrSize = 0; + short byteorder = 0; + savegame.ReadString( osType ); // operating system the savegame was crated on (written from D3_OSTYPE) + savegame.ReadString( cpuArch ); // written from D3_ARCH (which is set in CMake), like "x86" or "x86_64" + savegame.ReadString( engineVersion ); // written from ENGINE_VERSION + savegame.ReadShort( ptrSize ); // sizeof(void*) of system that created the savegame, 4 on 32bit systems, 8 on 64bit systems + savegame.ReadShort( byteorder ); // SDL_LIL_ENDIAN or SDL_BIG_ENDIAN + + Printf( "Savegame was created by %s on %s %s. BuildNumber was %d, savegameversion %d\n", + engineVersion.c_str(), osType.c_str(), cpuArch.c_str(), savegame.GetBuildNumber(), + savegame.GetInternalSavegameVersion() ); + + // right now I have no further use for this information, but in the future + // it can be used for quirks for (then-) old savegames + } + // DG end + // Create the list of all objects in the game savegame.CreateObjects(); diff --git a/d3xp/Game_local.h b/d3xp/Game_local.h index bcbc304..e0439c5 100644 --- a/d3xp/Game_local.h +++ b/d3xp/Game_local.h @@ -625,6 +625,7 @@ class idGameLocal : public idGame { private: const static int INITIAL_SPAWN_COUNT = 1; + const static int INTERNAL_SAVEGAME_VERSION = 1; // DG: added this for >= 1305 savegames idStr mapFileName; // name of the map, empty string if no map loaded idMapFile * mapFile; // will be NULL during the game unless in-game editing is used diff --git a/d3xp/gamesys/SaveGame.cpp b/d3xp/gamesys/SaveGame.cpp index 1207263..ce31cf0 100644 --- a/d3xp/gamesys/SaveGame.cpp +++ b/d3xp/gamesys/SaveGame.cpp @@ -786,6 +786,7 @@ idRestoreGame::RestoreGame */ idRestoreGame::idRestoreGame( idFile *savefile ) { file = savefile; + internalSavegameVersion = 0; } /* diff --git a/d3xp/gamesys/SaveGame.h b/d3xp/gamesys/SaveGame.h index 4b0927f..b781659 100644 --- a/d3xp/gamesys/SaveGame.h +++ b/d3xp/gamesys/SaveGame.h @@ -158,8 +158,23 @@ class idRestoreGame { // Used to retrieve the saved game buildNumber from within class Restore methods int GetBuildNumber( void ); + // DG: added these methods, internalSavegameVersion makes us independent of the global BUILD_NUMBER + void ReadInternalSavegameVersion( void ) + { + ReadInt( internalSavegameVersion ); + } + + // if it's 0, this is from a GetBuildNumber() < 1305 savegame + // otherwise, compare it to idGameLocal::INTERNAL_SAVEGAME_VERSION + int GetInternalSavegameVersion( void ) const + { + return internalSavegameVersion; + } + // DG end + private: int buildNumber; + int internalSavegameVersion; // DG added this idFile * file; diff --git a/game/Game_local.cpp b/game/Game_local.cpp index 8b57012..7a6e942 100644 --- a/game/Game_local.cpp +++ b/game/Game_local.cpp @@ -26,6 +26,7 @@ If you have questions concerning this license or the applicable additional terms =========================================================================== */ +#include "sys/Stub_SDL_endian.h" #include "sys/platform.h" #include "idlib/LangDict.h" #include "idlib/Timer.h" @@ -33,6 +34,7 @@ If you have questions concerning this license or the applicable additional terms #include "framework/BuildVersion.h" #include "framework/DeclEntityDef.h" #include "framework/FileSystem.h" +#include "framework/Licensee.h" #include "renderer/ModelManager.h" #include "gamesys/SysCvar.h" @@ -462,6 +464,15 @@ void idGameLocal::SaveGame( idFile *f ) { savegame.WriteBuildNumber( BUILD_NUMBER ); + // DG: add some more information to savegame to make future quirks easier + savegame.WriteInt( INTERNAL_SAVEGAME_VERSION ); // to be independent of BUILD_NUMBER + savegame.WriteString( D3_OSTYPE ); // operating system - from CMake + savegame.WriteString( D3_ARCH ); // CPU architecture (e.g. "x86" or "x86_64") - from CMake + savegame.WriteString( ENGINE_VERSION ); + savegame.WriteShort( (short)sizeof(void*) ); // tells us if it's from a 32bit (4) or 64bit system (8) + savegame.WriteShort( SDL_BYTEORDER ) ; // SDL_LIL_ENDIAN or SDL_BIG_ENDIAN + // DG end + // go through all entities and threads and add them to the object list for( i = 0; i < MAX_GENTITIES; i++ ) { ent = entities[i]; @@ -1502,6 +1513,36 @@ bool idGameLocal::InitFromSaveGame( const char *mapName, idRenderWorld *renderWo savegame.ReadBuildNumber(); + // DG: I enhanced the information in savegames a bit for dhewm3 1.5.1 + // for which I bumped th BUILD_NUMBER to 1305 + if( savegame.GetBuildNumber() >= 1305 ) + { + savegame.ReadInternalSavegameVersion(); + if( savegame.GetInternalSavegameVersion() > INTERNAL_SAVEGAME_VERSION ) { + Warning( "Savegame from newer dhewm3 version, don't know how to load! (its version is %d, only up to %d supported)", + savegame.GetInternalSavegameVersion(), INTERNAL_SAVEGAME_VERSION ); + return false; + } + idStr osType; + idStr cpuArch; + idStr engineVersion; + short ptrSize = 0; + short byteorder = 0; + savegame.ReadString( osType ); // operating system the savegame was crated on (written from D3_OSTYPE) + savegame.ReadString( cpuArch ); // written from D3_ARCH (which is set in CMake), like "x86" or "x86_64" + savegame.ReadString( engineVersion ); // written from ENGINE_VERSION + savegame.ReadShort( ptrSize ); // sizeof(void*) of system that created the savegame, 4 on 32bit systems, 8 on 64bit systems + savegame.ReadShort( byteorder ); // SDL_LIL_ENDIAN or SDL_BIG_ENDIAN + + Printf( "Savegame was created by %s on %s %s. BuildNumber was %d, savegameversion %d\n", + engineVersion.c_str(), osType.c_str(), cpuArch.c_str(), savegame.GetBuildNumber(), + savegame.GetInternalSavegameVersion() ); + + // right now I have no further use for this information, but in the future + // it can be used for quirks for (then-) old savegames + } + // DG end + // Create the list of all objects in the game savegame.CreateObjects(); diff --git a/game/Game_local.h b/game/Game_local.h index 54e9651..25156ab 100644 --- a/game/Game_local.h +++ b/game/Game_local.h @@ -561,6 +561,7 @@ class idGameLocal : public idGame { idStaticList initialSpots; //public for coop private: const static int INITIAL_SPAWN_COUNT = 1; + const static int INTERNAL_SAVEGAME_VERSION = 1; // DG: added this for >= 1305 savegames idStr mapFileName; // name of the map, empty string if no map loaded idMapFile * mapFile; // will be NULL during the game unless in-game editing is used diff --git a/game/gamesys/SaveGame.cpp b/game/gamesys/SaveGame.cpp index 40e1969..158f5ab 100644 --- a/game/gamesys/SaveGame.cpp +++ b/game/gamesys/SaveGame.cpp @@ -781,6 +781,7 @@ idRestoreGame::RestoreGame */ idRestoreGame::idRestoreGame( idFile *savefile ) { file = savefile; + internalSavegameVersion = 0; } /* diff --git a/game/gamesys/SaveGame.h b/game/gamesys/SaveGame.h index 4b0927f..b781659 100644 --- a/game/gamesys/SaveGame.h +++ b/game/gamesys/SaveGame.h @@ -158,8 +158,23 @@ class idRestoreGame { // Used to retrieve the saved game buildNumber from within class Restore methods int GetBuildNumber( void ); + // DG: added these methods, internalSavegameVersion makes us independent of the global BUILD_NUMBER + void ReadInternalSavegameVersion( void ) + { + ReadInt( internalSavegameVersion ); + } + + // if it's 0, this is from a GetBuildNumber() < 1305 savegame + // otherwise, compare it to idGameLocal::INTERNAL_SAVEGAME_VERSION + int GetInternalSavegameVersion( void ) const + { + return internalSavegameVersion; + } + // DG end + private: int buildNumber; + int internalSavegameVersion; // DG added this idFile * file; From 2ce7644129782c5cd3acaf2d2e369b1645afc4c1 Mon Sep 17 00:00:00 2001 From: Daniel Gibson Date: Mon, 15 Feb 2021 05:27:37 +0100 Subject: [PATCH 08/33] Fix player's clipModel->axis when loading savegame, fixes #328 idClipModel::axis is an idMat3 rotation matrix. Usually it's an identity matrix, but if the player is pushed around by an idPush entity it's modified and apparently can (wrongly) remain modified, possibly when saving while idPush is active. This seems to happen sometimes on the crashing elevator in game/delta1. The fix/workaround is to reset it to mat3_identity when loading a savegame. --- d3xp/physics/Physics_Player.cpp | 14 ++++++++++++++ game/physics/Physics_Player.cpp | 14 ++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/d3xp/physics/Physics_Player.cpp b/d3xp/physics/Physics_Player.cpp index 78adf2c..d68ee7e 100644 --- a/d3xp/physics/Physics_Player.cpp +++ b/d3xp/physics/Physics_Player.cpp @@ -1628,6 +1628,20 @@ void idPhysics_Player::Restore( idRestoreGame *savefile ) { savefile->ReadInt( (int &)waterLevel ); savefile->ReadInt( waterType ); + + /* DG: It can apparently happen that the player saves while the clipModel's axis are + * modified by idPush::TryRotatePushEntity() -> idPhysics_Player::Rotate() -> idClipModel::Link() + * Normally idPush seems to reset them to the identity matrix in the next frame, + * but apparently not when coming from a savegame. + * Usually clipModel->axis is the identity matrix, and if it isn't there's clipping bugs + * like CheckGround() reporting that it's steep even though the player is only trying to + * walk up normal stairs. + * Resetting the axis to mat3_identity when restoring a savegame works around that issue + * and makes sure players can go on playing if their savegame was "corrupted" by saving + * while idPush was active. See https://github.com/dhewm/dhewm3/issues/328 for more details */ + if ( clipModel != nullptr ) { + clipModel->SetPosition( clipModel->GetOrigin(), mat3_identity ); + } } /* diff --git a/game/physics/Physics_Player.cpp b/game/physics/Physics_Player.cpp index 132a5df..a3dc290 100644 --- a/game/physics/Physics_Player.cpp +++ b/game/physics/Physics_Player.cpp @@ -1628,6 +1628,20 @@ void idPhysics_Player::Restore( idRestoreGame *savefile ) { savefile->ReadInt( (int &)waterLevel ); savefile->ReadInt( waterType ); + + /* DG: It can apparently happen that the player saves while the clipModel's axis are + * modified by idPush::TryRotatePushEntity() -> idPhysics_Player::Rotate() -> idClipModel::Link() + * Normally idPush seems to reset them to the identity matrix in the next frame, + * but apparently not when coming from a savegame. + * Usually clipModel->axis is the identity matrix, and if it isn't there's clipping bugs + * like CheckGround() reporting that it's steep even though the player is only trying to + * walk up normal stairs. + * Resetting the axis to mat3_identity when restoring a savegame works around that issue + * and makes sure players can go on playing if their savegame was "corrupted" by saving + * while idPush was active. See https://github.com/dhewm/dhewm3/issues/328 for more details */ + if ( clipModel != nullptr ) { + clipModel->SetPosition( clipModel->GetOrigin(), mat3_identity ); + } } /* From 258413b3d7c17d3b5b83f14ae537a52b62adb6fd Mon Sep 17 00:00:00 2001 From: Turo Lamminen Date: Mon, 25 Jan 2021 11:44:50 +0200 Subject: [PATCH 09/33] Several commits about memcpy() and memset() from turol in idlib squashed "Don't use memcpy in idMat2 constructor" up to "Don't use memset to zero non-trivial type volumeIntegrals_t" from Turo Lamminen (in orig dhewm3 repo) --- idlib/geometry/Surface.h | 4 +++- idlib/geometry/TraceModel.cpp | 5 +++- idlib/math/Matrix.h | 44 ++++++++++++++++++++++++++++------- 3 files changed, 42 insertions(+), 11 deletions(-) diff --git a/idlib/geometry/Surface.h b/idlib/geometry/Surface.h index 983bce2..b88e3ab 100644 --- a/idlib/geometry/Surface.h +++ b/idlib/geometry/Surface.h @@ -123,7 +123,9 @@ idSurface::idSurface ID_INLINE idSurface::idSurface( const idDrawVert *verts, const int numVerts, const int *indexes, const int numIndexes ) { assert( verts != NULL && indexes != NULL && numVerts > 0 && numIndexes > 0 ); this->verts.SetNum( numVerts ); - memcpy( this->verts.Ptr(), verts, numVerts * sizeof( verts[0] ) ); + for (int i = 0; i < numVerts; i++) { + this->verts[i] = verts[i]; + } this->indexes.SetNum( numIndexes ); memcpy( this->indexes.Ptr(), indexes, numIndexes * sizeof( indexes[0] ) ); GenerateEdgeIndexes(); diff --git a/idlib/geometry/TraceModel.cpp b/idlib/geometry/TraceModel.cpp index b1a88ff..56e0958 100644 --- a/idlib/geometry/TraceModel.cpp +++ b/idlib/geometry/TraceModel.cpp @@ -1409,7 +1409,10 @@ void idTraceModel::VolumeIntegrals( struct volumeIntegrals_s &integrals ) const int i, a, b, c; float nx, ny, nz; - memset( &integrals, 0, sizeof(volumeIntegrals_t) ); + integrals.T0 = 0.0f; + integrals.T1.Zero(); + integrals.T2.Zero(); + integrals.TP.Zero(); for ( i = 0; i < numPolys; i++ ) { poly = &polys[i]; diff --git a/idlib/math/Matrix.h b/idlib/math/Matrix.h index c6cb187..4556259 100644 --- a/idlib/math/Matrix.h +++ b/idlib/math/Matrix.h @@ -135,7 +135,8 @@ ID_INLINE idMat2::idMat2( const float xx, const float xy, const float yx, const } ID_INLINE idMat2::idMat2( const float src[ 2 ][ 2 ] ) { - memcpy( mat, src, 2 * 2 * sizeof( float ) ); + mat[0].x = src[0][0]; mat[0].y = src[0][1]; + mat[1].x = src[1][0]; mat[1].y = src[1][1]; } ID_INLINE const idVec2 &idMat2::operator[]( int index ) const { @@ -439,7 +440,9 @@ ID_INLINE idMat3::idMat3( const float xx, const float xy, const float xz, const } ID_INLINE idMat3::idMat3( const float src[ 3 ][ 3 ] ) { - memcpy( mat, src, 3 * 3 * sizeof( float ) ); + mat[0].x = src[0][0]; mat[0].y = src[0][1]; mat[0].z = src[0][2]; + mat[1].x = src[1][0]; mat[1].y = src[1][1]; mat[1].z = src[1][2]; + mat[2].x = src[2][0]; mat[2].y = src[2][1]; mat[2].z = src[2][2]; } ID_INLINE const idVec3 &idMat3::operator[]( int index ) const { @@ -596,7 +599,9 @@ ID_INLINE bool idMat3::operator!=( const idMat3 &a ) const { } ID_INLINE void idMat3::Zero( void ) { - memset( mat, 0, sizeof( idMat3 ) ); + mat[0].x = 0.0f; mat[0].y = 0.0f; mat[0].z = 0.0f; + mat[1].x = 0.0f; mat[1].y = 0.0f; mat[1].z = 0.0f; + mat[2].x = 0.0f; mat[2].y = 0.0f; mat[2].z = 0.0f; } ID_INLINE void idMat3::Identity( void ) { @@ -882,7 +887,10 @@ ID_INLINE idMat4::idMat4( const idMat3 &rotation, const idVec3 &translation ) { } ID_INLINE idMat4::idMat4( const float src[ 4 ][ 4 ] ) { - memcpy( mat, src, 4 * 4 * sizeof( float ) ); + mat[0].x = src[0][0]; mat[0].y = src[0][1]; mat[0].z = src[0][2]; mat[0].w = src[0][3]; + mat[1].x = src[1][0]; mat[1].y = src[1][1]; mat[1].z = src[1][2]; mat[1].w = src[1][3]; + mat[2].x = src[2][0]; mat[2].y = src[2][1]; mat[2].z = src[2][2]; mat[2].w = src[2][3]; + mat[3].x = src[3][0]; mat[3].y = src[3][1]; mat[3].z = src[3][2]; mat[3].w = src[3][3]; } ID_INLINE const idVec4 &idMat4::operator[]( int index ) const { @@ -1058,7 +1066,10 @@ ID_INLINE bool idMat4::operator!=( const idMat4 &a ) const { } ID_INLINE void idMat4::Zero( void ) { - memset( mat, 0, sizeof( idMat4 ) ); + mat[0].x = 0.0f; mat[0].y = 0.0f; mat[0].z = 0.0f; mat[0].w = 0.0f; + mat[1].x = 0.0f; mat[1].y = 0.0f; mat[1].z = 0.0f; mat[1].w = 0.0f; + mat[2].x = 0.0f; mat[2].y = 0.0f; mat[2].z = 0.0f; mat[2].w = 0.0f; + mat[3].x = 0.0f; mat[3].y = 0.0f; mat[3].z = 0.0f; mat[3].w = 0.0f; } ID_INLINE void idMat4::Identity( void ) { @@ -1220,7 +1231,11 @@ ID_INLINE idMat5::idMat5( void ) { } ID_INLINE idMat5::idMat5( const float src[ 5 ][ 5 ] ) { - memcpy( mat, src, 5 * 5 * sizeof( float ) ); + mat[0].x = src[0][0]; mat[0].y = src[0][1]; mat[0].z = src[0][2]; mat[0].s = src[0][3]; mat[0].t = src[0][4]; + mat[1].x = src[1][0]; mat[1].y = src[1][1]; mat[1].z = src[1][2]; mat[1].s = src[1][3]; mat[1].t = src[1][4]; + mat[2].x = src[2][0]; mat[2].y = src[2][1]; mat[2].z = src[2][2]; mat[2].s = src[2][3]; mat[2].t = src[2][4]; + mat[3].x = src[3][0]; mat[3].y = src[3][1]; mat[3].z = src[3][2]; mat[3].s = src[3][3]; mat[3].t = src[3][4]; + mat[4].x = src[4][0]; mat[4].y = src[4][1]; mat[4].z = src[4][2]; mat[4].s = src[4][3]; mat[4].t = src[4][4]; } ID_INLINE idMat5::idMat5( const idVec5 &v0, const idVec5 &v1, const idVec5 &v2, const idVec5 &v3, const idVec5 &v4 ) { @@ -1383,7 +1398,11 @@ ID_INLINE bool idMat5::operator!=( const idMat5 &a ) const { } ID_INLINE void idMat5::Zero( void ) { - memset( mat, 0, sizeof( idMat5 ) ); + mat[0].x = 0.0f; mat[0].y = 0.0f; mat[0].z = 0.0f; mat[0].s = 0.0f; mat[0].t = 0.0f; + mat[1].x = 0.0f; mat[1].y = 0.0f; mat[1].z = 0.0f; mat[1].s = 0.0f; mat[1].t = 0.0f; + mat[2].x = 0.0f; mat[2].y = 0.0f; mat[2].z = 0.0f; mat[2].s = 0.0f; mat[2].t = 0.0f; + mat[3].x = 0.0f; mat[3].y = 0.0f; mat[3].z = 0.0f; mat[3].s = 0.0f; mat[3].t = 0.0f; + mat[4].x = 0.0f; mat[4].y = 0.0f; mat[4].z = 0.0f; mat[4].s = 0.0f; mat[4].t = 0.0f; } ID_INLINE void idMat5::Identity( void ) { @@ -1537,7 +1556,12 @@ ID_INLINE idMat6::idMat6( const idVec6 &v0, const idVec6 &v1, const idVec6 &v2, } ID_INLINE idMat6::idMat6( const float src[ 6 ][ 6 ] ) { - memcpy( mat, src, 6 * 6 * sizeof( float ) ); + memcpy( mat[0].ToFloatPtr(), src[0], 6 * sizeof( float ) ); + memcpy( mat[1].ToFloatPtr(), src[1], 6 * sizeof( float ) ); + memcpy( mat[2].ToFloatPtr(), src[2], 6 * sizeof( float ) ); + memcpy( mat[3].ToFloatPtr(), src[3], 6 * sizeof( float ) ); + memcpy( mat[4].ToFloatPtr(), src[4], 6 * sizeof( float ) ); + memcpy( mat[5].ToFloatPtr(), src[5], 6 * sizeof( float ) ); } ID_INLINE const idVec6 &idMat6::operator[]( int index ) const { @@ -1700,7 +1724,9 @@ ID_INLINE bool idMat6::operator!=( const idMat6 &a ) const { } ID_INLINE void idMat6::Zero( void ) { - memset( mat, 0, sizeof( idMat6 ) ); + for (int i = 0; i < 6; i++) { + mat[i].Zero(); + } } ID_INLINE void idMat6::Identity( void ) { From b4d10536be76fa4c90aa304d015ee6fb623a72c9 Mon Sep 17 00:00:00 2001 From: Turo Lamminen Date: Mon, 25 Jan 2021 13:07:04 +0200 Subject: [PATCH 10/33] Silence an uninitialized variable warning --- idlib/geometry/TraceModel.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/idlib/geometry/TraceModel.cpp b/idlib/geometry/TraceModel.cpp index 56e0958..62dd273 100644 --- a/idlib/geometry/TraceModel.cpp +++ b/idlib/geometry/TraceModel.cpp @@ -1164,6 +1164,7 @@ int idTraceModel::GetOrderedSilhouetteEdges( const int edgeIsSilEdge[MAX_TRACEMO int i, j, edgeNum, numSilEdges, nextSilVert; int unsortedSilEdges[MAX_TRACEMODEL_EDGES]; + unsortedSilEdges[0] = 0; numSilEdges = 0; for ( i = 1; i <= numEdges; i++ ) { if ( edgeIsSilEdge[i] ) { From 67dbb961433d36d2d38e245f1db73adedc6f5229 Mon Sep 17 00:00:00 2001 From: Daniel Gibson Date: Wed, 8 Jan 2025 03:02:53 +0100 Subject: [PATCH 11/33] MSVC: Treat pointer truncation warnings as errors, adjust idCVar for that All pointer<->integer conversion truncation warnings I'm aware of are now enabled for MSVC, to hopefully finally get that tool code 64bit-clean. I had to adjust the idCVar code for this - it should've been safe enough (highly unlikely that a valid pointer is exactly 0xFFFFFFFF on 64bit), but triggered those warnings - now it should behave the same as before but the warnings (which are now errors) are silenced. --- CMakeLists.txt | 3 +++ framework/CVarSystem.h | 8 +++++--- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 7187682..20d210f 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -168,6 +168,9 @@ if(CMAKE_COMPILER_IS_GNUCC OR CMAKE_C_COMPILER_ID STREQUAL "Clang") endif() elseif(MSVC) add_compile_options(/W4) + add_compile_options(/we4840) # treat as error when passing a class to a vararg-function (probably printf-like) + # treat several kinds of truncating int<->pointer conversions as errors (for more 64bit-safety) + add_compile_options(/we4306 /we4311 /we4312 /we4302) add_compile_options(/wd4100) # unreferenced formal parameter add_compile_options(/wd4127) # conditional expression is constant add_compile_options(/wd4244) # possible loss of data diff --git a/framework/CVarSystem.h b/framework/CVarSystem.h index 1f1bd31..e0ae01c 100644 --- a/framework/CVarSystem.h +++ b/framework/CVarSystem.h @@ -182,6 +182,8 @@ class idCVar { static idCVar * staticVars; }; +static idCVar const * const staticCVarsInvalid = (const idCVar*)(uintptr_t)0xFFFFFFFF; + ID_INLINE idCVar::idCVar( const char *name, const char *value, int flags, const char *description, argCompletion_t valueCompletion ) { if ( !valueCompletion && ( flags & CVAR_BOOL ) ) { @@ -293,7 +295,7 @@ ID_INLINE void idCVar::Init( const char *name, const char *value, int flags, con this->integerValue = 0; this->floatValue = 0.0f; this->internalVar = this; - if ( staticVars != (idCVar *)0xFFFFFFFF ) { + if ( staticVars != staticCVarsInvalid ) { this->next = staticVars; staticVars = this; } else { @@ -302,11 +304,11 @@ ID_INLINE void idCVar::Init( const char *name, const char *value, int flags, con } ID_INLINE void idCVar::RegisterStaticVars( void ) { - if ( staticVars != (idCVar *)0xFFFFFFFF ) { + if ( staticVars != staticCVarsInvalid ) { for ( idCVar *cvar = staticVars; cvar; cvar = cvar->next ) { cvarSystem->Register( cvar ); } - staticVars = (idCVar *)0xFFFFFFFF; + staticVars = (idCVar *)staticCVarsInvalid; } } From 77e1b68bf2a942551dfb61c33a7a71e264c622a2 Mon Sep 17 00:00:00 2001 From: Daniel Gibson Date: Tue, 17 May 2022 02:37:32 +0200 Subject: [PATCH 12/33] Make sure MAX_OSPATH has sane size --- framework/FileSystem.h | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/framework/FileSystem.h b/framework/FileSystem.h index 529ed5b..7692e55 100644 --- a/framework/FileSystem.h +++ b/framework/FileSystem.h @@ -59,7 +59,10 @@ If you have questions concerning this license or the applicable additional terms static const ID_TIME_T FILE_NOT_FOUND_TIMESTAMP = 0xFFFFFFFF; static const int MAX_PURE_PAKS = 128; -static const int MAX_OSPATH = FILENAME_MAX; +// DG: https://www.gnu.org/software/libc/manual/html_node/Limits-for-Files.html says +// that FILENAME_MAX can be *really* big on some systems and thus is not suitable +// for buffer lengths. So limit it to prevent stack overflow/out of memory issues +static const int MAX_OSPATH = (FILENAME_MAX < 32000) ? FILENAME_MAX : 32000; // modes for OpenFileByMode. used as bit mask internally typedef enum { From 6dda5b066f5d0a4109afc0f451f90479efb23a68 Mon Sep 17 00:00:00 2001 From: Daniel Gibson Date: Tue, 17 May 2022 02:51:49 +0200 Subject: [PATCH 13/33] Don't use stringDataAllocator in idStr, it's not thread-safe idStr is used in both the main thread and the async sound thread, so it should better be thread-safe.. idDynamicBlockAlloc is not. Use realloc() and free() instead. For some reason this caused a lot more crashes (due to inconsistencies in the allocator's heap) with newer Linux distros (like XUbuntu 20.04) and when using GCC9, while they rarely reproduced with GCC7 or on XUbuntu 18.04 fixes #391 --- idlib/Str.cpp | 29 ++++++++++++++++++++--------- 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/idlib/Str.cpp b/idlib/Str.cpp index 4ccfb96..9bd6b1b 100644 --- a/idlib/Str.cpp +++ b/idlib/Str.cpp @@ -33,7 +33,11 @@ If you have questions concerning this license or the applicable additional terms #include "idlib/Str.h" -#if !defined( ID_REDIRECT_NEWDELETE ) && !defined( MACOS_X ) +// DG: idDynamicBlockAlloc isn't thread-safe and idStr is used both in the main thread +// and the async thread! For some reason this seems to cause lots of problems on +// newer Linux distros if dhewm3 is built with GCC9 or newer (see #391). +// No idea why it apparently didn't cause that (noticeable) issues before.. +#if 0 // !defined( ID_REDIRECT_NEWDELETE ) && !defined( MACOS_X ) #define USE_STRING_DATA_ALLOCATOR #endif @@ -100,23 +104,30 @@ void idStr::ReAllocate( int amount, bool keepold ) { #ifdef USE_STRING_DATA_ALLOCATOR newbuffer = stringDataAllocator.Alloc( alloced ); -#else - newbuffer = new char[ alloced ]; -#endif if ( keepold && data ) { data[ len ] = '\0'; strcpy( newbuffer, data ); } if ( data && data != baseBuffer ) { -#ifdef USE_STRING_DATA_ALLOCATOR stringDataAllocator.Free( data ); -#else - delete [] data; -#endif } data = newbuffer; +#else + if ( data && data != baseBuffer ) { + data = (char *)realloc( data, newsize ); + } else { + newbuffer = (char *)malloc( newsize ); + if ( data && keepold ) { + memcpy( newbuffer, data, len ); + newbuffer[ len ] = '\0'; + } else { + newbuffer[ 0 ] = '\0'; + } + data = newbuffer; + } +#endif } /* @@ -129,7 +140,7 @@ void idStr::FreeData( void ) { #ifdef USE_STRING_DATA_ALLOCATOR stringDataAllocator.Free( data ); #else - delete[] data; + free( data ); #endif data = baseBuffer; } From cc10a033a4ce00fe6c7693a866df00d6e6b5bd74 Mon Sep 17 00:00:00 2001 From: Daniel Gibson Date: Mon, 10 Jan 2022 04:06:54 +0100 Subject: [PATCH 14/33] Add D3_(v)snprintfC99() for C99-compatible implementations These are now used by idStr::(v)snPrintf(), and in the future can be used if a (v)snprintf() that's guaranteed not to call common->Warning() or similar is needed (e.g. used during early startup) --- idlib/Str.cpp | 84 ++++++++++++++++++++++++++++++++++++++++----------- idlib/Str.h | 11 +++++++ 2 files changed, 77 insertions(+), 18 deletions(-) diff --git a/idlib/Str.cpp b/idlib/Str.cpp index 9bd6b1b..e730233 100644 --- a/idlib/Str.cpp +++ b/idlib/Str.cpp @@ -30,6 +30,7 @@ If you have questions concerning this license or the applicable additional terms #include "idlib/math/Vector.h" #include "idlib/Heap.h" #include "framework/Common.h" +#include #include "idlib/Str.h" @@ -1513,21 +1514,25 @@ idStr::snPrintf ================ */ int idStr::snPrintf( char *dest, int size, const char *fmt, ...) { - int len; va_list argptr; - char buffer[32000]; // big, but small enough to fit in PPC stack - + int len; va_start( argptr, fmt ); - len = vsprintf( buffer, fmt, argptr ); + len = D3_vsnprintfC99(dest, size, fmt, argptr); va_end( argptr ); - if ( len >= sizeof( buffer ) ) { + if ( len >= 32000 ) { + // TODO: Previously this function used a 32000 byte buffer to write into + // with vsprintf(), and raised this error if that was overflowed + // (more likely that'd have lead to a crash..). + // Technically we don't have that restriction anymore, so I'm unsure + // if this error should really still be raised to preserve + // the old intended behavior, maybe for compat with mod DLLs using + // the old version of the function or something? idLib::common->Error( "idStr::snPrintf: overflowed buffer" ); } if ( len >= size ) { idLib::common->Warning( "idStr::snPrintf: overflow of %i in %i\n", len, size ); len = size; } - idStr::Copynz( dest, buffer, size ); return len; } @@ -1550,18 +1555,7 @@ or returns -1 on failure or if the buffer would be overflowed. ============ */ int idStr::vsnPrintf( char *dest, int size, const char *fmt, va_list argptr ) { - int ret; - -#ifdef _WIN32 -#undef _vsnprintf - ret = _vsnprintf( dest, size-1, fmt, argptr ); -#define _vsnprintf use_idStr_vsnPrintf -#else -#undef vsnprintf - ret = vsnprintf( dest, size, fmt, argptr ); -#define vsnprintf use_idStr_vsnPrintf -#endif - dest[size-1] = '\0'; + int ret = D3_vsnprintfC99(dest, size, fmt, argptr); if ( ret < 0 || ret >= size ) { return -1; } @@ -1790,3 +1784,57 @@ idStr idStr::FormatNumber( int number ) { return string; } + +// behaves like C99's vsnprintf() by returning the amount of bytes that +// *would* have been written into a big enough buffer, even if that's > size +// unlike idStr::vsnPrintf() which returns -1 in that case +int D3_vsnprintfC99(char *dst, size_t size, const char *format, va_list ap) +{ + // before VS2015, it didn't have a standards-conforming (v)snprintf()-implementation + // same might be true for other windows compilers if they use old CRT versions, like MinGW does +#if defined(_WIN32) && (!defined(_MSC_VER) || _MSC_VER < 1900) + #undef _vsnprintf + // based on DG_vsnprintf() from https://github.com/DanielGibson/Snippets/blob/master/DG_misc.h + int ret = -1; + if(dst != NULL && size > 0) + { +#if defined(_MSC_VER) && _MSC_VER >= 1400 + // I think MSVC2005 introduced _vsnprintf_s(). + // this shuts up _vsnprintf() security/deprecation warnings. + ret = _vsnprintf_s(dst, size, _TRUNCATE, format, ap); +#else + ret = _vsnprintf(dst, size, format, ap); + dst[size-1] = '\0'; // ensure '\0'-termination +#endif + } + + if(ret == -1) + { + // _vsnprintf() returns -1 if the output is truncated + // it's also -1 if dst or size were NULL/0, so the user didn't want to write + // we want to return the number of characters that would've been + // needed, though.. fortunately _vscprintf() calculates that. + ret = _vscprintf(format, ap); + } + return ret; + #define _vsnprintf use_idStr_vsnPrintf +#else // other operating systems and VisualC++ >= 2015 should have a proper vsnprintf() + #undef vsnprintf + return vsnprintf(dst, size, format, ap); + #define vsnprintf use_idStr_vsnPrintf +#endif +} + +// behaves like C99's snprintf() by returning the amount of bytes that +// *would* have been written into a big enough buffer, even if that's > size +// unlike idStr::snPrintf() which returns the written bytes in that case +// and also calls common->Warning() in case of overflows +int D3_snprintfC99(char *dst, size_t size, const char *format, ...) +{ + int ret = 0; + va_list argptr; + va_start( argptr, format ); + ret = D3_vsnprintfC99(dst, size, format, argptr); + va_end( argptr ); + return ret; +} diff --git a/idlib/Str.h b/idlib/Str.h index 5dfabe9..a44bab2 100644 --- a/idlib/Str.h +++ b/idlib/Str.h @@ -1068,4 +1068,15 @@ ID_INLINE int idStr::DynamicMemoryUsed() const { return ( data == baseBuffer ) ? 0 : alloced; } +// behaves like C99's snprintf() by returning the amount of bytes that +// *would* have been written into a big enough buffer, even if that's > size +// unlike idStr::snPrintf() which returns the written bytes in that case +// and also calls common->Warning() in case of overflows +int D3_snprintfC99(char *dst, size_t size, const char *format, ...) id_attribute((format(printf,3,4))); + +// behaves like C99's vsnprintf() by returning the amount of bytes that +// *would* have been written into a big enough buffer, even if that's > size +// unlike idStr::vsnPrintf() which returns -1 in that case +int D3_vsnprintfC99(char *dst, size_t size, const char *format, va_list ap); + #endif /* !__STR_H__ */ From 213a256e81c4e9d36c0dd15cf031033cc709465d Mon Sep 17 00:00:00 2001 From: Daniel Gibson Date: Tue, 17 May 2022 03:10:28 +0200 Subject: [PATCH 15/33] renderer/RenderSystem.h: Changes from dhewm3 only affect glconfig_t which I don't think any mod should use --- renderer/RenderSystem.h | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/renderer/RenderSystem.h b/renderer/RenderSystem.h index 253b9eb..b57aad9 100644 --- a/renderer/RenderSystem.h +++ b/renderer/RenderSystem.h @@ -59,7 +59,7 @@ typedef struct glconfig_s { int maxTextureImageUnits; float maxTextureAnisotropy; - int colorBits, depthBits, stencilBits; + int colorBits, alphabits, depthBits, stencilBits; bool multitextureAvailable; bool textureCompressionAvailable; @@ -88,6 +88,10 @@ typedef struct glconfig_s { bool allowARB2Path; bool isInitialized; + + // DG: current video backend is known to need opaque default framebuffer + // used if r_fillWindowAlphaChan == -1 + bool shouldFillWindowAlpha; } glconfig_t; From 7eb84e07bdec752d519d7a079d92070b495ce5cf Mon Sep 17 00:00:00 2001 From: Daniel Gibson Date: Tue, 17 May 2022 03:12:55 +0200 Subject: [PATCH 16/33] Remove usage of C++11 nullptr --- d3xp/physics/Physics_Player.cpp | 2 +- game/physics/Physics_Player.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/d3xp/physics/Physics_Player.cpp b/d3xp/physics/Physics_Player.cpp index d68ee7e..b6e8afa 100644 --- a/d3xp/physics/Physics_Player.cpp +++ b/d3xp/physics/Physics_Player.cpp @@ -1639,7 +1639,7 @@ void idPhysics_Player::Restore( idRestoreGame *savefile ) { * Resetting the axis to mat3_identity when restoring a savegame works around that issue * and makes sure players can go on playing if their savegame was "corrupted" by saving * while idPush was active. See https://github.com/dhewm/dhewm3/issues/328 for more details */ - if ( clipModel != nullptr ) { + if ( clipModel != NULL ) { clipModel->SetPosition( clipModel->GetOrigin(), mat3_identity ); } } diff --git a/game/physics/Physics_Player.cpp b/game/physics/Physics_Player.cpp index a3dc290..1f98647 100644 --- a/game/physics/Physics_Player.cpp +++ b/game/physics/Physics_Player.cpp @@ -1639,7 +1639,7 @@ void idPhysics_Player::Restore( idRestoreGame *savefile ) { * Resetting the axis to mat3_identity when restoring a savegame works around that issue * and makes sure players can go on playing if their savegame was "corrupted" by saving * while idPush was active. See https://github.com/dhewm/dhewm3/issues/328 for more details */ - if ( clipModel != nullptr ) { + if ( clipModel != NULL ) { clipModel->SetPosition( clipModel->GetOrigin(), mat3_identity ); } } From a55339e95f90e0e84815cb12fe8b1eb0962d269f Mon Sep 17 00:00:00 2001 From: Daniel Gibson Date: Tue, 17 May 2022 03:16:21 +0200 Subject: [PATCH 17/33] From dhewm3: Fix most (according to warnings) remaining 64bit issues in tool code only the TypeInfo changes are applicable to the SDK --- d3xp/gamesys/TypeInfo.cpp | 10 +++++++++- game/gamesys/TypeInfo.cpp | 11 ++++++++++- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/d3xp/gamesys/TypeInfo.cpp b/d3xp/gamesys/TypeInfo.cpp index 3e4853f..20cf62c 100644 --- a/d3xp/gamesys/TypeInfo.cpp +++ b/d3xp/gamesys/TypeInfo.cpp @@ -569,10 +569,18 @@ int idTypeInfoTools::WriteVariable_r( const void *varPtr, const char *varName, c return typeSize; } +#if D3_SIZEOFPTR == 4 + const uintptr_t uninitPtr = (uintptr_t)0xcdcdcdcdUL; +#elif D3_SIZEOFPTR == 8 + const uintptr_t uninitPtr = (uintptr_t)0xcdcdcdcdcdcdcdcdULL; +#else +#error "Unexpected pointer size" +#endif + // if this is a pointer isPointer = 0; for ( i = typeString.Length(); i > 0 && typeString[i - 1] == '*'; i -= 2 ) { - if ( varPtr == (void *)0xcdcdcdcd || ( varPtr != NULL && *((unsigned int *)varPtr) == 0xcdcdcdcd ) ) { + if ( varPtr == (void*)uninitPtr || ( varPtr != NULL && *((unsigned int *)varPtr) == 0xcdcdcdcd ) ) { common->Warning( "%s%s::%s%s references uninitialized memory", prefix, scope, varName, "" ); return typeSize; } diff --git a/game/gamesys/TypeInfo.cpp b/game/gamesys/TypeInfo.cpp index 3e4853f..d1249fb 100644 --- a/game/gamesys/TypeInfo.cpp +++ b/game/gamesys/TypeInfo.cpp @@ -570,9 +570,18 @@ int idTypeInfoTools::WriteVariable_r( const void *varPtr, const char *varName, c } // if this is a pointer + +#if D3_SIZEOFPTR == 4 + const uintptr_t uninitPtr = (uintptr_t)0xcdcdcdcdUL; +#elif D3_SIZEOFPTR == 8 + const uintptr_t uninitPtr = (uintptr_t)0xcdcdcdcdcdcdcdcdULL; +#else + #error "Unexpected pointer size" +#endif + isPointer = 0; for ( i = typeString.Length(); i > 0 && typeString[i - 1] == '*'; i -= 2 ) { - if ( varPtr == (void *)0xcdcdcdcd || ( varPtr != NULL && *((unsigned int *)varPtr) == 0xcdcdcdcd ) ) { + if ( varPtr == (void*)uninitPtr || ( varPtr != NULL && *((unsigned int *)varPtr) == 0xcdcdcdcd ) ) { common->Warning( "%s%s::%s%s references uninitialized memory", prefix, scope, varName, "" ); return typeSize; } From bd5d08dcdcb38ec8ad5e83a1048f193424808762 Mon Sep 17 00:00:00 2001 From: Daniel Gibson Date: Tue, 17 May 2022 03:23:12 +0200 Subject: [PATCH 18/33] Use idStr::Copynz() instead of strncpy() to guarantee \0-termination (only the applicable parts of that dhewm3 commit) --- d3xp/Game_local.cpp | 4 ++-- d3xp/Game_network.cpp | 2 +- game/Game_local.cpp | 4 ++-- game/Game_network.cpp | 2 +- idlib/Heap.cpp | 2 +- 5 files changed, 7 insertions(+), 7 deletions(-) diff --git a/d3xp/Game_local.cpp b/d3xp/Game_local.cpp index e6b8b1d..8d71e41 100644 --- a/d3xp/Game_local.cpp +++ b/d3xp/Game_local.cpp @@ -3019,7 +3019,7 @@ gameReturn_t idGameLocal::RunFrame( const usercmd_t *clientCmds ) { // see if a target_sessionCommand has forced a changelevel if ( sessionCommand.Length() ) { - strncpy( ret.sessionCommand, sessionCommand, sizeof( ret.sessionCommand ) ); + idStr::Copynz( ret.sessionCommand, sessionCommand, sizeof( ret.sessionCommand ) ); break; } @@ -5560,7 +5560,7 @@ idGameLocal::GetBestGameType */ void idGameLocal::GetBestGameType( const char* map, const char* gametype, char buf[ MAX_STRING_CHARS ] ) { idStr aux = mpGame.GetBestGametype( map, gametype ); - strncpy( buf, aux.c_str(), MAX_STRING_CHARS ); + idStr::Copynz( buf, aux.c_str(), MAX_STRING_CHARS ); buf[ MAX_STRING_CHARS - 1 ] = '\0'; } diff --git a/d3xp/Game_network.cpp b/d3xp/Game_network.cpp index b0c1b57..9519b1e 100644 --- a/d3xp/Game_network.cpp +++ b/d3xp/Game_network.cpp @@ -1896,7 +1896,7 @@ gameReturn_t idGameLocal::ClientPrediction( int clientNum, const usercmd_t *clie } if ( sessionCommand.Length() ) { - strncpy( ret.sessionCommand, sessionCommand, sizeof( ret.sessionCommand ) ); + idStr::Copynz( ret.sessionCommand, sessionCommand, sizeof( ret.sessionCommand ) ); } return ret; } diff --git a/game/Game_local.cpp b/game/Game_local.cpp index 7a6e942..882ed6a 100644 --- a/game/Game_local.cpp +++ b/game/Game_local.cpp @@ -2831,7 +2831,7 @@ gameReturn_t idGameLocal::RunFrame( const usercmd_t *clientCmds ) { // see if a target_sessionCommand has forced a changelevel if ( sessionCommand.Length() ) { - strncpy( ret.sessionCommand, sessionCommand, sizeof( ret.sessionCommand ) ); + idStr::Copynz( ret.sessionCommand, sessionCommand, sizeof( ret.sessionCommand ) ); break; } @@ -5305,7 +5305,7 @@ idGameLocal::GetBestGameType ============ */ void idGameLocal::GetBestGameType( const char* map, const char* gametype, char buf[ MAX_STRING_CHARS ] ) { - strncpy( buf, gametype, MAX_STRING_CHARS ); + idStr::Copynz( buf, gametype, MAX_STRING_CHARS ); buf[ MAX_STRING_CHARS - 1 ] = '\0'; } diff --git a/game/Game_network.cpp b/game/Game_network.cpp index 35e0f55..44449bd 100644 --- a/game/Game_network.cpp +++ b/game/Game_network.cpp @@ -1883,7 +1883,7 @@ gameReturn_t idGameLocal::ClientPrediction( int clientNum, const usercmd_t *clie } if ( sessionCommand.Length() ) { - strncpy( ret.sessionCommand, sessionCommand, sizeof( ret.sessionCommand ) ); + idStr::Copynz( ret.sessionCommand, sessionCommand, sizeof( ret.sessionCommand ) ); } return ret; } diff --git a/idlib/Heap.cpp b/idlib/Heap.cpp index a76fb4a..e69f008 100644 --- a/idlib/Heap.cpp +++ b/idlib/Heap.cpp @@ -1275,7 +1275,7 @@ const char *Mem_CleanupFileName( const char *fileName ) { newFileName = newFileName.Left( i2 - 1 ) + newFileName.Right( newFileName.Length() - ( i1 + 4 ) ); } index = ( index + 1 ) & 3; - strncpy( newFileNames[index], newFileName.c_str(), sizeof( newFileNames[index] ) ); + idStr::Copynz( newFileNames[index], newFileName.c_str(), sizeof( newFileNames[index] ) ); return newFileNames[index]; } From 96de541b6eec90b23d62d0ce7b41333aa9d4feb0 Mon Sep 17 00:00:00 2001 From: Daniel Gibson Date: Wed, 8 Jan 2025 03:07:53 +0100 Subject: [PATCH 19/33] Improve some messages in game code - TestHugeTranslation() prints positions (and asserts only afterwards), from "Work around assertion in alphalabs4, fix #409" - idCompiler::CompileFile() prints proper filename from "Fix usage of invalid pointer in idCompiler::CompileFile()" --- d3xp/physics/Clip.cpp | 21 +++++++++++---------- d3xp/script/Script_Compiler.cpp | 10 +++++++++- game/physics/Clip.cpp | 10 ++++++---- game/script/Script_Compiler.cpp | 10 +++++++++- 4 files changed, 35 insertions(+), 16 deletions(-) diff --git a/d3xp/physics/Clip.cpp b/d3xp/physics/Clip.cpp index 73ee5cb..376e912 100644 --- a/d3xp/physics/Clip.cpp +++ b/d3xp/physics/Clip.cpp @@ -968,16 +968,6 @@ idClip::TestHugeTranslation */ ID_INLINE bool TestHugeTranslation( trace_t &results, const idClipModel *mdl, const idVec3 &start, const idVec3 &end, const idMat3 &trmAxis ) { if ( mdl != NULL && ( end - start ).LengthSqr() > Square( CM_MAX_TRACE_DIST ) ) { -#ifndef CTF - // May be important: This occurs in CTF when a player connects and spawns - // in the PVS of a player that has a flag that is spawning the idMoveableItem - // "nuggets". The error seems benign and the assert was getting in the way - // of testing. - if (!gameLocal.mpGame.IsGametypeCoopBased()) { //Avoid crash for clients in coop (and in rare cases for server), ROE did the same in CTF so... - assert( 0 ); - } -#endif - results.fraction = 0.0f; results.endpos = start; results.endAxis = trmAxis; @@ -989,6 +979,17 @@ ID_INLINE bool TestHugeTranslation( trace_t &results, const idClipModel *mdl, co } else { gameLocal.Printf( "huge translation for clip model %d\n", mdl->GetId() ); } + gameLocal.Printf( " from (%.2f %.2f %.2f) to (%.2f %.2f %.2f)\n", start.x, start.y, start.z, end.x, end.y, end.z); + +#ifndef CTF + // May be important: This occurs in CTF when a player connects and spawns + // in the PVS of a player that has a flag that is spawning the idMoveableItem + // "nuggets". The error seems benign and the assert was getting in the way + // of testing. + if (!gameLocal.mpGame.IsGametypeCoopBased()) { //Avoid crash for clients in coop (and in rare cases for server), ROE did the same in CTF so... + assert( 0 ); + } +#endif return true; } return false; diff --git a/d3xp/script/Script_Compiler.cpp b/d3xp/script/Script_Compiler.cpp index 6fac0a6..fd18ea3 100644 --- a/d3xp/script/Script_Compiler.cpp +++ b/d3xp/script/Script_Compiler.cpp @@ -28,6 +28,7 @@ If you have questions concerning this license or the applicable additional terms #include "sys/platform.h" #include "idlib/Timer.h" +#include "framework/FileSystem.h" #include "script/Script_Thread.h" #include "Game_local.h" @@ -2590,6 +2591,8 @@ void idCompiler::CompileFile( const char *text, const char *filename, bool toCon compile_time.Start(); + idStr origFileName = filename; // DG: filename pointer might become invalid when calling NextToken() below + scope = &def_namespace; basetype = NULL; callthread = false; @@ -2657,6 +2660,11 @@ void idCompiler::CompileFile( const char *text, const char *filename, bool toCon compile_time.Stop(); if ( !toConsole ) { - gameLocal.Printf( "Compiled '%s': %u ms\n", filename, compile_time.Milliseconds() ); + // DG: filename can be overwritten by NextToken() (via gameLocal.program.GetFilenum()), so + // use a copy, origFileName, that's still valid here. Furthermore, the path is nonsense, + // as idProgram::CompileText() called fileSystem->RelativePathToOSPath() on it + // which does not return the *actual* full path of that file but invents one, + // so revert that to the relative filename which at least isn't misleading + gameLocal.Printf( "Compiled '%s': %u ms\n", fileSystem->OSPathToRelativePath(origFileName), compile_time.Milliseconds() ); } } diff --git a/game/physics/Clip.cpp b/game/physics/Clip.cpp index 15649a7..59e5fe6 100644 --- a/game/physics/Clip.cpp +++ b/game/physics/Clip.cpp @@ -967,10 +967,6 @@ idClip::TestHugeTranslation */ ID_INLINE bool TestHugeTranslation( trace_t &results, const idClipModel *mdl, const idVec3 &start, const idVec3 &end, const idMat3 &trmAxis ) { if ( mdl != NULL && ( end - start ).LengthSqr() > Square( CM_MAX_TRACE_DIST ) ) { - if (!gameLocal.mpGame.IsGametypeCoopBased()) { //Avoid crash for clients in coop (and in rare cases for server), ROE did the same in CTF so... - assert( 0 ); - } - results.fraction = 0.0f; results.endpos = start; results.endAxis = trmAxis; @@ -984,6 +980,12 @@ ID_INLINE bool TestHugeTranslation( trace_t &results, const idClipModel *mdl, co } else { gameLocal.Printf( "huge translation for clip model %d\n", mdl->GetId() ); } + + gameLocal.Printf( " from (%.2f %.2f %.2f) to (%.2f %.2f %.2f)\n", start.x, start.y, start.z, end.x, end.y, end.z); + + if (!gameLocal.mpGame.IsGametypeCoopBased()) { //Avoid crash for clients in coop (and in rare cases for server), ROE did the same in CTF so... + assert( 0 ); + } return true; } return false; diff --git a/game/script/Script_Compiler.cpp b/game/script/Script_Compiler.cpp index 1ad2693..c8ef137 100644 --- a/game/script/Script_Compiler.cpp +++ b/game/script/Script_Compiler.cpp @@ -28,6 +28,7 @@ If you have questions concerning this license or the applicable additional terms #include "sys/platform.h" #include "idlib/Timer.h" +#include "framework/FileSystem.h" #include "script/Script_Thread.h" #include "Game_local.h" @@ -2590,6 +2591,8 @@ void idCompiler::CompileFile( const char *text, const char *filename, bool toCon compile_time.Start(); + idStr origFileName = filename; // DG: filename pointer might become invalid when calling NextToken() below + scope = &def_namespace; basetype = NULL; callthread = false; @@ -2657,6 +2660,11 @@ void idCompiler::CompileFile( const char *text, const char *filename, bool toCon compile_time.Stop(); if ( !toConsole ) { - gameLocal.Printf( "Compiled '%s': %u ms\n", filename, compile_time.Milliseconds() ); + // DG: filename can be overwritten by NextToken() (via gameLocal.program.GetFilenum()), so + // use a copy, origFileName, that's still valid here. Furthermore, the path is nonsense, + // as idProgram::CompileText() called fileSystem->RelativePathToOSPath() on it + // which does not return the *actual* full path of that file but invents one, + // so revert that to the relative filename which at least isn't misleading + gameLocal.Printf( "Compiled '%s': %u ms\n", fileSystem->OSPathToRelativePath(origFileName), compile_time.Milliseconds() ); } } From 8a5d7f250efb28225aa3f451d336f3ebcf80b53b Mon Sep 17 00:00:00 2001 From: Daniel Gibson Date: Tue, 22 Jun 2021 22:39:02 +0200 Subject: [PATCH 20/33] Fix some ubsan warnings --- d3xp/SmokeParticles.cpp | 2 +- game/SmokeParticles.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/d3xp/SmokeParticles.cpp b/d3xp/SmokeParticles.cpp index 710baec..4c63357 100644 --- a/d3xp/SmokeParticles.cpp +++ b/d3xp/SmokeParticles.cpp @@ -222,7 +222,7 @@ bool idSmokeParticles::EmitSmoke( const idDeclParticle *smoke, const int systemS int finalParticleTime = stage->cycleMsec * stage->spawnBunching; int deltaMsec = gameLocal.time - systemStartTime; - int nowCount, prevCount; + int nowCount=0, prevCount=0; if ( finalParticleTime == 0 ) { // if spawnBunching is 0, they will all come out at once if ( gameLocal.time == systemStartTime ) { diff --git a/game/SmokeParticles.cpp b/game/SmokeParticles.cpp index e63307d..b0867a7 100644 --- a/game/SmokeParticles.cpp +++ b/game/SmokeParticles.cpp @@ -207,7 +207,7 @@ bool idSmokeParticles::EmitSmoke( const idDeclParticle *smoke, const int systemS int finalParticleTime = stage->cycleMsec * stage->spawnBunching; int deltaMsec = gameLocal.time - systemStartTime; - int nowCount, prevCount; + int nowCount=0, prevCount=0; if ( finalParticleTime == 0 ) { // if spawnBunching is 0, they will all come out at once if ( gameLocal.time == systemStartTime ) { From 6a7931136a8a858144683c331f65a1bedf3ac46e Mon Sep 17 00:00:00 2001 From: Daniel Gibson Date: Sun, 6 Nov 2022 18:12:54 +0100 Subject: [PATCH 21/33] Fix renderlights loaded from savegames aliasing other lights Some entities wrote the handle from gameRenderWorld->AddLightDef() into savegames and reused it after restoring it. That's a bad idea, because at that point the handle most likely belongs to something else (likely some idLight). The most visible issue this can create is that the flashlight may not work correctly after loading a savegame with flashlight on, when it happens to alias a light that's updated each frame to (mostly) being off.. The correct way to handle this (THAT FOR SOME REASON WAS ALREADY IMPLEMENTED IN D3XP BUT NOT THE BASE GAME - WHY?!) is to get a fresh handle with AddLightDef() when restoring a savegame - unless the handle was -1, which means that the light didn't exist when saving. fixes #495 --- game/Moveable.cpp | 9 +++++++++ game/Projectile.cpp | 5 +++++ game/Weapon.cpp | 14 +++++++++++++- 3 files changed, 27 insertions(+), 1 deletion(-) diff --git a/game/Moveable.cpp b/game/Moveable.cpp index 474b8cd..2728f80 100644 --- a/game/Moveable.cpp +++ b/game/Moveable.cpp @@ -838,6 +838,15 @@ void idExplodingBarrel::Restore( idRestoreGame *savefile ) { savefile->ReadInt( particleTime ); savefile->ReadInt( lightTime ); savefile->ReadFloat( time ); + + // DG: enforce getting fresh handle, else this may be tied to an unrelated light! + if ( lightDefHandle != -1 ) { + lightDefHandle = gameRenderWorld->AddLightDef( &light ); + } + // DG: same for render entity + if ( particleModelDefHandle != -1 ) { + particleModelDefHandle = gameRenderWorld->AddEntityDef( &particleRenderEntity ); + } } /* diff --git a/game/Projectile.cpp b/game/Projectile.cpp index 3213eaa..02c22d1 100644 --- a/game/Projectile.cpp +++ b/game/Projectile.cpp @@ -162,6 +162,11 @@ void idProjectile::Restore( idRestoreGame *savefile ) { savefile->ReadRenderLight( renderLight ); savefile->ReadInt( (int &)lightDefHandle ); + // DG: enforce getting fresh handle, else this may be tied to an unrelated light! + if ( lightDefHandle != -1 ) { + lightDefHandle = gameRenderWorld->AddLightDef( &renderLight ); + } + savefile->ReadVec3( lightOffset ); savefile->ReadInt( lightStartTime ); savefile->ReadInt( lightEndTime ); diff --git a/game/Weapon.cpp b/game/Weapon.cpp index 353c4a7..d9006a2 100644 --- a/game/Weapon.cpp +++ b/game/Weapon.cpp @@ -463,12 +463,21 @@ void idWeapon::Restore( idRestoreGame *savefile ) { savefile->ReadInt( guiLightHandle ); savefile->ReadRenderLight( guiLight ); - + // DG: we need to get a fresh handle, otherwise this will be tied to a completely unrelated light! + if ( guiLightHandle != -1 ) { + guiLightHandle = gameRenderWorld->AddLightDef( &guiLight ); + } savefile->ReadInt( muzzleFlashHandle ); savefile->ReadRenderLight( muzzleFlash ); + if ( muzzleFlashHandle != -1 ) { // DG: enforce getting fresh handle + muzzleFlashHandle = gameRenderWorld->AddLightDef( &muzzleFlash ); + } savefile->ReadInt( worldMuzzleFlashHandle ); savefile->ReadRenderLight( worldMuzzleFlash ); + if ( worldMuzzleFlashHandle != -1 ) { // DG: enforce getting fresh handle + worldMuzzleFlashHandle = gameRenderWorld->AddLightDef( &worldMuzzleFlash ); + } savefile->ReadVec3( flashColor ); savefile->ReadInt( muzzleFlashEnd ); @@ -526,6 +535,9 @@ void idWeapon::Restore( idRestoreGame *savefile ) { savefile->ReadInt( nozzleGlowHandle ); savefile->ReadRenderLight( nozzleGlow ); + if ( nozzleGlowHandle != -1 ) { // DG: enforce getting fresh handle + nozzleGlowHandle = gameRenderWorld->AddLightDef( &nozzleGlow ); + } savefile->ReadVec3( nozzleGlowColor ); savefile->ReadMaterial( nozzleGlowShader ); From 0e1beeb9edee0630c0cdffbacedb1cd0e934d629 Mon Sep 17 00:00:00 2001 From: Daniel Gibson Date: Thu, 29 Dec 2022 05:38:13 +0100 Subject: [PATCH 22/33] Fix -Wformat-security warnings - thanks James Addison! This is based on https://github.com/dhewm/dhewm3/pull/500 by https://github.com/jayaddison See also https://github.com/blendogames/quadrilateralcowboy/pull/4 --- d3xp/Game_local.cpp | 2 +- d3xp/Game_network.cpp | 2 +- d3xp/MultiplayerGame.cpp | 22 +++++++++++----------- d3xp/gamesys/SysCmds.cpp | 4 ++-- d3xp/script/Script_Thread.cpp | 4 ++-- game/Game_local.cpp | 2 +- game/Game_network.cpp | 2 +- game/MultiplayerGame.cpp | 29 +++++++++++++++-------------- game/gamesys/SysCmds.cpp | 4 ++-- game/script/Script_Thread.cpp | 4 ++-- idlib/Parser.cpp | 4 ++-- idlib/math/Simd.cpp | 2 +- 12 files changed, 41 insertions(+), 40 deletions(-) diff --git a/d3xp/Game_local.cpp b/d3xp/Game_local.cpp index 8d71e41..f3ce7cf 100644 --- a/d3xp/Game_local.cpp +++ b/d3xp/Game_local.cpp @@ -1406,7 +1406,7 @@ bool idGameLocal::NextMap( void ) { int i; if ( !g_mapCycle.GetString()[0] ) { - Printf( common->GetLanguageDict()->GetString( "#str_04294" ) ); + Printf( "%s", common->GetLanguageDict()->GetString( "#str_04294" ) ); return false; } if ( fileSystem->ReadFile( g_mapCycle.GetString(), NULL, NULL ) < 0 ) { diff --git a/d3xp/Game_network.cpp b/d3xp/Game_network.cpp index 9519b1e..00e8448 100644 --- a/d3xp/Game_network.cpp +++ b/d3xp/Game_network.cpp @@ -868,7 +868,7 @@ void idGameLocal::NetworkEventWarning( const entityNetEvent_t *event, const char va_end( argptr ); idStr::Append( buf, sizeof(buf), "\n" ); - common->DWarning( buf ); + common->DWarning( "%s", buf ); } /* diff --git a/d3xp/MultiplayerGame.cpp b/d3xp/MultiplayerGame.cpp index 1819714..3bb3bbd 100644 --- a/d3xp/MultiplayerGame.cpp +++ b/d3xp/MultiplayerGame.cpp @@ -2968,10 +2968,10 @@ void idMultiplayerGame::PrintMessageEvent( int to, msg_evt_t evt, int parm1, int AddChatLine( common->GetLanguageDict()->GetString( "#str_04289" ), gameLocal.userInfo[ parm1 ].GetString( "ui_name" ) ); break; case MSG_VOTE: - AddChatLine( common->GetLanguageDict()->GetString( "#str_04288" ) ); + AddChatLine( "%s", common->GetLanguageDict()->GetString( "#str_04288" ) ); break; case MSG_SUDDENDEATH: - AddChatLine( common->GetLanguageDict()->GetString( "#str_04287" ) ); + AddChatLine( "%s", common->GetLanguageDict()->GetString( "#str_04287" ) ); break; case MSG_FORCEREADY: AddChatLine( common->GetLanguageDict()->GetString( "#str_04286" ), gameLocal.userInfo[ parm1 ].GetString( "ui_name" ) ); @@ -2983,7 +2983,7 @@ void idMultiplayerGame::PrintMessageEvent( int to, msg_evt_t evt, int parm1, int AddChatLine( common->GetLanguageDict()->GetString( "#str_04285" ), gameLocal.userInfo[ parm1 ].GetString( "ui_name" ) ); break; case MSG_TIMELIMIT: - AddChatLine( common->GetLanguageDict()->GetString( "#str_04284" ) ); + AddChatLine( "%s", common->GetLanguageDict()->GetString( "#str_04284" ) ); break; case MSG_FRAGLIMIT: if ( gameLocal.gameType == GAME_LASTMAN ) { @@ -2998,7 +2998,7 @@ void idMultiplayerGame::PrintMessageEvent( int to, msg_evt_t evt, int parm1, int AddChatLine( common->GetLanguageDict()->GetString( "#str_04280" ), gameLocal.userInfo[ parm1 ].GetString( "ui_name" ), parm2 ? common->GetLanguageDict()->GetString( "#str_02500" ) : common->GetLanguageDict()->GetString( "#str_02499" ) ); break; case MSG_HOLYSHIT: - AddChatLine( common->GetLanguageDict()->GetString( "#str_06732" ) ); + AddChatLine( "%s", common->GetLanguageDict()->GetString( "#str_06732" ) ); break; #ifdef CTF case MSG_POINTLIMIT: @@ -3024,9 +3024,9 @@ void idMultiplayerGame::PrintMessageEvent( int to, msg_evt_t evt, int parm1, int break; if ( gameLocal.GetLocalPlayer()->team != parm1 ) { - AddChatLine( common->GetLanguageDict()->GetString( "#str_11103" ) ); // your team + AddChatLine( "%s", common->GetLanguageDict()->GetString( "#str_11103" ) ); // your team } else { - AddChatLine( common->GetLanguageDict()->GetString( "#str_11104" ) ); // enemy + AddChatLine( "%s", common->GetLanguageDict()->GetString( "#str_11104" ) ); // enemy } break; @@ -3410,7 +3410,7 @@ void idMultiplayerGame::ClientStartVote( int clientNum, const char *_voteString } voteString = _voteString; - AddChatLine( va( common->GetLanguageDict()->GetString( "#str_04279" ), gameLocal.userInfo[ clientNum ].GetString( "ui_name" ) ) ); + AddChatLine( common->GetLanguageDict()->GetString( "#str_04279" ), gameLocal.userInfo[ clientNum ].GetString( "ui_name" ) ); gameSoundWorld->PlayShaderDirectly( GlobalSoundStrings[ SND_VOTE ] ); if ( clientNum == gameLocal.localClientNum ) { voted = true; @@ -3450,14 +3450,14 @@ void idMultiplayerGame::ClientUpdateVote( vote_result_t status, int yesCount, in switch ( status ) { case VOTE_FAILED: - AddChatLine( common->GetLanguageDict()->GetString( "#str_04278" ) ); + AddChatLine( "%s", common->GetLanguageDict()->GetString( "#str_04278" ) ); gameSoundWorld->PlayShaderDirectly( GlobalSoundStrings[ SND_VOTE_FAILED ] ); if ( gameLocal.isClient ) { vote = VOTE_NONE; } break; case VOTE_PASSED: - AddChatLine( common->GetLanguageDict()->GetString( "#str_04277" ) ); + AddChatLine( "%s", common->GetLanguageDict()->GetString( "#str_04277" ) ); gameSoundWorld->PlayShaderDirectly( GlobalSoundStrings[ SND_VOTE_PASSED ] ); break; case VOTE_RESET: @@ -3466,7 +3466,7 @@ void idMultiplayerGame::ClientUpdateVote( vote_result_t status, int yesCount, in } break; case VOTE_ABORTED: - AddChatLine( common->GetLanguageDict()->GetString( "#str_04276" ) ); + AddChatLine( "%s", common->GetLanguageDict()->GetString( "#str_04276" ) ); if ( gameLocal.isClient ) { vote = VOTE_NONE; } @@ -4060,7 +4060,7 @@ void idMultiplayerGame::ToggleSpectate( void ) { if ( gameLocal.serverInfo.GetBool( "si_spectators" ) ) { cvarSystem->SetCVarString( "ui_spectate", "Spectate" ); } else { - gameLocal.mpGame.AddChatLine( common->GetLanguageDict()->GetString( "#str_06747" ) ); + gameLocal.mpGame.AddChatLine( "%s", common->GetLanguageDict()->GetString( "#str_06747" ) ); } } } diff --git a/d3xp/gamesys/SysCmds.cpp b/d3xp/gamesys/SysCmds.cpp index 5f4b49f..a673a81 100644 --- a/d3xp/gamesys/SysCmds.cpp +++ b/d3xp/gamesys/SysCmds.cpp @@ -704,7 +704,7 @@ Cmd_AddChatLine_f ================== */ static void Cmd_AddChatLine_f( const idCmdArgs &args ) { - gameLocal.mpGame.AddChatLine( args.Argv( 1 ) ); + gameLocal.mpGame.AddChatLine( "%s", args.Argv( 1 ) ); } /* @@ -1307,7 +1307,7 @@ static void PrintFloat( float f ) { buf[i] = ' '; } buf[i] = '\0'; - gameLocal.Printf( buf ); + gameLocal.Printf( "%s", buf ); } /* diff --git a/d3xp/script/Script_Thread.cpp b/d3xp/script/Script_Thread.cpp index 41d7f0f..8ba3256 100644 --- a/d3xp/script/Script_Thread.cpp +++ b/d3xp/script/Script_Thread.cpp @@ -832,7 +832,7 @@ void idThread::Error( const char *fmt, ... ) const { vsprintf( text, fmt, argptr ); va_end( argptr ); - interpreter.Error( text ); + interpreter.Error( "%s", text ); } /* @@ -848,7 +848,7 @@ void idThread::Warning( const char *fmt, ... ) const { vsprintf( text, fmt, argptr ); va_end( argptr ); - interpreter.Warning( text ); + interpreter.Warning( "%s", text ); } /* diff --git a/game/Game_local.cpp b/game/Game_local.cpp index 882ed6a..a5c254c 100644 --- a/game/Game_local.cpp +++ b/game/Game_local.cpp @@ -1334,7 +1334,7 @@ bool idGameLocal::NextMap( void ) { int i; if ( !g_mapCycle.GetString()[0] ) { - Printf( common->GetLanguageDict()->GetString( "#str_04294" ) ); + Printf( "%s", common->GetLanguageDict()->GetString( "#str_04294" ) ); return false; } if ( fileSystem->ReadFile( g_mapCycle.GetString(), NULL, NULL ) < 0 ) { diff --git a/game/Game_network.cpp b/game/Game_network.cpp index 44449bd..3410f5c 100644 --- a/game/Game_network.cpp +++ b/game/Game_network.cpp @@ -874,7 +874,7 @@ void idGameLocal::NetworkEventWarning( const entityNetEvent_t *event, const char va_end( argptr ); idStr::Append( buf, sizeof(buf), "\n" ); - common->DWarning( buf ); + common->DWarning( "%s", buf ); } /* diff --git a/game/MultiplayerGame.cpp b/game/MultiplayerGame.cpp index b3f3fa0..70d4885 100644 --- a/game/MultiplayerGame.cpp +++ b/game/MultiplayerGame.cpp @@ -590,11 +590,12 @@ const char *idMultiplayerGame::GameTime() { ms = 0; } - s = ms / 1000; - m = s / 60; - s -= m * 60; - t = s / 10; - s -= t * 10; + s = ms / 1000; // => s <= 2 147 483 (INT_MAX / 1000) + m = s / 60; // => m <= 35 791 + s -= m * 60; // => s < 60 + t = s / 10; // => t < 6 + s -= t * 10; // => s < 10 + // writing <= 5 for m + 3 bytes for ":ts" + 1 byte for \0 => 16 bytes is enough sprintf( buff, "%i:%i%i", m, t, s ); } @@ -2364,10 +2365,10 @@ void idMultiplayerGame::PrintMessageEvent( int to, msg_evt_t evt, int parm1, int AddChatLine( common->GetLanguageDict()->GetString( "#str_04289" ), gameLocal.userInfo[ parm1 ].GetString( "ui_name" ) ); break; case MSG_VOTE: - AddChatLine( common->GetLanguageDict()->GetString( "#str_04288" ) ); + AddChatLine( "%s", common->GetLanguageDict()->GetString( "#str_04288" ) ); break; case MSG_SUDDENDEATH: - AddChatLine( common->GetLanguageDict()->GetString( "#str_04287" ) ); + AddChatLine( "%s", common->GetLanguageDict()->GetString( "#str_04287" ) ); break; case MSG_FORCEREADY: AddChatLine( common->GetLanguageDict()->GetString( "#str_04286" ), gameLocal.userInfo[ parm1 ].GetString( "ui_name" ) ); @@ -2379,7 +2380,7 @@ void idMultiplayerGame::PrintMessageEvent( int to, msg_evt_t evt, int parm1, int AddChatLine( common->GetLanguageDict()->GetString( "#str_04285" ), gameLocal.userInfo[ parm1 ].GetString( "ui_name" ) ); break; case MSG_TIMELIMIT: - AddChatLine( common->GetLanguageDict()->GetString( "#str_04284" ) ); + AddChatLine( "%s", common->GetLanguageDict()->GetString( "#str_04284" ) ); break; case MSG_FRAGLIMIT: if ( gameLocal.gameType == GAME_LASTMAN ) { @@ -2394,7 +2395,7 @@ void idMultiplayerGame::PrintMessageEvent( int to, msg_evt_t evt, int parm1, int AddChatLine( common->GetLanguageDict()->GetString( "#str_04280" ), gameLocal.userInfo[ parm1 ].GetString( "ui_name" ), parm2 ? common->GetLanguageDict()->GetString( "#str_02500" ) : common->GetLanguageDict()->GetString( "#str_02499" ) ); break; case MSG_HOLYSHIT: - AddChatLine( common->GetLanguageDict()->GetString( "#str_06732" ) ); + AddChatLine( "%s", common->GetLanguageDict()->GetString( "#str_06732" ) ); break; default: gameLocal.DPrintf( "PrintMessageEvent: unknown message type %d\n", evt ); @@ -2742,7 +2743,7 @@ void idMultiplayerGame::ClientStartVote( int clientNum, const char *_voteString } voteString = _voteString; - AddChatLine( va( common->GetLanguageDict()->GetString( "#str_04279" ), gameLocal.userInfo[ clientNum ].GetString( "ui_name" ) ) ); + AddChatLine( common->GetLanguageDict()->GetString( "#str_04279" ), gameLocal.userInfo[ clientNum ].GetString( "ui_name" ) ); gameSoundWorld->PlayShaderDirectly( GlobalSoundStrings[ SND_VOTE ] ); if ( clientNum == gameLocal.localClientNum ) { voted = true; @@ -2782,14 +2783,14 @@ void idMultiplayerGame::ClientUpdateVote( vote_result_t status, int yesCount, in switch ( status ) { case VOTE_FAILED: - AddChatLine( common->GetLanguageDict()->GetString( "#str_04278" ) ); + AddChatLine( "%s", common->GetLanguageDict()->GetString( "#str_04278" ) ); gameSoundWorld->PlayShaderDirectly( GlobalSoundStrings[ SND_VOTE_FAILED ] ); if ( gameLocal.isClient ) { vote = VOTE_NONE; } break; case VOTE_PASSED: - AddChatLine( common->GetLanguageDict()->GetString( "#str_04277" ) ); + AddChatLine( "%s", common->GetLanguageDict()->GetString( "#str_04277" ) ); gameSoundWorld->PlayShaderDirectly( GlobalSoundStrings[ SND_VOTE_PASSED ] ); break; case VOTE_RESET: @@ -2798,7 +2799,7 @@ void idMultiplayerGame::ClientUpdateVote( vote_result_t status, int yesCount, in } break; case VOTE_ABORTED: - AddChatLine( common->GetLanguageDict()->GetString( "#str_04276" ) ); + AddChatLine( "%s", common->GetLanguageDict()->GetString( "#str_04276" ) ); if ( gameLocal.isClient ) { vote = VOTE_NONE; } @@ -3357,7 +3358,7 @@ void idMultiplayerGame::ToggleSpectate( void ) { if ( gameLocal.serverInfo.GetBool( "si_spectators" ) ) { cvarSystem->SetCVarString( "ui_spectate", "Spectate" ); } else { - gameLocal.mpGame.AddChatLine( common->GetLanguageDict()->GetString( "#str_06747" ) ); + gameLocal.mpGame.AddChatLine( "%s", common->GetLanguageDict()->GetString( "#str_06747" ) ); } } } diff --git a/game/gamesys/SysCmds.cpp b/game/gamesys/SysCmds.cpp index 07dabee..4a34c41 100644 --- a/game/gamesys/SysCmds.cpp +++ b/game/gamesys/SysCmds.cpp @@ -634,7 +634,7 @@ Cmd_AddChatLine_f ================== */ static void Cmd_AddChatLine_f( const idCmdArgs &args ) { - gameLocal.mpGame.AddChatLine( args.Argv( 1 ) ); + gameLocal.mpGame.AddChatLine( "%s", args.Argv( 1 ) ); } /* @@ -1237,7 +1237,7 @@ static void PrintFloat( float f ) { buf[i] = ' '; } buf[i] = '\0'; - gameLocal.Printf( buf ); + gameLocal.Printf( "%s", buf ); } /* diff --git a/game/script/Script_Thread.cpp b/game/script/Script_Thread.cpp index a441f65..8126ad0 100644 --- a/game/script/Script_Thread.cpp +++ b/game/script/Script_Thread.cpp @@ -804,7 +804,7 @@ void idThread::Error( const char *fmt, ... ) const { vsprintf( text, fmt, argptr ); va_end( argptr ); - interpreter.Error( text ); + interpreter.Error( "%s", text ); } /* @@ -820,7 +820,7 @@ void idThread::Warning( const char *fmt, ... ) const { vsprintf( text, fmt, argptr ); va_end( argptr ); - interpreter.Warning( text ); + interpreter.Warning( "%s", text ); } /* diff --git a/idlib/Parser.cpp b/idlib/Parser.cpp index 86a57d3..4ef2b50 100644 --- a/idlib/Parser.cpp +++ b/idlib/Parser.cpp @@ -326,7 +326,7 @@ void idParser::Error( const char *str, ... ) const { vsprintf(text, str, ap); va_end(ap); if ( idParser::scriptstack ) { - idParser::scriptstack->Error( text ); + idParser::scriptstack->Error( "%s", text ); } } @@ -343,7 +343,7 @@ void idParser::Warning( const char *str, ... ) const { vsprintf(text, str, ap); va_end(ap); if ( idParser::scriptstack ) { - idParser::scriptstack->Warning( text ); + idParser::scriptstack->Warning( "%s", text ); } } diff --git a/idlib/math/Simd.cpp b/idlib/math/Simd.cpp index 7fbad1e..6cbdae1 100644 --- a/idlib/math/Simd.cpp +++ b/idlib/math/Simd.cpp @@ -213,7 +213,7 @@ PrintClocks void PrintClocks( const char *string, int dataCount, int clocks, int otherClocks = 0 ) { int i; - idLib::common->Printf( string ); + idLib::common->Printf( "%s", string ); for ( i = idStr::LengthWithoutColors(string); i < 48; i++ ) { idLib::common->Printf(" "); } From a52df3d78c0f4ee95b1c2e19d991b492c3645662 Mon Sep 17 00:00:00 2001 From: Daniel Gibson Date: Thu, 5 Jan 2023 04:57:54 +0100 Subject: [PATCH 23/33] Don't use "register" keyword, it was deprecated in C++11 .. and even removed in C++17 - and before that it probably didn't do much in most compilers --- idlib/Lib.cpp | 2 +- idlib/hashing/MD5.cpp | 2 +- idlib/math/Simd_Generic.cpp | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/idlib/Lib.cpp b/idlib/Lib.cpp index 3c83bae..8433095 100644 --- a/idlib/Lib.cpp +++ b/idlib/Lib.cpp @@ -300,7 +300,7 @@ RESULTS Reverses the byte order in each of elcount elements. ===================================================================== */ ID_INLINE static void RevBytesSwap( void *bp, int elsize, int elcount ) { - register unsigned char *p, *q; + unsigned char *p, *q; p = ( unsigned char * ) bp; diff --git a/idlib/hashing/MD5.cpp b/idlib/hashing/MD5.cpp index de9f855..f4f9836 100644 --- a/idlib/hashing/MD5.cpp +++ b/idlib/hashing/MD5.cpp @@ -54,7 +54,7 @@ the data and converts bytes into longwords for this routine. ================= */ void MD5_Transform( unsigned int state[4], unsigned int in[16] ) { - register unsigned int a, b, c, d; + unsigned int a, b, c, d; a = state[0]; b = state[1]; diff --git a/idlib/math/Simd_Generic.cpp b/idlib/math/Simd_Generic.cpp index c20f6a4..9ce25f6 100644 --- a/idlib/math/Simd_Generic.cpp +++ b/idlib/math/Simd_Generic.cpp @@ -1802,7 +1802,7 @@ void VPCALL idSIMD_Generic::MatX_LowerTriangularSolve( const idMatX &L, float *x lptr = L[skip]; int i, j; - register double s0, s1, s2, s3; + double s0, s1, s2, s3; for ( i = skip; i < n; i++ ) { s0 = lptr[0] * x[0]; @@ -1928,7 +1928,7 @@ void VPCALL idSIMD_Generic::MatX_LowerTriangularSolveTranspose( const idMatX &L, } int i, j; - register double s0, s1, s2, s3; + double s0, s1, s2, s3; float *xptr; lptr = L.ToFloatPtr() + n * nc + n - 4; From 2ce31ae2603598790a94bebf61629720a202219d Mon Sep 17 00:00:00 2001 From: Daniel Gibson Date: Thu, 5 Jan 2023 06:21:07 +0100 Subject: [PATCH 24/33] Fix GCC -W(maybe-)uninitialized warnings that at least kinda had a point --- idlib/geometry/Surface_Patch.cpp | 7 ++++++- idlib/geometry/Winding.cpp | 15 +++++++++++++-- idlib/geometry/Winding2D.cpp | 11 +++++++++++ idlib/math/Simd_SSE.cpp | 8 ++++++++ 4 files changed, 38 insertions(+), 3 deletions(-) diff --git a/idlib/geometry/Surface_Patch.cpp b/idlib/geometry/Surface_Patch.cpp index 6f6f744..f898c50 100644 --- a/idlib/geometry/Surface_Patch.cpp +++ b/idlib/geometry/Surface_Patch.cpp @@ -224,6 +224,7 @@ idSurface_Patch::LerpVert ============ */ void idSurface_Patch::LerpVert( const idDrawVert &a, const idDrawVert &b, idDrawVert &out ) const { + // DG: TODO: what about out.tangent and out.color ? out.xyz[0] = 0.5f * ( a.xyz[0] + b.xyz[0] ); out.xyz[1] = 0.5f * ( a.xyz[1] + b.xyz[1] ); out.xyz[2] = 0.5f * ( a.xyz[2] + b.xyz[2] ); @@ -554,7 +555,11 @@ idSurface_Patch::Subdivide */ void idSurface_Patch::Subdivide( float maxHorizontalError, float maxVerticalError, float maxLength, bool genNormals ) { int i, j, k, l; - idDrawVert prev, next, mid; + // DG: to shut up GCC (maybe-)uninitialized warnings, initialize prev, next and mid + // (maybe the warnings were at least partly correct, because .tangent and .color aren't set by idSurface_Patch::LerpVert()) + idDrawVert prev; + prev.Clear(); + idDrawVert next = prev, mid = prev; idVec3 prevxyz, nextxyz, midxyz; idVec3 delta; float maxHorizontalErrorSqr, maxVerticalErrorSqr, maxLengthSqr; diff --git a/idlib/geometry/Winding.cpp b/idlib/geometry/Winding.cpp index 5f38423..2def85f 100644 --- a/idlib/geometry/Winding.cpp +++ b/idlib/geometry/Winding.cpp @@ -102,7 +102,12 @@ int idWinding::Split( const idPlane &plane, const float epsilon, idWinding **fro idWinding * f, *b; int maxpts; - assert( this ); + assert( this && numPoints > 0); + + // DG: unlikely, but makes sure we don't use uninitialized memory below + if ( numPoints == 0 ) { + return 0; // it's not like the callers check the return value anyway.. + } dists = (float *) _alloca( (numPoints+4) * sizeof( float ) ); sides = (byte *) _alloca( (numPoints+4) * sizeof( byte ) ); @@ -245,7 +250,13 @@ idWinding *idWinding::Clip( const idPlane &plane, const float epsilon, const boo idVec5 mid; int maxpts; - assert( this ); + assert( this && numPoints > 0 ); + + // DG: this shouldn't happen, probably, but if it does we'd use uninitialized memory below + if ( numPoints == 0 ) { + delete this; + return NULL; + } dists = (float *) _alloca( (numPoints+4) * sizeof( float ) ); sides = (byte *) _alloca( (numPoints+4) * sizeof( byte ) ); diff --git a/idlib/geometry/Winding2D.cpp b/idlib/geometry/Winding2D.cpp index 8978f1d..bb02e6d 100644 --- a/idlib/geometry/Winding2D.cpp +++ b/idlib/geometry/Winding2D.cpp @@ -92,6 +92,12 @@ void idWinding2D::ExpandForAxialBox( const idVec2 bounds[2] ) { assert( numPlanes < MAX_POINTS_ON_WINDING_2D ); planes[numPlanes++] = plane; } + + // DG: make sure planes[] isn't used uninitialized and with index -1 below + if ( numPlanes == 0 ) { + return; + } + if ( GetAxialBevel( planes[numPlanes-1], planes[0], p[0], bevel ) ) { planes[numPlanes++] = bevel; } @@ -259,6 +265,11 @@ bool idWinding2D::ClipInPlace( const idVec3 &plane, const float epsilon, const b float dot, dists[MAX_POINTS_ON_WINDING_2D+1]; idVec2 *p1, *p2, mid, newPoints[MAX_POINTS_ON_WINDING_2D+4]; + // DG: avoid all kinds of unitialized usages below + if ( numPoints == 0 ) { + return false; + } + counts[SIDE_FRONT] = counts[SIDE_BACK] = counts[SIDE_ON] = 0; for ( i = 0; i < numPoints; i++ ) { diff --git a/idlib/math/Simd_SSE.cpp b/idlib/math/Simd_SSE.cpp index 609cb59..a9dad8b 100644 --- a/idlib/math/Simd_SSE.cpp +++ b/idlib/math/Simd_SSE.cpp @@ -456,6 +456,14 @@ void VPCALL idSIMD_SSE::Dot( float *dst, const idVec3 &constant, const idPlane * char *dst_p; __m128 xmm0, xmm1, xmm2, xmm3, xmm4, xmm5, xmm6, xmm7; + // DG: GCC and clang warn about xmm1-4 maybe being used uninitialized below. + // according to https://stackoverflow.com/a/18749079 the initialization + // code is generated anyway, so make it explicit to shut up the warning + xmm1 = _mm_setzero_ps(); + xmm2 = _mm_setzero_ps(); + xmm3 = _mm_setzero_ps(); + xmm4 = _mm_setzero_ps(); + /* mov eax, count mov edi, constant From 14ff9b13e83f4b0d809334928b31742a5e5dfa24 Mon Sep 17 00:00:00 2001 From: Daniel Gibson Date: Thu, 5 Jan 2023 07:54:41 +0100 Subject: [PATCH 25/33] Work around false positive GCC -W(maybe-)uninitialized warnings shouldn't hurt too much, and the R_IssueEntityDefCallback() code even is a bit better now --- d3xp/ai/AI_pathing.cpp | 7 ++++--- game/ai/AI_pathing.cpp | 5 +++-- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/d3xp/ai/AI_pathing.cpp b/d3xp/ai/AI_pathing.cpp index 813f3f1..5692ab8 100644 --- a/d3xp/ai/AI_pathing.cpp +++ b/d3xp/ai/AI_pathing.cpp @@ -155,7 +155,7 @@ GetPointOutsideObstacles void GetPointOutsideObstacles( const obstacle_t *obstacles, const int numObstacles, idVec2 &point, int *obstacle, int *edgeNum ) { int i, j, k, n, bestObstacle, bestEdgeNum, queueStart, queueEnd, edgeNums[2]; float d, bestd, scale[2]; - idVec3 plane, bestPlane; + idVec3 plane, bestPlane(0.0f, 0.0f, 0.0f); // DG: init it to shut up compiler idVec2 newPoint, dir, bestPoint; int *queue; bool *obstacleVisited; @@ -1128,7 +1128,8 @@ idAI::PredictPath */ bool idAI::PredictPath( const idEntity *ent, const idAAS *aas, const idVec3 &start, const idVec3 &velocity, int totalTime, int frameTime, int stopEvent, predictedPath_t &path ) { int i, j, step, numFrames, curFrameTime; - idVec3 delta, curStart, curEnd, curVelocity, lastEnd, stepUp, tmpStart; + idVec3 delta, curStart, curEnd, curVelocity, lastEnd, tmpStart; + idVec3 stepUp(0,0,0); // DG: init this to get rid of compiler warning idVec3 gravity, gravityDir, invGravityDir; float maxStepHeight, minFloorCos; pathTrace_t trace; @@ -1183,7 +1184,7 @@ bool idAI::PredictPath( const idEntity *ent, const idAAS *aas, const idVec3 &sta return true; } - if ( step ) { + if ( step != 0 ) { // step down at end point tmpStart = trace.endPos; diff --git a/game/ai/AI_pathing.cpp b/game/ai/AI_pathing.cpp index 49566aa..82cd0be 100644 --- a/game/ai/AI_pathing.cpp +++ b/game/ai/AI_pathing.cpp @@ -157,7 +157,7 @@ GetPointOutsideObstacles void GetPointOutsideObstacles( const obstacle_t *obstacles, const int numObstacles, idVec2 &point, int *obstacle, int *edgeNum ) { int i, j, k, n, bestObstacle, bestEdgeNum, queueStart, queueEnd, edgeNums[2]; float d, bestd, scale[2]; - idVec3 plane, bestPlane; + idVec3 plane, bestPlane(0.0f, 0.0f, 0.0f); // DG: init it to shut up compiler idVec2 newPoint, dir, bestPoint; int *queue; bool *obstacleVisited; @@ -1131,7 +1131,8 @@ idAI::PredictPath */ bool idAI::PredictPath( const idEntity *ent, const idAAS *aas, const idVec3 &start, const idVec3 &velocity, int totalTime, int frameTime, int stopEvent, predictedPath_t &path ) { int i, j, step, numFrames, curFrameTime; - idVec3 delta, curStart, curEnd, curVelocity, lastEnd, stepUp, tmpStart; + idVec3 delta, curStart, curEnd, curVelocity, lastEnd, tmpStart; + idVec3 stepUp(0,0,0); // DG: init this to get rid of compiler warning idVec3 gravity, gravityDir, invGravityDir; float maxStepHeight, minFloorCos; pathTrace_t trace; From 6fb25a983be14aeed362ab0155e8d811c834b4a8 Mon Sep 17 00:00:00 2001 From: Daniel Gibson Date: Thu, 5 Jan 2023 09:05:52 +0100 Subject: [PATCH 26/33] Fix date/time handling in idParser::ExpandBuiltinDefine() this was so broken, I think if anyone ever tried to use __DATE__ or __TIME__ it must've crashed, either from free(curtime) (which was the return value of ctime()) or from things like token[7] = NULL when token was a pointer (to a single element!). I think it could work now. Also fixed potential memory leaks in cases that don't "return" anything --- idlib/Parser.cpp | 58 +++++++++++++++++++++++++++++++++++------------- 1 file changed, 43 insertions(+), 15 deletions(-) diff --git a/idlib/Parser.cpp b/idlib/Parser.cpp index 4ef2b50..dedf5cc 100644 --- a/idlib/Parser.cpp +++ b/idlib/Parser.cpp @@ -669,6 +669,31 @@ define_t *idParser::CopyFirstDefine( void ) { return NULL; } +// simple abstraction around localtime()/localtime_r() or whatever +static bool my_localtime(const time_t* t, struct tm* ts) +{ + // TODO: does any non-windows platform not support localtime_r()? + // then add them here (or handle them specially with their own #elif) +#ifdef _WIN32 + // localtime() is C89 so it should be available everywhere, but it *may* not be threadsafe. + // It *is* threadsafe on Windows though (there it returns a thread-local variable). + // (yes, Windows has localtime_s(), but it's unclear if MinGW supports that + // or localtime_r() and in the end, *on Windows* localtime() is safe so use it) + + struct tm* tmp = localtime(t); + if(tmp != NULL) { + *ts = *tmp; + return true; + } +#else // localtime_r() assumed available, use it (all Unix-likes incl. macOS support it, AROS as well) + if(localtime_r(t, ts) != NULL) + return true; +#endif + // if we get here, the localtime() (or localtime_r() or whatever) call failed + memset(ts, 0, sizeof(*ts)); // make sure ts has deterministic content + return false; +} + /* ================ idParser::ExpandBuiltinDefine @@ -677,7 +702,6 @@ idParser::ExpandBuiltinDefine int idParser::ExpandBuiltinDefine( idToken *deftoken, define_t *define, idToken **firsttoken, idToken **lasttoken ) { idToken *token; ID_TIME_T t; - char *curtime; char buf[MAX_STRING_CHARS]; token = new idToken(deftoken); @@ -709,14 +733,15 @@ int idParser::ExpandBuiltinDefine( idToken *deftoken, define_t *define, idToken } case BUILTIN_DATE: { t = time(NULL); - curtime = ctime(&t); - (*token) = "\""; - token->Append( curtime+4 ); - token[7] = NULL; - token->Append( curtime+20 ); - token[10] = NULL; - token->Append( "\"" ); - free(curtime); + // DG: apparently this was supposed to extract the date part of "Wed Jun 30 21:49:08 1993\n" + // (like "Jun 30 1993") - it was both ugly and broken before I changed it, though + // Originally it copied stuff out of ctime(), which is ugly but ok, but also + // free'd the returned value (wrong) and tried to modify token in ways that should've crashed + // (like token[7] = NULL; which should've been (*token)[7] = '\0';) + struct tm ts; + my_localtime(&t, &ts); + strftime(buf, sizeof(buf), "\"%b %d %Y\"", &ts); + (*token) = buf; token->type = TT_STRING; token->subtype = token->Length(); token->line = deftoken->line; @@ -728,12 +753,13 @@ int idParser::ExpandBuiltinDefine( idToken *deftoken, define_t *define, idToken } case BUILTIN_TIME: { t = time(NULL); - curtime = ctime(&t); - (*token) = "\""; - token->Append( curtime+11 ); - token[8] = NULL; - token->Append( "\"" ); - free(curtime); + // DG: apparently this was supposed to extract the time part of "Wed Jun 30 21:49:08 1993\n" + // (like "21:49:08") - it was both ugly and broken before I changed it, though + // (had basicaly the same problems as BUILTIN_DATE) + struct tm ts; + my_localtime(&t, &ts); + strftime(buf, sizeof(buf), "\"%H:%M:%S\"", &ts); + (*token) = buf; token->type = TT_STRING; token->subtype = token->Length(); token->line = deftoken->line; @@ -745,11 +771,13 @@ int idParser::ExpandBuiltinDefine( idToken *deftoken, define_t *define, idToken } case BUILTIN_STDC: { idParser::Warning( "__STDC__ not supported\n" ); + delete token; // DG: we probably shouldn't leak it, right? *firsttoken = NULL; *lasttoken = NULL; break; } default: { + delete token; // DG: we probably shouldn't leak it, right? *firsttoken = NULL; *lasttoken = NULL; break; From 0f8f412abb442e3afdfcb85b7576c758caeb41c4 Mon Sep 17 00:00:00 2001 From: Daniel Gibson Date: Thu, 5 Jan 2023 08:07:51 +0100 Subject: [PATCH 27/33] Fix/work around other misc. compiler warnings or, in the case of AF.cpp, at least comment on them I think this fixes most of the remaining "interesting" GCC 12 on Linux warnings --- d3xp/AF.cpp | 5 +++++ d3xp/Pvs.cpp | 3 ++- d3xp/anim/Anim_Blend.cpp | 4 +++- game/AF.cpp | 5 +++++ game/Pvs.cpp | 3 ++- game/anim/Anim_Blend.cpp | 4 +++- 6 files changed, 20 insertions(+), 4 deletions(-) diff --git a/d3xp/AF.cpp b/d3xp/AF.cpp index e527b14..1a97edc 100644 --- a/d3xp/AF.cpp +++ b/d3xp/AF.cpp @@ -892,6 +892,11 @@ bool idAF::Load( idEntity *ent, const char *fileName ) { for ( i = 0; i < physicsObj.GetNumConstraints(); i++ ) { idAFConstraint *constraint = physicsObj.GetConstraint( i ); for ( j = 0; j < file->constraints.Num(); j++ ) { + // DG: FIXME: GCC rightfully complains that file->constraints[j]->type and constraint->GetType() + // are of different enum types, and their values are different in some cases: + // CONSTRAINT_HINGESTEERING has no DECLAF_CONSTRAINT_ equivalent, + // and thus DECLAF_CONSTRAINT_SLIDER != CONSTRAINT_SLIDER (5 != 6) + // and DECLAF_CONSTRAINT_SPRING != CONSTRAINT_SPRING (6 != 10) if ( file->constraints[j]->name.Icmp( constraint->GetName() ) == 0 && file->constraints[j]->type == constraint->GetType() ) { break; diff --git a/d3xp/Pvs.cpp b/d3xp/Pvs.cpp index bb29a29..c6eaf0c 100644 --- a/d3xp/Pvs.cpp +++ b/d3xp/Pvs.cpp @@ -872,7 +872,8 @@ void idPVS::Shutdown( void ) { delete[] areaPVS; areaPVS = NULL; } - if ( currentPVS ) { + // if ( currentPVS ) - DG: can't be NULL + { for ( int i = 0; i < MAX_CURRENT_PVS; i++ ) { delete[] currentPVS[i].pvs; currentPVS[i].pvs = NULL; diff --git a/d3xp/anim/Anim_Blend.cpp b/d3xp/anim/Anim_Blend.cpp index 2a68745..0b8b370 100644 --- a/d3xp/anim/Anim_Blend.cpp +++ b/d3xp/anim/Anim_Blend.cpp @@ -172,7 +172,7 @@ index 0 will never be NULL. Any anim >= NumAnims will return NULL. ===================== */ const idMD5Anim *idAnim::MD5Anim( int num ) const { - if ( anims == NULL || anims[0] == NULL ) { + if ( anims[0] == NULL ) { return NULL; } return anims[ num ]; @@ -3084,6 +3084,7 @@ idDeclModelDef::NumJointsOnChannel int idDeclModelDef::NumJointsOnChannel( int channel ) const { if ( ( channel < 0 ) || ( channel >= ANIM_NumAnimChannels ) ) { gameLocal.Error( "idDeclModelDef::NumJointsOnChannel : channel out of range" ); + return 0; // unreachable, (Error() doesn't return) just to shut up compiler } return channelJoints[ channel ].Num(); } @@ -3096,6 +3097,7 @@ idDeclModelDef::GetChannelJoints const int * idDeclModelDef::GetChannelJoints( int channel ) const { if ( ( channel < 0 ) || ( channel >= ANIM_NumAnimChannels ) ) { gameLocal.Error( "idDeclModelDef::GetChannelJoints : channel out of range" ); + return NULL; // unreachable, (Error() doesn't return) just to shut up compiler } return channelJoints[ channel ].Ptr(); } diff --git a/game/AF.cpp b/game/AF.cpp index e527b14..1a97edc 100644 --- a/game/AF.cpp +++ b/game/AF.cpp @@ -892,6 +892,11 @@ bool idAF::Load( idEntity *ent, const char *fileName ) { for ( i = 0; i < physicsObj.GetNumConstraints(); i++ ) { idAFConstraint *constraint = physicsObj.GetConstraint( i ); for ( j = 0; j < file->constraints.Num(); j++ ) { + // DG: FIXME: GCC rightfully complains that file->constraints[j]->type and constraint->GetType() + // are of different enum types, and their values are different in some cases: + // CONSTRAINT_HINGESTEERING has no DECLAF_CONSTRAINT_ equivalent, + // and thus DECLAF_CONSTRAINT_SLIDER != CONSTRAINT_SLIDER (5 != 6) + // and DECLAF_CONSTRAINT_SPRING != CONSTRAINT_SPRING (6 != 10) if ( file->constraints[j]->name.Icmp( constraint->GetName() ) == 0 && file->constraints[j]->type == constraint->GetType() ) { break; diff --git a/game/Pvs.cpp b/game/Pvs.cpp index b9818ad..05b009d 100644 --- a/game/Pvs.cpp +++ b/game/Pvs.cpp @@ -872,7 +872,8 @@ void idPVS::Shutdown( void ) { delete[] areaPVS; areaPVS = NULL; } - if ( currentPVS ) { + // if ( currentPVS ) - DG: can't be NULL + { for ( int i = 0; i < MAX_CURRENT_PVS; i++ ) { delete[] currentPVS[i].pvs; currentPVS[i].pvs = NULL; diff --git a/game/anim/Anim_Blend.cpp b/game/anim/Anim_Blend.cpp index 1d49f87..a639402 100644 --- a/game/anim/Anim_Blend.cpp +++ b/game/anim/Anim_Blend.cpp @@ -172,7 +172,7 @@ index 0 will never be NULL. Any anim >= NumAnims will return NULL. ===================== */ const idMD5Anim *idAnim::MD5Anim( int num ) const { - if ( anims == NULL || anims[0] == NULL ) { + if ( anims[0] == NULL ) { return NULL; } return anims[ num ]; @@ -3009,6 +3009,7 @@ idDeclModelDef::NumJointsOnChannel int idDeclModelDef::NumJointsOnChannel( int channel ) const { if ( ( channel < 0 ) || ( channel >= ANIM_NumAnimChannels ) ) { gameLocal.Error( "idDeclModelDef::NumJointsOnChannel : channel out of range" ); + return 0; // unreachable, (Error() doesn't return) just to shut up compiler } return channelJoints[ channel ].Num(); } @@ -3021,6 +3022,7 @@ idDeclModelDef::GetChannelJoints const int * idDeclModelDef::GetChannelJoints( int channel ) const { if ( ( channel < 0 ) || ( channel >= ANIM_NumAnimChannels ) ) { gameLocal.Error( "idDeclModelDef::GetChannelJoints : channel out of range" ); + return NULL; // unreachable, (Error() doesn't return) just to shut up compiler } return channelJoints[ channel ].Ptr(); } From 422000f956e552cbd58b22a1b6d7734854b32c7d Mon Sep 17 00:00:00 2001 From: James Addison Date: Wed, 11 Jan 2023 16:16:34 +0000 Subject: [PATCH 28/33] Fixup: typo: 'hiehgt' -> 'height' in a few places around the codebase --- d3xp/ai/AI_pathing.cpp | 2 +- d3xp/gamesys/SysCvar.cpp | 2 +- game/ai/AI_pathing.cpp | 2 +- game/gamesys/SysCvar.cpp | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/d3xp/ai/AI_pathing.cpp b/d3xp/ai/AI_pathing.cpp index 5692ab8..2d1a5e8 100644 --- a/d3xp/ai/AI_pathing.cpp +++ b/d3xp/ai/AI_pathing.cpp @@ -1339,7 +1339,7 @@ static int Ballistics( const idVec3 &start, const idVec3 &end, float speed, floa ===================== HeightForTrajectory -Returns the maximum hieght of a given trajectory +Returns the maximum height of a given trajectory ===================== */ #if 0 diff --git a/d3xp/gamesys/SysCvar.cpp b/d3xp/gamesys/SysCvar.cpp index 4aa1634..41cdd6c 100644 --- a/d3xp/gamesys/SysCvar.cpp +++ b/d3xp/gamesys/SysCvar.cpp @@ -319,7 +319,7 @@ idCVar rb_showVelocity( "rb_showVelocity", "0", CVAR_GAME | CVAR_BOOL, "s idCVar rb_showActive( "rb_showActive", "0", CVAR_GAME | CVAR_BOOL, "show rigid bodies that are not at rest" ); // The default values for player movement cvars are set in def/player.def -idCVar pm_jumpheight( "pm_jumpheight", "48", CVAR_GAME | CVAR_NETWORKSYNC | CVAR_FLOAT, "approximate hieght the player can jump" ); +idCVar pm_jumpheight( "pm_jumpheight", "48", CVAR_GAME | CVAR_NETWORKSYNC | CVAR_FLOAT, "approximate height the player can jump" ); idCVar pm_stepsize( "pm_stepsize", "16", CVAR_GAME | CVAR_NETWORKSYNC | CVAR_FLOAT, "maximum height the player can step up without jumping" ); idCVar pm_crouchspeed( "pm_crouchspeed", "80", CVAR_GAME | CVAR_NETWORKSYNC | CVAR_FLOAT, "speed the player can move while crouched" ); idCVar pm_walkspeed( "pm_walkspeed", "140", CVAR_GAME | CVAR_NETWORKSYNC | CVAR_FLOAT, "speed the player can move while walking" ); diff --git a/game/ai/AI_pathing.cpp b/game/ai/AI_pathing.cpp index 82cd0be..10ec408 100644 --- a/game/ai/AI_pathing.cpp +++ b/game/ai/AI_pathing.cpp @@ -1342,7 +1342,7 @@ static int Ballistics( const idVec3 &start, const idVec3 &end, float speed, floa ===================== HeightForTrajectory -Returns the maximum hieght of a given trajectory +Returns the maximum height of a given trajectory ===================== */ #if 0 diff --git a/game/gamesys/SysCvar.cpp b/game/gamesys/SysCvar.cpp index 9489a5f..a62e00e 100644 --- a/game/gamesys/SysCvar.cpp +++ b/game/gamesys/SysCvar.cpp @@ -260,7 +260,7 @@ idCVar rb_showVelocity( "rb_showVelocity", "0", CVAR_GAME | CVAR_BOOL, "s idCVar rb_showActive( "rb_showActive", "0", CVAR_GAME | CVAR_BOOL, "show rigid bodies that are not at rest" ); // The default values for player movement cvars are set in def/player.def -idCVar pm_jumpheight( "pm_jumpheight", "48", CVAR_GAME | CVAR_NETWORKSYNC | CVAR_FLOAT, "approximate hieght the player can jump" ); +idCVar pm_jumpheight( "pm_jumpheight", "48", CVAR_GAME | CVAR_NETWORKSYNC | CVAR_FLOAT, "approximate height the player can jump" ); idCVar pm_stepsize( "pm_stepsize", "16", CVAR_GAME | CVAR_NETWORKSYNC | CVAR_FLOAT, "maximum height the player can step up without jumping" ); idCVar pm_crouchspeed( "pm_crouchspeed", "80", CVAR_GAME | CVAR_NETWORKSYNC | CVAR_FLOAT, "speed the player can move while crouched" ); idCVar pm_walkspeed( "pm_walkspeed", "140", CVAR_GAME | CVAR_NETWORKSYNC | CVAR_FLOAT, "speed the player can move while walking" ); From 11fb2228a3423d56a2b71e35144d879865685801 Mon Sep 17 00:00:00 2001 From: Daniel Gibson Date: Thu, 2 Mar 2023 18:04:48 +0100 Subject: [PATCH 29/33] Fix MSVC non-Release builds (_alloca()/assert() didn't play nice) --- idlib/Lib.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/idlib/Lib.h b/idlib/Lib.h index 90287ba..5b2e999 100644 --- a/idlib/Lib.h +++ b/idlib/Lib.h @@ -125,7 +125,8 @@ int IntForSixtets( byte *in ); #ifdef _DEBUG void AssertFailed( const char *file, int line, const char *expression ); #undef assert -#define assert( X ) if ( X ) { } else AssertFailed( __FILE__, __LINE__, #X ) +// DG: change assert to use ?: so I can use it in _alloca()/_alloca16() (MSVC didn't like if() in there) +#define assert( X ) (X) ? 1 : (AssertFailed( __FILE__, __LINE__, #X ), 0) #endif class idException { From 92a47b105defc5426e74adea2d4aa6b6befd5af7 Mon Sep 17 00:00:00 2001 From: Daniel Gibson Date: Mon, 18 Mar 2024 23:03:05 +0100 Subject: [PATCH 30/33] Sync sys/platform.h, incl. Workaround for MCST-LCC compiler Workaround for MCST-LCC compiler < 1.28 version mcst-lcc < 1.28 does not support __builtin_alloca_with_align() Ref: http://mcst.ru/lcc --- sys/platform.h | 76 +++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 69 insertions(+), 7 deletions(-) diff --git a/sys/platform.h b/sys/platform.h index d9a9a5b..1beb68b 100644 --- a/sys/platform.h +++ b/sys/platform.h @@ -32,6 +32,16 @@ If you have questions concerning this license or the applicable additional terms #include "config.h" #include "framework/BuildDefines.h" +#ifdef _WIN32 +#include // _alloca() +#endif + +// NOTE: By default Win32 uses a 1MB stack. Doom3 1.3.1 uses 4MB (probably set after compiling with EDITBIN /STACK +// dhewm3 now uses a 8MB stack, set with a linker flag in CMakeLists.txt (/STACK:8388608 for MSVC, -Wl,--stack,8388608 for mingw) +// Linux has a 8MB stack by default, and so does macOS, at least for the main thread +// anyway, a 2MB limit alloca should be safe even when using it multiple times in the same function +#define ID_MAX_ALLOCA_SIZE 2097152 // 2MB + /* =============================================================================== @@ -40,7 +50,7 @@ If you have questions concerning this license or the applicable additional terms =============================================================================== */ -// Win32 +// AROS #if defined(__AROS__) #define _alloca alloca @@ -71,7 +81,14 @@ If you have questions concerning this license or the applicable additional terms // Win32 #if defined(WIN32) || defined(_WIN32) -#define _alloca16( x ) ((void *)((((uintptr_t)_alloca( (x)+15 )) + 15) & ~15)) +#ifdef __MINGW32__ + #undef _alloca // in mingw _alloca is a #define + #define _alloca16( x ) ( (assert((x)= 1.28 + #define _alloca16( x ) ( ({assert((x) #endif -#if defined(__MINGW32__) - #include -#endif #include #include #include @@ -200,6 +250,18 @@ If you have questions concerning this license or the applicable additional terms #undef FindText // stupid namespace poluting Microsoft monkeys #endif +// Apple legacy +#ifdef __APPLE__ +#include +#ifdef __MAC_OS_X_VERSION_MIN_REQUIRED +#if __MAC_OS_X_VERSION_MIN_REQUIRED == 1040 +#define OSX_TIGER +#elif __MAC_OS_X_VERSION_MIN_REQUIRED < 1060 +#define OSX_LEOPARD +#endif +#endif +#endif + #define ID_TIME_T time_t typedef unsigned char byte; // 8 bits From da88e8b66e06f2465f35123368f0a70a38bc6184 Mon Sep 17 00:00:00 2001 From: Daniel Gibson Date: Wed, 8 Jan 2025 03:13:01 +0100 Subject: [PATCH 31/33] merge sys_public.h changes for gamepad support not sure if any of this is relevant for mods, but shouldn't hurt.. --- sys/sys_public.h | 66 ++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 55 insertions(+), 11 deletions(-) diff --git a/sys/sys_public.h b/sys/sys_public.h index fbac1a8..8d5116f 100644 --- a/sys/sys_public.h +++ b/sys/sys_public.h @@ -44,12 +44,12 @@ typedef enum { } cpuidSimd_t; typedef enum { - AXIS_SIDE, - AXIS_FORWARD, - AXIS_UP, - AXIS_ROLL, - AXIS_YAW, - AXIS_PITCH, + AXIS_LEFT_X, + AXIS_LEFT_Y, + AXIS_RIGHT_X, + AXIS_RIGHT_Y, + AXIS_LEFT_TRIG, + AXIS_RIGHT_TRIG, MAX_JOYSTICK_AXIS } joystickAxis_t; @@ -57,8 +57,9 @@ typedef enum { SE_NONE, // evTime is still valid SE_KEY, // evValue is a key code, evValue2 is the down flag SE_CHAR, // evValue is an ascii char - SE_MOUSE, // evValue and evValue2 are reletive signed x / y moves - SE_JOYSTICK_AXIS, // evValue is an axis number and evValue2 is the current state (-127 to 127) + SE_MOUSE, // evValue and evValue2 are relative signed x / y moves + SE_MOUSE_ABS, // evValue and evValue2 are absolute x / y coordinates in the window + SE_JOYSTICK, // evValue is an axis number and evValue2 is the current state (-127 to 127) SE_CONSOLE // evPtr is a char*, from typing something at a non-game console } sysEventType_t; @@ -76,10 +77,52 @@ typedef enum { M_DELTAZ } sys_mEvents; +typedef enum { + J_ACTION_FIRST, + // these names are similar to the SDL3 SDL_GamepadButton names + J_BTN_SOUTH = J_ACTION_FIRST, // bottom face button, like Xbox A + J_BTN_EAST, // right face button, like Xbox B + J_BTN_WEST, // left face button, like Xbox X + J_BTN_NORTH, // top face button, like Xbox Y + J_BTN_BACK, + J_BTN_GUIDE, // Note: this one should probably not be used? + J_BTN_START, + J_BTN_LSTICK, // press left stick + J_BTN_RSTICK, // press right stick + J_BTN_LSHOULDER, + J_BTN_RSHOULDER, + + J_DPAD_UP, + J_DPAD_DOWN, + J_DPAD_LEFT, + J_DPAD_RIGHT, + + J_BTN_MISC1, // Additional button (e.g. Xbox Series X share button, PS5 microphone button, Nintendo Switch Pro capture button, Amazon Luna microphone button) + J_BTN_RPADDLE1, // Upper or primary paddle, under your right hand (e.g. Xbox Elite paddle P1) + J_BTN_LPADDLE1, // Upper or primary paddle, under your left hand (e.g. Xbox Elite paddle P3) + J_BTN_RPADDLE2, // Lower or secondary paddle, under your right hand (e.g. Xbox Elite paddle P2) + J_BTN_LPADDLE2, // Lower or secondary paddle, under your left hand (e.g. Xbox Elite paddle P4) + + J_ACTION_MAX = J_BTN_LPADDLE2, + // leaving some space here for about 12 additional J_ACTIONs, if needed + + J_AXIS_MIN = 32, + J_AXIS_LEFT_X = J_AXIS_MIN + AXIS_LEFT_X, + J_AXIS_LEFT_Y = J_AXIS_MIN + AXIS_LEFT_Y, + J_AXIS_RIGHT_X = J_AXIS_MIN + AXIS_RIGHT_X, + J_AXIS_RIGHT_Y = J_AXIS_MIN + AXIS_RIGHT_Y, + J_AXIS_LEFT_TRIG = J_AXIS_MIN + AXIS_LEFT_TRIG, + J_AXIS_RIGHT_TRIG = J_AXIS_MIN + AXIS_RIGHT_TRIG, + + J_AXIS_MAX = J_AXIS_MIN + MAX_JOYSTICK_AXIS - 1, + + MAX_JOY_EVENT +} sys_jEvents; + struct sysEvent_t { sysEventType_t evType; - int evValue; - int evValue2; + int evValue; // for keys: K_* or ASCII code; for joystick: axis; for mouse: mouseX + int evValue2; // for keys: 0/1 for up/down; for axis: value; for mouse: mouseY int evPtrLength; // bytes of data pointed to by evPtr, for journaling void * evPtr; // this must be manually freed if not NULL }; @@ -101,6 +144,7 @@ void Sys_Quit( void ); // note that this isn't journaled... char * Sys_GetClipboardData( void ); +void Sys_FreeClipboardData( char* data ); void Sys_SetClipboardData( const char *string ); // will go to the various text consoles @@ -295,7 +339,7 @@ typedef int (*xthread_t)( void * ); typedef struct { const char *name; SDL_Thread *threadHandle; - unsigned int threadId; + unsigned long threadId; } xthreadInfo; void Sys_CreateThread( xthread_t function, void *parms, xthreadInfo &info, const char *name ); From 15b15d4c6823cb66270b5f20649da9df91aca31a Mon Sep 17 00:00:00 2001 From: Daniel Gibson Date: Fri, 19 Apr 2024 07:24:32 +0200 Subject: [PATCH 32/33] Don't use GCC's __builtin_alloca_with_align(), fix #572 turns out that __builtin_alloca_with_align() might releases the allocated memory at the end of the block it was allocated in, instead of the end of the function (which is the behavior of regular alloca() and __builtin_alloca()): "The lifetime of the allocated object ends at the end of the block in which the function was called. The allocated storage is released no later than just before the calling function returns to its caller, but may be released at the end of the block in which the function was called." https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html#index-_005f_005fbuiltin_005falloca_005fwith_005falign Clang also supports __builtin_alloca_with_align(), but always releases the memory at the end of the function. And it seems that newer GCC versions also tend to release it at the end of the function, but GCC 4.7.2 (that I use for the official Linux release binaries) didn't, and that caused weird graphical glitches. But as I don't want to rely on newer GCC versions behaving like this (and the documentation explicitly says that it *may* be released at the end of the block, but will definitely be released at the end of the function), I removed all usage of __builtin_alloca_with_align(). (Side-Note: GCC only started documenting the behavior of __builtin_alloca and __builtin_alloca_with_align at all with GCC 6.1.0) --- sys/platform.h | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/sys/platform.h b/sys/platform.h index 1beb68b..7e936ff 100644 --- a/sys/platform.h +++ b/sys/platform.h @@ -83,7 +83,8 @@ If you have questions concerning this license or the applicable additional terms #ifdef __MINGW32__ #undef _alloca // in mingw _alloca is a #define - #define _alloca16( x ) ( (assert((x)= 1.28 - #define _alloca16( x ) ( ({assert((x) Date: Wed, 8 Jan 2025 03:22:54 +0100 Subject: [PATCH 33/33] Update CMakeLists.txt to current dhewm3-sdk state the latest dhewm3-sdk commit that affected CMakeLists.txt that is integrated here is 94b42895270a03d0fa8841b10f171a0d6c80aee2 "CMake: Detect all variations of the clang compiler (hopefully)" --- CMakeLists.txt | 193 ++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 176 insertions(+), 17 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 20d210f..a12fb47 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1,4 +1,4 @@ -cmake_minimum_required(VERSION 2.8 FATAL_ERROR) +cmake_minimum_required(VERSION 2.8...3.22 FATAL_ERROR) project(dhewm3sdk) option(BASE "Build the base (game/) game code" ON) @@ -11,6 +11,11 @@ set(D3XP_DEFS "GAME_DLL;_D3XP;CTF" CACHE STRING "Compiler definitions for the mo option(ONATIVE "Optimize for the host CPU" OFF) +if(NOT MSVC) # GCC/clang or compatible, hopefully + option(FORCE_COLORED_OUTPUT "Always produce ANSI-colored compiler warnings/errors (GCC/Clang only; esp. useful with ninja)." OFF) + option(ASAN "Enable GCC/Clang Adress Sanitizer (ASan)" OFF) # TODO: MSVC might also support this, somehow? +endif() + set(src_game_mod # add additional .cpp files of your mod in game/ # (that you added to the ones already existant in the SDK/in dhewm3) @@ -63,14 +68,56 @@ endif() # target cpu set(cpu ${CMAKE_SYSTEM_PROCESSOR}) + +# Originally, ${CMAKE_SYSTEM_PROCESSOR} was supposed to contain the *target* CPU, according to CMake's documentation. +# As far as I can tell this has always been broken (always returns host CPU) at least on Windows +# (see e.g. https://cmake.org/pipermail/cmake-developers/2014-September/011405.html) and wasn't reliable on +# other systems either, for example on Linux with 32bit userland but 64bit kernel it returned the kernel CPU type +# (e.g. x86_64 instead of i686). Instead of fixing this, CMake eventually updated their documentation in 3.20, +# now it's officially the same as CMAKE_HOST_SYSTEM_PROCESSOR except when cross-compiling (where it's explicitly set) +# So we gotta figure out the actual target CPU type ourselves.. (why am I sticking to this garbage buildsystem?) +if(NOT (CMAKE_SYSTEM_PROCESSOR STREQUAL CMAKE_HOST_SYSTEM_PROCESSOR)) + # special case: cross-compiling, here CMAKE_SYSTEM_PROCESSOR should be correct, hopefully + # (just leave cpu at ${CMAKE_SYSTEM_PROCESSOR}) +elseif(MSVC) + # because all this wasn't ugly enough, it turned out that, unlike standalone CMake, Visual Studio's + # integrated CMake doesn't set CMAKE_GENERATOR_PLATFORM, so I gave up on guessing the CPU arch here + # and moved the CPU detection to MSVC-specific code in neo/sys/platform.h +else() # not MSVC and not cross-compiling, assume GCC or clang (-compatible), seems to work for MinGW as well + execute_process(COMMAND ${CMAKE_C_COMPILER} "-dumpmachine" + RESULT_VARIABLE cc_dumpmachine_res + OUTPUT_VARIABLE cc_dumpmachine_out) + if(cc_dumpmachine_res EQUAL 0) + string(STRIP ${cc_dumpmachine_out} cc_dumpmachine_out) # get rid of trailing newline + message(STATUS "`${CMAKE_C_COMPILER} -dumpmachine` says: \"${cc_dumpmachine_out}\"") + # gcc -dumpmachine and clang -dumpmachine seem to print something like "x86_64-linux-gnu" (gcc) + # or "x64_64-pc-linux-gnu" (clang) or "i686-w64-mingw32" (32bit mingw-w64) i.e. starting with the CPU, + # then "-" and then OS or whatever - so use everything up to first "-" + string(REGEX MATCH "^[^-]+" cpu ${cc_dumpmachine_out}) + message(STATUS " => CPU architecture extracted from that: \"${cpu}\"") + else() + message(WARNING "${CMAKE_C_COMPILER} -dumpmachine failed with error (code) ${cc_dumpmachine_res}") + message(WARNING "will use the (sometimes incorrect) CMAKE_SYSTEM_PROCESSOR (${cpu}) to determine D3_ARCH") + endif() +endif() + if(cpu STREQUAL "powerpc") set(cpu "ppc") +elseif(cpu STREQUAL "aarch64") + # "arm64" is more obvious, and some operating systems (like macOS) use it instead of "aarch64" + set(cpu "arm64") elseif(cpu MATCHES "i.86") set(cpu "x86") -endif() - -if(MSVC AND CMAKE_CL_64) - set(cpu "amd64") +elseif(cpu MATCHES "[aA][mM][dD]64" OR cpu MATCHES "[xX]64") + set(cpu "x86_64") +elseif(cpu MATCHES "[aA][rR][mM].*") # some kind of arm.. + # On 32bit Raspbian gcc -dumpmachine returns sth starting with "arm-", + # while clang -dumpmachine says "arm6k-..." - try to unify that to "arm" + if(CMAKE_SIZEOF_VOID_P EQUAL 8) # sizeof(void*) == 8 => must be arm64 + set(cpu "arm64") + else() # should be 32bit arm then (probably "armv7l" "armv6k" or sth like that) + set(cpu "arm") + endif() endif() # target os @@ -80,6 +127,24 @@ else() string(TOLOWER "${CMAKE_SYSTEM_NAME}" os) endif() +add_definitions(-DD3_OSTYPE="${os}" -DD3_SIZEOFPTR=${CMAKE_SIZEOF_VOID_P}) + +if(MSVC) + # for MSVC D3_ARCH is set in code (in neo/sys/platform.h) + message(STATUS "Setting -DD3_SIZEOFPTR=${CMAKE_SIZEOF_VOID_P} -DD3_OSTYPE=\"${os}\" - NOT setting D3_ARCH, because we're targeting MSVC (VisualC++)") + # make sure ${cpu} isn't (cant't be) used by CMake when building with MSVC + unset(cpu) +else() + add_definitions(-DD3_ARCH="${cpu}") + message(STATUS "Setting -DD3_ARCH=\"${cpu}\" -DD3_SIZEOFPTR=${CMAKE_SIZEOF_VOID_P} -DD3_OSTYPE=\"${os}\" ") + + if(cpu MATCHES ".*64.*" AND NOT CMAKE_SIZEOF_VOID_P EQUAL 8) + # tough luck if some CPU architecture has "64" in its name but uses 32bit pointers + message(SEND_ERROR "CMake thinks sizeof(void*) == 4, but the target CPU ${cpu} looks like a 64bit CPU!") + message(FATAL_ERROR "If you're building in a 32bit chroot on a 64bit host, switch to it with 'linux32 chroot' or at least call cmake with linux32 (or your OSs equivalent)!") + endif() +endif() + # build type if(NOT CMAKE_BUILD_TYPE) set(CMAKE_BUILD_TYPE "RelWithDebInfo") @@ -89,8 +154,25 @@ endif() include(CheckCXXCompilerFlag) include(TestBigEndian) +set(D3_COMPILER_IS_CLANG FALSE) +set(D3_COMPILER_IS_GCC_OR_CLANG FALSE) + +if(NOT MSVC) + # check if this is some kind of clang (Clang, AppleClang, whatever) + # (convert compiler ID to lowercase so we match Clang, clang, AppleClang etc, regardless of case) + string(TOLOWER ${CMAKE_CXX_COMPILER_ID} compiler_id_lower) + if(compiler_id_lower MATCHES ".*clang.*") + message(STATUS "Compiler \"${CMAKE_CXX_COMPILER_ID}\" detected as some kind of clang") + set(D3_COMPILER_IS_CLANG TRUE) + set(D3_COMPILER_IS_GCC_OR_CLANG TRUE) + elseif(CMAKE_COMPILER_IS_GNUCC) + set(D3_COMPILER_IS_GCC_OR_CLANG TRUE) + endif() + unset(compiler_id_lower) +endif() # NOT MSVC + # compiler specific flags -if(CMAKE_COMPILER_IS_GNUCC OR CMAKE_C_COMPILER_ID STREQUAL "Clang") +if(D3_COMPILER_IS_GCC_OR_CLANG) add_compile_options(-pipe) add_compile_options(-Wall) @@ -100,17 +182,35 @@ if(CMAKE_COMPILER_IS_GNUCC OR CMAKE_C_COMPILER_ID STREQUAL "Clang") add_compile_options(-march=pentium3) endif() - set(CMAKE_C_FLAGS_DEBUG "-g -D_DEBUG -O1") + if(FORCE_COLORED_OUTPUT) + if(CMAKE_COMPILER_IS_GNUCC) + add_compile_options (-fdiagnostics-color=always) + elseif (D3_COMPILER_IS_CLANG) + add_compile_options (-fcolor-diagnostics) + endif () + endif () + + set(CMAKE_C_FLAGS_DEBUG "-g -D_DEBUG -O0") set(CMAKE_C_FLAGS_DEBUGALL "-g -ggdb -D_DEBUG") set(CMAKE_C_FLAGS_PROFILE "-g -ggdb -D_DEBUG -O1 -fno-omit-frame-pointer") - set(CMAKE_C_FLAGS_RELEASE "-O2 -fno-unsafe-math-optimizations -fno-math-errno -fno-trapping-math -fomit-frame-pointer") - set(CMAKE_C_FLAGS_RELWITHDEBINFO "-O2 -g -ggdb -fno-unsafe-math-optimizations -fno-math-errno -fno-trapping-math -fno-omit-frame-pointer") - set(CMAKE_C_FLAGS_MINSIZEREL "-Os -fno-unsafe-math-optimizations -fno-math-errno -fno-trapping-math -fomit-frame-pointer") + set(CMAKE_C_FLAGS_RELEASE "-O2 -fno-math-errno -fno-trapping-math -fomit-frame-pointer") + set(CMAKE_C_FLAGS_RELWITHDEBINFO "-O2 -g -ggdb -fno-math-errno -fno-trapping-math -fno-omit-frame-pointer") + set(CMAKE_C_FLAGS_MINSIZEREL "-Os -fno-math-errno -fno-trapping-math -fomit-frame-pointer") set(CMAKE_CXX_FLAGS_DEBUGALL ${CMAKE_C_FLAGS_DEBUGALL}) set(CMAKE_CXX_FLAGS_PROFILE ${CMAKE_C_FLAGS_PROFILE}) add_compile_options(-fno-strict-aliasing) + # dear idiot compilers, don't fuck up math code with useless FMA "optimizations" + # (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=100839) + add_compile_options(-ffp-contract=off) + + if(ASAN) + # if this doesn't work, ASan might not be available on your platform, don't set ASAN then.. + add_compile_options(-fsanitize=address) + # TODO: do we need to link against libasan or sth? or is it enough if dhewm3 executable does? + # set(ldflags ${ldflags} -fsanitize=address) + endif() if(NOT AROS) CHECK_CXX_COMPILER_FLAG("-fvisibility=hidden" cxx_has_fvisibility) @@ -124,7 +224,14 @@ if(CMAKE_COMPILER_IS_GNUCC OR CMAKE_C_COMPILER_ID STREQUAL "Clang") add_compile_options(-Wno-sign-compare) add_compile_options(-Wno-switch) add_compile_options(-Wno-strict-overflow) - add_compile_options(-Wno-format-security) + + if(CMAKE_COMPILER_IS_GNUCC AND CMAKE_CXX_COMPILER_VERSION VERSION_GREATER_EQUAL 8.0) + # ‘void* memset(void*, int, size_t)’ clearing an object of type ‘struct refSound_t’ with no trivial copy-assignment; use assignment or value-initialization instead + # ‘void* memcpy(void*, const void*, size_t)’ writing to an object of type ‘class idDrawVert’ with no trivial copy-assignment; use copy-assignment or copy-initialization instead + # etc - I don't care, all the cases I've checked were safe and I'm not gonna add (void*) casts in 1000 places + # (that would make merging new mods painful) + add_compile_options(-Wno-class-memaccess) + endif() CHECK_CXX_COMPILER_FLAG("-Woverloaded-virtual" cxx_has_Woverload_virtual) if(cxx_has_Woverload_virtual) @@ -140,6 +247,9 @@ if(CMAKE_COMPILER_IS_GNUCC OR CMAKE_C_COMPILER_ID STREQUAL "Clang") if(cpu STREQUAL "x86_64") add_compile_options(-arch x86_64 -mmacosx-version-min=10.6) set(ldflags "${ldflags} -arch x86_64 -mmacosx-version-min=10.6") + elseif(cpu STREQUAL "arm64") + add_compile_options(-arch arm64 -mmacosx-version-min=11.0) + set(ldflags "${ldflags} -arch arm64 -mmacosx-version-min=11.0") elseif(cpu STREQUAL "x86") CHECK_CXX_COMPILER_FLAG("-arch i386" cxx_has_arch_i386) if(cxx_has_arch_i386) @@ -152,14 +262,14 @@ if(CMAKE_COMPILER_IS_GNUCC OR CMAKE_C_COMPILER_ID STREQUAL "Clang") elseif(cpu STREQUAL "ppc") CHECK_CXX_COMPILER_FLAG("-arch ppc" cxx_has_arch_ppc) if(cxx_has_arch_ppc) - add_compile_options(-arch ppc) - set(ldflags "${ldflags} -arch ppc") + add_compile_options(-arch ppc -mone-byte-bool) + set(ldflags "${ldflags} -arch ppc -mone-byte-bool") endif() add_compile_options(-mmacosx-version-min=10.4) set(ldflags "${ldflags} -mmacosx-version-min=10.4") else() - message(FATAL_ERROR "Unsupported CPU architecture ${cpu} for OSX") + message(FATAL_ERROR "Unsupported CPU architecture for OSX") endif() elseif(WIN32) set(ldflags "${ldflags} -static-libgcc -static-libstdc++") @@ -167,6 +277,8 @@ if(CMAKE_COMPILER_IS_GNUCC OR CMAKE_C_COMPILER_ID STREQUAL "Clang") # set(sys_libs ${sys_libs} dl) FIXME: -ldl needed anyway? endif() elseif(MSVC) + add_definitions(/MP) # parallel build (use all cores, or as many as configured in VS) + add_compile_options(/W4) add_compile_options(/we4840) # treat as error when passing a class to a vararg-function (probably printf-like) # treat several kinds of truncating int<->pointer conversions as errors (for more 64bit-safety) @@ -211,7 +323,9 @@ configure_file( "${CMAKE_BINARY_DIR}/config.h" ) +if(NOT MSVC) message(STATUS "Building ${CMAKE_BUILD_TYPE} for ${os}-${cpu}") +endif() if(NOT APPLE AND NOT WIN32) message(STATUS "The install target will use the following directories:") @@ -220,6 +334,27 @@ if(NOT APPLE AND NOT WIN32) message(STATUS " Data directory: ${datadir}") endif() +# I'm a bit sloppy with headers and just glob them in.. +# they're only handled in CMake at all so they turn up in Visual Studio solutions.. + +# globs all the headers from ${PATHPREFIX}/ and adds them to ${SRCLIST} +function(add_globbed_headers SRCLIST PATHPREFIX) + file(GLOB_RECURSE tmp_hdrs RELATIVE "${CMAKE_SOURCE_DIR}" "${PATHPREFIX}/*.h") + set(${SRCLIST} ${tmp_hdrs} ${${SRCLIST}} PARENT_SCOPE) +endfunction() + +if(CMAKE_MAJOR_VERSION LESS 3 OR ( CMAKE_MAJOR_VERSION EQUAL 3 AND CMAKE_MINOR_VERSION LESS 8 )) + # cmake < 3.8 doesn't support source_group(TREE ...) so replace it with a dummy + # (it's only cosmetical anyway, to make source files show up properly in Visual Studio) + function(source_group) + endfunction() + message(STATUS "Using CMake < 3.8, doesn't support source_group(TREE ...), replacing it with a dummy") + message(STATUS " (this is only relevants for IDEs, doesn't matter for just compiling dhewm3)") +#else() +# message(STATUS "Using CMake >= 3.8, supports source_group(TREE ...)") +endif() + + set(src_game game/AF.cpp game/AFEntity.cpp @@ -294,6 +429,8 @@ set(src_game ${src_game_mod} ) +add_globbed_headers(src_game "game") + set(src_d3xp d3xp/AF.cpp d3xp/AFEntity.cpp @@ -370,6 +507,8 @@ set(src_d3xp ${src_d3xp_mod} ) +add_globbed_headers(src_d3xp "d3xp") + set(src_idlib idlib/bv/Bounds.cpp idlib/bv/Frustum.cpp @@ -421,6 +560,18 @@ set(src_idlib idlib/Heap.cpp ) +add_globbed_headers(src_idlib "idlib") +# just add all the remaining headers (that have no corresponding .cpp in the SDK) +# to idlib so they can be navigated there +add_globbed_headers(src_idlib "cm") +add_globbed_headers(src_idlib "framework") +add_globbed_headers(src_idlib "renderer") +add_globbed_headers(src_idlib "sound") +add_globbed_headers(src_idlib "sys") +add_globbed_headers(src_idlib "tools") +add_globbed_headers(src_idlib "ui") + + include_directories(${CMAKE_BINARY_DIR}) include_directories(${CMAKE_SOURCE_DIR}) @@ -433,11 +584,13 @@ if (AROS) set(AROS_ARCH ${CMAKE_SYSTEM_PROCESSOR}) endif() else() - if(CMAKE_COMPILER_IS_GNUCC OR CMAKE_C_COMPILER_ID STREQUAL "Clang" AND NOT MINGW) + if(D3_COMPILER_IS_GCC_OR_CLANG AND NOT MINGW) set_target_properties(idlib PROPERTIES COMPILE_FLAGS "-fPIC") endif() endif() +source_group(TREE ${CMAKE_CURRENT_SOURCE_DIR} PREFIX neo FILES ${src_idlib}) + if(BASE) if (AROS) add_executable(base sys/aros/dll/dllglue.c ${src_game}) @@ -447,9 +600,12 @@ if(BASE) # so mods can create cdoom.dll instead of base.dll from the code in game/ set_target_properties(base PROPERTIES OUTPUT_NAME "${BASE_NAME}") endif() + + source_group(TREE ${CMAKE_CURRENT_SOURCE_DIR} PREFIX neo FILES ${src_game}) + set_target_properties(base PROPERTIES PREFIX "") set_target_properties(base PROPERTIES COMPILE_DEFINITIONS "${BASE_DEFS}") - set_target_properties(base PROPERTIES COMPILE_FLAGS "-I${CMAKE_SOURCE_DIR}/game") + target_include_directories(base PRIVATE "${CMAKE_SOURCE_DIR}/game") set_target_properties(base PROPERTIES LINK_FLAGS "${ldflags}") set_target_properties(base PROPERTIES INSTALL_NAME_DIR "@executable_path") if (AROS) @@ -476,9 +632,12 @@ if(D3XP) # so mods can create whatever.dll instead of d3xp.dll from the code in d3xp/ set_target_properties(d3xp PROPERTIES OUTPUT_NAME "${D3XP_NAME}") endif() + + source_group(TREE ${CMAKE_CURRENT_SOURCE_DIR} PREFIX neo FILES ${src_d3xp}) + set_target_properties(d3xp PROPERTIES PREFIX "") set_target_properties(d3xp PROPERTIES COMPILE_DEFINITIONS "${D3XP_DEFS}") - set_target_properties(d3xp PROPERTIES COMPILE_FLAGS "-I${CMAKE_SOURCE_DIR}/d3xp") + target_include_directories(d3xp PRIVATE "${CMAKE_SOURCE_DIR}/d3xp") set_target_properties(d3xp PROPERTIES LINK_FLAGS "${ldflags}") set_target_properties(d3xp PROPERTIES INSTALL_NAME_DIR "@executable_path") if (AROS)