Code review - Issue 45180043: code review 45180043: runtime/cgo: always set signal mask before calling pthr...https://codereview.appspot.com/2013-12-24T16:08:23+00:00rietveld
Message from unknown
2013-12-24T04:32:06+00:00ianturn:md5:0b721dede3b5011223ea3204e219dee6
Message from iant@golang.org
2013-12-24T04:32:13+00:00ianturn:md5:5274246814c6341ed1eb167ad0c08469
Hello golang-codereviews@googlegroups.com,
I'd like you to review this change to
https://code.google.com/p/go
Message from dave@cheney.net
2013-12-24T04:38:56+00:00dfcurn:md5:aebcead60c44296afc2197a341079187
LGTM. This might explain why the Linux/arm builder has been flakey recently.
> On 24 Dec 2013, at 15:32, iant@golang.org wrote:
>
> Reviewers: golang-codereviews,
>
> Message:
> Hello golang-codereviews@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> runtime/cgo: always set signal mask before calling pthread_create
>
> This was done correctly for most targets but was missing from
> FreeBSD/ARM and Linux/ARM.
>
> Please review this at https://codereview.appspot.com/45180043/
>
> Affected files (+14, -0 lines):
> M src/pkg/runtime/cgo/gcc_freebsd_arm.c
> M src/pkg/runtime/cgo/gcc_linux_arm.c
>
>
> Index: src/pkg/runtime/cgo/gcc_freebsd_arm.c
> ===================================================================
> --- a/src/pkg/runtime/cgo/gcc_freebsd_arm.c
> +++ b/src/pkg/runtime/cgo/gcc_freebsd_arm.c
> @@ -39,10 +39,14 @@
> _cgo_sys_thread_start(ThreadStart *ts)
> {
> pthread_attr_t attr;
> + sigset_t ign, oset;
> pthread_t p;
> size_t size;
> int err;
>
> + SIGFILLSET(ign);
> + pthread_sigmask(SIG_SETMASK, &ign, &oset);
> +
> // Not sure why the memset is necessary here,
> // but without it, we get a bogus stack size
> // out of pthread_attr_getstacksize. C'est la Linux.
> @@ -52,6 +56,9 @@
> pthread_attr_getstacksize(&attr, &size);
> ts->g->stackguard = size;
> err = pthread_create(&p, &attr, threadentry, ts);
> +
> + pthread_sigmask(SIG_SETMASK, &oset, nil);
> +
> if (err != 0) {
> fprintf(stderr, "runtime/cgo: pthread_create failed: %s\n", strerror(err));
> abort();
> Index: src/pkg/runtime/cgo/gcc_linux_arm.c
> ===================================================================
> --- a/src/pkg/runtime/cgo/gcc_linux_arm.c
> +++ b/src/pkg/runtime/cgo/gcc_linux_arm.c
> @@ -28,10 +28,14 @@
> _cgo_sys_thread_start(ThreadStart *ts)
> {
> pthread_attr_t attr;
> + sigset_t ign, oset;
> pthread_t p;
> size_t size;
> int err;
>
> + sigfillset(&ign);
> + pthread_sigmask(SIG_SETMASK, &ign, &oset);
> +
> // Not sure why the memset is necessary here,
> // but without it, we get a bogus stack size
> // out of pthread_attr_getstacksize. C'est la Linux.
> @@ -41,6 +45,9 @@
> pthread_attr_getstacksize(&attr, &size);
> ts->g->stackguard = size;
> err = pthread_create(&p, &attr, threadentry, ts);
> +
> + pthread_sigmask(SIG_SETMASK, &oset, nil);
> +
> if (err != 0) {
> fprintf(stderr, "runtime/cgo: pthread_create failed: %s\n", strerror(err));
> abort();
>
>
> --
> You received this message because you are subscribed to the Google Groups "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
Message from dave@cheney.net
2013-12-24T06:08:10+00:00dfcurn:md5:cd87d4b3e90b027122fac4938db8a901
https://codereview.appspot.com/45180043/diff/1/src/pkg/runtime/cgo/gcc_freebsd_arm.c
File src/pkg/runtime/cgo/gcc_freebsd_arm.c (right):
https://codereview.appspot.com/45180043/diff/1/src/pkg/runtime/cgo/gcc_freebsd_arm.c#newcode60
src/pkg/runtime/cgo/gcc_freebsd_arm.c:60: pthread_sigmask(SIG_SETMASK, &oset, nil);
should this go below the check for err != 0 ?
https://codereview.appspot.com/45180043/diff/1/src/pkg/runtime/cgo/gcc_linux_arm.c
File src/pkg/runtime/cgo/gcc_linux_arm.c (right):
https://codereview.appspot.com/45180043/diff/1/src/pkg/runtime/cgo/gcc_linux_arm.c#newcode49
src/pkg/runtime/cgo/gcc_linux_arm.c:49: pthread_sigmask(SIG_SETMASK, &oset, nil);
same
Message from iant@golang.org
2013-12-24T16:07:56+00:00ianturn:md5:7636c22f2b446953f53231fc4d6c2f77
https://codereview.appspot.com/45180043/diff/1/src/pkg/runtime/cgo/gcc_freebsd_arm.c
File src/pkg/runtime/cgo/gcc_freebsd_arm.c (right):
https://codereview.appspot.com/45180043/diff/1/src/pkg/runtime/cgo/gcc_freebsd_arm.c#newcode60
src/pkg/runtime/cgo/gcc_freebsd_arm.c:60: pthread_sigmask(SIG_SETMASK, &oset, nil);
On 2013/12/24 06:08:10, dfc wrote:
> should this go below the check for err != 0 ?
I don't see a strong reason one way or another, and in any case this makes this code look like all the other instances. We should change all or none.
Message from unknown
2013-12-24T16:08:09+00:00ianturn:md5:165396a3e87f49c8ea822ba3dc55a791
Message from iant@golang.org
2013-12-24T16:08:23+00:00ianturn:md5:3165782d650f13096ac931a7c5f3b055
*** Submitted as https://code.google.com/p/go/source/detail?r=15deca59be01 ***
runtime/cgo: always set signal mask before calling pthread_create
This was done correctly for most targets but was missing from
FreeBSD/ARM and Linux/ARM.
R=golang-codereviews, dave
CC=golang-codereviews
https://codereview.appspot.com/45180043