Advertisement

MSVC optimization bug?

Started by May 09, 2018 06:38 PM
10 comments, last by Miss 6 years, 3 months ago

Not sure if this is an Angelscript bug or somehow an MSVC bug. I'm compiling Angelscript 2.23.0 on 32 bit in release build using platform toolset v141 and SDK 8.1, 10.0.x.0 (I've tried 8.1, 10.0.16299.0, and 10.0.17134.0) Though it's quite strange, because this only started happening on 1 project after the most recent Visual Studio 2017 update. (It seems to be fine in another project, which is using the 8.1 SDK, but that project is also a custom build from GENie, this project is built from the msvc2015 folder that comes with Angelscript itself.)

The problem I'm seeing comes from the script engine message callback. I'm registering it like this, as a CDECL call:


int r = 0; m_engine->SetMessageCallback(asFUNCTION(ScriptMessageCallback), nullptr, asCALL_CDECL); assert(r >= 0);

And the actual function is empty: (actually commented out just to test this problem)


static void ScriptMessageCallback(const asSMessageInfo *msg, void *param)
{
}

Which results in a single "ret" instruction in memory. This makes sense, it's a cdecl call, so the caller does the cleanup.

So, this function is eventually called on some script error. The actual call to the function is in "asCScriptEngine::CallGlobalFunction". In the debug version of Angelscript, the assembly is fine:


03F22B9F 8B 45 0C             mov         eax,dword ptr [param2]  
03F22BA2 50                   push        eax  
03F22BA3 8B 4D 08             mov         ecx,dword ptr [param1]  
03F22BA6 51                   push        ecx  
03F22BA7 FF 55 F0             call        dword ptr [ebp-10h]  
03F22BAA 83 C4 08             add         esp,8  

In the release build it works a little different, it sets up the call and calls the function, but it doesn't clean up the stack:


0400C461 56                   push        esi  
0400C462 8B 30                mov         esi,dword ptr [eax]  
0400C464 83 FA 02             cmp         edx,2  
0400C467 74 5F                je          asCScriptEngine::CallGlobalFunction+88h (0400C4C8h)  
...
0400C4C8 FF 75 0C             push        dword ptr [param2]  
0400C4CB FF 75 08             push        dword ptr [param1]  
0400C4CE FF D6                call        esi  
0400C4D0 8B 4D F4             mov         ecx,dword ptr [ebp-0Ch]  
0400C4D3 64 89 0D 00 00 00 00 mov         dword ptr fs:[0],ecx  
0400C4DA 5E                   pop         esi  
0400C4DB 8B E5                mov         esp,ebp  
0400C4DD 5D                   pop         ebp  
0400C4DE C2 10 00             ret         10h  

Note that "esi" on the first instruction above is the thisptr, and is how the caller eventually picks up thisptr again. Since the stack is not cleaned up (before esi is restored at "pop esi"), esi actually ends up being param1.

Even if I explicitly write the function pointer as __cdecl, it doesn't clean up the stack in time: (results in the same assembly)


typedef void (__cdecl *func_t)(void *, void *);
func_t f = (func_t)(i->func);
f(param1, param2);

For reference, here's the CallGlobalFunction C++ code directly from Angelscript:


void asCScriptEngine::CallGlobalFunction(void *param1, void *param2, asSSystemFunctionInterface *i, asCScriptFunction *s) const
{
	if( i->callConv == ICC_CDECL )
	{
		void (*f)(void *, void *) = (void (*)(void *, void *))(i->func);
		f(param1, param2);
	}
	else if( i->callConv == ICC_STDCALL )
	{
		typedef void (STDCALL *func_t)(void *, void *);
		func_t f = (func_t)(i->func);
		f(param1, param2);
	}
	else
	{
		// We must guarantee the order of the arguments which is why we copy them to this
		// array. Otherwise the compiler may put them anywhere it likes, or even keep them
		// in the registers which causes problem.
		void *params[2] = {param1, param2};

		asCGeneric gen(const_cast<asCScriptEngine*>(this), s, 0, (asDWORD*)&params);
		void (*f)(asIScriptGeneric *) = (void (*)(asIScriptGeneric *))(i->func);
		f(&gen);
	}
}

What's going on here? Am I missing something? Do I need to report this as an optimization bug to Microsoft? Do I need to make this a stdcall for some reason..?

You haven't shown the full assembler code, so it is hard to tell if this is wrong or not, but it looks to me that the optimized code is doing the cleanup of the arguments as it should. It just does it in a different way.

