race: gc changes
This is the first part of a bigger change that adds data race detection feature:
http://codereview.appspot.com/6456044
This change makes gc compiler instrument memory accesses when supplied with -b flag.
https://codereview.appspot.com/6497074/diff/5001/src/cmd/gc/racewalk.c File src/cmd/gc/racewalk.c (right): https://codereview.appspot.com/6497074/diff/5001/src/cmd/gc/racewalk.c#newcode1 src/cmd/gc/racewalk.c:1: // Copyright 2011 The Go Authors. All rights reserved. ...
11 years, 6 months ago
(2012-09-03 07:34:48 UTC)
#2
Thanks! https://codereview.appspot.com/6497074/diff/5001/src/cmd/gc/racewalk.c File src/cmd/gc/racewalk.c (right): https://codereview.appspot.com/6497074/diff/5001/src/cmd/gc/racewalk.c#newcode1 src/cmd/gc/racewalk.c:1: // Copyright 2011 The Go Authors. All rights ...
11 years, 6 months ago
(2012-09-03 08:55:33 UTC)
#4
I must say that the pass itself contains a lot of issues (you may see ...
11 years, 6 months ago
(2012-09-03 09:00:32 UTC)
#5
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.
On 2012/09/20 18:00:23, rsc wrote: > I'm a little skeptical that racewalk.c is correct. I ...
11 years, 6 months ago
(2012-09-21 00:20:34 UTC)
#10
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.
It would help me a little to have a comment at the top of racewalk ...
11 years, 6 months ago
(2012-09-21 02:21:39 UTC)
#12
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#newco...
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#newco...
src/cmd/gc/racewalk.c:330: default:
Please put this at the top of the switch, just because that's what other files
do.
All memory accesses must be instrumented. make(chan int, n) and others are just not implemented ...
11 years, 6 months ago
(2012-09-21 02:36:22 UTC)
#13
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, <rsc@golang.org> 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/6497...
>
On Thu, Sep 20, 2012 at 10:36 PM, Dmitry Vyukov <dvyukov@google.com> wrote: > All memory ...
11 years, 6 months ago
(2012-09-21 02:44:10 UTC)
#14
On Thu, Sep 20, 2012 at 10:36 PM, Dmitry Vyukov <dvyukov@google.com> 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
On 2012/09/21 02:44:10, rsc wrote: > On Thu, Sep 20, 2012 at 10:36 PM, Dmitry ...
11 years, 6 months ago
(2012-09-22 04:03:42 UTC)
#15
On 2012/09/21 02:44:10, rsc wrote:
> On Thu, Sep 20, 2012 at 10:36 PM, Dmitry Vyukov <mailto:dvyukov@google.com>
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.
On Thu, Sep 20, 2012 at 7:21 PM, <rsc@golang.org> wrote: > It would help me ...
11 years, 6 months ago
(2012-09-22 05:42:56 UTC)
#17
On Thu, Sep 20, 2012 at 7:21 PM, <rsc@golang.org> 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/6497...
>
LGTM https://codereview.appspot.com/6497074/diff/30001/src/cmd/gc/racewalk.c File src/cmd/gc/racewalk.c (right): https://codereview.appspot.com/6497074/diff/30001/src/cmd/gc/racewalk.c#newcode4 src/cmd/gc/racewalk.c:4: // The racewalk pass modifies the code tree ...
11 years, 6 months ago
(2012-10-01 20:07:49 UTC)
#18
LGTM
https://codereview.appspot.com/6497074/diff/30001/src/cmd/gc/racewalk.c
File src/cmd/gc/racewalk.c (right):
https://codereview.appspot.com/6497074/diff/30001/src/cmd/gc/racewalk.c#newcode4
src/cmd/gc/racewalk.c:4:
// The racewalk pass modifies the code tree for the function as follows:
//
// 1. It inserts a call to racefuncenter at the beginning of each function.
// 2. It inserts a call to racefuncexit at the end of each function.
// 3. It inserts a call to raceread before each memory read.
// 4. It inserts a call to racewrite before each memory write.
//
// The rewriting is not yet complete. Certain nodes are not rewritten
// but should be.
Issue 6497074: code review 6497074: race: gc changes
(Closed)
Created 11 years, 7 months ago by dvyukov
Modified 11 years, 6 months ago
Reviewers:
Base URL:
Comments: 19