[go] runtime: include throw and fatal message in SetCrashOutput
Joseph Tsai (Gerrit)
Joseph Tsai has uploaded the change for review![]( "Open in Gerrit")
Commit message
runtime: include throw and fatal message in SetCrashOutput
The throw and fatal message were unfortunately excluded
from the SetCrashOutput because the message was printed
before setting the releveant state needed to indicate
that the system is crashing.
Fix this by printing the message after setting m.throwing
and by having writeErrData emit the message
if the goroutine is in the process of throwing.
Fixes #76859
Change diff
diff --git a/src/runtime/panic.go b/src/runtime/panic.go
index d467e93..22b4291 100644
--- a/src/runtime/panic.go
+++ b/src/runtime/panic.go
@@ -1220,13 +1220,11 @@
func throw(s string) {
// Everything throw does should be recursively nosplit so it
// can be called even when it's unsafe to grow the stack.
- systemstack(func() {
+ fatalthrow(throwTypeRuntime, func() {
print("fatal error: ")
printindented(s) // logically printpanicval(s), but avoids convTstring write barrier
print("\n")
})
-
- fatalthrow(throwTypeRuntime)
}
// fatal triggers a fatal error that dumps a stack trace and exits.
@@ -1243,14 +1241,12 @@
// Everything fatal does should be recursively nosplit so it
// can be called even when it's unsafe to grow the stack.
printlock() // Prevent multiple interleaved fatal reports. See issue 69447.
- systemstack(func() {
+ fatalthrow(throwTypeUser, func() {
printPreFatalDeferPanic(p)
print("fatal error: ")
printindented(s) // logically printpanicval(s), but avoids convTstring write barrier
print("\n")
})
-
- fatalthrow(throwTypeUser)
printunlock()
}
@@ -1413,11 +1409,11 @@
}
// fatalthrow implements an unrecoverable runtime throw. It freezes the
-// system, prints stack traces starting from its caller, and terminates the
-// process.
+// system, calls preStackPrint, prints stack traces starting from its caller,
+// and terminates the process.
//
//go:nosplit
-func fatalthrow(t throwType) {
+func fatalthrow(t throwType, preStackPrint func()) {
pc := sys.GetCallerPC()
sp := sys.GetCallerSP()
gp := getg()
@@ -1429,6 +1425,12 @@
// Switch to the system stack to avoid any stack growth, which may make
// things worse if the runtime is in a bad state.
systemstack(func() {
+ // Print extra information after setting m.throwing so that
+ //it is also captured in the debug.SetCrashOutput file.
+ if preStackPrint != nil {
+ preStackPrint()
+ }
+
if isSecureMode() {
exit(2)
}
diff --git a/src/runtime/runtime.go b/src/runtime/runtime.go
index 016cbda..8aa9740 100644
--- a/src/runtime/runtime.go
+++ b/src/runtime/runtime.go
@@ -235,6 +235,7 @@
// If crashing, print a copy to the SetCrashOutput fd.
gp := getg()
if gp != nil && gp.m.dying > 0 ||
+ gp != nil && gp.m.throwing > 0 ||
gp == nil && panicking.Load() > 0 {
if fd := crashFD.Load(); fd != ^uintptr(0) {
write(fd, unsafe.Pointer(data), n)
Change information
- M src/runtime/panic.go
- M src/runtime/runtime.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. |
Joseph Tsai (Gerrit)
Joseph Tsai added 1 comment![]( "Open in Gerrit")
I'm not sure what's the best way to test this, but happy to hear suggestions. I did test it locally with the example linked in the issue.
Related details
- Michael Knyszek
- Michael Pratt
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michael Knyszek (Gerrit)
Michael Knyszek added 4 comments![]( "Open in Gerrit")
I'm not sure what's the best way to test this, but happy to hear suggestions. I did test it locally with the example linked in the issue.
the best thing would probably be to generalize TestSetCrashOutput https://cs.opensource.google/go/go/+/master:src/runtime/debug/stack_test.go;l=134;drc=dceee2e983f5dab65c3905ecf40e70e15cf41b7d, and crash for a different reason (like concurrent map writes), if you're up for it! otherwise just copying it is probably fine for now, we can clean it up later.
thanks!
// system, calls preStackPrint, prints stack traces starting from its caller,I think it would be helpful to explain why preStackPrint exists at all, since it's a bit odd. it's also worth documenting that the function will run on the system stack. (if it didn't, the preStackPrint should possibly be limited to nosplit functions. it's better if the caller doesn't have to wonder.)
I can't think of a nicer way to do this, and it does have upsides. we could set `gp.m.throwing` earlier via some other function, but that doesn't seem great either.
//it is also captured in the debug.SetCrashOutput file.nit: need extra space.
Related details
- Joseph Tsai
- Michael Pratt
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joseph Tsai (Gerrit)
Joseph Tsai uploaded new patchset![]( "Open in Gerrit")
Related details
- Joseph Tsai
- Michael Pratt
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joseph Tsai (Gerrit)
Joseph Tsai voted and added 2 comments![]( "Open in Gerrit")
Votes added by Joseph Tsai
| Commit-Queue | +1 |
2 comments
// system, calls preStackPrint, prints stack traces starting from its caller,I think it would be helpful to explain why preStackPrint exists at all, since it's a bit odd. it's also worth documenting that the function will run on the system stack. (if it didn't, the preStackPrint should possibly be limited to nosplit functions. it's better if the caller doesn't have to wonder.)
I can't think of a nicer way to do this, and it does have upsides. we could set `gp.m.throwing` earlier via some other function, but that doesn't seem great either.
Done
//it is also captured in the debug.SetCrashOutput file.Joseph Tsainit: need extra space.
Done
Related details
- Michael Knyszek
- Michael Pratt
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joseph Tsai (Gerrit)
Joseph Tsai added 1 comment![]( "Open in Gerrit")
Michael KnyszekI'm not sure what's the best way to test this, but happy to hear suggestions. I did test it locally with the example linked in the issue.
the best thing would probably be to generalize TestSetCrashOutput https://cs.opensource.google/go/go/+/master:src/runtime/debug/stack_test.go;l=134;drc=dceee2e983f5dab65c3905ecf40e70e15cf41b7d, and crash for a different reason (like concurrent map writes), if you're up for it! otherwise just copying it is probably fine for now, we can clean it up later.
Thanks for the tip. I was able to adjust the test, but I needed to use linkname to access `fatalthrow`. Trying to trigger one felt a bit flaky.
Related details
- Michael Knyszek
- Michael Pratt
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |