Fix savegame-compatibility of scripts, increase BUILD_NUMBER

"Fix "t->c->value.argSize == func->parmTotal" Assertion in Scripts, #303"
had broken old savegames because the script checksum
(idProgram::CalculateChecksum()) changed, see #344.
This is fixed now, also the BUILD_NUMBER is increased so old savegames
can be identified for a workaround.

Don't use this commit without the next one which will further modify the
savegame format (for the new BUILD_NUMBER 1305)

d3xp/script/Script_Compiler.cpp

@@ -987,6 +987,25 @@ idVarDef *idCompiler::EmitFunctionParms( int op, idVarDef *func, int startarg, i
 		// need arg size seperate since script object may be NULL
 		statement_t &statement = gameLocal.program.GetStatement( gameLocal.program.NumStatements() - 1 );
 		statement.c = SizeConstant( func->value.functionPtr->parmTotal );
+		// DG: before changes I did to ParseFunctionDef(), func->value.functionPtr->parmTotal was 0
+		//     if the function declaration/prototype has been parsed already, but the
+		//     definition/implementation hadn't been parsed yet. That was wrong and sometimes
+		//     (with debug game DLLs) lead to assertions in custom scripts, because the
+		//     stack space reserved for function parameters was wrong.
+		//     Now func->value.functionPtr->parmTotal is calculated when parsing the prototype,
+		//     but func->value.functionPtr->parmSize[i] is still only calculated when parsing
+		//     the implementation (as it's not needed before and so we can tell the cases apart here).
+		//     However, savegames from before the change have script checksums
+		//     (by idProgram::CalculateChecksum()) from statements with the wrong size, so
+		//     loading them would fail as the checksum doesn't match.
+		//     Setting this flag allows using the parmTotal argSize 0 when calculating the checksum
+		//     so it matches the one from old savegames (unless something else has also changed in
+		//     the script state so they really are incompatible). That's only done when actually
+		//     loading old savegames (detected via BUILD_NUMBER/savegame.GetBuildNumber())
+		if ( op == OP_OBJECTCALL && func->value.functionPtr->parmTotal > 0
+		     && func->value.functionPtr->parmSize.Num() == 0 ) {
+			statement.flags = statement_t::FLAG_OBJECTCALL_IMPL_NOT_PARSED_YET;
+		}
 	} else {
 		EmitOpcode( op, func, SizeConstant( size ) );
 	}
@@ -2144,46 +2163,57 @@ void idCompiler::ParseFunctionDef( idTypeDef *returnType, const char *name ) {
 		}
 	}
 
-	// DG: make sure parmSize gets calculated when parsing prototype (not just when parsing
+	// DG: make sure parmTotal gets calculated when parsing prototype (not just when parsing
-	//     implementation) so calling this function/method before implementation has been parsed
+	//     implementation) so calling this function/method before the implementation has been parsed
 	//     works without getting Assertions in IdInterpreter::Execute() and ::LeaveFunction()
-	//     ("st->c->value.argSize == func->parmTotal", "localstackUsed == localstackBase", see #303)
+	//     ("st->c->value.argSize == func->parmTotal", "localstackUsed == localstackBase", see #303 and #344)
 
 	// calculate stack space used by parms
 	numParms = type->NumParameters();
-	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 ];
-		}
-
-		// 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 );
-		}
-	}
-
-	// 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( ";" );
+		// DG: BUT only after calculating the stack space for the arguments because this
+		// function might be called before the implementation is parsed (see #303 and #344)
+		// which otherwise causes Assertions in IdInterpreter::Execute() and ::LeaveFunction()
+		// ("st->c->value.argSize == func->parmTotal", "localstackUsed == localstackBase")
+		func->parmTotal = 0;
+		for( i = 0; i < numParms; i++ ) {
+			parmType = type->GetParmType( i );
+			int size = parmType->Inherits( &type_object ) ? type_object.Size() : parmType->Size();
+			func->parmTotal += size;
+			// NOTE: Don't set func->parmSize[] yet, the workaround to keep compatibility
+			//       with old savegames checks for func->parmSize.Num() == 0
+			//       (see EmitFunctionParms() for more explanation of that workaround)
+			// Also not defining the parms yet, otherwise they're defined in a different order
+			// than before, so their .num is different which breaks compat with old savegames
+		}
 		return;
 	}
