Bug 15416 – dtoa Falls into infinite loop on ARMEL

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[wesleyZeng]

predefined marco in arm-linux-cpp

#define __DBL_MIN_EXP__ (-1021)
#define __FLT_MIN__ 1.17549435e-38F
#define __CHAR_BIT__ 8
#define __WCHAR_MAX__ 2147483647
#define __DBL_DENORM_MIN__ 4.9406564584124654e-324
#define __FLT_EVAL_METHOD__ 0
#define __unix__ 1
#define unix 1
#define __SIZE_TYPE__ unsigned int
#define __ELF__ 1
#define __DBL_MIN_10_EXP__ (-307)
#define __FINITE_MATH_ONLY__ 0
#define __ARMEL__ 1
#define __FLT_RADIX__ 2
#define __LDBL_EPSILON__ 2.2204460492503131e-16L
#define __ARM_ARCH_4__ 1
#define __SHRT_MAX__ 32767
#define __LDBL_MAX__ 1.7976931348623157e+308L
#define __linux 1
#define __CHAR_UNSIGNED__ 1
#define __LDBL_MAX_EXP__ 1024
#define __LONG_MAX__ 2147483647L
#define __linux__ 1
#define __unix 1
#define __SCHAR_MAX__ 127
#define __DBL_DIG__ 15
#define __USER_LABEL_PREFIX__
#define linux 1
#define __STDC_HOSTED__ 1
#define __LDBL_MANT_DIG__ 53
#define __FLT_EPSILON__ 1.19209290e-7F
#define __APCS_32__ 1
#define __LDBL_MIN__ 2.2250738585072014e-308L
#define __WCHAR_TYPE__ long int
#define __FLT_DIG__ 6
#define __FLT_MAX_10_EXP__ 38
#define __INT_MAX__ 2147483647
#define __gnu_linux__ 1
#define __FLT_MAX_EXP__ 128
#define __DECIMAL_DIG__ 17
#define __DBL_MANT_DIG__ 53
#define __WINT_TYPE__ unsigned int
#define __LDBL_MIN_EXP__ (-1021)
#define __arm__ 1
#define __LDBL_MAX_10_EXP__ 308
#define __DBL_EPSILON__ 2.2204460492503131e-16
#define __DBL_MAX__ 1.7976931348623157e+308
#define __USING_SJLJ_EXCEPTIONS__ 1
#define __DBL_MAX_EXP__ 1024
#define __FLT_DENORM_MIN__ 1.40129846e-45F
#define __LONG_LONG_MAX__ 9223372036854775807LL
#define __FLT_MAX__ 3.40282347e+38F
#define __GXX_ABI_VERSION 102
#define __FLT_MIN_10_EXP__ (-37)
#define __FLT_MIN_EXP__ (-125)
#define __DBL_MAX_10_EXP__ 308
#define __LDBL_DENORM_MIN__ 4.9406564584124654e-324L
#define __DBL_MIN__ 2.2250738585072014e-308
#define __PTRDIFF_TYPE__ int
#define __LDBL_MIN_10_EXP__ (-307)
#define __REGISTER_PREFIX__
#define __LDBL_DIG__ 15
#define __NO_INLINE__ 1
#define __FLT_MANT_DIG__ 24
#define __VERSION__ "3.3.2"

[Mark Rowe (bdash)]

I'm not sure what the purpose of including the predefined macros was.  Perhaps more useful would be determining where in the infinite loop is occurring?

[wesleyZeng]

Created attachment 19364 [details]
for mixed-endian ARM processor

#define IEEE_ARM for IEEE-arithmetic machines where the two words in a double are stored in big endian order but the two shorts in a word are still stored in little endian order.

[David Kilzer (:ddkilzer)]

If you'd like this patch reviewed, please set the "review?" flag.

You will need a ChangeLog entry before this patch may land.  Please see this page for more details:

http://webkit.org/coding/contributing.html

[wesleyZeng]

Created attachment 19444 [details]
fix mixed-endian

for mixed-endian ARM processor

#define IEEE_ARM for IEEE-arithmetic machines where the two words in a double
are stored in big endian order but the two shorts in a word are still stored in
little endian order.

[David Kilzer (:ddkilzer)]

Comment on attachment 19444 [details]
fix mixed-endian

Please set the "review?" flag, not the "review+" flag, to have your patch reviewed.

>[phone]  weihongzeng  <set EMAIL_ADDRESS environment variable>

Please set your first/last name (not just your account) and your email address in the ChangeLog file.

>+        Reviewed by NOBODY (OOPS!).
>+
>+        * kjs/dtoa.cpp:
>+        (Bigint::):
>+        (Bigint::freedtoa):

Please refer to this bug in the ChangeLog entry, and describe what you fixed.  (See other ChangeLog entries for examples.)

>+#ifdef USE_LOCALE
>+	s1 = localeconv()->decimal_point;
>+	if (c == *s1) {
>+		c = '.';
>+		if (*++s1) {
>+			s2 = s;
>+			for(;;) {
>+				if (*++s2 != *s1) {
>+					c = 0;
>+					break;
>+					}
>+				if (!*++s1) {
>+					s = s2;
>+					break;
>+					}
>+				}
>+			}
>+		}
>+#endif

This is incorrect style.  Curly braces appear below their corresponding if statement.  See this page for more information:

http://webkit.org/coding/coding-style.html

>-						while (*--s == '0') { }
>+						while(*--s == '0');

>-		while (*--s == '0') { }
>+		while(*--s == '0');

Why were these changes made?  I recall that some compilers require the empty block instead of a semi-colon.  These seem unnecessary.

