If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

ARM: Math failures running with THUMB or ARM jit and interp.

VERIFIED FIXED in flash10.1

Status

Tamarin
Virtual Machine
P3
normal
VERIFIED FIXED
10 years ago
8 years ago

People

(Reporter: Brent Baker, Assigned: Rick Reitmaier)

Tracking

({flashplayer})

unspecified
flash10.1
Other
Windows Mobile 6 Standard
flashplayer
Dependency tree / graph
Bug Flags:
in-testsuite +
wanted-flashplayer10 +
flashplayer-qrb +
flashplayer-triage +

Details

Attachments

(1 attachment)

(Reporter)

Description

10 years ago
The are a handful of math failures when running with either the ARM or THUMB jit.

ecma3/Math/e15_8_2_17_1.as
Math.sqrt(3) = 1.7320508075688774 FAILED! expected: 1.7320508075688772
Math.sqrt(3) = 1.73205080756887742 FAILED! expected: 1.73205080756887719
Math.sqrt(3) = 1.732050807568877415 FAILED! expected: 1.732050807568877193
Math.sqrt(3) = 1.7320508075688774152 FAILED! expected: 1.7320508075688771932

ecma3/Math/e15_8_2_5.as
Math.atan2(0, -0) = 0 FAILED! expected: 3.141592653589793
Infinity/Math.atan2(-0, 1) = Infinity FAILED! expected: -Infinity
Math.atan2(-0,       -0) = 0 FAILED! expected: -3.141592653589793
Math.atan2(-0,       -1) = 3.141592653589793 FAILED! expected: -3.141592653589793
Math.atan2(Infinity, Infinity) = NaN FAILED! expected: 0.7853981633974483
Math.atan2(Infinity, -Infinity) = NaN FAILED! expected: 2.356194490192345
Math.atan2(-Infinity, Infinity) = NaN FAILED! expected: -0.7853981633974483
Math.atan2(-Infinity, -Infinity) = NaN FAILED! expected: -2.356194490192345

Comment 1

9 years ago
The ARM versions of atan2 don't differentiate between +0 and -0. We'd need to add some code to check for these conditions before calling atan2. e.g.:

	bool xnan = (((int*)(&x))[1] & 0x7FF80000) == 0x7FF80000;
	bool ynan = (((int*)(&y))[1] & 0x7FF80000) == 0x7FF80000;
		
	if (xnan || ynan)
	{
		return nan();
	}

	int sx = (((int*)(&x))[1] & 0x80000000);
	int sy = (((int*)(&y))[1] & 0x80000000);
		
	if (y == 0.0)
	{
		if (sy) // y is -0
		{
			if (sx) // x is <0 or -0
			{
				return -KPi;
			}
			else // x is >0 or +0
			{
				return -0.0;
			}
		}
		else // y is +0
		{
			if (sx) // x is <0 or -0
			{
				return KPi;
			}
			else // x is >0 or +0
			{
				return +0.0;
			}
		}
	}
	return ::atan2(y, x);

Updated

9 years ago
Assignee: nobody → lhansen

Comment 2

9 years ago
Brent/Rob, this is marked as pertaining to TT; is that correct?  Even in TR/TC the testconfig.txt file has special handling of these tests.  Should I change the component?
(Reporter)

Comment 3

9 years ago
This now applies to TC/TR as there is an arm version which came from TT. I have updated the component to "Virtual Machine".
Assignee: lhansen → nobody
Component: Tracing Virtual Machine → Virtual Machine
Flags: wanted-flashplayer10+
Flags: in-testsuite+
Flags: flashplayer-triage+
Flags: flashplayer-qrb?
Keywords: flashplayer
QA Contact: tracing-vm → vm

Updated

9 years ago
Assignee: nobody → lhansen
Status: NEW → ASSIGNED
Flags: flashplayer-qrb? → flashplayer-qrb+
Priority: -- → P3
Target Milestone: --- → flash10.x

Updated

9 years ago
Blocks: 478870
(Assignee)

Comment 4

9 years ago
seeing these failures...reassign to me since i'm taking on the other arm issues; unless Lars has a ready fix.

BTW, I'd like to rename platform/arm/MathUtilsUnix to platform/arm/MathUtilsARM, but will check out the VMPI_ changes first.
Assignee: lhansen → rreitmai
(Assignee)

Comment 5

9 years ago
Created attachment 366704 [details] [diff] [review]
ver 1 - atan2 matches ecma spec for winmo

This is a winmo only fix.  If other platforms require this change then we need to log a new bug or annotate this one.  

Also, this does not modify the sqrt failures, which according to ecma docs are allowable.  

"Returns an implementation-dependent approximation to the square root of x."

We need to attach another patch to address this issue with the test suite.
Attachment #366704 - Flags: review?(rob.borcic)

Comment 6

9 years ago
(In reply to comment #5)
> Created an attachment (id=366704) [details]
>
> This is a winmo only fix.  If other platforms require this change then we need
> to log a new bug or annotate this one.  

I will do so, because the mathutils stuff looks like it could probably do with a little cleaning up, at a minimum.  (For example, X86_MATH doesn't mean that at all, it means IA32 && Visual Studio; for another example, everyone not using that pays for a double function call to get to the math functions, but that shouldn't be necessary.  But this bug shouldn't have to fix that.)
(Assignee)

Updated

9 years ago
Attachment #366704 - Flags: review?(rwinchel)

Comment 7

9 years ago
Comment on attachment 366704 [details] [diff] [review]
ver 1 - atan2 matches ecma spec for winmo


>+    const static double PI = 3.141592653589793;
>+    const static double PI3_BY_4 = 3*PI/4;
>+    const static double PI_BY_4 = PI/4;

Any reason not to define PI3_BY_4 and PI_BY_4 as constant values? Avoid the floating point divide (expensive).
(Assignee)

Comment 8

9 years ago
We could, I was concerned that the compiler might produce a different result than the runtime, but this is probably a bit of paranoia on my part.  I'll move them.

Comment 9

9 years ago
Yow, can't asc do this optimization at compile-time? I would presume/hope that the language would allow for this...

Comment 10

9 years ago
More than that, you're looking at C++ code here and -O should do the trick :-)

Comment 11

9 years ago
doh! that was a doumb mistake on my part :-/

Comment 12

9 years ago
Comment on attachment 366704 [details] [diff] [review]
ver 1 - atan2 matches ecma spec for winmo

Your patch would handle the condition I found on Symbian using ARM RVCT 2.2.
Attachment #366704 - Flags: review?(rob.borcic) → review+
(Assignee)

Comment 13

9 years ago
pushed as 085ae23e9c68: bugfix #429673 - special case for winmo atan2 support (r+=robb) - rev 1590

btw, sqrt() issues will be addressed in test suite, since ecma spec states that result is dependent on implementation.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
(Assignee)

Updated

9 years ago
Blocks: 473392
(Reporter)

Comment 14

9 years ago
Removed the expected fail for ecma3/Math/e15_8_2_5.as, pushed as 1596:74e23e3686ef

Rick what should be done for the sqrt() "failures", is the current result from windows mobile an acceptable value?
(Assignee)

Comment 15

9 years ago
Yes it is.  I sent an earlier email regarding this issue. Any way the test suite can be tweaked to ignore the round-off difference?
(Assignee)

Updated

9 years ago
Attachment #366704 - Flags: review?(rwinchel)

Comment 16

8 years ago
Resolved fixed engineering / work item that has been pushed.  Setting status to verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.