code review 153110044: spec: updated package unsafe

spec: updated package unsafe

This is a proposal for changes to package unsafe to
make its operations safer under the new garbage collector.
Based on discussions w/ khr, issue 7192, and borrowing
from a proposal by mdempsky (see issue 7192).

Summary of the changes:

1) It is not permitted anymore to convert a uintptr
   to an unsafe.Pointer.

2) There are two new functions, unsafe.Add and Unsafe.Mask
   implementing addition to and masking of pointers.

Comments:

- The 2nd function is called "Mask" rather than say "And"
  because "Add" and "And" can be easily confused.

- Add and Mask take an arbitrary pointer and return a pointer
  of the same type rather than only unsafe.Pointers. There is
  no technical reason for not handling arbitrary pointer types,
  and the hope is that this should reduce conversion clutter in
  the source.

- There's no urgency in implementing this for 1.4 (albeit it
  should be trivial in a compiler). But we probably want to
  document it for 1.4.

May fix issue 7192.

[gri, 2014-10-06 22:39:18]

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

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

[r, 2014-10-06 22:45:21]

An easier place to think about this would be the package docs for unsafe.

On Mon, Oct 6, 2014 at 3:39 PM,  <gri@golang.org> wrote:
> Reviewers: r, rsc, iant, ken2, khr,
>
> Message:
> Hello r@golang.org, rsc@golang.org, iant@golang.org, ken@golang.org,
> khr@golang.org (cc: golang-codereviews@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> spec: updated package unsafe
>
> This is a proposal for changes to package unsafe to
> make its operations safer under the new garbage collector.
> Based on discussions w/ khr, issue 7192, and borrowing
> from a proposal by mdempsky (see issue 7192).
>
> Summary of the changes:
>
> 1) It is not permitted anymore to convert a uintptr
>    to an unsafe.Pointer.
>
> 2) There are two new functions, unsafe.Add and Unsafe.Mask
>    implementing addition to and masking of pointers.
>
> Comments:
>
> - The 2nd function is called "Mask" rather than say "And"
>   because "Add" and "And" can be easily confused.
>
> - Add and Mask take an arbitrary pointer and return a pointer
>   of the same type rather than only unsafe.Pointers. There is
>   no technical reason for not handling arbitrary pointer types,
>   and the hope is that this should reduce conversion clutter in
>   the source.
>
> - There's no urgency in implementing this for 1.4 (albeit it
>   should be trivial in a compiler). But we probably want to
>   document it for 1.4.
>
> May fix issue 7192.
>
> Please review this at https://codereview.appspot.com/153110044/
>
> Affected files (+37, -16 lines):
>   M doc/go_spec.html
>
>
> Index: doc/go_spec.html
> ===================================================================
> --- a/doc/go_spec.html
> +++ b/doc/go_spec.html
> @@ -1,6 +1,6 @@
>  <!--{
>         "Title": "The Go Programming Language Specification",
> -       "Subtitle": "Version of September 30, 2014",
> +       "Subtitle": "Version of October 6, 2014",
>         "Path": "/ref/spec"
>  }-->
>
> @@ -6161,32 +6161,51 @@
>  <pre class="grammar">
>  package unsafe
>
> -type ArbitraryType int  // shorthand for an arbitrary Go type; it is not a
> real type
> -type Pointer *ArbitraryType
> -
> -func Alignof(variable ArbitraryType) uintptr
> -func Offsetof(selector ArbitraryType) uintptr
> -func Sizeof(variable ArbitraryType) uintptr
> -</pre>
> -
> -<p>
> -Any pointer or value of <a href="#Types">underlying type</a>
> <code>uintptr</code> can be converted to
> -a <code>Pointer</code> type and vice versa.
> +type arbitraryType int  // shorthand for an arbitrary Go type; it is not a
> real type
> +type Pointer *arbitraryType
> +
> +type arbitraryInt int   // shorthand for an arbitrary Go integer type; it's
> not a real type
> +func Add(pointer *arbitraryType, delta arbitraryInt) *arbitraryType
> +func Mask(pointer *arbitraryType, mask arbitraryInt) *arbitraryType
> +
> +func Alignof(variable arbitraryType) uintptr
> +func Offsetof(selector arbitraryType) uintptr
> +func Sizeof(variable arbitraryType) uintptr
> +</pre>
> +
> +<p>
> +Any pointer can be <a href="#Conversions">converted</a> to an
> <code>Pointer</code> type and vice versa;
> +and any <code>Pointer</code> value can be converted to a type with
> underlying type <code>uintptr</code>.
>  A <code>Pointer</code> is a <a href="#Pointer_types">pointer type</a> but a
> <code>Pointer</code>
>  value may not be <a href="#Address_operators">dereferenced</a>.
>  </p>
>
>  <pre>
>  var f float64
> -bits = *(*uint64)(unsafe.Pointer(&amp;f))
> +bits := *(*uint64)(unsafe.Pointer(&amp;f))
>
>  type ptr unsafe.Pointer
> -bits = *(*uint64)(ptr(&amp;f))
> +bits := *(*uint64)(ptr(&amp;f))
> +
> +type address uintptr
> +addr := address(ptr(&amp;f))
>
>  var p ptr = nil
>  </pre>
>
>  <p>
> +The functions <code>Add</code> and <code>Mask</code> implement unsafe
> pointer arithmetic.
> +Both take a pointer <code>p</code> of any pointer type <code>T</code> and
> an integer value
> +<code>x</code>, and return a pointer of the same type <code>T</code>. They
> are defined via
> +the following relations:
> +</p>
> +
> +<pre>
> +uintptr(unsafe.Pointer(unsafe.Add(p, x)))  == uintptr(unsafe.Pointer(p)) +
> uintptr(x)
> +unitptr(unsafe.Pointer(unsafe.Mask(p, x))) == uintptr(unsafe.Pointer(p)) &
> uintptr(x)
> +</pre>
> +
> +<p>
>  The functions <code>Alignof</code> and <code>Sizeof</code> take an
> expression <code>x</code>
>  of any type and return the alignment or size, respectively, of a
> hypothetical variable <code>v</code>
>  as if <code>v</code> was declared via <code>var v = x</code>.
> @@ -6201,7 +6220,7 @@
>  </p>
>
>  <pre>
> -uintptr(unsafe.Pointer(&amp;s)) + unsafe.Offsetof(s.f) ==
> uintptr(unsafe.Pointer(&amp;s.f))
> +unsafe.Add(&amp;s, unsafe.Offsetof(s.f)) == &amp;s.f
>  </pre>
>
>  <p>
> @@ -6214,7 +6233,8 @@
>  </p>
>
>  <pre>
> -uintptr(unsafe.Pointer(&amp;x)) % unsafe.Alignof(x) == 0
> +unsafe.Mask(&amp;x, unsafe.Alignof(x)-1)            == 0  // if alignment
> is a power of 2
> +uintptr(unsafe.Pointer(&amp;x)) % unsafe.Alignof(x) == 0  // in general
>  </pre>
>
>  <p>
> @@ -6222,6 +6242,7 @@
>  <code>Sizeof</code> are compile-time constant expressions of type
> <code>uintptr</code>.
>  </p>
>
> +
>  <h3 id="Size_and_alignment_guarantees">Size and alignment guarantees</h3>
>
>  <p>
>
>

