code review 55410043: syscall: use unsafe.Pointer instead of uintptr in places

syscall: use unsafe.Pointer instead of uintptr in net syscalls

In particular: setsockopt, getsockopt, bind, connect.

There are probably more.

All platforms cross-compile with make.bash, and all.bash still
pases on linux/amd64.

Update Issue 7169

[rsc, 2014-01-22 01:56:34]

This looks good, thanks for starting it. If it turns out we need changes to
public syscall API let's do those separately. That is, let's keep this CL just
about changes to internal (unexported) API.

https://codereview.appspot.com/55410043/diff/20001/src/pkg/syscall/syscall.go
File src/pkg/syscall/syscall.go (right):

https://codereview.appspot.com/55410043/diff/20001/src/pkg/syscall/syscall.go...
src/pkg/syscall/syscall.go:69: var zeroPtr = unsafe.Pointer(uintptr(0))
does 'var zeroPtr unsafe.Pointer' not work?

[bradfitz, 2014-01-22 01:58:28]

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

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

[bradfitz, 2014-01-22 01:59:16]

PTAL

All BSDs, Windows, Linux, and Darwin should work now.

Will rebuild them all to verify.

https://codereview.appspot.com/55410043/diff/20001/src/pkg/syscall/syscall.go
File src/pkg/syscall/syscall.go (right):

https://codereview.appspot.com/55410043/diff/20001/src/pkg/syscall/syscall.go...
src/pkg/syscall/syscall.go:69: var zeroPtr = unsafe.Pointer(uintptr(0))
On 2014/01/22 01:56:34, rsc wrote:
> does 'var zeroPtr unsafe.Pointer' not work?

Whoops. Just copy/paste mistake.

[bradfitz, 2014-01-22 01:59:37]

(No public API change was necessary)

[rsc, 2014-01-22 02:10:15]

Actually, I thought you were going to take the address of zeroPtr, but it
looks like you just need a value. If you just need a value, use 'nil'.

This is a good change and should be done regardless, but I don't think it
explains any of the failures on issue 7159 - all of them are in the reflect
test, and I don't believe that test uses networking. Is there a direct
connection I am missing?

[rsc, 2014-01-22 02:11:18]

LGTM after s/zeroPtr/nil/

[bradfitz, 2014-01-22 02:12:58]

On Tue, Jan 21, 2014 at 6:10 PM, Russ Cox <rsc@golang.org> wrote:

> Actually, I thought you were going to take the address of zeroPtr, but it
> looks like you just need a value. If you just need a value, use 'nil'.
>

Oh, right. I've never needed that.


> This is a good change and should be done regardless, but I don't think it
> explains any of the failures on issue 7159 - all of them are in the reflect
> test, and I don't believe that test uses networking. Is there a direct
> connection I am missing?
>

Maybe I got the wrong bug? I was looking for the one where Dmitry noted
Dragonfly's crash.

[rsc, 2014-01-22 02:18:21]

Dmitriy mentioned setsockopt on issue 7159 but every log in the issue is a
crash during the reflect test.

[bradfitz, 2014-01-22 02:54:51]

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

syscall: use unsafe.Pointer instead of uintptr in net syscalls

In particular: setsockopt, getsockopt, bind, connect.

There are probably more.

All platforms cross-compile with make.bash, and all.bash still
pases on linux/amd64.

Update Issue 7169

R=rsc
CC=golang-codereviews
https://codereview.appspot.com/55410043

[ebfe, 2014-01-22 07:36:16]

Hi,

this CL doesn't touch freebsd/arm and netbsd/arm. Is this intentional?

[bradfitz, 2014-01-22 08:09:35]

My build tool didn't include them because they weren't on the build
dashboard and that's how I built my list.

My later CL with a revised tool updated them.
On Jan 21, 2014 11:36 PM, <gnirheg.leahcim@gmail.com> wrote:

> Hi,
>
> this CL doesn't touch freebsd/arm and netbsd/arm. Is this intentional?
>
> https://codereview.appspot.com/55410043/
>

[minux1, 2014-01-23 06:46:57]

https://codereview.appspot.com/55410043/diff/60001/src/pkg/syscall/lsf_linux.go
File src/pkg/syscall/lsf_linux.go (right):

https://codereview.appspot.com/55410043/diff/60001/src/pkg/syscall/lsf_linux....
src/pkg/syscall/lsf_linux.go:72: return setsockopt(fd, SOL_SOCKET,
SO_ATTACH_FILTER, unsafe.Pointer(&p), unsafe.Sizeof(p))
perhaps I'm missing something, but I want to understand why we must use
unsafe.Pointer here?

setsockopt won't retain the pointer, and we can agree that it's a problem only
when p is allocated on heap.

however, p is also in the local stack frame of AttachLsf before setsockopt is
returned, so when can the garbage collector collect it before setsockopt
returns?

Or essentially, I don't think the GC could collect p in this function before the
return of f:
func f() int {
   var p BIgStruct // so big that it's allocated on the heap
   // a lot of code not using p
   r := g(uintptr(unsafe.Pointer(&p)))
   // could p be collected here? it's still in scope, so for the sake of
   // predictability, I'd rather believe that p won't be reclaimed here.
   return r
}

[bradfitz, 2014-01-23 14:51:41]

https://codereview.appspot.com/55410043/diff/60001/src/pkg/syscall/lsf_linux.go
File src/pkg/syscall/lsf_linux.go (right):

