code review 6976054: runtime: diagnose double wakeup on Note

runtime: diagnose double wakeup on Note
Double wakeup is prohibited by the Note interface
and checked in lock_sema.c.

[dvyukov, 2012-12-24 16:38:13]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://dvyukov%40google.com@code.google.com/p/go/

[bradfitz, 2012-12-24 16:56:59]

LGTM

On Mon, Dec 24, 2012 at 8:38 AM, <dvyukov@google.com> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
>
https://dvyukov%40google.com@**code.google.com/p/go/<http://40google.com@code...
>
>
> Description:
> runtime: diagnose double wakeup on Note
> Double wakeup is prohibited by the Note interface
> and checked in lock_sema.c.
>
> Please review this at
https://codereview.appspot.**com/6976054/<https://codereview.appspot.com/6976...
>
> Affected files:
>   M src/pkg/runtime/lock_futex.c
>
>
> Index: src/pkg/runtime/lock_futex.c
> ==============================**==============================**=======
> --- a/src/pkg/runtime/lock_futex.c
> +++ b/src/pkg/runtime/lock_futex.c
> @@ -111,7 +111,8 @@
>  void
>  runtime·notewakeup(Note *n)
>  {
> -       runtime·xchg(&n->key, 1);
> +       if(runtime·xchg(&n->key, 1))
> +               runtime·throw("notewakeup - double wakeup");
>         runtime·futexwakeup(&n->key, 1);
>  }
>
>
>
>

[dvyukov, 2012-12-24 17:07:26]

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

runtime: diagnose double wakeup on Note
Double wakeup is prohibited by the Note interface
and checked in lock_sema.c.

R=golang-dev, bradfitz
CC=golang-dev
https://codereview.appspot.com/6976054

[bradfitz, 2013-01-08 20:16:31]

Dmitri,

I just got this crash from this CL at 74e2affcfe39.

It went away after my next run of "go test -short std", though.  I only saw
it once:

fatal error: notewakeup - double wakeup

goroutine 0 [idle]:

goroutine 1 [chan receive]:
testing.RunTests(0x400c00, 0x5d25d0, 0x2, 0x2, 0x1, ...)
/home/bradfitz/go/src/pkg/testing/testing.go:378 +0x891
testing.Main(0x400c00, 0x5d25d0, 0x2, 0x2, 0x5d8c48, ...)
/home/bradfitz/go/src/pkg/testing/testing.go:313 +0x8a
main.main()
os/signal/_test/_testmain.go:45 +0x9a

goroutine 2 [syscall]:
created by runtime.main
/home/bradfitz/go/src/pkg/runtime/proc.c:225

goroutine 3 [syscall]:
os/signal.loop()
/home/bradfitz/go/src/pkg/os/signal/signal_unix.go:20 +0x1c
created by os/signal.init·1
/home/bradfitz/go/src/pkg/os/signal/signal_unix.go:26 +0x2f

goroutine 6 [sleep]:
time.Sleep(0x5f5e100, 0xc20005d140)
/home/bradfitz/go/src/pkg/runtime/ztime_linux_amd64.c:20 +0x2f
os/signal.TestStress()
/home/bradfitz/go/src/pkg/os/signal/signal_test.go:97 +0x141
testing.tRunner(0xc200085090, 0x5d25e8, 0x0, 0x0)
/home/bradfitz/go/src/pkg/testing/testing.go:301 +0x6c
created by testing.RunTests
/home/bradfitz/go/src/pkg/testing/testing.go:377 +0x86e

goroutine 5 [syscall]:
created by addtimer
/home/bradfitz/go/src/pkg/runtime/ztime_linux_amd64.c:73

goroutine 7 [select]:
os/signal.func·001(0xc20005d138, 0xc20005d140, 0x0, 0x0)
/home/bradfitz/go/src/pkg/os/signal/signal_test.go:76 +0x14d
created by os/signal.TestStress
/home/bradfitz/go/src/pkg/os/signal/signal_test.go:83 +0x110

