Sign in to follow this  
Popstar

x86 callfunc bug (with fix)

Recommended Posts

Popstar    122
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;

	}

Share this post


Link to post
Share on other sites
Deyja    920
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.

Share this post


Link to post
Share on other sites
Popstar    122
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.

Share this post


Link to post
Share on other sites
Deyja    920
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.

Share this post


Link to post
Share on other sites
WitchLord    4678
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

Share this post


Link to post
Share on other sites
Popstar    122
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.

Share this post


Link to post
Share on other sites
WitchLord    4678
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

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this