Wrong double value returned with class method (Mac / 32-bit)

[bluecataudio]

Using the latest version of the JIT, calling a simple method returning a double returns a random value, on Mac /32-bit (works fine on Mac 64-bit and Windows 32 and 64):

class Test { double Value(){return 0.0;} };

Test t;

double result=t.Value();

"result" is a completely random value. It is probably a regression due to the recent changes in the THIS_CALL implementation, but I have not checked yet if previous versions had the problem. The same code with floats work fine.

[ThyReaper]

Since we don't yet officially support Mac, and have no means to debug on Mac, you'll need to provide more information.

[bluecataudio]

What kind of information could help? Does it work fine on 32-bit Linux?

[removedmy previous remark as it has nothing to do with the actual problem since this is all angelscript code, not native function calls]

[bluecataudio]

After debugging and reading the JIT code a bit more, it seems that in the second half of the valueRegister is handled differently from the first, in 32-bit mode: on a RET instruction, only the first half is set (using pbx), while the other half was set previously, during the asBC_CpyVtoR8 instruction. So I am guessing that the second half of the value register is erased by instructions in between (maybe the release call?). But I have not found where yet...

By the way, is it possible to dump the generated assembly code for a JIT function?

Code example: class T {double Value(){return 5.2;}}; T t; double main(){ return t.Value();}

Generated bytecode: something is probably happening to the second half of the value register during calls 13 or 15 in the JIT function.

double T::Value()
Temps: 2
Variables: 
 000: T this

    0   2 *    JitEntry 0
    2   2 *    PshVPtr  v0
    3   3 *    CALLSYS  2           (void _builtin_object_::_beh_5_())
    5   2 *    JitEntry 0
   1,25
    7   2 *    JitEntry 0
    9   2 *    SetV8    v2, 0x4014cccccccccccd          (i:4617540697942969549, f:5.2)
   12   2 * {
   12   2 * }
   12   2 *    CpyVtoR8 v2
   13   2 * 0:
   13   2 *    FREE     v0, 8720560  
   15   2 *    RET      1

double main()
Temps: 2
Variables: 

    0   2 *    JitEntry 0
   1,60
    2   2 *    JitEntry 0
    4   2 *    PshGPtr  4717696
    6   3 *    CHKREF
    7   3 *    CALLINTF 113           (double T::Value())
    9   2 *    JitEntry 0
   11   2 *    CpyRtoV8 v2
   12   2 * {
   12   2 * }
   12   2 *    CpyVtoR8 v2
   13   2 * 0:
   13   2 *    RET      0

[bluecataudio]

I can actually confirm that it is the call to FREE that messes up the register: commenting it in the JIT makes the returned value ok.

[ThyReaper]

Pushed what I think is a fix for this. AngelScript normally discards return values at those locations, which I broke with the refcounting change. I think it should be working for you now.

[bluecataudio]

Thanks for pushing a fix so quickly! Unfortunately, the issue is still here. After further analysis, the FREE instruction (to release the object) ends in call_viaAS() to call the release function, because it is an ICC_VIRTUAL_THISCALL. So either the problem comes from the CallSystemFunction in angelscript itself, or it is not called with the appropriate arguments, so the value register is modified whereas it should not (or maybe the value register should be actually set AFTER, when the RET instruction is called?). Still investigating...

[ThyReaper]

I think I forgot to change call_viaAS.

[ThyReaper]

Is it fixed now?

[bluecataudio]

No, but I think I have found the root cause. If I understand well, your patch fixes the fact that registers may be modified by copying data from the value register when it is not needed. Whereas the problem here is just that the value register is modified by the angelscript engine (and that's the value register that is used to return the value, and it was set before calling the release method thru the AS engine).

I have noticed that when the function called does not return anything, the angelscript engine wrongly erases the value register with random values. It is easy to fix using the following patch:

--- angelscript/source/as_callfunc.cpp  (AS 2.29 WIP)
+++ angelscript/source/as_callfunc.cpp  (working copy)
@@ -672,7 +672,7 @@
            *(asDWORD*)&context->m_regs.valueRegister = (asDWORD)retQW;
 #endif
        }
-       else
+       else if( sysFunc->hostReturnSize == 2 )
            context->m_regs.valueRegister = retQW;
    }

However I am wondering if it would not be safer to modify the JIT too, so that when returning a 64-bit value, the value register is properly set AFTER the call to the FREE instruction (in the RET instruction). Today, only half of it is set at this stage: //Update value register as<void>(ebp+offsetof(asSVMRegisters,valueRegister)) = pbx;

What do you think? Please disregard my comment above if this is nonsense, as I am a complete assembly/JIT compiler beginner...

[ThyReaper]

Though it would be more correct for AngelScript not to touch the value register if there is no return, it is still valid for these functions to return something that would write to the register. AngelScript would normally save the value of the register across the call, so that's the only proper way to fix it in the JIT.

[bluecataudio]

I agree that it would indeed be safer to really fix it in the JIT, but I don't know how to do it properly... Where is the second half of the value stored (first half is in pbx)?

[ThyReaper]

Just put up the fix.

[bluecataudio]

Wonderful, thanks! You rule! It indeed fixes the issue, even when the AS engine overwrites the register. Great job!

At least, this issue has enabled me to understand a bit better how the JIT works :-).