code review 6610064: race: syscall changes

race: syscall changes
This is a part of a bigger change that adds data race detection feature:
http://codereview.appspot.com/6456044
The purpose of this patch is to provide coarse-grained synchronization
between all Read() and Write() calls.

[dvyukov, 2012-10-07 14:01:50]

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

I'd like you to review this change to
https://dvyukov%40google.com@code.google.com/p/go/

[rsc, 2012-10-07 14:22:55]

https://codereview.appspot.com/6610064/diff/6019/src/pkg/syscall/syscall_darw...
File src/pkg/syscall/syscall_darwin.go (left):

https://codereview.appspot.com/6610064/diff/6019/src/pkg/syscall/syscall_darw...
src/pkg/syscall/syscall_darwin.go:165: //sys	Read(fd int, p []byte) (n int, err
error)
Please let mksyscall do the generation of the system call. Instead, you can do

//sys read 

here and then write 

func Read(fd int, p []byte) (n int, err error) {
    n, err = read(fd, p)
    racestuff
    return
}

Same for Write.

[dvyukov, 2012-10-07 14:37:29]

makes sense


On Sun, Oct 7, 2012 at 7:22 AM, <rsc@golang.org> wrote:

>
> https://codereview.appspot.**com/6610064/diff/6019/src/pkg/**
>
syscall/syscall_darwin.go<https://codereview.appspot.com/6610064/diff/6019/src/pkg/syscall/syscall_darwin.go>
> File src/pkg/syscall/syscall_**darwin.go (left):
>
> https://codereview.appspot.**com/6610064/diff/6019/src/pkg/**
>
syscall/syscall_darwin.go#**oldcode165<https://codereview.appspot.com/6610064/diff/6019/src/pkg/syscall/syscall_darwin.go#oldcode165>
> src/pkg/syscall/syscall_**darwin.go:165: //sys    Read(fd int, p []byte)
> (n
> int, err error)
> Please let mksyscall do the generation of the system call. Instead, you
> can do
>
> //sys read
>
> here and then write
>
> func Read(fd int, p []byte) (n int, err error) {
>     n, err = read(fd, p)
>     racestuff
>     return
> }
>
> Same for Write.
>
>
https://codereview.appspot.**com/6610064/<https://codereview.appspot.com/6610...
>

[dvyukov, 2012-10-07 17:57:57]

On 2012/10/07 14:37:29, dvyukov wrote:
> makes sense
> 
> 
> On Sun, Oct 7, 2012 at 7:22 AM, <mailto:rsc@golang.org> wrote:
> 
> >
> > https://codereview.appspot.**com/6610064/diff/6019/src/pkg/**
> >
>
syscall/syscall_darwin.go<https://codereview.appspot.com/6610064/diff/6019/src/pkg/syscall/syscall_darwin.go>
> > File src/pkg/syscall/syscall_**darwin.go (left):
> >
> > https://codereview.appspot.**com/6610064/diff/6019/src/pkg/**
> >
>
syscall/syscall_darwin.go#**oldcode165<https://codereview.appspot.com/6610064/diff/6019/src/pkg/syscall/syscall_darwin.go#oldcode165>
> > src/pkg/syscall/syscall_**darwin.go:165: //sys    Read(fd int, p []byte)
> > (n
> > int, err error)
> > Please let mksyscall do the generation of the system call. Instead, you
> > can do
> >
> > //sys read
> >
> > here and then write
> >
> > func Read(fd int, p []byte) (n int, err error) {
> >     n, err = read(fd, p)
> >     racestuff
> >     return
> > }
> >
> > Same for Write.
> >
> >
>
https://codereview.appspot.**com/6610064/%3Chttps://codereview.appspot.com/66...>


I've renamed:
read->readlen
write->writelen
Read->read
Write->write
and provided common Read/Write in syscall.go

Now I can't understand why api says:

-pkg syscall (linux-amd64), func Read(int, []byte) (int, error)
-pkg syscall (linux-amd64), func Write(int, []byte) (int, error)

while

$ go doc syscall | egrep "Read|Write"
func Read(fd int, p []byte) (n int, err error)
func ReadDirent(fd int, buf []byte) (n int, err error)
func Readlink(path string, buf []byte) (n int, err error)
func Write(fd int, p []byte) (n int, err error)

?

[dvyukov, 2012-10-07 17:59:42]

