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#newcode37 src/pkg/sync/rwmutex.go:37: if raceenabled { It seems for such simple instrumentation ...
11 years, 7 months ago
(2012-09-19 22:05:04 UTC)
#3
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.
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#newcode9 src/pkg/sync/rwmutex.go:9: "unsafe" Currently this breaks build because of the unexpected ...
11 years, 7 months ago
(2012-09-19 22:11:59 UTC)
#4
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 ...
11 years, 7 months ago
(2012-09-24 15:13:40 UTC)
#6
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.
On Monday, September 24, 2012, rsc wrote: > > I don't understand what dependency we ...
11 years, 7 months ago
(2012-09-24 15:37:38 UTC)
#7
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.
Yes, it's go/build/deps_test. I will update the patch to allow the dep. On Mon, Sep ...
11 years, 7 months ago
(2012-09-24 20:30:25 UTC)
#9
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.
>
On 2012/09/24 20:30:25, dvyukov wrote: > Yes, it's go/build/deps_test. > I will update the patch ...
11 years, 7 months ago
(2012-09-25 03:47:55 UTC)
#10
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.
> >
Issue 6529053: code review 6529053: race: sync changes
(Closed)
Created 11 years, 7 months ago by dvyukov
Modified 11 years, 6 months ago
Reviewers:
Base URL:
Comments: 3