-	// DG end
+
+
+	int totalSize = 0; // DG: totalsize might already have been calculated for the prototype, see a few lines above
+	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();
+		}
+		totalSize += func->parmSize[ i ];
+	}
+	// DG: if parmTotal has been calculated before, it shouldn't have changed
+	assert((func->parmTotal == 0 || totalSize == func->parmTotal) && "function parameter sizes differ between protype vs implementation?!");
+	func->parmTotal = totalSize;
+
+	// 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 );
+	}
 
 	oldscope = scope;
 	scope = def;

d3xp/script/Script_Program.cpp

@@ -1634,7 +1634,9 @@ statement_t *idProgram::AllocStatement( void ) {
 	if ( statements.Num() >= statements.Max() ) {
 		throw idCompileError( va( "Exceeded maximum allowed number of statements (%d)", statements.Max() ) );
 	}
-	return statements.Alloc();
+	statement_t* ret = statements.Alloc();
+	ret->flags = 0; // DG: initialize the added flags (that are rarely set/used otherwise) to 0
+	return ret;
 }
 
 /*
@@ -2005,7 +2007,7 @@ void idProgram::Save( idSaveGame *savefile ) const {
 		savefile->WriteByte( variables[i] );
 	}
 
-	int checksum = CalculateChecksum();
+	int checksum = CalculateChecksum(false);
 	savefile->WriteInt( checksum );
 }
 
@@ -2039,9 +2041,11 @@ bool idProgram::Restore( idRestoreGame *savefile ) {
 	int saved_checksum, checksum;
 
 	savefile->ReadInt( saved_checksum );
-	checksum = CalculateChecksum();
+	bool isOldSavegame = savefile->GetBuildNumber() <= 1304;
+	checksum = CalculateChecksum(isOldSavegame);
 
 	if ( saved_checksum != checksum ) {
+		gameLocal.Warning( "WARNING: Real Script checksum didn't match the one from the savegame!");
 		result = false;
 	}
 
@@ -2053,7 +2057,7 @@ bool idProgram::Restore( idRestoreGame *savefile ) {
 idProgram::CalculateChecksum
 ================
 */
