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

cmd/compile: Escape analysis misses pointer assignment to the Data field of reflect.StringHeader and reflect.SliceHeader #19743

Closed
jeromefroe opened this issue Mar 28, 2017 · 8 comments

Comments

Projects
None yet
6 participants
@jeromefroe
Copy link

commented Mar 28, 2017

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

go version go1.7.5 darwin/amd64

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/jeromefroelich/golang"
GORACE=""
GOROOT="/usr/local/opt/go/libexec"
GOTOOLDIR="/usr/local/opt/go/libexec/pkg/tool/darwin_amd64"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/mc/_m9vnrfs0qjg9g8rl1h4s9n80000gn/T/go-build857571992=/tmp/go-build -gno-record-gcc-switches -fno-common"
CXX="clang++"
CGO_ENABLED="1"

What did you do?

When using the reflect and unsafe packages to access the underlying data for a string or slice it seems the compiler does not consider assignments to the Data field of these structs when performing escape analysis. Consider the following program, a runnable example on the Go Playground is here

package main

import (
	"fmt"
	"reflect"
	"unsafe"
)

type immutableBytes []byte

func toString(b immutableBytes) string {
	var s string
 	if len(b) == 0 {
 		return s
 	}
	
	strHeader := (*reflect.StringHeader)(unsafe.Pointer(&s))
 	strHeader.Data = (*reflect.SliceHeader)(unsafe.Pointer(&b)).Data
	
	l := len(b)
 	strHeader.Len = l
	return s
}

func getString() string {
	b := immutableBytes("foobarbaz")
	s := toString(b)
	return s
}

//go:noinline
func getStringNoInline() string {
	b := immutableBytes("foobarbaz")
	s := toString(b)
	return s
} 

func main() {
	s := getString()
	fmt.Println(s)
	
	s = getStringNoInline()
	fmt.Println(s)
}

What did you expect to see?

The unsafe package states that the following pattern is valid:

Conversion of a reflect.SliceHeader or reflect.StringHeader Data field to or from Pointer.

Consequently, I would expect to see the same output for both Println calls. That is:

foobarbaz
foobarbaz

What did you see instead?

Instead, the second Println call prints random data. For example, in one run, the output was:

foobarbaz
|�B���J

It seems that the problem is that the compiler does not "see" that the pointer to the underlying bytes is being returned in the string. Consequently, since the compiler thinks the bytes slice does not escape, it is allocating it on the stack. This isn't a problem in the call to getString because the function is inlined and so a pointer to the data is valid for subsequent instructions on the stack. However, if we force the compiler to not inline the function, as in getStringNoInline, then the data for the byte slice is once again allocated on the stack, but the pointer to is no longer valid when the function returns and its call stack is popped off the stack. Escape analysis output from the compiler seems to support this theory. Relevant lines are highlighted below:

$ go build -gcflags -m
# github.com/jeromefroe/test_unsafe_conversion
./main.go:11: can inline toString
./main.go:25: can inline getString
./main.go:27: inlining call to toString
./main.go:34: inlining call to toString
./main.go:39: inlining call to getString   <-----
./main.go:39: inlining call to toString
./main.go:11: toString b does not escape
./main.go:17: toString &s does not escape
./main.go:18: toString &b does not escape
./main.go:26: getString immutableBytes("foobarbaz") does not escape <-----
./main.go:27: getString &s does not escape
./main.go:27: getString &b does not escape
./main.go:33: getStringNoInline immutableBytes("foobarbaz") does not escape <-----
./main.go:34: getStringNoInline &s does not escape
./main.go:34: getStringNoInline &b does not escape
./main.go:40: s escapes to heap
./main.go:43: s escapes to heap
./main.go:39: main immutableBytes("foobarbaz") does not escape
./main.go:39: main &s does not escape
./main.go:39: main &b does not escape
./main.go:40: main ... argument does not escape
./main.go:43: main ... argument does not escape
@bradfitz

This comment has been minimized.

Copy link
Member

commented Mar 28, 2017

Can you try Go 1.8?

@jeromefroe

This comment has been minimized.

Copy link
Author

commented Mar 28, 2017

Sure thing:

$ go version
go version go1.8 darwin/amd64

$ go run main.go
foobarbaz
@

$ go build -gcflags -m
# github.com/jeromefroe/test_unsafe_conversion
./main.go:11: can inline toString
./main.go:25: can inline getString
./main.go:27: inlining call to toString
./main.go:34: inlining call to toString
./main.go:39: inlining call to getString
./main.go:39: inlining call to toString
./main.go:11: toString b does not escape
./main.go:17: toString &s does not escape
./main.go:18: toString &b does not escape
./main.go:26: getString immutableBytes("foobarbaz") does not escape
./main.go:27: getString &s does not escape
./main.go:27: getString &b does not escape
./main.go:33: getStringNoInline immutableBytes("foobarbaz") does not escape
./main.go:34: getStringNoInline &s does not escape
./main.go:34: getStringNoInline &b does not escape
./main.go:40: s escapes to heap
./main.go:43: s escapes to heap
./main.go:39: main immutableBytes("foobarbaz") does not escape
./main.go:39: main &s does not escape
./main.go:39: main &b does not escape
./main.go:40: main ... argument does not escape
./main.go:43: main ... argument does not escape

Seems the results are the same.

@cespare cespare added this to the Go1.9 milestone Mar 28, 2017

@cespare cespare removed the WaitingForInfo label Mar 28, 2017

@josharian

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2017

@dr2chase

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2017

Local discussion of this bug is that it's a special case flaw in the reflect package; we exposed those fields as uintptr, and so without a special case for that particular uintptr, the escape analysis code is "not wrong" because it's not actually pointers. Nonetheless, the special-case fix needs to be applied to the "not wrong" code in escape analysis.

There's a recent CL to some other part of the compiler dealing with the same issue elsewhere, I went looking for it but did not find it immediately.

@josharian

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2017

@dr2chase

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 2017

That's the one. Thanks.

@dr2chase dr2chase self-assigned this Mar 28, 2017

@gopherbot

This comment has been minimized.

Copy link

commented Mar 28, 2017

CL https://golang.org/cl/38738 mentions this issue.

@gopherbot gopherbot closed this in 24e9476 Mar 29, 2017

@gopherbot

This comment has been minimized.

Copy link

commented Apr 5, 2017

CL https://golang.org/cl/39616 mentions this issue.

gopherbot pushed a commit that referenced this issue Apr 5, 2017

[release-branch.go1.8] cmd/compile: added special case for reflect he…
…ader fields to esc

The uintptr-typed Data field in reflect.SliceHeader and
reflect.StringHeader needs special treatment because it is
really a pointer.  Add the special treatment in walk for
bug #19168 to escape analysis.

Includes extra debugging that was helpful.

Fixes #19743.

Change-Id: I6dab5002f0d436c3b2a7cdc0156e4fc48a43d6fe
Reviewed-on: https://go-review.googlesource.com/39616
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>

lparth added a commit to lparth/go that referenced this issue Apr 13, 2017

cmd/compile: added special case for reflect header fields to esc
The uintptr-typed Data field in reflect.SliceHeader and
reflect.StringHeader needs special treatment because it is
really a pointer.  Add the special treatment in walk for
bug golang#19168 to escape analysis.

Includes extra debugging that was helpful.

Fixes golang#19743.

Change-Id: I6dab5002f0d436c3b2a7cdc0156e4fc48a43d6fe
Reviewed-on: https://go-review.googlesource.com/38738
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>

@golang golang locked and limited conversation to collaborators Apr 5, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.