runtime: increase assumed stack size in externalthreadhandler

On Windows, externalthreadhandler currently sets the assumed stack
size for the profiler thread and the ctrlhandler threads to 8KB. The
actual stack size is determined by the SizeOfStackReserve field in the
binary set by the linker, which is currently at least 64KB (and
typically 128KB).

It turns out the profiler thread is running within a few words of the
8KB-(stack guard) bound set by externalthreadhandler. If it overflows
this bound, morestack crashes unceremoniously with an access
violation, which we then fail to handle, causing the whole process to
exit without explanation.

To avoid this problem and give us some breathing room, increase the
assumed stack size in externalthreadhandler to 32KB (there's some
unknown amount of stack already in use, so it's not safe to increase
this all the way to the reserve size).

We also document the relationships between externalthreadhandler and
SizeOfStackReserve to make this more obvious in the future.

Change-Id: I2f9f9c0892076d78e09827022ff0f2bedd9680a9
Reviewed-on: https://go-review.googlesource.com/18304
Run-TryBot: Austin Clements <austin@google.com>
Reviewed-by: Alex Brainman <alex.brainman@gmail.com>
Reviewed-by: Minux Ma <minux@golang.org>

diff --git a/src/cmd/link/internal/ld/pe.go b/src/cmd/link/internal/ld/pe.go
index 16ce7bd016b65438937a464971fa2cc102d2bc19..00fbb170b60533a94c7ce0329dbbcd298b0402dd 100644 (file)
--- a/src/cmd/link/internal/ld/pe.go
+++ b/src/cmd/link/internal/ld/pe.go
@@ -1217,12 +1217,17 @@ func Asmbpe() {
        // larger size, as verified with VMMap.
 
        // Go code would be OK with 64k stacks, but we need larger stacks for cgo.
-       // That default stack reserve size affects only the main thread,
-       // for other threads we specify stack size in runtime explicitly
+       //
+       // The default stack reserve size affects only the main
+       // thread, ctrlhandler thread, and profileloop thread. For
+       // these, it must be greater than the stack size assumed by
+       // externalthreadhandler.
+       //
+       // For other threads we specify stack size in runtime explicitly
        // (runtime knows whether cgo is enabled or not).
-       // If you change stack reserve sizes here,
-       // change STACKSIZE in runtime/cgo/gcc_windows_{386,amd64}.c and correspondent
-       // CreateThread parameter in runtime.newosproc as well.
+       // For these, the reserve must match STACKSIZE in
+       // runtime/cgo/gcc_windows_{386,amd64}.c and the correspondent
+       // CreateThread parameter in runtime.newosproc.
        if !iscgo {
                oh64.SizeOfStackReserve = 0x00020000
                oh.SizeOfStackReserve = 0x00020000

diff --git a/src/runtime/sys_windows_386.s b/src/runtime/sys_windows_386.s
index e5fe88afd8ab8f6ba528708178826b59ca69b39f..55cdcf407f6902f9ec02a7c9a14098afbc68fa15 100644 (file)
--- a/src/runtime/sys_windows_386.s
+++ b/src/runtime/sys_windows_386.s
@@ -206,7 +206,8 @@ TEXT runtime·externalthreadhandler(SB),NOSPLIT,$0
        CALL    runtime·memclr(SB)     // smashes AX,BX,CX
        LEAL    g__size(SP), BX
        MOVL    BX, g_m(SP)
-       LEAL    -8192(SP), CX
+
+       LEAL    -32768(SP), CX          // must be less than SizeOfStackReserve set by linker
        MOVL    CX, (g_stack+stack_lo)(SP)
        ADDL    $const__StackGuard, CX
        MOVL    CX, g_stackguard0(SP)

diff --git a/src/runtime/sys_windows_amd64.s b/src/runtime/sys_windows_amd64.s
index b15eacbf3294978fc63a8ddda402cd06ccffa0b1..caa18e68e91eb6d44ba487e59fc39ce36fd75f6a 100644 (file)
--- a/src/runtime/sys_windows_amd64.s
+++ b/src/runtime/sys_windows_amd64.s
@@ -243,7 +243,7 @@ TEXT runtime·externalthreadhandler(SB),NOSPLIT,$0
        LEAQ    g__size(SP), BX
        MOVQ    BX, g_m(SP)
 
-       LEAQ    -8192(SP), CX
+       LEAQ    -32768(SP), CX          // must be less than SizeOfStackReserve set by linker
        MOVQ    CX, (g_stack+stack_lo)(SP)
        ADDQ    $const__StackGuard, CX
        MOVQ    CX, g_stackguard0(SP)