Closed Bug 552627 Opened 12 years ago Closed 12 years ago

Crash with SSE2 enabled build on Linux

Categories

(Core :: XPConnect, defect)

x86
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: yong.wu, Assigned: yong.wu)

Details

(Keywords: crash)

Attachments

(1 file, 3 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US) AppleWebKit/532.5 (KHTML, like Gecko) Chrome/4.0.249.89 Safari/532.5
Build Identifier: 

Build Firefox or Fennec with SSE2 enabled (Linux/x86) and browse websites www.cnn.com or www.sina.com.cn can cause Firefox/Fennec to crash.

Reproducible: Always

Steps to Reproduce:
1. Build Linux Firefox or Fennec with SSE enabled. This is the mozconfig option:
ac_add_options --enable-optimize="-g -fno-inline -march=core2 -mssse3 -msse2 -mtune=generic -mfpmath=sse "
2. Run Firefox/Fennec on any Xeon/Core2/Atom platform platform.
3. Launch www.cnn.com or www.sina.com.cn 
3.
Actual Results:  
Firefox/Fennec crashes.
The direct reason it crashes is, the generated SSE memory operand for SSE2
instructions movapd/movdqa are not aligned to 16-bytes boundary, which causes
segmentation fault when those SSE instructions are executing. The root cause is
Firefox XPTCall did not aligned its stack pointer rightly when it calls native
function from Javascript interperter.

GCC compiler requires the stack pointer esp to be aligned to 16x boundary (when
calling a function), and frame pointer $ebp to be (16x + 8) boundary (at any
time). Thus later a movadp instruction could add/sub a 8 bytes offset in its
operand to align to 16x boundary. Unfortunately Firefox XPTCall did not conform
to the expected calling convention.
you didn't specify your version.

bug 410509 http://hg.mozilla.org/mozilla-central/rev/c4c01d41e11a supposedly fixed this.

bug 218629 has what i believe is a better fix for this, but it requires someone familiar with something to review. if you can vouch for it being better, we can see about pushing it.
I am using the Firefox 3.6 and Fennec 1.0rc3 with xulrunner 1.9.2 which has already those patches. But it still crash.

I have attached a simple patch to fix.
Attached patch Fix the sse misalignment issue. (obsolete) — Splinter Review
Comment on attachment 432763 [details] [diff] [review]
Fix the sse misalignment issue.

local style uses an asterisk for each line of a comment:
  * is assumed and required, e.g. by GCC4's -ftree-vectorize option.
  */

indentation should be to this column [using tabs :(]:
 	"andl  $0xfffffff0, %ecx\n\t"   /* drop(?) stack ptr to 128-bit align */
-	"subl  $8, %ecx\n\t"	        /* lower again; push/call below will re-align */
-	
+    /* The $esp should be aligned to 16-bytes boundary here (note there
+       is additional 4 bytes in push instrucitons below). 

instructions

But, probably "note we include an additional 4 bytes in a push later instruction"

+       This will ensure the $ebp in the function called below aligned to 

below _is_ aligned to _a_ 0x8 boundary. ?

+       8x boundary. The SSE instructions like movapd/movdqa expects the 

omit 'The'

+       memory operand to be aligned to 16-bytes boundary. GCC compiler will 

aligned on/at ? a 16-byte boundary. ?

"The GCC compiler" or "GCC"

+       generate the memory operand using $ebp with 8 bytes offset.

with an 8 byte offset.

+     */
+    "subl  $0xc, %ecx\n\t"          /* lower again; push/call below will re-align */
 	"movl  %ecx, %esp\n\t"          /* make stack space */
Attached patch Fix sse misalignment issue (v2) (obsolete) — Splinter Review
Thank Timeless's comments. I've corrected indention and syntax errors in comments. This is the second version of that patch.
Attachment #432763 - Attachment is obsolete: true
Attached patch Fix sse misalignment issue (v3) (obsolete) — Splinter Review
Attachment #432781 - Attachment is obsolete: true
Seems like this bug can be confirmed.
Wu: Do you know about the review process, etc.? Maybe you should request official review from timeless so that the patch can be marked r+ or r- :).
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attachment #432782 - Flags: review+
Attachment #432782 - Flags: review+ → review?(timeless)
Assignee: nobody → yong.wu
Severity: normal → critical
Status: NEW → ASSIGNED
Keywords: crash
Attachment #432782 - Flags: review?(timeless) → review+
Comment on attachment 432782 [details] [diff] [review]
Fix sse misalignment issue (v3)

>+/* The $esp should be aligned to 16-bytes boundary here (note we include an 

drop "The" before $esp/$ebp
... to _a_ 16-byte__ boundary ...

>+ * additional 4 bytes in a later push instruction). This will ensure the $ebp 
>+ * in the function called below is aligned to a 0x8 boundary. SSE instructions 
>+ * like movapd/movdqa expects memory operand to be aligned on a 16-byte 

... expect ...

>+ * boundary. The GCC compiler will generate the memory operand using $ebp with
>+ * an 8-byte offset.
Thanks again for timeless's correction. Should I submit a new patch with the changes.. timeless?
it'll be easier if you do
Updated patch according to timeless's corrections.
Attachment #432782 - Attachment is obsolete: true
Attachment #447024 - Flags: review?
Attachment #447024 - Flags: review? → review?(timeless)
Attachment #447024 - Flags: review?(timeless) → review+
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/7493eb9e9cbc
Thanks!
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.