Assertion "st->c->value.argSize == func->parmTotal" from some Scripts

[DanielGibson]

I ran into an assertion in the script interpreter when testing a dhewm3 mod in a Debug build: "st->c->value.argSize == func->parmTotal" from idInterpreter::Execute().
When ignoring or commenting out that assertion, another one in idInterpreter::LeaveFunction() triggers: "localstackUsed == localstackBase".

It turned out that the problem was the following:
We have a player "class" like:

object player : player_base {
	// .. lots of stuff not important for the issue
	void		init();

	float		music_volume;

	void		check_music_volume();
	// ... and more details
};

and then below there's implementations for those methods - and init() creates a thread with check_music_volume(), but the check_music_volume() implementation is below the init() implementation!

void player::init() {
	// .. whatever ..
	thread check_music_volume();
	// ...
}

// ...

void player::check_music_volume() {
	// ... does some stuff in an endless loop ...
	while(1) {
		if(music_volume > 0) {
			// .. do whatever ..
		}
	}
}

Now when I start a map and that code gets executed (player::init() is called when the player spawns), I get the aforementioned assertion (if assertions are enabled, like in debug builds), because the opcode pretends that the function call has no arguments at all (that need to be passed when calling), while in reality the "self" pointer to the player object is implicitly passed (so check_music_volume() knows what player object it belongs to and can access its fields, like music_volume).

This can be "fixed" by moving
void player::check_music_volume() { ... }
above
void player::init() { ... }

According to http://idtechforums.fuzzylogicinc.com/index.php?topic=897.0 this is a known problem in Doom3; it has also been mentioned in #204 (incl. the workaround of reordering); weirdly it doesn't seem to be documented anywhere else.
#265 (comment) also mentions this problem, with the Yet another flashlight Mod (which is script-only).

What is going wrong in the C++ Code

Now the problem was that the script compiler, when generating the ops for that thread creation
(idCompiler::ParseObjectCall() => idCompiler::EmitFunctionParms(), op == OP_OBJTHREAD)
it sets the wrong size for the statement, taken from func->value.functionPtr->parmTotal.
This happens at game startup, when all the scripts are parsed and compiled.

