-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
Comments
@aclements At what Git revision did you get the "0 MB" output? At 6222997 I still get the ICE. |
Here's a minimal ICE repro:
|
Looking at the GOSSAFUNC output, it looks like 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. |
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 |
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. |
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 |
Change https://golang.org/cl/74210 mentions this issue: |
@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. |
Re-open for cherry-pick to Go 1.9. |
Awesome, thank you for letting me know @ianlancetaylor, I'll keep that in mind :) |
CL 74210 OK for Go 1.9.3. |
Change https://golang.org/cl/88318 mentions this issue: |
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>
go1.9.3 has been packaged and includes:
The release is posted at golang.org/dl. — golang.org/x/build/cmd/releasebot, Jan 22 21:02:54 UTC |
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
)?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: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):
On master, I get:
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
The text was updated successfully, but these errors were encountered: