Closed zikaeroh closed 3 years ago
danscales
commented
3 years ago Thanks, I think I may have already seen this when I lowered the inlining cost of functions with closures. But wanted to get the main change in, since it was stable and passing tests. Will fix this bug next.
mdempsky
commented
3 years ago # github.com/robfig/cron/v3
<autogenerated>:1: internal compiler error: cannot export SELRECV2 (102) node
==> please file an issue and assign to gri@I'm guessing this code has a function that contains a function literal that contains a select statement. We don't currently support inlining select statements, so we shouldn't consider the outer function inlinable either.
# github.com/go-chi/chi/middleware
panic: interface conversion: interface {} is nil, not *escape.locationThis looks like Dcl or ClosureVars aren't correct for one of the functions. Either not matching the PAUTO/PPARAM/PPARAMOUT variables used within the function body, or the Defn/Outer for closure variables is mixed up.
cuonglm
commented
3 years ago Another ICE, which also breaks real code, but I made it smallest reproduce:
package main
import "github.com/soheilhy/cmux"
func main() {
cmux.HTTP2HeaderField("content-type", "application/grpc")
}which yields:
# command-line-arguments
./main.go:6:23: internal compiler error: 'main.func2': Value live at entry. It shouldn't be. func main.func2, node cmux.value, value nil
goroutine 21 [running]:
runtime/debug.Stack(0x102cf7f68, 0x140000ae008, 0x0)
/Users/cuonglm/sources/go/src/runtime/debug/stack.go:24 +0x88
cmd/compile/internal/base.FatalfAt(0x9733000000008, 0x140000ee280, 0x46, 0x140003fe000, 0x4, 0x4)
/Users/cuonglm/sources/go/src/cmd/compile/internal/base/print.go:227 +0x18c
cmd/compile/internal/base.Fatalf(...)
/Users/cuonglm/sources/go/src/cmd/compile/internal/base/print.go:196
cmd/compile/internal/ssagen.(*ssafn).Fatalf(0x14000395d10, 0x9733000000008, 0x102b137f1, 0x40, 0x14000395f20, 0x3, 0x3)
/Users/cuonglm/sources/go/src/cmd/compile/internal/ssagen/ssa.go:7319 +0x160
cmd/compile/internal/ssagen.(*state).Fatalf(...)
/Users/cuonglm/sources/go/src/cmd/compile/internal/ssagen/ssa.go:844
cmd/compile/internal/ssagen.(*state).variable(0x140002f0400, 0x102d07380, 0x14000386f70, 0x140000b0e40, 0x14000094d68)
/Users/cuonglm/sources/go/src/cmd/compile/internal/ssagen/ssa.go:6325 +0x310
cmd/compile/internal/ssagen.(*state).expr(0x140002f0400, 0x102d07380, 0x14000386f70, 0x0)
/Users/cuonglm/sources/go/src/cmd/compile/internal/ssagen/ssa.go:2296 +0x300c
cmd/compile/internal/ssagen.(*state).stmt(0x140002f0400, 0x102d05c10, 0x140003c3310)
/Users/cuonglm/sources/go/src/cmd/compile/internal/ssagen/ssa.go:1497 +0x22c
cmd/compile/internal/ssagen.(*state).stmtList(0x140002f0400, 0x1400008f050, 0x1, 0x1)
/Users/cuonglm/sources/go/src/cmd/compile/internal/ssagen/ssa.go:1268 +0x68
cmd/compile/internal/ssagen.(*state).stmt(0x140002f0400, 0x102d05e68, 0x140000b3c80)
/Users/cuonglm/sources/go/src/cmd/compile/internal/ssagen/ssa.go:1291 +0x13c8
cmd/compile/internal/ssagen.(*state).stmtList(0x140002f0400, 0x140000b3cc0, 0x3, 0x4)
/Users/cuonglm/sources/go/src/cmd/compile/internal/ssagen/ssa.go:1268 +0x68
cmd/compile/internal/ssagen.(*state).stmt(0x140002f0400, 0x102d05c10, 0x140003c2f50)
/Users/cuonglm/sources/go/src/cmd/compile/internal/ssagen/ssa.go:1286 +0xc0
cmd/compile/internal/ssagen.(*state).stmtList(0x140002f0400, 0x140000b3b80, 0x3, 0x4)
/Users/cuonglm/sources/go/src/cmd/compile/internal/ssagen/ssa.go:1268 +0x68
cmd/compile/internal/ssagen.buildssa(0x140000f29a0, 0x0, 0x0)
/Users/cuonglm/sources/go/src/cmd/compile/internal/ssagen/ssa.go:521 +0xe84
cmd/compile/internal/ssagen.Compile(0x140000f29a0, 0x0)
/Users/cuonglm/sources/go/src/cmd/compile/internal/ssagen/pgen.go:168 +0x3c
cmd/compile/internal/gc.compileFunctions.func2.1(0x14000300310, 0x140000f29a0, 0x140000ae658, 0x140000b4b20)
/Users/cuonglm/sources/go/src/cmd/compile/internal/gc/compile.go:127 +0x4c
created by cmd/compile/internal/gc.compileFunctions.func2
/Users/cuonglm/sources/go/src/cmd/compile/internal/gc/compile.go:125 +0x7c
FAIL
gopherbot
commented
3 years ago Change https://golang.org/cl/285676 mentions this issue: [dev.regabi] cmd/compile: scan body of closure in tooHairy to check for disallowed nodes
gopherbot
commented
3 years ago Change https://golang.org/cl/285992 mentions this issue: [dev.typeparams] all: merge dev.regabi (7e0a81d) into dev.typeparams
gopherbot
commented
3 years ago Change https://golang.org/cl/285677 mentions this issue: [dev.regabi] cmd/compile: fix escape analysis problem with closures
zikaeroh
commented
3 years ago FWIW, the first of the CLs (the tooHairy one) made my code compile and pass tests again.
The lack of tests in the CL is a little spooky, though... :slightly_smiling_face:
cuonglm
commented
3 years ago FWIW, the first of the CLs (the tooHairy one) made my code compile and pass tests again.
The lack of tests in the CL is a little spooky, though... 🙂
Same situation for my report. I think we should definitely add tests. I don't notice the issue until I built my program with my local go dev accidently (I usually do export GO=go1.x.x and build program with stable version).
mdempsky
commented
3 years ago Same situation for my report.
To clarify, "same situation" here means that your test program is now successfully passing as well?
I expect we will add more tests before closing this issue. The fix in CL 285676 though was just very obvious in retrospect.
If someone wants to try writing a regress test that they can confirm fails before that CL and is fixed after, that would be a welcome contribution. I'd recommend something of the form:
func ExportedFunction() func() *int {
x := new(int)
return func() *int {
// [... some code that isn't exportable/inlinable ... ]
return x
}
}A handful of reasonable candidate function bodies would be things like select statements, type declarations, and uses of defer and/or recover (all things we don't currently support inlining). Even better would be to have a separate package that calls foo.ExportedFunction()() to exercise potentially-inlinable calls to the function literals.
Also, FYI, the return x is valuable to make sure the function literal is actually a closure. Currently, we optimize function literals that don't use any free variables during SSA construction. I've been thinking about moving this optimization earlier in the frontend, which could then allow inlining functions even if they contain non-inlinable function literals. (So maybe it's actually worthwhile to have versions of the functions both with and without the return x.)
cuonglm
commented
3 years ago To clarify, "same situation" here means that your test program is now successfully passing as well?
Yes, that's it.
gopherbot
commented
3 years ago Change https://golang.org/cl/286656 mentions this issue: [dev.regabi] repro of issue43818
gopherbot
commented
3 years ago Change https://golang.org/cl/288392 mentions this issue: [dev.regabi] test: add a test for inlining closures
danscales
commented
3 years ago Fixed the bug and just added a test case.
What version of Go are you using (
go version)?Does this issue reproduce with the latest release?
On dev.regabi.
What operating system and processor architecture are you using (
go env)?go envOutputWhat did you do?
Was curious to see how the new inlining code would affect my code, so I built
dev.regabiand did ago buildin my repo.What did you expect to see?
Successfully compiling code I can benchmark.
What did you see instead?
ICEs and nil pointer derefs. Packages that repro this are listed in the logs (github.com/robfig/cron/v3, github.com/go-chi/chi/middleware are the smaller ones).
A bisect points to CL 283112, as expected.
/cc @danscales @mdempsky @randall77 (from the CL)