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: value converted to uintptr escapes to heap #28369

Closed
ainar-g opened this Issue Oct 24, 2018 · 10 comments

Comments

Projects
None yet
8 participants
@ainar-g
Copy link
Contributor

ainar-g commented Oct 24, 2018

go version go1.11 linux/amd64
go version devel +5538ecadca Wed Oct 24 20:08:18 2018 +0000 linux/amd64

$ nl -b a foo.go 
     1	package foo
     2	
     3	import "unsafe"
     4	
     5	var top uintptr
     6	
     7	func f(n int) int {
     8		if n == 0 {
     9			top = uintptr(unsafe.Pointer(&n))
    10			return n
    11		}
    12	
    13		return 1 + f(n-1)
    14	}
$ go build -gcflags "-m" foo.go
# command-line-arguments
foo.go:9:32: &n escapes to heap
foo.go:7:8: moved to heap: n

Playground: https://play.golang.org/p/AP4_XYd5tFX.

n here shouldn't escape, since its address is only taken for debugging purposes. The usage of unsafe.Pointer is correct, see (2). If I comment out line 9, cmd/compile doesn't move n to heap, as it should.

@randall77

This comment has been minimized.

Copy link
Contributor

randall77 commented Oct 24, 2018

@bcmills bcmills added this to the Go1.12 milestone Oct 25, 2018

@dr2chase

This comment has been minimized.

Copy link
Contributor

dr2chase commented Oct 25, 2018

Note that uintptr is treated specially because reasons, but this "obvious" change to escassign in esc.go

func (e *EscState) escassign(dst, src *Node, step *EscStep) {
	if dst.isBlank() || dst == nil || src == nil || src.Op == ONONAME || src.Op == OXXX {
		return
	}
	if !types.Haspointers(src.Type) && (src.Type.Etype != types.TUINTPTR) {
		if Debug['m'] > 2 {
			Warnl(src.Pos, "Esc-assigned type has no pointers")
		}
		return
	}

produced a build that (1) failed some tests in horrible crashy ways and (2) passed all the escape analysis tests. This was supposed to be easy, and instead it looks interesting.

@dr2chase

This comment has been minimized.

Copy link
Contributor

dr2chase commented Oct 25, 2018

One of the places where this wrong-fix causes a difference in allocation behavior is test/gogcrt.go, line 174

				a[new(bool)] = false

Putting this off (unless someone else is interested) till after the freeze.

@odeke-em

This comment has been minimized.

Copy link
Member

odeke-em commented Feb 4, 2019

Hello @dr2chase @randall77, shall we punt this to Go1.13? If for Go1.13, @dr2chase let's team up and work on this issue during the cycle.

@dr2chase

This comment has been minimized.

Copy link
Contributor

dr2chase commented Feb 4, 2019

Definitely punt to 1.13, this is not a crash, and I don't think this is a regression, either.
@mdempsky is working on (mostly done with) a rewrite of the escape analysis code, his new code might handle this more nicely (or at least more understandably).

I don't much like the the bit about my change passing escape analysis tests, yet producing a sometimes-crashing build.

@andybons andybons modified the milestones: Go1.12, Go1.13 Feb 12, 2019

@mdempsky

This comment has been minimized.

Copy link
Member

mdempsky commented Apr 2, 2019

My escape analysis rewrite keep n allocated on the stack.

@ainar-g

This comment has been minimized.

Copy link
Contributor Author

ainar-g commented Apr 16, 2019

@mdempsky I'm on version 13b7c04, and it seems fixed. Should I close the issue, or is there something else to do here, like adding test cases?

@mdempsky

This comment has been minimized.

Copy link
Member

mdempsky commented Apr 16, 2019

@ainar-g A regress test case CL would be great!

@gopherbot

This comment has been minimized.

Copy link

gopherbot commented Apr 16, 2019

Change https://golang.org/cl/172358 mentions this issue: test: add regress test for issue 28369

@ainar-g

This comment has been minimized.

Copy link
Contributor Author

ainar-g commented Apr 16, 2019

This is my first change to test/, so I'm not entirely sure I've done it correctly.

@gopherbot gopherbot closed this in 7cdacf5 Apr 16, 2019

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