From 1037774770d67bc95fd19efe326b60e1f6fc3d4d Mon Sep 17 00:00:00 2001 From: Coder666 Date: Tue, 17 May 2022 09:14:42 +0100 Subject: [PATCH 1/4] Fix for incorrect return value offset --- as_jit.cpp | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 55 insertions(+), 3 deletions(-) diff --git a/as_jit.cpp b/as_jit.cpp index bb8f708..0c0dc8f 100644 --- a/as_jit.cpp +++ b/as_jit.cpp @@ -3515,10 +3515,62 @@ void SystemCall::call_64conv(asSSystemFunctionInterface* func, if(sFunc->DoesReturnOnStack()) { Register arg0 = as(cpu.intArg64(firstPos, firstPos, pax)); - if(pos == OP_None || objPointer) - arg0 = as(*esi); - else + // + // TOMW - 16/05/2022 + // This switch statement is a (MSVC?) fix for incorrect return value + // location when an object is returned on the stack + // + // AngelScript uses a structure: + // + // struct asSVMRegisters + // { + // asDWORD* programPointer; // points to current bytecode instruction + // asDWORD* stackFramePointer; // function stack frame + // asDWORD* stackPointer; // top of stack (grows downward) + // asQWORD valueRegister; // temp register for primitives + // void* objectRegister; // temp register for objects and handles + // asITypeInfo* objectType; // type of object held in object register + // bool doProcessSuspend; // whether or not the JIT should break out when it encounters a suspend instruction + // asIScriptContext* ctx; // the active context + // }; + // + // + // And "stackPointer" is an array that contains any "this" or object pointer + // any argument pointers and if "DoesReturnOnStack()" is true, it also contains + // a pointer to the space for the return value + // + // This was not correctly being handled an was overwriting one of the argument pointers + // + // The bug only appeared when we converted functions from this: string foo( string, string ) + // to this: string foo( const string& in, const string& in ) + // + // which caused an object lifetime error, where the reference was being released + // + // In this JIT compiler, this asSVMRegisters is pointed to by ebp + // also, r13 (the variable esi) is pointing at asDWORD* stackPointer + // + + // AngelScript return value memory is already preallocated on the stack (r13), + // and the pointer to the location is found before the first arg + switch (pos) + { + case OP_First: + case OP_Last: + { + //always second item in the array arg0 = as(*esi + sizeof(asPWORD)); + } + break; + case OP_This: + case OP_None: + { + arg0 = as(*esi); + } + break; + default: + _ASSERT(false && "unknown operation"); + } + if(acceptReturn) as(*esp + local::retPointer) = arg0; retPointer = true; From 3a275212701e94926ba7c334f1803d6b0d05b3f5 Mon Sep 17 00:00:00 2001 From: Coder666 Date: Fri, 20 May 2022 17:32:15 +0100 Subject: [PATCH 2/4] fixed issue with arguments being the wrong stack location --- as_jit.cpp | 58 +++--------------------------------------------------- 1 file changed, 3 insertions(+), 55 deletions(-) diff --git a/as_jit.cpp b/as_jit.cpp index 0c0dc8f..bb8f708 100644 --- a/as_jit.cpp +++ b/as_jit.cpp @@ -3515,62 +3515,10 @@ void SystemCall::call_64conv(asSSystemFunctionInterface* func, if(sFunc->DoesReturnOnStack()) { Register arg0 = as(cpu.intArg64(firstPos, firstPos, pax)); - // - // TOMW - 16/05/2022 - // This switch statement is a (MSVC?) fix for incorrect return value - // location when an object is returned on the stack - // - // AngelScript uses a structure: - // - // struct asSVMRegisters - // { - // asDWORD* programPointer; // points to current bytecode instruction - // asDWORD* stackFramePointer; // function stack frame - // asDWORD* stackPointer; // top of stack (grows downward) - // asQWORD valueRegister; // temp register for primitives - // void* objectRegister; // temp register for objects and handles - // asITypeInfo* objectType; // type of object held in object register - // bool doProcessSuspend; // whether or not the JIT should break out when it encounters a suspend instruction - // asIScriptContext* ctx; // the active context - // }; - // - // - // And "stackPointer" is an array that contains any "this" or object pointer - // any argument pointers and if "DoesReturnOnStack()" is true, it also contains - // a pointer to the space for the return value - // - // This was not correctly being handled an was overwriting one of the argument pointers - // - // The bug only appeared when we converted functions from this: string foo( string, string ) - // to this: string foo( const string& in, const string& in ) - // - // which caused an object lifetime error, where the reference was being released - // - // In this JIT compiler, this asSVMRegisters is pointed to by ebp - // also, r13 (the variable esi) is pointing at asDWORD* stackPointer - // - - // AngelScript return value memory is already preallocated on the stack (r13), - // and the pointer to the location is found before the first arg - switch (pos) - { - case OP_First: - case OP_Last: - { - //always second item in the array + if(pos == OP_None || objPointer) + arg0 = as(*esi); + else arg0 = as(*esi + sizeof(asPWORD)); - } - break; - case OP_This: - case OP_None: - { - arg0 = as(*esi); - } - break; - default: - _ASSERT(false && "unknown operation"); - } - if(acceptReturn) as(*esp + local::retPointer) = arg0; retPointer = true; From 2fc746352f129204bc11b464ec9adbb2febc647a Mon Sep 17 00:00:00 2001 From: Coder666 Date: Fri, 20 May 2022 17:33:40 +0100 Subject: [PATCH 3/4] reverted commit in error --- as_jit.cpp | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 55 insertions(+), 3 deletions(-) diff --git a/as_jit.cpp b/as_jit.cpp index bb8f708..0c0dc8f 100644 --- a/as_jit.cpp +++ b/as_jit.cpp @@ -3515,10 +3515,62 @@ void SystemCall::call_64conv(asSSystemFunctionInterface* func, if(sFunc->DoesReturnOnStack()) { Register arg0 = as(cpu.intArg64(firstPos, firstPos, pax)); - if(pos == OP_None || objPointer) - arg0 = as(*esi); - else + // + // TOMW - 16/05/2022 + // This switch statement is a (MSVC?) fix for incorrect return value + // location when an object is returned on the stack + // + // AngelScript uses a structure: + // + // struct asSVMRegisters + // { + // asDWORD* programPointer; // points to current bytecode instruction + // asDWORD* stackFramePointer; // function stack frame + // asDWORD* stackPointer; // top of stack (grows downward) + // asQWORD valueRegister; // temp register for primitives + // void* objectRegister; // temp register for objects and handles + // asITypeInfo* objectType; // type of object held in object register + // bool doProcessSuspend; // whether or not the JIT should break out when it encounters a suspend instruction + // asIScriptContext* ctx; // the active context + // }; + // + // + // And "stackPointer" is an array that contains any "this" or object pointer + // any argument pointers and if "DoesReturnOnStack()" is true, it also contains + // a pointer to the space for the return value + // + // This was not correctly being handled an was overwriting one of the argument pointers + // + // The bug only appeared when we converted functions from this: string foo( string, string ) + // to this: string foo( const string& in, const string& in ) + // + // which caused an object lifetime error, where the reference was being released + // + // In this JIT compiler, this asSVMRegisters is pointed to by ebp + // also, r13 (the variable esi) is pointing at asDWORD* stackPointer + // + + // AngelScript return value memory is already preallocated on the stack (r13), + // and the pointer to the location is found before the first arg + switch (pos) + { + case OP_First: + case OP_Last: + { + //always second item in the array arg0 = as(*esi + sizeof(asPWORD)); + } + break; + case OP_This: + case OP_None: + { + arg0 = as(*esi); + } + break; + default: + _ASSERT(false && "unknown operation"); + } + if(acceptReturn) as(*esp + local::retPointer) = arg0; retPointer = true; From 57d080c605835b36918fbd13e1329c997412a93d Mon Sep 17 00:00:00 2001 From: Coder666 Date: Mon, 17 Oct 2022 12:45:29 +0100 Subject: [PATCH 4/4] fix issue with thiscall returning a stack pre-allocated object --- as_jit.cpp | 25 ++++++++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/as_jit.cpp b/as_jit.cpp index 0c0dc8f..57fd41c 100644 --- a/as_jit.cpp +++ b/as_jit.cpp @@ -3561,16 +3561,35 @@ void SystemCall::call_64conv(asSSystemFunctionInterface* func, arg0 = as(*esi + sizeof(asPWORD)); } break; - case OP_This: + case OP_This: + { + // + // If we have any argument then argOffset is the stack (r13) offset to that + // in the case of a return value allocated on the stack, when calling a "thiscall" + // function, argOffset will point to it, i.e: + // + // string foo::bar() -> r13[0]->this r13[1]->string mem + // + // if it's a simple function that returns a register size value, then argOffset == 0 + // + // something@ bar::foo() -> r13[0]->this r13[1]->nothing + // + // the return value is picked up in rax + // + // NOTE: "esi" is actually r13 in this case + + arg0 = as(*esi + argOffset); + } + break; case OP_None: { - arg0 = as(*esi); + arg0 = as(*esi); } break; default: _ASSERT(false && "unknown operation"); } - + if(acceptReturn) as(*esp + local::retPointer) = arg0; retPointer = true;