552627 - Crash with SSE2 enabled build on Linux

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Yong]

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.

Comment 1

[Yong]

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.

Comment 2

[timeless]

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.

Comment 3

[Yong]

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.

Comment 4

[Yong]

Attachment: Fix the sse misalignment issue.

Comment 5

[timeless]

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 */

Comment 6

[Yong]

Attachment: Fix sse misalignment issue (v2)

Thank Timeless's comments. I've corrected indention and syntax errors in comments. This is the second version of that patch.

Comment 7

[Yong]

Attachment: Fix sse misalignment issue (v3)

Comment 8

[Frank Wein [:mcsmurf]]

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- :).

Comment 9

[timeless]

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.

Comment 10

[Yong]

Thanks again for timeless's correction. Should I submit a new patch with the changes.. timeless?

Comment 11

[timeless]

it'll be easier if you do

Comment 12

[Yong]

Attachment: Fix sse misalignment issue (v4)

Updated patch according to timeless's corrections.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/mozilla-central/rev/7493eb9e9cbc
Thanks!