The reason that parmTotal for the "check_music_volume" function_t is 0 (while it should be 8, at least on my machine, for the self object reference) is, that even though an idTypeDef with type ev_function and the function_t is created when object player : player_base { is parsed and it encounters the void check_music_volume(); line, parmTotal is not calculated and set until the implementation of the function is parsed much later (when it reaches void player::check_music_volume() { way below).

It all happens in idCompiler::ParseFunctionDef(...) - that function is called both for the function declaration/prototype (void check_music_volume();) is called from idCompiler::ParseObjectDef() when it parses the player object, [i]and[/i] later when parsing the [i]implementation[/i] (after the bytecode for player::init() is generated), in that case it's called from ParseNamespace() => ParseDefs() => ParseFunctionDef().

The reason the second call calculates and sets parmTotal (and also numParams) correctly (and the first call doesn't) is the following lines in idCompiler::ParseFunctionDef(...):

	// check if this is a prototype or declaration
	if ( !CheckToken( "{" ) ) {
		// it's just a prototype, so get the ; and move on
		ExpectToken( ";" );
		return;
	}

	// 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();
		}
		func->parmTotal += func->parmSize[ i ];
	}

	// define the parms
	// ... etc ...

So when parsing the method declaration/prototype ParseFunctionDef(...) returns before doing the calculations for stack space needed by function parameters - this is only done when parsing the implementation!
So, if the method is called in the script file [i]before[/i] the the implementation, assert( st->c->value.argSize == func->parmTotal ); in idInterpreter::Execute() fails.

I have no idea why the stack space calculation etc isn't just done when parsing the prototype.

A Workaround

Modify idCompiler::EmitFunctionParms() like this:

	} else if ( ( op == OP_OBJECTCALL ) || ( op == OP_OBJTHREAD ) ) {
		EmitOpcode( op, object, VirtualFunctionConstant( func ) );

		// need arg size seperate since script object may be NULL
		statement_t &statement = gameLocal.program.GetStatement( gameLocal.program.NumStatements() - 1 );
		int size = func->value.functionPtr->parmTotal; // DG: added this and the following lines
		if(size == 0) // can happen if function implementation hasn't been parsed yet
		{
			const idTypeDef* funcType = func->value.functionPtr->type;
			int calcSize = 0;
			int numParms = funcType->NumParameters();
			for( int i = 0; i < numParms; i++ ) {
				idTypeDef* parmType = funcType->GetParmType( i );
				if ( parmType->Inherits( &type_object ) ) {
					calcSize += type_object.Size();
				} else {
					calcSize += parmType->Size();
				}
			}
			if(calcSize > size) {
				size = calcSize;
			}
		}
		statement.c = SizeConstant( size ); // DG: modified this to use size
	} else {

A proper solution would be to do the full parameter calculation in ParseFunctionDef() when parsing the function prototype and not just when parsing the implementation - but then you gotta make sure the parameters and their sizes aren't added twice (when parsing the prototype and when parsing the implementation), of course, so it needs a bit of thinking and cautiousness.

(I originally documented this at http://idtechforums.fuzzylogicinc.com/index.php?topic=897.0 but I think we should have a proper issue here so I hopefully don't forget to "properly" fix it eventually)

[DanielGibson]

I think I have a "proper" fix for this, see the script-fixes branch.

Not sure I dare to merge this before 1.5.1; testing+feedback is welcome!

[DanielGibson]

Here is a windows build (latest git master + this script fix for base.dll and d3xp.dll): dhewm3-scriptfix.zip

As the script interpreter is part of the gamecode this must be fixed separately in every mod that needs its own DLL, so right now only the main game and RoE are fixed (and mods without their own DLL that use base.dll).

[AntonieS]

dhewm3.exe do not unload properly from RAM. Process stay in memory.
Win 7 Ult SP1 x64, 64Gb RAM. No any antiviruses.

[DanielGibson]

There's some kind of crash when ending the game due to a bug in the current openal-soft version - but I've never seen it not exit at all

[AntonieS]

So, I have a situation during test when 7 processes named dhewm3.exe exists in memory.

[DanielGibson]

Weird - I'll have to test on my old Win7 machine.
Is is sufficient to start dhewm3.exe and immediately quit from main menu?

[AntonieS]

My test:
Load Dhewm3 (Doom3), load game/hellhole, exit from dhewm3 using main menu.

On version 1.5.1pre1304 there is no problem.

[DanielGibson]

So the problem doesn't happen if you don't load the map?

Can you try if the problem goes away if you use 1.5.1rc2 with the OpenAL32.dll from 1.5.1pre ?

[AntonieS]

Made a test: just load dhewm3 without load any map: still stay in memory.

Upd: test with OpenAL from 1.5.1.per1304: dhewm3 unload from RAM correctly.
By the way: I have Creative X-Fi Titanium HD card in system installed.

[DanielGibson]

@AntonieS: Can you try if it works better when replacing OpenAL32.dll with this one:
OpenAL32.dll.zip

[AntonieS]

With new OpenAL.dll work well. But in Lost Mission crush when load exis1.

[DanielGibson]

I need more information on the exis1 crash.
Does it happen if you do map game/le_exis1? Or only from a savegame or something? If so, can you share that savegame?
Does it crash while loading, directly after loading or ... ?
Do you have any further information on the crash, like errors being displayed when it crashes?

[AntonieS]

I became crash only when load savegame (while loading game hangs). Whean load from console - all Ok.
Exis1 autosave
https://yadi.sk/d/5AQ-ftycKTGd1Q
No errors displayed.

[AntonieS]

With new dll sound is much better. In both rc2 and pre2 versions.

[DanielGibson]

@AntonieS: The crash should be fixed in the latest build, can you test it? dhewm3_1.5.1_RC3_win32.zip

[AntonieS]

Now all works fine. Thanks.

[DanielGibson]

Good to know, thanks for testing!

By the way, I also put the fix for that script assertion into the master branch and it's also in the 1.5.1 RC3 build linked above, so I guess this can be closed