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

Jump to bottom

cmd/compile: unnamed functions missing FuncInfo #54959

Closed
prattmic opened this issue Sep 8, 2022 · 5 comments
Closed

cmd/compile: unnamed functions missing FuncInfo #54959

prattmic opened this issue Sep 8, 2022 · 5 comments
Assignees
@prattmic
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge
Milestone
Backlog

Comments

@prattmic
Copy link
Member

prattmic commented Sep 8, 2022

Context: I am adding additional information to runtime.inlinedCall (function start line), which I am plumbing via the FuncInfo. In the linker, this FuncInfo.Valid() got me thinking that (IMO) inlined functions ought to always have a FuncInfo, since they must be Go functions, so I switched the check to panic if it is missing.

This program fails that check:

package main

func main() {
        var i int
        fn := func() { i++ }
        fn()

        func() { i++ }()
}
panic: inlined function main.main.func2 missing func info

goroutine 1 [running]:
cmd/link/internal/ld.genInlTreeSym(0xc000510000?, 0x200000010?, {0xc000510000, 0xc000166080, {0xc00016888d, 0x48, 0x48}, {0x1, 0x10, 0x2, ...}}, ...)
        /usr/local/google/home/mpratt/src/go/src/cmd/link/internal/ld/pcln.go:172 +0x419
cmd/link/internal/ld.makeInlSyms(0xc000158000, {0xc000a5a000, 0x40c, 0x40c?}, 0x20?)
        /usr/local/google/home/mpratt/src/go/src/cmd/link/internal/ld/pcln.go:194 +0x245
cmd/link/internal/ld.(*Link).pclntab(0xc000158000, {0xc000105900?, 0x1?, 0xf?})
        /usr/local/google/home/mpratt/src/go/src/cmd/link/internal/ld/pcln.go:786 +0x1e6
cmd/link/internal/ld.Main(_, {0x20, 0x20, 0x1, 0x7, 0x10, 0x0, {0x0, 0x0}, {0x6886dd, ...}, ...})
        /usr/local/google/home/mpratt/src/go/src/cmd/link/internal/ld/main.go:328 +0x144a
main.main()
        /usr/local/google/home/mpratt/src/go/src/cmd/link/main.go:72 +0xedb

The first closure (fn) has a FuncInfo, but the second does not. This seems like a bug to me, though it probably had low impact before because FuncID would default to FuncID_normal anyways.

I am still digging, but it looks like this is somewhere in the compiler, as func2 never makes it to ssagen.InitLSym, which sets up the FuncInfo.

cc @golang/compiler @cherrymui
@prattmic prattmic added this to the Backlog milestone Sep 8, 2022
@prattmic prattmic self-assigned this Sep 8, 2022
@gopherbot gopherbot added the compiler/runtime Issues related to the Go compiler and/or runtime. label Sep 8, 2022
@prattmic
Copy link
Member Author

prattmic commented Sep 8, 2022 •
edited

Note to self: enqueueFunc skips both func1 and func2 because they aren't trivial closures. When enqueueing main.main, the later loop prepares func1, but for some reason func2 never ends up in next.Closures.

@mdempsky
Copy link
Member

mdempsky commented Sep 8, 2022

@prattmic Without looking too closely, I think it's because the directly called closure gets inlined and then we know there won't be any other references to it, so we intentionally drop references to it from the AST because we know we don't need to compile its function body on its own. (Contrast that the indirectly called closure gets inlined too, but we don't do any frontend analysis to realize that was the only possible use.)

@cuonglm
Copy link
Member

cuonglm commented Sep 9, 2022 •
edited

The place where the direct call closure is replaced: https://cs.opensource.google/go/go/+/refs/tags/go1.19.1:src/cmd/compile/internal/inline/inl.go;l=596

Maybe we can mark it as need lsym and call ssagen.IniLSym in enqueueFunc if the closure was marked.

@gopherbot
Copy link

Change https://go.dev/cl/429637 mentions this issue: cmd/link/internal/ld: panic if inlined functions missing funcinfo

@prattmic prattmic mentioned this issue Sep 29, 2022
Open
@gopherbot
Copy link

Change https://go.dev/cl/436240 mentions this issue: cmd/compile: eagerly create LSym for closures

gopherbot pushed a commit that referenced this issue Sep 30, 2022
@prattmic
cmd/link/internal/ld: panic if inlined functions missing FuncInfo
8a0cf71 
All inlined functions are Go functions, and thus should be capable of
having a FuncInfo. Missing FuncInfo is likely indication of a compiler
bug that dropped the symbol too early, failing to add it to the symbol
list used for writing output. I believe all existing cases have been
fixed; this check will prevent regressions.

The exception is -linkshared mode. There symbols are loaded from the
shared library, and the FuncInfo is not available. This is a bug, as it
can result in incorrect the FuncID in inlinedCall, but it is very
involved to fix.

For #54959.
For #55954.

Change-Id: Ib0dc4f1ea62525b55f68604d6013ff33223fdcdd
Reviewed-on: https://go-review.googlesource.com/c/go/+/429637
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Michael Pratt <mpratt@google.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
@golang golang locked and limited conversation to collaborators Sep 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@prattmic prattmic

Labels
compiler/runtime Issues related to the Go compiler and/or runtime. FrozenDueToAge
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
4 participants
@mdempsky @prattmic @cuonglm @gopherbot