Bug 15416 - dtoa Falls into infinite loop on ARMEL
Summary: dtoa Falls into infinite loop on ARMEL
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 523.x (Safari 3)
Hardware: Other All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 17923
  Show dependency treegraph
 
Reported: 2007-10-07 20:47 PDT by wesleyZeng
Modified: 2008-09-03 06:38 PDT (History)
3 users (show)

See Also:


Attachments
for mixed-endian ARM processor (3.09 KB, patch)
2008-02-25 17:25 PST, wesleyZeng
no flags Details | Formatted Diff | Diff
fix mixed-endian (3.70 KB, patch)
2008-02-28 23:55 PST, wesleyZeng
ddkilzer: review-
Details | Formatted Diff | Diff
fix mixed-endian (3.16 KB, patch)
2008-03-09 19:17 PDT, wesleyZeng
no flags Details | Formatted Diff | Diff
fix mixed-endian (1.79 KB, patch)
2008-03-09 23:07 PDT, wesleyZeng
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description wesleyZeng 2007-10-07 20:47:43 PDT
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"
Comment 1 Mark Rowe (bdash) 2007-10-08 00:06:34 PDT
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?
Comment 2 wesleyZeng 2008-02-25 17:25:35 PST
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.
Comment 3 David Kilzer (:ddkilzer) 2008-02-25 17:51:52 PST
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

Comment 4 wesleyZeng 2008-02-28 23:55:39 PST
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.
Comment 5 David Kilzer (:ddkilzer) 2008-02-29 03:21:16 PST
Comment on attachment 19444 [details]
fix mixed-endian

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

>+2008-02-28  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.
Comment 6 wesleyZeng 2008-03-09 19:17:07 PDT
Created attachment 19624 [details]
fix mixed-endian

fix mixed-endian
Comment 7 Darin Adler 2008-03-09 19:55:39 PDT
Comment on attachment 19624 [details]
fix mixed-endian

Why did you add the USE_LOCALE code? This change is not appropriate for JavaScriptCore.
Comment 8 wesleyZeng 2008-03-09 23:07:08 PDT
Created attachment 19628 [details]
fix mixed-endian

fix mixed-endian
Comment 9 wesleyZeng 2008-03-09 23:08:35 PDT
ok, I remove USE_LOCAL and submit attachment again.
Comment 10 David Krause 2008-03-16 12:17:10 PDT
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?
Comment 11 Darin Adler 2008-03-16 15:24:09 PDT
Comment on attachment 19628 [details]
fix mixed-endian

Looks fine, at least harmless to non-middle-endian platforms. r=me
Comment 12 Darin Adler 2008-03-16 20:27:11 PDT
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).]
Comment 13 David Krause 2008-03-16 20:49:33 PDT
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.
Comment 14 David Kilzer (:ddkilzer) 2008-03-17 18:56:01 PDT
(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>

Comment 15 David Krause 2008-03-18 06:42:30 PDT
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.
Comment 16 David Kilzer (:ddkilzer) 2008-03-18 12:46:17 PDT
(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!

Comment 17 David Kilzer (:ddkilzer) 2008-03-18 15:16:32 PDT
(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.

Comment 18 Martin Guy 2008-09-03 01:46:04 PDT
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!
Comment 19 David Kilzer (:ddkilzer) 2008-09-03 06:38:03 PDT
(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!