On 2012/10/07 17:57:57, dvyukov wrote:
> On 2012/10/07 14:37:29, dvyukov wrote:
> > makes sense
> > 
> > 
> > On Sun, Oct 7, 2012 at 7:22 AM, <mailto:rsc@golang.org> wrote:
> > 
> > >
> > > https://codereview.appspot.**com/6610064/diff/6019/src/pkg/**
> > >
> >
>
syscall/syscall_darwin.go<https://codereview.appspot.com/6610064/diff/6019/src/pkg/syscall/syscall_darwin.go>
> > > File src/pkg/syscall/syscall_**darwin.go (left):
> > >
> > > https://codereview.appspot.**com/6610064/diff/6019/src/pkg/**
> > >
> >
>
syscall/syscall_darwin.go#**oldcode165<https://codereview.appspot.com/6610064/diff/6019/src/pkg/syscall/syscall_darwin.go#oldcode165>
> > > src/pkg/syscall/syscall_**darwin.go:165: //sys    Read(fd int, p []byte)
> > > (n
> > > int, err error)
> > > Please let mksyscall do the generation of the system call. Instead, you
> > > can do
> > >
> > > //sys read
> > >
> > > here and then write
> > >
> > > func Read(fd int, p []byte) (n int, err error) {
> > >     n, err = read(fd, p)
> > >     racestuff
> > >     return
> > > }
> > >
> > > Same for Write.
> > >
> > >
> >
>
https://codereview.appspot.**com/6610064/%253Chttps://codereview.appspot.com/...>
> 
> 
> I've renamed:
> read->readlen
> write->writelen
> Read->read
> Write->write
> and provided common Read/Write in syscall.go
> 
> Now I can't understand why api says:
> 
> -pkg syscall (linux-amd64), func Read(int, []byte) (int, error)
> -pkg syscall (linux-amd64), func Write(int, []byte) (int, error)
> 
> while
> 
> $ go doc syscall | egrep "Read|Write"
> func Read(fd int, p []byte) (n int, err error)
> func ReadDirent(fd int, buf []byte) (n int, err error)
> func Readlink(path string, buf []byte) (n int, err error)
> func Write(fd int, p []byte) (n int, err error)
> 
> ?

Api says:

-pkg syscall (darwin-386), func Read(int, []byte) (int, error)
-pkg syscall (darwin-386), func Write(int, []byte) (int, error)
-pkg syscall (darwin-386-cgo), func Read(int, []byte) (int, error)
-pkg syscall (darwin-386-cgo), func Write(int, []byte) (int, error)
-pkg syscall (darwin-amd64), func Read(int, []byte) (int, error)
-pkg syscall (darwin-amd64), func Write(int, []byte) (int, error)
-pkg syscall (darwin-amd64-cgo), func Read(int, []byte) (int, error)
-pkg syscall (darwin-amd64-cgo), func Write(int, []byte) (int, error)
-pkg syscall (freebsd-386), func Read(int, []byte) (int, error)
-pkg syscall (freebsd-386), func Write(int, []byte) (int, error)
-pkg syscall (freebsd-amd64), func Read(int, []byte) (int, error)
-pkg syscall (freebsd-amd64), func Write(int, []byte) (int, error)
-pkg syscall (linux-386), func Read(int, []byte) (int, error)
-pkg syscall (linux-386), func Write(int, []byte) (int, error)
-pkg syscall (linux-386-cgo), func Read(int, []byte) (int, error)
-pkg syscall (linux-386-cgo), func Write(int, []byte) (int, error)
-pkg syscall (linux-amd64), func Read(int, []byte) (int, error)
-pkg syscall (linux-amd64), func Write(int, []byte) (int, error)
-pkg syscall (linux-amd64-cgo), func Read(int, []byte) (int, error)
-pkg syscall (linux-amd64-cgo), func Write(int, []byte) (int, error)
-pkg syscall (linux-arm), func Read(int, []byte) (int, error)
-pkg syscall (linux-arm), func Write(int, []byte) (int, error)
-pkg syscall (windows-386), func Read(Handle, []byte) (int, error)
-pkg syscall (windows-386), func Write(Handle, []byte) (int, error)
-pkg syscall (windows-amd64), func Read(Handle, []byte) (int, error)
-pkg syscall (windows-amd64), func Write(Handle, []byte) (int, error)

any ideas?

[bradfitz, 2012-10-07 18:43:32]

It's a Windows issue.  Note this part of the diff:

