Skip to content

Commit

Permalink
[release-branch.go1.21] cmd/compile/internal/typecheck: fix closure f…
Browse files Browse the repository at this point in the history
…ield 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.

For #62498.
Fixes #62545.

Change-Id: I9c973f5087bf228649a8f74f7dc1522d84a26b51
Reviewed-on: https://go-review.googlesource.com/c/go/+/527135
Auto-Submit: Matthew Dempsky <[email protected]>
Reviewed-by: Cuong Manh Le <[email protected]>
Reviewed-by: Keith Randall <[email protected]>
LUCI-TryBot-Result: Go LUCI <[email protected]>
(cherry picked from commit e3ce312)
Reviewed-on: https://go-review.googlesource.com/c/go/+/534916
Reviewed-by: Matthew Dempsky <[email protected]>
Auto-Submit: Dmitri Shuralyov <[email protected]>
Reviewed-by: Dmitri Shuralyov <[email protected]>
  • Loading branch information
mdempsky authored and gopherbot committed Oct 13, 2023
1 parent 64b6c48 commit f9a31cd
Show file tree
Hide file tree
Showing 5 changed files with 50 additions and 31 deletions.
40 changes: 12 additions & 28 deletions src/cmd/compile/internal/typecheck/func.go
Original file line number Diff line number Diff line change
Expand Up @@ -94,40 +94,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)
Expand Down
3 changes: 0 additions & 3 deletions src/cmd/compile/internal/types/fmt.go
Original file line number Diff line number Diff line change
Expand Up @@ -642,9 +642,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)
}
Expand Down
13 changes: 13 additions & 0 deletions test/fixedbugs/issue62498.dir/a.go
Original file line number Diff line number Diff line change
@@ -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) {}
18 changes: 18 additions & 0 deletions test/fixedbugs/issue62498.dir/main.go
Original file line number Diff line number Diff line change
@@ -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)
}()
}
7 changes: 7 additions & 0 deletions test/fixedbugs/issue62498.go
Original file line number Diff line number Diff line change
@@ -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

0 comments on commit f9a31cd

Please sign in to comment.