cmd/compile: closure func name changes when inlining

[rsc]

% cat x.go
package main

func f(x int) func() {
	return func() {
		panic(x)
	}
}

func main() {
	f(1)()
}
% go run -gcflags=-l x.go
panic: 1

goroutine 1 [running]:
main.f.func1()
	/private/tmp/x.go:5 +0x26
main.main()
	/private/tmp/x.go:10 +0x22
exit status 2
% go run x.go
panic: 1

goroutine 1 [running]:
main.main.func1(...)
	/private/tmp/x.go:5
main.main()
	/private/tmp/x.go:10 +0x28
exit status 2
% 

Without inlining, the stack shows main.f.func1 panicking, which is approximately correct: it's the 1st closure inside main.f.

With inlining, the stack instead shows main.main.func1, because now it's the 1st closure inside main.main due to inlining of f. However, that's a much less useful name, since the closure is still lexically inside main.f. If there is some way to preserve the old name even when inlining, that would be a good idea.

This is the source of a Kubernetes test failure using Go 1.21, because a more complicated function was not being inlined in Go 1.20 but is being inlined in Go 1.21 after CL 492017. The test expected to see "newHandler" on the stack when a closure lexically inside newHandler panicked.

If there's something we can do here, we probably should, both to keep the closure names helpful and to keep tests like the Kubernetes one passing.

[thediveo]

not only k8s, but for instance leaky go routine tests may fail b/c whitelisting doesn't work correctly anymore.

[thanm]

Hi Austin, since you are the original author of CL 479095, assigning this bug to you. If you would prefer that I work on this particular cleanup, please assign back to me. Thanks, Than.

[aarzilli]

This was noticed before: #55980

[mdempsky]

If there is some way to preserve the old name even when inlining, that would be a good idea.

Is it okay if we just arrange that the runtime.Func for the inlined closure reports the original closure's link symbol name, instead of the duplicate's own name?

[rsc]

I don't think I understand the suggestion, but we want the inlining-disabled name to appear in profiles, stack traces, and symbol tables. I don't think changing just runtime.Func will do that.

[rsc]

For what it's worth, we have the name of the lexically enclosing function at the point of the closure creation, because if the program crashed at that PC we would show that frame in the stack trace marked [inlined].

[mdempsky]

Yes, it's easy to track the original closure symbol name. That's not a problem.

What I'm trying to understand is what you want done with that.

We currently duplicate the closure and underlying function text, because they can be optimized differently due to different escape analysis and variable capture. So the duplicated closure needs its own linker symbol.

[rsc]

Does it? I thought the linker addressed all the symbols by table index now, so if we made it a symbol internal to the package being compiled, it would not collide with any others with the same name.

[mdempsky]

What I'm trying to understand is what you want done with that.

[rsc]

I am interpreting the previous comment as my not having answered your questions, so I will elaborate a bit here.

The function names reported in profiles, stack traces, symbol tables, disassembly, and so on should not depend on how aggressively we inline. That is, the names that we get with no inlining at all are the "official" names, and inlining should preserve them.

I understand that, in terms of the example at the top, if we do:

func main() {
    f(1)()
    f(2)()
    f(3)()
}

that there will be three copies of the actual text symbol generated when f is inlined, and that the texts may actually differ across the 3 calls. But I think the new linker's ability to address symbols by table index instead of name should mean that it's completely fine to have those three text symbols all use the same name: the original name they'd have used without inlining.

[mdempsky]

@cherrymui How should the compiler create multiple obj.LSyms that are kept separate but all end up with the same linkname in the symbol tables?

[rsc]

As background, we originally named closures opaque names like 'func•1', but that was extremely unhelpful in all the places I mentioned where function names appear, not to mention in all tools that didn't support UTF-8. Issue #8291 was to give the closures more useful names, which Dmitri did in CL 3964. Inlining of functions containing closure creation is essentially a regression of that issue. It may seem like they just need a name, any name, but that's not true. The names carry useful information that has been lost since unified IR the compiler started inlining these kinds of functions. I'm not just being picky.

[mdempsky]

The names carry useful information that has been lost since unified IR started inlining these kinds of functions.

Please retract that accusation. The stack trace for your test program is the same in Go 1.19 as in Go 1.20: https://go.dev/play/p/yjP9hrxZ_K0?v=goprev

[rsc]

It was not an accusation, but I apologize nonetheless. My memory was that one of unified IR's important inlining advances was to enable inlining of closure-containing functions. I misremembered - we started inlining those in Go 1.17, so obviously that must have been with the original IR. In any event, as unified IR has made it possible to do more aggressive inlining, we lose more and more of this information. This is not a criticism of unified IR. My point is only to explain the context of how we got here. All the changes individually make sense, but this is a rough edge where they aren't quite working well enough together.

