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: runtime.KeepAlive doesn't work #22458

Closed
aclements opened this issue Oct 26, 2017 · 13 comments

Comments

Projects
None yet
7 participants
@aclements
Copy link
Member

commented Oct 26, 2017

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

go version devel +bd48d37e30 Thu Oct 26 17:29:27 2017 +0000 linux/amd64

Does this issue reproduce with the latest release?

Go 1.9 produces an internal compiler error.
Go 1.8 works.

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

GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"
GOPATH="/home/austin/r/go"
GORACE=""
GOROOT="/home/austin/gotmp"
GOTOOLDIR="/home/austin/gotmp/pkg/tool/linux_amd64"
GCCGO="/usr/bin/gccgo"
CC="gcc"
GOGCCFLAGS="-fPIC -m64 -pthread -fmessage-length=0 -fdebug-prefix-map=/tmp/go-build276745456=/tmp/go-build -gno-record-gcc-switches"
CXX="g++"
CGO_ENABLED="1"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"

What did you do?

The following program creates a 64MB linked list and then starts to reverse it. (This is distilled from a benchmark.)

https://play.golang.org/p/P3GLK4gz7u

What did you expect to see?

The program uses runtime.KeepAlive to keep the list alive until the function returns, so I would expect the heap size before and after the reversal to be the same:

heap size before: 64 MB
heap size after (should be the same): 64 MB

What did you see instead?

In Go 1.8, I get the above output.

In Go 1.9, I get an ICE (this can be seen on the playground):

./xxx.go:41:19: internal compiler error: internal error: main prev (type *node) recorded as live on entry

goroutine 1 [running]:
runtime/debug.Stack(0x0, 0x0, 0x0)
	/home/austin/gotmp/src/runtime/debug/stack.go:24 +0xa7
cmd/compile/internal/gc.Fatalf(0xb6e637, 0x2f, 0xc42036b418, 0x2, 0x2)
	/home/austin/gotmp/src/cmd/compile/internal/gc/subr.go:181 +0x230
cmd/compile/internal/gc.livenessepilogue(0xc4200c63c0)
	/home/austin/gotmp/src/cmd/compile/internal/gc/plive.go:772 +0x12c8
cmd/compile/internal/gc.liveness(0xc420395cb0, 0xc42000e3c0, 0x0)
	/home/austin/gotmp/src/cmd/compile/internal/gc/plive.go:1293 +0xad
cmd/compile/internal/gc.genssa(0xc42000e3c0, 0xc42008e820)
	/home/austin/gotmp/src/cmd/compile/internal/gc/ssa.go:4383 +0x12d6
cmd/compile/internal/gc.compileSSA(0xc420390160, 0x0)
	/home/austin/gotmp/src/cmd/compile/internal/gc/pgen.go:242 +0x7e
cmd/compile/internal/gc.compile(0xc420390160)
	/home/austin/gotmp/src/cmd/compile/internal/gc/pgen.go:219 +0x218
cmd/compile/internal/gc.funccompile(0xc420390160)
	/home/austin/gotmp/src/cmd/compile/internal/gc/dcl.go:1049 +0xb7
cmd/compile/internal/gc.Main(0xb73f98)
	/home/austin/gotmp/src/cmd/compile/internal/gc/main.go:585 +0x29d2
main.main()
	/home/austin/gotmp/src/cmd/compile/main.go:49 +0x95

command failed: exit status 2

On master, I get:

heap size before: 64 MB
heap size after (should be the same): 0 MB

Which suggests KeepAlive just isn't working.

If I uncomment the println(prev, head) at the end, all three produce the expected output.

/cc @iant @randall77

@aclements aclements added this to the Go1.9.3 milestone Oct 26, 2017

@mvdan mvdan changed the title cmd/compile: runtime.KeepAlive doesn't cmd/compile: runtime.KeepAlive doesn't work Oct 26, 2017

@mvdan mvdan added the NeedsFix label Oct 26, 2017

@mdempsky

This comment has been minimized.

Copy link
Member

commented Oct 26, 2017

@aclements At what Git revision did you get the "0 MB" output? At 6222997 I still get the ICE.

@mdempsky

This comment has been minimized.

Copy link
Member

commented Oct 27, 2017

Here's a minimal ICE repro:

package main

import "runtime"

type node struct {
        next *node
}

var x bool

func main() {
        var head *node
        for x {
                head = &node{head}
        }

        runtime.KeepAlive(head)
}
@mdempsky

This comment has been minimized.

Copy link
Member

commented Oct 27, 2017

Looking at the GOSSAFUNC output, it looks like head is being allocated to a register, except when it needs to be spilled for the runtime.newobject call.

But then the OpKeepAlive instruction is referencing the StoreReg spill from inside the loop body, which seems ill-formed because the loop body isn't guaranteed to have even executed.

@aclements

This comment has been minimized.

Copy link
Member Author

commented Oct 27, 2017

@aclements At what Git revision did you get the "0 MB" output? At 6222997 I still get the ICE.