[mdempsky, 2014-10-06 23:18:14]

On 2014/10/06 22:39:18, gri wrote:
> - Add and Mask take an arbitrary pointer and return a pointer
>   of the same type rather than only unsafe.Pointers. There is
>   no technical reason for not handling arbitrary pointer types,
>   and the hope is that this should reduce conversion clutter in
>   the source.

It could be worthwhile to restrict at least unsafe.Add to unsafe.Pointer to
avoid confusion for people familiar with C pointer arithmetic.  E.g., given "var
p *int32" it seems easy to think "unsafe.Add(p, 42)" will advance by 42 int32s
instead of 42 bytes, whereas that seems less likely if you had to write
"(*int32)(unsafe.Add(unsafe.Pointer(p), 42))".

[gri, 2014-10-06 23:21:08]

On 2014/10/06 23:18:14, mdempsky wrote:
> On 2014/10/06 22:39:18, gri wrote:
> > - Add and Mask take an arbitrary pointer and return a pointer
> >   of the same type rather than only unsafe.Pointers. There is
> >   no technical reason for not handling arbitrary pointer types,
> >   and the hope is that this should reduce conversion clutter in
> >   the source.
> 
> It could be worthwhile to restrict at least unsafe.Add to unsafe.Pointer to
> avoid confusion for people familiar with C pointer arithmetic.  E.g., given
"var
> p *int32" it seems easy to think "unsafe.Add(p, 42)" will advance by 42 int32s
> instead of 42 bytes, whereas that seems less likely if you had to write
> "(*int32)(unsafe.Add(unsafe.Pointer(p), 42))".

