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

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.

This was noticed before: #55980

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?

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.

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].

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.

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.

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

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.

@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?

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.

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

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.

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

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.

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.

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.

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

If we could find a less special special character, I think this would be a nice improvement to the status quo.

@aarzilli Thanks, @cherrymui reminded me of that privately. I'm happy for an alternative character if anyone has suggestions. ("foo@bar" just seemed the instinctual suggestion for suggesting that foo was inlined at bar.)

My personal opinion is that (aside from the @ issue) the inlining breadcrumbs don't carry enough value to justify the noise they introduce. I think this suggestion is the better one:

I suggest instead that we name the inlined closures pkg.h.func1@F.1, pkg.h.func1@F.2, pkg.h.func1@F.3, and pkg.h.func1@F.4.

A few other suggestions I want to make, as long as we're discussing it:

1. I'd like to adopt the naming that x/tools/go/ssa uses instead. That is, instead of p.F.func1, p.F.func2.3, etc, the anonymous functions are p.F$1, p.F$2$3, etc. This is shorter, uniform, easier to recognize as anonymous functions at a glance (IMO), and also easier to reason about whether the resulting mangled names remain collision free.
   Note: A consequence of go.dev/cl/522318, is that function literals that appear in package-scope variable initialization expressions have had their name change from p.glob..func1 to p.init.func1. So these would now become p.init$1.
2. When a function literal is assigned to a variable (e.g., helper := func() { ... } or _, helper, _ = ..., func() { ... }, ...), then we instead name it p.F$helper. If it's necessary to disambiguate function literals given the same name this way, they'll be disambiguated p.F$helper#1, p.F$helper#2, etc. (This is inspired by x/tools/go/ssa naming explicit init functions p.init#1, p.init#2, etc, instead of p.init.1, p.init.2, etc.)
3. I'm leaning towards naming the inlined anonymous functions like p.F$1[X], p.F$2$3[X], p.F$helper[X], etc., where [X] exists to disambiguate the anonymous function across inlined call sites.
   Note: If anonymous function p.F$2 contains p.F$2$3 and is inlined at X, the functions will be named p.F$2[X] and p.F$2[X]$3, respectively. If p.F$2[X]$3 (or logically equivalently just p.F$2$3) is inlined at Y, it will be named simply p.F$2$3[Y]. There's no need for multiple disambiguators (e.g., p.F$2[X]$3[Y]).
   Edit: Actually, I think the inlining disambiguator can and should just always go at the end. There's no need for users to worry about both p.F$2[X]$3 and p.F$2$3[Y]. It's not much extra trouble for the compiler to ensure X and Y are disjoint sets of strings.

