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 does't treat reflect header correctly #39497

Closed
choleraehyq opened this issue Jun 10, 2020 · 4 comments
Closed

cmd/compile: escape analysis does't treat reflect header correctly #39497

choleraehyq opened this issue Jun 10, 2020 · 4 comments
Labels
FrozenDueToAge WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.

Comments

@choleraehyq
Copy link
Contributor

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

$ go version
go version go1.14.4 darwin/amd64

Does this issue reproduce with the latest release?

Yes.

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/cholerae/Library/Caches/go-build"
GOENV="/Users/cholerae/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GONOPROXY="code.byted.org,git.byted.org"
GONOSUMDB="code.byted.org,git.byted.org"
GOOS="darwin"
GOPATH="/Users/cholerae/Documents/gopath"
GOPRIVATE="code.byted.org,git.byted.org"
GOPROXY="https://goproxy.cn,direct"
GOROOT="/Users/cholerae/Documents/gopath/go"
GOSUMDB="sum.golang.google.cn"
GOTMPDIR=""
GOTOOLDIR="/Users/cholerae/Documents/gopath/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/lg/ld5t5rss459241qtzmqfp0h80000gn/T/go-build022327195=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

https://play.golang.org/p/PL0FCLCv3qU
and run escape analysis on this code.

What did you expect to see?

The return value of x1 and x2 should be the same. And in both x1 and x2, b should escape to heap.

What did you see instead?

x1() and x2() are not the same. b in x2 doesn't escape to heap.
Full escape analysis output:

unsafestr.go:10:6: cannot inline x1: function too complex: cost 110 exceeds budget 80
unsafestr.go:12:31: inlining call to binary.littleEndian.PutUint64 method(binary.littleEndian) func([]byte, uint64) { _ = binary.b[int(7)]; binary.b[int(0)] = byte(binary.v); binary.b[int(1)] = byte(binary.v >> uint(8)); binary.b[int(2)] = byte(binary.v >> uint(16)); binary.b[int(3)] = byte(binary.v >> uint(24)); binary.b[int(4)] = byte(binary.v >> uint(32)); binary.b[int(5)] = byte(binary.v >> uint(40)); binary.b[int(6)] = byte(binary.v >> uint(48)); binary.b[int(7)] = byte(binary.v >> uint(56)) }
unsafestr.go:21:6: cannot inline x2: function too complex: cost 107 exceeds budget 80
unsafestr.go:23:31: inlining call to binary.littleEndian.PutUint64 method(binary.littleEndian) func([]byte, uint64) { _ = binary.b[int(7)]; binary.b[int(0)] = byte(binary.v); binary.b[int(1)] = byte(binary.v >> uint(8)); binary.b[int(2)] = byte(binary.v >> uint(16)); binary.b[int(3)] = byte(binary.v >> uint(24)); binary.b[int(4)] = byte(binary.v >> uint(32)); binary.b[int(5)] = byte(binary.v >> uint(40)); binary.b[int(6)] = byte(binary.v >> uint(48)); binary.b[int(7)] = byte(binary.v >> uint(56)) }
unsafestr.go:31:6: cannot inline main: function too complex: cost 196 exceeds budget 80
unsafestr.go:32:13: inlining call to fmt.Println func(...interface {}) (int, error) { var fmt..autotmp_3 int; fmt..autotmp_3 = <N>; var fmt..autotmp_4 error; fmt..autotmp_4 = <N>; fmt..autotmp_3, fmt..autotmp_4 = fmt.Fprintln(io.Writer(os.Stdout), fmt.a...); return fmt..autotmp_3, fmt..autotmp_4 }
unsafestr.go:11:11: make([]byte, 12) escapes to heap:
unsafestr.go:11:11:   flow: b = &{storage for make([]byte, 12)}:
unsafestr.go:11:11:     from make([]byte, 12) (spill) at unsafestr.go:11:11
unsafestr.go:11:11:     from b := make([]byte, 12) (assign) at unsafestr.go:11:4
unsafestr.go:11:11:   flow: pbytes = &b:
unsafestr.go:11:11:     from &b (address-of) at unsafestr.go:14:50
unsafestr.go:11:11:     from pbytes := (*reflect.SliceHeader)(unsafe.Pointer(&b)) (assign) at unsafestr.go:14:9
unsafestr.go:11:11:   flow: {heap} = *pbytes:
unsafestr.go:11:11:     from pbytes.Data (reflect.Header.Data) at unsafestr.go:16:23
unsafestr.go:11:11:     from pstring.Data = pbytes.Data (assign) at unsafestr.go:16:15
unsafestr.go:11:11: make([]byte, 12) escapes to heap
unsafestr.go:22:11: make([]byte, 12) does not escape
unsafestr.go:32:34: ([]byte)(x2()) escapes to heap:
unsafestr.go:32:34:   flow: ~arg1 = &{storage for ([]byte)(x2())}:
unsafestr.go:32:34:     from ([]byte)(x2()) (spill) at unsafestr.go:32:34
unsafestr.go:32:34:     from ~arg0, ~arg1 = <N> (assign-pair) at unsafestr.go:32:13
unsafestr.go:32:34:   flow: {storage for []interface {} literal} = ~arg1:
unsafestr.go:32:34:     from []interface {} literal (slice-literal-element) at unsafestr.go:32:13
unsafestr.go:32:34:   flow: fmt.a = &{storage for []interface {} literal}:
unsafestr.go:32:34:     from []interface {} literal (spill) at unsafestr.go:32:13
unsafestr.go:32:34:     from fmt.a = []interface {} literal (assign) at unsafestr.go:32:13
unsafestr.go:32:34:   flow: {heap} = *fmt.a:
unsafestr.go:32:34:     from fmt.Fprintln(io.Writer(os.Stdout), fmt.a...) (call parameter) at unsafestr.go:32:13
unsafestr.go:32:20: ([]byte)(x1()) escapes to heap:
unsafestr.go:32:20:   flow: ~arg0 = &{storage for ([]byte)(x1())}:
unsafestr.go:32:20:     from ([]byte)(x1()) (spill) at unsafestr.go:32:20
unsafestr.go:32:20:     from ~arg0, ~arg1 = <N> (assign-pair) at unsafestr.go:32:13
unsafestr.go:32:20:   flow: {storage for []interface {} literal} = ~arg0:
unsafestr.go:32:20:     from []interface {} literal (slice-literal-element) at unsafestr.go:32:13
unsafestr.go:32:20:   flow: fmt.a = &{storage for []interface {} literal}:
unsafestr.go:32:20:     from []interface {} literal (spill) at unsafestr.go:32:13
unsafestr.go:32:20:     from fmt.a = []interface {} literal (assign) at unsafestr.go:32:13
unsafestr.go:32:20:   flow: {heap} = *fmt.a:
unsafestr.go:32:20:     from fmt.Fprintln(io.Writer(os.Stdout), fmt.a...) (call parameter) at unsafestr.go:32:13
unsafestr.go:32:34: ([]byte)(x2()) escapes to heap:
unsafestr.go:32:34:   flow: {storage for ([]byte)(x2())} = &{storage for ([]byte)(x2())}:
unsafestr.go:32:34:     from ([]byte)(x2()) (spill) at unsafestr.go:32:34
unsafestr.go:32:34:     from ([]byte)(x2()) (interface-converted) at unsafestr.go:32:34
unsafestr.go:32:20: ([]byte)(x1()) escapes to heap:
unsafestr.go:32:20:   flow: {storage for ([]byte)(x1())} = &{storage for ([]byte)(x1())}:
unsafestr.go:32:20:     from ([]byte)(x1()) (spill) at unsafestr.go:32:20
unsafestr.go:32:20:     from ([]byte)(x1()) (interface-converted) at unsafestr.go:32:20
unsafestr.go:32:20: ([]byte)(x1()) escapes to heap
unsafestr.go:32:20: ([]byte)(x1()) escapes to heap
unsafestr.go:32:34: ([]byte)(x2()) escapes to heap
unsafestr.go:32:34: ([]byte)(x2()) escapes to heap
unsafestr.go:32:13: []interface {} literal does not escape
<autogenerated>:1: .this does not escape

