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: improve escape analysis of make([]T, len, cap) where len is non-constant #37975

Closed
TriAnMan opened this issue Mar 20, 2020 · 3 comments
Closed

Comments

@TriAnMan
Copy link

@TriAnMan TriAnMan commented Mar 20, 2020

$ go version
go version go1.14.1 windows/amd64

$ cat test.go
package main

func main() {
	i := 2
	s := make([]int, 2, 3)
	s = make([]int, i, 3)
	s = make([]int, i, 1)
	s[0] = 1
}

$ go run -gcflags -m=2 test.go
# command-line-arguments
.\test.go:3:6: can inline main as: func() { i := 2; s := make([]int, 2, 3); s = make([]int, i, 3); s = make([]int, i, 1); s[0] = 1 }
.\test.go:6:10: make([]int, i, 3) escapes to heap:
.\test.go:6:10:   flow: {heap} = &{storage for make([]int, i, 3)}:
.\test.go:6:10:     from make([]int, i, 3) (non-constant size) at .\test.go:6:10
.\test.go:7:10: make([]int, i, 1) escapes to heap:
.\test.go:7:10:   flow: {heap} = &{storage for make([]int, i, 1)}:
.\test.go:7:10:     from make([]int, i, 1) (non-constant size) at .\test.go:7:10
.\test.go:5:11: make([]int, 2, 3) does not escape
.\test.go:6:10: make([]int, i, 3) escapes to heap
.\test.go:7:10: make([]int, i, 1) escapes to heap
panic: runtime error: makeslice: cap out of range

goroutine 1 [running]:
main.main()
        C:/cygwin64/home/work/projects/test/test.go:7 +0x68
exit status 2

In this program, the compiler's escape analysis judges that the array allocated by make escapes to the heap, when len is variable. But actually this array can be allocated on stack because we can't make len greater than cap in runtime and cap is constant.

@randall77 randall77 added this to the Unplanned milestone Mar 20, 2020
@ivzhh
Copy link

@ivzhh ivzhh commented Mar 21, 2020

Logic check stack on master branch (commit hash: 287d67e3dd3972b1d1006b06e0d57929540a1591
)

func isSmallMakeSlice(n *Node) bool: cmd/compile/internal/gc/walk.go:364
func mustHeapAlloc(n *Node) bool: cmd/compile/internal/gc/esc.go:190
func (e *Escape) newLoc(n *Node, transient bool) *EscLocation: cmd/compile/internal/gc/escape.go:1066

The test for smallintconst(l) fails and escapes this slice into heap.

func isSmallMakeSlice(n *Node) bool {
	if n.Op != OMAKESLICE {
		return false
	}
	l := n.Left
	r := n.Right
	if r == nil {
		r = l
	}
	t := n.Type

	return smallintconst(l) && smallintconst(r) && (t.Elem().Width == 0 || r.Int64() < maxImplicitStackVarSize/t.Elem().Width)
}

If cap is not set, then len == cap, so in any case, the compile only needs to check whether r (aka cap) is constant. The validity of len, as suggested by @TriAnMan, is checked at runtime. So we can always use:

- return smallintconst(l) && smallintconst(r) && (t.Elem().Width == 0 || r.Int64() < maxImplicitStackVarSize/t.Elem().Width)
+ return smallintconst(r) && (t.Elem().Width == 0 || r.Int64() < maxImplicitStackVarSize/t.Elem().Width)
@andybons andybons added the NeedsFix label Mar 23, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Mar 28, 2020

Change https://golang.org/cl/226278 mentions this issue: cmd/compile: make isSmallMakeSlice checks slice cap only

@ivzhh
Copy link

@ivzhh ivzhh commented Mar 29, 2020

Seems related to #20533 and this cl.

@gopherbot gopherbot closed this in 7b30a2d Mar 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.