Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
dsnet opened this issue Apr 3, 2018 · 10 comments

Comments

@dsnet
Copy link
Member

commented Apr 3, 2018

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

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.

@gopherbot gopherbot added this to the Proposal milestone Apr 3, 2018

@gopherbot gopherbot added the Proposal label Apr 3, 2018

@dsnet dsnet added Go2 and removed Proposal labels Apr 3, 2018

@gopherbot gopherbot added the Proposal label Apr 3, 2018

@cznic

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2018

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

This comment has been minimized.

Copy link
Member

commented Apr 3, 2018

@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

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2018

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

@bcmills

This comment has been minimized.

Copy link
Member

commented Apr 3, 2018

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

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2018

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 ianlancetaylor changed the title proposal: reflect: return opaque pointer instead of uintptr proposal: Go 2: reflect: return opaque pointer instead of uintptr Apr 12, 2018

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Apr 12, 2018

@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

This comment has been minimized.

Copy link
Contributor

commented Apr 13, 2018

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

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2018

See also #19367.

@griesemer

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2018

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.