code review 5684060: runtime: fix FreeBSD signal handling around thread creation

runtime: fix FreeBSD signal handling around thread creation
Ignore signals while we are spawning a new thread. Previously, a
signal arriving just before runtime.minit setting up the signal
handler triggers a "double fault" in signal trampolining.
Fixes issue 3017.

[dho, 2012-02-22 02:26:23]

Hello rsc@golang.org, mikioh.mikioh@gmail.com, minux <minux.ma@gmail.com>,
adg@golang.org (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[dho, 2012-02-22 02:28:50]

On 2012/02/22 02:26:23, dho wrote:
> Hello mailto:rsc@golang.org, mailto:mikioh.mikioh@gmail.com, minux
<mailto:minux.ma@gmail.com>,
> mailto:adg@golang.org (cc: mailto:golang-dev@googlegroups.com),
> 
> I'd like you to review this change to
> https://go.googlecode.com/hg/

I need someone to please test this on FreeBSD/386. I can't do this with my
hardware, and I'm not at all sure my sigprocmask implementation is correct on
386 since I haven't done the syscall trampolining on 386 since the initial port.
Seems to fix the issue for me on amd64. Takes care around native go threads as
well as cgo threads.

[adg, 2012-02-22 02:30:09]

On it.

On 22 February 2012 13:28,  <devon.odell@gmail.com> wrote:
> On 2012/02/22 02:26:23, dho wrote:
>>
>> Hello mailto:rsc@golang.org, mailto:mikioh.mikioh@gmail.com, minux
>
> <mailto:minux.ma@gmail.com>,
>>
>> mailto:adg@golang.org (cc: mailto:golang-dev@googlegroups.com),
>
>
>> I'd like you to review this change to
>> https://go.googlecode.com/hg/
>
>
> I need someone to please test this on FreeBSD/386. I can't do this with
> my hardware, and I'm not at all sure my sigprocmask implementation is
> correct on 386 since I haven't done the syscall trampolining on 386
> since the initial port. Seems to fix the issue for me on amd64. Takes
> care around native go threads as well as cgo threads.
>
> http://codereview.appspot.com/5684060/

[adg, 2012-02-22 02:37:27]

Fun. freebsd-386:

$ ./all.bash
# Building C bootstrap tool.
cmd/dist

# Building compilers and Go bootstrap tool for host, freebsd/386.
lib9
libbio
libmach
misc/pprof
cmd/cov
cmd/nm
cmd/pack
cmd/prof
cmd/cc
cmd/gc
cmd/8l
cmd/8a
cmd/8c
cmd/8g
pkg/runtime
pkg/errors
pkg/sync/atomic
pkg/sync
pkg/io
pkg/unicode
pkg/unicode/utf8
pkg/unicode/utf16
pkg/bytes
pkg/math
pkg/strings
pkg/strconv
pkg/bufio
pkg/sort
pkg/container/heap
pkg/encoding/base64
pkg/syscall
pkg/time
pkg/os
pkg/reflect
pkg/fmt
pkg/encoding/json
pkg/flag
pkg/path/filepath
pkg/path
pkg/io/ioutil
pkg/log
pkg/regexp/syntax
pkg/regexp
pkg/go/token
pkg/go/scanner
pkg/go/ast
pkg/go/parser
pkg/go/build
pkg/os/exec
pkg/net/url
pkg/text/template/parse
pkg/text/template
pkg/go/doc
cmd/go
panic: runtime error: invalid memory address or nil pointer dereference
[signal 0xb code=0x1 addr=0xf1 pc=0x807d37e]

goroutine 1 [runnable]:

goroutine 2 [runnable]:
created by _rt0_386
	/home/adg/go/src/pkg/runtime/asm_386.s:80 +0xbe

[dho, 2012-02-22 02:51:00]

Op 21 februari 2012 21:36 heeft Andrew Gerrand <adg@golang.org> het
volgende geschreven:
> Fun. freebsd-386:

Taking a look, thanks

[dho, 2012-02-22 04:09:23]

Hello rsc@golang.org, mikioh.mikioh@gmail.com, minux <minux.ma@gmail.com>,
adg@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[dho, 2012-02-22 04:09:42]

This seems to do the trick. PTAL!

[adg, 2012-02-22 04:15:45]

LGTM
Thanks.

Although for the assembly I'll have to take your word for it. (Russ?)

[rsc, 2012-02-22 04:22:29]

http://codereview.appspot.com/5684060/diff/6/src/pkg/runtime/sys_freebsd_amd64.s
File src/pkg/runtime/sys_freebsd_amd64.s (right):

http://codereview.appspot.com/5684060/diff/6/src/pkg/runtime/sys_freebsd_amd6...
src/pkg/runtime/sys_freebsd_amd64.s:239: LEAQ	8(SP), SI		// arg 2 - set
MOVQ
next line too

http://codereview.appspot.com/5684060/diff/6/src/pkg/runtime/thread_freebsd.c
File src/pkg/runtime/thread_freebsd.c (right):

http://codereview.appspot.com/5684060/diff/6/src/pkg/runtime/thread_freebsd.c...
src/pkg/runtime/thread_freebsd.c:16: Sigset sigset_all = { ~0, ~0, ~0, ~0, };
Use ~(uint32)0 or whatever the type is.

static Sigset please

http://codereview.appspot.com/5684060/diff/6/src/pkg/runtime/thread_freebsd.c...
src/pkg/runtime/thread_freebsd.c:131: runtime·sigprocmask(&sigset_none, &oset);
You should be able to pass nil instead of &oset.

[rsc, 2012-02-22 04:22:57]

fixing reviewer list

[dho, 2012-02-22 04:34:46]

Hello rsc@golang.org, mikioh.mikioh@gmail.com, minux.ma@gmail.com,
adg@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[rsc, 2012-02-22 04:38:07]

LGTM

I also double-checked that FreeBSD sigprocmask is per-thread, not per-process.

[dho, 2012-02-22 04:41:59]

On 2012/02/22 04:22:29, rsc wrote:
>
http://codereview.appspot.com/5684060/diff/6/src/pkg/runtime/sys_freebsd_amd64.s
> File src/pkg/runtime/sys_freebsd_amd64.s (right):
> 
>
http://codereview.appspot.com/5684060/diff/6/src/pkg/runtime/sys_freebsd_amd6...
> src/pkg/runtime/sys_freebsd_amd64.s:239: LEAQ	8(SP), SI		// arg 2 - set
> MOVQ
> next line too

Ha, tyvm. Done

> http://codereview.appspot.com/5684060/diff/6/src/pkg/runtime/thread_freebsd.c
> File src/pkg/runtime/thread_freebsd.c (right):
> 
>
http://codereview.appspot.com/5684060/diff/6/src/pkg/runtime/thread_freebsd.c...
> src/pkg/runtime/thread_freebsd.c:16: Sigset sigset_all = { ~0, ~0, ~0, ~0, };
> Use ~(uint32)0 or whatever the type is.
> 
> static Sigset please

Done and done.

>
http://codereview.appspot.com/5684060/diff/6/src/pkg/runtime/thread_freebsd.c...
> src/pkg/runtime/thread_freebsd.c:131: runtime·sigprocmask(&sigset_none,
&oset);
> You should be able to pass nil instead of &oset.

Done. Was trying to be clever in an earlier incantation. Oops!

PTAL

[adg, 2012-02-22 04:44:16]

*** Submitted as http://code.google.com/p/go/source/detail?r=d1c35c41fb2a ***

runtime: fix FreeBSD signal handling around thread creation
Ignore signals while we are spawning a new thread. Previously, a
signal arriving just before runtime.minit setting up the signal
handler triggers a "double fault" in signal trampolining.
Fixes issue 3017.

R=rsc, mikioh.mikioh, minux.ma, adg
CC=golang-dev
http://codereview.appspot.com/5684060

Committer: Andrew Gerrand <adg@golang.org>