FAQ
https://codereview.appspot.com/6497074/diff/15001/src/cmd/gc/racewalk.c
File src/cmd/gc/racewalk.c (right):

https://codereview.appspot.com/6497074/diff/15001/src/cmd/gc/racewalk.c#newcode80
src/cmd/gc/racewalk.c:80: default:
On 2012/09/20 18:00:23, rsc wrote:
Unindent switch body 1 tab
Done.

https://codereview.appspot.com/6497074/diff/15001/src/cmd/gc/racewalk.c#newcode80
src/cmd/gc/racewalk.c:80: default:
On 2012/09/20 18:00:23, rsc wrote:
Why do you have a default and all the other cases explicitly
enumerated?
I could see saying default: fatal("racewalk %O", n->op) or something
like that.
But if you have default: goto ret, then all the other case FOO: goto ret are
unnecessary, no?
Done.

https://codereview.appspot.com/6497074/diff/15001/src/cmd/gc/racewalk.c#newcode96
src/cmd/gc/racewalk.c:96: case OCALL:
On 2012/09/20 18:00:23, rsc wrote:
There sure are a lot of these. Might as well merge them all into one case list
instead of having lots of different case lists that all do nothing.
Done.

https://codereview.appspot.com/6497074/

Search Discussions

  • Dvyukov at Sep 21, 2012 at 12:21 am
    On 2012/09/21 00:19:28, dvyukov wrote:

    https://codereview.appspot.com/6497074/diff/15001/src/cmd/gc/racewalk.c
    File src/cmd/gc/racewalk.c (right):

    https://codereview.appspot.com/6497074/diff/15001/src/cmd/gc/racewalk.c#newcode80
    src/cmd/gc/racewalk.c:80: default:
    On 2012/09/20 18:00:23, rsc wrote:
    Unindent switch body 1 tab
    Done.

    https://codereview.appspot.com/6497074/diff/15001/src/cmd/gc/racewalk.c#newcode80
    src/cmd/gc/racewalk.c:80: default:
    On 2012/09/20 18:00:23, rsc wrote:
    Why do you have a default and all the other cases explicitly
    enumerated?
    I could see saying default: fatal("racewalk %O", n->op) or something
    like
    that.
    But if you have default: goto ret, then all the other case FOO: goto
    ret are
    unnecessary, no?
    Done.

    https://codereview.appspot.com/6497074/diff/15001/src/cmd/gc/racewalk.c#newcode96
    src/cmd/gc/racewalk.c:96: case OCALL:
    On 2012/09/20 18:00:23, rsc wrote:
    There sure are a lot of these. Might as well merge them all into one
    case list
    instead of having lots of different case lists that all do nothing.
    Done.
    PTAL

    https://codereview.appspot.com/6497074/
  • Dvyukov at Sep 21, 2012 at 12:27 am

    On 2012/09/20 18:00:23, rsc wrote:
    I'm a little skeptical that racewalk.c is correct. I don't see any kind of
    generic traversal and yet so many nodes with subexpressions don't walk into the
    subexpressions. What am I missing?
    You are missing nothing. That's just what I currently have:
    On 2012/09/03 09:00:32, dvyukov wrote:
    I must say that the pass itself contains a lot of issues (you may see commented
    out code, and TODOs).
    However it works reasonably well in practice and does not produce
    known false
    positives. I would prefer to do small fixes now if you know how to fix, and
    delay more significant fixes until after we upstream everything else.

    https://codereview.appspot.com/6497074/
  • Rsc at Sep 21, 2012 at 2:21 am
    It would help me a little to have a comment at the top of racewalk that
    explains what it should be doing. I am happy to iterate with you on what
    the comment says, or even to write it if we can have a short discussion
    here over email. I can see that callinstr inserts a call to the race
    instrumentation and that many of the top-level node types recurse to
    walk the arguments.

    But for example why is slicing a string not relevant? It seems like the
    string might be loaded from a shared variable, and of course the indices
    are integers that might be computed with races too. Or why is the count
    passed to make(chan int, n) not considered?

    One possible reason is that these nodes cannot happen, but there is a
    separate block for those, and they are not listed in it. So I'm not
    entirely sure what's going on.

    Thanks.


    https://codereview.appspot.com/6497074/diff/23001/src/cmd/gc/racewalk.c
    File src/cmd/gc/racewalk.c (right):

    https://codereview.appspot.com/6497074/diff/23001/src/cmd/gc/racewalk.c#newcode284
    src/cmd/gc/racewalk.c:284: case OSLICESTR:
    I'm a little confused. Just to take one example, why are the arguments
    to OSLICESTR not relevant here?

    https://codereview.appspot.com/6497074/diff/23001/src/cmd/gc/racewalk.c#newcode330
    src/cmd/gc/racewalk.c:330: default:
    Please put this at the top of the switch, just because that's what other
    files do.

    https://codereview.appspot.com/6497074/
  • Dmitry Vyukov at Sep 21, 2012 at 2:36 am
    All memory accesses must be instrumented. make(chan int, n) and others are
    just not implemented yet.

    There are (at least) 2 quirks.
    1. raceread() (or racewrite()) must be executed iff the memory access is
    executed. That's why && and || are not currently instrumented. The proper
    transformation of && would be:
    a&&b
    \/\/\/\/\/
    raceread(&a);
    if(a) {
    raceread(&b);
    if(b)
    yield true;
    }
    yield false;

    And note that a&&b may appear in switch case or as a nested function
    argument (foo(..., bar(..., a&&b, ...), ...)).

    2. raceread/racewrite can't be reordered with function calls (because
    function calls can contain synchronization).
    E.g. if we have
    foo(bar(), a);
    it's *incorrect* to transform it just to:
    raceread(&a);
    foo(bar(), a);
    because bar() is sequenced before read of a, and can e.g. lock a mutex
    (thus preventing race on a).
    So the correct transformation would be along the lines of:
    x = bar();
    raceread(&a);
    foo(x, a);








    On Thu, Sep 20, 2012 at 7:21 PM, wrote:

    It would help me a little to have a comment at the top of racewalk that
    explains what it should be doing. I am happy to iterate with you on what
    the comment says, or even to write it if we can have a short discussion
    here over email. I can see that callinstr inserts a call to the race
    instrumentation and that many of the top-level node types recurse to
    walk the arguments.

    But for example why is slicing a string not relevant? It seems like the
    string might be loaded from a shared variable, and of course the indices
    are integers that might be computed with races too. Or why is the count
    passed to make(chan int, n) not considered?

    One possible reason is that these nodes cannot happen, but there is a
    separate block for those, and they are not listed in it. So I'm not
    entirely sure what's going on.

    Thanks.


    https://codereview.appspot.**com/6497074/diff/23001/src/**
    cmd/gc/racewalk.c<https://codereview.appspot.com/6497074/diff/23001/src/cmd/gc/racewalk.c>
    File src/cmd/gc/racewalk.c (right):

    https://codereview.appspot.**com/6497074/diff/23001/src/**
    cmd/gc/racewalk.c#newcode284<https://codereview.appspot.com/6497074/diff/23001/src/cmd/gc/racewalk.c#newcode284>
    src/cmd/gc/racewalk.c:284: case OSLICESTR:
    I'm a little confused. Just to take one example, why are the arguments
    to OSLICESTR not relevant here?

    https://codereview.appspot.**com/6497074/diff/23001/src/**
    cmd/gc/racewalk.c#newcode330<https://codereview.appspot.com/6497074/diff/23001/src/cmd/gc/racewalk.c#newcode330>
    src/cmd/gc/racewalk.c:330: default:
    Please put this at the top of the switch, just because that's what other
    files do.

    https://codereview.appspot.**com/6497074/<https://codereview.appspot.com/6497074/>
  • Russ Cox at Sep 21, 2012 at 2:44 am

    On Thu, Sep 20, 2012 at 10:36 PM, Dmitry Vyukov wrote:
    All memory accesses must be instrumented. make(chan int, n) and others are
    just not implemented yet.

    There are (at least) 2 quirks.
    1. raceread() (or racewrite()) must be executed iff the memory access is
    executed. That's why && and || are not currently instrumented. The proper
    transformation of && would be:
    a&&b
    \/\/\/\/\/
    raceread(&a);
    if(a) {
    raceread(&b);
    if(b)
    yield true;
    }
    yield false;

    And note that a&&b may appear in switch case or as a nested function
    argument (foo(..., bar(..., a&&b, ...), ...)).
    We have the same kinds of problems hoisting function calls out of a &&
    b. See case OANDAND around walk.c:504 and its use of a separate list
    and addinit. You should be able to do something similar.
    2. raceread/racewrite can't be reordered with function calls (because
    function calls can contain synchronization).
    E.g. if we have
    foo(bar(), a);
    it's *incorrect* to transform it just to:
    raceread(&a);
    foo(bar(), a);
    because bar() is sequenced before read of a, and can e.g. lock a mutex (thus
    preventing race on a).
    So the correct transformation would be along the lines of:
    x = bar();
    raceread(&a);
    foo(x, a);
    Actually the read of a is not sequenced compared to the call of bar.
    Only function calls and communication operations are sequenced. In
    this example 'a' can be evaluated in either order. However, builtins
    count as function calls, so if a were 'len(a)' the call to len would
    be sequenced but the evaluation of a would not, if that makes sense.

    I'm happy with this code as is, mostly, but please separate the bottom
    list of cases into ones that you expect to ignore forever (such as
    print or println) and ones that are just unimplemented. Please mark
    the latter block:

    // unimplemented
    goto ret;

    or something like that. Thanks.

    Russ
  • Dmitry Vyukov at Sep 22, 2012 at 5:42 am

    On Thu, Sep 20, 2012 at 7:21 PM, wrote:

    It would help me a little to have a comment at the top of racewalk that
    explains what it should be doing. I am happy to iterate with you on what
    the comment says, or even to write it

    That would be great. I suspect it will be much faster on your side.

    Gc AST is considerably harder to analyze and mutate than gcc/llvm. Gcc
    lowers to very primitive AST, while llvm uses abstract assembly at all
    (each memory load and store is a separate instruction). So I tried to stick
    with the most simple transformations to not end up with crashing compiler
    and/or code.

    However, I expect there to be a long tail of races that we currently do not
    catch due to missed instrumentation. Every time I instrumented few more
    cases, I catch few more bugs. E.g.
    http://code.google.com/p/go/issues/detail?id=3886 was caught only when I
    instrument arguments of slicing operator.

    There is another aspect - redundant instrumentation. E.g. for
    x = x + y
    z = x + z
    ideally we emit only 1 racewrite(&x). But that's for future.




    if we can have a short discussion
    here over email. I can see that callinstr inserts a call to the race
    instrumentation and that many of the top-level node types recurse to
    walk the arguments.

    But for example why is slicing a string not relevant? It seems like the
    string might be loaded from a shared variable, and of course the indices
    are integers that might be computed with races too. Or why is the count
    passed to make(chan int, n) not considered?

    One possible reason is that these nodes cannot happen, but there is a
    separate block for those, and they are not listed in it. So I'm not
    entirely sure what's going on.

    Thanks.


    https://codereview.appspot.**com/6497074/diff/23001/src/**
    cmd/gc/racewalk.c<https://codereview.appspot.com/6497074/diff/23001/src/cmd/gc/racewalk.c>
    File src/cmd/gc/racewalk.c (right):

    https://codereview.appspot.**com/6497074/diff/23001/src/**
    cmd/gc/racewalk.c#newcode284<https://codereview.appspot.com/6497074/diff/23001/src/cmd/gc/racewalk.c#newcode284>
    src/cmd/gc/racewalk.c:284: case OSLICESTR:
    I'm a little confused. Just to take one example, why are the arguments
    to OSLICESTR not relevant here?

    https://codereview.appspot.**com/6497074/diff/23001/src/**
    cmd/gc/racewalk.c#newcode330<https://codereview.appspot.com/6497074/diff/23001/src/cmd/gc/racewalk.c#newcode330>
    src/cmd/gc/racewalk.c:330: default:
    Please put this at the top of the switch, just because that's what other
    files do.

    https://codereview.appspot.**com/6497074/<https://codereview.appspot.com/6497074/>
  • Dvyukov at Sep 22, 2012 at 4:03 am

    On 2012/09/21 02:44:10, rsc wrote:
    On Thu, Sep 20, 2012 at 10:36 PM, Dmitry Vyukov
    wrote:
    All memory accesses must be instrumented. make(chan int, n) and
    others are
    just not implemented yet.

    There are (at least) 2 quirks.
    1. raceread() (or racewrite()) must be executed iff the memory
    access is
    executed. That's why && and || are not currently instrumented. The
    proper
    transformation of && would be:
    a&&b
    \/\/\/\/\/
    raceread(&a);
    if(a) {
    raceread(&b);
    if(b)
    yield true;
    }
    yield false;

    And note that a&&b may appear in switch case or as a nested function
    argument (foo(..., bar(..., a&&b, ...), ...)).
    We have the same kinds of problems hoisting function calls out of a &&
    b. See case OANDAND around walk.c:504 and its use of a separate list
    and addinit. You should be able to do something similar.
    2. raceread/racewrite can't be reordered with function calls
    (because
    function calls can contain synchronization).
    E.g. if we have
    foo(bar(), a);
    it's *incorrect* to transform it just to:
    raceread(&a);
    foo(bar(), a);
    because bar() is sequenced before read of a, and can e.g. lock a
    mutex (thus
    preventing race on a).
    So the correct transformation would be along the lines of:
    x = bar();
    raceread(&a);
    foo(x, a);
    Actually the read of a is not sequenced compared to the call of bar.
    Only function calls and communication operations are sequenced. In
    this example 'a' can be evaluated in either order. However, builtins
    count as function calls, so if a were 'len(a)' the call to len would
    be sequenced but the evaluation of a would not, if that makes sense.
    I'm happy with this code as is, mostly, but please separate the bottom
    list of cases into ones that you expect to ignore forever (such as
    print or println) and ones that are just unimplemented. Please mark
    the latter block:
    // unimplemented
    goto ret;
    or something like that. Thanks.

    Done. PTAL.
    I am not sure right now what exactly cases does not require
    instrumentation, so I moved only the obvious ones.


    https://codereview.appspot.com/6497074/
  • Dvyukov at Sep 22, 2012 at 4:03 am
    https://codereview.appspot.com/6497074/diff/23001/src/cmd/gc/racewalk.c
    File src/cmd/gc/racewalk.c (right):

    https://codereview.appspot.com/6497074/diff/23001/src/cmd/gc/racewalk.c#newcode330
    src/cmd/gc/racewalk.c:330: default:
    On 2012/09/21 02:21:40, rsc wrote:
    Please put this at the top of the switch, just because that's what
    other files
    do.
    Done.

    https://codereview.appspot.com/6497074/

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedSep 21, '12 at 12:19a
activeSep 22, '12 at 5:42a
posts9
users2
websitegolang.org

2 users in discussion

Dvyukov: 7 posts Russ Cox: 2 posts

People

Translate

site design / logo © 2022 Grokbase