Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(2017)

Issue 6498057: code review 6498057: runtime: discard SIGPROF delivered to non-Go threads. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 3 months ago by adonovan
Modified:
9 years, 2 months ago
Reviewers:
CC:
iant, rsc, minux1, golang-dev
Visibility:
Public.

Description

runtime: discard SIGPROF delivered to non-Go threads. Signal handlers are global resources but many language environments (Go, C++ at Google, etc) assume they have sole ownership of a particular handler. Signal handlers in mixed-language applications must therefore be robust against unexpected delivery of certain signals, such as SIGPROF. The default Go signal handler runtime·sigtramp assumes that it will never be called on a non-Go thread, but this assumption is violated by when linking in C++ code that spawns threads. Specifically, the handler asserts the thread has an associated "m" (Go scheduler). This CL is a very simple workaround: discard SIGPROF delivered to non-Go threads. runtime.badsignal(int32) now receives the signal number; if it returns without panicking (e.g. sig==SIGPROF) the signal is discarded. I don't think there is any really satisfactory solution to the problem of signal-based profiling in a mixed-language application. It's not only the issue of handler clobbering, but also that a C++ SIGPROF handler called in a Go thread can't unwind the Go stack (and vice versa). The best we can hope for is not crashing. Note: - I've ported this to all POSIX platforms, except ARM-linux which already ignores unexpected signals on m-less threads. - I've avoided tail-calling runtime.badsignal because AFAICT the 6a/6l don't support it. - I've avoided hoisting 'push sig' (common to both function calls) because it makes the code harder to read. - Fixed an (apparently incorrect?) docstring.

Patch Set 1 #

Patch Set 2 : diff -r 008d345d81e0 https://code.google.com/p/go/ #

Patch Set 3 : diff -r 008d345d81e0 https://code.google.com/p/go/ #

Patch Set 4 : diff -r 008d345d81e0 https://code.google.com/p/go/ #

Total comments: 2

Patch Set 5 : diff -r 008d345d81e0 https://code.google.com/p/go/ #

Total comments: 1

Patch Set 6 : diff -r 92e962e13197 https://code.google.com/p/go/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+67 lines, -19 lines) Patch
M src/pkg/runtime/signal_linux_amd64.c View 1 2 3 4 5 1 chunk +2 lines, -0 lines 0 comments Download
M src/pkg/runtime/sys_darwin_386.s View 1 2 2 chunks +6 lines, -3 lines 0 comments Download
M src/pkg/runtime/sys_darwin_amd64.s View 1 2 1 chunk +3 lines, -1 line 0 comments Download
M src/pkg/runtime/sys_freebsd_386.s View 1 2 1 chunk +4 lines, -1 line 0 comments Download
M src/pkg/runtime/sys_freebsd_amd64.s View 1 2 1 chunk +3 lines, -1 line 0 comments Download
M src/pkg/runtime/sys_linux_386.s View 1 2 1 chunk +4 lines, -1 line 0 comments Download
M src/pkg/runtime/sys_linux_amd64.s View 1 2 3 1 chunk +3 lines, -1 line 0 comments Download
M src/pkg/runtime/sys_linux_arm.s View 1 2 3 4 5 1 chunk +2 lines, -1 line 0 comments Download
M src/pkg/runtime/sys_netbsd_386.s View 1 2 1 chunk +4 lines, -1 line 0 comments Download
M src/pkg/runtime/sys_netbsd_amd64.s View 1 2 1 chunk +3 lines, -1 line 0 comments Download
M src/pkg/runtime/sys_openbsd_386.s View 1 2 1 chunk +4 lines, -1 line 0 comments Download
M src/pkg/runtime/sys_openbsd_amd64.s View 1 2 1 chunk +3 lines, -1 line 0 comments Download
M src/pkg/runtime/thread_darwin.c View 1 2 3 4 1 chunk +5 lines, -1 line 0 comments Download
M src/pkg/runtime/thread_freebsd.c View 1 2 3 4 1 chunk +5 lines, -1 line 0 comments Download
M src/pkg/runtime/thread_linux.c View 1 2 3 4 1 chunk +5 lines, -1 line 0 comments Download
M src/pkg/runtime/thread_netbsd.c View 1 2 3 4 1 chunk +5 lines, -1 line 0 comments Download
M src/pkg/runtime/thread_openbsd.c View 1 2 3 4 1 chunk +5 lines, -1 line 0 comments Download
M src/pkg/runtime/thread_plan9.c View 1 2 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 13
adonovan
Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go/
9 years, 3 months ago (2012-08-29 21:22:47 UTC) #1
iant
This CL only fixes one instance. Suppose we change all the instances to pass the ...
9 years, 3 months ago (2012-08-29 21:55:48 UTC) #2
adonovan
On 29 August 2012 17:55, <iant@golang.org> wrote: > This CL only fixes one instance. Suppose ...
9 years, 3 months ago (2012-08-29 22:11:03 UTC) #3
iant
On 2012/08/29 22:11:03, adonovan wrote: > On 29 August 2012 17:55, <mailto:iant@golang.org> wrote: > > ...
9 years, 3 months ago (2012-08-29 23:38:56 UTC) #4
adonovan
Hello golang-dev@googlegroups.com, iant@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
9 years, 3 months ago (2012-08-30 16:52:26 UTC) #5
adonovan
On 29 August 2012 19:38, <iant@golang.org> wrote: >> Ok. Should badsignal return a bool to ...
9 years, 3 months ago (2012-08-30 16:52:49 UTC) #6
adonovan
On 30 August 2012 12:52, Alan Donovan <adonovan@google.com> wrote: > What I meant to ask ...
9 years, 3 months ago (2012-08-30 19:19:10 UTC) #7
rsc
Seems good. Please keep using the same runtime.write that was there, and then call runtime.exit(1). ...
9 years, 3 months ago (2012-08-31 17:42:18 UTC) #8
adonovan
Hello iant@golang.org, rsc@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
9 years, 3 months ago (2012-08-31 18:10:37 UTC) #9
rsc
LGTM I added you to the list of committers. You should be able to submit ...
9 years, 3 months ago (2012-08-31 19:17:36 UTC) #10
minux1
linux/arm's behavior is different. it will always try to load saved g and m from ...
9 years, 2 months ago (2012-09-01 07:41:07 UTC) #11
adonovan
On 2012/09/01 07:41:07, minux wrote: > linux/arm's behavior is different. > > it will always ...
9 years, 2 months ago (2012-09-04 16:39:38 UTC) #12
adonovan
9 years, 2 months ago (2012-09-04 18:41:04 UTC) #13
*** Submitted as http://code.google.com/p/go/source/detail?r=ef1158a73717 ***

