proposal: Go 2: reflect: return opaque pointer instead of uintptr

[dsnet]

The Value.Pointer and Value.UnsafeAddr methods and the SliceHeader.Data and StringHeader.Data fields all use uintptr. This has some downsides:

• Exception in unsafe to special-case conversions:

Package reflect's Value methods named Pointer and UnsafeAddr return type uintptr instead of unsafe.Pointer to keep callers from changing the result to an arbitrary type without first importing "unsafe". However, this means that the result is fragile and must be converted to Pointer immediately after making the call, in the same expression.

• It is ill-suited for applications that only care about pointer comparability (don't care about pointer arithmetic and converting to a specific type). For example, the reflect.DeepEqual implementation imports unsafe in order to convert the uintptr to an unsafe.Pointer to have a pointer that the GC knows how to scan (or move if we ever have a moving GC).

In Go2, I propose that Value.UnsafeAddr and Value.Pointer methods return and the SliceHeader.Data and StringHeader.Data fields be a reflect.Pointer type:

package reflect

// Pointer is an opaque pointer and is guaranteed to be comparable.
type Pointer struct { p unsafe.Pointer }

func (p Pointer) SetInto(p2 *unsafe.Pointer) {
    *p2 = p.p
}

The SetInto method writes the pointer into p2 and requires the user to already have imported unsafe. It's a more clunky, but less error prone.

Alternatively, we could define unsafe.OpaquePointer that is safe to store and pass around, but does not permit arithmetic on it, nor can it be converted to a pointer of any arbitrary type. You can convert unsafe.OpaquePointer to unsafe.Pointer and vice versa. However, you cannot convert an uintptr to an unsafe.OpaquePointer as this would easily allow the creation of invalid pointers. This simplifies the warnings about uintptr not being stored in a variable, and must be within the same expression. To ensure usage of OpaquePointer does not require unsafe, we could type alias it from the reflect package.

Thus, rules 5 and 6 in unsafe can be entirely eliminated.

[cznic]

As just a first example I can recall: This will close the only legal way how to pass a static (Go text segment allocated) Go string to code automatically translated from C to use as its text segment without copying. Similar problems arise for the BSS and DS function just above TS.

[bcmills]

@cznic, could you elaborate? It seems like you could add one more method to @dsnet's proposal to cover that use-case, while still addressing his concerns about uintptr conversion and needless imports of unsafe.

That method would be a simple setter:

func (p *Pointer) Set(v unsafe.Pointer) {
	p.p = v
}

[cznic]

The discussed TS (for example) fucntion looks like this

// TS allocates the R/O text segment of a package/command.
func TS(init string) uintptr { return (*reflect.StringHeader)(unsafe.Pointer(&init)).Data }

For hello.c

#include <stdio.h>

int main() {
	printf("Hello World\n");
}

The translated Go code is currently

package main

import (
	"os"
	"unsafe"

	"github.com/cznic/crt"
)

var _ unsafe.Pointer

const null = uintptr(0)

func main() {
	psz := unsafe.Sizeof(uintptr(0))
	argv := crt.MustCalloc((len(os.Args) + 1) * int(psz))
	p := argv
	for _, v := range os.Args {
		*(*uintptr)(unsafe.Pointer(p)) = crt.CString(v)
		p += psz
	}
	a := os.Environ()
	env := crt.MustCalloc((len(a) + 1) * int(psz))
	p = env
	for _, v := range a {
		*(*uintptr)(unsafe.Pointer(p)) = crt.CString(v)
		p += psz
	}
	*(*uintptr)(unsafe.Pointer(Xenviron)) = env
	X_start(crt.NewTLS(), int32(len(os.Args)), argv)
}

// X_start is defined at crt0.c:7:6
func X_start(tls crt.TLS, _argc int32, _argv uintptr /* **int8 */) {
	crt.X__register_stdfiles(tls, Xstdin, Xstdout, Xstderr, Xenviron)
	crt.X__builtin_exit(tls, Xmain(tls, _argc, _argv))
}

// Xstdin *void, escapes: false, crt0.c:5:6
var Xstdin = X__stdfiles

// Xstdout *void, escapes: false, crt0.c:5:31
var Xstdout = X__stdfiles + 8

// Xstderr *void, escapes: false, crt0.c:5:57
var Xstderr = X__stdfiles + 16

// Xenviron **int8, escapes: true, crt0.c:4:6
var Xenviron = bss

// Xmain is defined at hello.c:3:5
func Xmain(tls crt.TLS, _ int32, _ uintptr /* **int8 */) (r int32) {
	crt.Xprintf(tls, ts /* "Hello World\n" */)
	return r
}

// X__stdfiles [3]uintptr, escapes: true, crt0.c:3:15
var X__stdfiles = bss + 8

var (
	bss     = crt.BSS(&bssInit[0])
	bssInit [32]byte
	ts      = crt.TS("Hello World\n\x00")
)

So a setter is not what's needed. TS uses/wants/needs the uintptr in reflect.StringHeader.Data. The proposal would make that impossible. If you've meant a getter instead, I'm afraid that re-enables what this proposal wants to disable. But maybe I'm mistaken. WDYT @dsnet ?

[cznic]

FTR: Examples above apply to the tcl branches of sqlite2go and crt packages.

[bcmills]

Oh, I think I see. Under this proposal, that code would end up like:

func TS(init string) (p unsafe.Pointer) {
	h := (*reflect.StringHeader)(unsafe.Pointer(&init))
	h.Data.SetInto(&p)
	return p
}

// Xmain is defined at hello.c:3:5
func Xmain(tls crt.TLS, _ int32, _ uintptr /* **int8 */) (r int32) {
	crt.Xprintf(tls, uintptr(ts) /* "Hello World\n" */)
	return r
}

and the rest of the code would remain unchanged. (Am I missing StringHeader usage elsewhere in that snippet?)

[cznic]

Almost, except for the different signature. TS (at tcl branch) returns an uintptr, not unsafe.Pointer.

The sqlite2go project is more or less a rewrite of an older ccgo project which avoids as much as it can mixing of C-world and Go-world pointers. In few places, which I've verified with @ianlancetaylor to be safe, the C world can observe pointers to Go-world, like in the TS, DS and BSS functions. So yes, in these places even getting out the unsafe.Pointer and then immediately converting it to uintptr should still be safe.

PS: Hmm, it seems I've misread the SetInto method, mistakenly seeing it as Set and thus not a solution to the problem I'm concerned about. My bad.

[ianlancetaylor]

@cznic You may be interested in the _GoString_ type available in Go 1.10. You might be able to use that in wrapper functions.

[cznic]

Thanks @ianlancetaylor, however in this particular case of sqlite2go, the produced Go code does not directly or indirectly use CGO.

[ianlancetaylor]

See also #19367.

[griesemer]

Instead of introducing another representation for an unsafe.Pointer, we could instead use an unsafe.Pointer directly together with the property that an unsafe.Pointer can only be used in a conversion if the unsafe package is imported (as proposed in #26070).

[dsnet]

I'm rescinding this proposal in light of #40592.