-int idProgram::CalculateChecksum( void ) const {
+int idProgram::CalculateChecksum( bool forOldSavegame ) const {
 	int i, result;
 
 	typedef struct {
@@ -2069,6 +2073,17 @@ int idProgram::CalculateChecksum( void ) const {
 
 	memset( statementList, 0, ( sizeof(statementBlock_t) * statements.Num() ) );
 
+	// DG hack: get the vardef for the argSize == 0 constant for savegame-compat
+	int constantZeroNum = -1;
+	if ( forOldSavegame ) {
+		for( idVarDef* def = GetDefList( "<IMMEDIATE>" ); def != NULL; def = def->Next() ) {
+			if ( def->Type() == ev_argsize && def->value.argSize == 0 ) {
+				constantZeroNum = def->num;
+				break;
+			}
+		}
+	}
+
 	// Copy info into new list, using the variable numbers instead of a pointer to the variable
 	for( i = 0; i < statements.Num(); i++ ) {
 		statementList[i].op = statements[i].op;
@@ -2084,7 +2099,15 @@ int idProgram::CalculateChecksum( void ) const {
 			statementList[i].b = -1;
 		}
 		if ( statements[i].c ) {
-			statementList[i].c = statements[i].c->num;
+			// DG: old savegames wrongly assumed argSize 0 for some statements.
+			//     So for the checksums to match we need to use the corresponding vardef num here
+			//     See idCompiler::EmitFunctionParms() and ParseFunctionDef() for more details.
+			if ( forOldSavegame && statements[i].op == OP_OBJECTCALL
+			     && statements[i].flags == statement_t::FLAG_OBJECTCALL_IMPL_NOT_PARSED_YET ) {
+				statementList[i].c = constantZeroNum;
+			} else {
+				statementList[i].c = statements[i].c->num;
+			}
 		} else {
 			statementList[i].c = -1;
 		}

d3xp/script/Script_Program.h

@@ -331,7 +331,7 @@ class idVarDef {
 	friend class idVarDefName;
 
 public:
-	int						num;
+	int						num;			// global index/ID of variable
 	varEval_t				value;
 	idVarDef *				scope;			// function, namespace, or object the var was defined in
 	int						numUsers;		// number of users if this is a constant
@@ -432,11 +432,19 @@ extern	idVarDef	def_boolean;
 
 typedef struct statement_s {
 	unsigned short	op;
+	unsigned short	flags; // DG: added this for ugly hacks
+	enum {
+		// op is OP_OBJECTCALL and when the statement was created the function/method
+		// implementation hasn't been parsed yet (only the declaration/prototype)
+		// see idCompiler::EmitFunctionParms() and idProgram::CalculateChecksum()
+		FLAG_OBJECTCALL_IMPL_NOT_PARSED_YET = 1,
+	};
+	// DG: moved linenumber and file up here to prevent wasting 8 bytes of padding on 64bit
+	unsigned short	linenumber;
+	unsigned short	file;
 	idVarDef		*a;
 	idVarDef		*b;
 	idVarDef		*c;
-	unsigned short	linenumber;
-	unsigned short	file;
 } statement_t;
 
 /***********************************************************************
@@ -488,7 +496,7 @@ public:
 	// save games
 	void										Save( idSaveGame *savefile ) const;
 	bool										Restore( idRestoreGame *savefile );
-	int											CalculateChecksum( void ) const;		// Used to insure program code has not
+	int											CalculateChecksum( bool forOldSavegame ) const;		// Used to insure program code has not
 																						//    changed between savegames
 
 	void										Startup( const char *defaultScript );

framework/BuildVersion.h

@@ -25,4 +25,4 @@ If you have questions concerning this license or the applicable additional terms
 
 ===========================================================================
 */
-const int BUILD_NUMBER = 1304;
+const int BUILD_NUMBER = 1305;

game/Game_local.cpp

@@ -427,6 +427,10 @@ void idGameLocal::SaveGame( idFile *f ) {
 
 	savegame.WriteBuildNumber( BUILD_NUMBER );
 
+	// DG: TODO: write other things (maybe internal savegame version so I don't have to bump BUILD_NUMBER AGAIN)
+	//           and OS, architecture etc, see #344
+	// TODO: adjust InitFromSavegame() accordingly!
+
 	// go through all entities and threads and add them to the object list
 	for( i = 0; i < MAX_GENTITIES; i++ ) {
 		ent = entities[i];
@@ -1220,6 +1224,12 @@ bool idGameLocal::InitFromSaveGame( const char *mapName, idRenderWorld *renderWo
 
 	savegame.ReadBuildNumber();
 
+
+	if(savegame.GetBuildNumber() >= 1305)
+	{
+		// TODO: read stuff additionally written in SaveGame()
+	}
+
 	// Create the list of all objects in the game
 	savegame.CreateObjects();
 

game/script/Script_Compiler.cpp

@@ -987,6 +987,25 @@ idVarDef *idCompiler::EmitFunctionParms( int op, idVarDef *func, int startarg, i
 		// need arg size seperate since script object may be NULL
 		statement_t &statement = gameLocal.program.GetStatement( gameLocal.program.NumStatements() - 1 );
 		statement.c = SizeConstant( func->value.functionPtr->parmTotal );
+		// DG: before changes I did to ParseFunctionDef(), func->value.functionPtr->parmTotal was 0
+		//     if the function declaration/prototype has been parsed already, but the
+		//     definition/implementation hadn't been parsed yet. That was wrong and sometimes
+		//     (with debug game DLLs) lead to assertions in custom scripts, because the
+		//     stack space reserved for function parameters was wrong.
+		//     Now func->value.functionPtr->parmTotal is calculated when parsing the prototype,
+		//     but func->value.functionPtr->parmSize[i] is still only calculated when parsing
+		//     the implementation (as it's not needed before and so we can tell the cases apart here).
+		//     However, savegames from before the change have script checksums
+		//     (by idProgram::CalculateChecksum()) from statements with the wrong size, so
+		//     loading them would fail as the checksum doesn't match.
+		//     Setting this flag allows using the parmTotal argSize 0 when calculating the checksum
+		//     so it matches the one from old savegames (unless something else has also changed in
+		//     the script state so they really are incompatible). That's only done when actually
+		//     loading old savegames (detected via BUILD_NUMBER/savegame.GetBuildNumber())
+		if ( op == OP_OBJECTCALL && func->value.functionPtr->parmTotal > 0
+		     && func->value.functionPtr->parmSize.Num() == 0 ) {
+			statement.flags = statement_t::FLAG_OBJECTCALL_IMPL_NOT_PARSED_YET;
+		}
 	} else {
 		EmitOpcode( op, func, SizeConstant( size ) );
 	}
@@ -2144,46 +2163,57 @@ void idCompiler::ParseFunctionDef( idTypeDef *returnType, const char *name ) {
 		}
 	}
 
-	// DG: make sure parmSize gets calculated when parsing prototype (not just when parsing
+	// DG: make sure parmTotal gets calculated when parsing prototype (not just when parsing
-	//     implementation) so calling this function/method before implementation has been parsed
+	//     implementation) so calling this function/method before the implementation has been parsed
 	//     works without getting Assertions in IdInterpreter::Execute() and ::LeaveFunction()
-	//     ("st->c->value.argSize == func->parmTotal", "localstackUsed == localstackBase", see #303)
+	//     ("st->c->value.argSize == func->parmTotal", "localstackUsed == localstackBase", see #303 and #344)
 
 	// calculate stack space used by parms
 	numParms = type->NumParameters();
-	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 ];
-		}
-
-		// 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 );
-		}
-	}
-
-	// 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( ";" );
+		// DG: BUT only after calculating the stack space for the arguments because this
+		// function might be called before the implementation is parsed (see #303 and #344)
+		// which otherwise causes Assertions in IdInterpreter::Execute() and ::LeaveFunction()
+		// ("st->c->value.argSize == func->parmTotal", "localstackUsed == localstackBase")
+		func->parmTotal = 0;
+		for( i = 0; i < numParms; i++ ) {
+			parmType = type->GetParmType( i );
+			int size = parmType->Inherits( &type_object ) ? type_object.Size() : parmType->Size();
+			func->parmTotal += size;
+			// NOTE: Don't set func->parmSize[] yet, the workaround to keep compatibility
+			//       with old savegames checks for func->parmSize.Num() == 0
+			//       (see EmitFunctionParms() for more explanation of that workaround)
+			// Also not defining the parms yet, otherwise they're defined in a different order
+			// than before, so their .num is different which breaks compat with old savegames
+		}
 		return;
 	}
-	// DG end
+
+
+	int totalSize = 0; // DG: totalsize might already have been calculated for the prototype, see a few lines above
+	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();
+		}
+		totalSize += func->parmSize[ i ];
+	}
+	// DG: if parmTotal has been calculated before, it shouldn't have changed
+	assert((func->parmTotal == 0 || totalSize == func->parmTotal) && "function parameter sizes differ between protype vs implementation?!");
+	func->parmTotal = totalSize;
+
+	// 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 );
+	}
 
 	oldscope = scope;
 	scope = def;

