code review 6529053: race: sync changes

race: sync changes
This is a part of a bigger change that adds data race detection feature:
http://codereview.appspot.com/6456044

[dvyukov, 2012-09-19 22:01:03]

Hello golang-dev@googlegroups.com,

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

[dvyukov, 2012-09-19 22:01:27]

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

Please take another look.

[dvyukov, 2012-09-19 22:05:04]

https://codereview.appspot.com/6529053/diff/3007/src/pkg/sync/rwmutex.go
File src/pkg/sync/rwmutex.go (right):

https://codereview.appspot.com/6529053/diff/3007/src/pkg/sync/rwmutex.go#newc...
src/pkg/sync/rwmutex.go:37: if raceenabled {
It seems for such simple instrumentation raceenabled flag is not actually
required, because when !race the functions are empty, and so inliner eliminates
the calls. However currently it does eliminate them completely, here is what I
see in objdump when if raceenabled is commented out:

        //if raceenabled {
                raceEnable()
                raceAcquire(unsafe.Pointer(&rw.readerSem))
  450e3e:       48 8b 44 24 20          mov    0x20(%rsp),%rax
  450e43:       48 83 c0 0c             add    $0xc,%rax
        //}

Can we make the compiler eliminate this completely? The address computation does
not have any side effects.

[dvyukov, 2012-09-19 22:11:59]

https://codereview.appspot.com/6529053/diff/3007/src/pkg/sync/rwmutex.go
File src/pkg/sync/rwmutex.go (right):

https://codereview.appspot.com/6529053/diff/3007/src/pkg/sync/rwmutex.go#newc...
src/pkg/sync/rwmutex.go:9: "unsafe"
Currently this breaks build because of the unexpected dependency between
packages.
What would be the best solution?
I can make this dependency legal. Or import unsafe only into race.go (make
raceAcquire() accept *uint32), then I want to disable dependency check with
-race anyway.

[gobot, 2012-09-20 06:47:38]

R=rsc (assigned by dsymonds)

[rsc, 2012-09-24 15:13:40]

Code seems fine but I don't understand the part about the import cycle.

http://codereview.appspot.com/6529053/diff/3007/src/pkg/sync/rwmutex.go
File src/pkg/sync/rwmutex.go (right):

http://codereview.appspot.com/6529053/diff/3007/src/pkg/sync/rwmutex.go#newcode9
src/pkg/sync/rwmutex.go:9: "unsafe"
On 2012/09/19 22:11:59, dvyukov wrote:
> Currently this breaks build because of the unexpected dependency between
> packages.
> What would be the best solution?
> I can make this dependency legal. Or import unsafe only into race.go (make
> raceAcquire() accept *uint32), then I want to disable dependency check with
> -race anyway.

I don't understand what dependency we are talking about. Surely unsafe does not
import sync.

[minux1, 2012-09-24 15:37:38]

On Monday, September 24, 2012, rsc wrote:
>
> I don't understand what dependency we are talking about. Surely unsafe
> does not import sync.
>
I think he talks about go/build/deps_test.go
We don't allow sync to depend on unsafe.

[rsc, 2012-09-24 16:49:15]

> I think he talks about go/build/deps_test.go
> We don't allow sync to depend on unsafe.

If that's all it is, allowing sync to use unsafe is fine.

[dvyukov, 2012-09-24 20:30:25]

Yes, it's go/build/deps_test.
I will update the patch to allow the dep.


On Mon, Sep 24, 2012 at 9:49 AM, Russ Cox <rsc@golang.org> wrote:

> > I think he talks about go/build/deps_test.go
> > We don't allow sync to depend on unsafe.
>
> If that's all it is, allowing sync to use unsafe is fine.
>

[dvyukov, 2012-09-25 03:47:55]

On 2012/09/24 20:30:25, dvyukov wrote:
> Yes, it's go/build/deps_test.
> I will update the patch to allow the dep.

Done. PTAL.

> On Mon, Sep 24, 2012 at 9:49 AM, Russ Cox <mailto:rsc@golang.org> wrote:
> 
> > > I think he talks about go/build/deps_test.go
> > > We don't allow sync to depend on unsafe.
> >
> > If that's all it is, allowing sync to use unsafe is fine.
> >

[rsc, 2012-10-01 20:18:33]

LGTM

[dvyukov, 2012-10-07 18:07:17]

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

race: sync changes
This is a part of a bigger change that adds data race detection feature:
http://codereview.appspot.com/6456044

R=rsc, minux.ma
CC=gobot, golang-dev
http://codereview.appspot.com/6529053