code review 137070043: runtime: adapt race detector for runtime written in Goadapt

runtime: adopt race detector for runtime written in Go
Ignore memory access on g0/gsignal.
See the issue for context and explanation.
Fixes issue 8627.

[dvyukov, 2014-09-02 07:42:57]

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

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

[mdempsky, 2014-09-02 16:23:42]

Seems to fix the crashes I was running into.

https://codereview.appspot.com/137070043/diff/40001/src/pkg/runtime/cpuprof.go
File src/pkg/runtime/cpuprof.go (right):

https://codereview.appspot.com/137070043/diff/40001/src/pkg/runtime/cpuprof.g...
src/pkg/runtime/cpuprof.go:267: for i := uintptr(0); i < d; i++ {
This loop can be replaced with

    copy(log[q:], e.stack[:d])
    q += d

too, fwiw.

https://codereview.appspot.com/137070043/diff/40001/src/pkg/runtime/race.c
File src/pkg/runtime/race.c (right):

https://codereview.appspot.com/137070043/diff/40001/src/pkg/runtime/race.c#ne...
src/pkg/runtime/race.c:156: if(g != g->m->curg) {
Would it be easier to test for this just once in race_amd64.s:racecall<>?

[khr, 2014-09-02 19:49:23]

https://codereview.appspot.com/137070043/diff/40001/src/pkg/runtime/race.c
File src/pkg/runtime/race.c (right):

https://codereview.appspot.com/137070043/diff/40001/src/pkg/runtime/race.c#ne...
src/pkg/runtime/race.c:175: }
Don't you also need to do racereadpc/racewritepc in the assembly files?  Or just
do racecalladdr (mdempsky's suggestion)?

[dvyukov, 2014-09-03 07:28:21]

Regarding the other comments.
Racecall is the wrong place for the check, as main goroutine is created on g0.
Racecalladdr is the wrong place as the additional check with several
indirections will slow down programs by 10%. Racereadpc is not required right
now otherwise race tests would crash.
I agree that we need something more systematic in future. But the change will
complex and subtle and can potentially severely degrade performance. I don't
want to rewrite it just before release.

https://codereview.appspot.com/137070043/diff/40001/src/pkg/runtime/cpuprof.go
File src/pkg/runtime/cpuprof.go (right):

https://codereview.appspot.com/137070043/diff/40001/src/pkg/runtime/cpuprof.g...
src/pkg/runtime/cpuprof.go:267: for i := uintptr(0); i < d; i++ {
On 2014/09/02 16:23:41, mdempsky wrote:
> This loop can be replaced with
> 
>     copy(log[q:], e.stack[:d])
>     q += d
> 
> too, fwiw.

Done.

[khr, 2014-09-03 16:33:06]

On 2014/09/03 07:28:21, dvyukov wrote:
> Regarding the other comments.
> Racecall is the wrong place for the check, as main goroutine is created on g0.
> Racecalladdr is the wrong place as the additional check with several
> indirections will slow down programs by 10%. Racereadpc is not required right
> now otherwise race tests would crash.
> I agree that we need something more systematic in future. But the change will
> complex and subtle and can potentially severely degrade performance. I don't
> want to rewrite it just before release.
> 
> https://codereview.appspot.com/137070043/diff/40001/src/pkg/runtime/cpuprof.go
> File src/pkg/runtime/cpuprof.go (right):
> 
>
https://codereview.appspot.com/137070043/diff/40001/src/pkg/runtime/cpuprof.g...
> src/pkg/runtime/cpuprof.go:267: for i := uintptr(0); i < d; i++ {
> On 2014/09/02 16:23:41, mdempsky wrote:
> > This loop can be replaced with
> > 
> >     copy(log[q:], e.stack[:d])
> >     q += d
> > 
> > too, fwiw.
> 
> Done.

LGTM.

[rsc, 2014-09-03 16:47:21]

In CL description, s/adopt/adapt/

[dvyukov, 2014-09-03 16:47:36]

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

runtime: adopt race detector for runtime written in Go
Ignore memory access on g0/gsignal.
See the issue for context and explanation.
Fixes issue 8627.

LGTM=khr
R=golang-codereviews, mdempsky, khr
CC=golang-codereviews, rsc
https://codereview.appspot.com/137070043

[dvyukov, 2014-09-03 16:49:37]

sorry, race condition

On Wed, Sep 3, 2014 at 8:47 PM, Russ Cox <rsc@golang.org> wrote:
> In CL description, s/adopt/adapt/
>

[gobot, 2014-09-03 16:56:29]

This CL appears to have broken the linux-amd64-race-temp builder.
See http://build.golang.org/log/b3ca1808a3d321720b7783bccd3fb7c4dfa0ae46