game/script/Script_Program.cpp

@@ -1634,7 +1634,9 @@ statement_t *idProgram::AllocStatement( void ) {
 	if ( statements.Num() >= statements.Max() ) {
 		throw idCompileError( va( "Exceeded maximum allowed number of statements (%d)", statements.Max() ) );
 	}
-	return statements.Alloc();
+	statement_t* ret = statements.Alloc();
+	ret->flags = 0; // DG: initialize the added flags (that are rarely set/used otherwise) to 0
+	return ret;
 }
 
 /*
@@ -2005,7 +2007,7 @@ void idProgram::Save( idSaveGame *savefile ) const {
 		savefile->WriteByte( variables[i] );
 	}
 
-	int checksum = CalculateChecksum();
+	int checksum = CalculateChecksum(false);
 	savefile->WriteInt( checksum );
 }
 
@@ -2039,9 +2041,11 @@ bool idProgram::Restore( idRestoreGame *savefile ) {
 	int saved_checksum, checksum;
 
 	savefile->ReadInt( saved_checksum );
-	checksum = CalculateChecksum();
+	bool isOldSavegame = savefile->GetBuildNumber() <= 1304;
+	checksum = CalculateChecksum(isOldSavegame);
 
 	if ( saved_checksum != checksum ) {
+		gameLocal.Warning( "WARNING: Real Script checksum didn't match the one from the savegame!");
 		result = false;
 	}
 
@@ -2053,7 +2057,7 @@ bool idProgram::Restore( idRestoreGame *savefile ) {
 idProgram::CalculateChecksum
 ================
 */