The exact numbers used (i.e., for p.F$1 or p.F$helper#1) or the inlining disambiguators (i.e., for p.F$1[X], including whether they're used at all) continue to be explicitly unspecified. I could be convinced though to guarantee the numbers correspond to source order (i.e., stable under optimizations like deadcode elimination).

Looking through Google's internal Go code base, there's some code that looks for the current \.func\d+ patterns, but it seems not a lot. I'd be happy to put the new naming behind a GOEXPERIMENT and shipping it disabled for a release to ease migration.

Note: gccgo already uses a completely different naming scheme for anonymous functions.

not exactly thrilled when closure func names change (again), because in the Gomega go routine leak checker (https://github.com/onsi/gomega/blob/55a33f3ff7b97c9f999d641bb6f92621d1c2edcf/gleak/have_leaked_matcher.go#L44) we need to maintain an ignore list of known background go routines from stdlib and testing. I think that we luckily have avoided hardcoding closure funcs using ... in the ignores. However, changes discussed here always keep me wary...

I'm leaning towards naming the inlined anonymous functions like p.F$1[X], p.F$2$3[X], p.F$helper[X], etc., where [X] exists to disambiguate the anonymous function across inlined call sites.

@mdempsky , just to clarify, the p.F$1 part is from the non-inlined name of the closure, and the X part serves to disambiguate inlining site?

I always worry about what assumptions linkers and tools make about what characters are allowed in symbol names. 😅

Gomega go routine leak checker

@thediveo , thanks for pointing that out. If we implement @mdempsky's ideas, it looks like the one way Gomega's matcher will break is that ... matches "a . followed by anything" and @mdempsky's proposed naming no longer includes a . after the base function/method name.

I'm concerned about the user experience for non-expert Go programmers.

This will add churn to the overall user experience, as every change here does. Do we have evidence that the changes made for Go 1.21 aren't good enough? It seems like a change here is only worthwhile if it is obviously dramatically better or it addresses something in the status quo that is demonstrably not good enough. I don't see either one of those here.

The go/ssa names seem strictly worse than what we have today. From the point of view of someone not familiar with the ins and outs of the compiler, it seems guessable that F.func1 is the first func in F. In contrast, F$1 seems like cryptic line noise (or Perl syntax) to me.

If the func is assigned to a unique variable g, then F.g seems very clear too. I don't see any reason to introduce a new delimiter $ like in F$g. I also don't think I understand what [X] means but p.F$2$3[X] seems like a complete non-starter to me. It seems even more like cryptic line noise (or Perl syntax) than F$1.

The transitive inlining "breadcrumbs" within a package seem important to me to preserve as well. They're much more readable than an arbitrary numbering.

The choice of delimiter matters too. If p.F.g is inlined into H and from there inlined into J and we want a symbol of the form p.F.g(delim)H(delim)J, then we need a delimiter that suggests "in". @ is pretty good for that: p.F.g at the use in H at the use in J. Are we sure that C toolchains will break if we use @ here? The symbols would not be exported in any way, so I would hope a C linker would leave them alone.

If @ is off the table, then > might be good, as it suggests injection; p.F.g>H>K.

• line noise ... then it most probably is a valid TECO command.
• would exposing the names of local variables be a problem? what in case the func is returned immediately, so there's no name to associate with?

One thing to keep in mind is that, I think, the [X] breaks any parser for symbol names that has been updated to work with generics, including debug/gosym.

This will add churn to the overall user experience, as every change here does. Do we have evidence that the changes made for Go 1.21 aren't good enough?

One thing is that the .funcN convention has always been ambiguous (is pkg.A.func1 a closure in function pkg.A or is it a method of type pkg.A?).

It would be nice if a delimiter other than . was used here but this is also at odds with backwards compatibility.

Another thing is that the inlining breadcrumbs aren't all that user friendly either, I think. If I didn't know about this thread pkg.A.B.func3 would make me look at pkg.A when I should really be looking at B (possibly in a different package?).

unless you name all your packages pkg, A, B, etc., it usually becomes quite obvious where the package in the name is, more so as it is an import path with github.com, or some other TLD.

Are we sure that C toolchains will break if we use @ here?

In an ELF object file, the @ character is used to separate a symbol name from the symbol version. So, yes, you're going to get odd behavior if you pass an object file with symbol names containing '@' characters to an ELF linker.

Do you think renaming a.F.func1 to b.G.F.func2 is "good enough" for Gomega? Do you think changing it to a.F.func1+suffix would be "obviously dramatically better"?

ad 1. should work as we would add the new pattern next to the existing one, both coexisting without interference.

ad 2. I lived halfway happily with the existing scheme but hadn't an urge for longer symbol names.

@ianlancetaylor

In an ELF object file, the @ character is used to separate a symbol name from the symbol version. So, yes, you're going to get odd behavior if you pass an object file with symbol names containing '@' characters to an ELF linker.

I understand that, but I also noted "The symbols would not be exported in any way, so I would hope a C linker would leave them alone." That is, I think that the vast majority of our symbols are or can be generated as STB_LOCAL, and in particular any closure functions can definitely be set STB_LOCAL. If we use the @ syntax in STB_LOCAL names, will that still affect ELF linkers?

Technically you're right that an ELF linker shouldn't care about @ in a local symbol name. But it seems like a bad idea to count on such symbols being handled cleanly by the various ELF linkers and by the various ELF tools in general. I would only try that if it were fairly important to use @. None of the @ support is standardized or seriously documented (Sun introduced symbol versioning, and they documented it well, but the @ support is a GNU extension that lets people skip writing actual version definition files).

If you really want the @ then an option would be to only use the @ in the debug info, and use a mangled name in the ELF symbol table.

Sounds good, thanks for the extra details.

One thing is that the .funcN convention has always been ambiguous (is pkg.A.func1 a closure in function pkg.A or is it a method of type pkg.A?).

That's not ambiguity, because pkg.A can only be one of those two things.
We don't say that pkg.A itself is ambiguous because it might be a function or it might be a type.
We assume it's okay to need to know what kind of thing A is.
And if we know what kind of thing A is, then we know what kind of thing A.func1 is.

Another thing is that the inlining breadcrumbs aren't all that user friendly either, I think. If I didn't know about this thread pkg.A.B.func3 would make me look at pkg.A when I should really be looking at B (possibly in a different package?).

I agree with Matthew's observation that it would be more helpful to reverse the breadcrumbs. My point was that if we have F inlined into G inlined into H as well as F inlined into J inlined into K, it is much more helpful to name them with some representation that includes (F,G,H) and (F,J,K) than it is to name them F$1 and F$2.

That's not ambiguity, because pkg.A can only be one of those two things.
We don't say that pkg.A itself is ambiguous because it might be a function or it might be a type.
We assume it's okay to need to know what kind of thing A is.
And if we know what kind of thing A is, then we know what kind of thing A.func1 is.

Ok, but for example debug/gosym doesn't know that and gets them wrong.

@aarzilli Do you mean just debug/gosym.Sym.BaseName, or other code in debug/gosym too? What do you use BaseName for?

If we were to make the inlining breadcrumbs unambiguously parseable from the symbol name, then in a traceback, we could print only the "original" name by default, and include the full name if and only if we're showing full PCs in the traceback (I'm thinking as additional information at the end of the line, not replacing the original name). That seems to be the exact case where the actual symbol name may matter. Perhaps we suppress the PC offset if we're only printing the original name to avoid confusion. This is a more refined version of the point I made in #60324 (comment).

@aarzilli Do you mean just debug/gosym.Sym.BaseName, or other code in debug/gosym too? What do you use BaseName for?

BaseName and ReceiverName. Delve doesn't use debug/gosym, exactly, but it has its own equivalent reimplementation. It uses it to find functions with a partial name match (for example to set a breakpoint). It has never needed to work correctly with closures, so far, but it would be nice to know that it could.

Looking around with sourcegraph I can't find many users of debug/gosym, but for example GoRE could be affected by this problem as well. Hard to say if there are other half-reimplementations floating around that are affected.

Personally, as a user of go, I agree that $ is too noisy as a delimiter but , or - are also unambiguous and not too noisy. I don't see the value of the inlining breadcrumbs at all.

It occurs to me that this case of inlining is just one example of function multicompilation/multiversioning. In this case, we're compiling multiple versions of the function because they may be optimized differently in different inlining contexts. But this is a technique we've considered for other things, like multiversioning for escape analysis and for access to CPU features that are detected at runtime.

In fact, there's another case where we do this already: GC shape stenciling. We had to disambiguate the symbols in that case, too. But in that case we decided to hide the disambiguator in tracebacks and only show the "user" name. I think that was the right choice (though we still show PCs, which might be a mistake 😅). In that case, the disambiguator was pretty meaningless to users, and I could see an argument that the inlining breadcrumbs are more meaningful, but I'm not sure I buy that these cases are so different.

It occurs to me that this case of inlining is just one example of function multicompilation/multiversioning.

Aren't generics the same? So maybe it would be neat to include in stack traces also the concrete type for which a generic function got compiled?

Aren't generics the same?

Sorry, that's exactly what I meant by "GC shape stenciling" (I can see how that wouldn't be obvious!)

The thing is, the symbol name doesn't include the concrete type for which a generic function got instantiated. What it includes is much more abstract and not particularly helpful to the user.

Hi just to weigh in on this comment by @mdempsky #60324 (comment).

It also makes it very difficult for tools to correlate these symbols

I agree to the whole comment because I'm writing such tool within the IDE, and it is sometimes not possible to correctly resolve the symbol there. Stack traces in particular have the file name, but this might be transformed (trimmed, or else, e.g. Bazel does that with the default ruleset).

Having a marker to identify inlines so it's possible to handle them (maybe skip them) could help.