code review 45930043: syscall: mark arguments to Syscall as noescape

syscall: mark arguments to Syscall as noescape
Heap arguments to "async" syscalls will break when/if we have moving GC anyway.
With this change is must not break until moving GC, because a user must
reference the object in Go to preserve liveness. Otherwise the code is broken already.
Reduces number of leaked params from 125 to 36 on linux.

[dvyukov, 2014-01-01 14:01:07]

Hello golang-codereviews@googlegroups.com (cc: cshapiro@golang.org,
khr@golang.org, rsc@golang.org),

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

[mikio, 2014-01-02 11:57:28]

Can we have syscall_decl_$goos.go or appropriate files like other packages using
go:noescape annotation?
Also please don't forget socketcall/rawsocketcall on linux/386.

[dvyukov, 2014-01-09 13:38:37]

On Thu, Jan 2, 2014 at 3:57 PM,  <mikioh.mikioh@gmail.com> wrote:
> Can we have syscall_decl_$goos.go or appropriate files like other
> packages using go:noescape annotation?

Are you sure it's necessary? Why?

> Also please don't forget socketcall/rawsocketcall on linux/386.

Will do.

[mikio, 2014-01-10 02:22:41]

On Thu, Jan 9, 2014 at 10:38 PM, Dmitry Vyukov <dvyukov@google.com> wrote:

> Are you sure it's necessary? Why?

To keep separate go-decl+asm and go-decl+go-body doesn't make sense
for maintenance purposes?
Hm, but it's just a thought, leave it to you.

[dvyukov, 2014-01-10 09:03:36]

On Fri, Jan 10, 2014 at 6:22 AM, Mikio Hara <mikioh.mikioh@gmail.com> wrote:
> On Thu, Jan 9, 2014 at 10:38 PM, Dmitry Vyukov <dvyukov@google.com> wrote:
>
>> Are you sure it's necessary? Why?
>
> To keep separate go-decl+asm and go-decl+go-body doesn't make sense
> for maintenance purposes?
> Hm, but it's just a thought, leave it to you.

We can split it when/if it represents maintenance problem.

[bradfitz, 2014-01-16 18:27:24]

LGTM



On Wed, Jan 1, 2014 at 6:01 AM, <dvyukov@google.com> wrote:

> Reviewers: golang-codereviews,
>
> Message:
> Hello golang-codereviews@googlegroups.com (cc: cshapiro@golang.org,
> khr@golang.org, rsc@golang.org),
>
> I'd like you to review this change to
> https://dvyukov%40google.com@code.google.com/p/go/
>
>
> Description:
> syscall: mark arguments to Syscall as noescape
> Heap arguments to "async" syscalls will break when/if we have moving GC
> anyway.
> With this change is must not break until moving GC, because a user must
> reference the object in Go to preserve liveness. Otherwise the code is
> broken already.
> Reduces number of leaked params from 125 to 36 on linux.
>
> Please review this at https://codereview.appspot.com/45930043/
>
> Affected files (+57, -2 lines):
>   M src/cmd/gc/esc.c
>   M src/pkg/syscall/dll_windows.go
>   M src/pkg/syscall/syscall_plan9.go
>   M src/pkg/syscall/syscall_unix.go
>
>
> Index: src/cmd/gc/esc.c
> ===================================================================
> --- a/src/cmd/gc/esc.c
> +++ b/src/cmd/gc/esc.c
> @@ -1135,8 +1135,13 @@
>         if(func->nbody == nil) {
>                 if(func->noescape) {
>                         for(t=getinargx(func->type)->type; t; t=t->down)
> -                               if(haspointers(t->type))
> -                                       t->note = mktag(EscNone);
> +                               // Mark all arguments, not only pointers,
> +                               // to support the following use case.
> +                               // Syscall package converts all pointers
> to uintptr
> +                               // when calls asm-implemented Syscall
> function:
> +                               //
> +                               //   Syscall(SYS_FOO,
> uintptr(unsafe.Pointer(p)), 0, 0)
> +                               t->note = mktag(EscNone);
>                 }
>                 return;
>         }
> Index: src/pkg/syscall/dll_windows.go
> ===================================================================
> --- a/src/pkg/syscall/dll_windows.go
> +++ b/src/pkg/syscall/dll_windows.go
> @@ -20,11 +20,31 @@
>  func (e *DLLError) Error() string { return e.Msg }
>
>  // Implemented in ../runtime/syscall_windows.goc.
> +
> +// Pointers passed to syscalls must not escape (be accessed by OS after
> the syscall returns).
> +// For heap objects this will break when/if we have moving GC.
> +// And for other objects (global, C allocated) go:noescape has no effect.
> +
> +//go:noescape
> +
>  func Syscall(trap, nargs, a1, a2, a3 uintptr) (r1, r2 uintptr, err Errno)
> +
> +//go:noescape
> +
>  func Syscall6(trap, nargs, a1, a2, a3, a4, a5, a6 uintptr) (r1, r2
> uintptr, err Errno)
> +
> +//go:noescape
> +
>  func Syscall9(trap, nargs, a1, a2, a3, a4, a5, a6, a7, a8, a9 uintptr)
> (r1, r2 uintptr, err Errno)
> +
> +//go:noescape
> +
>  func Syscall12(trap, nargs, a1, a2, a3, a4, a5, a6, a7, a8, a9, a10, a11,
> a12 uintptr) (r1, r2 uintptr, err Errno)
> +
> +//go:noescape
> +
>  func Syscall15(trap, nargs, a1, a2, a3, a4, a5, a6, a7, a8, a9, a10, a11,
> a12, a13, a14, a15 uintptr) (r1, r2 uintptr, err Errno)
> +
>  func loadlibrary(filename *uint16) (handle uintptr, err Errno)
>  func getprocaddress(handle uintptr, procname *uint8) (proc uintptr, err
> Errno)
>
> Index: src/pkg/syscall/syscall_plan9.go
> ===================================================================
> --- a/src/pkg/syscall/syscall_plan9.go
> +++ b/src/pkg/syscall/syscall_plan9.go
> @@ -51,9 +51,24 @@
>  // creation of IPv6 sockets to return EAFNOSUPPORT.
>  var SocketDisableIPv6 bool
>
> +// Pointers passed to syscalls must not escape (be accessed by OS after
> the syscall returns).
> +// For heap objects this will break when/if we have moving GC.
> +// And for other objects (global, C allocated) go:noescape has no effect.
> +
> +//go:noescape
> +
>  func Syscall(trap, a1, a2, a3 uintptr) (r1, r2 uintptr, err ErrorString)
> +
> +//go:noescape
> +
>  func Syscall6(trap, a1, a2, a3, a4, a5, a6 uintptr) (r1, r2 uintptr, err
> ErrorString)
> +
> +//go:noescape
> +
>  func RawSyscall(trap, a1, a2, a3 uintptr) (r1, r2, err uintptr)
> +
> +//go:noescape
> +
>  func RawSyscall6(trap, a1, a2, a3, a4, a5, a6 uintptr) (r1, r2, err
> uintptr)
>
>  func atoi(b []byte) (n uint) {
> Index: src/pkg/syscall/syscall_unix.go
> ===================================================================
> --- a/src/pkg/syscall/syscall_unix.go
> +++ b/src/pkg/syscall/syscall_unix.go
> @@ -23,9 +23,24 @@
>         netbsd32Bit = runtime.GOOS == "netbsd" && sizeofPtr == 4
>  )
>
> +// Pointers passed to syscalls must not escape (be accessed by OS after
> the syscall returns).
> +// For heap objects this will break when/if we have moving GC.
> +// And for other objects (global, C allocated) go:noescape has no effect.
> +
> +//go:noescape
> +
>  func Syscall(trap, a1, a2, a3 uintptr) (r1, r2 uintptr, err Errno)
> +
> +//go:noescape
> +
>  func Syscall6(trap, a1, a2, a3, a4, a5, a6 uintptr) (r1, r2 uintptr, err
> Errno)
> +
> +//go:noescape
> +
>  func RawSyscall(trap, a1, a2, a3 uintptr) (r1, r2 uintptr, err Errno)
> +
> +//go:noescape
> +
>  func RawSyscall6(trap, a1, a2, a3, a4, a5, a6 uintptr) (r1, r2 uintptr,
> err Errno)
>
>  // Mmap manager, for use by operating system-specific implementations.
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
>

[dvyukov, 2014-01-17 14:54:48]

On 2014/01/02 11:57:28, mikio wrote:
> Also please don't forget socketcall/rawsocketcall on linux/386.


Done

[dvyukov, 2014-01-17 16:18:44]

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

syscall: mark arguments to Syscall as noescape
Heap arguments to "async" syscalls will break when/if we have moving GC anyway.
With this change is must not break until moving GC, because a user must
reference the object in Go to preserve liveness. Otherwise the code is broken
already.
Reduces number of leaked params from 125 to 36 on linux.

R=golang-codereviews, mikioh.mikioh, bradfitz
CC=cshapiro, golang-codereviews, khr, rsc
https://codereview.appspot.com/45930043

[rsc, 2014-01-17 21:12:27]

Please do not submit compiler CLs without an LGTM from someone who works on the
compiler. This change creeps me out, even though it does appear to be working.
If you need a review, please ping the thread.

[dvyukov, 2014-01-18 15:25:13]

This actually reduced amount of allocated memory by garbage benchmark by 2.10%
http://goperfd.appspot.com/perfgraph?builder=windows-amd64&benchmark=garbage&...
due to syscall. FindFirstFile/FindNextFile.

So generally it would be a useful optimization (even if implemented
differently).

[rsc, 2014-01-21 18:32:04]

I rolled back this CL. I submitted CL 53940043 instead.

Russ