0400C4DB 8B E5 mov esp,ebp

This instruction restores the stack pointer to the original value that was backed up before the arguments were pushed on the stack. 

 

I don't see that you're doing anything wrong in your code. Are you sure the problem is actually what you say it is, that the stack is not cleaned up? How did you reach that conclusion? What is the actual symptom you're seeing? 

If you place some logic in the message callback (for example a cout, or print) does the problem go away?

 

I recently installed MSVC 2017 myself. I'll try to reproduce the problem.

 

AngelCode.com - game development and more - Reference DB - game developer references
AngelScript - free scripting library - BMFont - free bitmap font generator - Tower - free puzzle game

Advertisement

Sorry, you're right, it is indeed cleaning the stack there, but at that point it's already too late. I should've added the code that is being returned to as well in my post. Here's the disassembly right after CallGlobalFunction, where it's using esi as thisptr (popped from the stack before the stack is cleaned up):


03EC65B1 E8 CA 73 00 00       call        asCScriptEngine::CallGlobalFunction (03ECD980h)  

		preMessage.isSet = false;
03EC65B6 8B 45 08             mov         eax,dword ptr [section]  
03EC65B9 C6 86 BC 0B 00 00 00 mov         byte ptr [esi+0BBCh],0  

Basically, I think msvc is confusing the stdcall code with the cdecl code in the optimization. If there's an "add esp, 8" above "pop esi", everything would be fine.

For reference, at the time of "pop esi", this is what the stack looks like:


$    param1
$+04 param2
$+08 esi (thisptr)

 

I see what you're referring to now. 

It does indeed look like the MSVC compiler is producing the wrong assembler code with regards to the esi register. 

I suggest you report this to Microsoft, as it would definitely be a bug in their compiler. 

AngelCode.com - game development and more - Reference DB - game developer references
AngelScript - free scripting library - BMFont - free bitmap font generator - Tower - free puzzle game

So did you reproduce it as well?

Edit: Here's the reproduce steps that work for me. https://gist.github.com/codecat/9128e290b7f3fc74374a3035ff79e9af

Okay, I've narrowed it down a bit. This is caused by the latest Visual Studio 15.7.1 update, and this is isolated reproduction code: https://pastebin.com/nGW11NAx

It will run on a debug build, but crash on a release build (it jumps to 0x1337 at the end of main() because the stack isn't cleaned up).

I've sent a bugreport to Microsoft regarding this issue.

Advertisement

Okay, so Microsoft reported back to me (very quickly!), saying they've filed this bug and that they're working on it.

As a workaround, there's 2 possibilities, either using #pragma optimize("", off) on CallGlobalFunction, or - which I think makes more sense since this bug might cause problems elsewhere in Angelscript - is to use a new undocumented (and completely unsupported) compiler flag /d2SSAOptimizerCFG- which "turns off all of our new control-flow-graph (CFG) based optimizations for the compilation, of which the erroneous optimization is a part".

It's not recommended to use this compiler flag though, as it's completely unsupported and might disappear in the future, but if anyone is looking for a critical fix for this problem for the time being, it's probably a nice temporary solution.

PS: Sorry for the triple post, but I figured it's fine since these are sequential updates..

Thanks for informing that Microsoft is working on the fix. Probably they'll have a fix soon enough.

I haven't had a chance to try to reproduce the problem yet.

It ought to be possible to use the #pragma directive combines with a #ifdef to check the version of MSVC. If MS takes too long to fix the bug I'll add that to the repository.

 

 

AngelCode.com - game development and more - Reference DB - game developer references
AngelScript - free scripting library - BMFont - free bitmap font generator - Tower - free puzzle game

Hi,

I'm the Microsoft compiler developer who handled Miss's excellent bug-report and can happily say that we have a fix in the compiler, and it should soon start appearing in VS2017 15.8 previews (not sure which one yet) and of course the final release of 15.8. (Book-keeping: For anyone finding this discussion online, you can use the "Report a Problem" button in the IDE, or you can go directly to https://developercommunity.visualstudio.com and hit the "C++" tab).

We really like supporting all the games developers and want to make sure there aren't any barriers to you all filing bug reports.

Thanks again everyone!

Hi Aaron,

thanks for taking the time to let us know that the fix has been identified and in which version it will available in.

Regards,
Andreas

AngelCode.com - game development and more - Reference DB - game developer references
AngelScript - free scripting library - BMFont - free bitmap font generator - Tower - free puzzle game

This topic is closed to new replies.

Advertisement