reflect: documentation about "Conversion of a reflect.SliceHeader or reflect.StringHeader Data field to or from Pointer" is misleading

[GingerMoon]

The document about "Conversion of a reflect.SliceHeader or reflect.StringHeader Data field to or from Pointer" is misleading.

var s string
hdr := (*reflect.StringHeader)(unsafe.Pointer(&s)) // case 1
hdr.Data = uintptr(unsafe.Pointer(p))              // case 6 (this case)
hdr.Len = n

It would be great if the doc specifies one more time the risk of the code below:

hdr.Data = uintptr(unsafe.Pointer(p)) 

When p point to a temporary memory, we will have problem. For example,

func demo(val_copy int64) *reflect.StringHeader {
	var s string
	hdr := (*reflect.StringHeader)(unsafe.Pointer(&s)) 			// case 1
	hdr.Data = uintptr(unsafe.Pointer(&val_copy))               // case 6 (this case)
	hdr.Len = 8
	return hdr
}

func main() {
	val := int64(0xab)
	hdr := demo(val)
	s := *(*string)(unsafe.Pointer(hdr))
	fmt.Printf("%x", s)
}

The tricky thing is, it's often that the output is the same as expectation.
But this is a bug because &val_copy is not referenced by others except uintptr, hence the content of the memory pointed by &val_copy is undefined after the demo function returned.

[bcmills]

this is a bug because &val_copy is not referenced by others except uintptr, hence the content of the memory pointed by &val_copy is undefined after the demo function returned.

I don't think that actually is a bug: assigning &val_copy to hdr.Data causes it to be referenced by the variable s, which is itself referenced via the returned *reflect.StringHeader. So the compiler should report that val_copy escapes, and therefore allocate it on the heap.

