[go] runtime: print error code on Windows VirtualAlloc failure
Gerrit Bot (Gerrit)
Gerrit Bot has uploaded the change for review![]( "Open in Gerrit")
Commit message
runtime: print error code on Windows VirtualAlloc failure
runtime: print error code on Windows VirtualAlloc failure
When VirtualAlloc fails on Windows, the resulting crash is currently
opaque. It is difficult to distinguish between physical memory exhaustion
(ERROR_NOT_ENOUGH_MEMORY), the commit limit being reached
(ERROR_COMMITMENT_LIMIT), or other system limits.
This change prints the GetLastError code to stderr when VirtualAlloc
returns nil to aid debugging.
Fixes #70558
Change diff
diff --git a/src/runtime/mem_windows.go b/src/runtime/mem_windows.go
index afc2dee..5729aa5 100644
--- a/src/runtime/mem_windows.go
+++ b/src/runtime/mem_windows.go
@@ -26,7 +26,13 @@
//
//go:nosplit
func sysAllocOS(n uintptr, _ string) unsafe.Pointer {
- return unsafe.Pointer(stdcall(_VirtualAlloc, 0, n, _MEM_COMMIT|_MEM_RESERVE, _PAGE_READWRITE))
+ p := stdcall(_VirtualAlloc, 0, n, _MEM_COMMIT|_MEM_RESERVE, _PAGE_READWRITE)
+ if p == 0 {
+ errno := getlasterror()
+ print("runtime: VirtualAlloc of ", n, " bytes failed with errno=", errno, "\n")
+ return nil
+ }
+ return unsafe.Pointer(p)
}
func sysUnusedOS(v unsafe.Pointer, n uintptr) {
@@ -120,14 +126,19 @@
func sysReserveOS(v unsafe.Pointer, n uintptr, _ string) unsafe.Pointer {
// v is just a hint.
// First try at v.
- // This will fail if any of [v, v+n) is already reserved.
- v = unsafe.Pointer(stdcall(_VirtualAlloc, uintptr(v), n, _MEM_RESERVE, _PAGE_READWRITE))
- if v != nil {
- return v
+ p := stdcall(_VirtualAlloc, uintptr(v), n, _MEM_RESERVE, _PAGE_READWRITE)
+ if p != 0 {
+ return unsafe.Pointer(p)
}
// Next let the kernel choose the address.
- return unsafe.Pointer(stdcall(_VirtualAlloc, 0, n, _MEM_RESERVE, _PAGE_READWRITE))
+ p = stdcall(_VirtualAlloc, 0, n, _MEM_RESERVE, _PAGE_READWRITE)
+ if p == 0 {
+ errno := getlasterror()
+ print("runtime: VirtualAlloc of ", n, " bytes failed with errno=", errno, "\n")
+ return nil
+ }
+ return unsafe.Pointer(p)
}
func sysMapOS(v unsafe.Pointer, n uintptr, _ string) {
Change information
- M src/runtime/mem_windows.go
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Brainman (Gerrit)
Alex Brainman added 2 comments![]( "Open in Gerrit")
Thanks for sending the CL.
Please, answer my question below.
Alex
print("runtime: VirtualAlloc of ", n, " bytes failed with errno=", errno, "\n")If you look at the log https://logs.chromium.org/logs/golang/buildbucket/cr-buildbucket/8730352753718672849/+/u/step/19/log/2 mentioned at the issue https://github.com/golang/go/issues/70558#issue-2691890092 , you will see that the stack trace already prints that information, like
```
runtime: VirtualAlloc of 8192 bytes failed with errno=1455
```
Why do we need your CL to print similar information?
Related details
- Austin Clements
- Keith Randall
- Martin Möhrmann
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bill Morgan (Gerrit)
Bill Morgan added 2 comments![]( "Open in Gerrit")
Thanks for the clarification Alex. I replied below.
print("runtime: VirtualAlloc of ", n, " bytes failed with errno=", errno, "\n")If you look at the log https://logs.chromium.org/logs/golang/buildbucket/cr-buildbucket/8730352753718672849/+/u/step/19/log/2 mentioned at the issue https://github.com/golang/go/issues/70558#issue-2691890092 , you will see that the stack trace already prints that information, like
```
runtime: VirtualAlloc of 8192 bytes failed with errno=1455
```Why do we need your CL to print similar information?
Thanks, I see what you mean now Alex. I tested by doing `_ = make([]byte, 200*1024*1024*1024*1024)` and did not get the errno printed out so I modified `sysAllocOS` and `sysReserveOS` behavior to be in line with `sysUsedOS` and tested to confirm I got the errno for that huge allocation failure. After a closer re-reading of the issue, I realize this doesn't really address the deeper 'unactionable' part of the original issue Alan mentioned though.
Related details
- Alex Brainman
- Austin Clements
- Keith Randall
- Martin Möhrmann
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Brainman (Gerrit)
Alex Brainman voted and added 4 comments![]( "Open in Gerrit")
Votes added by Alex Brainman
| Commit-Queue | +1 |
4 comments
Thanks for the clarification Alex. I replied below.
I did not review your CL properly. I will do it once we agree what to do here.
Fixes #70558s/Fixes/For/
otherwise the issue will get close when we submit this CL.
print("runtime: VirtualAlloc of ", n, " bytes failed with errno=", errno, "\n")Bill MorganIf you look at the log https://logs.chromium.org/logs/golang/buildbucket/cr-buildbucket/8730352753718672849/+/u/step/19/log/2 mentioned at the issue https://github.com/golang/go/issues/70558#issue-2691890092 , you will see that the stack trace already prints that information, like
```
runtime: VirtualAlloc of 8192 bytes failed with errno=1455
```Why do we need your CL to print similar information?
Thanks, I see what you mean now Alex. I tested by doing `_ = make([]byte, 200*1024*1024*1024*1024)` and did not get the errno printed out so I modified `sysAllocOS` and `sysReserveOS` behavior to be in line with `sysUsedOS` and tested to confirm I got the errno for that huge allocation failure. After a closer re-reading of the issue, I realize this doesn't really address the deeper 'unactionable' part of the original issue Alan mentioned though.
Thanks for explaining what you see. I run this program
```
package main
import (
"fmt"
)
func main() {
_ = make([]byte, 200*1024*1024*1024*1024)
fmt.Printf("OK\n")
}
```and indeed I see this stack trace
```
runtime: out of memory: cannot allocate 219902325555200-byte block (4096000 in use)
fatal error: out of memory
goroutine 1 gp=0xc0000021c0 m=0 mp=0x7ff6619afbe0 [running]:
...
```
so you are correct Windows error number is missing here.
I suggest you add test to make sure that your change does not get lost.
I also think we should stick to the same message format as in
but instead of
```
runtime: VirtualAlloc of 8192 bytes failed with errno=1455
fatal error: out of memory
```
here it should say
```
runtime: VirtualAlloc of 8192 bytes failed with errno=1455: cannot allocate 219902325555200-byte block (4096000 in use)
fatal error: out of memory
```
Please, confirm my suggestion at https://github.com/golang/go/issues/70558 to make sure everyone agrees.
p := stdcall(_VirtualAlloc, uintptr(v), n, _MEM_RESERVE, _PAGE_READWRITE)We need new test and adjust the message text similar to what I suggested in sysAllocOS.
Related details
- Austin Clements
- Bill Morgan
- Keith Randall
- Martin Möhrmann
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- Alex Brainman
- Austin Clements
- Bill Morgan
- Keith Randall
- Martin Möhrmann
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Alex Brainman
- Austin Clements
- Bill Morgan
- Keith Randall
- Martin Möhrmann
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
- Alex Brainman
- Austin Clements
- Bill Morgan
- Keith Randall
- Martin Möhrmann
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
Related details
- Alex Brainman
- Austin Clements
- Bill Morgan
- Keith Randall
- Martin Möhrmann
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bill Morgan (Gerrit)
Bill Morgan added 4 comments![]( "Open in Gerrit")
Thanks Alex. I have pushed updates and commented on the issue.
s/Fixes/For/
otherwise the issue will get close when we submit this CL.
Done
print("runtime: VirtualAlloc of ", n, " bytes failed with errno=", errno, "\n")Done
p := stdcall(_VirtualAlloc, uintptr(v), n, _MEM_RESERVE, _PAGE_READWRITE)We need new test and adjust the message text similar to what I suggested in sysAllocOS.
- Alex Brainman
- Austin Clements
- Keith Randall
- Martin Möhrmann
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Brainman (Gerrit)
Alex Brainman voted and added 4 comments![]( "Open in Gerrit")
Votes added by Alex Brainman
| Commit-Queue | +1 |
4 comments
More questions.
Thank you.
Alex
if !regexp.MustCompile(want).MatchString(output) {I would not use `regexp.MustCompile` here. Just search for the string you want. Search for this string
```
runtime: VirtualAlloc of 211106236727296 bytes failed with errno=87: cannot allocate 211106236727296-byte block (4096000 in use)
fatal error: out of memory
```
This way you will avoid bugs with dup lines.
print("runtime: VirtualAlloc of ", n, " bytes failed with errno=", errno, "\n")I tried running your new test, and the stack trace after your CL now starts with
```
runtime: VirtualAlloc of 211106236727296 bytes failed with errno=87: cannot allocate 211106236727296-byte block (4096000 in use)
runtime: out of memory: cannot allocate 211106232532992-byte block (4096000 in use)
fatal error: out of memory
```
and not what I asked for
```
runtime: VirtualAlloc of 211106236727296 bytes failed with errno=87: cannot allocate 211106236727296-byte block (4096000 in use)
fatal error: out of memory
```
The stack trace now has extra `runtime: out of memory: ...` line. I find that extra line confusing.
Is that expected?
b := make([]byte, size)
I assume this test your `runtime.sysAllocOS` change. Is it possible to add tests for your `runtime.sysUnusedOS` and `runtime.sysReserveOS` changes?
Related details
- Austin Clements
- Keith Randall
- Martin Möhrmann
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Alex Brainman
- Austin Clements
- Keith Randall
- Martin Möhrmann
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bill Morgan (Gerrit)
Bill Morgan added 4 comments![]( "Open in Gerrit")
if !regexp.MustCompile(want).MatchString(output) {I would not use `regexp.MustCompile` here. Just search for the string you want. Search for this string
```
runtime: VirtualAlloc of 211106236727296 bytes failed with errno=87: cannot allocate 211106236727296-byte block (4096000 in use)
fatal error: out of memory
```This way you will avoid bugs with dup lines.
I think I still need regex here because `inUse` can vary between runs, and `size` varies between 32-bit and 64-bit architectures. I am also unsure if `errno` is constant across all failure modes. Additionally,`sysUsedOS` has different print output logic (printing `n` or `small`), so static string match wouldn't cover those variations.
The 2nd `runtime:` line printed because the previous code returned `nil`, causing the caller to print its own error message. I have changed `sysAllocOS` to `throw`, which prevents the double print and matches the output format you requested.
I assume this test your `runtime.sysAllocOS` change. Is it possible to add tests for your `runtime.sysUnusedOS` and `runtime.sysReserveOS` changes?
I have rewritten the test to cover all modified functions.
Related details
- Alex Brainman
- Austin Clements
- Keith Randall
- Martin Möhrmann
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Brainman (Gerrit)
Alex Brainman voted and added 4 comments![]( "Open in Gerrit")
Votes added by Alex Brainman
| Commit-Queue | +1 |
4 comments
Nice.
Just one suggestion and then we are done.
Thank you.
Alex
if runtime.GOOS != "windows" {If you move this test into syscall_windows_test.go file, you will not need to check for `runtime.GOOS != "windows"`.
if !regexp.MustCompile(want).MatchString(output) {Bill MorganI would not use `regexp.MustCompile` here. Just search for the string you want. Search for this string
```
runtime: VirtualAlloc of 211106236727296 bytes failed with errno=87: cannot allocate 211106236727296-byte block (4096000 in use)
fatal error: out of memory
```This way you will avoid bugs with dup lines.
I think I still need regex here because `inUse` can vary between runs, and `size` varies between 32-bit and 64-bit architectures. I am also unsure if `errno` is constant across all failure modes. Additionally,`sysUsedOS` has different print output logic (printing `n` or `small`), so static string match wouldn't cover those variations.
Acknowledged
print("runtime: VirtualAlloc of ", n, " bytes failed with errno=", errno, "\n")- Austin Clements
- Bill Morgan
- Keith Randall
- Martin Möhrmann
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Brainman (Gerrit)
Alex Brainman added 1 comment![]( "Open in Gerrit")
Nice.
Just one suggestion and then we are done.
Thank you.
Alex
The suggestion is not optional, because some other platform tests failed. For example,
https://ci.chromium.org/ui/p/golang/builders/try/gotip-linux-386/b8690822241663058017/overview
Bill Morgan (Gerrit)
Bill Morgan added 3 comments![]( "Open in Gerrit")
Alex BrainmanNice.
Just one suggestion and then we are done.
Thank you.
Alex
The suggestion is not optional, because some other platform tests failed. For example,
https://ci.chromium.org/ui/p/golang/builders/try/gotip-linux-386/b8690822241663058017/overview
Done
Thanks Alex.
if runtime.GOOS != "windows" {If you move this test into syscall_windows_test.go file, you will not need to check for `runtime.GOOS != "windows"`.
- Alex Brainman
- Austin Clements
- Keith Randall
- Martin Möhrmann
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gerrit Bot (Gerrit)
Gerrit Bot uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result-1 by Go LUCI
Related details
- Alex Brainman
- Austin Clements
- Keith Randall
- Martin Möhrmann
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Brainman (Gerrit)
Alex Brainman voted Commit-Queue+1![]( "Open in Gerrit")
- Austin Clements
- Keith Randall
- Martin Möhrmann
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Brainman (Gerrit)
Alex Brainman voted and added 1 comment![]( "Open in Gerrit")
Votes added by Alex Brainman
Related details
- Austin Clements
- Keith Randall
- Martin Möhrmann
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex Brainman (Gerrit)
Alex Brainman added 1 comment![]( "Open in Gerrit")
I can see your CL on this list
so we need to wait for 2 +1 Googlers reviews before we can submit your change.
Alex
Michael Knyszek (Gerrit)
Michael Knyszek added 2 comments![]( "Open in Gerrit")
inUse := gcController.heapFree.load() + gcController.heapReleased.load() + gcController.heapInUse.load()I think this should be excluded. on windows this is uncommitted memory, it costs next to nothing. but, see my next comment...
print("runtime: VirtualAlloc of ", n, " bytes failed with errno=", errno, ": cannot allocate ", n, "-byte block (", inUse, " in use)\n")this isn't quite accurate. this adds up only heap memory. there's a better measure for this, but it's not trivial to compute, and I'd rather not duplicate that code. I don't think this terribly useful, I suspect it would be better to just drop this. (sysAlloc specifically tends to have little to do with the heap, for example.)
Related details
- Austin Clements
- Keith Randall
- Martin Möhrmann
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bill Morgan (Gerrit)
Bill Morgan added 1 comment![]( "Open in Gerrit")
I've reviewed the feedback and looked at the original issue #70558 again. Since the inUse stats are inaccurate in this context, this CL is not a solution for the missing informative metrics Alan requested. Since it won't solve the issue, I'm going to abandon this. Thanks for the reviews Alex and Michael.
Gopher Robot (Gerrit)
Gopher Robot abandoned this change.
Gopher Robot abandoned this change![]( "Open in Gerrit")
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |