From: Matthew Dempsky Date: Sat, 9 Sep 2023 01:04:31 +0000 (-0700) Subject: cmd/compile/internal/typecheck: fix closure field naming X-Git-Tag: go1.22rc1~910 X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=e3ce3126212115808bc248bdc9ad92c0a46436fe;p=gostls13.git cmd/compile/internal/typecheck: fix closure field naming When creating the struct type to hold variables captured by a function literal, we currently reuse the captured variable names as fields. However, there's no particular reason to do this: these struct types aren't visible to users, and it adds extra complexity in making sure fields belong to the correct packages. Further, it turns out we were getting that subtly wrong. If two function literals from different packages capture variables with identical names starting with an uppercase letter (and in the same order and with corresponding identical types) end up in the same function (e.g., due to inlining), then we could end up creating closure struct types that are "different" (i.e., not types.Identical) yet end up with equal LinkString representations (which violates LinkString's contract). The easy fix is to just always use simple, exported, generated field names in the struct. This should allow further struct reuse across packages too, and shrink binary sizes slightly. Fixes #62498. Change-Id: I9c973f5087bf228649a8f74f7dc1522d84a26b51 Reviewed-on: https://go-review.googlesource.com/c/go/+/527135 Auto-Submit: Matthew Dempsky Reviewed-by: Cuong Manh Le Reviewed-by: Keith Randall LUCI-TryBot-Result: Go LUCI --- diff --git a/src/cmd/compile/internal/typecheck/func.go b/src/cmd/compile/internal/typecheck/func.go index 4f076c4854..3dbe51007c 100644 --- a/src/cmd/compile/internal/typecheck/func.go +++ b/src/cmd/compile/internal/typecheck/func.go @@ -91,40 +91,24 @@ func ClosureType(clo *ir.ClosureExpr) *types.Type { // and has one float64 argument and no results, // the generated code looks like: // - // clos = &struct{.F uintptr; i *int; s *string}{func.1, &i, &s} + // clos = &struct{F uintptr; X0 *int; X1 *string}{func.1, &i, &s} // // The use of the struct provides type information to the garbage - // collector so that it can walk the closure. We could use (in this case) - // [3]unsafe.Pointer instead, but that would leave the gc in the dark. - // The information appears in the binary in the form of type descriptors; - // the struct is unnamed so that closures in multiple packages with the - // same struct type can share the descriptor. - - // Make sure the .F field is in the same package as the rest of the - // fields. This deals with closures in instantiated functions, which are - // compiled as if from the source package of the generic function. - var pkg *types.Pkg - if len(clo.Func.ClosureVars) == 0 { - pkg = types.LocalPkg - } else { - for _, v := range clo.Func.ClosureVars { - if pkg == nil { - pkg = v.Sym().Pkg - } else if pkg != v.Sym().Pkg { - base.Fatalf("Closure variables from multiple packages: %+v", clo) - } - } - } - - fields := []*types.Field{ - types.NewField(base.Pos, pkg.Lookup(".F"), types.Types[types.TUINTPTR]), - } - for _, v := range clo.Func.ClosureVars { + // collector so that it can walk the closure. We could use (in this + // case) [3]unsafe.Pointer instead, but that would leave the gc in + // the dark. The information appears in the binary in the form of + // type descriptors; the struct is unnamed and uses exported field + // names so that closures in multiple packages with the same struct + // type can share the descriptor. + + fields := make([]*types.Field, 1+len(clo.Func.ClosureVars)) + fields[0] = types.NewField(base.AutogeneratedPos, types.LocalPkg.Lookup("F"), types.Types[types.TUINTPTR]) + for i, v := range clo.Func.ClosureVars { typ := v.Type() if !v.Byval() { typ = types.NewPtr(typ) } - fields = append(fields, types.NewField(base.Pos, v.Sym(), typ)) + fields[1+i] = types.NewField(base.AutogeneratedPos, types.LocalPkg.LookupNum("X", i), typ) } typ := types.NewStruct(fields) typ.SetNoalg(true) diff --git a/src/cmd/compile/internal/types/fmt.go b/src/cmd/compile/internal/types/fmt.go index f45a44eed9..1b53430af7 100644 --- a/src/cmd/compile/internal/types/fmt.go +++ b/src/cmd/compile/internal/types/fmt.go @@ -633,9 +633,6 @@ func fldconv(b *bytes.Buffer, f *Field, verb rune, mode fmtMode, visited map[*Ty name = fmt.Sprint(f.Nname) } else if verb == 'L' { name = s.Name - if name == ".F" { - name = "F" // Hack for toolstash -cmp. - } if !IsExported(name) && mode != fmtTypeIDName { name = sconv(s, 0, mode) // qualify non-exported names (used on structs, not on funarg) } diff --git a/test/fixedbugs/issue62498.dir/a.go b/test/fixedbugs/issue62498.dir/a.go new file mode 100644 index 0000000000..68f97475ab --- /dev/null +++ b/test/fixedbugs/issue62498.dir/a.go @@ -0,0 +1,13 @@ +// Copyright 2023 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package a + +func One(L any) { + func() { + defer F(L) + }() +} + +func F(any) {} diff --git a/test/fixedbugs/issue62498.dir/main.go b/test/fixedbugs/issue62498.dir/main.go new file mode 100644 index 0000000000..e55a24fb3a --- /dev/null +++ b/test/fixedbugs/issue62498.dir/main.go @@ -0,0 +1,18 @@ +// Copyright 2023 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package main + +import "./a" + +func main() { + a.One(nil) + Two(nil) +} + +func Two(L any) { + func() { + defer a.F(L) + }() +} diff --git a/test/fixedbugs/issue62498.go b/test/fixedbugs/issue62498.go new file mode 100644 index 0000000000..8fe8d8783f --- /dev/null +++ b/test/fixedbugs/issue62498.go @@ -0,0 +1,7 @@ +// rundir + +// Copyright 2023 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package ignored