-int idProgram::CalculateChecksum( void ) const {
+int idProgram::CalculateChecksum( bool forOldSavegame ) const {
 	int i, result;
 
 	typedef struct {
@@ -2069,6 +2073,17 @@ int idProgram::CalculateChecksum( void ) const {
 
 	memset( statementList, 0, ( sizeof(statementBlock_t) * statements.Num() ) );
 
+	// DG hack: get the vardef for the argSize == 0 constant for savegame-compat
+	int constantZeroNum = -1;
+	if ( forOldSavegame ) {
+		for( idVarDef* def = GetDefList( "<IMMEDIATE>" ); def != NULL; def = def->Next() ) {
+			if ( def->Type() == ev_argsize && def->value.argSize == 0 ) {
+				constantZeroNum = def->num;
+				break;
+			}
+		}
+	}
+
 	// Copy info into new list, using the variable numbers instead of a pointer to the variable
 	for( i = 0; i < statements.Num(); i++ ) {
 		statementList[i].op = statements[i].op;
@@ -2084,7 +2099,15 @@ int idProgram::CalculateChecksum( void ) const {
 			statementList[i].b = -1;
 		}
 		if ( statements[i].c ) {
-			statementList[i].c = statements[i].c->num;
+			// DG: old savegames wrongly assumed argSize 0 for some statements.
+			//     So for the checksums to match we need to use the corresponding vardef num here
+			//     See idCompiler::EmitFunctionParms() and ParseFunctionDef() for more details.
+			if ( forOldSavegame && statements[i].op == OP_OBJECTCALL
+			     && statements[i].flags == statement_t::FLAG_OBJECTCALL_IMPL_NOT_PARSED_YET ) {
+				statementList[i].c = constantZeroNum;
+			} else {
+				statementList[i].c = statements[i].c->num;
+			}
 		} else {
 			statementList[i].c = -1;
 		}

game/script/Script_Program.h

@@ -317,7 +317,7 @@ class idVarDef {
 	friend class idVarDefName;
 
 public:
-	int						num;
+	int						num;			// global index/ID of variable
 	varEval_t				value;
 	idVarDef *				scope;			// function, namespace, or object the var was defined in
 	int						numUsers;		// number of users if this is a constant
@@ -418,11 +418,19 @@ extern	idVarDef	def_boolean;
 
 typedef struct statement_s {
 	unsigned short	op;
+	unsigned short	flags; // DG: added this for ugly hacks
+	enum {
+		// op is OP_OBJECTCALL and when the statement was created the function/method
+		// implementation hasn't been parsed yet (only the declaration/prototype)
+		// see idCompiler::EmitFunctionParms() and idProgram::CalculateChecksum()
+		FLAG_OBJECTCALL_IMPL_NOT_PARSED_YET = 1,
+	};
+	// DG: moved linenumber and file up here to prevent wasting 8 bytes of padding on 64bit
+	unsigned short	linenumber;
+	unsigned short	file;
 	idVarDef		*a;
 	idVarDef		*b;
 	idVarDef		*c;
-	unsigned short	linenumber;
-	unsigned short	file;
 } statement_t;
 
 /***********************************************************************
@@ -474,7 +482,7 @@ public:
 	// save games
 	void										Save( idSaveGame *savefile ) const;
 	bool										Restore( idRestoreGame *savefile );
-	int											CalculateChecksum( void ) const;		// Used to insure program code has not
+	int											CalculateChecksum( bool forOldSavegame ) const;		// Used to insure program code has not
 																						//    changed between savegames
 
 	void										Startup( const char *defaultScript );