556149 - Math.sin() and Math.cos() return incorrect results for NaN and infinities on iPhone

Status: VERIFIED FIXED

(Tamarin Graveyard :: Virtual Machine, defect, P3)

Description

[Stan Switzer]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.8) Gecko/20100202 Firefox/3.5.8
Build Identifier: Tamarin Redux, March '10

The runtime libraries on the iPhone return incorrect values for sin() and cos() of NaN and infinites.

The problem appears to be limited to those library routines, tan, exp, and the arc-etc routines seem to behave correctly.

The problem is reproducible only on the actual device and not in the simulator.

Reproducible: Always

Steps to Reproduce:
Run avmshell tests e15_8_2_7 (cos) and e15_8_2_16 (sin) on the iPhone
Actual Results:  
Math.sin() and Math.cos() return meaningless values in these cases

Expected Results:  
should return NaN.

Watson BUG# 2471259
ATS test 9658

Comment 1

[Stan Switzer]

Attachment: Proposed patch

Patch corresponding to Perforce changelist 655292

Comment 2

[Edwin Smith]

Comment on attachment 436078 [details] [diff] [review]
Proposed patch

questions:
- follow VMCFG convention?
- should we have a platform dir for iphone?

Comment 3

[Stan Switzer]

Possibly, dunno... this is a runtime library specific bug workaround. There doesn't seem to be a convention for that, but I don't see this as lining up along a platform-qualities axis (VMCFG_UNALIGNED_INT_ACCESS) unless that would be something like VMCFG_SDK_HAS_J_RANDOM_BUG. Tying it to a platform in the ifdef makes the issue really easy to understand in the source code. One-stop shopping: "Oh! This SDK has that bug."

Duping the entire file to work around a (one hopes!) temporary bug seems like a bad precedent. "It's a UNIX system! I know this!"

Anyway, happy to take any specific guidance or if you'd like to change it when integrating, that'd be cool too.  But if the ifdef changes we'll need to know because we have to be sure to define it.

Comment 4

[William Maddox]

This looks reasonable if in fact the bug is strictly in the handling 
of NaNs and infinities.  Please verify that correct results are given
for other values, including large ones.  There is a bug on WinMo that
is worked around in MathUtilsWin.cpp, and might possibly be related to
the ARM processor or to vendor-supplied libraries.

Comment 5

[Steven Johnson]

Comment on attachment 436078 [details] [diff] [review]
Proposed patch

Conceptually this is fine, but AVMSYSTEM_IPHONE isn't defined anywhere. To date we've been using "AVMSYSTEM_MAC && AVMSYSTEM_ARM" to detect CocoaTouch targets.

Comment 6

[Steven Johnson]

From offline discussion, it sounds like "MAC && ARM" isn't the ideal detection strategy for iPhone; Stan is going to resubmit a patch that includes an explicit AVMSYSTEM_IPHONE (or some such) in avmfeatures.as

Comment 7

[Steven Johnson]

It's worth adding that there is almost certainly code in VM/Flash/AIR that assumes that the various "_MAC" compile flags are set for iPhone/CocoaTouch targets; thus I'm skeptical as to whether "AVMSYSTEM_IPHONE" could/should be added as a toplevel config, or as a subtype of mac...

Comment 8

[Lars T Hansen]

(In reply to comment #3)
> this is a runtime library specific bug workaround. There
> doesn't seem to be a convention for that, but I don't see this as lining up
> along a platform-qualities axis (VMCFG_UNALIGNED_INT_ACCESS) unless that would
> be something like VMCFG_SDK_HAS_J_RANDOM_BUG. Tying it to a platform in the
> ifdef makes the issue really easy to understand in the source code. One-stop
> shopping: "Oh! This SDK has that bug."

Well... the SDK has that bug in that version.

A refinement of the feature system I've been hoping to introduce for a while now is a "tweaks" system.  Tweaks are features that have sane default values and that apply to exactly these kinds of problems; the point is just to expose the fact that a tweak is available and not hide it deep in a file with an #ifdef _IPHONE.

Eg,

  <feature>
    <name>AVMTWEAK_IPHONE_HW_SIN_COS_NAN_BUG</name>
    <default>false</default>
    <desc> Iphone hardware version x, y, z have bug in sin and cos where
           they do not handle NaN properly and we must work around it in
           software. </desc>
    ...
  </feature>

would be false everywhere by default (so config files would not have to manually record the setting) but would be set to true for iphone builds.  (I'm not 100% sure that a default setting is a good idea, but the high bit of value for tweaks is the exposure in avmfeatures.as probably.)

Possibly it is too heavyweight a change to do something about this for 10.1 at this point, even though most of the changes would be in the avmfeatures.as script, not in the C++ code itself.

Comment 9

[Lars T Hansen]

Attachment: Patch

Same functionality with a different #ifdef based on a newly introduced tweak; requires the presence of the tweaks system of course.  See blocking bug.

Comment 10

[Lars T Hansen]

tamarin-redux-argo changeset:   3945:aaea0de6a778

Comment 11

[Chris Peyer]

Verified fixed per Stan Switzer