spec: write rules for safe use of unsafe.Pointer and uintptr

[ianlancetaylor]

People are confused about when it is safe to convert an unsafe.Pointer to a uintptr.  If
the unsafe.Pointer is the only pointer to the object, when is the GC permitted to
collect it?  We need to write down some rules for constructs that are always safe.

It seems to me that we must permit
    unsafe.Pointer(uintptr(p) + offset)

It also seems to me that we must permit passing unsafe.Pointer values converted to
uintptr to syscall.Syscall.
    syscall.Syscall(SYS_OPEN, uintptr(unsafe.Pointer(StringBytePtr(n))))

If these sorts of constructs will not work safely, then we should consider disallowing
them.

And in general we must write down what is permitted.

[davecheney]

Comment 1:

Thank you for raising this issue Ian.

[rsc]

Comment 3:

Labels changed: added release-go1.4, removed release-go1.3.

[mdempsky]

Comment 4:

Somewhat relatedly, the Go 1.3 release notes warn "Programs that use package unsafe to
store pointers in integer-typed values are also illegal but more difficult to diagnose
during execution."  Strictly interpreting this wording seems to imply even the
aforementioned "unsafe.Pointer(uintptr(p) + offset)" is illegal as the pointer value p
will be converted ("stored") to an integer-typed value during its evaluation.
Arguably saying "integer-typed variables" would be more appropriate (though see also
issue #8496 for issues with the term "variable").
Incidentally/alternatively, it might be nice if it was possible to allow some pointer
arithmetic directly on unsafe.Pointer values rather than always requiring them to be
converted to uintptr and back.  E.g., given "var p *int" and trying to get a pointer to
the byte that follows, it's quite tedious to need to write:
    q := (*byte)(unsafe.Pointer(uintptr(unsafe.Pointer(p)) + unsafe.Sizeof(*p)))
Whereas it would be a bit easier to read (IMO) as:
    q := (*byte)(unsafe.Pointer(p) + unsafe.Sizeof(*p))
and the GC safety implications would be clearer too I think.
As for syscall.Syscall(), one option might be to change "func Syscall(trap, a1, a2, a3
uintptr)" to "func Syscall(trap uintptr, a1, a2, a3 interface{})" and make callers
responsible for passing pointers instead of integers when they point to memory that
needs to be kept alive.
Unfortunately, that's not 100% Go1 compatible because (assuming 32-bit) the untyped
constant 1<<31 is assignable to uintptr but not interface{} (because the Go spec
says it should be converted to int first), so it might be necessary to instead provide a
new set of SyscallX() functions that can accept pointer values via interface{}
parameters and require users to switch.

[mdempsky]

Comment 5:

I've put together a proposal for unsafe.Pointer arithmetic at
https://docs.google.com/document/d/1yyCMzE4YPfsXvnZNjhszaYNqavxHhvbY-OWPqdzZK30/pub.

[griesemer]

Comment 6:

Here's a proposal based on the input from above: https://golang.org/cl/153110044

Owner changed to @griesemer.

Status changed to Started.

[gopherbot]

Comment 7:

CL https://golang.org/cl/153110044 mentions this issue.

[gopherbot]

Comment 8:

CL https://golang.org/cl/163050043 mentions this issue.

[griesemer]

Comment 9:

This issue was closed by revision 5361b74.

Status changed to Fixed.

[alexbrainman]

Comment 10:

> This issue was closed by revision 5db49b99612c.
> Status: Fixed 
I don't see how this fixes original issue - "write rules for safe use of unsafe.Pointer
and uintptr". The CL just adds: "... is implementation-defined". And that is fair
enough. But we still need to document somewhere what these rules are, even if they can
change in the future. We have plenty of code that uses unsafe.Pointer inside main repo -
who is to say if that code is correct. What about outside external packages? If you
don't have answers to this questions, that is OK too. But lets reopen the issue.
Thank you.
Alex

[griesemer]

Comment 11:

