New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

runtime: make crashing more useful on Windows #20498

Open
bhiggins opened this Issue May 25, 2017 · 10 comments

Comments

Projects
None yet
7 participants
@bhiggins

bhiggins commented May 25, 2017

What did you do?

Triggered an access violation, crashing the program.

What did you expect to see?

We configured user-mode dumps and were hoping to get a dump (either a full dump or a minidump). See: https://msdn.microsoft.com/en-us/library/windows/desktop/bb787181(v=vs.85).aspx

What did you see instead?

No dump.

Two suggestions:

  1. In runtime/signal_windows.go, change lastcontinuehandler to return _EXCEPTION_CONTINUE_SEARCH if docrash is true (instead of calling crash and exit(2)-ing). This will allow Windows's crash handling to run instead of being suppressed.
  2. In runtime/signal_windows.go, change crash() to call RaiseException instead of being a no-op.

We can submit a PR if this is agreeable.

@bradfitz bradfitz added this to the Go1.10 milestone May 25, 2017

@bradfitz

This comment has been minimized.

Member

bradfitz commented May 25, 2017

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented May 25, 2017

Try setting the environment variable GOTRACEBACK to "crash".

@ianlancetaylor

This comment has been minimized.

Contributor

ianlancetaylor commented May 25, 2017

I should mention that GOTRACEBACK is documented at https://golang.org/pkg/runtime/ .

@bhiggins

This comment has been minimized.

bhiggins commented May 25, 2017

@ianlancetaylor, thanks, I didn't know about GOTRACEBACK, but it doesn't help here as "crashes in an operating system-specific manner" for Windows is currently a no-op with a TODO comment. See runtime/signal_windows.go (and my second suggestion above):

func crash() {
	// TODO: This routine should do whatever is needed
	// to make the Windows program abort/crash as it
	// would if Go was not intercepting signals.
	// On Unix the routine would remove the custom signal
	// handler and then raise a signal (like SIGABRT).
	// Something like that should happen here.
	// It's okay to leave this empty for now: if crash returns
	// the ordinary exit-after-panic happens.
}
@ptdiscus

This comment has been minimized.

ptdiscus commented May 26, 2017

@ianlancetaylor
I believe calling a "crash" function in lastcontinuehandler would cause the exception context record in the dump to point to the crash function. Returning _EXCEPTION_CONTINUE_SEARCH should cause the exception context record to point to the original source of the exception.

@alexbrainman

This comment has been minimized.

Member

alexbrainman commented May 28, 2017

Two suggestions:

Sounds good to me.

We can submit a PR if this is agreeable.

That is agreeble from me, thank you.
We use this https://golang.org/doc/contribute.html procedure instead of Github PRs.

I didn't know about GOTRACEBACK

I did know about GOTRACEBACK, but I did not know about "crashes in an operating system-specific manner instead of exiting" if GOTRACEBACK=crash. So the "crashing" part is not implemented on Windows.

I believe calling a "crash" function in lastcontinuehandler would cause the exception context record in the dump to point to the crash function.

Correct.

Returning _EXCEPTION_CONTINUE_SEARCH should cause the exception context record to point to the original source of the exception.

Correct. And that is what @bhiggins is proposing to add.

I think we are all on the same page.

Alex

@rsc rsc modified the milestones: Go1.10, Go1.11 Nov 22, 2017

@usirsiwal

This comment has been minimized.

usirsiwal commented Mar 19, 2018

We have a go+C program on Windows. In absence of coredump, it is hard for us to root cause errors in the C code. Will really appreciate resolution of this issue.

@alexbrainman

This comment has been minimized.

Member

alexbrainman commented Mar 20, 2018

Will really appreciate resolution of this issue.

@usirsiwal try changing runtime yourself and see if it helps you. And if it works for you, submit it here to help others.

If you have problems, just ask for help here.

Alex

@usirsiwal

This comment has been minimized.

usirsiwal commented Mar 20, 2018

@alexbrainman I will try the runtime change and see if it works for us.

@usirsiwal

This comment has been minimized.

usirsiwal commented Mar 28, 2018

Hello,
We came up with the following solution. We had to conditionally remove disableWER to get the crashdump. We are not sure if that is the right thing to do. Please let us know if there is a better way of doing it.

Thanks,
Umesh

diff --git "a/C:\\go1.10.windows-amd64\\go\\src\\runtime\\signal_windows.go.orig" "b/C:\\go1.10.windows-amd64\\go\\src\\runtime\\signal_windows.go"
index 518aac3c4..939379ce4 100644
--- "a/C:\\go1.10.windows-amd64\\go\\src\\runtime\\signal_windows.go.orig"
+++ "b/C:\\go1.10.windows-amd64\\go\\src\\runtime\\signal_windows.go"
@@ -148,7 +148,7 @@ func lastcontinuehandler(info *exceptionrecord, r *context, gp *g) int32 {
        }

        if docrash {
-               crash()
+               return _EXCEPTION_CONTINUE_SEARCH
        }

        exit(2)
@@ -228,6 +228,10 @@ func crash() {
        // Something like that should happen here.
        // It's okay to leave this empty for now: if crash returns
        // the ordinary exit-after-panic happens.
+       _, _, docrash := gotraceback()
+       if docrash {
+               raiseException(0xdead)
+       }
 }

 // gsignalStack is unused on Windows.

 
 
 diff --git "a/C:\\go1.10.windows-amd64\\go\\src\\runtime\\os_windows.go.orig" "b/C:\\go1.10.windows-amd64\\go\\src\\runtime\\os_windows.go"
index 7aeadd9ef..18acc4dcf 100644
--- "a/C:\\go1.10.windows-amd64\\go\\src\\runtime\\os_windows.go.orig"
+++ "b/C:\\go1.10.windows-amd64\\go\\src\\runtime\\os_windows.go"
@@ -80,6 +80,7 @@ var (
        _LoadLibraryA,
        _QueryPerformanceCounter,
        _QueryPerformanceFrequency,
+       _RaiseException,
        _ResumeThread,
        _SetConsoleCtrlHandler,
        _SetErrorMode,
@@ -302,7 +303,10 @@ func osinit() {

        useLoadLibraryEx = (_LoadLibraryExW != nil && _AddDllDirectory != nil)

-       disableWER()
+       _, _, docrash := gotraceback()
+       if docrash {
+               disableWER()
+       }

        externalthreadhandlerp = funcPC(externalthreadhandler)

@@ -445,6 +449,15 @@ func exit(code int32) {
        stdcall1(_ExitProcess, uintptr(code))
 }

+//go:nosplit
+func raiseException(code int32) {
+       stdcall4(_RaiseException,
+               uintptr(code),
+               0,
+               0,
+               0)
+}
+
 //go:nosplit
 func write(fd uintptr, buf unsafe.Pointer, n int32) int32 {
        const (
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment