code review 8053044: cmd/gc: instrument arrays properly in race detector.

cmd/gc: instrument arrays properly in race detector.

The previous implementation would only record access to
the address of the array but the memory access to the whole
memory range must be recorded instead.

[remyoudompheng, 2013-03-27 21:58:29]

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

I'd like you to review this change to
https://go.googlecode.com/hg/

[remyoudompheng, 2013-03-27 22:01:42]

Now go test -race runtime/race complains:

=== RUN TestRace
[...]
RaceFailingSliceStruct                   .
RaceFailingAppendSliceStruct             .
[...]
Passed 266 of 269 tests (98.88%, 0+, 3-)
5 expected failures (2 has not fail)
--- FAIL: TestRace (12.45 seconds)
FAIL
exit status 1
FAIL	runtime/race	12.474s

But I am pretty positive that these tests still fail, something looks wrong.

Rémy.

[dvyukov, 2013-03-28 11:11:45]

On 2013/03/27 22:01:42, remyoudompheng wrote:
> Now go test -race runtime/race complains:
> 
> === RUN TestRace
> [...]
> RaceFailingSliceStruct                   .
> RaceFailingAppendSliceStruct             .
> [...]
> Passed 266 of 269 tests (98.88%, 0+, 3-)
> 5 expected failures (2 has not fail)
> --- FAIL: TestRace (12.45 seconds)
> FAIL
> exit status 1
> FAIL	runtime/race	12.474s
> 
> But I am pretty positive that these tests still fail, something looks wrong.

In clean tree with this CL, I see:

Passed 264 of 269 tests (98.14%, 0+, 5-)
5 expected failures (0 has not fail)

[dvyukov, 2013-03-28 11:27:20]

LGTM

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