Let's not re-open this issue. Here's a comment from rsc:
"go vet has a check for what I believe we should support long term. The Go 1.3 release
notes very nearly promised this (http://golang.org/doc/go1.3#garbage_collector). The
cases it allows are (given p unsafe.Pointer, v reflect.Value, h *reflect.SliceHeader or
*reflect.StringHeader):
unsafe.Pointer(uintptr(p) +/- delta))
unsafe.Pointer(uintptr(p))
unsafe.Pointer(v.Pointer())
unsafe.Pointer(v.UnsafeAddr())
unsafe.Pointer(h.Data)
The header variable h must be a pointer to an actual slice or string values; you cannot
just say 'var h reflect.SliceHeader' and use it."
The text in http://golang.org/doc/go1.3#garbage_collector has been updated and mentions
go vet as well. We may want to emphasize this again in the 1.4 release notes.
I'm not against exact rules, but the spec is probably not where they should be.
Giving exact rules also is not very helpful unless mechanically checked, hence go vet.
I've tried to propose a modified unsafe package that would have reduced the "dangerous"
operations to one, but it's not really making a big difference and still requires manual
inspection (besides the general opposition to changing the API).

[mdempsky]

Comment 12:

As an aside, it seems worth pointing out that unsafe.Pointer(v.Pointer()) and
unsafe.Pointer(v.UnsafeAddr()) aren't 100% safe with gc, because if they're part of a
complex expression then gc spills them to uintptr-typed autotmp variables on the stack. 
E.g.,
    func foo(int) reflect.Value
    if unsafe.Pointer(foo(1).Pointer()) == unsafe.Pointer(foo(2).Pointer()) { ... }
is unsafe because gc compiles it as
    var autotmp1 uintptr = foo(1).Pointer()
    var autotmp2 uintptr = foo(2).Pointer()  // If foo(2) triggers a GC, autotmp1 might become invalid!
    if unsafe.Pointer(autotmp1) == unsafe.Pointer(autotmp2) { ... }
but "go vet" doesn't warn about this.

[alexbrainman]

Comment 13:

Thank you for explanation.
> I'm not against exact rules, but the spec is probably not where they should be.
I have nothing against not putting them into spec. But they should be somewhere. I (as a
user) should be able to find them when I need them.
> ... hence go vet.
"go vet" failed me just the other day. I had this issue
https://code.google.com/p/odbc/issues/detail?id=51, which, I believe, is gc related. But
"go vet" didn't complained about that:
C:\go\path\src\code.google.com\p\odbc>hg par
changeset:   37:78314be168c8
user:        Alex Brainman <[email protected]>
date:        Tue Sep 02 11:28:47 2014 +1000
summary:     odbc: better handling of string parameters
C:\go\path\src\code.google.com\p\odbc>go vet ./...
c:\go\path\src\code.google.com\p\odbc\column.go:103: unreachable code
c:\go\path\src\code.google.com\p\odbc\mssql_test.go:227: database/sql.NullString
composite literal uses unkeyed fields
c:\go\path\src\code.google.com\p\odbc\mssql_test.go:235: database/sql.NullString
composite literal uses unkeyed fields
c:\go\path\src\code.google.com\p\odbc\mssql_test.go:308: arg is.weight for printf verb
%d of wrong type: float64
exit status 1
Maybe I am wrong thinking that "go vet" should hold my hand here. If not "go vet", then
I want some rules I can base my decisions on. I was hopping using "unsafe" would be less
like black magic at this moment. And, I expect, things will change again in the near
future.
Alex

[ianlancetaylor]

Comment 14:

I agree with both of you: the spec is probably not the right place to write down these
rules, but the current rules must be written down somewhere.  I originally opened the
issue because the current state is confusing, and that remains the case.  I think we
should either reopen this issue as a non-spec issue, or open a new one.
This is related to issue #8310.

[griesemer]

Comment 15:

I opened a new issue #8994.

[alexbrainman]

Comment 16:

Sounds like a plan. Thank you, Ian.
Alex