Yes, I have been thinking about that. The hope though is really to reduce some
of the conversion clutter in code using unsafe which makes that code usually
pretty hard to read (and thus even more unsafe). I'd rather address this with
making this more explicit in the documentation.

But let's see what others think.

[czaplinski, 2014-10-07 13:43:58]

On 2014/10/06 23:21:08, gri wrote:
> On 2014/10/06 23:18:14, mdempsky wrote:
> > It could be worthwhile to [...]
> > avoid confusion for people familiar with C pointer arithmetic.  E.g., given
> "var
> > p *int32" it seems easy to think "unsafe.Add(p, 42)" will advance by 42
int32s
> > instead of 42 bytes [...]
> 
> Yes, I have been thinking about that. The hope though is really to reduce some
> of the conversion clutter in code using unsafe which makes that code usually
> pretty hard to read (and thus even more unsafe). I'd rather address this with
> making this more explicit in the documentation.

This could be possibly achieved also by using some other name; some alternative
proposals I can now think of: Offset, Shift, AddBytes

[czaplinski, 2014-10-07 13:44:44]

https://codereview.appspot.com/153110044/diff/60001/doc/go_spec.html
File doc/go_spec.html (right):

https://codereview.appspot.com/153110044/diff/60001/doc/go_spec.html#newcode6205
doc/go_spec.html:6205: unitptr(unsafe.Pointer(unsafe.Mask(p, x))) ==
uintptr(unsafe.Pointer(p)) & uintptr(x)
s/unitptr/uintptr/

https://codereview.appspot.com/153110044/diff/60001/doc/go_spec.html#newcode6236
doc/go_spec.html:6236: unsafe.Mask(&x, unsafe.Alignof(x)-1)            ==
nil  // if alignment is a power of 2
The comment in the line above seems to me to hint at a disadvantage of
unsafe.Mask: that it is useless if "alignment is not a power of 2", and renders
any code using it incompatible with such architectures.

Also, I'm not sure what are all the expected use cases for unsafe.Mask; it'd be
probably a good idea to spell them out for the discussion and API design. I'm
only aware of one, for which I'd actually propose instead something more akin to
"unsafe.Align", where:
uintptr(unsafe.Pointer(unsafe.Align(&x))) % unsafe.Alignof(x) == 0

[iant, 2014-10-07 14:05:25]

How can we reconcile a change like this with the Go 1 guarantee?  Our own code
converts unsafe.Pointer to uintptr in many places.  It's impossible to use
syscall.Syscall at all without converting from unsafe.Pointer to uintptr.

[gri, 2014-10-07 15:55:25]

It's ok to convert from unsafe.Pointer to uintptr; it's just not ok to
convert back. Once "pointerness" has been lost (via conversion to uintptr),
the compiler can't track the value as a pointer anymore at which point it's
lost with respect to GC.