[mdempsky]

I apologize nonetheless.

Thank you.

but this is a rough edge where they aren't quite working well enough together.

Ack. That's what I'm trying to understand: how should this work?

As I've said already, we need multiple linker symbols (i.e., obj.LSyms), because in general the inlined closure can get optimized differently than the original closure.

You suggest the multiple obj.LSyms should just have identical names in the symbol tables, etc (i.e., use the same linknames). That's not something we do anywhere else in the compiler to my knowledge, so I don't know how to invoke cmd/internal/obj to make that happen. Hence why I asked @cherrymui how to do that.

However, I remember she's on vacation, so if any other linker experts can advise on this, that would be great. @golang/compiler

[rsc]

I think it would be good enough for Go 1.21 if we embed the inlining stack in the name (eliding package names), so for example if F inlines f inlines g inlines h creates a closure, the function would be named F.f.g.h.func1. As long as the same counter is used for all the funcs, it won't matter if that sequences appears again: it would get a different trailing number.

So if you had

func F() {
    f(1)()  // F inlines f inlines g inlines h creates a closure
    g(2)() // F inlines g inlines h creates a closure
    h(3)() // F inlines h creates a closure
    f(4)() // F inlines f inlines g inlines h creates a closure
}

The four text symbols created in main would be named F.f.g.h.func1, F.g.h.func2, F.h.func3, and F.f.g.h.func4. That's not ideal, since in general the package name might be important, but it should be enough breadcrumbs to help people and tools and tests. And then if we find out how to do the duplicate text symbol thing in the linker, we can adjust for Go 1.22.

[mdempsky]

I think it would be good enough for Go 1.21 if we embed the inlining stack in the name (eliding package names)

Thanks, that seems doable for 1.21.

[prattmic]

I agree that we should improve the names of inlined closures and the suggestion in #60324 (comment) seems like a reasonable short-term compromise.

If that is easy to do at the end of the cycle, great, but I'd push back a bit on this being a release-blocking issue that needs an immediate fix right before the freeze. We know that this is an existing issue that gets more annoying as we are more aggressive with inlining closures, but are we actually that much aggressive than before?

CL 479095 states that it increases inlinability by +0.6%. It doesn't say how much it increases inlinability of closures specifically, which is probably the more important metric. But at first glance, it looks like we got unlucky with one specific test rather than us getting sharply more aggressive and causing widespread issues.

[gopherbot]

Change https://go.dev/cl/497137 mentions this issue: cmd/compile: incorporate inlined function names into closure naming

[mdempsky]

Russ's suggested naming change ("main.main.func1" -> "main.main.f.func1") has been implemented and submitted for Go 1.21.

I'm happy to implement further changes for Go 1.22, pending advice from the linker devs on how to best approach that.

[thanm]

Thanks Matthew.

[aclements]

Do we want the symbols to have the non-unique names? There's a tension: we go to some effort to make runtime tracebacks reflect the language, not the implementation, which suggests those should show the "original" name of the closure, even if that's non-unique; but at the same time developers sometimes need to map that to an ELF/etc symbol, in which case you really want them to be unique (the PC offset traceback prints doesn't do you much good without a unique symbol name!).

We could change how we show closures and/or inlining in runtime tracebacks. Is there just a nicer way to present this information? I don't have any answers, but maybe thinking about that bigger picture will inspire someone. 🙂

[mdempsky]

One possible solution would be to ensure the closure is never optimized differently when inlined, so we can just always reuse the same underlying function all the time.

With how things are currently implemented, it's certainly possible that the closure gets optimized differently. But I don't have an example off hand to demonstrate that we do.

[rsc]

Thanks for the CL @mdempsky.

I've confirmed that the fix changes the stack frame in question in the failing Kubernetes test from
k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/server/filters.TestTimeout.func5
to being named
k8s.io/kubernetes/vendor/k8s.io/apiserver/pkg/server/filters.TestTimeout.newHandler.func5
which makes the test (which wanted to see 'newHandler') pass again.

I've changed this to the Go 1.22 milestone, and I agree with @austin that there is a question about whether we should stop at these not-unique names.

It was a release blocker for Go 1.21 because there was no way to test for the presence of a function on a call stack (in, for example, a test of panic behavior) that worked reliably across changes to the amount of inlining. That was breaking real tests with no obvious answer for how to rewrite them to be more robust. Now we have the guarantee that the lexically enclosing function name is preserved. Yes, it was not a new bug in Go 1.21 (as we established, it first started in Go 1.17), but Go 1.21 changes made the problem worse. I appreciate the fix.