🎉 Celebrating 25 Years of GameDev.net! 🎉

Not many can claim 25 years on the Internet! Join us in celebrating this milestone. Learn more about our history, and thank you for being a part of our community!

x86 callfunc bug (with fix)

Started by
6 comments, last by WitchLord 17 years, 3 months ago
With 2.7 I encountered a bug where a simple function that returned a bool was always returning true. The function worked fine in 2.4. Looking into it appears that only the least significant byte of EAX was being set. So if EAX contained, say, 0xcccccccc and your function returned false it would now contain 0xcccccc00. RetQW then gets set to 0xcccccc00 as does eventually context->register1. The fix for this was to just copy over the last byte when returning a bool. Looking at the way the return values where set revealed another bug, the register isn't cleared before setting, so like EAX it may contain garbage since it's 64bit and typical return values are 32bit. Also, we must now check to see if we're returning a reference since GetSizeInMemoryBytes seems to return the size of the object being referred to (1 byte in the case of a bool) not the size of a reference (a 32bit pointer). Replacing the code around line 450 in as_callfunc_x86.cpp with the following fixes things:

	else
	{
		context->register1 = 0;
		// Store value in register1 register
		if( sysFunc->hostReturnFloat )
		{
			if( sysFunc->hostReturnSize == 1 )
				*(asDWORD*)&context->register1 = GetReturnedFloat();
			else
				context->register1 = GetReturnedDouble();
		}
		else if( descr->returnType.IsReference() )
		{	// pointer returned
			*(asDWORD*)&context->register1 = (asDWORD)retQW;
		}
		else if( descr->returnType.GetSizeInMemoryBytes() > 0 )
		{	// BUGFIX: if a bool is returned only the low byte of retQW
			// is set and the rest might be garbage so only take the
			// needed bytes over.
			if( descr->returnType.GetSizeInMemoryBytes() == 1 )
				*(asBYTE*)&context->register1 = (asBYTE)retQW;
			else if( descr->returnType.GetSizeInMemoryBytes() == 2 )
				*(asWORD*)&context->register1 = (asWORD)retQW;
			else// if( descr->returnType.GetSizeInMemoryBytes() == 4 )
				*(asDWORD*)&context->register1 = (asDWORD)retQW;
		}
		else
			context->register1 = retQW;

	}
Advertisement
The C++ compiler can choose to use any size it wants for bool, and MSVC at least will use 32 bits in some places rather than 8 for optimization and alignment reasons. The only effective way to defeat this issue is to just use ints.
No Deyja. That's not true. MSVC has very strict rules for type size and alignments. If this wasn't the case it wouldn't be able to properly link across translation units. You can change these rules using compiler switches or things like #pragma pack but the compiler doesn't just pick whatever it wants.

Also, using ints won't save you from the above bug because EAX/context->register1 is using a 64bit integer while an int only takes up 32bits (assuming a 32bit OS) so you'll still potentially have uncleared garbage.
No, because MSVC will never change the size of an int for optimization reasons. Now, it might not mess with bools if their in a function that is being exported from a dll or some such, I hadn't considered that case. And, like I said, it can mess with a bool and still link across translation units fine. The compiler keeps a hell of a lot of bookkeeping information.
Hi Popstar,

I'll have to make some tests on this. I'm sure you're right in that the bug is there, but I believe the bug fix is not quite what I want. Sure, it works, but it doesn't do it the way I want.

What should have happened is that the virtual machine should cleared the most significant bytes after the call by executing a BC_ubTOi. This is what is done when for example a char is returned.

As to how C++ handles the bool type. It is always returned in EAX, thus 32bits. Whether or not the topmost bytes are cleared or not is another matter. The calling function should make sure only the least significant byte is used to interpret the value. AngelScript didn't do this properly, as Popstar noticed.

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

Yeah I figured my fix wasn't the prettiest way to do it which is why I tried to explain exactly what was being fixed. That code is actually something that I backported from a PowerPC version of callfunc I got working (and I'll give you this week sometime - just need to clean up the file). It was needed to take care of some endian issues.

I did run your test_feature code on it to check if I'd broken anything else and all the tests did pass.
Yay, that would be cool indeed. Support for native calling conventions on yet another target platform is very welcome.

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

I've tried to reproduce this problem with 2.8.1 WIP, but the problem doesn't appear. Any chance you're interested in upgrading to version 2.8.0a or even the WIP? At least verify for me if the bug really doesn't exist in this newer version?

Do you need me to fix the bug in 2.7.1b? Having very little time left over for working on AngelScript lately I'd rather work on the new version than fixing bugs in older versions.

I think I'll update the AngelScript page to state that if bug fixes are needed for the STABLE versions they have to be specifically requested, otherwise I won't work on them.

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