-
Notifications
You must be signed in to change notification settings - Fork 18k
cmd/compile: [dev.regabi] various ICE and SIGSEGV after OCLOSURE export/import/inline CL #43818
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
Comments
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. |
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.
This looks like |
Another ICE, which also breaks real code, but I made it smallest reproduce:
which yields:
|
Change https://golang.org/cl/285676 mentions this issue: |
Change https://golang.org/cl/285992 mentions this issue: |
Change https://golang.org/cl/285677 mentions this issue: |
…or disallowed nodes Several of the bugs in #43818 are because we were not scanning the body of an possibly inlined closure in tooHairy(). I think this scanning got lost in the rebase past some of the ir changes. This fixes the issue related to the SELRECV2 and the bug reported from cuonglm. There is at least one other bug related to escape analysis which I'll fix in another change. Change-Id: I8f38cd12a287881155403bbabbc540ed5fc2248e Reviewed-on: https://go-review.googlesource.com/c/go/+/285676 Trust: Dan Scales <[email protected]> Run-TryBot: Dan Scales <[email protected]> TryBot-Result: Go Bot <[email protected]> Reviewed-by: Matthew Dempsky <[email protected]>
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... 🙂 |
As with CL 285875, this required resolving some conflicts around handling of //go:embed directives. Still further work is needed to reject uses of //go:embed in files that don't import "embed", so this is left as a TODO. (When this code was written for dev.typeparams, we were still leaning towards not requiring the "embed" import.) Also, the recent support for inlining closures (CL 283112) interacts poorly with -G=3 mode. There are some known issues with this code already (#43818), so for now this CL disables inlining of closures when in -G=3 mode with a TODO to revisit this once closure inlining is working fully. Conflicts: - src/cmd/compile/internal/noder/noder.go - src/cmd/compile/internal/typecheck/dcl.go - src/cmd/compile/internal/typecheck/func.go - test/run.go Merge List: + 2021-01-22 7e0a81d [dev.regabi] all: merge master (dab3e5a) into dev.regabi + 2021-01-22 dab3e5a runtime: switch runtime to libc for openbsd/amd64 + 2021-01-22 a1b53d8 cmd/go: add documentation for test and xtest fields output by go list + 2021-01-22 b268b60 runtime: remove pthread_kill/pthread_self for openbsd + 2021-01-22 ec40517 runtime: fix typo in mgcscavenge.go + 2021-01-22 7ece3a7 net/http: fix flaky TestDisableKeepAliveUpgrade + 2021-01-22 50cba05 time: clarify Timer.Reset behavior on AfterFunc Timers + 2021-01-22 cf10e69 doc/go1.16: mention net/http.Transport.GetProxyConnectHeader + 2021-01-22 ec1b945 doc/go1.16: mention path/filepath.WalkDir + 2021-01-22 11def3d doc/go1.16: mention syscall.AllThreadsSyscall + 2021-01-21 07b0235 doc/go1.16: add notes about package-specific fs.FS changes + 2021-01-21 e2b4f1f doc/go1.16: minor formatting fix + 2021-01-21 9f43a9e doc/go1.16: mention new debug/elf constants + 2021-01-21 3c2f11b cmd/go: overwrite program name with full path + 2021-01-21 953d1fe all: introduce and use internal/execabs + 2021-01-21 b186e4d cmd/go: add test case for cgo CC setting + 2021-01-21 5a8a226 cmd/cgo: report exec errors a bit more clearly + 2021-01-21 46e2e2e cmd/go: pass resolved CC, GCCGO to cgo + 2021-01-21 3d40895 runtime: switch openbsd/arm64 to pthreads + 2021-01-21 d95ca91 crypto/elliptic: fix P-224 field reduction + 2021-01-21 d7e71c0 [dev.regabi] cmd/compile: replace ir.Name map with ir.NameSet for dwarf + 2021-01-21 5248f59 [dev.regabi] cmd/compile: replace ir.Name map with ir.NameSet for SSA + 2021-01-21 970d8b6 [dev.regabi] cmd/compile: replace ir.Name map with ir.NameSet in inlining + 2021-01-21 68a4664 [dev.regabi] cmd/compile: remove tempAssigns in walkCall1 + 2021-01-21 fd9a391 [dev.regabi] cmd/compile: remove CallExpr.Rargs + 2021-01-21 19a6db6 [dev.regabi] cmd/compile: make sure mkcall* passed non-nil init + 2021-01-21 9f03684 [dev.regabi] cmd/compile: use ir.DoChildren directly in inlining + 2021-01-21 213c390 [dev.regabi] cmd/compile: use node walked flag to prevent double walk for walkSelect + 2021-01-20 1760d73 [dev.regabi] cmd/compile: exporting, importing, and inlining functions with OCLOSURE + 2021-01-20 ecf4ebf cmd/internal/moddeps: check content of all modules in GOROOT + 2021-01-20 92cb157 [dev.regabi] cmd/compile: late expansion of return values + 2021-01-20 d2d155d runtime: don't adjust timer pp field in timerWaiting status + 2021-01-20 803d18f cmd/go: set Incomplete field on go list output if no files match embed + 2021-01-20 6e243ce cmd/go: have go mod vendor copy embedded files in subdirs + 2021-01-20 be28e5a cmd/go: fix mod_get_fallback test + 2021-01-20 928bda4 runtime: convert openbsd/amd64 locking to libc + 2021-01-19 824f2d6 cmd/go: allow go fmt to complete when embedded file is missing + 2021-01-19 0575e35 cmd/compile: require 'go 1.16' go.mod line for //go:embed + 2021-01-19 9423d50 [dev.regabi] cmd/compile: use '%q' for printing rune values less than 128 + 2021-01-19 ccb2e90 cmd/link: exit before Asmb2 if error + 2021-01-19 ca5774a embed: treat uninitialized FS as empty + 2021-01-19 d047c91 cmd/link,runtime: switch openbsd/amd64 to pthreads + 2021-01-19 61debff runtime: factor out usesLibcall + 2021-01-19 9fed39d runtime: factor out mStackIsSystemAllocated + 2021-01-19 a2f825c [dev.regabi] cmd/compile: directly create go.map and go.track symbols + 2021-01-19 4a4212c [dev.regabi] cmd/compile: refactor Linksym creation + 2021-01-19 4f5c603 [dev.regabi] cmd/compile: cleanup callTargetLSym + 2021-01-18 dbab079 runtime: free Windows event handles after last lock is dropped + 2021-01-18 5a8fbb0 os: do not close syscall.Stdin in TestReadStdin + 2021-01-18 422f38f [dev.regabi] cmd/compile: move stack objects to liveness + 2021-01-18 6113db0 [dev.regabi] cmd/compile: convert OPANIC argument to interface{} during typecheck + 2021-01-18 4c835f9 [dev.regabi] cmd/compile: use LinksymOffsetExpr in TypePtr/ItabAddr + 2021-01-18 0ffa1ea [dev.regabi] cmd/compile: use *obj.LSym instead of *ir.Name for staticdata functions + 2021-01-17 7e0fa38 [dev.regabi] cmd/compile: remove unneeded packages from ir.Pkgs + 2021-01-17 99a5db1 [dev.regabi] cmd/compile: use LinksymOffsetExpr in walkConvInterface + 2021-01-17 87845d1 [dev.regabi] cmd/compile: add ir.TailCallStmt + 2021-01-17 e3027c6 [dev.regabi] cmd/compile: fix linux-amd64-noopt builder + 2021-01-17 59ff93f [dev.regabi] cmd/compile: rename NameOffsetExpr to LinksymOffsetExpr + 2021-01-17 82b9cae [dev.regabi] cmd/compile: change ir.NameOffsetExpr to use *obj.LSym instead of *Name + 2021-01-17 88956fc [dev.regabi] cmd/compile: stop analyze NameOffsetExpr.Name_ in escape analysis + 2021-01-17 7ce2a83 [dev.regabi] cmd/compile: simplify stack temp initialization + 2021-01-17 ba0e8a9 [dev.regabi] cmd/compile: refactor temp construction in walk + 2021-01-17 78e5aab [dev.regabi] cmd/compile: replace Node.HasCall with walk.mayCall + 2021-01-16 6de9423 [dev.regabi] cmd/compile: cleanup OAS2FUNC ordering + 2021-01-16 a956a0e [dev.regabi] cmd/compile, runtime: fix up comments/error messages from recent renames + 2021-01-16 ab3b67a [dev.regabi] cmd/compile: remove ONEWOBJ + 2021-01-16 c9b1445 [dev.regabi] cmd/compile: remove TypeAssertExpr {Src,Dst}Type fields + 2021-01-15 682a1d2 runtime: detect errors in DuplicateHandle + 2021-01-15 9f83418 cmd/link: remove GOROOT write in TestBuildForTvOS + 2021-01-15 ec94701 cmd/compile: allow embed into any string or byte slice type + 2021-01-15 54198b0 cmd/compile: disallow embed of var inside func + 2021-01-15 b386c73 cmd/go: fix go generate docs + 2021-01-15 bb5075a syscall: remove RtlGenRandom and move it into internal/syscall + 2021-01-15 1deae0b os: invoke processKiller synchronously in testKillProcess + 2021-01-15 03a8751 [dev.regabi] cmd/compile: unexport reflectdata.WriteType + 2021-01-15 14537e6 [dev.regabi] cmd/compile: move stkobj symbol generation to SSA + 2021-01-15 ab523fc [dev.regabi] cmd/compile: don't promote Byval CaptureVars if Addrtaken + 2021-01-15 ff196c3 crypto/x509: update iOS bundled roots to version 55188.40.9 + 2021-01-15 b7a698c [dev.regabi] test: disable test on windows because expected contains path separators. + 2021-01-15 4be7af2 [dev.regabi] cmd/compile: fix ICE during ir.Dump + 2021-01-14 e125ccd cmd/go: in 'go mod edit', validate versions given to -retract and -exclude + 2021-01-14 eb33002 cmd/dist, cmd/go: pass -arch for C compilation on Darwin + 2021-01-14 84e8a06 cmd/cgo: remove unnecessary space in cgo export header + 2021-01-14 0c86b99 cmd/test2json: document passing -test.paniconexit0 + 2021-01-14 9135795 cmd/go/internal/load: report positions for embed errors + 2021-01-14 35b9c66 [dev.regabi] cmd/compile,cmd/link: additional code review suggestions for CL 270863 + 2021-01-14 d9b79e5 cmd/compile: fix wrong complement for arm64 floating-point comparisons + 2021-01-14 c73232d cmd/go/internal/load: refactor setErrorPos to PackageError.setPos + 2021-01-14 6aa28d3 go/build: report positions for go:embed directives + 2021-01-14 9734fd4 [dev.regabi] cmd/compile: use node walked flag to prevent double walk for walkSwitch + 2021-01-14 f979832 [dev.regabi] cmd/compile: move more PAUTOHEAP to SSA construction + 2021-01-14 4476300 [dev.regabi] cmd/compile: use byte for CallExpr.Use + 2021-01-14 5a5ab24 [dev.regabi] cmd/compile: do not rely on CallExpr.Rargs for detect already walked calls + 2021-01-14 983ac4b [dev.regabi] cmd/compile: fix ICE when initializing blank vars + 2021-01-13 7eb31d9 cmd/go: add hints to more missing sum error messages + 2021-01-13 d6d4673 [dev.regabi] cmd/compile: fix GOEXPERIMENT=regabi builder + 2021-01-13 c41b999 [dev.regabi] cmd/compile: refactor abiutils from "gc" into new "abi" + 2021-01-13 861707a [dev.regabi] cmd/compile: added limited //go:registerparams pragma for new ABI dev + 2021-01-13 c1370e9 [dev.regabi] cmd/compile: add code to support register ABI spills around morestack calls + 2021-01-13 2abd24f [dev.regabi] test: make run.go error messages slightly more informative + 2021-01-13 9a19481 [dev.regabi] cmd/compile: make ordering for InvertFlags more stable + 2021-01-12 ba76567 cmd/go/internal/modload: delete unused *mvsReqs.next method + 2021-01-12 665def2 encoding/asn1: document unmarshaling behavior for IMPLICIT string fields + 2021-01-11 81ea89a cmd/go: fix non-script staleness checks interacting badly with GOFLAGS + 2021-01-11 7593090 doc: update editors.html for Go 1.16 + 2021-01-11 c3b4c70 cmd/internal/objfile: don't require runtime.symtab symbol for XCOFF + 2021-01-08 59bfc18 cmd/go: add hint to read 'go help vcs' to GOVCS errors + 2021-01-08 cd6f3a5 cmd/go: revise 'go help' documentation for modules + 2021-01-08 6192b98 cmd/go: make hints in error messages more consistent + 2021-01-08 25886cf cmd/go: preserve sums for indirect deps fetched by 'go mod download' + 2021-01-08 6250833 runtime/metrics: mark histogram metrics as cumulative + 2021-01-08 8f6a9ac runtime/metrics: remove unused StopTheWorld Description field + 2021-01-08 6598c65 cmd/compile: fix exponential-time init-cycle reporting + 2021-01-08 fefad1d test: fix timeout code for invoking compiler + 2021-01-08 6728118 cmd/go: pass signals forward during "go tool" + 2021-01-08 e65c543 go/build/constraint: add parser for build tag constraint expressions + 2021-01-08 0c5afc4 testing/fstest,os: clarify racy behavior of TestFS + 2021-01-08 32afcc9 runtime/metrics: change unit on *-by-size metrics to match bucket unit + 2021-01-08 c6513bc io/fs: minor corrections to Glob doc + 2021-01-08 304f769 cmd/compile: don't short-circuit copies whose source is volatile + 2021-01-08 ae97717 runtime,runtime/metrics: use explicit histogram boundaries + 2021-01-08 a9ccd2d go/build: skip string literal while findEmbed + 2021-01-08 d92f8ad archive/tar: fix typo in comment + 2021-01-08 cab1202 cmd/link: accept extra blocks in TestFallocate + 2021-01-08 ee4d322 io/fs: minor corrections to Glob release date + 2021-01-08 54bd1cc cmd: update to latest golang.org/x/tools + 2021-01-07 9ec21a8 Revert "reflect: support multiple keys in struct tags" + 2021-01-07 091414b io/fs: correct WalkDirFunc documentation + 2021-01-07 9b55088 doc/go1.16: add release note for disallowing non-ASCII import paths + 2021-01-07 fa90aac cmd/compile: fix late expand_calls leaf type for OpStructSelect/OpArraySelect + 2021-01-07 7cee66d cmd/go: add documentation for Embed fields in go list output + 2021-01-07 e60cffa html/template: attach functions to namespace + 2021-01-07 6da2d3b cmd/link: fix typo in asm.go + 2021-01-07 df81a15 runtime: check mips64 VDSO clock_gettime return code + 2021-01-06 4787e90 crypto/x509: rollback new CertificateRequest fields + 2021-01-06 c9658be cmd/go: make module suggestion more friendly + 2021-01-06 4c668b2 runtime/metrics: fix panic message for Float64Histogram + 2021-01-06 d213170 net/http/httputil: fix deadlock in DumpRequestOut + 2021-01-05 3e1e13c cmd/go: set cfg.BuildMod to "readonly" by default with no module root + 2021-01-05 0b0d004 cmd/go: pass embedcfg to gccgo if supported + 2021-01-05 1b85e7c cmd/go: don't scan gccgo standard library packages for imports + 2021-01-05 6b37b15 runtime: don't take allglock in tracebackothers + 2021-01-04 9eef49c math/rand: fix typo in comment + 2021-01-04 b01fb2a testing/fstest: fix typo in error message + 2021-01-01 3dd5867 doc: 2021 is the Year of the Gopher + 2020-12-31 95ce805 io/fs: remove darwin/arm64 special condition + 2020-12-30 20d0991 lib/time, time/tzdata: update tzdata to 2020f + 2020-12-30 ed30173 misc/cgo/testcarchive: remove special flags for Darwin/ARM + 2020-12-30 0ae2e03 misc/cgo/test: enable TestCrossPackageTests on darwin/arm64 + 2020-12-29 780b4de misc/ios: fix wording for command line instructions + 2020-12-29 b4a71c9 doc/go1.16: reference misc/ios/README for how to build iOS programs + 2020-12-29 f83e0f6 misc/ios: add to README how to build ios executables + 2020-12-28 4fd9455 io/fs: fix typo in comment Change-Id: If24bb93f1e1e7deb1d92ba223c85940ab93b2732
In reflect.methodWrapper, we call escape analysis without including the full batch of dependent functions, including the closure functions. Because of this, we haven't created locations for the params/local variables of a closure when we are processing a function that inlines that closure. (Whereas in the normal compilation of the function, we do call with the full batch.) To deal with this, I am creating locations for the params/local variables of a closure when needed. Without this fix, the new test closure6.go would fail. Updates #43818 Change-Id: I5f91cfb6f35efe2937ef88cbcc468e403e0da9ad Reviewed-on: https://go-review.googlesource.com/c/go/+/285677 Run-TryBot: Dan Scales <[email protected]> TryBot-Result: Go Bot <[email protected]> Trust: Dan Scales <[email protected]> Reviewed-by: Matthew Dempsky <[email protected]>
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). |
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:
A handful of reasonable candidate function bodies would be things like Also, FYI, the |
Yes, that's it. |
Change https://golang.org/cl/286656 mentions this issue: |
Change https://golang.org/cl/288392 mentions this issue: |
Add a test case for issue 43818. We don't want to mark as inlinable a function with a closure that has an operation (such as OSELRECV2) that we don't currently support for exporting. This test case fails to compile without the fix for #43818. Updates #43818 Change-Id: Ief322a14aefaefc6913c40a6b8505214bd622fda Reviewed-on: https://go-review.googlesource.com/c/go/+/288392 Run-TryBot: Dan Scales <[email protected]> Reviewed-by: Cuong Manh Le <[email protected]> TryBot-Result: Go Bot <[email protected]> Trust: Dan Scales <[email protected]>
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 env
OutputWhat did you do?
Was curious to see how the new inlining code would affect my code, so I built
dev.regabi
and did ago build
in 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)
The text was updated successfully, but these errors were encountered: