Closed
Bug 289394
Opened 19 years ago
Closed 16 years ago
Double.cpp causes unaligned accesses
Categories
(Core :: XSLT, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.9beta3
People
(Reporter: davidm, Assigned: davidm)
Details
(Keywords: perf)
Attachments
(1 file, 4 obsolete files)
3.62 KB,
patch
|
sicking
:
review+
sicking
:
superreview+
beltzner
:
approval1.9+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (X11; U; Linux ia64; en-US; rv:1.7.6) Gecko/20050325 Firefox/1.0.2 (Debian package 1.0.2-1) Build Identifier: Mozilla/5.0 (X11; U; Linux ia64; en-US; rv:1.7.6) Gecko/20050325 Firefox/1.0.2 (Debian package 1.0.2-1) When running mozilla-firefox on ia64 linux messages along the lines of: firefox-bin(5320): unaligned access to 0x600000000008ff8c, ip=0x40000000017356a0 are being displayed, indicating that firefox is making unaligned memory accesses. Besides being unsightly, unaligned accesses are slow since they need to be fixed up in software by the kernel. The source of these unaligned accesses are the *Mask constants defined by Double.cpp. The patch below fixes them by defining the txdpun union unconditionally and then using this type rather than a 2-element array of type PRUint32 to define the various *Mask constants. This ensures that the masks are suitably aligned. With the patch applied, I don't see any unaligned accesses anymore in firefox. ---------------------------------- diff -urN mozilla-firefox-1.0.2/extensions/transformiix/source/base/Double.cpp mozilla-firefox-1.0.2-fixed/extensions/transformiix/source/base/Double.cpp --- mozilla-firefox-1.0.2/extensions/transformiix/source/base/Double.cpp 2004-01-15 13:23:18.000000000 -0800 +++ mozilla-firefox-1.0.2-fixed/extensions/transformiix/source/base/Double.cpp 2005-04-06 22:27:28.000000000 -0700 @@ -74,14 +74,7 @@ #define CPU_IS_ARM #endif -#if (__GNUC__ == 2 && __GNUC_MINOR__ > 95) || __GNUC__ > 2 -/** - * This version of the macros is safe for the alias optimizations - * that gcc does, but uses gcc-specific extensions. - */ - typedef union txdpun { - PRFloat64 d; struct { #if defined(IS_LITTLE_ENDIAN) && !defined(CPU_IS_ARM) PRUint32 lo, hi; @@ -89,8 +82,14 @@ PRUint32 hi, lo; #endif } s; + PRFloat64 d; } txdpun; +#if (__GNUC__ == 2 && __GNUC_MINOR__ > 95) || __GNUC__ > 2 +/** + * This version of the macros is safe for the alias optimizations + * that gcc does, but uses gcc-specific extensions. + */ #define TX_DOUBLE_HI32(x) (__extension__ ({ txdpun u; u.d = (x); u.s.hi; })) #define TX_DOUBLE_LO32(x) (__extension__ ({ txdpun u; u.d = (x); u.s.lo; })) @@ -116,15 +115,15 @@ //-- Initialize Double related constants #ifdef IS_BIG_ENDIAN -const PRUint32 nanMask[2] = {TX_DOUBLE_HI32_EXPMASK | TX_DOUBLE_HI32_MANTMASK, +const txdpun nanMask = {TX_DOUBLE_HI32_EXPMASK | TX_DOUBLE_HI32_MANTMASK, 0xffffffff}; -const PRUint32 infMask[2] = {TX_DOUBLE_HI32_EXPMASK, 0}; -const PRUint32 negInfMask[2] = {TX_DOUBLE_HI32_EXPMASK | TX_DOUBLE_HI32_SIGNBIT, 0}; +const txdpun infMask = {TX_DOUBLE_HI32_EXPMASK, 0}; +const txdpun negInfMask = {TX_DOUBLE_HI32_EXPMASK | TX_DOUBLE_HI32_SIGNBIT, 0}; #else -const PRUint32 nanMask[2] = {0xffffffff, +const txdpun nanMask[2] = {0xffffffff, TX_DOUBLE_HI32_EXPMASK | TX_DOUBLE_HI32_MANTMASK}; -const PRUint32 infMask[2] = {0, TX_DOUBLE_HI32_EXPMASK}; -const PRUint32 negInfMask[2] = {0, TX_DOUBLE_HI32_EXPMASK | TX_DOUBLE_HI32_SIGNBIT}; +const txdpun infMask[2] = {0, TX_DOUBLE_HI32_EXPMASK}; +const txdpun negInfMask[2] = {0, TX_DOUBLE_HI32_EXPMASK | TX_DOUBLE_HI32_SIGNBIT}; #endif const double Double::NaN = *((double*)nanMask); Reproducible: Always Steps to Reproduce: 1. Invoke "prctl --unaligned=default" to ensure unaligned accesses are being reported. 2. Invoke mozilla-firefox Actual Results: Get messages of the form: firefox-bin(5320): unaligned access to 0x600000000008ff8c, ip=0x40000000017356a0 Expected Results: Should see no output.
The change looks good though it looks like you're messing up the whitespace a little bit. Have you tested it against the testcase in bug 53518 (preferably on as many platforms as you can)? If so please attach a real patch so we can get it reviewed and checked in.
Assignee | ||
Comment 2•19 years ago
|
||
OK, it finally occurred to me that the browser might be blocking cookies and that's why I couldn't create an attachment for this bug-report...
Assignee | ||
Comment 3•19 years ago
|
||
(In reply to comment #1) > The change looks good though it looks like you're messing up the whitespace a > little bit. Have you tested it against the testcase in bug 53518 (preferably on > as many platforms as you can)? I have run this test on Debian/ia64 (sarge) now with the patched browser and it does pass all the tests. Thanks.
>+const txdpun nanMask[2] = {0xffffffff,
> TX_DOUBLE_HI32_EXPMASK | TX_DOUBLE_HI32_MANT...
Sorry, I should have been more clear about the whitespace problem. It'd simply
be nice if the first and the second line in the above lined up properly.
Also, shouldn't the [2] be removed there?
Assigning bug to you since it's your patch.
Assignee: peterv → davidm
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Comment 5•19 years ago
|
||
Sure, here is a revised patch which fixes the whitespace so the lines are aligned.
Assignee | ||
Comment 6•19 years ago
|
||
Argh, I should finish reading your message before posting an updated patch. I think this patch takes care of all the issues you mentioned. Thanks.
Attachment #179956 -
Attachment is obsolete: true
Attachment #179968 -
Attachment is obsolete: true
Attachment #179969 -
Flags: superreview?(bryner)
Attachment #179969 -
Flags: review+
Comment 7•19 years ago
|
||
There's still problems with this patch. When I apply it I get: Double.cpp:129: error: cannot convert `nanMask' from type `const txdpun' to type `double*' Double.cpp:130: error: cannot convert `infMask' from type `const txdpun' to type `double*' Double.cpp:131: error: cannot convert `negInfMask' from type `const txdpun' to type `double*' make[6]: *** [Double.o] Error 1 So removing the [2]'s broke something, I'm not understanding it deeply enough if it's correct or not.
Eric, does it help if you make those lines say const double Double::NaN = *((const double*)nanMask.d) ?
Comment 9•19 years ago
|
||
(In reply to comment #8) Perhaps this is silly, but is nanMask even a pointer? You want to convert from a const union to a double * just seems wrong to me. I must be missing something.
Doh, you're of course right. It should say const double Double::NaN = nanMask.d;
Comment 11•19 years ago
|
||
Well that worked, I didn't test it extensively but it looks good.
Eric, did you run the testcase in bug 53518? As long as that passes we should be fine.
Updated•18 years ago
|
Attachment #179969 -
Flags: superreview?(bryner) → superreview+
Comment 13•18 years ago
|
||
I don't have access to a ia64 to test this on personally, but Debian has some active ia64 users. I applied this patch over a year ago and I've had no complaints, so I think it's safe to assume this helps ia64 users.
Updated•18 years ago
|
Attachment #181384 -
Flags: review?
Attachment #181384 -
Flags: review? → review?(bugmail)
Eric: The tests in bug 53518 isn't something most users are going to run in to in normal day-to-day surfing. I would really like to get confirmation that this patch actually works before reviewing it. Could you ask some of the people that are using it if the testcase in that bug works? It's really easy to test.
Comment on attachment 181384 [details] [diff] [review] actually compiles Removing review request we get confirmation that it doesn't break anything.
Attachment #181384 -
Flags: review?(bugmail)
Comment 16•18 years ago
|
||
Comment on attachment 181384 [details] [diff] [review] actually compiles Confirming it helps ia64 as well. Been in use in gentoo for over a year.
Attachment #181384 -
Flags: review?(bugmail)
Comment 17•18 years ago
|
||
Comment on attachment 181384 [details] [diff] [review] actually compiles would someone please explain why it's a good idea to move this line: PRFloat64 d; around?
Comment 18•17 years ago
|
||
Confirming that the latest patch does fix the problems for ia64. As for timeless's question about moving around "PRFloat64 d": that has to do with the rules of how unions get initialized. Without moving member "d" to the end of the union, the compiler wouldn't like initializers of the form: const txdpun infMask = {TX_DOUBLE_HI32_EXPMASK, 0}; For example, G++ v4.1.2 would complain with "too many initializers", since it would attempt to assign TX_DOUBLE_HI32_EXPMASK to the PRFloat64 member.
Attachment #181384 -
Flags: review?(jonas) → review+
Comment 19•17 years ago
|
||
This is an update for trunk, where the file has been split in two, and another part showed up in the xforms extension. The double braces are necessary to avoid these warnings: txDouble.cpp:53: warning: missing braces around initializer for 'txdpun::<anonymous struct>' txDouble.cpp:58: warning: missing braces around initializer for 'txdpun::<anonymous struct>' txDouble.cpp:59: warning: missing braces around initializer for 'txdpun::<anonymous struct>'
Attachment #179969 -
Attachment is obsolete: true
Attachment #181384 -
Attachment is obsolete: true
Attachment #292222 -
Flags: review?(jonas)
Attachment #292222 -
Flags: superreview+
Attachment #292222 -
Flags: review?(jonas)
Attachment #292222 -
Flags: review+
Comment 20•16 years ago
|
||
Comment on attachment 292222 [details] [diff] [review] New patch for trunk Fix some unaligned access issues.
Attachment #292222 -
Flags: approval1.9?
Comment 21•16 years ago
|
||
Comment on attachment 292222 [details] [diff] [review] New patch for trunk a=beltzner for 1.9
Attachment #292222 -
Flags: approval1.9? → approval1.9+
Updated•16 years ago
|
Keywords: checkin-needed
QA Contact: keith → xslt
Comment 22•16 years ago
|
||
Checking in content/xslt/public/txDouble.h; /cvsroot/mozilla/content/xslt/public/txDouble.h,v <-- txDouble.h new revision: 1.4; previous revision: 1.3 done Checking in content/xslt/src/base/txDouble.cpp; /cvsroot/mozilla/content/xslt/src/base/txDouble.cpp,v <-- txDouble.cpp new revision: 1.32; previous revision: 1.31 done Checking in extensions/xforms/nsXFormsXPathFunctions.cpp; /cvsroot/mozilla/extensions/xforms/nsXFormsXPathFunctions.cpp,v <-- nsXFormsXPathFunctions.cpp new revision: 1.7; previous revision: 1.6 done
Status: NEW → RESOLVED
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M11
You need to log in
before you can comment on or make changes to this bug.
Description
•