Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1576)

Issue 6497074: code review 6497074: race: gc changes (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
11 years, 7 months ago by dvyukov
Modified:
11 years, 6 months ago
Reviewers:
CC:
rsc, nigeltao, lvd, golang-dev
Visibility:
Public.

Description

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.

Patch Set 1 #

Patch Set 2 : diff -r 272e1dd72156 https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 3 : diff -r 272e1dd72156 https://dvyukov%40google.com@code.google.com/p/go/ #

Total comments: 7

Patch Set 4 : diff -r 272e1dd72156 https://dvyukov%40google.com@code.google.com/p/go/ #

Total comments: 1

Patch Set 5 : diff -r b52f95ff09ab https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 6 : diff -r 5d36993ec24f https://dvyukov%40google.com@code.google.com/p/go/ #

Total comments: 6

Patch Set 7 : diff -r 998714a0082d https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 8 : diff -r 998714a0082d https://dvyukov%40google.com@code.google.com/p/go/ #

Total comments: 3

Patch Set 9 : diff -r b4d17e91718d https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 10 : diff -r d4627113a3e9 https://dvyukov%40google.com@code.google.com/p/go/ #

Total comments: 2

Patch Set 11 : diff -r b09f4985724f https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 12 : diff -r b09f4985724f https://dvyukov%40google.com@code.google.com/p/go/ #

Patch Set 13 : diff -r b09f4985724f https://dvyukov%40google.com@code.google.com/p/go/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+467 lines, -4 lines) Patch
M src/cmd/gc/builtin.c View 1 2 3 4 1 chunk +4 lines, -0 lines 0 comments Download
M src/cmd/gc/doc.go View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
M src/cmd/gc/go.h View 1 2 3 4 5 6 7 8 9 2 chunks +6 lines, -0 lines 0 comments Download
M src/cmd/gc/lex.c View 1 2 3 4 5 3 chunks +12 lines, -4 lines 0 comments Download
M src/cmd/gc/pgen.c View 1 1 chunk +4 lines, -0 lines 0 comments Download
A src/cmd/gc/racewalk.c View 1 2 3 4 5 6 7 8 9 10 1 chunk +431 lines, -0 lines 0 comments Download
M src/cmd/gc/reflect.c View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -0 lines 0 comments Download
M src/cmd/gc/runtime.go View 1 2 3 4 5 6 7 8 9 1 chunk +6 lines, -0 lines 0 comments Download

Messages

Total messages: 19
dvyukov
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://dvyukov%40google.com@code.google.com/p/go/
11 years, 7 months ago (2012-09-02 14:46:52 UTC) #1
nigeltao
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
nigeltao
11 years, 6 months ago (2012-09-03 07:35:03 UTC) #3
dvyukov
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
dvyukov
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
dvyukov
https://codereview.appspot.com/6497074/diff/10001/src/cmd/gc/racewalk.c File src/cmd/gc/racewalk.c (right): https://codereview.appspot.com/6497074/diff/10001/src/cmd/gc/racewalk.c#newcode466 src/cmd/gc/racewalk.c:466: if((class&PHEAP) || class == PPARAMREF || class == PEXTERN ...
11 years, 6 months ago (2012-09-03 09:04:28 UTC) #6
dvyukov
Added -b flag to gc/doc.go. PTAL
11 years, 6 months ago (2012-09-18 19:46:36 UTC) #7
rsc
I'm a little skeptical that racewalk.c is correct. I don't see any kind of generic ...
11 years, 6 months ago (2012-09-20 18:00:23 UTC) #8
dvyukov
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 ...
11 years, 6 months ago (2012-09-21 00:19:28 UTC) #9
dvyukov
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
dvyukov
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 > ...
11 years, 6 months ago (2012-09-21 00:21:04 UTC) #11
rsc
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
dvyukov
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
rsc
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
dvyukov
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
dvyukov
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 ...
11 years, 6 months ago (2012-09-22 04:03:49 UTC) #16
dvyukov
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
rsc
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
dvyukov
11 years, 6 months ago (2012-10-02 06:06:30 UTC) #19
*** Submitted as http://code.google.com/p/go/source/detail?r=f468e193586b ***

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.

R=rsc, nigeltao, lvd
CC=golang-dev
http://codereview.appspot.com/6497074

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

http://codereview.appspot.com/6497074/diff/30001/src/cmd/gc/racewalk.c#newcode4
src/cmd/gc/racewalk.c:4: 
On 2012/10/01 20:07:49, rsc wrote:
> 
> // 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.

Done.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b