runtime: discard SIGPROF delivered to non-Go threads.

Signal handlers are global resources but many language
environments (Go, C++ at Google, etc) assume they have sole
ownership of a particular handler.  Signal handlers in
mixed-language applications must therefore be robust against
unexpected delivery of certain signals, such as SIGPROF.

The default Go signal handler runtime·sigtramp assumes that it
will never be called on a non-Go thread, but this assumption
is violated by when linking in C++ code that spawns threads.
Specifically, the handler asserts the thread has an associated
"m" (Go scheduler).

This CL is a very simple workaround: discard SIGPROF delivered to non-Go
threads.  runtime.badsignal(int32) now receives the signal number; if it returns
without panicking (e.g. sig==SIGPROF) the signal is discarded.

I don't think there is any really satisfactory solution to the
problem of signal-based profiling in a mixed-language
application.  It's not only the issue of handler clobbering,
but also that a C++ SIGPROF handler called in a Go thread
can't unwind the Go stack (and vice versa).  The best we can
hope for is not crashing.

Note:
- I've ported this to all POSIX platforms, except ARM-linux which already
ignores unexpected signals on m-less threads.
- I've avoided tail-calling runtime.badsignal because AFAICT the 6a/6l don't
support it.
- I've avoided hoisting 'push sig' (common to both function calls) because it
makes the code harder to read.
- Fixed an (apparently incorrect?) docstring.

R=iant, rsc, minux.ma
CC=golang-dev
http://codereview.appspot.com/6498057

http://codereview.appspot.com/6498057/diff/7001/src/pkg/runtime/thread_openbsd.c
File src/pkg/runtime/thread_openbsd.c (right):

http://codereview.appspot.com/6498057/diff/7001/src/pkg/runtime/thread_openbs...
src/pkg/runtime/thread_openbsd.c:240: /* Ignore SIGPROFs intended for a non-Go
thread. */
On 2012/08/31 17:42:18, rsc wrote:
> return
> }
> 
> No else.
> 
> http://golang.org/doc/effective_go.html#if

Done, everywhere.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b