Closed Bug 289394 Opened 19 years ago Closed 16 years ago

Double.cpp causes unaligned accesses

Categories

(Core :: XSLT, defect)

Other
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9beta3

People

(Reporter: davidm, Assigned: davidm)

Details

(Keywords: perf)

Attachments

(1 file, 4 obsolete files)

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.
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...
(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
Attached patch revised patch to fix alignment (obsolete) — Splinter Review
Sure, here is a revised patch which fixes the whitespace so the lines are
aligned.
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
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)

?
(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;
Attached patch actually compiles (obsolete) — Splinter Review
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.
Attachment #179969 - Flags: superreview?(bryner) → superreview+
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.
Attachment #181384 - Flags: review?
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 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 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?
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.
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 on attachment 292222 [details] [diff] [review]
New patch for trunk

Fix some unaligned access issues.
Attachment #292222 - Flags: approval1.9?
Comment on attachment 292222 [details] [diff] [review]
New patch for trunk

a=beltzner for 1.9
Attachment #292222 - Flags: approval1.9? → approval1.9+
Keywords: checkin-needed
QA Contact: keith → xslt
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
Keywords: perf
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: