View Issue Details

IDProjectCategoryView StatusLast Update
0004713The Dark ModScript/Defpublic27.03.2021 14:28
Reporterstgatilov Assigned Tostgatilov  
PrioritylowSeverityminorReproducibilityN/A
Status resolvedResolutionfixed 
Product VersionTDM 2.06 
Target VersionTDM 2.10Fixed in VersionTDM 2.10 
Summary0004713: Starting new script thread with method is wrong
DescriptionIn order to start a thread in script, you write:
  thread myFunc(param1, param2, param3);
Here myFunc should be a global function.

Using a method of an object as myFunc is also possible.
((((( it is wrong, because it won't receive "this" pointer.))))) --- UPDATE: there is no problem with "this" here!
If you run such a script in debug build, it will have "assertion failed" error.
Such scripts are present "in the wild".
Primary example is "Accountant 2": it has assertion failed on startup in debug build due to line convbelt.script:13.

We should find a way to fix this issue. For example:
(((((1. Make sure that this error crashes release build too, then fix missions.))))) --- UPDATE: this is perfectly valid and specifically supported case!
2. (((((Find some limited case when calling method without this is legal))))), and make sure it is properly supported in all builds.
Steps To Reproduce1. Start debug build of TDM.
2. Start mission "Accountant 2".

You'll get assertion failed in Script_Interpreter.cpp on OP_OBJTHREAD case in this line:
  assert( st->c->value.argSize == func->parmTotal );
Additional InformationNote: there is function called callFunctionOn in tdm_util.script, which allows you to create new thread like this:
  thread callFunctionOn("frob_ignite", child);
Here new thread calls child.frob_ignite();

This function does not support passing parameters.

UPDATE: This approach is not good, since it does not catch compiler errors due to type on method name.
As it turned out, just writing the call directly is OK:
  thread child.frob_ignite();
Hopefully, all the minor problems with such approach are fixed now...
TagsNo tags attached.

Activities

stgatilov

stgatilov

26.03.2021 17:38

administrator   ~0013806

How to reproduce on smaller scale:

1) Add the following spawnargs to any entity on the map:
  "scriptobject" "test"
  "start" "start" //this is probably not needed?
2) Add the following to file {mapname}.script near map:
  object test {
      float v0;
      float v1;
      float v2;
      float v3;
      void init();
      void printText();
  };
  void test::init() {
      thread printText();
  }
  void test::printText() {
      sys.println("thread done");
  }

When you start map, the init method is called, and the problem happens.
stgatilov

stgatilov

26.03.2021 17:48

administrator   ~0013807

I tried to print out some members of the current class straight in the method where thread was created.
The members are printed properly.
It means that despite the assert, the "this" pointer is properly passed into the new thread (as expected).

It remains to understand what these asserts are about...
stgatilov

stgatilov

27.03.2021 10:09

administrator   ~0013808

Last edited: 27.03.2021 10:09

View 2 revisions

Gathered all types of calls in one place:

-----------------------------------------------------------------------------
object test {
    float v0;
    float v1;
    float v2;
    float v3;
    void init();
    void printText(float x0, float x1);
};

void freefunc(float x0, float x1) {
    sys.println("free func");
    sys.println(x0);
    sys.println(x1);
}

void test::init() {
    v0 = 10;
    v1 = 11;
    v2 = 12;
    v3 = 13;
    test e = self;

    printText(-3.0, -4.3);
    e.printText(-0.3, -3.4);
    freefunc(-0.97, -9.7);

    thread printText(3.0, 4.3);
    thread e.printText(0.3, 3.4);
    thread freefunc(0.97, 9.7);
}
void test::printText(float x0, float x1) {
    sys.println("method");
    sys.println(v0);
    sys.println(v1);
    sys.println(v2);
    sys.println(v3);
    sys.println(x0);
    sys.println(x1);
}
-----------------------------------------------------------------------------

And looked at the disassembly ("g_disasm 1"):

-----------------------------------------------------------------------------
function test::init() 24 stack used, 4 parms, 20 locals {

              # stack[0] contains "this" pointer
              # save 10.0 into address (this + 0) (which is address of this->v0)
(L17): 20896: ADDRESS a: test stack[0] b: field 0 c: pointer stack[4]
(L17): 20897: STOREP_F a: const float 10.000000 b: pointer stack[4]
              # same for v1, v2, v3
(L18): 20898: ADDRESS a: test stack[0] b: field 4 c: pointer stack[8]
(L18): 20899: STOREP_F a: const float 11.000000 b: pointer stack[8]
(L19): 20900: ADDRESS a: test stack[0] b: field 8 c: pointer stack[12]
(L19): 20901: STOREP_F a: const float 12.000000 b: pointer stack[12]
(L20): 20902: ADDRESS a: test stack[0] b: field 12 c: pointer stack[16]
(L20): 20903: STOREP_F a: const float 13.000000 b: pointer stack[16]

              # copy "this" into local variable "e" (which has offset 20)
(L21): 20904: STORE_OBJ a: test stack[0] b: test stack[20]

              # method(c1, c2); //implicit object
(L23): 20905: PUSH_OBJ a: test stack[0]
(L23): 20906: PUSH_F a: const float -3.000000
(L23): 20907: PUSH_F a: const float -4.300000
              # note that function is specified as index of VFT, since all methods are automatically virtual
(L23): 20908: OBJECTCALL a: test stack[0] b: const virtual function vtable[ 1 ] c: const args 0

              # e.method(c1, c2); //explicit object
(L24): 20909: PUSH_OBJ a: test stack[20]
(L24): 20910: PUSH_F a: const float -0.300000
(L24): 20911: PUSH_F a: const float -3.400000
(L24): 20912: OBJECTCALL a: test stack[20] b: const virtual function vtable[ 1 ] c: const args 0

              # freefunc(c1, c2); //no object
(L25): 20913: PUSH_F a: const float -0.970000
(L25): 20914: PUSH_F a: const float -9.700000
(L25): 20915: CALL a: const function freefunc

              # thread method(c1, c2); //implicit object
(L27): 20916: PUSH_OBJ a: test stack[0]
(L27): 20917: PUSH_F a: const float 3.000000
(L27): 20918: PUSH_F a: const float 4.300000
(L27): 20919: OBJTHREAD a: test stack[0] b: const virtual function vtable[ 1 ] c: const args 0

              # thread e.method(c1, c2); //explicit object
(L28): 20920: PUSH_OBJ a: test stack[20]
(L28): 20921: PUSH_F a: const float 0.300000
(L28): 20922: PUSH_F a: const float 3.400000
(L28): 20923: OBJTHREAD a: test stack[20] b: const virtual function vtable[ 1 ] c: const args 0

              # thread freefunc(c1, c2); //no object
(L29): 20924: PUSH_F a: const float 0.970000
(L29): 20925: PUSH_F a: const float 9.700000
(L29): 20926: THREAD a: const function freefunc b: const args 8

(L30): 20927: RETURN
-----------------------------------------------------------------------------

Obviously:
1) Creating thread by object + method is supported by its own opcode OBJTHREAD, so it should be legal.
2) The pointer to object and all arguments are properly passed in all cases.
stgatilov

stgatilov

27.03.2021 10:21

administrator   ~0013809

Last edited: 27.03.2021 10:31

View 3 revisions

It seems that the "c" argument to object calls is wrong.
It is zero even though there are some parameters here.

For ordinary method calls (OP_OBJECTCALL), the "c" value is used only if method is called on entity, which is dead, null, or misses script object.
It is used to pop function parameters from the locals stack:
    PopParms( st->c->value.argSize );
If the call succeeds, then callee allocates additional space for local variables (which are not parameters), see EnterFunction:
   // allocate space on the stack for locals
   // parms are already on stack
   c = func->locals - func->parmTotal;
   localstackUsed += c;
and callee also removes both local variables and parameters from stack, see LeaveFunction:
   PopParms( currentFunction->locals );
For some reason, I see "c" = 0 in the disassembly, which seems to be wrong.

Calls of free functions (CALL) don't have such argument.

But free functions called in a newly created thread (THREAD) do have this argument as "b".
Its value seems to be set correctly to the total size of arguments (8 bytes in the example).
In the C++ code, it is used to pop back parameters of the called function (unconditionally):
    PopParms( st->b->value.argSize );

Finally, there are method calls in newly created thread (OP_OBJTHREAD).
They also seem to have wrong value "c" = 0 right now.
This "c" value is also used to remove function parameters from stack:
  PopParms( st->c->value.argSize );
Also, it is checked against some function-defined number:
  assert( st->c->value.argSize == func->parmTotal );
In the current case, it is 12 --- obviously 12 bytes is used for two float arguments + object pointer.

So I think the "c" value should be 12 (size of all arguments including object pointer) both in OBJECTCALL and OBJTHREAD calls.
stgatilov

stgatilov

27.03.2021 11:10

administrator   ~0013810

The problem happens because func->parmTotal is computed only when function definition is parsed.
When function declaration is met, the function is created, but all of its members like "parmTotal" are zero.
See idCompiler::ParseFunctionDef.

Also, the sample given above is fully fixed (no asserts) if I swap the definitions of test::init and test::printText methods.
stgatilov

stgatilov

27.03.2021 14:22

administrator   ~0013811

Last edited: 27.03.2021 14:23

View 2 revisions

Fixed in svn rev 9226.

The fix is inside idCompiler::ParseFunctionDef.
Now when we parse function declaration/prototype, we compute parmSize and parmTotal too.
It is not critical that we can compute it twice (or maybe even more times), but it is critical to have it computed when we call the method (which can happen before we see definition).

This fix removed all asserts both on the testing code I attached, and on the Accountant 2 FM.
I have skimmed through this FM, and all scripted events seem to work properly.

So the bottom line is: creating new thread with object method call is perfectly valid!
Specifying the object explicitly is not required, "this" is implicitly passed --- just like for ordinary method calls.

Issue History

Date Modified Username Field Change
25.12.2017 10:06 stgatilov New Issue
25.12.2017 10:09 stgatilov Additional Information Updated View Revisions
01.07.2018 04:05 stgatilov Assigned To => stgatilov
01.07.2018 04:05 stgatilov Status new => assigned
21.03.2020 17:45 stgatilov Target Version => TDM 2.09
05.12.2020 12:36 stgatilov Target Version TDM 2.09 => TDM 2.10
26.03.2021 17:38 stgatilov Note Added: 0013806
26.03.2021 17:48 stgatilov Note Added: 0013807
27.03.2021 10:09 stgatilov Note Added: 0013808
27.03.2021 10:10 stgatilov Note Edited: 0013808 View Revisions
27.03.2021 10:21 stgatilov Note Added: 0013809
27.03.2021 10:22 stgatilov Note Edited: 0013809 View Revisions
27.03.2021 10:31 stgatilov Note Edited: 0013809 View Revisions
27.03.2021 11:10 stgatilov Note Added: 0013810
27.03.2021 14:22 stgatilov Note Added: 0013811
27.03.2021 14:23 stgatilov Note Edited: 0013811 View Revisions
27.03.2021 14:25 stgatilov Description Updated View Revisions
27.03.2021 14:28 stgatilov Description Updated View Revisions
27.03.2021 14:28 stgatilov Additional Information Updated View Revisions
27.03.2021 14:28 stgatilov Status assigned => resolved
27.03.2021 14:28 stgatilov Resolution open => fixed
27.03.2021 14:28 stgatilov Fixed in Version => TDM 2.10