News:

One Minute Game Review by The Happy Friar: https://ugetube.com/@OneMinteGameReviews
Also on Rumble: https://rumble.com/c/c-1115371

idTech 4 (aka Doom 3 tech) Discord Server! https://discord.gg/9wtCGHa

Main Menu

Should order of method implementations matter?

Started by caedes, April 14, 2020, 01:30:02 PM

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

caedes

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()

(Please read this if you're familiar with Doom3 scripting; I will explain what seems to go wrong in C++, but you can ignore that part, I'd just like to know if this issue is know and/or considered normal. Thanks! :))

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() { ... }

Is it a known problem or considered normal, that the order of function/method implementations matters if one function calls another function?


(Following the C++ analysis:)

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, and later when parsing the implementation (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 before 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.

The Happy Friar

I want to say that's how it's always behaved, so it's "normal".   I think it's weird though.

caedes

Thanks, that's good to know!

I googled that assertion and got basically no results (except for a dhewm3 bugreport), so it seemed to me like no one ran into this before..
Maybe people usually use release builds of the DLLs with assertions disabled (that would explain why I stumbled upon it in someone elses script), or only doom3world knew about that and took that knowledge to the grave.

It's indeed weird, and totally unnecessary - I have a simple workaround that's less than 20 lines of code - and it shouldn't be super hard to do it properly and do the full parameter calculation in ParseFunctionDef() when parsing the function prototype and not just when parsing the implementation either (you gotta make sure the parameters and their sizes aren't added twice, of course, so it needs a bit of thinking and cautiousness).

The 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; // caedes: 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 ); // caedes: modified this to use size
} else {

caedes

I think I have a fix for the Assertion, see https://github.com/dhewm/dhewm3/issues/303 or https://github.com/dhewm/dhewm3/commit/4902bc3e2051c5d4ec125dee5d20e3d549ff46bc for the code change.
As far as I can tell it works (and doesn't seem to break anything else), but it should probably be tested more before going into the master branch or even the next release..