r- for code style issues and incomplete ChangeLog.

[wesleyZeng]

Created attachment 19624 [details]
fix mixed-endian

fix mixed-endian

[Darin Adler]

Comment on attachment 19624 [details]
fix mixed-endian

Why did you add the USE_LOCALE code? This change is not appropriate for JavaScriptCore.

[wesleyZeng]

Created attachment 19628 [details]
fix mixed-endian

fix mixed-endian

[wesleyZeng]

ok, I remove USE_LOCAL and submit attachment again.

[David Krause]

I'm working on recent WebKit on a standard ARM platform (little-endian), and this bug is also affecting us.  The patch resolves our issue.  The comment on line 287 in the latest patch isn't quite accurate (should also mention ARM).

Is there anything else to be done to get this patch landed?

[Darin Adler]

Comment on attachment 19628 [details]
fix mixed-endian

Looks fine, at least harmless to non-middle-endian platforms. r=me

[Darin Adler]

Committed revision 31088.

[But I don't see how this could help for little-endian ARM, since the patch is triggered by PLATFORM(MIDDLE_ENDIAN).]

[David Krause]

I think it helps us because the defines in JavaScriptCore/wtf/Platform.h for ARM set either WTF_PLATFORM_BIG_ENDIAN or WTF_PLATFORM_MIDDLE_ENDIAN - there is no little endian define.  Maybe I'm mis-using my terms and we are "middle-endian", but regular 32-bit numbers are little-endian.  The platform has an ARM9 running Linux.

[David Kilzer (:ddkilzer)]

(In reply to comment #13)
> I think it helps us because the defines in JavaScriptCore/wtf/Platform.h for
> ARM set either WTF_PLATFORM_BIG_ENDIAN or WTF_PLATFORM_MIDDLE_ENDIAN - there is
> no little endian define.  Maybe I'm mis-using my terms and we are
> "middle-endian", but regular 32-bit numbers are little-endian.  The platform
> has an ARM9 running Linux.

Fixed in r31115.

<http://trac.webkit.org/projects/webkit/changeset/31115>

[David Krause]

The last patch (r31115) may not provide correct behavior for all ARM platforms. 

It seems that __ARMEL__ isn't enough to define the layout of the floating-point doubles.  On the particular platform we're working on, the layout does actually match that shown by MIDDLE_ENDIAN in JavaScriptCore/kjs/vaue.cpp, even though __ARMEL__ is defined.

Not an expert on the different ARM floating-point layouts (FPA vs. VFP), but the actual arrangement of the bytes varies - FPA is the "middle endian" format as it's called by WebKit, and VFP is "natural endian".

In our particular case, we still want MIDDLE_ENDIAN defined to get the proper behavior in the two places it's used (value.cpp and dtoa.cpp) since we are using FPA.  Some other searching turns up that the pre-processor define of __SOFTFP__  might be good enough to trigger that.  Our arm-linux-gcc does defines that, but the original bug report didn't have that or __VFP_FP__ defined.

If a change is warranted at all in Platform.h, having only our compiler here, I'd use __SOFTFP__ and __ARMEL__ to enable the WTF_PLATFORM_MIDDLE_ENDIAN define, but I'm not sure if that will break for those using VFP.  Maybe this at line 151 in Platform.h:

#elif !defined(__ARM_EABI__) && !defined(__ARMEB__) && defined(__ARMEL__) && defined(__SOFTFP__)

Backing out the change in Platform.h would also likely work, and not affect anyone that wasn't already having issues.

Sorry if my previous post (#13) led to confusion, but the net result is that the r31115 change will probably break ARM systems using FPA.

[David Kilzer (:ddkilzer)]

(In reply to comment #15)
> The last patch (r31115) may not provide correct behavior for all ARM platforms. 

Thanks for the follow-up, David!  Could you please file a new bug to track this issue and copy me on it?  Thanks!

[David Kilzer (:ddkilzer)]

(In reply to comment #16)
> Thanks for the follow-up, David!  Could you please file a new bug to track this
> issue and copy me on it?  Thanks!

Bug 17923.

[Martin Guy]

Hi!
   I found this bug report while searching for something else. I had to wrangle ARM FP variants over the last few years for wiki.debian.org/ArmEabiPort so can shed some light...
   Unfortunately there is no single define to isolate the bizarre 45670123-order doubles as used in ancient ARM FPA and the old-abi FP emulator, but to the code in Platform.h:

#elif !defined(__ARM_EABI__) && !defined(__ARMEB__) && !defined(__VFP_FP__)
#define WTF_PLATFORM_MIDDLE_ENDIAN 1

you might add && !defined(__MAVERICK__), which is the other true-little-endian FP coprocessor from Cirrus. __VFP_FP__ is also defined if soft-float is in use, so that case is alrady covered correctly by this code.

Also, the following phrase
#if !defined(__ARM_EABI__)
#define WTF_PLATFORM_FORCE_PACK 1
#endif
is suspect, since ARMs require data accesses to be aligned to a multiple of their size for 2- and 4-byte words regardless of ABI; the only difference is that in EABI, doubles should be 8-byte-aligned instead of 4.
However WTF_PLATFORM_FORCE_PACK is not used anywhere in the rest of WebKit, so it looks redundant now.

Cheers!

[David Kilzer (:ddkilzer)]

(In reply to comment #18)
>    I found this bug report while searching for something else. I had to wrangle
> ARM FP variants over the last few years for wiki.debian.org/ArmEabiPort so can
> shed some light...

Martin, please open a new bug with a patch if you'd like to see these changes made.  Thanks!