567707 - Crash in [@ nsThread::ProcessNextEvent] on x64 build with VC10 + --enable-optimize

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Makoto Kato [:m_kato]]

from http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/94c5a3e604cdbe3b#.

We need make more space to store parameters in XPTC__InvokebyIndex (currently is not enough sapce for callee).

Comment 1

[Makoto Kato [:m_kato]]

Attachment: patch

after testing VC8 and VC10, I will send a review

Comment 2

[Makoto Kato [:m_kato]]

Comment on attachment 447049 [details] [diff] [review]
patch

I will create new patch...

Comment 3

[timeless]

i'd still like to see a stack trace, i can't figure out if this bug's summary should have [@ nsThread::ProcessNextEvent] or if you mean "it crashes somewhere under".

Comment 4

[Makoto Kato [:m_kato]]

xul!nsThread::ProcessNextEvent+0x1cb:
000007fe`e5182fc7 488b5530        mov     rdx,qword ptr [rbp+30h] ss:00000000`00000031=????????????????

0:018> k
Child-SP          RetAddr           Call Site
00000000`0b7cf7e0 000007fe`e513b782 xul!nsThread::ProcessNextEvent+0x1cb [c:\workspace\hg.mozilla.org\mozilla-win64\xpcom\threads\nsthread.cpp @ 547]
00000000`0b7cf840 000007fe`e5182a94 xul!NS_ProcessNextEvent_P+0x56 [c:\workspace\hg.mozilla.org\objdir-win64-vc10\xpcom\build\nsthreadutils.cpp @ 250]
00000000`0b7cf880 000007fe`fc2fd11d xul!nsThread::ThreadFunc+0xe8 [c:\workspace\hg.mozilla.org\mozilla-win64\xpcom\threads\nsthread.cpp @ 262]
00000000`0b7cf900 000007fe`fc2ff7cd nspr4!_PR_NativeRunThread+0x10d [c:\workspace\hg.mozilla.org\mozilla-win64\nsprpub\pr\src\threads\combined\pruthr.c @ 435]
00000000`0b7cf930 00000000`74e172e5 nspr4!pr_root+0xd [c:\workspace\hg.mozilla.org\mozilla-win64\nsprpub\pr\src\md\windows\w95thred.c @ 123]
00000000`0b7cf960 00000000`74e172a4 MSVCR100D!_callthreadstartex+0x25 [f:\dd\vctools\crt_bld\self_64_amd64\crt\src\threadex.c @ 314]
00000000`0b7cf9b0 00000000`778dbe3d MSVCR100D!_threadstartex+0xb4 [f:\dd\vctools\crt_bld\self_64_amd64\crt\src\threadex.c @ 297]
00000000`0b7cf9f0 00000000`77a16a51 kernel32!BaseThreadInitThunk+0xd
00000000`0b7cfa20 00000000`00000000 ntdll!RtlUserThreadStart+0x1d


This bug is XPTC__InvokebyIndex.  XPTC__InvokebyIndex may store invalid RBP register. So I am testing new patch for this.

Comment 5

[Makoto Kato [:m_kato]]

Attachment: patch v2

Comment 6

[timeless]

Comment on attachment 447447 [details] [diff] [review]
patch v2

>diff -r 242cafe7bc40 xpcom/reflect/xptcall/src/md/win32/xptcinvoke_asm_x86_64.asm
>@@ -59,7 +59,8 @@
>     ; store RBX/RBP register for backup

please adjust the comment

>+    push    rbp
>+    .PUSHREG rbp

>@@ -78,8 +79,7 @@
>     ; make space for 1st parameter
> 
>     mov     eax, r8d

you've removed a comment here, does it belong somewhere else?

>-    and     eax, 1              ; AMD64 must be alignment to 16 bytes
>-    add     eax, r8d
>+    or      eax, 1

>@@ -96,32 +96,33 @@
>-    sub     rsp, 32
>+    sub     rsp, 40

this comment seems odd:
>+    ; 1st parameter is this

it doesn't match the later comment about 1st parameter (this)

>     ; Build parameters
>+    mov     rdx, qword ptr [rsp+8] ; 1st parameter
>+    movsd   xmm1, qword ptr [rsp+8] ; for double
>+    mov     r8, qword ptr [rsp+16] ; 2nd parameter
>+    movsd   xmm2, qword ptr [rsp+16] ; for double
>+    mov     r9, qword ptr [rsp+24] ; 3rd parameter
>+    movsd   xmm3, qword ptr [rsp+24] ; for double
> 

later comment:
>     ;
>     ; 1st parameter (this)
>     ;
> 
>+    mov     rcx, qword ptr [rbp+8+8] ; that

>@@ -130,14 +131,12 @@
>     mov     r11, qword ptr [rcx]
>+    mov     eax, dword ptr [rbp+16+8] ; methodIndex

this comment probably needs to be updated:
>     ; Now current stack has parameter list
>     ; But, since callee function backups parameters, make space into stack.
> 
>-    sub     rsp, 8
>-
>     call    qword ptr [r11+rax*8]      ; stdcall, i.e. callee cleans up stack.

>@@ -145,7 +144,7 @@

I can't tell if you need to update this comment:
>     ;
> 
>     mov     rsp, rbp
>-    mov     rbp, qword ptr [rsp-16]
>+    pop     rbp
> 
>     ret
>

Comment 7

[Makoto Kato [:m_kato]]

Attachment: patch v2.1

Comment 8

[timeless]

Comment on attachment 447739 [details] [diff] [review]
patch v2.1

thanks, the comments make a lot more sense now :)

please note the changes i've made to this line:
>+    ; On Win64 ABI, _the_ first 4 parameters are _passed_ using registers,
>+    ; and others is on stack. 

you said 4 above, but there are only 3 below + "that", if "that" counts as one of them, then please use 'arguments' instead of 'parameters' below, or add a note indicating that 'that' is the 0th parameter....

please note the changes i've made to this line:
>+    ; 1st, 2nd and 3rd parameter _are_ *passed* via registers

>+    mov     rdx, qword ptr [rsp+8] ; 1st parameter
>+    movsd   xmm1, qword ptr [rsp+8] ; for double
>+    mov     r8, qword ptr [rsp+16] ; 2nd parameter
>+    movsd   xmm2, qword ptr [rsp+16] ; for double
>+    mov     r9, qword ptr [rsp+24] ; 3rd parameter
>+    movsd   xmm3, qword ptr [rsp+24] ; for double

>+    ; rcx register is this
>+
>+    mov     rcx, qword ptr [rbp+8+8] ; that

Comment 9

[Makoto Kato [:m_kato]]

Attachment: patch v2.2

Comment 10

[Makoto Kato [:m_kato]]

landed
http://hg.mozilla.org/mozilla-central/rev/fadb38356e0f