https://codereview.appspot.com/55410043/diff/60001/src/pkg/syscall/lsf_linux....
src/pkg/syscall/lsf_linux.go:72: return setsockopt(fd, SOL_SOCKET,
SO_ATTACH_FILTER, unsafe.Pointer(&p), unsafe.Sizeof(p))
On 2014/01/23 06:46:58, minux wrote:
> perhaps I'm missing something, but I want to understand why we must use
> unsafe.Pointer here?
> 
> setsockopt won't retain the pointer, and we can agree that it's a problem only
> when p is allocated on heap.
> 
> however, p is also in the local stack frame of AttachLsf before setsockopt is
> returned, so when can the garbage collector collect it before setsockopt
> returns?
> 
> Or essentially, I don't think the GC could collect p in this function before
the
> return of f:
> func f() int {
>    var p BIgStruct // so big that it's allocated on the heap
>    // a lot of code not using p
>    r := g(uintptr(unsafe.Pointer(&p)))
>    // could p be collected here? it's still in scope, so for the sake of
>    // predictability, I'd rather believe that p won't be reclaimed here.
>    return r
> }

This code had to change because setsockopt itself had to change.

But also: p being in scope doesn't mean it won't be collected.  There's a
difference between in scope and "live".

Imagine a goroutine working forever in a loop:

  go f.work()

  func (f *Foo) loop() {
       for {
           buf := make([]byte, 10 << 20)
           // ...something with buf....

           // ...now buf is garbage

           select {
           case <-time.After(5 * time.Minute):
           case <-wakeUpCh:
           }
       }
  }


That goroutine will spend most of its time in that select, waiting up to 5
minutes before doing the next round of work, but "buf" is always in-scope.  But
that's still a lot of memory that could be collected.

This sort of situation happens often.

[minux1, 2014-01-23 18:16:47]

On Thu, Jan 23, 2014 at 9:51 AM, <bradfitz@golang.org> wrote:

> This code had to change because setsockopt itself had to change.
>
Yes, I was asking the more generic case here.

>
> But also: p being in scope doesn't mean it won't be collected.  There's
> a difference between in scope and "live".
>
My thinking is that they probably should be the same or it will be very
confusing.

>
> Imagine a goroutine working forever in a loop:
>
>   go f.work()
>
>   func (f *Foo) loop() {
>        for {
>            buf := make([]byte, 10 << 20)
>            // ...something with buf....
>
>            // ...now buf is garbage
>
I think we're suppose to do
buf = nil
to actually release the backing array for buf. No?

Otherwise, buf's backing array will be eligible to be collected after
the result of another make() overwrites it.

>
>            select {
>            case <-time.After(5 * time.Minute):
>            case <-wakeUpCh:
>            }
>        }
>   }
>
>
> That goroutine will spend most of its time in that select, waiting up to
> 5 minutes before doing the next round of work, but "buf" is always
> in-scope.  But that's still a lot of memory that could be collected.
>
> This sort of situation happens often.
>

[bradfitz, 2014-01-23 18:37:21]

If one finds this confusing, one should not be using unsafe.

Without using unsafe, there is no change visible except that the GC works
better.
 On Jan 23, 2014 10:16 AM, "minux" <minux.ma@gmail.com> wrote:

>
> On Thu, Jan 23, 2014 at 9:51 AM, <bradfitz@golang.org> wrote:
>
>> This code had to change because setsockopt itself had to change.
>>
> Yes, I was asking the more generic case here.
>
>>
>> But also: p being in scope doesn't mean it won't be collected.  There's
>> a difference between in scope and "live".
>>
> My thinking is that they probably should be the same or it will be very
> confusing.
>
>>
>> Imagine a goroutine working forever in a loop:
>>
>>   go f.work()
>>
>>   func (f *Foo) loop() {
>>        for {
>>            buf := make([]byte, 10 << 20)
>>            // ...something with buf....
>>
>>            // ...now buf is garbage
>>
> I think we're suppose to do
> buf = nil
> to actually release the backing array for buf. No?
>
> Otherwise, buf's backing array will be eligible to be collected after
> the result of another make() overwrites it.
>
>>
>>            select {
>>            case <-time.After(5 * time.Minute):
>>            case <-wakeUpCh:
>>            }
>>        }
>>   }
>>
>>
>> That goroutine will spend most of its time in that select, waiting up to
>> 5 minutes before doing the next round of work, but "buf" is always
>> in-scope.  But that's still a lot of memory that could be collected.
>>
>> This sort of situation happens often.
>>
>
>

[rsc, 2014-01-23 20:04:27]

On Thu, Jan 23, 2014 at 1:16 PM, minux <minux.ma@gmail.com> wrote:

>
> On Thu, Jan 23, 2014 at 9:51 AM, <bradfitz@golang.org> wrote:
>
>> This code had to change because setsockopt itself had to change.
>>
> Yes, I was asking the more generic case here.
>
>>
>> But also: p being in scope doesn't mean it won't be collected.  There's
>> a difference between in scope and "live".
>>
> My thinking is that they probably should be the same or it will be very
> confusing.
>

They're just not. Forget about unsafe. If I have

foo, _ := ioutil.ReadFile("foo")
use(foo)
bar, _ := ioutil.ReadFile("bar")
use(bar)
baz, _ := ioutil.ReadFile("baz")
use(baz)

there is no reason that foo, bar, and baz should not all use the same
storage to hold the file contents. Introducing artificial restrictions like
tying them to scope just creates leaks (and worse it assigns semantic
meaning to { }, which triggers the nonsense people write for RAII in C++).

Russ