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: unused variable escapes to the heap #27188

Closed
mariecurried opened this issue Aug 24, 2018 · 7 comments
Closed

cmd/compile: unused variable escapes to the heap #27188

mariecurried opened this issue Aug 24, 2018 · 7 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@mariecurried
Copy link

mariecurried commented Aug 24, 2018

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

go version go1.10.3 windows/amd64

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

set GOARCH=amd64
set GOBIN=
set GOCACHE=...
set GOEXE=.exe
set GOHOSTARCH=amd64
set GOHOSTOS=windows
set GOOS=windows
set GOPATH=...
set GORACE=
set GOROOT=C:\Go
set GOTMPDIR=
set GOTOOLDIR=C:\Go\pkg\tool\windows_amd64
set GCCGO=gccgo
set CC=gcc
set CXX=g++
set CGO_ENABLED=1
set CGO_CFLAGS=-g -O2
set CGO_CPPFLAGS=
set CGO_CXXFLAGS=-g -O2
set CGO_FFLAGS=-g -O2
set CGO_LDFLAGS=-g -O2
set PKG_CONFIG=pkg-config
set GOGCCFLAGS=...

What did you do?

I compiled the following program using `go build -gcflags="-m -m" to check if any of the variables escaped to the heap.
https://play.golang.org/p/iZCz8oXRoi__h

What did you expect to see?

I expected only y to escape.

What did you see instead?

Instead, both x and y escaped to the heap.
In my opinion, only y should escape, because there is no way for x to leave the scope of this function. Alternatively, the assignment of the address of x to k should be entirely removed.

@mariecurried mariecurried changed the title cmd/compile: cmd/compile: unused variable escapes to the heap Aug 24, 2018
@ianlancetaylor ianlancetaylor added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 24, 2018
@ianlancetaylor ianlancetaylor added this to the Go1.12 milestone Aug 24, 2018
@ianlancetaylor
Copy link
Contributor

CC @dr2chase

@dr2chase dr2chase modified the milestones: Go1.12, Unplanned Aug 24, 2018
@dr2chase
Copy link
Contributor

The algorithm we use is not that clever and just tracks flow by variable name.
If/when we get around to moving escape analysis into the SSA phase (there are several difficult prerequisites in the way of this happening -- and these are planned, e.g., moving call expansion into SSA) this will be fixed as a side-effect.

Since there's an easy workaround and big blockers for the proper fix, I would not commit to fixing this in 1.12.

@rasky
Copy link
Member

rasky commented Aug 24, 2018

Do you have a list offhand of all the prerequisites for a new SSA-based escape analysis?

@dr2chase
Copy link
Contributor

I think (but now I am a little bit less sure, but lack the time right now to be more sure):

  • need generic opcodes to allow us to talk about making things on-stack-or-heap-but-not-sure-yet, to be resolved by escape analysis.
  • calls expanded in SSA itself, not before (I think). At least, calls to newobject, right?
  • need generic opcodes for unexpanded calls.
  • (maybe) an early part of SSA made able to run across some or all of a package, since escape analysis is interprocedural within a package (I am having some second thoughts here, current analysis punts on cycles and thus we could break edges with worst-case punts and do things one function at a time). But we might not always punt on cycles; if we improve the analysis to be able to tell the difference between the spine of a list and the leaves of the list (CDR vs CAR, e.g.) then there will be profit in recursive analysis; we can obtain interesting fixed points (I prototyped this, then realized that inability to tell CAR from CDR meant the benefits were very minor).
  • dealing with shared versus not for things allocated in a stack frame will force a revision of SSA form; anything shared is potentially modified by a call, anything local (that is not address-taken and passed in to call) is not.

@randall77
Copy link
Contributor

In addition to that, I think we need to understand what the benefit would be. It's going to be a lot of work, I only want to undertake it if we expect some commensurate benefits.

@dr2chase
Copy link
Contributor

One possible benefit, from talking to a guy at GopherCon (@cixel) who's interested in doing instrumentation for taint detection/analysis, is doing dataflow-informed instrumentation. Idea is that we'd transform to SSA, do whatever instrumentation we want (that might, in some cases, have effects on escape analysis -- and might also have effects on stack frames), then run escape analysis etc.

This is still a little hand-wavy. He's looking the code for tsan and msan, and modifying things at the AST level, but having the ability to do flow analysis would be helpful (I wonder if it might not also help tsan and msan)

@mdempsky
Copy link
Member

Dup of #18300; TL;DR: escape analysis is not path sensitive.

See also #31501.

@golang golang locked and limited conversation to collaborators Apr 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

7 participants