goroutine 8 [running]:
created by os/signal.TestStress
/home/bradfitz/go/src/pkg/os/signal/signal_test.go:96 +0x131
FAIL os/signal 0.094s


On Mon, Dec 24, 2012 at 8:38 AM, <dvyukov@google.com> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
>
https://dvyukov%40google.com@**code.google.com/p/go/<http://40google.com@code...
>
>
> Description:
> runtime: diagnose double wakeup on Note
> Double wakeup is prohibited by the Note interface
> and checked in lock_sema.c.
>
> Please review this at
https://codereview.appspot.**com/6976054/<https://codereview.appspot.com/6976...
>
> Affected files:
>   M src/pkg/runtime/lock_futex.c
>
>
> Index: src/pkg/runtime/lock_futex.c
> ==============================**==============================**=======
> --- a/src/pkg/runtime/lock_futex.c
> +++ b/src/pkg/runtime/lock_futex.c
> @@ -111,7 +111,8 @@
>  void
>  runtime·notewakeup(Note *n)
>  {
> -       runtime·xchg(&n->key, 1);
> +       if(runtime·xchg(&n->key, 1))
> +               runtime·throw("notewakeup - double wakeup");
>         runtime·futexwakeup(&n->key, 1);
>  }
>
>
>
>

[dvyukov, 2013-01-09 07:00:42]

On Wed, Jan 9, 2013 at 12:16 AM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> Dmitri,
>
> I just got this crash from this CL at 74e2affcfe39.
>
> It went away after my next run of "go test -short std", though.  I only saw
> it once:
>
> fatal error: notewakeup - double wakeup


Is it linux?
I guess you don't have local changes in runtime, right?
I've run os/signal test several hundreds times and can't reproduce it...
Can you try to reproduce it with GOTRACEBACK=2 (it's useful to have in bashrc)?

[dvyukov, 2013-01-09 07:22:21]

On Wed, Jan 9, 2013 at 11:00 AM, Dmitry Vyukov <dvyukov@google.com> wrote:
> On Wed, Jan 9, 2013 at 12:16 AM, Brad Fitzpatrick <bradfitz@golang.org>
wrote:
>> Dmitri,
>>
>> I just got this crash from this CL at 74e2affcfe39.
>>
>> It went away after my next run of "go test -short std", though.  I only saw
>> it once:
>>
>> fatal error: notewakeup - double wakeup
>
>
> Is it linux?
> I guess you don't have local changes in runtime, right?
> I've run os/signal test several hundreds times and can't reproduce it...
> Can you try to reproduce it with GOTRACEBACK=2 (it's useful to have in
bashrc)?

What about adding GOTRACEBACK=2 to run.bash?
https://codereview.appspot.com/7069060
If it even happen on bots, it can provide some additional information.

[bradfitz, 2013-01-09 22:58:36]

On Tue, Jan 8, 2013 at 11:00 PM, Dmitry Vyukov <dvyukov@google.com> wrote:

> On Wed, Jan 9, 2013 at 12:16 AM, Brad Fitzpatrick <bradfitz@golang.org>
> wrote:
> > Dmitri,
> >
> > I just got this crash from this CL at 74e2affcfe39.
> >
> > It went away after my next run of "go test -short std", though.  I only
> saw
> > it once:
> >
> > fatal error: notewakeup - double wakeup
>
>
> Is it linux?
>

yes


> I guess you don't have local changes in runtime, right?
>

no runtime changes

[dvyukov, 2013-01-10 05:55:03]

On Thu, Jan 10, 2013 at 2:58 AM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
>> wrote:
>> > Dmitri,
>> >
>> > I just got this crash from this CL at 74e2affcfe39.
>> >
>> > It went away after my next run of "go test -short std", though.  I only
>> > saw
>> > it once:
>> >
>> > fatal error: notewakeup - double wakeup
>>
>>
>> Is it linux?
>
>
> yes
>
>>
>> I guess you don't have local changes in runtime, right?
>
>
> no runtime changes


I will inspect usages of Note, but without knowing what Note it is,
it's unlikely to succeed...