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

Please take another look.


http://codereview.appspot.com/6529053/

Search Discussions

  • Dvyukov at Sep 19, 2012 at 10:07 pm
    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/


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

    Please review this at http://codereview.appspot.com/6529053/

    Affected files:
    M src/pkg/sync/cond.go
    M src/pkg/sync/mutex.go
    A src/pkg/sync/race.go
    A src/pkg/sync/race0.go
    M src/pkg/sync/rwmutex.go
    M src/pkg/sync/waitgroup.go
  • Dvyukov at Sep 19, 2012 at 10:12 pm
    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 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/
  • Dvyukov at Sep 19, 2012 at 10:17 pm
    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 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.

    https://codereview.appspot.com/6529053/
  • Gobot at Sep 20, 2012 at 6:47 am
  • Rsc at Sep 24, 2012 at 3:13 pm
    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.

    http://codereview.appspot.com/6529053/
  • Minux at Sep 24, 2012 at 3:37 pm

    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.
  • Russ Cox at Sep 24, 2012 at 4:56 pm

    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.
  • Dmitry Vyukov at Sep 24, 2012 at 8:53 pm
    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 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 at Sep 25, 2012 at 3:47 am

    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
    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.


    https://codereview.appspot.com/6529053/

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedSep 19, '12 at 10:01p
activeSep 25, '12 at 3:47a
posts10
users4
websitegolang.org

People

Translate

site design / logo © 2022 Grokbase