Go: reflect: add func (Value) UnsafePointer() unsafe.Pointer

[mdempsky]

[Splitting out separately from [my comment](https://github.com/golang/go/issues/40481#issuecomment-665857684) on #40481, per @rsc's [suggestion](https://github.com/golang/go/issues/40481#issuecomment-669316065).]

This proposal is to add a method

func (v Value) UnsafePointer() unsafe.Pointer { return unsafe.Pointer(v.Pointer()) }

This would allow callers to migrate from Value.Addr and Value.Pointer so that the later could be deprecated (fulfilling their TODOs [1], [2]), and eliminating a class of possible misuse of unsafe.Pointer.

Optionally, we can then also have go fix rewrite:

1. unsafe.Pointer(v.Pointer()) -> v.UnsafePointer()
2. unsafe.Pointer(v.UnsafeAddr()) -> v.Addr().UnsafePointer()
3. v.Pointer() -> uintptr(v.UnsafePointer())
4. v.UnsafeAddr() -> uintptr(v.Addr().UnsafePointer())

Replacing v.UnsafeAddr() with v.Addr().UnsafePointer() has a slight added cost v.UnsafeAddr() was able to simply directly return the address pointer, while Addr() potentially has to compute the resulting pointer's type. However, (*rtype).ptrTo already uses caching, and the compiler also already eagerly generates *T types for most types, so I expect in practice this to have negligible overhead.

--

Tangentially, the InterfaceData method also has a TODO to deprecate. I don't think there's anything preventing this from being deprecated today. Callers should probably just use Elem() to get a Value representing the concrete value, and then do something based on its Kind().

Within Google's internal Go code base, the only two uses of reflect.Value.InterfaceData appear to be:

1. https://github.com/hashicorp/terraform/blob/master/dag/marshal.go#L205, which looks like dead code anyway: reflect.ValueOf(...).Kind() never returns reflect.Interface.

2. https://fuchsia.googlesource.com/fuchsia.git/+/refs/heads/master/src/connectivity/network/netstack/socket_encode.go#41, which needs to be fixed anyway since it's using reflect.SliceHeader incorrectly too.

[ianlancetaylor]

My recollection is that one of the reasons that Value.Pointer returns uintptr is that we wanted to prevent code like

p := v.Pointer()

from giving p the type unsafe.Pointer in a package that does not import unsafe. Of course you can do this already using your own packages, but we didn't want to encourage it in the standard library.

(This also gets us to a discussion about type based alias analysis. In ordinary Go code a pointer to *byte and a pointer to *int must always point to different locations, so changing memory through one pointer can't affect memory changed through the other pointer. But that statement is no longer true if either value was set via a conversion through unsafe.Pointer. Does this mean that the compiler can never assume that they point to different locations, or can we say that that assumption changes based on whether the package imports unsafe?)

[bcmills]

Re type-based alias analysis: even if the package does not import unsafe we can only assume that variables do not alias if they were allocated from within the package. (A caller can always supply pre-punned pointers as arguments.)

So I think if we want to do any deep reasoning about aliasing we would generally need whole-program analysis either way.

[mdempsky]

My recollection is that one of the reasons that Value.Pointer returns uintptr is that we wanted to prevent code like p := v.Pointer() from giving p the type unsafe.Pointer in a package that does not import unsafe.

Yes, that reason was justifiable back when Go supported "safe" mode, but we removed that back in 2018. There are many better ways to run untrusted Go programs today (e.g., within a process sandbox like gVisor or by compiling them to wasm).

This also gets us to a discussion about type based alias analysis.

This seems like an orthogonal concern to me. The only way to safely use reflect.Value.Pointer and reflect.Value.UnsafeAddr as pointer values today is to immediately convert them to unsafe.Pointer anyway.

[ianlancetaylor]

It's somewhat orthogonal but perhaps not entirely negligible: see #26070.

[mdempsky]

I don't see any mention of type-based alias analysis in #26070. Can you elaborate on how they're related?

[ianlancetaylor]

The connection I see here is what should happen when people write p := v.UnsafePointer() in a package that doesn't import unsafe, if we adopt #26070. Will it be confusing that they can use := to get an unsafe.Pointer that they can't use?

Perhaps this concern is not important.

[mdempsky]

I see. I wrote "This seems like an orthogonal concern to me," with "this" referring to type-based alias analysis. When you wrote "It's somewhat orthogonal," I thought you were still talking about type-based alias analysis too.

Assuming both #26070 and this proposal were accepted, I expect users would adapt by writing import _ "unsafe" // for unsafe.Pointer conversion analogous to how they write import _ "unsafe" // for go:linkname today. (Though as I've commented on #26070, I disagree with that proposal.)

[rsc]

26070 was retracted.

As I remember it, we were picky about importing "unsafe" to get an unsafe.Pointer (and not providing ways to get one otherwise) for two reasons:

1. It's good practice to call out the unsafe parts of the code clearly, and the import does that.
2. App Engine needed to be kept from using unsafe.

(2) is no longer a concern, and (1) seems well enough handled by the name v.UnsafePointer, which seems just as clearly "unsafe" as import "unsafe".

So overall this seems like a fine change. It might be worth thinking about for Go 1.17 along with the other unsafe/pointer cleanup.

[rsc]

Based on the discussion above, this seems like a likely accept (for Go 1.17).

[rsc]

No change in consensus, so accepted.