And indeed it does exactly that (https://play.golang.org/p/nYH-4TVrXWU):

example.com$ go build -gcflags=-m -o=/dev/null .
# example.com
./main.go:9:6: can inline demo
./main.go:19:13: inlining call to demo
./main.go:21:12: inlining call to fmt.Printf
./main.go:9:11: moved to heap: val_copy
./main.go:10:6: moved to heap: s
./main.go:19:13: moved to heap: val_copy
./main.go:21:13: s escapes to heap
./main.go:21:12: []interface {}{...} does not escape
<autogenerated>:1: .this does not escape

CC @mdempsky

[bcmills]

That said, it would be much clearer for the demo function to return the string rather than the *reflect.SliceHeader: https://play.golang.org/p/HD-XVm2iSZi

func demo(val_copy int64) string {
	var s string
	…
	return s
}

But that's a matter of readability, not correctness.

[bcmills]

@GingerMoon, given the above, is there some other way you think we should clarify the documentation?

[mdempsky]

@GingerMoon As I understand your concern, it's that: (1) the documentation suggests your sample program will always print "ab00000000000000" (at least on little-endian CPUs), but (2) the Go compilers do not guarantee this. Do I understand that concern correctly?

If so, then concern 2 is mistaken. The Go compilers do guarantee that output, and the documentation is correct to suggest it.

However, I'm happy to understand the source of misunderstanding here so that we can improve the documentation and prevent future misunderstandings.

[GingerMoon]

Thanks you guys. Before talking about the document, I would like to share a strange case to show you why I have a higher expectation on the document.
(Sorry I cannot access the playground.)

package main

import (
	"bytes"
	"fmt"
	reflect "reflect"
	"sync"
	"unsafe"
)

var (
	n = 1_000_000
)

func demo(val int64) []byte {
	hdr := reflect.SliceHeader{Data: uintptr(unsafe.Pointer(&val)), Len: 8, Cap: 8}
	return *(*[]byte)(unsafe.Pointer(&hdr))
}

func test() {
	val := int64(0xab)

	ch := make(chan []byte, 1_000_000)
	go func() {
		expectation := []byte{0xab,0,0,0, 0,0,0,0}
		for r := range ch {
			if bytes.Compare(expectation, r) != 0 {
				fmt.Printf("expectation: %x, but result is: %x", expectation, r)
				panic("oops")
			}
		}
	}()

	wg := &sync.WaitGroup{}
	wg.Add(n)
	for i := 0; i < n; i++ {
		go func() {
			defer wg.Done()
			r := demo(val)
			ch <- r
		}()
	}
	wg.Wait()
	close(ch)
	fmt.Println("done")
}
func main() {
	for {
		test()
	}
}

The result returned by demo is not ALWAYS the same as our expectation.
The root cause is, hdr.Data references &val_copy via uintptr.
But uintptr is not a "strong" reference to &val_copy.
So the memory pointed by &val_copy is invalid after the function returns.
Please correct me if my understanding is wrong. Any insight would be much appreciated.

/ Even if a uintptr holds the address of some object,
// the garbage collector will not update that uintptr's value
// if the object moves, nor will that uintptr keep the object
// from being reclaimed.

//  // INVALID: a directly-declared header will not hold Data as a reference.
// var hdr reflect.StringHeader
// hdr.Data = uintptr(unsafe.Pointer(p))
// hdr.Len = n
// s := *(*string)(unsafe.Pointer(&hdr)) // p possibly already lost

[mdempsky]

The issue with that code is you've declared a variable hdr of type reflect.SliceHeader, and then tried converting it to []byte using *(*[]T)(unsafe.Pointer(&hdr)). That code is not supported. (And cmd/vet should warn about it in Go 1.16: #40701.)

The only supported unsafe use of reflect.SliceHeader is to use *reflect.SliceHeader as a pointer to an actual slice type. The Go compiler has code that specially handles *reflect.SliceHeader, but not reflect.SliceHeader.

[GingerMoon]

The demo func has two problems:

• 1. Incorrect usage of SliceHeader , the document is clear for this.
• 2. Reference the memory pointed by &val_copy by uintptr. uintptr doesn't make val_copy escape to the heap.
     After demo returns, the content of the memory pointed by &val_copy becomes invalid.

This bug is difficult to debug becasue most of the time, the demo returns the expected result.

The sample in the doc below makes people think that hdr.Data = uintptr(unsafe.Pointer(p)) is a correct usage without thinking of the restrictation of using uintptr.
Especially there is a case 6 comment which makes peopole think they are using uintptr correctly.

var s string
hdr := (*reflect.StringHeader)(unsafe.Pointer(&s)) // case 1
hdr.Data = uintptr(unsafe.Pointer(p))              // case 6 (this case)
hdr.Len = n

So I'd like to change the document to remind people the risk of using uintptr.
Also add some more info (not just gc) to the doc below.

// Even if a uintptr holds the address of some object,
// the garbage collector will not update that uintptr's value
// if the object moves, nor will that uintptr keep the object
// from being reclaimed.

Please correct me if my understanding is wrong.

[mdempsky]

Reference the memory pointed by &val_copy by uintptr. uintptr doesn't make val_copy escape to the heap.
After demo returns, the content of the memory pointed by &val_copy becomes invalid.

This is only an issue because of the first issue. The rules allow converting pointers to uintptr for the purposes of storing into the Data field of a *reflect.SliceHeader or *reflect.StringHeader. When unsafe.Pointer is used correctly, the compiler knows val_copy escapes to the heap.

For example, this code that you originally shared is allowed:

func demo(val_copy int64) *reflect.StringHeader {
	var s string
	hdr := (*reflect.StringHeader)(unsafe.Pointer(&s)) 			// case 1
	hdr.Data = uintptr(unsafe.Pointer(&val_copy))               // case 6 (this case)
	hdr.Len = 8
	return hdr
}

[GingerMoon]

Thanks to @mdempsky for sharing the insight!
How about appending this insight into the document sample code?

//	var s string
//	hdr := (*reflect.StringHeader)(unsafe.Pointer(&s)) // case 1
//	hdr.Data = uintptr(unsafe.Pointer(p))              // case 6 (this case). 
//	hdr.Len = n
//
// In this usage hdr.Data is really an alternate way to refer to the underlying
// pointer in the string header, not a uintptr variable itself. 

// However, in the INVALID case below, hdr.Datar will not be the "strong" reference to p,
// so p is possible to be lost.

//
// In general, reflect.SliceHeader and reflect.StringHeader should be used
// only as *reflect.SliceHeader and *reflect.StringHeader pointing at actual
// slices or strings, never as plain structs.
// A program should not declare or allocate variables of these struct types.
//
//	// INVALID: a directly-declared header will not hold Data as a reference.
//	var hdr reflect.StringHeader
//	hdr.Data = uintptr(unsafe.Pointer(p))
//	hdr.Len = n
//	s := *(*string)(unsafe.Pointer(&hdr)) // p possibly already lost

[ianlancetaylor]

It seems to me that your new text basically says the same thing as the INVALID comment in the example.