code review 6498057: runtime: discard SIGPROF delivered to non-Go threads.

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.

[adonovan, 2012-08-29 21:22:47]

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/

[iant, 2012-08-29 21:55:48]

This CL only fixes one instance.  Suppose we change all the instances to pass
the signal number to badsignal.  Then we have a consistent way to ignore the
SIGPROF.

I don't understand how the C code would ever use the same TLS slot as the Go
code, unless somebody is doing something really wacky.  That's not how TLS
works.

Another thing we could consider is having cgocall mask signals.  Then the C code
would not receive any signals by default.  Of course it might re-enable signals,
or even change the signal handler, but there is no much we can do about that.

[adonovan, 2012-08-29 22:11:03]

On 29 August 2012 17:55,  <iant@golang.org> wrote:
> This CL only fixes one instance.  Suppose we change all the instances to
> pass the signal number to badsignal.  Then we have a consistent way to
> ignore the SIGPROF.

Ok.  Should badsignal return a bool to indicate whether to RET
(discard signal) or continue (and encounter a SIGSEGV)?  Better
perhaps for it to abort within the C routine; what's the recommended
async-signal-safe way to do that?


> I don't understand how the C code would ever use the same TLS slot as
> the Go code, unless somebody is doing something really wacky.  That's
> not how TLS works.

I was hoping that was the case.  Is that because Go's TLS (on
linux-amd64) based on %GS as opposed to sysvabi's %FS?


> Another thing we could consider is having cgocall mask signals.  Then
> the C code would not receive any signals by default.  Of course it might
> re-enable signals, or even change the signal handler, but there is no
> much we can do about that.

Google C++ enables and disables SIGPROF (to name but one) from many
places, so I'm not sure much would be gained from this.

[iant, 2012-08-29 23:38:56]

On 2012/08/29 22:11:03, adonovan wrote:
> On 29 August 2012 17:55,  <mailto:iant@golang.org> wrote:
> > This CL only fixes one instance.  Suppose we change all the instances to
> > pass the signal number to badsignal.  Then we have a consistent way to
> > ignore the SIGPROF.
> 
> Ok.  Should badsignal return a bool to indicate whether to RET
> (discard signal) or continue (and encounter a SIGSEGV)?  Better
> perhaps for it to abort within the C routine; what's the recommended
> async-signal-safe way to do that?

The abort function works by raising SIGABRT.  So the async-signal safe way to
abort is to call sigaction to restore the default SIGABRT handler and then call
kill(getpid(), SIGABRT).

That said I don't know how hard we need to work here.


> > I don't understand how the C code would ever use the same TLS slot as
> > the Go code, unless somebody is doing something really wacky.  That's
> > not how TLS works.
> 
> I was hoping that was the case.  Is that because Go's TLS (on
> linux-amd64) based on %GS as opposed to sysvabi's %FS?

Actually Go uses the same TLS segment registers as ordinary ELF programs: %fs on
amd64, %gs on 386.  When using cgo to produce a dynamically linked executable,
the Go linker generates a PT_TLS segment that reserves the TLS entries that it
uses.

[adonovan, 2012-08-30 16:52:26]

Hello golang-dev@googlegroups.com, iant@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[adonovan, 2012-08-30 16:52:49]

On 29 August 2012 19:38,  <iant@golang.org> wrote:
>> Ok.  Should badsignal return a bool to indicate whether to RET
>> (discard signal) or continue (and encounter a SIGSEGV)?  Better
>> perhaps for it to abort within the C routine; what's the recommended
>> async-signal-safe way to do that?
>
> The abort function works by raising SIGABRT.  So the async-signal safe
> way to abort is to call sigaction to restore the default SIGABRT handler
> and then call kill(getpid(), SIGABRT).
>
> That said I don't know how hard we need to work here.

What I meant to ask was: what's the local idiom for abort()?  Seems
like the answer is runtime·panicstring(), so I've used that.


> Actually Go uses the same TLS segment registers as ordinary ELF
> programs: %fs on amd64, %gs on 386.  When using cgo to produce a
> dynamically linked executable, the Go linker generates a PT_TLS segment
> that reserves the TLS entries that it uses.

Ok; thanks for the explanation.

[adonovan, 2012-08-30 19:19:10]

On 30 August 2012 12:52, Alan Donovan <adonovan@google.com> wrote:
> What I meant to ask was: what's the local idiom for abort()?  Seems
> like the answer is runtime·panicstring(), so I've used that.

Hmm... actually this just initiates a Go panic, which is not what I want.
I'll use runtime·write followed by abort.

[rsc, 2012-08-31 17:42:18]

Seems good. Please keep using the same runtime.write that was there, and then
call runtime.exit(1).

Normally you'd want runtime.throw for fatal errors like this, but you're running
on a non-Go stack so there is very little that is safe to do.

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

https://codereview.appspot.com/6498057/diff/7001/src/pkg/runtime/thread_openb...
src/pkg/runtime/thread_openbsd.c:240: /* Ignore SIGPROFs intended for a non-Go
thread. */
return
}

No else.

http://golang.org/doc/effective_go.html#if

[adonovan, 2012-08-31 18:10:37]

Hello iant@golang.org, rsc@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2012-08-31 19:17:36]

LGTM

I added you to the list of committers. You should be able to submit
this change yourself after following the directions at
http://swtch.com/~rsc/gohg.html.

Russ

[minux1, 2012-09-01 07:41:07]

linux/arm's behavior is different.

it will always try to load saved g and m from TLS.
code for badsignal is added latter and for some reason,
Linux/ARM doesn't change.

perhaps we need to revise it now (in another CL), for if
a signal received on a thread not created by go
(thus g and m TLS slot would be nil),  the program
will still crash.

http://codereview.appspot.com/6498057/diff/1021/src/pkg/runtime/signal_linux_...
File src/pkg/runtime/signal_linux_amd64.c (right):

http://codereview.appspot.com/6498057/diff/1021/src/pkg/runtime/signal_linux_...
src/pkg/runtime/signal_linux_amd64.c:138: // Linux manpage says "sa_restart
element is obsolete and
s/sa_restart/sa_restorer/

btw, why add this comment? it seems confusing
if we add this comment but don't explain why
we use it.

[adonovan, 2012-09-04 16:39:38]

On 2012/09/01 07:41:07, minux wrote:
> linux/arm's behavior is different.
> 
> it will always try to load saved g and m from TLS.
> code for badsignal is added latter and for some reason,
> Linux/ARM doesn't change.
> 
> perhaps we need to revise it now (in another CL), for if
> a signal received on a thread not created by go
> (thus g and m TLS slot would be nil),  the program
> will still crash.

Quite right; I've added a TODO(adonovan) comment to make this consistent.


>
http://codereview.appspot.com/6498057/diff/1021/src/pkg/runtime/signal_linux_...
> src/pkg/runtime/signal_linux_amd64.c:138: // Linux manpage says "sa_restart
> element is obsolete and
> s/sa_restart/sa_restorer/

Fixed.

> btw, why add this comment? it seems confusing
> if we add this comment but don't explain why
> we use it.

Yeah; it now reads:

  // TODO(adonovan): "Linux manpage says...". Avoid it here, and test.

[adonovan, 2012-09-04 18:41:04]

*** 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.