The new runtime code already uses a form of "Add" internally.

The Go 1 guarantee doesn't make an exception for package unsafe; this
proposal suggest that we do. It seems much more error-prone and fragile to
leave package unsafe as is, and instead specify the expression patterns
that are guaranteed to work, such as:

   (unsafe.Pointer(uintptr(unsafe.Pointer(p)) + delta)

Again, this is just a starting point for a discussion.

- gri

On Tue, Oct 7, 2014 at 7:05 AM, <iant@golang.org> wrote:

> How can we reconcile a change like this with the Go 1 guarantee?  Our
> own code converts unsafe.Pointer to uintptr in many places.  It's
> impossible to use syscall.Syscall at all without converting from
> unsafe.Pointer to uintptr.
>
>
> https://codereview.appspot.com/153110044/
>

[r, 2014-10-07 16:07:31]

Please file a ticket. We can return to this topic in the 1.5 cycle.

-rob


On Tue, Oct 7, 2014 at 8:55 AM, Robert Griesemer <gri@golang.org> wrote:
> It's ok to convert from unsafe.Pointer to uintptr; it's just not ok to
> convert back. Once "pointerness" has been lost (via conversion to uintptr),
> the compiler can't track the value as a pointer anymore at which point it's
> lost with respect to GC.
>
> The new runtime code already uses a form of "Add" internally.
>
> The Go 1 guarantee doesn't make an exception for package unsafe; this
> proposal suggest that we do. It seems much more error-prone and fragile to
> leave package unsafe as is, and instead specify the expression patterns that
> are guaranteed to work, such as:
>
>    (unsafe.Pointer(uintptr(unsafe.Pointer(p)) + delta)
>
> Again, this is just a starting point for a discussion.
>
> - gri
>
> On Tue, Oct 7, 2014 at 7:05 AM, <iant@golang.org> wrote:
>>
>> How can we reconcile a change like this with the Go 1 guarantee?  Our
>> own code converts unsafe.Pointer to uintptr in many places.  It's
>> impossible to use syscall.Syscall at all without converting from
>> unsafe.Pointer to uintptr.
>>
>>
>> https://codereview.appspot.com/153110044/
>
>

[gri, 2014-10-07 17:01:30]

There's an issue regarding documentation. Happy to file a ticket for this
as well, but it seems that we should tackle this rather sooner than later
given the requirements going forward with the new GC.
- gri

On Tue, Oct 7, 2014 at 9:07 AM, Rob Pike <r@golang.org> wrote:

> Please file a ticket. We can return to this topic in the 1.5 cycle.
>
> -rob
>
>
> On Tue, Oct 7, 2014 at 8:55 AM, Robert Griesemer <gri@golang.org> wrote:
> > It's ok to convert from unsafe.Pointer to uintptr; it's just not ok to
> > convert back. Once "pointerness" has been lost (via conversion to
> uintptr),
> > the compiler can't track the value as a pointer anymore at which point
> it's
> > lost with respect to GC.
> >
> > The new runtime code already uses a form of "Add" internally.
> >
> > The Go 1 guarantee doesn't make an exception for package unsafe; this
> > proposal suggest that we do. It seems much more error-prone and fragile
> to
> > leave package unsafe as is, and instead specify the expression patterns
> that
> > are guaranteed to work, such as:
> >
> >    (unsafe.Pointer(uintptr(unsafe.Pointer(p)) + delta)
> >
> > Again, this is just a starting point for a discussion.
> >
> > - gri
> >
> > On Tue, Oct 7, 2014 at 7:05 AM, <iant@golang.org> wrote:
> >>
> >> How can we reconcile a change like this with the Go 1 guarantee?  Our
> >> own code converts unsafe.Pointer to uintptr in many places.  It's
> >> impossible to use syscall.Syscall at all without converting from
> >> unsafe.Pointer to uintptr.
> >>
> >>
> >> https://codereview.appspot.com/153110044/
> >
> >
>

[gri, 2014-10-07 18:44:53]

https://codereview.appspot.com/153110044/diff/60001/doc/go_spec.html
File doc/go_spec.html (right):

https://codereview.appspot.com/153110044/diff/60001/doc/go_spec.html#newcode6205
doc/go_spec.html:6205: unitptr(unsafe.Pointer(unsafe.Mask(p, x))) ==
uintptr(unsafe.Pointer(p)) & uintptr(x)
On 2014/10/07 13:44:43, czaplinski wrote:
> s/unitptr/uintptr/

Done.

https://codereview.appspot.com/153110044/diff/60001/doc/go_spec.html#newcode6236
doc/go_spec.html:6236: unsafe.Mask(&x, unsafe.Alignof(x)-1)            ==
nil  // if alignment is a power of 2
On 2014/10/07 13:44:43, czaplinski wrote:
> The comment in the line above seems to me to hint at a disadvantage of
> unsafe.Mask: that it is useless if "alignment is not a power of 2", and
renders
> any code using it incompatible with such architectures.
> 
> Also, I'm not sure what are all the expected use cases for unsafe.Mask; it'd
be
> probably a good idea to spell them out for the discussion and API design. I'm
> only aware of one, for which I'd actually propose instead something more akin
to
> "unsafe.Align", where:
> uintptr(unsafe.Pointer(unsafe.Align(&x))) % unsafe.Alignof(x) == 0

There are a few common operations one may want to do with pointers p:

p + delta
p - delta
p & mask
p | mask

The first 2 are covered by And, the 3rd is covered by Mask (which is called Mask
only because And is too close to Add in name). The 4th seems more rare and could
be added if necessary. But it's also covered by Mask followed by Add.

[minux, 2014-10-07 19:36:00]

On Oct 7, 2014 10:05 AM, <iant@golang.org> wrote:
> How can we reconcile a change like this with the Go 1 guarantee?  Our
> own code converts unsafe.Pointer to uintptr in many places.  It's
> impossible to use syscall.Syscall at all without converting from
> unsafe.Pointer to uintptr.
I agree.

this sets a precedence of changing the language to match a specific
(runtime) implementation.

I suggest we don't go there.

it's fine to say that certain operations are not safe for the gc toolchain
anymore in the release note and make govet issue errors for them, but it's
entirely different to just forbiding casting uintptr to unsafe.Pointer.

This IS a language change, and it's definitely not covered in Go 1 gurantee
exemption about bugs.
For example, if a Go implementation uses conservative GC, then there is no
need to forbid the cast and FWIW the spec doesn't say anything about the
requirements of the GC, only that it exists in some form. But this change
will make Go code targeting those valid Go implementations invalid. Why?
Please note that the spec doesn't preclude conservative GC implementations
and before 1.4, the gc implementation isn't fully precise.

the exact behavior of GC is unspecified in the spec, so any interaction is
implementation-defined.

To put it another way, if we apply this change, we should as well also add
that a compliant Go implementation must use a fully precise GC.

[rsc, 2014-10-07 19:39:59]

I don't believe we should be defining new API at this point in the cycle,
especially if the compiler's not going to implement it but even if it were.

[minux, 2014-10-07 19:42:21]

On Oct 7, 2014 11:55 AM, "Robert Griesemer" <gri@golang.org> wrote:
> The Go 1 guarantee doesn't make an exception for package unsafe; this
proposal suggest that we do.
interaction with GC is already implementation defined. we don't need to
change the language to achieve the goal. Think about other implementations
which might still be using conservative GCs.

plus, this proposal essentially (perhaps unintentionally) make it easier to
write unsafe code in Go. That seems against the philosophy of Go, IMHO.

[gri, 2014-10-08 19:35:29]

Abandoning this approach in favor of documenting status quo; per feedback from
internal team discussion.

[gri, 2014-10-08 19:35:46]

*** Abandoned ***