https://codereview.appspot.com/8053044/diff/5001/src/cmd/gc/racewalk.c#newcod...
src/cmd/gc/racewalk.c:471: if(callinstr(&f, init, wr, 0)) {
Now I am thinking, since we pass &f to callinstr(), can't it change f?..
I can't come up with a counter-example and function calls should be generally
moved away by previous passes. But I've added that 'hascalls' logic because
there were precedents.

Please add a check that f is not changed. This bug would be painful to debug.

https://codereview.appspot.com/8053044/diff/5001/src/cmd/gc/racewalk.c#newcod...
src/cmd/gc/racewalk.c:486: if(callinstr(&f, init, wr, 0)) {
and here as wel

[remyoudompheng, 2013-03-28 18:58:30]

On 2013/03/28 11:11:45, dvyukov wrote:
> On 2013/03/27 22:01:42, remyoudompheng wrote:
> > Now go test -race runtime/race complains:
> > 
> > === RUN TestRace
> > [...]
> > RaceFailingSliceStruct                   .
> > RaceFailingAppendSliceStruct             .
> > [...]
> > Passed 266 of 269 tests (98.88%, 0+, 3-)
> > 5 expected failures (2 has not fail)
> > --- FAIL: TestRace (12.45 seconds)
> > FAIL
> > exit status 1
> > FAIL	runtime/race	12.474s
> > 
> > But I am pretty positive that these tests still fail, something looks wrong.
> 
> In clean tree with this CL, I see:
> 
> Passed 264 of 269 tests (98.14%, 0+, 5-)
> 5 expected failures (0 has not fail)

Ah, I got confused because race_tes.go runs "go" regardless of what GOROOT may
be, so it is not necessarily testing what I think it does.

[remyoudompheng, 2013-03-28 19:03:34]

Does it LGTY with my remark?

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

https://codereview.appspot.com/8053044/diff/5001/src/cmd/gc/racewalk.c#newcod...
src/cmd/gc/racewalk.c:471: if(callinstr(&f, init, wr, 0)) {
On 2013/03/28 11:27:20, dvyukov wrote:
> Now I am thinking, since we pass &f to callinstr(), can't it change f?..
> I can't come up with a counter-example and function calls should be generally
> moved away by previous passes. But I've added that 'hascalls' logic because
> there were precedents.
> 
> Please add a check that f is not changed. This bug would be painful to debug.

I don't understand, why does it matter that f can change? typecheck can also
change f. But on the next loop iteration the value of f is discarded so we don't
care. Do we ?

[remyoudompheng, 2013-03-28 22:49:10]

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

Please take another look.

[remyoudompheng, 2013-03-28 22:49:21]

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

Please take another look.

[remyoudompheng, 2013-03-28 22:52:16]

I suggest a completely different approach now. I may revert to the previous
patchste if needed.

The new test, TestRaceNestedArrayCopy, makes the previous patch go out of memory
even when just using Point32k instead of Point1M.

https://codereview.appspot.com/8053044/diff/23001/src/pkg/runtime/race/testda...
File src/pkg/runtime/race/testdata/mop_test.go (right):

https://codereview.appspot.com/8053044/diff/23001/src/pkg/runtime/race/testda...
src/pkg/runtime/race/testdata/mop_test.go:510: func TestRaceNestedArrayCopy(t
*testing.T) {
This example is ridiculous but similar things may happen in real life. I'm not
sure we want the compiler to generate 1 million instructions just because a
[1<<20]struct{a, b int} was accessed.

[dvyukov, 2013-03-29 06:45:03]

I need some time to review this.

Yes, it's quite radical. I need to do more extensive testing if we are going to
push it before Go1.1.

FYI, for C when you do memcpy(p1, p2, 1<<30), ThreadSanitizer handles only
beginning of the array, because it's a way too expensive to process whole array.
And if there is a race, high chances it's either on the beginning or on each
element. Note that TSan internal shadow memory is 4x larger than user memory, so
1GB it needs to touch 4GB of memory.

It's good if the range accesses are handled completely in runtime, because then
I can implement similar logic.

It's bad that for struct {uint64; bool; bool}, we need to process 16 memory
accesses instead of 3.

[remyoudompheng, 2013-06-08 07:22:59]

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

Please take another look.

[r, 2013-06-08 15:18:24]

please write a more thorough CL description. i had no idea what 'instrumentation
 of arrays' would refer to.

[dvyukov, 2013-06-09 16:58:56]

LGTM, but please wait until I update the race runtime to better handle range
accesses (https://codereview.appspot.com/10082043/)

[remyoudompheng, 2013-06-09 17:01:38]

On 2013/06/09 16:58:56, dvyukov wrote:
> LGTM, but please wait until I update the race runtime to better handle range
> accesses (https://codereview.appspot.com/10082043/)

Yes, for some reason I had to rebuild a linux/amd64 syso with your patches for
it to work.

[remyoudompheng, 2013-06-09 17:04:33]

On 2013/06/08 15:18:24, r wrote:
> please write a more thorough CL description. i had no idea what
'instrumentation
>  of arrays' would refer to.

Done.

[dvyukov, 2013-06-09 17:07:34]

On 2013/06/09 17:04:33, remyoudompheng wrote:
> On 2013/06/08 15:18:24, r wrote:
> > please write a more thorough CL description. i had no idea what
> 'instrumentation
> >  of arrays' would refer to.
> 
> Done.

Btw, does it handle struct initialization?
x := &X{...}

[remyoudompheng, 2013-06-13 21:37:46]

Hello golang-dev@googlegroups.com, dvyukov@google.com, r@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[dvyukov, 2013-06-14 04:27:58]

LGTM

[remyoudompheng, 2013-06-14 07:15:54]

*** Submitted as https://code.google.com/p/go/source/detail?r=52d53d0e177e ***

cmd/gc: instrument arrays properly in race detector.

The previous implementation would only record access to
the address of the array but the memory access to the whole
memory range must be recorded instead.

R=golang-dev, dvyukov, r
CC=golang-dev
https://codereview.appspot.com/8053044