Closed Bug 517999 Opened 10 years ago Closed 10 years ago

Some floating point values crash VFP enabled CPUs

Categories

(Core :: JavaScript Engine, defect)

ARM
Windows Mobile 6 Standard
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- beta1-fixed

People

(Reporter: dougt, Assigned: dougt)

References

Details

Attachments

(3 files)

For example, the following code causes an illegal instruction on a Samsung S3c6410 (ARM1176).  This is the processor in the Samsung Omnia 2 phone.


	int x[2], y[2];

	x[0] = 0x54d39020;
	x[1] = 0x00000009;
	y[0] = 0x1fab884c;
	y[1] = 0x588621e0;

	double a = *(double*)&x[0];
	double b = *(double*)&y[0];
	
	if (a < b)                        <---- crash here
		printf("aa\n");
	else
		printf("ba\n");


Using either a or b results in a crash.
asm:

	int x[2], y[2];

	x[0] = 0x54d39020;
00011010  ldr         r3, [pc, #0x10C] 
00011014  str         r3, x, #0x10 
	x[1] = 0x00000009;
00011018  mov         r3, #9 
0001101C  str         r3, [sp, #0x14] 
	y[0] = 0x1fab884c;
00011020  ldr         r3, [pc, #0xF8] 
00011024  str         r3, y, #8 
	y[1] = 0x588621e0;
00011028  ldr         r3, [pc, #0xEC] 
0001102C  str         r3, [sp, #0xC] 

	double a = *(double*)&x[0];
00011030  fldd        d0, [sp,#+16] 
00011034  fstd        d0, [sp] 
	double b = *(double*)&y[0];
00011038  fldd        d0, [sp,#+8] 
0001103C  fstd        d0, [sp,#+56] 
	
	if (a < b)
00011040  fldd        d1, [sp] 
00011044  fldd        d0, [sp,#+56] 
00011048  fcmpd       d1, d0 
0001104C  fmrx        pc,fpscr                            <-- crash
00011050  bge         |WinMain + 0x60 ( 11060h )| 
		printf("aa\n");
00011054  ldr         r0, [pc, #0xBC] 
00011058  bl          000117E4 
	else
0001105C  b           |WinMain + 0x68 ( 11068h )| 
		printf("ba\n");
On windows, you must disable fp emulation:

/QRfpe-
Attached file testcase
Here's that testcase in a C file.

This works fine for me on a Tegra 600, which is also ARMv6-based.  It could well be that there are VFP bugs wither with WinMo6.1 or with the Samsung implementation; the safest thing here might be to just disable VFP support for WinMo entirely, and then reevaluate that once WinMo 6.5 is out and we can check again.

There probably isn't much else using VFP on WinMo, since most devices don't support it; parts of the OS/system libs probably use it, but in a much more constrained environment.
kanth, any insight here -- can user applications use VFP?
OEM required fix?

http://support.microsoft.com/kb/960413

From a comment:

Also note that since CE6 does not support the VFP coprocessor, you'll need
to ensure that your OAL code sets it up to be in RunFast mode.  Otherwise
whenever an exception or bounced instruction occurs, the OS will kill your
process.

A blanket #ifdef winmo is terrible.  I wonder if there is a better test we can use?  Eg. maybe we should use the test case above to determine if there is VFP support?
(In reply to comment #5)
> OEM required fix?
> 
> http://support.microsoft.com/kb/960413
> 
> From a comment:
> 
> Also note that since CE6 does not support the VFP coprocessor, you'll need
> to ensure that your OAL code sets it up to be in RunFast mode.  Otherwise
> whenever an exception or bounced instruction occurs, the OS will kill your
> process.
> 
> A blanket #ifdef winmo is terrible.  I wonder if there is a better test we can
> use?  Eg. maybe we should use the test case above to determine if there is VFP
> support?


Can we just disable floating point exceptions using FPSCR?

I can try to whip up a test case that does this tomorrow.
No, that's not the problem -- if that code wasn't present, then the testcase I had for a related problem on CE6 would have failed.  We're not getting a floating point exception here, but an illegal instruction exception (which is the only type of exception that FMSTAT is listed as being able to raise, and then only when it's not actually present/supported).
Is this a spidermonkey bug?  It seems like it's perhaps an OS bug, or worse a processor issue.  If you can trigger it with legal C, it's hard to see how it's the JIT's fault. :-)
It's likely not directly, but if we can understand it better we may be able to work around it.
Attachment #402632 - Flags: review?(gal)
Comment on attachment 402632 [details] [diff] [review]
test in js_arm_try_vfp_op

I have absolutely no idea what this does. I will r+ this so you can push this based on the comment in the patch and ask Jacob for a secondary review.
Attachment #402632 - Flags: review?(gal)
Attachment #402632 - Flags: review?(Jacob.Bramley)
Attachment #402632 - Flags: review+
We don't know for sure that the problem is no sw support, so at the very least we shouldn't say that.  I don't really like having this check in here; it doesn't make any sense to me.  I would rather us just unconditionally disable VFP on Windows Mobile.
does the check not make sense, or having TM produce VFP code when the rest of the app is using software emulation, or something else?
Comment on attachment 402632 [details] [diff] [review]
test in js_arm_try_vfp_op

Based on the discussion earlier in this thread, the patch looks right. The only niggle is that elsewhere in the file, DCD is written in capitals, probably because it's considered a directive rather than an instruction.
Attachment #402632 - Flags: review?(Jacob.Bramley) → review+
(In reply to comment #5)
> [...] ensure that your OAL code sets it up to be in RunFast mode.  Otherwise
> whenever an exception or bounced instruction occurs, the OS will kill your
> process.

You probably already know, but we can't do this here. RunFast mode isn't completely IEEE 754 compliant, and we need that to meet the JavaScript specifications, even though most real-world applications won't care. Details: http://infocenter.arm.com/help/topic/com.arm.doc.ddi0301g/Chdiibig.html

(In reply to comment #12)
> We don't know for sure that the problem is no sw support, so at the very least
> we shouldn't say that.  I don't really like having this check in here; it
> doesn't make any sense to me.  I would rather us just unconditionally disable
> VFP on Windows Mobile.

That is a good point. Perhaps we should wait for an outcome to bug 510340 before pushing this patch. (My r+ still stands in that the patch will do what you wanted it to do and shouldn't break anything else, but I don't know whether we want it to do that or not!)

Unconditionally disabling VFP on Windows is the _safe_ way forward, but we'll pay a performance penalty for doing that. Because this is temporary, should we just do as Vlad suggests, and disable VFP for Windows, then re-enable it once we've resolved the underlying issue?
note that I had to add a new configure variable.
Attachment #402865 - Flags: review?(gal)
Attachment #402865 - Flags: review?(gal) → review+
Comment on attachment 402865 [details] [diff] [review]
patch to disable VFP on windows mobile

We should document this somewhere on the wiki. No clue what the right place is.

>diff --git a/js/src/config/autoconf.mk.in b/js/src/config/autoconf.mk.in
>--- a/js/src/config/autoconf.mk.in
>+++ b/js/src/config/autoconf.mk.in
>@@ -330,16 +330,17 @@ MSMANIFEST_TOOL = @MSMANIFEST_TOOL@
> MSMANIFEST_TOOL = @MSMANIFEST_TOOL@
> WIN32_REDIST_DIR = @WIN32_REDIST_DIR@
> MOZ_MEMORY_LDFLAGS = @MOZ_MEMORY_LDFLAGS@
> 
> # Codesighs tools option, enables win32 mapfiles.
> MOZ_MAPINFO	= @MOZ_MAPINFO@
> 
> WINCE		= @WINCE@
>+WINCE_WINDOWS_MOBILE = @WINCE_WINDOWS_MOBILE@
> 
> MACOS_SDK_DIR	= @MACOS_SDK_DIR@
> NEXT_ROOT	= @NEXT_ROOT@
> GCC_VERSION	= @GCC_VERSION@
> XCODEBUILD_VERSION= @XCODEBUILD_VERSION@
> HAS_XCODE_2_1	= @HAS_XCODE_2_1@
> UNIVERSAL_BINARY= @UNIVERSAL_BINARY@
> HAVE_DTRACE= @HAVE_DTRACE@
>diff --git a/js/src/configure.in b/js/src/configure.in
>--- a/js/src/configure.in
>+++ b/js/src/configure.in
>@@ -394,16 +394,17 @@ if test "$GCC" = yes; then
>    fi
> fi
> 
> if test "$GXX" = yes; then
>    if test "`$CXX -help 2>&1 | grep -c 'Intel(R) C++ Compiler'`" != "0"; then
>      INTEL_CXX=1
>    fi
> fi
>+
> 

Seems unnecessary.


> dnl Special win32 checks
> dnl ========================================================
> case "$target" in
> *-wince)
>     WINVER=500
>     ;;
> *)
>@@ -1860,16 +1861,29 @@ case "$target" in
>     AC_DEFINE(WIN32_LEAN_AND_MEAN)
> 
>     TARGET_MD_ARCH=win32
>     _PLATFORM_DEFAULT_TOOLKIT='windows'
>     BIN_SUFFIX='.exe'
>     USE_SHORT_LIBNAME=1
>     MOZ_ENABLE_POSTSCRIPT=
>     MOZ_USER_DIR="Mozilla"
>+
>+    dnl Default to Windows Mobile components enabled
>+    WINCE_WINDOWS_MOBILE=1
>+
>+    MOZ_ARG_DISABLE_BOOL(windows-mobile-components,
>+    [  --disable-windows-mobile-components
>+         Disable Windows Mobile specific components from CE build],
>+    WINCE_WINDOWS_MOBILE=,
>+    WINCE_WINDOWS_MOBILE=1)
>+
>+    if test "$WINCE_WINDOWS_MOBILE"; then
>+        AC_DEFINE(WINCE_WINDOWS_MOBILE)
>+    fi
> ;;
> 
> *-symbian*)
> 
>     AC_DEFINE(XP_UNIX)
>     AC_DEFINE(SYMBIAN)
>     AC_DEFINE(__arm__)
>     AC_DEFINE(__SYMBIAN32__)
>@@ -4930,16 +4944,17 @@ AC_SUBST(MOZ_BUILD_ROOT)
> AC_SUBST(MOZ_BUILD_ROOT)
> AC_SUBST(MOZ_OS2_TOOLS)
> AC_SUBST(MOZ_OS2_USE_DECLSPEC)
> 
> AC_SUBST(MOZ_POST_DSO_LIB_COMMAND)
> AC_SUBST(MOZ_POST_PROGRAM_COMMAND)
> AC_SUBST(MOZ_TIMELINE)
> AC_SUBST(WINCE)
>+AC_SUBST(WINCE_WINDOWS_MOBILE)
> 
> AC_SUBST(MOZ_APP_NAME)
> AC_SUBST(MOZ_APP_DISPLAYNAME)
> AC_SUBST(MOZ_APP_VERSION)
> 
> AC_SUBST(MOZ_PKG_SPECIAL)
> 
> AC_SUBST(MOZILLA_OFFICIAL)
>diff --git a/js/src/jstracer.cpp b/js/src/jstracer.cpp
>--- a/js/src/jstracer.cpp
>+++ b/js/src/jstracer.cpp
>@@ -6296,24 +6296,28 @@ js_arm_check_arch() {
>         arch = 7;
>     } __except(GetExceptionCode() == EXCEPTION_ILLEGAL_INSTRUCTION) {
>     }
>     return arch;
> }
> 
> static bool
> js_arm_check_vfp() {
>+#ifdef WINCE_WINDOWS_MOBILE
>+    return false;
>+#else
>     bool ret = false;
>     __try {
>         js_arm_try_vfp_op();
>         ret = true;
>     } __except(GetExceptionCode() == EXCEPTION_ILLEGAL_INSTRUCTION) {
>         ret = false;
>     }
>     return ret;
>+#endif
> }
> 
> #define HAVE_ENABLE_DISABLE_DEBUGGER_EXCEPTIONS 1
> 
> /* See "Suppressing Exception Notifications while Debugging", at
>  * http://msdn.microsoft.com/en-us/library/ms924252.aspx
>  */
> static void
http://hg.mozilla.org/mozilla-central/rev/5dd538ac1f12
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Attachment #402865 - Flags: approval1.9.2?
Attachment #402865 - Flags: approval1.9.2? → approval1.9.2+
OS: Windows XP → Windows Mobile 6 Standard
Hardware: x86 → ARM
Depends on: 519185
Is "test in js_arm_try_vfp_op" obsolete here?
(In reply to comment #19)
> Is "test in js_arm_try_vfp_op" obsolete here?

Nope! Once we've solved the underlying problem, VFP support in WinCE should be turned back on, so we still need the feature detection support for it, even if the current code doesn't use it.
You need to log in before you can comment on or make changes to this bug.