[go] runtime: query thread stack size from OS on Windows
Austin Clements (Gerrit)
Austin Clements would like Ian Lance Taylor and Alex Brainman to review this change.
runtime: query thread stack size from OS on Windows
Currently, on Windows, the thread stack size is set or assumed in many
different places. In non-cgo binaries, both the Go linker and the
runtime have a copy of the stack size, the Go linker sets the size of
the main thread stack, and the runtime sets the size of other thread
stacks. In cgo binaries, the external linker sets the main thread
stack size, the runtime assumes the size of the main thread stack will
be the same as used by the Go linker, and the cgo entry code assumes
the same.
Furthermore, users can change the main thread stack size using
editbin, so the runtime doesn't even really know what size it is, and
user C code can create threads with unknown thread stack sizes, which
we also assume have the same default stack size.
This is all a mess.
Fix the corner cases of this and the duplication of knowledge between
the linker and the runtime by querying the OS for the stack bounds
during thread setup. Furthermore, we unify all of this into just
runtime.minit for both cgo and non-cgo binaries and for the main
thread, other runtime-created threads, and C-created threads.
Updates #20975.
Change-Id: I45dbee2b5ea2ae721a85a27680737ff046f9d464
---
M src/cmd/link/internal/ld/pe.go
M src/runtime/cgo/gcc_windows_386.c
M src/runtime/cgo/gcc_windows_amd64.c
M src/runtime/crash_cgo_test.go
M src/runtime/defs_windows_386.go
M src/runtime/defs_windows_amd64.go
M src/runtime/os_windows.go
M src/runtime/proc.go
M src/runtime/sys_windows_386.s
M src/runtime/sys_windows_amd64.s
A src/runtime/testdata/testprogcgo/bigstack_windows.go
11 files changed, 100 insertions(+), 42 deletions(-)
diff --git a/src/cmd/link/internal/ld/pe.go b/src/cmd/link/internal/ld/pe.go
index 3b7df9a..efd971c 100644
--- a/src/cmd/link/internal/ld/pe.go
+++ b/src/cmd/link/internal/ld/pe.go
@@ -845,15 +845,18 @@
// and system calls even in "pure" Go code are actually C
// calls that may need more stack than we think.
//
- // The default stack reserve size affects only the main
+ // The default stack reserve size directly 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.
- // For these, the reserve must match STACKSIZE in
- // runtime/cgo/gcc_windows_{386,amd64}.c and osStackSize in
- // runtime/os_windows.go.
+ // For other threads, the runtime explicitly asks the kernel
+ // to use the default stack size so that all stacks are
+ // consistent.
+ //
+ // At thread start, in minit, the runtime queries the OS for
+ // the actual stack bounds so that the stack size doesn't need
+ // to be hard-coded into the runtime.
oh64.SizeOfStackReserve = 0x00200000
if !iscgo {
oh64.SizeOfStackCommit = 0x00001000
diff --git a/src/runtime/cgo/gcc_windows_386.c b/src/runtime/cgo/gcc_windows_386.c
index e80a564..f2ff710 100644
--- a/src/runtime/cgo/gcc_windows_386.c
+++ b/src/runtime/cgo/gcc_windows_386.c
@@ -11,16 +11,9 @@
static void threadentry(void*);
-/* 1MB is default stack size for 32-bit Windows.
- Allocation granularity on Windows is typically 64 KB.
- This constant must match SizeOfStackReserve in ../cmd/link/internal/ld/pe.go. */
-#define STACKSIZE (1*1024*1024)
-
void
x_cgo_init(G *g)
{
- int tmp;
- g->stacklo = (uintptr)&tmp - STACKSIZE + 8*1024;
}
@@ -44,8 +37,7 @@
ts = *(ThreadStart*)v;
free(v);
- ts.g->stackhi = (uintptr)&ts;
- ts.g->stacklo = (uintptr)&ts - STACKSIZE + 8*1024;
+ // minit queries stack bounds from the OS.
/*
* Set specific keys in thread local storage.
diff --git a/src/runtime/cgo/gcc_windows_amd64.c b/src/runtime/cgo/gcc_windows_amd64.c
index 75a7dc8..511ab44 100644
--- a/src/runtime/cgo/gcc_windows_amd64.c
+++ b/src/runtime/cgo/gcc_windows_amd64.c
@@ -11,16 +11,9 @@
static void threadentry(void*);
-/* 2MB is default stack size for 64-bit Windows.
- Allocation granularity on Windows is typically 64 KB.
- This constant must match SizeOfStackReserve in ../cmd/link/internal/ld/pe.go. */
-#define STACKSIZE (2*1024*1024)
-
void
x_cgo_init(G *g)
{
- int tmp;
- g->stacklo = (uintptr)&tmp - STACKSIZE + 8*1024;
}
@@ -44,8 +37,7 @@
ts = *(ThreadStart*)v;
free(v);
- ts.g->stackhi = (uintptr)&ts;
- ts.g->stacklo = (uintptr)&ts - STACKSIZE + 8*1024;
+ // minit queries stack bounds from the OS.
/*
* Set specific keys in thread local storage.
diff --git a/src/runtime/crash_cgo_test.go b/src/runtime/crash_cgo_test.go
index d8f75a4..6cfb209 100644
--- a/src/runtime/crash_cgo_test.go
+++ b/src/runtime/crash_cgo_test.go
@@ -489,3 +489,17 @@
t.Fatalf("failure incorrectly contains %q. output:\n%s\n", nowant, got)
}
}
+
+func TestCgoBigStack(t *testing.T) {
+ // Test that C code can use the large Windows stack without
+ // crashing Go.
+ if runtime.GOOS != "windows" {
+ t.Skip("skipping windows specific test")
+ }
+ t.Parallel()
+ got := runTestProg(t, "testprogcgo", "BigStack")
+ want := "OK\n"
+ if got != want {
+ t.Errorf("expected %q got %v", want, got)
+ }
+}
diff --git a/src/runtime/defs_windows_386.go b/src/runtime/defs_windows_386.go
index bac6ce7..589a788 100644
--- a/src/runtime/defs_windows_386.go
+++ b/src/runtime/defs_windows_386.go
@@ -129,3 +129,13 @@
anon0 [8]byte
hevent *byte
}
+
+type memoryBasicInformation struct {
+ baseAddress uintptr
+ allocationBase uintptr
+ allocationProtect uint32
+ regionSize uintptr
+ state uint32
+ protect uint32
+ type_ uint32
+}
diff --git a/src/runtime/defs_windows_amd64.go b/src/runtime/defs_windows_amd64.go
index 6e04568..1e173e9 100644
--- a/src/runtime/defs_windows_amd64.go
+++ b/src/runtime/defs_windows_amd64.go
@@ -151,3 +151,13 @@
anon0 [8]byte
hevent *byte
}
+
+type memoryBasicInformation struct {
+ baseAddress uintptr
+ allocationBase uintptr
+ allocationProtect uint32
+ regionSize uintptr
+ state uint32
+ protect uint32
+ type_ uint32
+}
diff --git a/src/runtime/os_windows.go b/src/runtime/os_windows.go
index a4df970..a879e4b 100644
--- a/src/runtime/os_windows.go
+++ b/src/runtime/os_windows.go
@@ -45,6 +45,7 @@
//go:cgo_import_dynamic runtime._SwitchToThread SwitchToThread%0 "kernel32.dll"
//go:cgo_import_dynamic runtime._VirtualAlloc VirtualAlloc%4 "kernel32.dll"
//go:cgo_import_dynamic runtime._VirtualFree VirtualFree%3 "kernel32.dll"
+//go:cgo_import_dynamic runtime._VirtualQuery VirtualQuery%3 "kernel32.dll"
//go:cgo_import_dynamic runtime._WSAGetOverlappedResult WSAGetOverlappedResult%5 "ws2_32.dll"
//go:cgo_import_dynamic runtime._WaitForSingleObject WaitForSingleObject%2 "kernel32.dll"
//go:cgo_import_dynamic runtime._WriteConsoleW WriteConsoleW%5 "kernel32.dll"
@@ -92,6 +93,7 @@
_SwitchToThread,
_VirtualAlloc,
_VirtualFree,
+ _VirtualQuery,
_WSAGetOverlappedResult,
_WaitForSingleObject,
_WriteConsoleW,
@@ -291,9 +293,6 @@
}
}
-// osStackSize must match SizeOfStackReserve in ../cmd/link/internal/ld/pe.go.
-var osStackSize uintptr = 0x00200000*_64bit + 0x00100000*(1-_64bit)
-
func osinit() {
asmstdcallAddr = unsafe.Pointer(funcPC(asmstdcall))
usleep2Addr = unsafe.Pointer(funcPC(usleep2))
@@ -322,15 +321,6 @@
// equivalent threads that all do a mix of GUI, IO, computations, etc.
// In such context dynamic priority boosting does nothing but harm, so we turn it off.
stdcall2(_SetProcessPriorityBoost, currentProcess, 1)
-
- // Fix the entry thread's stack bounds, since runtime entry
- // assumed we were on a tiny stack. If this is a cgo binary,
- // x_cgo_init already fixed these.
- if !iscgo {
- g0.stack.lo = g0.stack.hi - osStackSize + 8*1024
- g0.stackguard0 = g0.stack.lo + _StackGuard
- g0.stackguard1 = g0.stackguard0
- }
}
func nanotime() int64
@@ -631,10 +621,11 @@
//go:nowritebarrierrec
//go:nosplit
func newosproc(mp *m) {
- const _STACK_SIZE_PARAM_IS_A_RESERVATION = 0x00010000
- thandle := stdcall6(_CreateThread, 0, osStackSize,
+ // Pass 0 for the stack size to use the default for this binary.
+ const stackSizeFromPEHeader = 0
+ thandle := stdcall6(_CreateThread, 0, stackSizeFromPEHeader,
funcPC(tstart_stdcall), uintptr(unsafe.Pointer(mp)),
- _STACK_SIZE_PARAM_IS_A_RESERVATION, 0)
+ 0, 0)
if thandle == 0 {
if atomic.Load(&exiting) != 0 {
@@ -699,8 +690,24 @@
var thandle uintptr
stdcall7(_DuplicateHandle, currentProcess, currentThread, currentProcess, uintptr(unsafe.Pointer(&thandle)), 0, 0, _DUPLICATE_SAME_ACCESS)
atomic.Storeuintptr(&getg().m.thread, thandle)
+
+ // Query the true stack base from the OS. Currently we're
+ // running on a small assumed stack.
+ var mbi memoryBasicInformation
+ res := stdcall3(_VirtualQuery, uintptr(unsafe.Pointer(&mbi)), uintptr(unsafe.Pointer(&mbi)), unsafe.Sizeof(mbi))
+ if res == 0 {
+ throw("VirtualQuery for stack base failed")
+ }
+ base := mbi.allocationBase + 1024
+ g0 := getg()
+ g0.stack.lo = base
+ g0.stackguard0 = g0.stack.lo + _StackGuard
+ g0.stackguard1 = g0.stackguard0
+ stackcheck()
}
+func stackcheck()
+
// Called from dropm to undo the effect of an minit.
//go:nosplit
func unminit() {
diff --git a/src/runtime/proc.go b/src/runtime/proc.go
index 36c74a1..22132bf 100644
--- a/src/runtime/proc.go
+++ b/src/runtime/proc.go
@@ -1231,6 +1231,7 @@
if osStack {
// Initialize stack bounds from system stack.
// Cgo may have left stack size in stack.hi.
+ // minit may update the stack bounds.
size := _g_.stack.hi
if size == 0 {
size = 8192 * sys.StackGuardMultiplier
diff --git a/src/runtime/sys_windows_386.s b/src/runtime/sys_windows_386.s
index abb6bad..d6f3997 100644
--- a/src/runtime/sys_windows_386.s
+++ b/src/runtime/sys_windows_386.s
@@ -315,8 +315,7 @@
// Layout new m scheduler stack on os stack.
MOVL SP, AX
MOVL AX, (g_stack+stack_hi)(DX)
- SUBL runtime·osStackSize(SB), AX // stack size
- ADDL $(8*1024), AX
+ SUBL $(64*1024), AX // initial stack size
MOVL AX, (g_stack+stack_lo)(DX)
ADDL $const__StackGuard, AX
MOVL AX, g_stackguard0(DX)
diff --git a/src/runtime/sys_windows_amd64.s b/src/runtime/sys_windows_amd64.s
index 8a12cda..f682e8a 100644
--- a/src/runtime/sys_windows_amd64.s
+++ b/src/runtime/sys_windows_amd64.s
@@ -363,8 +363,7 @@
// Layout new m scheduler stack on os stack.
MOVQ SP, AX
MOVQ AX, (g_stack+stack_hi)(DX)
- SUBQ runtime·osStackSize(SB), AX // stack size
- ADDQ $(8*1024), AX
+ SUBQ $(64*1024), AX // inital stack size
MOVQ AX, (g_stack+stack_lo)(DX)
ADDQ $const__StackGuard, AX
MOVQ AX, g_stackguard0(DX)
diff --git a/src/runtime/testdata/testprogcgo/bigstack_windows.go b/src/runtime/testdata/testprogcgo/bigstack_windows.go
new file mode 100644
index 0000000..150928a
--- /dev/null
+++ b/src/runtime/testdata/testprogcgo/bigstack_windows.go
@@ -0,0 +1,31 @@
+// Copyright 2018 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package main
+
+/*
+extern void goBigStack1(char*);
+
+static void bigStack(void) {
+ char x[256<<10];
+ goBigStack1(x);
+}
+*/
+import "C"
+
+func init() {
+ register("BigStack", BigStack)
+}
+
+func BigStack() {
+ // Use a lot of stack and call back into Go.
+ C.bigStack()
+}
+
+var alwaysFalse bool
+
+//export goBigStack1
+func goBigStack1(x *C.char) {
+ println("OK")
+}
To view, visit change 120336. To unsubscribe, or for help writing mail filters, visit settings.
Gobot Gobot (Gerrit)
TryBots beginning. Status page: https://farmer.golang.org/try?commit=0f575d4c
Austin Clements (Gerrit)
We may want to go ahead and do this for Go 1.11. It's obviously a more complicated change than the parent CL, but I'm also not convinced I got all of the code paths in the previous CL since we set up stacks in a surprising variety of ways. This CL is a much cleaner and more complete solution.
Gobot Gobot (Gerrit)
Build is still in progress...
This change failed on misc-vet-vetall:
See https://storage.googleapis.com/go-build-log/0f575d4c/misc-vet-vetall_b850a9d3.log
Consult https://build.golang.org/ to see whether it's a new failure. Other builds still in progress; subsequent failure notices suppressed until final report.
Austin Clements (Gerrit)
Austin Clements uploaded patch set #2 to this change.
runtime: query thread stack size from OS on Windows
Currently, on Windows, the thread stack size is set or assumed in many
different places. In non-cgo binaries, both the Go linker and the
runtime have a copy of the stack size, the Go linker sets the size of
the main thread stack, and the runtime sets the size of other thread
stacks. In cgo binaries, the external linker sets the main thread
stack size, the runtime assumes the size of the main thread stack will
be the same as used by the Go linker, and the cgo entry code assumes
the same.
Furthermore, users can change the main thread stack size using
editbin, so the runtime doesn't even really know what size it is, and
user C code can create threads with unknown thread stack sizes, which
we also assume have the same default stack size.
This is all a mess.
Fix the corner cases of this and the duplication of knowledge between
the linker and the runtime by querying the OS for the stack bounds
during thread setup. Furthermore, we unify all of this into just
runtime.minit for both cgo and non-cgo binaries and for the main
thread, other runtime-created threads, and C-created threads.
Updates #20975.
Change-Id: I45dbee2b5ea2ae721a85a27680737ff046f9d464
---
M src/cmd/link/internal/ld/pe.go
M src/cmd/vet/all/whitelist/386.txt
M src/cmd/vet/all/whitelist/amd64.txt
M src/cmd/vet/all/whitelist/nacl_amd64p32.txt
M src/runtime/cgo/gcc_windows_386.c
M src/runtime/cgo/gcc_windows_amd64.c
M src/runtime/crash_cgo_test.go
M src/runtime/defs_windows_386.go
M src/runtime/defs_windows_amd64.go
M src/runtime/os_windows.go
M src/runtime/proc.go
A src/runtime/stubs_x86.go
M src/runtime/sys_windows_386.s
M src/runtime/sys_windows_amd64.s
A src/runtime/testdata/testprogcgo/bigstack_windows.go
15 files changed, 108 insertions(+), 47 deletions(-)
To view, visit change 120336. To unsubscribe, or for help writing mail filters, visit settings.
Gobot Gobot (Gerrit)
Gobot Gobot (Gerrit)
TryBots are happy.
Patch set 2:TryBot-Result +1
Ian Lance Taylor (Gerrit)
Patch set 2:Code-Review +2
1 comment:
File src/runtime/defs_windows_386.go:
Patch Set #2, Line 133:
type memoryBasicInformation struct {Even though it's not used, it's useful documentation to add a line for this struct to defs_windows.go.
To view, visit change 120336. To unsubscribe, or for help writing mail filters, visit settings.
Austin Clements (Gerrit)
Alex, if you have a minute to sanity check my VirtualQuery approach to finding the stack base, I would appreciate it. The same technique is used by Firefox (https://dxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCJSContext.cpp#939), so I'm pretty sure it's sound.
1 comment:
Patch Set #2, Line 133:
type memoryBasicInformation struct {
Even though it's not used, it's useful documentation to add a line for this struct to defs_windows. […]
Done
To view, visit change 120336. To unsubscribe, or for help writing mail filters, visit settings.
Austin Clements (Gerrit)
Austin Clements uploaded patch set #3 to this change.
runtime: query thread stack size from OS on Windows
Currently, on Windows, the thread stack size is set or assumed in many
different places. In non-cgo binaries, both the Go linker and the
runtime have a copy of the stack size, the Go linker sets the size of
the main thread stack, and the runtime sets the size of other thread
stacks. In cgo binaries, the external linker sets the main thread
stack size, the runtime assumes the size of the main thread stack will
be the same as used by the Go linker, and the cgo entry code assumes
the same.
Furthermore, users can change the main thread stack size using
editbin, so the runtime doesn't even really know what size it is, and
user C code can create threads with unknown thread stack sizes, which
we also assume have the same default stack size.
This is all a mess.
Fix the corner cases of this and the duplication of knowledge between
the linker and the runtime by querying the OS for the stack bounds
during thread setup. Furthermore, we unify all of this into just
runtime.minit for both cgo and non-cgo binaries and for the main
thread, other runtime-created threads, and C-created threads.
Updates #20975.
Change-Id: I45dbee2b5ea2ae721a85a27680737ff046f9d464
---
M src/cmd/link/internal/ld/pe.go
M src/cmd/vet/all/whitelist/386.txt
M src/cmd/vet/all/whitelist/amd64.txt
M src/cmd/vet/all/whitelist/nacl_amd64p32.txt
M src/runtime/cgo/gcc_windows_386.c
M src/runtime/cgo/gcc_windows_amd64.c
M src/runtime/crash_cgo_test.go
M src/runtime/defs_windows.go
M src/runtime/defs_windows_386.go
M src/runtime/defs_windows_amd64.go
M src/runtime/os_windows.go
M src/runtime/proc.go
A src/runtime/stubs_x86.go
M src/runtime/sys_windows_386.s
M src/runtime/sys_windows_amd64.s
A src/runtime/testdata/testprogcgo/bigstack_windows.go
16 files changed, 109 insertions(+), 47 deletions(-)
To view, visit change 120336. To unsubscribe, or for help writing mail filters, visit settings.
Alex Brainman (Gerrit)
Alex, if you have a minute to sanity check my VirtualQuery approach to finding the stack base, I would appreciate it. The same technique is used by Firefox (https://dxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCJSContext.cpp#939), so I'm pretty sure it's sound.
I am not sure about your approach, but if you want to do it, do it.
My biggest concern, is we do not have a test to reproduce the problem that you are fixing. You took away consts that we used everywhere, and now rely on what VirtualQuery returns plus some logic you copied somewhere. It would be nice at least verify the numbers you get are sane.
As far as I understand your change affects only cgo code, so, hopefully, not many people use cgo.
Alex
4 comments:
File src/runtime/os_windows.go:
Patch Set #3, Line 625:
stackSizeFromPEHeaderIntroducing stackSizeFromPEHeader const just confuses me. I would just use 0 instead. People will lookup CreateThread documentation to see how it works.
Why is that OK to replace _STACK_SIZE_PARAM_IS_A_RESERVATION with 0?
Patch Set #3, Line 699:
throw("VirtualQuery for stack base failed")You can call GetLastError to get details about why VirtualQuery failed, if you want. And print it as part of throw message.
Patch Set #3, Line 701:
base := mbi.allocationBase + 1024Why adding 1024. You need some comment to explain.
To view, visit change 120336. To unsubscribe, or for help writing mail filters, visit settings.
Austin Clements (Gerrit)
Patch Set 3:
(4 comments)
Alex, if you have a minute to sanity check my VirtualQuery approach to finding the stack base, I would appreciate it. The same technique is used by Firefox (https://dxr.mozilla.org/mozilla-central/source/js/xpconnect/src/XPCJSContext.cpp#939), so I'm pretty sure it's sound.
I am not sure about your approach, but if you want to do it, do it.
My biggest concern, is we do not have a test to reproduce the problem that you are fixing. You took away consts that we used everywhere, and now rely on what VirtualQuery returns plus some logic you copied somewhere. It would be nice at least verify the numbers you get are sane.
I took away consts that weren't really const. :)
I'm not sure what you mean by "some logic I copied somewhere". My goal was to reduce the amount of copied logic (and to make it more correct).
I've beefed up the test I added. That test did reproduce the original problem (which was fixed by the previous CL), but the new version more specifically tests this CL.
And I've added more verification of the numbers from VirtualQuery in addition to the stackcheck().
As far as I understand your change affects only cgo code, so, hopefully, not many people use cgo.
Alex
4 comments:
Patch Set #3, Line 625:
stackSizeFromPEHeader
Introducing stackSizeFromPEHeader const just confuses me. I would just use 0 instead. […]
Okay.
Why is that OK to replace _STACK_SIZE_PARAM_IS_A_RESERVATION with 0?
We're not passing a stack size, so this flag would be a no-op.
Patch Set #3, Line 699:
throw("VirtualQuery for stack base failed")
You can call GetLastError to get details about why VirtualQuery failed, if you want. […]
Done
Patch Set #3, Line 701:
base := mbi.allocationBase + 1024Why adding 1024. You need some comment to explain.
Done
To view, visit change 120336. To unsubscribe, or for help writing mail filters, visit settings.
Austin Clements (Gerrit)
Austin Clements uploaded patch set #4 to this change.
runtime: query thread stack size from OS on Windows
Currently, on Windows, the thread stack size is set or assumed in many
different places. In non-cgo binaries, both the Go linker and the
runtime have a copy of the stack size, the Go linker sets the size of
the main thread stack, and the runtime sets the size of other thread
stacks. In cgo binaries, the external linker sets the main thread
stack size, the runtime assumes the size of the main thread stack will
be the same as used by the Go linker, and the cgo entry code assumes
the same.
Furthermore, users can change the main thread stack size using
editbin, so the runtime doesn't even really know what size it is, and
user C code can create threads with unknown thread stack sizes, which
we also assume have the same default stack size.
This is all a mess.
Fix the corner cases of this and the duplication of knowledge between
the linker and the runtime by querying the OS for the stack bounds
during thread setup. Furthermore, we unify all of this into just
runtime.minit for both cgo and non-cgo binaries and for the main
thread, other runtime-created threads, and C-created threads.
Updates #20975.
Change-Id: I45dbee2b5ea2ae721a85a27680737ff046f9d464
---
M src/cmd/link/internal/ld/pe.go
M src/cmd/vet/all/whitelist/386.txt
M src/cmd/vet/all/whitelist/amd64.txt
M src/cmd/vet/all/whitelist/nacl_amd64p32.txt
M src/runtime/cgo/gcc_windows_386.c
M src/runtime/cgo/gcc_windows_amd64.c
M src/runtime/crash_cgo_test.go
M src/runtime/defs_windows.go
M src/runtime/defs_windows_386.go
M src/runtime/defs_windows_amd64.go
M src/runtime/os_windows.go
M src/runtime/proc.go
A src/runtime/stubs_x86.go
M src/runtime/sys_windows_386.s
M src/runtime/sys_windows_amd64.s
A src/runtime/testdata/testprogcgo/bigstack_windows.go
16 files changed, 142 insertions(+), 50 deletions(-)
To view, visit change 120336. To unsubscribe, or for help writing mail filters, visit settings.
Gobot Gobot (Gerrit)
Gobot Gobot (Gerrit)
Build is still in progress...
This change failed on windows-386-2008:
See https://storage.googleapis.com/go-build-log/f2d77670/windows-386-2008_61b69dff.log
Consult https://build.golang.org/ to see whether it's a new failure. Other builds still in progress; subsequent failure notices suppressed until final report.
To view, visit change 120336. To unsubscribe, or for help writing mail filters, visit settings.
Gobot Gobot (Gerrit)
1 of 19 TryBots failed:
Failed on windows-386-2008: https://storage.googleapis.com/go-build-log/f2d77670/windows-386-2008_61b69dff.log
Consult https://build.golang.org/ to see whether they are new failures.
Patch set 4:TryBot-Result -1
Alex Brainman (Gerrit)
I took away consts that weren't really const. :)
This code was written by many people over many years. You cannot blame me for everything. :-)
I'm not sure what you mean by "some logic I copied somewhere". My goal was to reduce the amount of copied logic (and to make it more correct).
I've beefed up the test I added. That test did reproduce the original problem (which was fixed by the previous CL), but the new version more specifically tests this CL.
And I've added more verification of the numbers from VirtualQuery in addition to the stackcheck().
Yes, you put more checks in runtime and tests. Thank you very much.
Now we just need to work out why test fails. :-)
I will try and find some time to debug this on weekend.
Alex
2 comments:
Patch Set #4, Line 31:
20975You mention issue #20975. But your fix affects CGO code only (as far as I can see), but, I believe, code mentioned in #20975 does not use CGO.
We're not passing a stack size, so this flag would be a no-op.
I don't think you are correct.
From https://docs.microsoft.com/en-us/windows/desktop/api/processthreadsapi/nf-processthreadsapi-createthread
```
STACK_SIZE_PARAM_IS_A_RESERVATIONThe dwStackSize parameter specifies the initial reserve size of the stack. If this flag is not specified, dwStackSize specifies the commit size.
```
So the fact that STACK_SIZE_PARAM_IS_A_RESERVATION is here, means we are not specifying "stack size" here, but "initial reserve size of the stack".I don't believe we have test for this, so we should be careful changing this flag. This flag controls how much real memory allocated at the start of a thread, and this will affect every Go program memory usage.
To view, visit change 120336. To unsubscribe, or for help writing mail filters, visit settings.
Alex Brainman (Gerrit)
I will try and find some time to debug this on weekend.
Sorry I meant to say I will try to debug your CL 120857 failure.
Alex
Austin Clements (Gerrit)
Patch Set 4:
(2 comments)
I took away consts that weren't really const. :)
This code was written by many people over many years. You cannot blame me for everything. :-)
Oh! I definitely don't blame you. I've been so deep in the UNIX world for so long that dealing with Windows things often feels like I'm feeling around in the dark. I trust you to tell me if I'm about to walk into a wall.
I'm not sure what you mean by "some logic I copied somewhere". My goal was to reduce the amount of copied logic (and to make it more correct).
I've beefed up the test I added. That test did reproduce the original problem (which was fixed by the previous CL), but the new version more specifically tests this CL.
And I've added more verification of the numbers from VirtualQuery in addition to the stackcheck().
Yes, you put more checks in runtime and tests. Thank you very much.
I'm going to try to add a non-cgo test today as well (based on the DLL-building tests you pointed to in the issue).
Now we just need to work out why test fails. :-)
I'm a little perplexed by why only the 386 Windows 2008 trybot thinks STACK_SIZE_PARAM_IS_A_RESERVATION isn't defined, but I suppose I can define it if it's not defined without getting into trouble.
I will try and find some time to debug this on weekend.
Alex
2 comments:
Patch Set #4, Line 31:
20975
You mention issue #20975. […]
This CL affects both cgo and non-cgo code since minit runs in both cases. That was intentional, since users may editbin even a non-cgo binary to set a larger stack size if they're calling out to C code using syscall.
File src/runtime/os_windows.go:
I don't think you are correct. […]
Interesting. But the documentation on dwStackSize says "If this parameter is zero, the new thread uses the default size for the executable" which is what I want. If I pass both 0 and STACK_SIZE_PARAM_IS_A_RESERVATION it seems like it would either 1) ignore the flag because it's using the defaults from the executable anyway (presumably for both the reserve and commit sizes), or 2) reserve 0 bytes for the stack, which would definitely be bad :)
To view, visit change 120336. To unsubscribe, or for help writing mail filters, visit settings.
Austin Clements (Gerrit)
1 comment:
Interesting. […]
Forgot to add: if I understand what you're saying, we do have tests for it: TestWindowsStackMemory and TestWindowsStackMemoryCgo. Each forces Go to create 100 threads and checks that the committed memory is small. Though TestWindowsStackMemoryCgo is marked as flaky (issue #22575, filed by you, actually :)
To view, visit change 120336. To unsubscribe, or for help writing mail filters, visit settings.
Austin Clements (Gerrit)
Austin Clements uploaded patch set #5 to this change.
runtime: query thread stack size from OS on Windows
Currently, on Windows, the thread stack size is set or assumed in many
different places. In non-cgo binaries, both the Go linker and the
runtime have a copy of the stack size, the Go linker sets the size of
the main thread stack, and the runtime sets the size of other thread
stacks. In cgo binaries, the external linker sets the main thread
stack size, the runtime assumes the size of the main thread stack will
be the same as used by the Go linker, and the cgo entry code assumes
the same.
Furthermore, users can change the main thread stack size using
editbin, so the runtime doesn't even really know what size it is, and
user C code can create threads with unknown thread stack sizes, which
we also assume have the same default stack size.
This is all a mess.
Fix the corner cases of this and the duplication of knowledge between
the linker and the runtime by querying the OS for the stack bounds
during thread setup. Furthermore, we unify all of this into just
runtime.minit for both cgo and non-cgo binaries and for the main
thread, other runtime-created threads, and C-created threads.
Updates #20975.
Change-Id: I45dbee2b5ea2ae721a85a27680737ff046f9d464
---
M src/cmd/link/internal/ld/pe.go
M src/cmd/vet/all/whitelist/386.txt
M src/cmd/vet/all/whitelist/amd64.txt
M src/cmd/vet/all/whitelist/nacl_amd64p32.txt
M src/runtime/cgo/gcc_windows_386.c
M src/runtime/cgo/gcc_windows_amd64.c
M src/runtime/crash_cgo_test.go
M src/runtime/defs_windows.go
M src/runtime/defs_windows_386.go
M src/runtime/defs_windows_amd64.go
M src/runtime/os_windows.go
M src/runtime/proc.go
A src/runtime/stubs_x86.go
M src/runtime/sys_windows_386.s
M src/runtime/sys_windows_amd64.s
M src/runtime/syscall_windows_test.go
A src/runtime/testdata/testprogcgo/bigstack_windows.c
A src/runtime/testdata/testprogcgo/bigstack_windows.go
18 files changed, 208 insertions(+), 50 deletions(-)
To view, visit change 120336. To unsubscribe, or for help writing mail filters, visit settings.
Gobot Gobot (Gerrit)
Austin Clements (Gerrit)
PS5 adds a non-cgo test for stack size
Gobot Gobot (Gerrit)
TryBots are happy.
Patch set 5:TryBot-Result +1
Alex Brainman (Gerrit)
I'm going to try to add a non-cgo test today as well (based on the DLL-building tests you pointed to in the issue).
I can see you have done that.
I'm a little perplexed by why only the 386 Windows 2008 trybot thinks STACK_SIZE_PARAM_IS_A_RESERVATION isn't defined, but I suppose I can define it if it's not defined without getting into trouble.
Windows does not come with C compiler or build tools. So whatever Brad installed on that builder is what we get. But I agree looks strange that STACK_SIZE_PARAM_IS_A_RESERVATION is not found. Perhaps it is "new" flag. I can see you just defined it, and it seems to work.
Sorry for pushing you around, but we have better tests now out of my whinging. Hopefully new tests are not flaky or take too much resources.
Thank you.
Alex
Patch set 5:Code-Review +2
2 comments:
Patch Set #4, Line 31:
20975
This CL affects both cgo and non-cgo code since minit runs in both cases. […]
Ack
File src/runtime/os_windows.go:
Interesting. But the documentation on dwStackSize says "If this parameter is zero, the new thread uses the default size for the executable" which is what I want. If I pass both 0 and STACK_SIZE_PARAM_IS_A_RESERVATION it seems like it would either 1) ignore the flag because it's using the defaults from the executable anyway (presumably for both the reserve and commit sizes), or 2) reserve 0 bytes for the stack, which would definitely be bad :)
Sorry I confused you. Your code does look right.
Old code reserved 0x200000 bytes for amd64 by explicitly providing 0x200000 with STACK_SIZE_PARAM_IS_A_RESERVATION, and committed 0x1000 bytes (by virtue of using what SizeOfStackCommit says in PE file header).
And your new code just asks for values as per what is in PE file header: reserved = 0x200000 and committed 0x1000.
From https://docs.microsoft.com/en-us/windows/desktop/ProcThread/thread-stack-size
The default size for the reserved and initially committed stack memory is specified in the executable file header.
...
To change the initially committed stack space, use the dwStackSize parameter of the CreateThread
...
To change the reserved stack size, set the dwCreationFlags parameter of CreateThread or CreateRemoteThread to STACK_SIZE_PARAM_IS_A_RESERVATION and use the dwStackSize parameter.
Forgot to add: if I understand what you're saying, we do have tests for it: TestWindowsStackMemory and TestWindowsStackMemoryCgo. Each forces Go to create 100 threads and checks that the committed memory is small. Though TestWindowsStackMemoryCgo is marked as flaky (issue #22575, filed by you, actually :)
Yes, I was remembering we, probably, had a test like that. I worry too much (and it was Friday evening when I reviewed your code last).
To view, visit change 120336. To unsubscribe, or for help writing mail filters, visit settings.
Austin Clements (Gerrit)
Thanks!
Gobot Gobot (Gerrit)
TryBots beginning. Status page: https://farmer.golang.org/try?commit=f38a595a
Gobot Gobot (Gerrit)
TryBots are happy.
Patch set 6:TryBot-Result +1
Austin Clements (Gerrit)
Austin Clements merged this change.
runtime: query thread stack size from OS on Windows
Currently, on Windows, the thread stack size is set or assumed in many
different places. In non-cgo binaries, both the Go linker and the
runtime have a copy of the stack size, the Go linker sets the size of
the main thread stack, and the runtime sets the size of other thread
stacks. In cgo binaries, the external linker sets the main thread
stack size, the runtime assumes the size of the main thread stack will
be the same as used by the Go linker, and the cgo entry code assumes
the same.
Furthermore, users can change the main thread stack size using
editbin, so the runtime doesn't even really know what size it is, and
user C code can create threads with unknown thread stack sizes, which
we also assume have the same default stack size.
This is all a mess.
Fix the corner cases of this and the duplication of knowledge between
the linker and the runtime by querying the OS for the stack bounds
during thread setup. Furthermore, we unify all of this into just
runtime.minit for both cgo and non-cgo binaries and for the main
thread, other runtime-created threads, and C-created threads.
Updates #20975.
Change-Id: I45dbee2b5ea2ae721a85a27680737ff046f9d464
Reviewed-on: https://go-review.googlesource.com/120336
Run-TryBot: Austin Clements <aus...@google.com>
TryBot-Result: Gobot Gobot <go...@golang.org>
Reviewed-by: Alex Brainman <alex.b...@gmail.com>
Reviewed-by: Ian Lance Taylor <ia...@golang.org>
---
M src/cmd/link/internal/ld/pe.go
M src/cmd/vet/all/whitelist/386.txt
M src/cmd/vet/all/whitelist/amd64.txt
M src/cmd/vet/all/whitelist/nacl_amd64p32.txt
M src/runtime/cgo/gcc_windows_386.c
M src/runtime/cgo/gcc_windows_amd64.c
M src/runtime/crash_cgo_test.go
M src/runtime/defs_windows.go
M src/runtime/defs_windows_386.go
M src/runtime/defs_windows_amd64.go
M src/runtime/os_windows.go
M src/runtime/proc.go
A src/runtime/stubs_x86.go
M src/runtime/sys_windows_386.s
M src/runtime/sys_windows_amd64.s
M src/runtime/syscall_windows_test.go
A src/runtime/testdata/testprogcgo/bigstack_windows.c
A src/runtime/testdata/testprogcgo/bigstack_windows.go
18 files changed, 208 insertions(+), 50 deletions(-)
diff --git a/src/cmd/link/internal/ld/pe.go b/src/cmd/link/internal/ld/pe.go
index 3b7df9a..efd971c 100644
--- a/src/cmd/link/internal/ld/pe.go
+++ b/src/cmd/link/internal/ld/pe.go
@@ -845,15 +845,18 @@
// and system calls even in "pure" Go code are actually C
// calls that may need more stack than we think.
//
- // The default stack reserve size affects only the main
+ // The default stack reserve size directly 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.
- // For these, the reserve must match STACKSIZE in
- // runtime/cgo/gcc_windows_{386,amd64}.c and osStackSize in
- // runtime/os_windows.go.
+ // For other threads, the runtime explicitly asks the kernel
+ // to use the default stack size so that all stacks are
+ // consistent.
+ //
+ // At thread start, in minit, the runtime queries the OS for
+ // the actual stack bounds so that the stack size doesn't need
+ // to be hard-coded into the runtime.
oh64.SizeOfStackReserve = 0x00200000
if !iscgo {
oh64.SizeOfStackCommit = 0x00001000
diff --git a/src/cmd/vet/all/whitelist/386.txt b/src/cmd/vet/all/whitelist/386.txt
index 76e8231..f59094e 100644
--- a/src/cmd/vet/all/whitelist/386.txt
+++ b/src/cmd/vet/all/whitelist/386.txt
@@ -22,5 +22,3 @@
runtime/asm_386.s: [386] uint32tofloat64: function uint32tofloat64 missing Go declaration
runtime/asm_386.s: [386] float64touint32: function float64touint32 missing Go declaration
-
-runtime/asm_386.s: [386] stackcheck: function stackcheck missing Go declaration
diff --git a/src/cmd/vet/all/whitelist/amd64.txt b/src/cmd/vet/all/whitelist/amd64.txt
index 2268b39..20e0d48 100644
--- a/src/cmd/vet/all/whitelist/amd64.txt
+++ b/src/cmd/vet/all/whitelist/amd64.txt
@@ -20,4 +20,3 @@
runtime/asm_amd64.s: [amd64] addmoduledata: function addmoduledata missing Go declaration
runtime/duff_amd64.s: [amd64] duffzero: function duffzero missing Go declaration
runtime/duff_amd64.s: [amd64] duffcopy: function duffcopy missing Go declaration
-runtime/asm_amd64.s: [amd64] stackcheck: function stackcheck missing Go declaration
diff --git a/src/cmd/vet/all/whitelist/nacl_amd64p32.txt b/src/cmd/vet/all/whitelist/nacl_amd64p32.txt
index 9280c68..1ec11f7 100644
--- a/src/cmd/vet/all/whitelist/nacl_amd64p32.txt
+++ b/src/cmd/vet/all/whitelist/nacl_amd64p32.txt
@@ -24,5 +24,3 @@
runtime/asm_amd64p32.s: [amd64p32] rt0_go: unknown variable argv
runtime/asm_amd64p32.s: [amd64p32] asmcgocall: RET without writing to 4-byte ret+8(FP)
-
-runtime/asm_amd64p32.s: [amd64p32] stackcheck: function stackcheck missing Go declaration
diff --git a/src/runtime/cgo/gcc_windows_386.c b/src/runtime/cgo/gcc_windows_386.c
index e80a564..f2ff710 100644
--- a/src/runtime/cgo/gcc_windows_386.c
+++ b/src/runtime/cgo/gcc_windows_386.c
@@ -11,16 +11,9 @@
static void threadentry(void*);
-/* 1MB is default stack size for 32-bit Windows.
- Allocation granularity on Windows is typically 64 KB.
- This constant must match SizeOfStackReserve in ../cmd/link/internal/ld/pe.go. */
-#define STACKSIZE (1*1024*1024)
-
void
x_cgo_init(G *g)
{
- int tmp;
- g->stacklo = (uintptr)&tmp - STACKSIZE + 8*1024;
}
@@ -44,8 +37,7 @@
ts = *(ThreadStart*)v;
free(v);
- ts.g->stackhi = (uintptr)&ts;
- ts.g->stacklo = (uintptr)&ts - STACKSIZE + 8*1024;
+ // minit queries stack bounds from the OS.
/*
* Set specific keys in thread local storage.
diff --git a/src/runtime/cgo/gcc_windows_amd64.c b/src/runtime/cgo/gcc_windows_amd64.c
index 75a7dc8..511ab44 100644
--- a/src/runtime/cgo/gcc_windows_amd64.c
+++ b/src/runtime/cgo/gcc_windows_amd64.c
@@ -11,16 +11,9 @@
static void threadentry(void*);
-/* 2MB is default stack size for 64-bit Windows.
- Allocation granularity on Windows is typically 64 KB.
- This constant must match SizeOfStackReserve in ../cmd/link/internal/ld/pe.go. */
-#define STACKSIZE (2*1024*1024)
-
void
x_cgo_init(G *g)
{
- int tmp;
- g->stacklo = (uintptr)&tmp - STACKSIZE + 8*1024;
}
@@ -44,8 +37,7 @@
ts = *(ThreadStart*)v;
free(v);
- ts.g->stackhi = (uintptr)&ts;
- ts.g->stacklo = (uintptr)&ts - STACKSIZE + 8*1024;
+ // minit queries stack bounds from the OS.
/*
* Set specific keys in thread local storage.
diff --git a/src/runtime/crash_cgo_test.go b/src/runtime/crash_cgo_test.go
index d8f75a4..b2ee8df 100644
--- a/src/runtime/crash_cgo_test.go
+++ b/src/runtime/crash_cgo_test.go
@@ -489,3 +489,19 @@
t.Fatalf("failure incorrectly contains %q. output:\n%s\n", nowant, got)
}
}
+
+// Test that C code called via cgo can use large Windows thread stacks
+// and call back in to Go without crashing. See issue #20975.
+//
+// See also TestBigStackCallbackSyscall.
+func TestBigStackCallbackCgo(t *testing.T) {
+ if runtime.GOOS != "windows" {
+ t.Skip("skipping windows specific test")
+ }
+ t.Parallel()
+ got := runTestProg(t, "testprogcgo", "BigStack")
+ want := "OK\n"
+ if got != want {
+ t.Errorf("expected %q got %v", want, got)
+ }
+}
diff --git a/src/runtime/defs_windows.go b/src/runtime/defs_windows.go
index 7ce6797..9bd9107 100644
--- a/src/runtime/defs_windows.go
+++ b/src/runtime/defs_windows.go
@@ -71,3 +71,4 @@
type M128a C.M128A
type Context C.CONTEXT
type Overlapped C.OVERLAPPED
+type MemoryBasicInformation C.MEMORY_BASIC_INFORMATION
diff --git a/src/runtime/defs_windows_386.go b/src/runtime/defs_windows_386.go
index bac6ce7..589a788 100644
--- a/src/runtime/defs_windows_386.go
+++ b/src/runtime/defs_windows_386.go
@@ -129,3 +129,13 @@
anon0 [8]byte
hevent *byte
}
+
+type memoryBasicInformation struct {
+ baseAddress uintptr
+ allocationBase uintptr
+ allocationProtect uint32
+ regionSize uintptr
+ state uint32
+ protect uint32
+ type_ uint32
+}
diff --git a/src/runtime/defs_windows_amd64.go b/src/runtime/defs_windows_amd64.go
index 6e04568..1e173e9 100644
--- a/src/runtime/defs_windows_amd64.go
+++ b/src/runtime/defs_windows_amd64.go
@@ -151,3 +151,13 @@
anon0 [8]byte
hevent *byte
}
+
+type memoryBasicInformation struct {
+ baseAddress uintptr
+ allocationBase uintptr
+ allocationProtect uint32
+ regionSize uintptr
+ state uint32
+ protect uint32
+ type_ uint32
+}
diff --git a/src/runtime/os_windows.go b/src/runtime/os_windows.go
index 1f3ebf6..bf5baea 100644
--- a/src/runtime/os_windows.go
+++ b/src/runtime/os_windows.go
@@ -45,6 +45,7 @@
//go:cgo_import_dynamic runtime._SwitchToThread SwitchToThread%0 "kernel32.dll"
//go:cgo_import_dynamic runtime._VirtualAlloc VirtualAlloc%4 "kernel32.dll"
//go:cgo_import_dynamic runtime._VirtualFree VirtualFree%3 "kernel32.dll"
+//go:cgo_import_dynamic runtime._VirtualQuery VirtualQuery%3 "kernel32.dll"
//go:cgo_import_dynamic runtime._WSAGetOverlappedResult WSAGetOverlappedResult%5 "ws2_32.dll"
//go:cgo_import_dynamic runtime._WaitForSingleObject WaitForSingleObject%2 "kernel32.dll"
//go:cgo_import_dynamic runtime._WriteConsoleW WriteConsoleW%5 "kernel32.dll"
@@ -92,6 +93,7 @@
_SwitchToThread,
_VirtualAlloc,
_VirtualFree,
+ _VirtualQuery,
_WSAGetOverlappedResult,
_WaitForSingleObject,
_WriteConsoleW,
@@ -291,9 +293,6 @@
}
}
-// osStackSize must match SizeOfStackReserve in ../cmd/link/internal/ld/pe.go.
-var osStackSize uintptr = 0x00200000*_64bit + 0x00100000*(1-_64bit)
-
func osinit() {
asmstdcallAddr = unsafe.Pointer(funcPC(asmstdcall))
usleep2Addr = unsafe.Pointer(funcPC(usleep2))
@@ -322,18 +321,6 @@
// equivalent threads that all do a mix of GUI, IO, computations, etc.
// In such context dynamic priority boosting does nothing but harm, so we turn it off.
stdcall2(_SetProcessPriorityBoost, currentProcess, 1)
-
- // Fix the entry thread's stack bounds, since runtime entry
- // assumed we were on a tiny stack. If this is a cgo binary,
- // x_cgo_init already fixed these.
- if !iscgo {
- // Leave 8K of slop for calling C functions that don't
- // have stack checks. We shouldn't be anywhere near
- // this bound anyway.
- g0.stack.lo = g0.stack.hi - osStackSize + 8*1024
- g0.stackguard0 = g0.stack.lo + _StackGuard
- g0.stackguard1 = g0.stackguard0
- }
}
func nanotime() int64
@@ -634,10 +621,10 @@
//go:nowritebarrierrec
//go:nosplit
func newosproc(mp *m) {
- const _STACK_SIZE_PARAM_IS_A_RESERVATION = 0x00010000
- thandle := stdcall6(_CreateThread, 0, osStackSize,
+ // We pass 0 for the stack size to use the default for this binary.
+ thandle := stdcall6(_CreateThread, 0, 0,
funcPC(tstart_stdcall), uintptr(unsafe.Pointer(mp)),
- _STACK_SIZE_PARAM_IS_A_RESERVATION, 0)
+ 0, 0)
if thandle == 0 {
if atomic.Load(&exiting) != 0 {
@@ -702,6 +689,30 @@
var thandle uintptr
stdcall7(_DuplicateHandle, currentProcess, currentThread, currentProcess, uintptr(unsafe.Pointer(&thandle)), 0, 0, _DUPLICATE_SAME_ACCESS)
atomic.Storeuintptr(&getg().m.thread, thandle)
+
+ // Query the true stack base from the OS. Currently we're
+ // running on a small assumed stack.
+ var mbi memoryBasicInformation
+ res := stdcall3(_VirtualQuery, uintptr(unsafe.Pointer(&mbi)), uintptr(unsafe.Pointer(&mbi)), unsafe.Sizeof(mbi))
+ if res == 0 {
+ print("runtime: VirtualQuery failed; errno=", getlasterror(), "\n")
+ throw("VirtualQuery for stack base failed")
+ }
+ // Add 8K of slop for calling C functions that don't have
+ // stack checks. We shouldn't be anywhere near this bound
+ // anyway.
+ base := mbi.allocationBase + 8*1024
+ // Sanity check the stack bounds.
+ g0 := getg()
+ if base > g0.stack.hi || g0.stack.hi-base > 64<<20 {
+ print("runtime: g0 stack [", hex(base), ",", hex(g0.stack.hi), ")\n")
+ throw("bad g0 stack")
+ }
+ g0.stack.lo = base
+ g0.stackguard0 = g0.stack.lo + _StackGuard
+ g0.stackguard1 = g0.stackguard0
+ // Sanity check the SP.
+ stackcheck()
}
// Called from dropm to undo the effect of an minit.
diff --git a/src/runtime/proc.go b/src/runtime/proc.go
index b548632..f82014e 100644
--- a/src/runtime/proc.go
+++ b/src/runtime/proc.go
@@ -1233,6 +1233,7 @@
if osStack {
// Initialize stack bounds from system stack.
// Cgo may have left stack size in stack.hi.
+ // minit may update the stack bounds.
size := _g_.stack.hi
if size == 0 {
size = 8192 * sys.StackGuardMultiplier
diff --git a/src/runtime/stubs_x86.go b/src/runtime/stubs_x86.go
new file mode 100644
index 0000000..830c48b
--- /dev/null
+++ b/src/runtime/stubs_x86.go
@@ -0,0 +1,10 @@
+// Copyright 2018 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// +build amd64 amd64p32 386
+
+package runtime
+
+// stackcheck checks that SP is in range [g->stack.lo, g->stack.hi).
+func stackcheck()
diff --git a/src/runtime/sys_windows_386.s b/src/runtime/sys_windows_386.s
index 56d5cfa..3c091ad 100644
--- a/src/runtime/sys_windows_386.s
+++ b/src/runtime/sys_windows_386.s
@@ -315,8 +315,7 @@
// Layout new m scheduler stack on os stack.
MOVL SP, AX
MOVL AX, (g_stack+stack_hi)(DX)
- SUBL runtime·osStackSize(SB), AX // stack size
- ADDL $(8*1024), AX // slop for calling C
+ SUBL $(64*1024), AX // initial stack size (adjusted later)
MOVL AX, (g_stack+stack_lo)(DX)
ADDL $const__StackGuard, AX
MOVL AX, g_stackguard0(DX)
diff --git a/src/runtime/sys_windows_amd64.s b/src/runtime/sys_windows_amd64.s
index 119e04c..c1449db 100644
--- a/src/runtime/sys_windows_amd64.s
+++ b/src/runtime/sys_windows_amd64.s
@@ -363,8 +363,7 @@
// Layout new m scheduler stack on os stack.
MOVQ SP, AX
MOVQ AX, (g_stack+stack_hi)(DX)
- SUBQ runtime·osStackSize(SB), AX // stack size
- ADDQ $(8*1024), AX // slop for calling C
+ SUBQ $(64*1024), AX // inital stack size (adjusted later)
MOVQ AX, (g_stack+stack_lo)(DX)
ADDQ $const__StackGuard, AX
MOVQ AX, g_stackguard0(DX)
diff --git a/src/runtime/syscall_windows_test.go b/src/runtime/syscall_windows_test.go
index 0f5e13f..0882e9c 100644
--- a/src/runtime/syscall_windows_test.go
+++ b/src/runtime/syscall_windows_test.go
@@ -957,6 +957,52 @@
}
}
+// Test that C code called via a DLL can use large Windows thread
+// stacks and call back in to Go without crashing. See issue #20975.
+//
+// See also TestBigStackCallbackCgo.
+func TestBigStackCallbackSyscall(t *testing.T) {
+ if _, err := exec.LookPath("gcc"); err != nil {
+ t.Skip("skipping test: gcc is missing")
+ }
+
+ srcname, err := filepath.Abs("testdata/testprogcgo/bigstack_windows.c")
+ if err != nil {
+ t.Fatal("Abs failed: ", err)
+ }
+
+ tmpdir, err := ioutil.TempDir("", "TestBigStackCallback")
+ if err != nil {
+ t.Fatal("TempDir failed: ", err)
+ }
+ defer os.RemoveAll(tmpdir)
+
+ outname := "mydll.dll"
+ cmd := exec.Command("gcc", "-shared", "-s", "-Werror", "-o", outname, srcname)
+ cmd.Dir = tmpdir
+ out, err := cmd.CombinedOutput()
+ if err != nil {
+ t.Fatalf("failed to build dll: %v - %v", err, string(out))
+ }
+ dllpath := filepath.Join(tmpdir, outname)
+
+ dll := syscall.MustLoadDLL(dllpath)
+ defer dll.Release()
+
+ var ok bool
+ proc := dll.MustFindProc("bigStack")
+ cb := syscall.NewCallback(func() uintptr {
+ // Do something interesting to force stack checks.
+ forceStackCopy()
+ ok = true
+ return 0
+ })
+ proc.Call(cb)
+ if !ok {
+ t.Fatalf("callback not called")
+ }
+}
+
// wantLoadLibraryEx reports whether we expect LoadLibraryEx to work for tests.
func wantLoadLibraryEx() bool {
return testenv.Builder() == "windows-amd64-gce" || testenv.Builder() == "windows-386-gce"
diff --git a/src/runtime/testdata/testprogcgo/bigstack_windows.c b/src/runtime/testdata/testprogcgo/bigstack_windows.c
new file mode 100644
index 0000000..cd85ac8
--- /dev/null
+++ b/src/runtime/testdata/testprogcgo/bigstack_windows.c
@@ -0,0 +1,46 @@
+// Copyright 2018 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// This test source is used by both TestBigStackCallbackCgo (linked
+// directly into the Go binary) and TestBigStackCallbackSyscall
+// (compiled into a DLL).
+
+#include <windows.h>
+#include <stdio.h>
+
+#ifndef STACK_SIZE_PARAM_IS_A_RESERVATION
+#define STACK_SIZE_PARAM_IS_A_RESERVATION 0x00010000
+#endif
+
+typedef void callback(char*);
+
+// Allocate a stack that's much larger than the default.
+static const int STACK_SIZE = 16<<20;
+
+static callback *bigStackCallback;
+
+static void useStack(int bytes) {
+ // Windows doesn't like huge frames, so we grow the stack 64k at a time.
+ char x[64<<10];
+ if (bytes < sizeof x) {
+ bigStackCallback(x);
+ } else {
+ useStack(bytes - sizeof x);
+ }
+}
+
+static DWORD WINAPI threadEntry(LPVOID lpParam) {
+ useStack(STACK_SIZE - (128<<10));
+ return 0;
+}
+
+void bigStack(callback *cb) {
+ bigStackCallback = cb;
+ HANDLE hThread = CreateThread(NULL, STACK_SIZE, threadEntry, NULL, STACK_SIZE_PARAM_IS_A_RESERVATION, NULL);
+ if (hThread == NULL) {
+ fprintf(stderr, "CreateThread failed\n");
+ exit(1);
+ }
+ WaitForSingleObject(hThread, INFINITE);
+}
diff --git a/src/runtime/testdata/testprogcgo/bigstack_windows.go b/src/runtime/testdata/testprogcgo/bigstack_windows.go
new file mode 100644
index 0000000..f58fcf9
--- /dev/null
+++ b/src/runtime/testdata/testprogcgo/bigstack_windows.go
@@ -0,0 +1,27 @@
+// Copyright 2018 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package main
+
+/*
+typedef void callback(char*);
+extern void goBigStack1(char*);
+extern void bigStack(callback*);
+*/
+import "C"
+
+func init() {
+ register("BigStack", BigStack)
+}
+
+func BigStack() {
+ // Create a large thread stack and call back into Go to test
+ // if Go correctly determines the stack bounds.
+ C.bigStack((*C.callback)(C.goBigStack1))
+}
+
+//export goBigStack1
+func goBigStack1(x *C.char) {
+ println("OK")
+}
To view, visit change 120336. To unsubscribe, or for help writing mail filters, visit settings.