code review 10031043: [release-branch.go1.1] runtime: zeroize g->fnstart to n...

[release-branch.go1.1] runtime: zeroize g->fnstart to not prevent GC of the closure

««« CL 9557043 / 2c128d417029
runtime: zeroize g->fnstart to not prevent GC of the closure
Fixes issue 5493.

R=golang-dev, minux.ma, iant
CC=golang-dev
https://codereview.appspot.com/9557043
»»»

[adg, 2013-06-05 00:44:18]

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

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

[bradfitz, 2013-06-05 00:49:30]

LGTM



On Tue, Jun 4, 2013 at 5:44 PM, <adg@golang.org> wrote:

> Reviewers: dvyukov, iant, minux,
>
> Message:
> Hello dvyukov, iant, minux (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> [release-branch.go1.1] runtime: zeroize g->fnstart to not prevent GC of
> the closure
>
> ««« CL 9557043 / 2c128d417029
> runtime: zeroize g->fnstart to not prevent GC of the closure
> Fixes issue 5493.
>
> R=golang-dev, minux.ma, iant
> CC=golang-dev
>
https://codereview.appspot.**com/9557043<https://codereview.appspot.com/9557043>
> »»»
>
>
> Please review this at
https://codereview.appspot.**com/10031043/<https://codereview.appspot.com/100...
>
> Affected files:
>   M src/pkg/runtime/proc.c
>   A test/fixedbugs/issue5493.go
>
>
> Index: src/pkg/runtime/proc.c
> ==============================**==============================**=======
> --- a/src/pkg/runtime/proc.c
> +++ b/src/pkg/runtime/proc.c
> @@ -1232,6 +1232,7 @@
>  goexit0(G *gp)
>  {
>         gp->status = Gdead;
> +       gp->fnstart = nil;
>         gp->m = nil;
>         gp->lockedm = nil;
>         m->curg = nil;
> Index: test/fixedbugs/issue5493.go
> ==============================**==============================**=======
> new file mode 100644
> --- /dev/null
> +++ b/test/fixedbugs/issue5493.go
> @@ -0,0 +1,52 @@
> +// run
> +
> +// Copyright 2013 The Go Authors. All rights reserved.
> +// Use of this source code is governed by a BSD-style
> +// license that can be found in the LICENSE file.
> +
> +package main
> +
> +import (
> +       "runtime"
> +       "sync"
> +       "sync/atomic"
> +       "time"
> +)
> +
> +const N = 10
> +var count int64
> +
> +func run() error {
> +       f1 := func() {}
> +       f2 := func() {
> +               func() {
> +                       f1()
> +               }()
> +       }
> +       runtime.SetFinalizer(&f1, func(f *func()) {
> +               atomic.AddInt64(&count, -1)
> +       })
> +       go f2()
> +       return nil
> +}
> +
> +func main() {
> +       count = N
> +       var wg sync.WaitGroup
> +       wg.Add(N)
> +       for i := 0; i < N; i++ {
> +               go func() {
> +                       run()
> +                       wg.Done()
> +               }()
> +       }
> +       wg.Wait()
> +       for i := 0; i < 2*N; i++ {
> +               time.Sleep(10 * time.Millisecond)
> +               runtime.GC()
> +       }
> +       if count != 0 {
> +               panic("not all finalizers are called")
> +       }
> +}
> +
>
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>

[dave_cheney.net, 2013-06-05 00:51:40]

LGTM.

On Wed, Jun 5, 2013 at 10:49 AM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> LGTM
>
>
>
> On Tue, Jun 4, 2013 at 5:44 PM, <adg@golang.org> wrote:
>>
>> Reviewers: dvyukov, iant, minux,
>>
>> Message:
>> Hello dvyukov, iant, minux (cc: golang-dev@googlegroups.com),
>>
>> I'd like you to review this change to
>> https://code.google.com/p/go
>>
>>
>> Description:
>> [release-branch.go1.1] runtime: zeroize g->fnstart to not prevent GC of
>> the closure
>>
>> ««« CL 9557043 / 2c128d417029
>> runtime: zeroize g->fnstart to not prevent GC of the closure
>> Fixes issue 5493.
>>
>> R=golang-dev, minux.ma, iant
>> CC=golang-dev
>> https://codereview.appspot.com/9557043
>> »»»
>>
>>
>> Please review this at https://codereview.appspot.com/10031043/
>>
>> Affected files:
>>   M src/pkg/runtime/proc.c
>>   A test/fixedbugs/issue5493.go
>>
>>
>> Index: src/pkg/runtime/proc.c
>> ===================================================================
>> --- a/src/pkg/runtime/proc.c
>> +++ b/src/pkg/runtime/proc.c
>> @@ -1232,6 +1232,7 @@
>>  goexit0(G *gp)
>>  {
>>         gp->status = Gdead;
>> +       gp->fnstart = nil;
>>         gp->m = nil;
>>         gp->lockedm = nil;
>>         m->curg = nil;
>> Index: test/fixedbugs/issue5493.go
>> ===================================================================
>> new file mode 100644
>> --- /dev/null
>> +++ b/test/fixedbugs/issue5493.go
>> @@ -0,0 +1,52 @@
>> +// run
>> +
>> +// Copyright 2013 The Go Authors. All rights reserved.
>> +// Use of this source code is governed by a BSD-style
>> +// license that can be found in the LICENSE file.
>> +
>> +package main
>> +
>> +import (
>> +       "runtime"
>> +       "sync"
>> +       "sync/atomic"
>> +       "time"
>> +)
>> +
>> +const N = 10
>> +var count int64
>> +
>> +func run() error {
>> +       f1 := func() {}
>> +       f2 := func() {
>> +               func() {
>> +                       f1()
>> +               }()
>> +       }
>> +       runtime.SetFinalizer(&f1, func(f *func()) {
>> +               atomic.AddInt64(&count, -1)
>> +       })
>> +       go f2()
>> +       return nil
>> +}
>> +
>> +func main() {
>> +       count = N
>> +       var wg sync.WaitGroup
>> +       wg.Add(N)
>> +       for i := 0; i < N; i++ {
>> +               go func() {
>> +                       run()
>> +                       wg.Done()
>> +               }()
>> +       }
>> +       wg.Wait()
>> +       for i := 0; i < 2*N; i++ {
>> +               time.Sleep(10 * time.Millisecond)
>> +               runtime.GC()
>> +       }
>> +       if count != 0 {
>> +               panic("not all finalizers are called")
>> +       }
>> +}
>> +
>>
>>
>> --
>>
>> ---You received this message because you are subscribed to the Google
>> Groups "golang-dev" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email to golang-dev+unsubscribe@googlegroups.com.
>> For more options, visit https://groups.google.com/groups/opt_out.
>>
>>
>
> --
>
> ---
> You received this message because you are subscribed to the Google Groups
> "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>

[adg, 2013-06-05 00:52:08]

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

[release-branch.go1.1] runtime: zeroize g->fnstart to not prevent GC of the
closure

««« CL 9557043 / 2c128d417029
runtime: zeroize g->fnstart to not prevent GC of the closure
Fixes issue 5493.

R=golang-dev, minux.ma, iant
CC=golang-dev
https://codereview.appspot.com/9557043
»»»

R=dvyukov, iant, minux.ma, bradfitz, dave
CC=golang-dev
https://codereview.appspot.com/10031043

[rsc, 2013-06-10 11:49:17]

LGTM