Oops, you're right. It looks like I was actually at CL 73712. That CL appears to "fix" the ICE, but the list still gets GC'd, so KeepAlive isn't working. That CL eliminates the write barrier call from the loop, which probably affects the liveness tracking.

@mdempsky

This comment has been minimized.

Copy link
Member

commented Oct 27, 2017

It appears the issue is in regalloc (/cc @randall77). In the GOSSAFUNC output, before regalloc, the SSA graph is well-formed: the OpKeepAlive is referencing an OpPhi in a dominating block.

However, after regalloc, OpKeepAlive directly references the OpStoreReg.

@odeke-em

This comment has been minimized.

Copy link
Member

commented Oct 28, 2017

Just a drive-by comment, this issue seems to also exist on tip 6eaf7bc, should we instead mark it as Go1.10 and then backport on fixing?

After running @mdempsky's #22458 (comment)

$ go run main.go 
# command-line-arguments
<autogenerated>:1:0: internal compiler error: internal error: main head (type *node) recorded as live on entry

goroutine 11 [running]:
runtime/debug.Stack(0x0, 0x0, 0x0)
	/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/runtime/debug/stack.go:24 +0xa7
cmd/compile/internal/gc.Fatalf(0x179e686, 0x2f, 0xc420313ac0, 0x2, 0x2)
	/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/cmd/compile/internal/gc/subr.go:182 +0x1f2
cmd/compile/internal/gc.(*Liveness).epilogue(0xc4200b6180)
	/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/cmd/compile/internal/gc/plive.go:743 +0x1243
cmd/compile/internal/gc.liveness(0xc4203f4ed0, 0xc420426140, 0x178b560)
	/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/cmd/compile/internal/gc/plive.go:1259 +0xad
cmd/compile/internal/gc.genssa(0xc420426140, 0xc420568460)
	/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/cmd/compile/internal/gc/ssa.go:4446 +0xa3
cmd/compile/internal/gc.compileSSA(0xc420394160, 0x3)
	/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/cmd/compile/internal/gc/pgen.go:246 +0x19a
cmd/compile/internal/gc.compileFunctions.func2(0xc420424a20, 0xc4203ec3c0, 0x3)
	/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/cmd/compile/internal/gc/pgen.go:287 +0x49
created by cmd/compile/internal/gc.compileFunctions
	/Users/emmanuelodeke/go/src/go.googlesource.com/go/src/cmd/compile/internal/gc/pgen.go:285 +0x104
@gopherbot

This comment has been minimized.

Copy link

commented Oct 28, 2017

Change https://golang.org/cl/74210 mentions this issue: cmd/compile: fix runtime.KeepAlive

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Oct 28, 2017

@odeke-em Marking an issue 1.9.3 doesn't change the fact that we should fix it on tip and then backport the fix. We should always work that way except in exceptional circumstances. The 1.9.3 marking just means to make sure that we backport it. In general the issue milestone should be the earlier release number that should receive the fix; all later releases should get it to.

@gopherbot gopherbot closed this in 0153a41 Oct 30, 2017

@aclements

This comment has been minimized.

Copy link
Member Author

commented Oct 30, 2017

Re-open for cherry-pick to Go 1.9.

@aclements aclements reopened this Oct 30, 2017

@odeke-em

This comment has been minimized.

Copy link
Member

commented Oct 31, 2017

Awesome, thank you for letting me know @ianlancetaylor, I'll keep that in mind :)

@andybons

This comment has been minimized.

Copy link
Member

commented Jan 18, 2018

CL 74210 OK for Go 1.9.3.

@gopherbot

This comment has been minimized.

Copy link

commented Jan 18, 2018

Change https://golang.org/cl/88318 mentions this issue: [release-branch.go1.9] cmd/compile: fix runtime.KeepAlive

gopherbot pushed a commit that referenced this issue Jan 22, 2018

[release-branch.go1.9] cmd/compile: fix runtime.KeepAlive
KeepAlive needs to introduce a use of the spill of the
value it is keeping alive.  Without that, we don't guarantee
that the spill dominates the KeepAlive.

This bug was probably introduced with the code to move spills
down to the dominator of the restores, instead of always spilling
just after the value itself (CL 34822).

Fixes #22458.

Change-Id: I94955a21960448ffdacc4df775fe1213967b1d4c
Reviewed-on: https://go-review.googlesource.com/74210
Reviewed-by: Cherry Zhang <cherryyz@google.com>
Reviewed-by: David Chase <drchase@google.com>
Reviewed-on: https://go-review.googlesource.com/88318
Run-TryBot: Andrew Bonventre <andybons@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
@andybons

This comment has been minimized.

Copy link
Member

commented Jan 22, 2018

go1.9.3 has been packaged and includes:

  • CL 74210 cmd/compile: fix runtime.KeepAlive

The release is posted at golang.org/dl.

— golang.org/x/build/cmd/releasebot, Jan 22 21:02:54 UTC

@andybons andybons closed this Jan 22, 2018

@golang golang locked and limited conversation to collaborators Jan 22, 2019

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.