-pkg syscall (windows-amd64), func Read(Handle, []byte) (int, error)
-pkg syscall (windows-amd64), func Write(Handle, []byte) (int, error)
+pkg syscall (windows-amd64), func WriteConsole(Handle, *uint16, uint32,
*uint32, *byte) error
+pkg syscall, func Read(int, []byte) (int, error)
+pkg syscall, func Write(int, []byte) (int, error)

You added your Read & Wrap wrappers to syscall.go, but Windows takes a
Handle, not an int.


On Sun, Oct 7, 2012 at 10:59 AM, <dvyukov@google.com> wrote:

> On 2012/10/07 17:57:57, dvyukov wrote:
>
>> On 2012/10/07 14:37:29, dvyukov wrote:
>> > makes sense
>> >
>> >
>> > On Sun, Oct 7, 2012 at 7:22 AM, <mailto:rsc@golang.org> wrote:
>> >
>> > >
>> > > https://codereview.appspot.****com/6610064/diff/6019/src/pkg/****
>> > >
>> >
>>
>
> syscall/syscall_darwin.go<http**s://codereview.appspot.com/**
>
6610064/diff/6019/src/pkg/**syscall/syscall_darwin.go<https://codereview.appspot.com/6610064/diff/6019/src/pkg/syscall/syscall_darwin.go>
> >
>
>> > > File src/pkg/syscall/syscall_****darwin.go (left):
>> > >
>> > > https://codereview.appspot.****com/6610064/diff/6019/src/pkg/****
>> > >
>> >
>>
>
> syscall/syscall_darwin.go#****oldcode165<https://codereview.**
> appspot.com/6610064/diff/6019/**src/pkg/syscall/syscall_**
>
darwin.go#oldcode165<https://codereview.appspot.com/6610064/diff/6019/src/pkg/syscall/syscall_darwin.go#oldcode165>
> >
>
>> > > src/pkg/syscall/syscall_****darwin.go:165: //sys    Read(fd int, p
>>
> []byte)
>
>> > > (n
>> > > int, err error)
>> > > Please let mksyscall do the generation of the system call.
>>
> Instead, you
>
>> > > can do
>> > >
>> > > //sys read
>> > >
>> > > here and then write
>> > >
>> > > func Read(fd int, p []byte) (n int, err error) {
>> > >     n, err = read(fd, p)
>> > >     racestuff
>> > >     return
>> > > }
>> > >
>> > > Same for Write.
>> > >
>> > >
>> >
>>
>
> https://codereview.appspot.****com/6610064/%253Chttps://coder**
> eview.appspot.com/6610064/ <http://codereview.appspot.com/6610064/>>
>
>
>
>  I've renamed:
>> read->readlen
>> write->writelen
>> Read->read
>> Write->write
>> and provided common Read/Write in syscall.go
>>
>
>  Now I can't understand why api says:
>>
>
>  -pkg syscall (linux-amd64), func Read(int, []byte) (int, error)
>> -pkg syscall (linux-amd64), func Write(int, []byte) (int, error)
>>
>
>  while
>>
>
>  $ go doc syscall | egrep "Read|Write"
>> func Read(fd int, p []byte) (n int, err error)
>> func ReadDirent(fd int, buf []byte) (n int, err error)
>> func Readlink(path string, buf []byte) (n int, err error)
>> func Write(fd int, p []byte) (n int, err error)
>>
>
>  ?
>>
>
> Api says:
>
> -pkg syscall (darwin-386), func Read(int, []byte) (int, error)
> -pkg syscall (darwin-386), func Write(int, []byte) (int, error)
> -pkg syscall (darwin-386-cgo), func Read(int, []byte) (int, error)
> -pkg syscall (darwin-386-cgo), func Write(int, []byte) (int, error)
> -pkg syscall (darwin-amd64), func Read(int, []byte) (int, error)
> -pkg syscall (darwin-amd64), func Write(int, []byte) (int, error)
> -pkg syscall (darwin-amd64-cgo), func Read(int, []byte) (int, error)
> -pkg syscall (darwin-amd64-cgo), func Write(int, []byte) (int, error)
> -pkg syscall (freebsd-386), func Read(int, []byte) (int, error)
> -pkg syscall (freebsd-386), func Write(int, []byte) (int, error)
> -pkg syscall (freebsd-amd64), func Read(int, []byte) (int, error)
> -pkg syscall (freebsd-amd64), func Write(int, []byte) (int, error)
> -pkg syscall (linux-386), func Read(int, []byte) (int, error)
> -pkg syscall (linux-386), func Write(int, []byte) (int, error)
> -pkg syscall (linux-386-cgo), func Read(int, []byte) (int, error)
> -pkg syscall (linux-386-cgo), func Write(int, []byte) (int, error)
>
> -pkg syscall (linux-amd64), func Read(int, []byte) (int, error)
> -pkg syscall (linux-amd64), func Write(int, []byte) (int, error)
> -pkg syscall (linux-amd64-cgo), func Read(int, []byte) (int, error)
> -pkg syscall (linux-amd64-cgo), func Write(int, []byte) (int, error)
> -pkg syscall (linux-arm), func Read(int, []byte) (int, error)
> -pkg syscall (linux-arm), func Write(int, []byte) (int, error)
> -pkg syscall (windows-386), func Read(Handle, []byte) (int, error)
> -pkg syscall (windows-386), func Write(Handle, []byte) (int, error)
> -pkg syscall (windows-amd64), func Read(Handle, []byte) (int, error)
> -pkg syscall (windows-amd64), func Write(Handle, []byte) (int, error)
>
> any ideas?
>
>
https://codereview.appspot.**com/6610064/<https://codereview.appspot.com/6610...
>

[brainman, 2012-10-08 00:32:37]

> ...
> any ideas?

Like Brad said, syscall.Read and syscall.Write have different signature on
windows. In fact, your CL does not compile on windows. You can see it yourself:
http://code.google.com/p/go-wiki/wiki/WindowsCrossCompiling.

The obvious way to fix your problem is to move Read and Write functions into
different source file with // +build !windows
 in it. Unfortunately, we do not have one yet, so create one. You would also
need to add windows versions of Read and Write in syscall_windows.go

Alex

[dvyukov, 2012-10-08 11:25:43]

Hello rsc@golang.org, bradfitz@golang.org, alex.brainman@gmail.com (cc:
golang-dev@googlegroups.com),

Please take another look.

[dvyukov, 2012-10-08 11:27:18]

On 2012/10/08 00:32:37, brainman wrote:
> > ...
> > any ideas?
> 
> Like Brad said, syscall.Read and syscall.Write have different signature on
> windows. In fact, your CL does not compile on windows. You can see it
yourself:
> http://code.google.com/p/go-wiki/wiki/WindowsCrossCompiling.
> 
> The obvious way to fix your problem is to move Read and Write functions into
> different source file with // +build !windows
>  in it. Unfortunately, we do not have one yet, so create one. You would also
> need to add windows versions of Read and Write in syscall_windows.go


Thanks! Done. PTAL.

Now I have version of Read/Write in syscall_unix.go, syscall_windows.go and
syscall_plan9.go. The tests pass now on Linux, and I've cross-built Windows
version as well.

[brainman, 2012-10-08 22:34:44]

On 2012/10/08 11:27:18, dvyukov wrote:
> ...
> Now I have version of Read/Write in syscall_unix.go, syscall_windows.go and
> syscall_plan9.go. The tests pass now on Linux, and I've cross-built Windows
> version as well.

windows part

LGTM

Even better then my suggestion (I have forgotten about syscall_unix.go).

Full build runs to success on windows/386

Alex

[dvyukov, 2012-10-09 03:57:52]

On Tue, Oct 9, 2012 at 2:34 AM, <alex.brainman@gmail.com> wrote:

> On 2012/10/08 11:27:18, dvyukov wrote:
>
>> ...
>>
>> Now I have version of Read/Write in syscall_unix.go,
>>
> syscall_windows.go and
>
>> syscall_plan9.go. The tests pass now on Linux, and I've cross-built
>>
> Windows
>
>> version as well.
>>
>
> windows part
>
> LGTM
>
> Even better then my suggestion (I have forgotten about syscall_unix.go).
>
> Full build runs to success on windows/386
>

Russ?

[rsc, 2012-10-09 15:44:50]

LGTM

[dvyukov, 2012-10-09 16:52:24]

*** Submitted as http://code.google.com/p/go/source/detail?r=ba847b110365 ***

race: syscall changes
This is a part of a bigger change that adds data race detection feature:
http://codereview.appspot.com/6456044
The purpose of this patch is to provide coarse-grained synchronization
between all Read() and Write() calls.

R=rsc, bradfitz, alex.brainman
CC=golang-dev
http://codereview.appspot.com/6610064