The root cause it https://github.com/golang/go/blob/master/src/cmd/compile/internal/gc/escape.go#L725 this line. I'm not sure whether this is a bug or a feature.

@rsc
Copy link
Contributor

rsc commented Jun 10, 2020

The docs on reflect.SliceHeader say that it cannot be the sole reference to a piece of data. x2 declares a SliceHeader directly - you can't do that. You must instead cast an existing slice to a *SliceHeader, like in x1.

@choleraehyq
Copy link
Contributor Author

But if I change x2 like this:

func x2() string {
	b := make([]byte, 12)
	binary.LittleEndian.PutUint64(b, 1)
	bh := (*reflect.SliceHeader)(unsafe.Pointer(&b))
	sh := &reflect.StringHeader{}
	sh.Data = bh.Data
	sh.Len = bh.Len
	return *(*string)(unsafe.Pointer(sh))
}

It will behave the same as x1. In this code, StringHeader is also the sole reference to that piece of data. Can I consider this version of x2 is also wrong, and it has the same behavior as x2 coincidently just under our current implementation and may change its behavior in the future?

@ianlancetaylor ianlancetaylor added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Jun 10, 2020
@ianlancetaylor
Copy link
Contributor

The code is also incorrect, in that the reflect.StringHeader does not point to a string. See the discussion of StringHeader and SliceHeader at https://golang.org/pkg/unsafe/#Pointer.

@choleraehyq
Copy link
Contributor Author

@ianlancetaylor Thanks ian.

@golang golang locked and limited conversation to collaborators Jun 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Development

No branches or pull requests

4 participants