From b241938e04ed7171897390fdaefd3d3017a16a0b Mon Sep 17 00:00:00 2001 From: Baokun Lee Date: Tue, 29 Dec 2020 18:49:13 +0800 Subject: [PATCH 01/16] [dev.regabi] cmd/compile: fix some methods error text Change-Id: Ie9b034efba30d66a869c5e991b60c76198fd330f Reviewed-on: https://go-review.googlesource.com/c/go/+/279444 Run-TryBot: Baokun Lee TryBot-Result: Go Bot Reviewed-by: Matthew Dempsky Reviewed-by: Cuong Manh Le --- src/cmd/compile/internal/reflectdata/alg.go | 4 ++-- src/cmd/compile/internal/staticdata/data.go | 2 +- src/cmd/compile/internal/types/alg.go | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/cmd/compile/internal/reflectdata/alg.go b/src/cmd/compile/internal/reflectdata/alg.go index d23ca6c7aa..d576053753 100644 --- a/src/cmd/compile/internal/reflectdata/alg.go +++ b/src/cmd/compile/internal/reflectdata/alg.go @@ -42,8 +42,8 @@ func eqCanPanic(t *types.Type) bool { } } -// AlgType is like algtype1, except it returns the fixed-width AMEMxx variants -// instead of the general AMEM kind when possible. +// AlgType returns the fixed-width AMEMxx variants instead of the general +// AMEM kind when possible. func AlgType(t *types.Type) types.AlgKind { a, _ := types.AlgType(t) if a == types.AMEM { diff --git a/src/cmd/compile/internal/staticdata/data.go b/src/cmd/compile/internal/staticdata/data.go index 94fa6760a0..a2a844f940 100644 --- a/src/cmd/compile/internal/staticdata/data.go +++ b/src/cmd/compile/internal/staticdata/data.go @@ -276,7 +276,7 @@ func FuncLinksym(n *ir.Name) *obj.LSym { // the s·f stubs in s's package. func NeedFuncSym(s *types.Sym) { if !base.Ctxt.Flag_dynlink { - base.Fatalf("makefuncsym dynlink") + base.Fatalf("NeedFuncSym: dynlink") } if s.IsBlank() { return diff --git a/src/cmd/compile/internal/types/alg.go b/src/cmd/compile/internal/types/alg.go index f1a472cca5..6091ee249c 100644 --- a/src/cmd/compile/internal/types/alg.go +++ b/src/cmd/compile/internal/types/alg.go @@ -132,7 +132,7 @@ func AlgType(t *Type) (AlgKind, *Type) { return ret, nil } - base.Fatalf("algtype1: unexpected type %v", t) + base.Fatalf("algtype: unexpected type %v", t) return 0, nil } From 6ee9b118a2a70371e038fb6bec4fe7989a3a2b2d Mon Sep 17 00:00:00 2001 From: Robert Griesemer Date: Fri, 8 Jan 2021 14:04:50 -0800 Subject: [PATCH 02/16] [dev.regabi] cmd/compile: remove fmt_test code; it has outlived its usefulness With the recent compiler rewrites and cleanups to gc/fmt.go, the "safety net" provided by fmt_test has become less important and the test itself has become a burden (often breaks because of small format changes elsewhere). Eventually, the syntax and types2 packages will provide most error and diagnostic compiler output at which point fmt.go can be further simplified as well. Change-Id: Ie93eefd3e1166f3548fed0199b732dbd6c81948a Reviewed-on: https://go-review.googlesource.com/c/go/+/282560 Trust: Robert Griesemer Run-TryBot: Robert Griesemer Reviewed-by: Matthew Dempsky Reviewed-by: Austin Clements TryBot-Result: Go Bot --- src/cmd/compile/fmt_test.go | 615 --------------------------------- src/cmd/compile/fmtmap_test.go | 75 ---- 2 files changed, 690 deletions(-) delete mode 100644 src/cmd/compile/fmt_test.go delete mode 100644 src/cmd/compile/fmtmap_test.go diff --git a/src/cmd/compile/fmt_test.go b/src/cmd/compile/fmt_test.go deleted file mode 100644 index 6398a84f8f..0000000000 --- a/src/cmd/compile/fmt_test.go +++ /dev/null @@ -1,615 +0,0 @@ -// Copyright 2016 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. - -// This file implements TestFormats; a test that verifies -// format strings in the compiler (this directory and all -// subdirectories, recursively). -// -// TestFormats finds potential (Printf, etc.) format strings. -// If they are used in a call, the format verbs are verified -// based on the matching argument type against a precomputed -// map of valid formats (knownFormats). This map can be used to -// automatically rewrite format strings across all compiler -// files with the -r flag. -// -// The format map needs to be updated whenever a new (type, -// format) combination is found and the format verb is not -// 'v' or 'T' (as in "%v" or "%T"). To update the map auto- -// matically from the compiler source's use of format strings, -// use the -u flag. (Whether formats are valid for the values -// to be formatted must be verified manually, of course.) -// -// The -v flag prints out the names of all functions called -// with a format string, the names of files that were not -// processed, and any format rewrites made (with -r). -// -// Run as: go test -run Formats [-r][-u][-v] -// -// Known shortcomings: -// - indexed format strings ("%[2]s", etc.) are not supported -// (the test will fail) -// - format strings that are not simple string literals cannot -// be updated automatically -// (the test will fail with respective warnings) -// - format strings in _test packages outside the current -// package are not processed -// (the test will report those files) -// -package main_test - -import ( - "bytes" - "flag" - "fmt" - "go/ast" - "go/build" - "go/constant" - "go/format" - "go/importer" - "go/parser" - "go/token" - "go/types" - "internal/testenv" - "io" - "io/fs" - "io/ioutil" - "log" - "os" - "path/filepath" - "sort" - "strconv" - "strings" - "testing" - "unicode/utf8" -) - -var ( - rewrite = flag.Bool("r", false, "rewrite format strings") - update = flag.Bool("u", false, "update known formats") -) - -// The following variables collect information across all processed files. -var ( - fset = token.NewFileSet() - formatStrings = make(map[*ast.BasicLit]bool) // set of all potential format strings found - foundFormats = make(map[string]bool) // set of all formats found - callSites = make(map[*ast.CallExpr]*callSite) // map of all calls -) - -// A File is a corresponding (filename, ast) pair. -type File struct { - name string - ast *ast.File -} - -func TestFormats(t *testing.T) { - if testing.Short() && testenv.Builder() == "" { - t.Skip("Skipping in short mode") - } - testenv.MustHaveGoBuild(t) // more restrictive than necessary, but that's ok - - // process all directories - filepath.WalkDir(".", func(path string, info fs.DirEntry, err error) error { - if info.IsDir() { - if info.Name() == "testdata" { - return filepath.SkipDir - } - - importPath := filepath.Join("cmd/compile", path) - if ignoredPackages[filepath.ToSlash(importPath)] { - return filepath.SkipDir - } - - pkg, err := build.Import(importPath, path, 0) - if err != nil { - if _, ok := err.(*build.NoGoError); ok { - return nil // nothing to do here - } - t.Fatal(err) - } - collectPkgFormats(t, pkg) - } - return nil - }) - - // test and rewrite formats - updatedFiles := make(map[string]File) // files that were rewritten - for _, p := range callSites { - // test current format literal and determine updated one - out := formatReplace(p.str, func(index int, in string) string { - if in == "*" { - return in // cannot rewrite '*' (as in "%*d") - } - // in != '*' - typ := p.types[index] - format := typ + " " + in // e.g., "*Node %n" - - // Do not bother reporting basic types, nor %v, %T, %p. - // Vet handles basic types, and those three formats apply to all types. - if !strings.Contains(typ, ".") || (in == "%v" || in == "%T" || in == "%p") { - return in - } - - // check if format is known - out, known := knownFormats[format] - - // record format if not yet found - _, found := foundFormats[format] - if !found { - foundFormats[format] = true - } - - // report an error if the format is unknown and this is the first - // time we see it; ignore "%v" and "%T" which are always valid - if !known && !found && in != "%v" && in != "%T" { - t.Errorf("%s: unknown format %q for %s argument", posString(p.arg), in, typ) - } - - if out == "" { - out = in - } - return out - }) - - // replace existing format literal if it changed - if out != p.str { - // we cannot replace the argument if it's not a string literal for now - // (e.g., it may be "foo" + "bar") - lit, ok := p.arg.(*ast.BasicLit) - if !ok { - delete(callSites, p.call) // treat as if we hadn't found this site - continue - } - - if testing.Verbose() { - fmt.Printf("%s:\n\t- %q\n\t+ %q\n", posString(p.arg), p.str, out) - } - - // find argument index of format argument - index := -1 - for i, arg := range p.call.Args { - if p.arg == arg { - index = i - break - } - } - if index < 0 { - // we may have processed the same call site twice, - // but that shouldn't happen - panic("internal error: matching argument not found") - } - - // replace literal - new := *lit // make a copy - new.Value = strconv.Quote(out) // this may introduce "-quotes where there were `-quotes - p.call.Args[index] = &new - updatedFiles[p.file.name] = p.file - } - } - - // write dirty files back - var filesUpdated bool - if len(updatedFiles) > 0 && *rewrite { - for _, file := range updatedFiles { - var buf bytes.Buffer - if err := format.Node(&buf, fset, file.ast); err != nil { - t.Errorf("WARNING: gofmt %s failed: %v", file.name, err) - continue - } - if err := ioutil.WriteFile(file.name, buf.Bytes(), 0x666); err != nil { - t.Errorf("WARNING: writing %s failed: %v", file.name, err) - continue - } - fmt.Printf("updated %s\n", file.name) - filesUpdated = true - } - } - - // report the names of all functions called with a format string - if len(callSites) > 0 && testing.Verbose() { - set := make(map[string]bool) - for _, p := range callSites { - set[nodeString(p.call.Fun)] = true - } - var list []string - for s := range set { - list = append(list, s) - } - fmt.Println("\nFunctions called with a format string") - writeList(os.Stdout, list) - } - - // update formats - if len(foundFormats) > 0 && *update { - var list []string - for s := range foundFormats { - list = append(list, fmt.Sprintf("%q: \"\",", s)) - } - var buf bytes.Buffer - buf.WriteString(knownFormatsHeader) - writeList(&buf, list) - buf.WriteString("}\n") - out, err := format.Source(buf.Bytes()) - const outfile = "fmtmap_test.go" - if err != nil { - t.Errorf("WARNING: gofmt %s failed: %v", outfile, err) - out = buf.Bytes() // continue with unformatted source - } - if err = ioutil.WriteFile(outfile, out, 0644); err != nil { - t.Errorf("WARNING: updating format map failed: %v", err) - } - } - - // check that knownFormats is up to date - if !*rewrite && !*update { - var mismatch bool - for s := range foundFormats { - if _, ok := knownFormats[s]; !ok { - mismatch = true - break - } - } - if !mismatch { - for s := range knownFormats { - if _, ok := foundFormats[s]; !ok { - mismatch = true - break - } - } - } - if mismatch { - t.Errorf("format map is out of date; run 'go test -u' to update and manually verify correctness of change'") - } - } - - // all format strings of calls must be in the formatStrings set (self-verification) - for _, p := range callSites { - if lit, ok := p.arg.(*ast.BasicLit); ok && lit.Kind == token.STRING { - if formatStrings[lit] { - // ok - delete(formatStrings, lit) - } else { - // this should never happen - panic(fmt.Sprintf("internal error: format string not found (%s)", posString(lit))) - } - } - } - - // if we have any strings left, we may need to update them manually - if len(formatStrings) > 0 && filesUpdated { - var list []string - for lit := range formatStrings { - list = append(list, fmt.Sprintf("%s: %s", posString(lit), nodeString(lit))) - } - fmt.Println("\nWARNING: Potentially missed format strings") - writeList(os.Stdout, list) - t.Fail() - } - - fmt.Println() -} - -// A callSite describes a function call that appears to contain -// a format string. -type callSite struct { - file File - call *ast.CallExpr // call containing the format string - arg ast.Expr // format argument (string literal or constant) - str string // unquoted format string - types []string // argument types -} - -func collectPkgFormats(t *testing.T, pkg *build.Package) { - // collect all files - var filenames []string - filenames = append(filenames, pkg.GoFiles...) - filenames = append(filenames, pkg.CgoFiles...) - filenames = append(filenames, pkg.TestGoFiles...) - - // TODO(gri) verify _test files outside package - for _, name := range pkg.XTestGoFiles { - // don't process this test itself - if name != "fmt_test.go" && testing.Verbose() { - fmt.Printf("WARNING: %s not processed\n", filepath.Join(pkg.Dir, name)) - } - } - - // make filenames relative to . - for i, name := range filenames { - filenames[i] = filepath.Join(pkg.Dir, name) - } - - // parse all files - files := make([]*ast.File, len(filenames)) - for i, filename := range filenames { - f, err := parser.ParseFile(fset, filename, nil, parser.ParseComments) - if err != nil { - t.Fatal(err) - } - files[i] = f - } - - // typecheck package - conf := types.Config{Importer: importer.Default()} - etypes := make(map[ast.Expr]types.TypeAndValue) - if _, err := conf.Check(pkg.ImportPath, fset, files, &types.Info{Types: etypes}); err != nil { - t.Fatal(err) - } - - // collect all potential format strings (for extra verification later) - for _, file := range files { - ast.Inspect(file, func(n ast.Node) bool { - if s, ok := stringLit(n); ok && isFormat(s) { - formatStrings[n.(*ast.BasicLit)] = true - } - return true - }) - } - - // collect all formats/arguments of calls with format strings - for index, file := range files { - ast.Inspect(file, func(n ast.Node) bool { - if call, ok := n.(*ast.CallExpr); ok { - if ignoredFunctions[nodeString(call.Fun)] { - return true - } - // look for an arguments that might be a format string - for i, arg := range call.Args { - if s, ok := stringVal(etypes[arg]); ok && isFormat(s) { - // make sure we have enough arguments - n := numFormatArgs(s) - if i+1+n > len(call.Args) { - t.Errorf("%s: not enough format args (ignore %s?)", posString(call), nodeString(call.Fun)) - break // ignore this call - } - // assume last n arguments are to be formatted; - // determine their types - argTypes := make([]string, n) - for i, arg := range call.Args[len(call.Args)-n:] { - if tv, ok := etypes[arg]; ok { - argTypes[i] = typeString(tv.Type) - } - } - // collect call site - if callSites[call] != nil { - panic("internal error: file processed twice?") - } - callSites[call] = &callSite{ - file: File{filenames[index], file}, - call: call, - arg: arg, - str: s, - types: argTypes, - } - break // at most one format per argument list - } - } - } - return true - }) - } -} - -// writeList writes list in sorted order to w. -func writeList(w io.Writer, list []string) { - sort.Strings(list) - for _, s := range list { - fmt.Fprintln(w, "\t", s) - } -} - -// posString returns a string representation of n's position -// in the form filename:line:col: . -func posString(n ast.Node) string { - if n == nil { - return "" - } - return fset.Position(n.Pos()).String() -} - -// nodeString returns a string representation of n. -func nodeString(n ast.Node) string { - var buf bytes.Buffer - if err := format.Node(&buf, fset, n); err != nil { - log.Fatal(err) // should always succeed - } - return buf.String() -} - -// typeString returns a string representation of n. -func typeString(typ types.Type) string { - s := filepath.ToSlash(typ.String()) - - // Report all the concrete IR types as Node, to shorten fmtmap. - const ir = "cmd/compile/internal/ir." - if s == "*"+ir+"Name" || s == "*"+ir+"Func" || s == "*"+ir+"Decl" || - s == ir+"Ntype" || s == ir+"Expr" || s == ir+"Stmt" || - strings.HasPrefix(s, "*"+ir) && (strings.HasSuffix(s, "Expr") || strings.HasSuffix(s, "Stmt")) { - return "cmd/compile/internal/ir.Node" - } - - return s -} - -// stringLit returns the unquoted string value and true if -// n represents a string literal; otherwise it returns "" -// and false. -func stringLit(n ast.Node) (string, bool) { - if lit, ok := n.(*ast.BasicLit); ok && lit.Kind == token.STRING { - s, err := strconv.Unquote(lit.Value) - if err != nil { - log.Fatal(err) // should not happen with correct ASTs - } - return s, true - } - return "", false -} - -// stringVal returns the (unquoted) string value and true if -// tv is a string constant; otherwise it returns "" and false. -func stringVal(tv types.TypeAndValue) (string, bool) { - if tv.IsValue() && tv.Value != nil && tv.Value.Kind() == constant.String { - return constant.StringVal(tv.Value), true - } - return "", false -} - -// formatIter iterates through the string s in increasing -// index order and calls f for each format specifier '%..v'. -// The arguments for f describe the specifier's index range. -// If a format specifier contains a "*", f is called with -// the index range for "*" alone, before being called for -// the entire specifier. The result of f is the index of -// the rune at which iteration continues. -func formatIter(s string, f func(i, j int) int) { - i := 0 // index after current rune - var r rune // current rune - - next := func() { - r1, w := utf8.DecodeRuneInString(s[i:]) - if w == 0 { - r1 = -1 // signal end-of-string - } - r = r1 - i += w - } - - flags := func() { - for r == ' ' || r == '#' || r == '+' || r == '-' || r == '0' { - next() - } - } - - index := func() { - if r == '[' { - log.Fatalf("cannot handle indexed arguments: %s", s) - } - } - - digits := func() { - index() - if r == '*' { - i = f(i-1, i) - next() - return - } - for '0' <= r && r <= '9' { - next() - } - } - - for next(); r >= 0; next() { - if r == '%' { - i0 := i - next() - flags() - digits() - if r == '.' { - next() - digits() - } - index() - // accept any letter (a-z, A-Z) as format verb; - // ignore anything else - if 'a' <= r && r <= 'z' || 'A' <= r && r <= 'Z' { - i = f(i0-1, i) - } - } - } -} - -// isFormat reports whether s contains format specifiers. -func isFormat(s string) (yes bool) { - formatIter(s, func(i, j int) int { - yes = true - return len(s) // stop iteration - }) - return -} - -// oneFormat reports whether s is exactly one format specifier. -func oneFormat(s string) (yes bool) { - formatIter(s, func(i, j int) int { - yes = i == 0 && j == len(s) - return j - }) - return -} - -// numFormatArgs returns the number of format specifiers in s. -func numFormatArgs(s string) int { - count := 0 - formatIter(s, func(i, j int) int { - count++ - return j - }) - return count -} - -// formatReplace replaces the i'th format specifier s in the incoming -// string in with the result of f(i, s) and returns the new string. -func formatReplace(in string, f func(i int, s string) string) string { - var buf []byte - i0 := 0 - index := 0 - formatIter(in, func(i, j int) int { - if sub := in[i:j]; sub != "*" { // ignore calls for "*" width/length specifiers - buf = append(buf, in[i0:i]...) - buf = append(buf, f(index, sub)...) - i0 = j - } - index++ - return j - }) - return string(append(buf, in[i0:]...)) -} - -// ignoredPackages is the set of packages which can -// be ignored. -var ignoredPackages = map[string]bool{} - -// ignoredFunctions is the set of functions which may have -// format-like arguments but which don't do any formatting and -// thus may be ignored. -var ignoredFunctions = map[string]bool{} - -func init() { - // verify that knownFormats entries are correctly formatted - for key, val := range knownFormats { - // key must be "typename format", and format starts with a '%' - // (formats containing '*' alone are not collected in this map) - i := strings.Index(key, "%") - if i < 0 || !oneFormat(key[i:]) { - log.Fatalf("incorrect knownFormats key: %q", key) - } - // val must be "format" or "" - if val != "" && !oneFormat(val) { - log.Fatalf("incorrect knownFormats value: %q (key = %q)", val, key) - } - } -} - -const knownFormatsHeader = `// Copyright 2018 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. - -// This file implements the knownFormats map which records the valid -// formats for a given type. The valid formats must correspond to -// supported compiler formats implemented in fmt.go, or whatever -// other format verbs are implemented for the given type. The map may -// also be used to change the use of a format verb across all compiler -// sources automatically (for instance, if the implementation of fmt.go -// changes), by using the -r option together with the new formats in the -// map. To generate this file automatically from the existing source, -// run: go test -run Formats -u. -// -// See the package comment in fmt_test.go for additional information. - -package main_test - -// knownFormats entries are of the form "typename format" -> "newformat". -// An absent entry means that the format is not recognized as valid. -// An empty new format means that the format should remain unchanged. -var knownFormats = map[string]string{ -` diff --git a/src/cmd/compile/fmtmap_test.go b/src/cmd/compile/fmtmap_test.go deleted file mode 100644 index a925ec05ac..0000000000 --- a/src/cmd/compile/fmtmap_test.go +++ /dev/null @@ -1,75 +0,0 @@ -// Copyright 2018 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. - -// This file implements the knownFormats map which records the valid -// formats for a given type. The valid formats must correspond to -// supported compiler formats implemented in fmt.go, or whatever -// other format verbs are implemented for the given type. The map may -// also be used to change the use of a format verb across all compiler -// sources automatically (for instance, if the implementation of fmt.go -// changes), by using the -r option together with the new formats in the -// map. To generate this file automatically from the existing source, -// run: go test -run Formats -u. -// -// See the package comment in fmt_test.go for additional information. - -package main_test - -// knownFormats entries are of the form "typename format" -> "newformat". -// An absent entry means that the format is not recognized as valid. -// An empty new format means that the format should remain unchanged. -var knownFormats = map[string]string{ - "*bytes.Buffer %s": "", - "*cmd/compile/internal/ssa.Block %s": "", - "*cmd/compile/internal/ssa.Func %s": "", - "*cmd/compile/internal/ssa.Register %s": "", - "*cmd/compile/internal/ssa.Value %s": "", - "*cmd/compile/internal/types.Sym %+v": "", - "*cmd/compile/internal/types.Sym %S": "", - "*cmd/compile/internal/types.Type %+v": "", - "*cmd/compile/internal/types.Type %-S": "", - "*cmd/compile/internal/types.Type %L": "", - "*cmd/compile/internal/types.Type %S": "", - "*cmd/compile/internal/types.Type %s": "", - "*math/big.Float %f": "", - "*math/big.Int %s": "", - "[]cmd/compile/internal/syntax.token %s": "", - "cmd/compile/internal/arm.shift %d": "", - "cmd/compile/internal/gc.RegIndex %d": "", - "cmd/compile/internal/ir.Class %d": "", - "cmd/compile/internal/ir.Node %+v": "", - "cmd/compile/internal/ir.Node %L": "", - "cmd/compile/internal/ir.Nodes %+v": "", - "cmd/compile/internal/ir.Nodes %.v": "", - "cmd/compile/internal/ir.Op %+v": "", - "cmd/compile/internal/ssa.Aux %#v": "", - "cmd/compile/internal/ssa.Aux %q": "", - "cmd/compile/internal/ssa.Aux %s": "", - "cmd/compile/internal/ssa.BranchPrediction %d": "", - "cmd/compile/internal/ssa.ID %d": "", - "cmd/compile/internal/ssa.LocalSlot %s": "", - "cmd/compile/internal/ssa.Location %s": "", - "cmd/compile/internal/ssa.Op %s": "", - "cmd/compile/internal/ssa.ValAndOff %s": "", - "cmd/compile/internal/ssa.flagConstant %s": "", - "cmd/compile/internal/ssa.rbrank %d": "", - "cmd/compile/internal/ssa.regMask %d": "", - "cmd/compile/internal/ssa.register %d": "", - "cmd/compile/internal/ssa.relation %s": "", - "cmd/compile/internal/syntax.Error %q": "", - "cmd/compile/internal/syntax.Expr %#v": "", - "cmd/compile/internal/syntax.LitKind %d": "", - "cmd/compile/internal/syntax.Operator %s": "", - "cmd/compile/internal/syntax.Pos %s": "", - "cmd/compile/internal/syntax.position %s": "", - "cmd/compile/internal/syntax.token %q": "", - "cmd/compile/internal/syntax.token %s": "", - "cmd/compile/internal/types.Kind %d": "", - "cmd/compile/internal/types.Kind %s": "", - "cmd/compile/internal/walk.initKind %d": "", - "go/constant.Value %#v": "", - "math/big.Accuracy %s": "", - "reflect.Type %s": "", - "time.Duration %d": "", -} From 8b2efa990b08e6c32422fbfdab746f4f6948ae42 Mon Sep 17 00:00:00 2001 From: Matthew Dempsky Date: Sun, 3 Jan 2021 19:49:49 -0800 Subject: [PATCH 03/16] [dev.regabi] cmd/compile: deref PAUTOHEAPs during SSA construction Currently, during walk we rewrite PAUTOHEAP uses into derefs of their corresponding Heapaddr, but we can easily do this instead during SSA construction. This does involve updating two test cases: * nilptr3.go This file had a test that we emit a "removed nil check" diagnostic for the implicit dereference from accessing a PAUTOHEAP variable. This CL removes this diagnostic, since it's not really useful to end users: from the user's point of view, there's no pointer anyway, so they needn't care about whether we check for nil or not. That's a purely internal detail. And with the PAUTOHEAP dereference handled during SSA construction, we can more robustly ensure this happens, rather than relying on setting a flag in walk and hoping that SSA sees it. * issue20780.go Previously, when PAUTOHEAPs were dereferenced during walk, it had a consequence that when they're passed as a function call argument, they would first get copied to the stack before being copied to their actual destination. Moving the dereferencing to SSA had a side-effect of eliminating this unnecessary temporary, and copying directly to the destination parameter. The test is updated to instead call "g(h(), h())" where h() returns a large value, as the first result will always need to be spilled somewhere will calling the second function. Maybe eventually we're smart enough to realize it can be spilled to the heap, but we don't do that today. Because I'm concerned that the direct copy-to-parameter optimization could interfere with race-detector instrumentation (e.g., maybe the copies were previously necessary to ensure they're not clobbered by inserted raceread calls?), I've also added issue20780b.go to exercise this in a few different ways. Change-Id: I720598cb32b17518bc10a03e555620c0f25fd28d Reviewed-on: https://go-review.googlesource.com/c/go/+/281293 Run-TryBot: Matthew Dempsky TryBot-Result: Go Bot Trust: Matthew Dempsky Reviewed-by: Keith Randall Reviewed-by: Cuong Manh Le --- src/cmd/compile/internal/ssagen/ssa.go | 11 +++-- src/cmd/compile/internal/walk/expr.go | 10 ++--- test/fixedbugs/issue20780.go | 16 ++++--- test/fixedbugs/issue20780b.go | 62 ++++++++++++++++++++++++++ test/nilptr3.go | 8 ---- 5 files changed, 81 insertions(+), 26 deletions(-) create mode 100644 test/fixedbugs/issue20780b.go diff --git a/src/cmd/compile/internal/ssagen/ssa.go b/src/cmd/compile/internal/ssagen/ssa.go index 5998c42012..f48909e6be 100644 --- a/src/cmd/compile/internal/ssagen/ssa.go +++ b/src/cmd/compile/internal/ssagen/ssa.go @@ -3222,8 +3222,8 @@ func (s *state) assign(left ir.Node, right *ssa.Value, deref bool, skip skipMask // If this assignment clobbers an entire local variable, then emit // OpVarDef so liveness analysis knows the variable is redefined. - if base := clobberBase(left); base.Op() == ir.ONAME && base.(*ir.Name).Class != ir.PEXTERN && skip == 0 { - s.vars[memVar] = s.newValue1Apos(ssa.OpVarDef, types.TypeMem, base.(*ir.Name), s.mem(), !ir.IsAutoTmp(base)) + if base, ok := clobberBase(left).(*ir.Name); ok && base.Op() == ir.ONAME && base.Class != ir.PEXTERN && base.Class != ir.PAUTOHEAP && skip == 0 { + s.vars[memVar] = s.newValue1Apos(ssa.OpVarDef, types.TypeMem, base, s.mem(), !ir.IsAutoTmp(base)) } // Left is not ssa-able. Compute its address. @@ -4986,6 +4986,8 @@ func (s *state) addr(n ir.Node) *ssa.Value { // ensure that we reuse symbols for out parameters so // that cse works on their addresses return s.newValue2Apos(ssa.OpLocalAddr, t, n, s.sp, s.mem(), true) + case ir.PAUTOHEAP: + return s.expr(n.Heapaddr) default: s.Fatalf("variable address class %v not implemented", n.Class) return nil @@ -5096,11 +5098,8 @@ func (s *state) canSSAName(name *ir.Name) bool { if ir.IsParamHeapCopy(name) { return false } - if name.Class == ir.PAUTOHEAP { - s.Fatalf("canSSA of PAUTOHEAP %v", name) - } switch name.Class { - case ir.PEXTERN: + case ir.PEXTERN, ir.PAUTOHEAP: return false case ir.PPARAMOUT: if s.hasdefer { diff --git a/src/cmd/compile/internal/walk/expr.go b/src/cmd/compile/internal/walk/expr.go index 3dffb496e9..6fdb8f15f5 100644 --- a/src/cmd/compile/internal/walk/expr.go +++ b/src/cmd/compile/internal/walk/expr.go @@ -52,19 +52,15 @@ func walkExpr(n ir.Node, init *ir.Nodes) ir.Node { base.Fatalf("expression has untyped type: %+v", n) } - if n.Op() == ir.ONAME && n.(*ir.Name).Class == ir.PAUTOHEAP { - n := n.(*ir.Name) - nn := ir.NewStarExpr(base.Pos, n.Heapaddr) - nn.X.MarkNonNil() - return walkExpr(typecheck.Expr(nn), init) - } - n = walkExpr1(n, init) // Eagerly compute sizes of all expressions for the back end. if typ := n.Type(); typ != nil && typ.Kind() != types.TBLANK && !typ.IsFuncArgStruct() { types.CheckSize(typ) } + if n, ok := n.(*ir.Name); ok && n.Heapaddr != nil { + types.CheckSize(n.Heapaddr.Type()) + } if ir.IsConst(n, constant.String) { // Emit string symbol now to avoid emitting // any concurrently during the backend. diff --git a/test/fixedbugs/issue20780.go b/test/fixedbugs/issue20780.go index 53c4f615e1..f73e6d1f79 100644 --- a/test/fixedbugs/issue20780.go +++ b/test/fixedbugs/issue20780.go @@ -9,11 +9,17 @@ package main +type Big = [400e6]byte + func f() { // GC_ERROR "stack frame too large" - var x [800e6]byte - g(x) - return + // Note: This test relies on the fact that we currently always + // spill function-results to the stack, even if they're so + // large that we would normally heap allocate them. If we ever + // improve the backend to spill temporaries to the heap, this + // test will probably need updating to find some new way to + // construct an overly large stack frame. + g(h(), h()) } -//go:noinline -func g([800e6]byte) {} +func g(Big, Big) +func h() Big diff --git a/test/fixedbugs/issue20780b.go b/test/fixedbugs/issue20780b.go new file mode 100644 index 0000000000..c8bf1f8349 --- /dev/null +++ b/test/fixedbugs/issue20780b.go @@ -0,0 +1,62 @@ +// +build cgo,linux,amd64 +// run -race + +// Copyright 2021 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. + +// Test that CL 281293 doesn't interfere with race detector +// instrumentation. + +package main + +import "fmt" + +const N = 2e6 + +type Big = [N]int + +var sink interface{} + +func main() { + g(0, f(0)) + + x1 := f(1) + sink = &x1 + g(1, x1) + g(7, f(7)) + g(1, x1) + + x3 := f(3) + sink = &x3 + g(1, x1) + g(3, x3) + + h(f(0), x1, f(2), x3, f(4)) +} + +//go:noinline +func f(k int) (x Big) { + for i := range x { + x[i] = k*N + i + } + return +} + +//go:noinline +func g(k int, x Big) { + for i := range x { + if x[i] != k*N+i { + panic(fmt.Sprintf("x%d[%d] = %d", k, i, x[i])) + } + } +} + +//go:noinline +func h(x0, x1, x2, x3, x4 Big) { + g(0, x0) + g(1, x1) + g(2, x2) + g(3, x3) + g(4, x4) +} diff --git a/test/nilptr3.go b/test/nilptr3.go index e0f2ed9767..3345cfa5ab 100644 --- a/test/nilptr3.go +++ b/test/nilptr3.go @@ -214,14 +214,6 @@ func p1() byte { return p[5] // ERROR "removed nil check" } -// make sure not to do nil check for access of PAUTOHEAP -//go:noinline -func (p *Struct) m() {} -func c1() { - var x Struct - func() { x.m() }() // ERROR "removed nil check" -} - type SS struct { x byte } From 950cf4d46c5bc343644e7ef08828b9e5114d4676 Mon Sep 17 00:00:00 2001 From: Matthew Dempsky Date: Sun, 3 Jan 2021 21:34:03 -0800 Subject: [PATCH 04/16] [dev.regabi] cmd/compile: bind closure vars during SSA constructions For function literals that aren't inlined or directly called, we need to pass their arguments via a closure struct. This also means we need to rewrite uses of closure variables to access from this closure struct. Currently we do this rewrite in a pass before walking begins. This CL moves the code to SSA construction instead, alongside binding other input parameters. Change-Id: I13538ef3394e2d6f75d5b7b2d0adbb00db812dc2 Reviewed-on: https://go-review.googlesource.com/c/go/+/281352 Run-TryBot: Matthew Dempsky TryBot-Result: Go Bot Trust: Matthew Dempsky Reviewed-by: Keith Randall --- src/cmd/compile/internal/ssagen/ssa.go | 41 +++++++ src/cmd/compile/internal/walk/closure.go | 135 ++++++++--------------- 2 files changed, 89 insertions(+), 87 deletions(-) diff --git a/src/cmd/compile/internal/ssagen/ssa.go b/src/cmd/compile/internal/ssagen/ssa.go index f48909e6be..0c222b12cf 100644 --- a/src/cmd/compile/internal/ssagen/ssa.go +++ b/src/cmd/compile/internal/ssagen/ssa.go @@ -470,6 +470,47 @@ func buildssa(fn *ir.Func, worker int) *ssa.Func { } } + // Populate closure variables. + if !fn.ClosureCalled() { + clo := s.entryNewValue0(ssa.OpGetClosurePtr, s.f.Config.Types.BytePtr) + offset := int64(types.PtrSize) // PtrSize to skip past function entry PC field + for _, n := range fn.ClosureVars { + typ := n.Type() + if !n.Byval() { + typ = types.NewPtr(typ) + } + + offset = types.Rnd(offset, typ.Alignment()) + r := s.newValue1I(ssa.OpOffPtr, types.NewPtr(typ), offset, clo) + offset += typ.Size() + + if n.Byval() && TypeOK(n.Type()) { + // If it is a small variable captured by value, downgrade it to PAUTO. + r = s.load(n.Type(), r) + + n.Class = ir.PAUTO + } else { + if !n.Byval() { + r = s.load(typ, r) + } + + // Declare variable holding address taken from closure. + addr := ir.NewNameAt(fn.Pos(), &types.Sym{Name: "&" + n.Sym().Name, Pkg: types.LocalPkg}) + addr.SetType(types.NewPtr(n.Type())) + addr.Class = ir.PAUTO + addr.SetUsed(true) + addr.Curfn = fn + types.CalcSize(addr.Type()) + + n.Heapaddr = addr + n = addr + } + + fn.Dcl = append(fn.Dcl, n) + s.assign(n, r, false, 0) + } + } + // Convert the AST-based IR to the SSA-based IR s.stmtList(fn.Enter) s.stmtList(fn.Body) diff --git a/src/cmd/compile/internal/walk/closure.go b/src/cmd/compile/internal/walk/closure.go index 449df88f9e..acb74b9901 100644 --- a/src/cmd/compile/internal/walk/closure.go +++ b/src/cmd/compile/internal/walk/closure.go @@ -15,103 +15,64 @@ import ( // Closure is called in a separate phase after escape analysis. // It transform closure bodies to properly reference captured variables. func Closure(fn *ir.Func) { + if len(fn.ClosureVars) == 0 { + return + } + + if !fn.ClosureCalled() { + // The closure is not directly called, so it is going to stay as closure. + fn.SetNeedctxt(true) + return + } + lno := base.Pos base.Pos = fn.Pos() - if fn.ClosureCalled() { - // If the closure is directly called, we transform it to a plain function call - // with variables passed as args. This avoids allocation of a closure object. - // Here we do only a part of the transformation. Walk of OCALLFUNC(OCLOSURE) - // will complete the transformation later. - // For illustration, the following closure: - // func(a int) { - // println(byval) - // byref++ - // }(42) - // becomes: - // func(byval int, &byref *int, a int) { - // println(byval) - // (*&byref)++ - // }(byval, &byref, 42) + // If the closure is directly called, we transform it to a plain function call + // with variables passed as args. This avoids allocation of a closure object. + // Here we do only a part of the transformation. Walk of OCALLFUNC(OCLOSURE) + // will complete the transformation later. + // For illustration, the following closure: + // func(a int) { + // println(byval) + // byref++ + // }(42) + // becomes: + // func(byval int, &byref *int, a int) { + // println(byval) + // (*&byref)++ + // }(byval, &byref, 42) - // f is ONAME of the actual function. - f := fn.Nname + // f is ONAME of the actual function. + f := fn.Nname - // We are going to insert captured variables before input args. - var params []*types.Field - var decls []*ir.Name - for _, v := range fn.ClosureVars { - if !v.Byval() { - // If v of type T is captured by reference, - // we introduce function param &v *T - // and v remains PAUTOHEAP with &v heapaddr - // (accesses will implicitly deref &v). - addr := typecheck.NewName(typecheck.Lookup("&" + v.Sym().Name)) - addr.SetType(types.NewPtr(v.Type())) - v.Heapaddr = addr - v = addr - } - - v.Class = ir.PPARAM - decls = append(decls, v) - - fld := types.NewField(src.NoXPos, v.Sym(), v.Type()) - fld.Nname = v - params = append(params, fld) + // We are going to insert captured variables before input args. + var params []*types.Field + var decls []*ir.Name + for _, v := range fn.ClosureVars { + if !v.Byval() { + // If v of type T is captured by reference, + // we introduce function param &v *T + // and v remains PAUTOHEAP with &v heapaddr + // (accesses will implicitly deref &v). + addr := typecheck.NewName(typecheck.Lookup("&" + v.Sym().Name)) + addr.SetType(types.NewPtr(v.Type())) + v.Heapaddr = addr + v = addr } - if len(params) > 0 { - // Prepend params and decls. - f.Type().Params().SetFields(append(params, f.Type().Params().FieldSlice()...)) - fn.Dcl = append(decls, fn.Dcl...) - } + v.Class = ir.PPARAM + decls = append(decls, v) - types.CalcSize(f.Type()) - fn.Nname.SetType(f.Type()) // update type of ODCLFUNC - } else { - // The closure is not called, so it is going to stay as closure. - var body []ir.Node - offset := int64(types.PtrSize) - for _, v := range fn.ClosureVars { - // cv refers to the field inside of closure OSTRUCTLIT. - typ := v.Type() - if !v.Byval() { - typ = types.NewPtr(typ) - } - offset = types.Rnd(offset, int64(typ.Align)) - cr := ir.NewClosureRead(typ, offset) - offset += typ.Width - - if v.Byval() && v.Type().Width <= int64(2*types.PtrSize) { - // If it is a small variable captured by value, downgrade it to PAUTO. - v.Class = ir.PAUTO - fn.Dcl = append(fn.Dcl, v) - body = append(body, ir.NewAssignStmt(base.Pos, v, cr)) - } else { - // Declare variable holding addresses taken from closure - // and initialize in entry prologue. - addr := typecheck.NewName(typecheck.Lookup("&" + v.Sym().Name)) - addr.SetType(types.NewPtr(v.Type())) - addr.Class = ir.PAUTO - addr.SetUsed(true) - addr.Curfn = fn - fn.Dcl = append(fn.Dcl, addr) - v.Heapaddr = addr - var src ir.Node = cr - if v.Byval() { - src = typecheck.NodAddr(cr) - } - body = append(body, ir.NewAssignStmt(base.Pos, addr, src)) - } - } - - if len(body) > 0 { - typecheck.Stmts(body) - fn.Enter = body - fn.SetNeedctxt(true) - } + fld := types.NewField(src.NoXPos, v.Sym(), v.Type()) + fld.Nname = v + params = append(params, fld) } + // Prepend params and decls. + f.Type().Params().SetFields(append(params, f.Type().Params().FieldSlice()...)) + fn.Dcl = append(decls, fn.Dcl...) + base.Pos = lno } From c9c26d7ffb3c4077ffaa80f7c8e2d550528e1445 Mon Sep 17 00:00:00 2001 From: Matthew Dempsky Date: Mon, 4 Jan 2021 02:24:48 -0800 Subject: [PATCH 05/16] [dev.regabi] cmd/compile: use ClosureVars for method value wrappers Similar to with regular closures, we can change method value wrappers to use ClosureVars and allow SSA construction to take care of wiring it up appropriately. Change-Id: I05c0b1bcec4e24305324755df35b7bc5b8a6ce7a Reviewed-on: https://go-review.googlesource.com/c/go/+/281353 Run-TryBot: Matthew Dempsky TryBot-Result: Go Bot Trust: Matthew Dempsky Reviewed-by: Keith Randall --- src/cmd/compile/internal/escape/escape.go | 3 +++ src/cmd/compile/internal/ir/name.go | 4 ++-- src/cmd/compile/internal/typecheck/func.go | 25 ++++++++++------------ 3 files changed, 16 insertions(+), 16 deletions(-) diff --git a/src/cmd/compile/internal/escape/escape.go b/src/cmd/compile/internal/escape/escape.go index 5df82d8cdc..9b9b8f6a58 100644 --- a/src/cmd/compile/internal/escape/escape.go +++ b/src/cmd/compile/internal/escape/escape.go @@ -583,6 +583,9 @@ func (e *escape) exprSkipInit(k hole, n ir.Node) { if n.Class == ir.PFUNC || n.Class == ir.PEXTERN { return } + if n.IsClosureVar() && n.Defn == nil { + return // ".this" from method value wrapper + } e.flow(k, e.oldLoc(n)) case ir.ONAMEOFFSET: diff --git a/src/cmd/compile/internal/ir/name.go b/src/cmd/compile/internal/ir/name.go index a51cf79929..cfb481e31c 100644 --- a/src/cmd/compile/internal/ir/name.go +++ b/src/cmd/compile/internal/ir/name.go @@ -264,7 +264,7 @@ const ( nameNeedzero // if it contains pointers, needs to be zeroed on function entry nameAutoTemp // is the variable a temporary (implies no dwarf info. reset if escapes to heap) nameUsed // for variable declared and not used error - nameIsClosureVar // PAUTOHEAP closure pseudo-variable; original at n.Name.Defn + nameIsClosureVar // PAUTOHEAP closure pseudo-variable; original (if any) at n.Defn nameIsOutputParamHeapAddr // pointer to a result parameter's heap copy nameAddrtaken // address taken, even if not moved to heap nameInlFormal // PAUTO created by inliner, derived from callee formal @@ -332,7 +332,7 @@ func (n *Name) SetVal(v constant.Value) { // it appears in the function that immediately contains the // declaration. Otherwise, Canonical simply returns n itself. func (n *Name) Canonical() *Name { - if n.IsClosureVar() { + if n.IsClosureVar() && n.Defn != nil { n = n.Defn.(*Name) } return n diff --git a/src/cmd/compile/internal/typecheck/func.go b/src/cmd/compile/internal/typecheck/func.go index 8789395ffb..12762f7ee8 100644 --- a/src/cmd/compile/internal/typecheck/func.go +++ b/src/cmd/compile/internal/typecheck/func.go @@ -246,29 +246,26 @@ func MethodValueWrapper(dot *ir.SelectorExpr) *ir.Func { fn.SetWrapper(true) // Declare and initialize variable holding receiver. - cr := ir.NewClosureRead(rcvrtype, types.Rnd(int64(types.PtrSize), int64(rcvrtype.Align))) - var ptr *ir.Name - var body []ir.Node - if rcvrtype.IsPtr() || rcvrtype.IsInterface() { - ptr = Temp(rcvrtype) - body = append(body, ir.NewAssignStmt(base.Pos, ptr, cr)) - } else { - ptr = Temp(types.NewPtr(rcvrtype)) - body = append(body, ir.NewAssignStmt(base.Pos, ptr, NodAddr(cr))) - } + ptr := ir.NewNameAt(base.Pos, Lookup(".this")) + ptr.Class = ir.PAUTOHEAP + ptr.SetType(rcvrtype) + ptr.Curfn = fn + ptr.SetIsClosureVar(true) + ptr.SetByval(true) + fn.ClosureVars = append(fn.ClosureVars, ptr) call := ir.NewCallExpr(base.Pos, ir.OCALL, ir.NewSelectorExpr(base.Pos, ir.OXDOT, ptr, meth), nil) call.Args = ir.ParamNames(tfn.Type()) call.IsDDD = tfn.Type().IsVariadic() + + var body ir.Node = call if t0.NumResults() != 0 { ret := ir.NewReturnStmt(base.Pos, nil) ret.Results = []ir.Node{call} - body = append(body, ret) - } else { - body = append(body, call) + body = ret } - fn.Body = body + fn.Body = []ir.Node{body} FinishFuncBody() Func(fn) From 7fd84c6e465d9c9d9424538ec99da2c59afdd469 Mon Sep 17 00:00:00 2001 From: Matthew Dempsky Date: Mon, 4 Jan 2021 16:33:30 -0800 Subject: [PATCH 06/16] [dev.regabi] cmd/compile: remove OCLOSUREREAD After the previous CLs, all closure reads are handled during SSA construction. Change-Id: Iad67b01fa2d3798f50ea647be7ccf8195f189c27 Reviewed-on: https://go-review.googlesource.com/c/go/+/281512 Run-TryBot: Matthew Dempsky TryBot-Result: Go Bot Trust: Matthew Dempsky Reviewed-by: Cuong Manh Le --- src/cmd/compile/internal/escape/escape.go | 4 +-- src/cmd/compile/internal/ir/expr.go | 17 ++---------- src/cmd/compile/internal/ir/node.go | 27 +++++++++---------- src/cmd/compile/internal/ir/node_gen.go | 16 ----------- src/cmd/compile/internal/ir/op_string.go | 27 +++++++++---------- src/cmd/compile/internal/ssagen/ssa.go | 7 ----- .../compile/internal/typecheck/typecheck.go | 3 --- src/cmd/compile/internal/walk/expr.go | 2 +- src/cmd/compile/internal/walk/walk.go | 2 +- 9 files changed, 32 insertions(+), 73 deletions(-) diff --git a/src/cmd/compile/internal/escape/escape.go b/src/cmd/compile/internal/escape/escape.go index 9b9b8f6a58..c63383af43 100644 --- a/src/cmd/compile/internal/escape/escape.go +++ b/src/cmd/compile/internal/escape/escape.go @@ -575,7 +575,7 @@ func (e *escape) exprSkipInit(k hole, n ir.Node) { default: base.Fatalf("unexpected expr: %v", n) - case ir.OLITERAL, ir.ONIL, ir.OGETG, ir.OCLOSUREREAD, ir.OTYPE, ir.OMETHEXPR: + case ir.OLITERAL, ir.ONIL, ir.OGETG, ir.OTYPE, ir.OMETHEXPR: // nop case ir.ONAME: @@ -1926,7 +1926,7 @@ func mayAffectMemory(n ir.Node) bool { // an ir.Any looking for any op that's not the ones in the case statement. // But that produces changes in the compiled output detected by buildall. switch n.Op() { - case ir.ONAME, ir.OCLOSUREREAD, ir.OLITERAL, ir.ONIL: + case ir.ONAME, ir.OLITERAL, ir.ONIL: return false case ir.OADD, ir.OSUB, ir.OOR, ir.OXOR, ir.OMUL, ir.OLSH, ir.ORSH, ir.OAND, ir.OANDNOT, ir.ODIV, ir.OMOD: diff --git a/src/cmd/compile/internal/ir/expr.go b/src/cmd/compile/internal/ir/expr.go index e7aa9c6a8f..51425db42d 100644 --- a/src/cmd/compile/internal/ir/expr.go +++ b/src/cmd/compile/internal/ir/expr.go @@ -203,19 +203,6 @@ func NewClosureExpr(pos src.XPos, fn *Func) *ClosureExpr { return n } -// A ClosureRead denotes reading a variable stored within a closure struct. -type ClosureReadExpr struct { - miniExpr - Offset int64 -} - -func NewClosureRead(typ *types.Type, offset int64) *ClosureReadExpr { - n := &ClosureReadExpr{Offset: offset} - n.typ = typ - n.op = OCLOSUREREAD - return n -} - // A CompLitExpr is a composite literal Type{Vals}. // Before type-checking, the type is Ntype. type CompLitExpr struct { @@ -727,7 +714,7 @@ func IsAddressable(n Node) bool { return false } fallthrough - case ODEREF, ODOTPTR, OCLOSUREREAD: + case ODEREF, ODOTPTR: return true case ODOT: @@ -889,7 +876,7 @@ func SameSafeExpr(l Node, r Node) bool { } switch l.Op() { - case ONAME, OCLOSUREREAD: + case ONAME: return l == r case ODOT, ODOTPTR: diff --git a/src/cmd/compile/internal/ir/node.go b/src/cmd/compile/internal/ir/node.go index 850d7343aa..a2b6e7203b 100644 --- a/src/cmd/compile/internal/ir/node.go +++ b/src/cmd/compile/internal/ir/node.go @@ -294,20 +294,19 @@ const ( OTSLICE // []int // misc - OINLCALL // intermediary representation of an inlined call. - OEFACE // itable and data words of an empty-interface value. - OITAB // itable word of an interface value. - OIDATA // data word of an interface value in Left - OSPTR // base pointer of a slice or string. - OCLOSUREREAD // read from inside closure struct at beginning of closure function - OCFUNC // reference to c function pointer (not go func value) - OCHECKNIL // emit code to ensure pointer/interface not nil - OVARDEF // variable is about to be fully initialized - OVARKILL // variable is dead - OVARLIVE // variable is alive - ORESULT // result of a function call; Xoffset is stack offset - OINLMARK // start of an inlined body, with file/line of caller. Xoffset is an index into the inline tree. - ONAMEOFFSET // offset within a name + OINLCALL // intermediary representation of an inlined call. + OEFACE // itable and data words of an empty-interface value. + OITAB // itable word of an interface value. + OIDATA // data word of an interface value in Left + OSPTR // base pointer of a slice or string. + OCFUNC // reference to c function pointer (not go func value) + OCHECKNIL // emit code to ensure pointer/interface not nil + OVARDEF // variable is about to be fully initialized + OVARKILL // variable is dead + OVARLIVE // variable is alive + ORESULT // result of a function call; Xoffset is stack offset + OINLMARK // start of an inlined body, with file/line of caller. Xoffset is an index into the inline tree. + ONAMEOFFSET // offset within a name // arch-specific opcodes ORETJMP // return to other function diff --git a/src/cmd/compile/internal/ir/node_gen.go b/src/cmd/compile/internal/ir/node_gen.go index 7f494b16cd..f1b0a21628 100644 --- a/src/cmd/compile/internal/ir/node_gen.go +++ b/src/cmd/compile/internal/ir/node_gen.go @@ -353,22 +353,6 @@ func (n *ClosureExpr) editChildren(edit func(Node) Node) { } } -func (n *ClosureReadExpr) Format(s fmt.State, verb rune) { fmtNode(n, s, verb) } -func (n *ClosureReadExpr) copy() Node { - c := *n - c.init = copyNodes(c.init) - return &c -} -func (n *ClosureReadExpr) doChildren(do func(Node) bool) bool { - if doNodes(n.init, do) { - return true - } - return false -} -func (n *ClosureReadExpr) editChildren(edit func(Node) Node) { - editNodes(n.init, edit) -} - func (n *CommClause) Format(s fmt.State, verb rune) { fmtNode(n, s, verb) } func (n *CommClause) copy() Node { c := *n diff --git a/src/cmd/compile/internal/ir/op_string.go b/src/cmd/compile/internal/ir/op_string.go index 0339444132..b54b4785a2 100644 --- a/src/cmd/compile/internal/ir/op_string.go +++ b/src/cmd/compile/internal/ir/op_string.go @@ -150,23 +150,22 @@ func _() { _ = x[OITAB-139] _ = x[OIDATA-140] _ = x[OSPTR-141] - _ = x[OCLOSUREREAD-142] - _ = x[OCFUNC-143] - _ = x[OCHECKNIL-144] - _ = x[OVARDEF-145] - _ = x[OVARKILL-146] - _ = x[OVARLIVE-147] - _ = x[ORESULT-148] - _ = x[OINLMARK-149] - _ = x[ONAMEOFFSET-150] - _ = x[ORETJMP-151] - _ = x[OGETG-152] - _ = x[OEND-153] + _ = x[OCFUNC-142] + _ = x[OCHECKNIL-143] + _ = x[OVARDEF-144] + _ = x[OVARKILL-145] + _ = x[OVARLIVE-146] + _ = x[ORESULT-147] + _ = x[OINLMARK-148] + _ = x[ONAMEOFFSET-149] + _ = x[ORETJMP-150] + _ = x[OGETG-151] + _ = x[OEND-152] } -const _Op_name = "XXXNAMENONAMETYPEPACKLITERALNILADDSUBORXORADDSTRADDRANDANDAPPENDBYTES2STRBYTES2STRTMPRUNES2STRSTR2BYTESSTR2BYTESTMPSTR2RUNESASAS2AS2DOTTYPEAS2FUNCAS2MAPRAS2RECVASOPCALLCALLFUNCCALLMETHCALLINTERCALLPARTCAPCLOSECLOSURECOMPLITMAPLITSTRUCTLITARRAYLITSLICELITPTRLITCONVCONVIFACECONVNOPCOPYDCLDCLFUNCDCLCONSTDCLTYPEDELETEDOTDOTPTRDOTMETHDOTINTERXDOTDOTTYPEDOTTYPE2EQNELTLEGEGTDEREFINDEXINDEXMAPKEYSTRUCTKEYLENMAKEMAKECHANMAKEMAPMAKESLICEMAKESLICECOPYMULDIVMODLSHRSHANDANDNOTNEWNEWOBJNOTBITNOTPLUSNEGORORPANICPRINTPRINTNPARENSENDSLICESLICEARRSLICESTRSLICE3SLICE3ARRSLICEHEADERRECOVERRECVRUNESTRSELRECV2IOTAREALIMAGCOMPLEXALIGNOFOFFSETOFSIZEOFMETHEXPRSTMTEXPRBLOCKBREAKCASECONTINUEDEFERFALLFORFORUNTILGOTOIFLABELGORANGERETURNSELECTSWITCHTYPESWTCHANTMAPTSTRUCTTINTERTFUNCTARRAYTSLICEINLCALLEFACEITABIDATASPTRCLOSUREREADCFUNCCHECKNILVARDEFVARKILLVARLIVERESULTINLMARKNAMEOFFSETRETJMPGETGEND" +const _Op_name = "XXXNAMENONAMETYPEPACKLITERALNILADDSUBORXORADDSTRADDRANDANDAPPENDBYTES2STRBYTES2STRTMPRUNES2STRSTR2BYTESSTR2BYTESTMPSTR2RUNESASAS2AS2DOTTYPEAS2FUNCAS2MAPRAS2RECVASOPCALLCALLFUNCCALLMETHCALLINTERCALLPARTCAPCLOSECLOSURECOMPLITMAPLITSTRUCTLITARRAYLITSLICELITPTRLITCONVCONVIFACECONVNOPCOPYDCLDCLFUNCDCLCONSTDCLTYPEDELETEDOTDOTPTRDOTMETHDOTINTERXDOTDOTTYPEDOTTYPE2EQNELTLEGEGTDEREFINDEXINDEXMAPKEYSTRUCTKEYLENMAKEMAKECHANMAKEMAPMAKESLICEMAKESLICECOPYMULDIVMODLSHRSHANDANDNOTNEWNEWOBJNOTBITNOTPLUSNEGORORPANICPRINTPRINTNPARENSENDSLICESLICEARRSLICESTRSLICE3SLICE3ARRSLICEHEADERRECOVERRECVRUNESTRSELRECV2IOTAREALIMAGCOMPLEXALIGNOFOFFSETOFSIZEOFMETHEXPRSTMTEXPRBLOCKBREAKCASECONTINUEDEFERFALLFORFORUNTILGOTOIFLABELGORANGERETURNSELECTSWITCHTYPESWTCHANTMAPTSTRUCTTINTERTFUNCTARRAYTSLICEINLCALLEFACEITABIDATASPTRCFUNCCHECKNILVARDEFVARKILLVARLIVERESULTINLMARKNAMEOFFSETRETJMPGETGEND" -var _Op_index = [...]uint16{0, 3, 7, 13, 17, 21, 28, 31, 34, 37, 39, 42, 48, 52, 58, 64, 73, 85, 94, 103, 115, 124, 126, 129, 139, 146, 153, 160, 164, 168, 176, 184, 193, 201, 204, 209, 216, 223, 229, 238, 246, 254, 260, 264, 273, 280, 284, 287, 294, 302, 309, 315, 318, 324, 331, 339, 343, 350, 358, 360, 362, 364, 366, 368, 370, 375, 380, 388, 391, 400, 403, 407, 415, 422, 431, 444, 447, 450, 453, 456, 459, 462, 468, 471, 477, 480, 486, 490, 493, 497, 502, 507, 513, 518, 522, 527, 535, 543, 549, 558, 569, 576, 580, 587, 595, 599, 603, 607, 614, 621, 629, 635, 643, 651, 656, 661, 665, 673, 678, 682, 685, 693, 697, 699, 704, 706, 711, 717, 723, 729, 735, 740, 744, 751, 757, 762, 768, 774, 781, 786, 790, 795, 799, 810, 815, 823, 829, 836, 843, 849, 856, 866, 872, 876, 879} +var _Op_index = [...]uint16{0, 3, 7, 13, 17, 21, 28, 31, 34, 37, 39, 42, 48, 52, 58, 64, 73, 85, 94, 103, 115, 124, 126, 129, 139, 146, 153, 160, 164, 168, 176, 184, 193, 201, 204, 209, 216, 223, 229, 238, 246, 254, 260, 264, 273, 280, 284, 287, 294, 302, 309, 315, 318, 324, 331, 339, 343, 350, 358, 360, 362, 364, 366, 368, 370, 375, 380, 388, 391, 400, 403, 407, 415, 422, 431, 444, 447, 450, 453, 456, 459, 462, 468, 471, 477, 480, 486, 490, 493, 497, 502, 507, 513, 518, 522, 527, 535, 543, 549, 558, 569, 576, 580, 587, 595, 599, 603, 607, 614, 621, 629, 635, 643, 651, 656, 661, 665, 673, 678, 682, 685, 693, 697, 699, 704, 706, 711, 717, 723, 729, 735, 740, 744, 751, 757, 762, 768, 774, 781, 786, 790, 795, 799, 804, 812, 818, 825, 832, 838, 845, 855, 861, 865, 868} func (i Op) String() string { if i >= Op(len(_Op_index)-1) { diff --git a/src/cmd/compile/internal/ssagen/ssa.go b/src/cmd/compile/internal/ssagen/ssa.go index 0c222b12cf..54bde20f1c 100644 --- a/src/cmd/compile/internal/ssagen/ssa.go +++ b/src/cmd/compile/internal/ssagen/ssa.go @@ -2168,9 +2168,6 @@ func (s *state) expr(n ir.Node) *ssa.Value { } addr := s.addr(n) return s.load(n.Type(), addr) - case ir.OCLOSUREREAD: - addr := s.addr(n) - return s.load(n.Type(), addr) case ir.ONIL: n := n.(*ir.NilExpr) t := n.Type() @@ -5074,10 +5071,6 @@ func (s *state) addr(n ir.Node) *ssa.Value { n := n.(*ir.SelectorExpr) p := s.exprPtr(n.X, n.Bounded(), n.Pos()) return s.newValue1I(ssa.OpOffPtr, t, n.Offset(), p) - case ir.OCLOSUREREAD: - n := n.(*ir.ClosureReadExpr) - return s.newValue1I(ssa.OpOffPtr, t, n.Offset, - s.entryNewValue0(ssa.OpGetClosurePtr, s.f.Config.Types.BytePtr)) case ir.OCONVNOP: n := n.(*ir.ConvExpr) if n.Type() == n.X.Type() { diff --git a/src/cmd/compile/internal/typecheck/typecheck.go b/src/cmd/compile/internal/typecheck/typecheck.go index 07bbd25105..3160725e3c 100644 --- a/src/cmd/compile/internal/typecheck/typecheck.go +++ b/src/cmd/compile/internal/typecheck/typecheck.go @@ -789,9 +789,6 @@ func typecheck1(n ir.Node, top int) ir.Node { n := n.(*ir.UnaryExpr) return tcSPtr(n) - case ir.OCLOSUREREAD: - return n - case ir.OCFUNC: n := n.(*ir.UnaryExpr) n.X = Expr(n.X) diff --git a/src/cmd/compile/internal/walk/expr.go b/src/cmd/compile/internal/walk/expr.go index 6fdb8f15f5..df575d6985 100644 --- a/src/cmd/compile/internal/walk/expr.go +++ b/src/cmd/compile/internal/walk/expr.go @@ -162,7 +162,7 @@ func walkExpr1(n ir.Node, init *ir.Nodes) ir.Node { n := n.(*ir.CallExpr) return mkcall("gorecover", n.Type(), init, typecheck.NodAddr(ir.RegFP)) - case ir.OCLOSUREREAD, ir.OCFUNC: + case ir.OCFUNC: return n case ir.OCALLINTER, ir.OCALLFUNC, ir.OCALLMETH: diff --git a/src/cmd/compile/internal/walk/walk.go b/src/cmd/compile/internal/walk/walk.go index 928b673752..e780a90660 100644 --- a/src/cmd/compile/internal/walk/walk.go +++ b/src/cmd/compile/internal/walk/walk.go @@ -476,7 +476,7 @@ func calcHasCall(n ir.Node) bool { n := n.(*ir.SelectorExpr) return n.X.HasCall() - case ir.OGETG, ir.OCLOSUREREAD, ir.OMETHEXPR: + case ir.OGETG, ir.OMETHEXPR: return false // TODO(rsc): These look wrong in various ways but are what calcHasCall has always done. From f57f484053f276c6fb57047cf02fa043974d7b95 Mon Sep 17 00:00:00 2001 From: Matthew Dempsky Date: Mon, 11 Jan 2021 14:30:16 -0800 Subject: [PATCH 07/16] [dev.regabi] cmd/compile: decouple escape analysis from Name.Vargen Escape analysis needs to know the index of result parameters for recording escape-flow information. It currently relies on Vargen for this, but it can easily figure this out for itself. So just do that instead, so that we can remove Vargen. Passes toolstash -cmp. For #43633. Change-Id: I65dedc2d73bc25e85ff400f308e50b73dc503630 Reviewed-on: https://go-review.googlesource.com/c/go/+/283192 Trust: Matthew Dempsky Trust: Dan Scales Run-TryBot: Matthew Dempsky TryBot-Result: Go Bot Reviewed-by: Dan Scales --- src/cmd/compile/internal/escape/escape.go | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/src/cmd/compile/internal/escape/escape.go b/src/cmd/compile/internal/escape/escape.go index c63383af43..bee3878f10 100644 --- a/src/cmd/compile/internal/escape/escape.go +++ b/src/cmd/compile/internal/escape/escape.go @@ -126,6 +126,11 @@ type location struct { edges []edge // incoming edges loopDepth int // loopDepth at declaration + // resultIndex records the tuple index (starting at 1) for + // PPARAMOUT variables within their function's result type. + // For non-PPARAMOUT variables it's 0. + resultIndex int + // derefs and walkgen are used during walkOne to track the // minimal dereferences from the walk root. derefs int // >= -1 @@ -259,11 +264,16 @@ func (b *batch) initFunc(fn *ir.Func) { } // Allocate locations for local variables. - for _, dcl := range fn.Dcl { - if dcl.Op() == ir.ONAME { - e.newLoc(dcl, false) + for _, n := range fn.Dcl { + if n.Op() == ir.ONAME { + e.newLoc(n, false) } } + + // Initialize resultIndex for result parameters. + for i, f := range fn.Type().Results().FieldSlice() { + e.oldLoc(f.Nname.(*ir.Name)).resultIndex = 1 + i + } } func (b *batch) walkFunc(fn *ir.Func) { @@ -1609,8 +1619,7 @@ func (l *location) leakTo(sink *location, derefs int) { // If sink is a result parameter and we can fit return bits // into the escape analysis tag, then record a return leak. if sink.isName(ir.PPARAMOUT) && sink.curfn == l.curfn { - // TODO(mdempsky): Eliminate dependency on Vargen here. - ri := int(sink.n.Name().Vargen) - 1 + ri := sink.resultIndex - 1 if ri < numEscResults { // Leak to result parameter. l.paramEsc.AddResult(ri, derefs) From b4d2a0445b0ca54a159e0895e1a8b31d47411894 Mon Sep 17 00:00:00 2001 From: Matthew Dempsky Date: Mon, 11 Jan 2021 15:58:19 -0800 Subject: [PATCH 08/16] [dev.regabi] cmd/compile: refactor closure var setup/teardown Creating closure vars is subtle and is also needed in both CL 281932 and CL 283112, so refactor out a common implementation that can be used in all 3 places. Passes toolstash -cmp. Change-Id: Ib993eb90c895b52759bfbfbaad88921e391b0b4d Reviewed-on: https://go-review.googlesource.com/c/go/+/283194 Run-TryBot: Matthew Dempsky TryBot-Result: Go Bot Reviewed-by: Dan Scales Trust: Dan Scales Trust: Matthew Dempsky --- src/cmd/compile/internal/ir/name.go | 76 +++++++++++++++++++++++++ src/cmd/compile/internal/noder/noder.go | 64 +-------------------- 2 files changed, 79 insertions(+), 61 deletions(-) diff --git a/src/cmd/compile/internal/ir/name.go b/src/cmd/compile/internal/ir/name.go index cfb481e31c..2375eddb99 100644 --- a/src/cmd/compile/internal/ir/name.go +++ b/src/cmd/compile/internal/ir/name.go @@ -351,6 +351,82 @@ func (n *Name) Byval() bool { return n.Canonical().flags&nameByval != 0 } +// CaptureName returns a Name suitable for referring to n from within function +// fn or from the package block if fn is nil. If n is a free variable declared +// within a function that encloses fn, then CaptureName returns a closure +// variable that refers to n and adds it to fn.ClosureVars. Otherwise, it simply +// returns n. +func CaptureName(pos src.XPos, fn *Func, n *Name) *Name { + if n.IsClosureVar() { + base.FatalfAt(pos, "misuse of CaptureName on closure variable: %v", n) + } + if n.Op() != ONAME || n.Curfn == nil || n.Curfn == fn { + return n // okay to use directly + } + if fn == nil { + base.FatalfAt(pos, "package-block reference to %v, declared in %v", n, n.Curfn) + } + + c := n.Innermost + if c != nil && c.Curfn == fn { + return c + } + + // Do not have a closure var for the active closure yet; make one. + c = NewNameAt(pos, n.Sym()) + c.Curfn = fn + c.Class = PAUTOHEAP + c.SetIsClosureVar(true) + c.Defn = n + + // Link into list of active closure variables. + // Popped from list in FinishCaptureNames. + c.Outer = n.Innermost + n.Innermost = c + fn.ClosureVars = append(fn.ClosureVars, c) + + return c +} + +// FinishCaptureNames handles any work leftover from calling CaptureName +// earlier. outerfn should be the function that immediately encloses fn. +func FinishCaptureNames(pos src.XPos, outerfn, fn *Func) { + // closure-specific variables are hanging off the + // ordinary ones; see CaptureName above. + // unhook them. + // make the list of pointers for the closure call. + for _, cv := range fn.ClosureVars { + // Unlink from n; see comment in syntax.go type Param for these fields. + n := cv.Defn.(*Name) + n.Innermost = cv.Outer + + // If the closure usage of n is not dense, we need to make it + // dense by recapturing n within the enclosing function. + // + // That is, suppose we just finished parsing the innermost + // closure f4 in this code: + // + // func f() { + // n := 1 + // func() { // f2 + // use(n) + // func() { // f3 + // func() { // f4 + // use(n) + // }() + // }() + // }() + // } + // + // At this point cv.Outer is f2's n; there is no n for f3. To + // construct the closure f4 from within f3, we need to use f3's + // n and in this case we need to create f3's n with CaptureName. + // + // We'll decide later in walk whether to use v directly or &v. + cv.Outer = CaptureName(pos, outerfn, n) + } +} + // SameSource reports whether two nodes refer to the same source // element. // diff --git a/src/cmd/compile/internal/noder/noder.go b/src/cmd/compile/internal/noder/noder.go index 76913c62a6..ec0debdbbd 100644 --- a/src/cmd/compile/internal/noder/noder.go +++ b/src/cmd/compile/internal/noder/noder.go @@ -1872,45 +1872,7 @@ func (p *noder) funcLit(expr *syntax.FuncLit) ir.Node { p.funcBody(fn, expr.Body) - // closure-specific variables are hanging off the - // ordinary ones in the symbol table; see oldname. - // unhook them. - // make the list of pointers for the closure call. - for _, v := range fn.ClosureVars { - // Unlink from v1; see comment in syntax.go type Param for these fields. - v1 := v.Defn - v1.Name().Innermost = v.Outer - - // If the closure usage of v is not dense, - // we need to make it dense; now that we're out - // of the function in which v appeared, - // look up v.Sym in the enclosing function - // and keep it around for use in the compiled code. - // - // That is, suppose we just finished parsing the innermost - // closure f4 in this code: - // - // func f() { - // v := 1 - // func() { // f2 - // use(v) - // func() { // f3 - // func() { // f4 - // use(v) - // }() - // }() - // }() - // } - // - // At this point v.Outer is f2's v; there is no f3's v. - // To construct the closure f4 from within f3, - // we need to use f3's v and in this case we need to create f3's v. - // We are now in the context of f3, so calling oldname(v.Sym) - // obtains f3's v, creating it if necessary (as it is in the example). - // - // capturevars will decide whether to use v directly or &v. - v.Outer = oldname(v.Sym()).(*ir.Name) - } + ir.FinishCaptureNames(base.Pos, ir.CurFunc, fn) return clo } @@ -1944,32 +1906,12 @@ func oldname(s *types.Sym) ir.Node { return ir.NewIdent(base.Pos, s) } - if ir.CurFunc != nil && n.Op() == ir.ONAME && n.Name().Curfn != nil && n.Name().Curfn != ir.CurFunc { - // Inner func is referring to var in outer func. - // + if n, ok := n.(*ir.Name); ok { // TODO(rsc): If there is an outer variable x and we // are parsing x := 5 inside the closure, until we get to // the := it looks like a reference to the outer x so we'll // make x a closure variable unnecessarily. - n := n.(*ir.Name) - c := n.Innermost - if c == nil || c.Curfn != ir.CurFunc { - // Do not have a closure var for the active closure yet; make one. - c = typecheck.NewName(s) - c.Class = ir.PAUTOHEAP - c.SetIsClosureVar(true) - c.Defn = n - - // Link into list of active closure variables. - // Popped from list in func funcLit. - c.Outer = n.Innermost - n.Innermost = c - - ir.CurFunc.ClosureVars = append(ir.CurFunc.ClosureVars, c) - } - - // return ref to closure var, not original - return c + return ir.CaptureName(base.Pos, ir.CurFunc, n) } return n From 12ee55ba7bf22157267e735e8e4bbf651c5b4e7d Mon Sep 17 00:00:00 2001 From: Matthew Dempsky Date: Mon, 11 Jan 2021 15:07:09 -0800 Subject: [PATCH 09/16] [dev.regabi] cmd/compile: stop using Vargen for import/export Historically, inline function bodies were exported as plain Go source code, and symbol mangling was a convenient hack because it allowed variables to be re-imported with largely the same names as they were originally exported as. However, nowadays we use a binary format that's more easily extended, so we can simply serialize all of a function's declared objects up front, and then refer to them by index later on. This also allows us to easily report unmangled names all the time (e.g., error message from issue7921.go). Fixes #43633. Change-Id: I46c88f5a47cb921f70ab140976ba9ddce38df216 Reviewed-on: https://go-review.googlesource.com/c/go/+/283193 Run-TryBot: Matthew Dempsky TryBot-Result: Go Bot Reviewed-by: Dan Scales Trust: Dan Scales Trust: Matthew Dempsky --- src/cmd/compile/internal/ir/func.go | 6 + src/cmd/compile/internal/ir/name.go | 8 +- src/cmd/compile/internal/typecheck/dcl.go | 27 +---- src/cmd/compile/internal/typecheck/iexport.go | 58 +++++----- src/cmd/compile/internal/typecheck/iimport.go | 103 +++++++++++++----- .../compile/internal/typecheck/typecheck.go | 2 +- test/fixedbugs/issue43633.dir/a.go | 28 +++++ test/fixedbugs/issue43633.dir/main.go | 18 +++ test/fixedbugs/issue43633.go | 7 ++ test/fixedbugs/issue7921.go | 2 +- 10 files changed, 171 insertions(+), 88 deletions(-) create mode 100644 test/fixedbugs/issue43633.dir/a.go create mode 100644 test/fixedbugs/issue43633.dir/main.go create mode 100644 test/fixedbugs/issue43633.go diff --git a/src/cmd/compile/internal/ir/func.go b/src/cmd/compile/internal/ir/func.go index 12ef083c19..d660fe3b40 100644 --- a/src/cmd/compile/internal/ir/func.go +++ b/src/cmd/compile/internal/ir/func.go @@ -61,8 +61,14 @@ type Func struct { // memory for escaping parameters. Enter Nodes Exit Nodes + // ONAME nodes for all params/locals for this func/closure, does NOT // include closurevars until transformclosure runs. + // Names must be listed PPARAMs, PPARAMOUTs, then PAUTOs, + // with PPARAMs and PPARAMOUTs in order corresponding to the function signature. + // However, as anonymous or blank PPARAMs are not actually declared, + // they are omitted from Dcl. + // Anonymous and blank PPARAMOUTs are declared as ~rNN and ~bNN Names, respectively. Dcl []*Name ClosureType Ntype // closure representation type diff --git a/src/cmd/compile/internal/ir/name.go b/src/cmd/compile/internal/ir/name.go index 2375eddb99..30f7e9b9e0 100644 --- a/src/cmd/compile/internal/ir/name.go +++ b/src/cmd/compile/internal/ir/name.go @@ -55,11 +55,9 @@ type Name struct { // The function, method, or closure in which local variable or param is declared. Curfn *Func - // Unique number for ONAME nodes within a function. Function outputs - // (results) are numbered starting at one, followed by function inputs - // (parameters), and then local variables. Vargen is used to distinguish - // local variables/params with the same name. - Vargen int32 + // Unique number for OTYPE names within a function. + // TODO(mdempsky): Remove completely. + Typegen int32 Ntype Ntype Heapaddr *Name // temp holding heap address of param diff --git a/src/cmd/compile/internal/typecheck/dcl.go b/src/cmd/compile/internal/typecheck/dcl.go index ffbf474a58..caa3e8203a 100644 --- a/src/cmd/compile/internal/typecheck/dcl.go +++ b/src/cmd/compile/internal/typecheck/dcl.go @@ -7,7 +7,6 @@ package typecheck import ( "fmt" "strconv" - "strings" "cmd/compile/internal/base" "cmd/compile/internal/ir" @@ -47,7 +46,6 @@ func Declare(n *ir.Name, ctxt ir.Class) { base.ErrorfAt(n.Pos(), "cannot declare name %v", s) } - gen := 0 if ctxt == ir.PEXTERN { if s.Name == "init" { base.ErrorfAt(n.Pos(), "cannot declare init - must be func") @@ -66,10 +64,7 @@ func Declare(n *ir.Name, ctxt ir.Class) { } if n.Op() == ir.OTYPE { declare_typegen++ - gen = declare_typegen - } else if n.Op() == ir.ONAME && ctxt == ir.PAUTO && !strings.Contains(s.Name, "·") { - vargen++ - gen = vargen + n.Typegen = int32(declare_typegen) } types.Pushdcl(s) n.Curfn = ir.CurFunc @@ -90,7 +85,6 @@ func Declare(n *ir.Name, ctxt ir.Class) { s.Block = types.Block s.Lastlineno = base.Pos s.Def = n - n.Vargen = int32(gen) n.Class = ctxt if ctxt == ir.PFUNC { n.Sym().SetFunc(true) @@ -338,9 +332,6 @@ func funcarg(n *ir.Field, ctxt ir.Class) { n.Decl = name name.Ntype = n.Ntype Declare(name, ctxt) - - vargen++ - n.Decl.Vargen = int32(vargen) } func funcarg2(f *types.Field, ctxt ir.Class) { @@ -358,15 +349,6 @@ func funcargs(nt *ir.FuncType) { base.Fatalf("funcargs %v", nt.Op()) } - // re-start the variable generation number - // we want to use small numbers for the return variables, - // so let them have the chunk starting at 1. - // - // TODO(mdempsky): This is ugly, and only necessary because - // esc.go uses Vargen to figure out result parameters' index - // within the result tuple. - vargen = len(nt.Results) - // declare the receiver and in arguments. if nt.Recv != nil { funcarg(nt.Recv, ir.PPARAM) @@ -375,9 +357,6 @@ func funcargs(nt *ir.FuncType) { funcarg(n, ir.PPARAM) } - oldvargen := vargen - vargen = 0 - // declare the out arguments. gen := len(nt.Params) for _, n := range nt.Results { @@ -399,8 +378,6 @@ func funcargs(nt *ir.FuncType) { funcarg(n, ir.PPARAMOUT) } - - vargen = oldvargen } // Same as funcargs, except run over an already constructed TFUNC. @@ -422,8 +399,6 @@ func funcargs2(t *types.Type) { } } -var vargen int - func Temp(t *types.Type) *ir.Name { return TempAt(base.Pos, ir.CurFunc, t) } diff --git a/src/cmd/compile/internal/typecheck/iexport.go b/src/cmd/compile/internal/typecheck/iexport.go index a7927c39a3..4d48b80346 100644 --- a/src/cmd/compile/internal/typecheck/iexport.go +++ b/src/cmd/compile/internal/typecheck/iexport.go @@ -422,6 +422,10 @@ type exportWriter struct { prevFile string prevLine int64 prevColumn int64 + + // dclIndex maps function-scoped declarations to their index + // within their respective Func's Dcl list. + dclIndex map[*ir.Name]int } func (p *iexporter) doDecl(n *ir.Name) { @@ -529,7 +533,8 @@ func (p *iexporter) doInline(f *ir.Name) { w := p.newWriter() w.setPkg(fnpkg(f), false) - w.stmtList(ir.Nodes(f.Func.Inl.Body)) + w.dclIndex = make(map[*ir.Name]int, len(f.Func.Inl.Dcl)) + w.funcBody(f.Func) w.finish("inl", p.inlineIndex, f.Sym()) } @@ -756,7 +761,7 @@ func (w *exportWriter) paramList(fs []*types.Field) { func (w *exportWriter) param(f *types.Field) { w.pos(f.Pos) - w.localIdent(types.OrigSym(f.Sym), 0) + w.localIdent(types.OrigSym(f.Sym)) w.typ(f.Type) } @@ -1030,7 +1035,19 @@ func (w *exportWriter) typeExt(t *types.Type) { // Inline bodies. -func (w *exportWriter) stmtList(list ir.Nodes) { +func (w *exportWriter) funcBody(fn *ir.Func) { + w.int64(int64(len(fn.Inl.Dcl))) + for i, n := range fn.Inl.Dcl { + w.pos(n.Pos()) + w.localIdent(n.Sym()) + w.typ(n.Type()) + w.dclIndex[n] = i + } + + w.stmtList(fn.Inl.Body) +} + +func (w *exportWriter) stmtList(list []ir.Node) { for _, n := range list { w.node(n) } @@ -1070,10 +1087,11 @@ func (w *exportWriter) stmt(n ir.Node) { case ir.ODCL: n := n.(*ir.Decl) + if ir.IsBlank(n.X) { + return // blank declarations not useful to importers + } w.op(ir.ODCL) - w.pos(n.X.Pos()) w.localName(n.X) - w.typ(n.X.Type()) case ir.OAS: // Don't export "v = " initializing statements, hope they're always @@ -1288,7 +1306,7 @@ func (w *exportWriter) expr(n ir.Node) { } s = n.Tag.Sym() } - w.localIdent(s, 0) // declared pseudo-variable, if any + w.localIdent(s) // declared pseudo-variable, if any w.expr(n.X) // case OTARRAY, OTMAP, OTCHAN, OTSTRUCT, OTINTER, OTFUNC: @@ -1518,22 +1536,19 @@ func (w *exportWriter) fieldList(list ir.Nodes) { } func (w *exportWriter) localName(n *ir.Name) { - // Escape analysis happens after inline bodies are saved, but - // we're using the same ONAME nodes, so we might still see - // PAUTOHEAP here. - // - // Check for Stackcopy to identify PAUTOHEAP that came from - // PPARAM/PPARAMOUT, because we only want to include vargen in - // non-param names. - var v int32 - if n.Class == ir.PAUTO || (n.Class == ir.PAUTOHEAP && n.Stackcopy == nil) { - v = n.Vargen + if ir.IsBlank(n) { + w.int64(-1) + return } - w.localIdent(n.Sym(), v) + i, ok := w.dclIndex[n] + if !ok { + base.FatalfAt(n.Pos(), "missing from dclIndex: %+v", n) + } + w.int64(int64(i)) } -func (w *exportWriter) localIdent(s *types.Sym, v int32) { +func (w *exportWriter) localIdent(s *types.Sym) { if w.currPkg == nil { base.Fatalf("missing currPkg") } @@ -1555,13 +1570,6 @@ func (w *exportWriter) localIdent(s *types.Sym, v int32) { base.Fatalf("unexpected dot in identifier: %v", name) } - if v > 0 { - if strings.Contains(name, "·") { - base.Fatalf("exporter: unexpected · in symbol name") - } - name = fmt.Sprintf("%s·%d", name, v) - } - if s.Pkg != w.currPkg { base.Fatalf("weird package in name: %v => %v from %q, not %q", s, name, s.Pkg.Path, w.currPkg.Path) } diff --git a/src/cmd/compile/internal/typecheck/iimport.go b/src/cmd/compile/internal/typecheck/iimport.go index 15c57b2380..c9effabce0 100644 --- a/src/cmd/compile/internal/typecheck/iimport.go +++ b/src/cmd/compile/internal/typecheck/iimport.go @@ -262,6 +262,9 @@ type importReader struct { prevBase *src.PosBase prevLine int64 prevColumn int64 + + // curfn is the current function we're importing into. + curfn *ir.Func } func (p *iimporter) newReader(off uint64, pkg *types.Pkg) *importReader { @@ -715,19 +718,7 @@ func (r *importReader) doInline(fn *ir.Func) { base.Fatalf("%v already has inline body", fn) } - StartFuncBody(fn) - body := r.stmtList() - FinishFuncBody() - if body == nil { - // - // Make sure empty body is not interpreted as - // no inlineable body (see also parser.fnbody) - // (not doing so can cause significant performance - // degradation due to unnecessary calls to empty - // functions). - body = []ir.Node{} - } - fn.Inl.Body = body + r.funcBody(fn) importlist = append(importlist, fn) @@ -755,6 +746,68 @@ func (r *importReader) doInline(fn *ir.Func) { // unrefined nodes (since this is what the importer uses). The respective case // entries are unreachable in the importer. +func (r *importReader) funcBody(fn *ir.Func) { + outerfn := r.curfn + r.curfn = fn + + // Import local declarations. + dcls := make([]*ir.Name, r.int64()) + for i := range dcls { + n := ir.NewDeclNameAt(r.pos(), ir.ONAME, r.localIdent()) + n.Class = ir.PAUTO // overwritten below for parameters/results + n.Curfn = fn + n.SetType(r.typ()) + dcls[i] = n + } + fn.Inl.Dcl = dcls + + // Fixup parameter classes and associate with their + // signature's type fields. + i := 0 + fix := func(f *types.Field, class ir.Class) { + if class == ir.PPARAM && (f.Sym == nil || f.Sym.Name == "_") { + return + } + n := dcls[i] + n.Class = class + f.Nname = n + i++ + } + + typ := fn.Type() + if recv := typ.Recv(); recv != nil { + fix(recv, ir.PPARAM) + } + for _, f := range typ.Params().FieldSlice() { + fix(f, ir.PPARAM) + } + for _, f := range typ.Results().FieldSlice() { + fix(f, ir.PPARAMOUT) + } + + // Import function body. + body := r.stmtList() + if body == nil { + // Make sure empty body is not interpreted as + // no inlineable body (see also parser.fnbody) + // (not doing so can cause significant performance + // degradation due to unnecessary calls to empty + // functions). + body = []ir.Node{} + } + fn.Inl.Body = body + + r.curfn = outerfn +} + +func (r *importReader) localName() *ir.Name { + i := r.int64() + if i < 0 { + return ir.BlankNode.(*ir.Name) + } + return r.curfn.Inl.Dcl[i] +} + func (r *importReader) stmtList() []ir.Node { var list []ir.Node for { @@ -784,13 +837,8 @@ func (r *importReader) caseList(switchExpr ir.Node) []*ir.CaseClause { cas := ir.NewCaseStmt(r.pos(), nil, nil) cas.List = r.stmtList() if namedTypeSwitch { - // Note: per-case variables will have distinct, dotted - // names after import. That's okay: swt.go only needs - // Sym for diagnostics anyway. - caseVar := ir.NewNameAt(cas.Pos(), r.localIdent()) - Declare(caseVar, DeclContext) - cas.Var = caseVar - caseVar.Defn = switchExpr + cas.Var = r.localName() + cas.Var.Defn = switchExpr } cas.Body = r.stmtList() cases[i] = cas @@ -854,7 +902,7 @@ func (r *importReader) node() ir.Node { return r.qualifiedIdent() case ir.ONAME: - return r.localIdent().Def.(*ir.Name) + return r.localName() // case OPACK, ONONAME: // unreachable - should have been resolved by typechecking @@ -991,16 +1039,11 @@ func (r *importReader) node() ir.Node { // -------------------------------------------------------------------- // statements case ir.ODCL: - pos := r.pos() - lhs := ir.NewDeclNameAt(pos, ir.ONAME, r.localIdent()) - lhs.SetType(r.typ()) - - Declare(lhs, ir.PAUTO) - var stmts ir.Nodes - stmts.Append(ir.NewDecl(base.Pos, ir.ODCL, lhs)) - stmts.Append(ir.NewAssignStmt(base.Pos, lhs, nil)) - return ir.NewBlockStmt(pos, stmts) + n := r.localName() + stmts.Append(ir.NewDecl(n.Pos(), ir.ODCL, n)) + stmts.Append(ir.NewAssignStmt(n.Pos(), n, nil)) + return ir.NewBlockStmt(n.Pos(), stmts) // case OAS, OASWB: // unreachable - mapped to OAS case below by exporter diff --git a/src/cmd/compile/internal/typecheck/typecheck.go b/src/cmd/compile/internal/typecheck/typecheck.go index 3160725e3c..431fb04bef 100644 --- a/src/cmd/compile/internal/typecheck/typecheck.go +++ b/src/cmd/compile/internal/typecheck/typecheck.go @@ -1687,7 +1687,7 @@ func typecheckdeftype(n *ir.Name) { } t := types.NewNamed(n) - t.Vargen = n.Vargen + t.Vargen = n.Typegen if n.Pragma()&ir.NotInHeap != 0 { t.SetNotInHeap(true) } diff --git a/test/fixedbugs/issue43633.dir/a.go b/test/fixedbugs/issue43633.dir/a.go new file mode 100644 index 0000000000..946a37e87e --- /dev/null +++ b/test/fixedbugs/issue43633.dir/a.go @@ -0,0 +1,28 @@ +// Copyright 2021 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 F() bool { + { + x := false + _ = x + } + if false { + _ = func(x bool) {} + } + x := true + return x +} + +func G() func() bool { + x := true + return func() bool { + { + x := false + _ = x + } + return x + } +} diff --git a/test/fixedbugs/issue43633.dir/main.go b/test/fixedbugs/issue43633.dir/main.go new file mode 100644 index 0000000000..320e00013c --- /dev/null +++ b/test/fixedbugs/issue43633.dir/main.go @@ -0,0 +1,18 @@ +// Copyright 2021 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" + +var g = a.G() + +func main() { + if !a.F() { + panic("FAIL") + } + if !g() { + panic("FAIL") + } +} diff --git a/test/fixedbugs/issue43633.go b/test/fixedbugs/issue43633.go new file mode 100644 index 0000000000..40df49f83b --- /dev/null +++ b/test/fixedbugs/issue43633.go @@ -0,0 +1,7 @@ +// rundir + +// Copyright 2021 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 diff --git a/test/fixedbugs/issue7921.go b/test/fixedbugs/issue7921.go index 5dce557ca3..a4e7b246d4 100644 --- a/test/fixedbugs/issue7921.go +++ b/test/fixedbugs/issue7921.go @@ -41,7 +41,7 @@ func bufferNoEscape3(xs []string) string { // ERROR "xs does not escape$" func bufferNoEscape4() []byte { var b bytes.Buffer - b.Grow(64) // ERROR "bufferNoEscape4 ignoring self-assignment in bytes.b.buf = bytes.b.buf\[:bytes.m·3\]$" "inlining call to bytes.\(\*Buffer\).Grow$" + b.Grow(64) // ERROR "bufferNoEscape4 ignoring self-assignment in bytes.b.buf = bytes.b.buf\[:bytes.m\]$" "inlining call to bytes.\(\*Buffer\).Grow$" useBuffer(&b) return b.Bytes() // ERROR "inlining call to bytes.\(\*Buffer\).Bytes$" } From 95acd8121bf76a15ecba0259367dca0efe6d3a77 Mon Sep 17 00:00:00 2001 From: Matthew Dempsky Date: Mon, 11 Jan 2021 17:22:20 -0800 Subject: [PATCH 10/16] [dev.regabi] cmd/compile: remove Name.Typegen Just directly set Type.Vargen when declaring defined types within a function. Change-Id: Idcc0007084a660ce1c39da4a3697e158a1c615b5 Reviewed-on: https://go-review.googlesource.com/c/go/+/283212 Trust: Matthew Dempsky Trust: Dan Scales Run-TryBot: Matthew Dempsky TryBot-Result: Go Bot Reviewed-by: Dan Scales --- src/cmd/compile/internal/ir/name.go | 4 ---- src/cmd/compile/internal/ir/sizeof_test.go | 2 +- src/cmd/compile/internal/typecheck/dcl.go | 8 -------- src/cmd/compile/internal/typecheck/typecheck.go | 11 ++++++++++- 4 files changed, 11 insertions(+), 14 deletions(-) diff --git a/src/cmd/compile/internal/ir/name.go b/src/cmd/compile/internal/ir/name.go index 30f7e9b9e0..514b303893 100644 --- a/src/cmd/compile/internal/ir/name.go +++ b/src/cmd/compile/internal/ir/name.go @@ -55,10 +55,6 @@ type Name struct { // The function, method, or closure in which local variable or param is declared. Curfn *Func - // Unique number for OTYPE names within a function. - // TODO(mdempsky): Remove completely. - Typegen int32 - Ntype Ntype Heapaddr *Name // temp holding heap address of param diff --git a/src/cmd/compile/internal/ir/sizeof_test.go b/src/cmd/compile/internal/ir/sizeof_test.go index 1a4d2e5c7a..2ada7231aa 100644 --- a/src/cmd/compile/internal/ir/sizeof_test.go +++ b/src/cmd/compile/internal/ir/sizeof_test.go @@ -21,7 +21,7 @@ func TestSizeof(t *testing.T) { _64bit uintptr // size on 64bit platforms }{ {Func{}, 184, 320}, - {Name{}, 120, 216}, + {Name{}, 116, 208}, } for _, tt := range tests { diff --git a/src/cmd/compile/internal/typecheck/dcl.go b/src/cmd/compile/internal/typecheck/dcl.go index caa3e8203a..c7d7506fd1 100644 --- a/src/cmd/compile/internal/typecheck/dcl.go +++ b/src/cmd/compile/internal/typecheck/dcl.go @@ -62,10 +62,6 @@ func Declare(n *ir.Name, ctxt ir.Class) { if ir.CurFunc != nil && ctxt != ir.PFUNC && n.Op() == ir.ONAME { ir.CurFunc.Dcl = append(ir.CurFunc.Dcl, n) } - if n.Op() == ir.OTYPE { - declare_typegen++ - n.Typegen = int32(declare_typegen) - } types.Pushdcl(s) n.Curfn = ir.CurFunc } @@ -308,10 +304,6 @@ func checkembeddedtype(t *types.Type) { } } -// declare individual names - var, typ, const - -var declare_typegen int - func fakeRecvField() *types.Field { return types.NewField(src.NoXPos, nil, types.FakeRecvType()) } diff --git a/src/cmd/compile/internal/typecheck/typecheck.go b/src/cmd/compile/internal/typecheck/typecheck.go index 431fb04bef..3fc077b00c 100644 --- a/src/cmd/compile/internal/typecheck/typecheck.go +++ b/src/cmd/compile/internal/typecheck/typecheck.go @@ -1681,13 +1681,22 @@ func CheckMapKeys() { mapqueue = nil } +// typegen tracks the number of function-scoped defined types that +// have been declared. It's used to generate unique linker symbols for +// their runtime type descriptors. +var typegen int32 + func typecheckdeftype(n *ir.Name) { if base.EnableTrace && base.Flag.LowerT { defer tracePrint("typecheckdeftype", n)(nil) } t := types.NewNamed(n) - t.Vargen = n.Typegen + if n.Curfn != nil { + typegen++ + t.Vargen = typegen + } + if n.Pragma()&ir.NotInHeap != 0 { t.SetNotInHeap(true) } From cd5b74d2dfe6009d55c86e90f6c204e58c229c16 Mon Sep 17 00:00:00 2001 From: Matthew Dempsky Date: Tue, 12 Jan 2021 11:34:00 -0800 Subject: [PATCH 11/16] [dev.regabi] cmd/compile: call NeedFuncSym in InitLSym InitLSym is where we're now generating ABI wrappers, so it seems as good a place as any to make sure we're generating the degenerate closure wrappers for declared functions and methods. Change-Id: I097f34bbcee65dee87a97f9ed6f3f38e4cf2e2b5 Reviewed-on: https://go-review.googlesource.com/c/go/+/283312 Run-TryBot: Matthew Dempsky TryBot-Result: Go Bot Trust: Matthew Dempsky Reviewed-by: Keith Randall --- src/cmd/compile/internal/gc/main.go | 2 -- src/cmd/compile/internal/ssagen/abi.go | 5 ++++- src/cmd/compile/internal/staticdata/data.go | 13 ++++++++----- src/cmd/compile/internal/typecheck/func.go | 4 ---- src/cmd/compile/internal/typecheck/typecheck.go | 7 ------- 5 files changed, 12 insertions(+), 19 deletions(-) diff --git a/src/cmd/compile/internal/gc/main.go b/src/cmd/compile/internal/gc/main.go index c3756309ea..1541bc4285 100644 --- a/src/cmd/compile/internal/gc/main.go +++ b/src/cmd/compile/internal/gc/main.go @@ -20,7 +20,6 @@ import ( "cmd/compile/internal/reflectdata" "cmd/compile/internal/ssa" "cmd/compile/internal/ssagen" - "cmd/compile/internal/staticdata" "cmd/compile/internal/typecheck" "cmd/compile/internal/types" "cmd/compile/internal/walk" @@ -194,7 +193,6 @@ func Main(archInit func(*ssagen.ArchInfo)) { typecheck.Target = new(ir.Package) - typecheck.NeedFuncSym = staticdata.NeedFuncSym typecheck.NeedITab = func(t, iface *types.Type) { reflectdata.ITabAddr(t, iface) } typecheck.NeedRuntimeType = reflectdata.NeedRuntimeType // TODO(rsc): typenamesym for lock? diff --git a/src/cmd/compile/internal/ssagen/abi.go b/src/cmd/compile/internal/ssagen/abi.go index 1c013dd2d8..dc27ec3a29 100644 --- a/src/cmd/compile/internal/ssagen/abi.go +++ b/src/cmd/compile/internal/ssagen/abi.go @@ -14,6 +14,7 @@ import ( "cmd/compile/internal/base" "cmd/compile/internal/escape" "cmd/compile/internal/ir" + "cmd/compile/internal/staticdata" "cmd/compile/internal/typecheck" "cmd/compile/internal/types" "cmd/internal/obj" @@ -137,6 +138,8 @@ func ReadSymABIs(file, myimportpath string) { // For body-less functions, we only create the LSym; for functions // with bodies call a helper to setup up / populate the LSym. func InitLSym(f *ir.Func, hasBody bool) { + staticdata.NeedFuncSym(f.Sym()) + // FIXME: for new-style ABI wrappers, we set up the lsym at the // point the wrapper is created. if f.LSym != nil && base.Flag.ABIWrap { @@ -152,7 +155,7 @@ func InitLSym(f *ir.Func, hasBody bool) { // makes calls to helpers to create ABI wrappers if needed. func selectLSym(f *ir.Func, hasBody bool) { if f.LSym != nil { - base.Fatalf("Func.initLSym called twice") + base.FatalfAt(f.Pos(), "Func.initLSym called twice on %v", f) } if nam := f.Nname; !ir.IsBlank(nam) { diff --git a/src/cmd/compile/internal/staticdata/data.go b/src/cmd/compile/internal/staticdata/data.go index a2a844f940..4b12590fde 100644 --- a/src/cmd/compile/internal/staticdata/data.go +++ b/src/cmd/compile/internal/staticdata/data.go @@ -265,7 +265,7 @@ func FuncLinksym(n *ir.Name) *obj.LSym { return FuncSym(n.Sym()).Linksym() } -// NeedFuncSym ensures that s·f is exported. +// NeedFuncSym ensures that s·f is exported, if needed. // It is only used with -dynlink. // When not compiling for dynamic linking, // the funcsyms are created as needed by @@ -275,8 +275,13 @@ func FuncLinksym(n *ir.Name) *obj.LSym { // So instead, when dynamic linking, we only create // the s·f stubs in s's package. func NeedFuncSym(s *types.Sym) { + if base.Ctxt.InParallel { + // The append below probably just needs to lock + // funcsymsmu, like in FuncSym. + base.Fatalf("NeedFuncSym must be called in serial") + } if !base.Ctxt.Flag_dynlink { - base.Fatalf("NeedFuncSym: dynlink") + return } if s.IsBlank() { return @@ -287,9 +292,7 @@ func NeedFuncSym(s *types.Sym) { // get funcsyms. return } - if _, existed := s.Pkg.LookupOK(ir.FuncSymName(s)); !existed { - funcsyms = append(funcsyms, s) - } + funcsyms = append(funcsyms, s) } func WriteFuncSyms() { diff --git a/src/cmd/compile/internal/typecheck/func.go b/src/cmd/compile/internal/typecheck/func.go index 12762f7ee8..8f7411daec 100644 --- a/src/cmd/compile/internal/typecheck/func.go +++ b/src/cmd/compile/internal/typecheck/func.go @@ -364,10 +364,6 @@ func tcFunc(n *ir.Func) { n.Nname.SetSym(ir.MethodSym(rcvr.Type, n.Shortname)) Declare(n.Nname, ir.PFUNC) } - - if base.Ctxt.Flag_dynlink && !inimport && n.Nname != nil { - NeedFuncSym(n.Sym()) - } } // tcCall typechecks an OCALL node. diff --git a/src/cmd/compile/internal/typecheck/typecheck.go b/src/cmd/compile/internal/typecheck/typecheck.go index 3fc077b00c..814af59772 100644 --- a/src/cmd/compile/internal/typecheck/typecheck.go +++ b/src/cmd/compile/internal/typecheck/typecheck.go @@ -24,7 +24,6 @@ var inimport bool // set during import var TypecheckAllowed bool var ( - NeedFuncSym = func(*types.Sym) {} NeedITab = func(t, itype *types.Type) {} NeedRuntimeType = func(*types.Type) {} ) @@ -1140,12 +1139,6 @@ func typecheckMethodExpr(n *ir.SelectorExpr) (res ir.Node) { n.SetOp(ir.OMETHEXPR) n.Selection = m n.SetType(NewMethodType(m.Type, n.X.Type())) - - // Issue 25065. Make sure that we emit the symbol for a local method. - if base.Ctxt.Flag_dynlink && !inimport && (t.Sym() == nil || t.Sym().Pkg == types.LocalPkg) { - NeedFuncSym(n.FuncName().Sym()) - } - return n } From cc90e7a51e15659ea1a1eb53ca08361b6a77696a Mon Sep 17 00:00:00 2001 From: Matthew Dempsky Date: Tue, 12 Jan 2021 11:38:32 -0800 Subject: [PATCH 12/16] [dev.regabi] cmd/compile: always use the compile queue The compiler currently has two modes for compilation: one where it compiles each function as it sees them, and another where it enqueues them all into a work queue. A subsequent CL is going to reorder function compilation to ensure that functions are always compiled before any non-trivial function literals they enclose, and this will be easier if we always use the compile work queue. Also, fewer compilation modes makes things simpler to reason about. Change-Id: Ie090e81f7476c49486296f2b90911fa0a466a5dd Reviewed-on: https://go-review.googlesource.com/c/go/+/283313 Run-TryBot: Matthew Dempsky TryBot-Result: Go Bot Trust: Matthew Dempsky Reviewed-by: Keith Randall --- src/cmd/compile/internal/base/debug.go | 1 - src/cmd/compile/internal/gc/compile.go | 87 ++++++---------------- src/cmd/compile/internal/gc/main.go | 5 +- src/cmd/compile/internal/gc/obj.go | 5 +- src/cmd/compile/internal/liveness/plive.go | 1 + test/fixedbugs/issue20250.go | 2 +- 6 files changed, 28 insertions(+), 73 deletions(-) diff --git a/src/cmd/compile/internal/base/debug.go b/src/cmd/compile/internal/base/debug.go index 3acdcea846..164941bb26 100644 --- a/src/cmd/compile/internal/base/debug.go +++ b/src/cmd/compile/internal/base/debug.go @@ -32,7 +32,6 @@ type DebugFlags struct { Append int `help:"print information about append compilation"` Checkptr int `help:"instrument unsafe pointer conversions"` Closure int `help:"print information about closure compilation"` - CompileLater int `help:"compile functions as late as possible"` DclStack int `help:"run internal dclstack check"` Defer int `help:"print information about defer compilation"` DisableNil int `help:"disable nil checks"` diff --git a/src/cmd/compile/internal/gc/compile.go b/src/cmd/compile/internal/gc/compile.go index 25b1c76737..b9c10056b4 100644 --- a/src/cmd/compile/internal/gc/compile.go +++ b/src/cmd/compile/internal/gc/compile.go @@ -26,21 +26,17 @@ var ( compilequeue []*ir.Func // functions waiting to be compiled ) -func funccompile(fn *ir.Func) { +func enqueueFunc(fn *ir.Func) { if ir.CurFunc != nil { - base.Fatalf("funccompile %v inside %v", fn.Sym(), ir.CurFunc.Sym()) + base.FatalfAt(fn.Pos(), "enqueueFunc %v inside %v", fn, ir.CurFunc) } - if fn.Type() == nil { - if base.Errors() == 0 { - base.Fatalf("funccompile missing type") - } + if ir.FuncName(fn) == "_" { + // Skip compiling blank functions. + // Frontend already reported any spec-mandated errors (#29870). return } - // assign parameter offsets - types.CalcSize(fn.Type()) - if len(fn.Body) == 0 { // Initialize ABI wrappers if necessary. ssagen.InitLSym(fn, false) @@ -48,35 +44,31 @@ func funccompile(fn *ir.Func) { return } - typecheck.DeclContext = ir.PAUTO - ir.CurFunc = fn - compile(fn) - ir.CurFunc = nil - typecheck.DeclContext = ir.PEXTERN + errorsBefore := base.Errors() + prepareFunc(fn) + if base.Errors() > errorsBefore { + return + } + + compilequeue = append(compilequeue, fn) } -func compile(fn *ir.Func) { +// prepareFunc handles any remaining frontend compilation tasks that +// aren't yet safe to perform concurrently. +func prepareFunc(fn *ir.Func) { // Set up the function's LSym early to avoid data races with the assemblers. // Do this before walk, as walk needs the LSym to set attributes/relocations // (e.g. in markTypeUsedInInterface). ssagen.InitLSym(fn, true) - errorsBefore := base.Errors() + // Calculate parameter offsets. + types.CalcSize(fn.Type()) + + typecheck.DeclContext = ir.PAUTO + ir.CurFunc = fn walk.Walk(fn) - if base.Errors() > errorsBefore { - return - } - - // From this point, there should be no uses of Curfn. Enforce that. - ir.CurFunc = nil - - if ir.FuncName(fn) == "_" { - // We don't need to generate code for this function, just report errors in its body. - // At this point we've generated any errors needed. - // (Beyond here we generate only non-spec errors, like "stack frame too large".) - // See issue 29870. - return - } + ir.CurFunc = nil // enforce no further uses of CurFunc + typecheck.DeclContext = ir.PEXTERN // Make sure type syms are declared for all types that might // be types of stack objects. We need to do this here @@ -95,28 +87,6 @@ func compile(fn *ir.Func) { } } } - - if compilenow(fn) { - ssagen.Compile(fn, 0) - } else { - compilequeue = append(compilequeue, fn) - } -} - -// compilenow reports whether to compile immediately. -// If functions are not compiled immediately, -// they are enqueued in compilequeue, -// which is drained by compileFunctions. -func compilenow(fn *ir.Func) bool { - // Issue 38068: if this function is a method AND an inline - // candidate AND was not inlined (yet), put it onto the compile - // queue instead of compiling it immediately. This is in case we - // wind up inlining it into a method wrapper that is generated by - // compiling a function later on in the Target.Decls list. - if ir.IsMethod(fn) && isInlinableButNotInlined(fn) { - return false - } - return base.Flag.LowerC == 1 && base.Debug.CompileLater == 0 } // compileFunctions compiles all functions in compilequeue. @@ -163,16 +133,3 @@ func compileFunctions() { types.CalcSizeDisabled = false } } - -// isInlinableButNotInlined returns true if 'fn' was marked as an -// inline candidate but then never inlined (presumably because we -// found no call sites). -func isInlinableButNotInlined(fn *ir.Func) bool { - if fn.Inl == nil { - return false - } - if fn.Sym() == nil { - return true - } - return !fn.Linksym().WasInlined() -} diff --git a/src/cmd/compile/internal/gc/main.go b/src/cmd/compile/internal/gc/main.go index 1541bc4285..2903d64ff8 100644 --- a/src/cmd/compile/internal/gc/main.go +++ b/src/cmd/compile/internal/gc/main.go @@ -300,9 +300,8 @@ func Main(archInit func(*ssagen.ArchInfo)) { base.Timer.Start("be", "compilefuncs") fcount := int64(0) for i := 0; i < len(typecheck.Target.Decls); i++ { - n := typecheck.Target.Decls[i] - if n.Op() == ir.ODCLFUNC { - funccompile(n.(*ir.Func)) + if fn, ok := typecheck.Target.Decls[i].(*ir.Func); ok { + enqueueFunc(fn) fcount++ } } diff --git a/src/cmd/compile/internal/gc/obj.go b/src/cmd/compile/internal/gc/obj.go index fbb2145e1b..753db80f76 100644 --- a/src/cmd/compile/internal/gc/obj.go +++ b/src/cmd/compile/internal/gc/obj.go @@ -131,9 +131,8 @@ func dumpdata() { // It was not until issue 24761 that we found any code that required a loop at all. for { for i := numDecls; i < len(typecheck.Target.Decls); i++ { - n := typecheck.Target.Decls[i] - if n.Op() == ir.ODCLFUNC { - funccompile(n.(*ir.Func)) + if n, ok := typecheck.Target.Decls[i].(*ir.Func); ok { + enqueueFunc(n) } } numDecls = len(typecheck.Target.Decls) diff --git a/src/cmd/compile/internal/liveness/plive.go b/src/cmd/compile/internal/liveness/plive.go index 26d90824b2..8d1754c813 100644 --- a/src/cmd/compile/internal/liveness/plive.go +++ b/src/cmd/compile/internal/liveness/plive.go @@ -1223,6 +1223,7 @@ func WriteFuncMap(fn *ir.Func) { if ir.FuncName(fn) == "_" || fn.Sym().Linkname != "" { return } + types.CalcSize(fn.Type()) lsym := base.Ctxt.Lookup(fn.LSym.Name + ".args_stackmap") nptr := int(fn.Type().ArgWidth() / int64(types.PtrSize)) bv := bitvec.New(int32(nptr) * 2) diff --git a/test/fixedbugs/issue20250.go b/test/fixedbugs/issue20250.go index c190515274..1a513bea56 100644 --- a/test/fixedbugs/issue20250.go +++ b/test/fixedbugs/issue20250.go @@ -1,4 +1,4 @@ -// errorcheck -0 -live -l -d=compilelater +// errorcheck -0 -live -l // Copyright 2017 The Go Authors. All rights reserved. // Use of this source code is governed by a BSD-style From 432f9ffb11231b00b67c8fa8047f21a8282fa914 Mon Sep 17 00:00:00 2001 From: Matthew Dempsky Date: Tue, 12 Jan 2021 11:39:10 -0800 Subject: [PATCH 13/16] [dev.regabi] cmd/compile: unindent compileFunctions No real code changes. Just splitting into a separate CL so the next one is easier to review. Change-Id: I428dc986b76370d8d3afc12cf19585f6384389d7 Reviewed-on: https://go-review.googlesource.com/c/go/+/283314 Run-TryBot: Matthew Dempsky TryBot-Result: Go Bot Trust: Matthew Dempsky Reviewed-by: Keith Randall --- src/cmd/compile/internal/gc/compile.go | 78 +++++++++++++------------- 1 file changed, 40 insertions(+), 38 deletions(-) diff --git a/src/cmd/compile/internal/gc/compile.go b/src/cmd/compile/internal/gc/compile.go index b9c10056b4..c2894ab012 100644 --- a/src/cmd/compile/internal/gc/compile.go +++ b/src/cmd/compile/internal/gc/compile.go @@ -93,43 +93,45 @@ func prepareFunc(fn *ir.Func) { // It fans out nBackendWorkers to do the work // and waits for them to complete. func compileFunctions() { - if len(compilequeue) != 0 { - types.CalcSizeDisabled = true // not safe to calculate sizes concurrently - if race.Enabled { - // Randomize compilation order to try to shake out races. - tmp := make([]*ir.Func, len(compilequeue)) - perm := rand.Perm(len(compilequeue)) - for i, v := range perm { - tmp[v] = compilequeue[i] - } - copy(compilequeue, tmp) - } else { - // Compile the longest functions first, - // since they're most likely to be the slowest. - // This helps avoid stragglers. - sort.Slice(compilequeue, func(i, j int) bool { - return len(compilequeue[i].Body) > len(compilequeue[j].Body) - }) - } - var wg sync.WaitGroup - base.Ctxt.InParallel = true - c := make(chan *ir.Func, base.Flag.LowerC) - for i := 0; i < base.Flag.LowerC; i++ { - wg.Add(1) - go func(worker int) { - for fn := range c { - ssagen.Compile(fn, worker) - } - wg.Done() - }(i) - } - for _, fn := range compilequeue { - c <- fn - } - close(c) - compilequeue = nil - wg.Wait() - base.Ctxt.InParallel = false - types.CalcSizeDisabled = false + if len(compilequeue) == 0 { + return } + + types.CalcSizeDisabled = true // not safe to calculate sizes concurrently + if race.Enabled { + // Randomize compilation order to try to shake out races. + tmp := make([]*ir.Func, len(compilequeue)) + perm := rand.Perm(len(compilequeue)) + for i, v := range perm { + tmp[v] = compilequeue[i] + } + copy(compilequeue, tmp) + } else { + // Compile the longest functions first, + // since they're most likely to be the slowest. + // This helps avoid stragglers. + sort.Slice(compilequeue, func(i, j int) bool { + return len(compilequeue[i].Body) > len(compilequeue[j].Body) + }) + } + var wg sync.WaitGroup + base.Ctxt.InParallel = true + c := make(chan *ir.Func, base.Flag.LowerC) + for i := 0; i < base.Flag.LowerC; i++ { + wg.Add(1) + go func(worker int) { + for fn := range c { + ssagen.Compile(fn, worker) + } + wg.Done() + }(i) + } + for _, fn := range compilequeue { + c <- fn + } + close(c) + compilequeue = nil + wg.Wait() + base.Ctxt.InParallel = false + types.CalcSizeDisabled = false } From d6ad88b4db454813e1bdf09635cd853fe3b7ef13 Mon Sep 17 00:00:00 2001 From: Matthew Dempsky Date: Tue, 12 Jan 2021 12:00:58 -0800 Subject: [PATCH 14/16] [dev.regabi] cmd/compile: compile functions before closures This CL reorders function compilation to ensure that functions are always compiled before any enclosed function literals. The primary goal of this is to reduce the risk of race conditions that arise due to compilation of function literals needing to inspect data from their closure variables. However, a pleasant side effect is that it allows skipping the redundant, separate compilation of function literals that were inlined into their enclosing function. Change-Id: I03ee96212988cb578c2452162b7e99cc5e92918f Reviewed-on: https://go-review.googlesource.com/c/go/+/282892 Run-TryBot: Matthew Dempsky TryBot-Result: Go Bot Reviewed-by: Keith Randall Trust: Matthew Dempsky --- src/cmd/compile/internal/gc/compile.go | 51 +++++++++++++++++----- src/cmd/compile/internal/ir/func.go | 4 ++ src/cmd/compile/internal/ir/sizeof_test.go | 2 +- src/cmd/compile/internal/walk/closure.go | 2 + src/cmd/compile/internal/walk/expr.go | 7 ++- 5 files changed, 52 insertions(+), 14 deletions(-) diff --git a/src/cmd/compile/internal/gc/compile.go b/src/cmd/compile/internal/gc/compile.go index c2894ab012..410b3e90ea 100644 --- a/src/cmd/compile/internal/gc/compile.go +++ b/src/cmd/compile/internal/gc/compile.go @@ -37,6 +37,10 @@ func enqueueFunc(fn *ir.Func) { return } + if clo := fn.OClosure; clo != nil && !ir.IsTrivialClosure(clo) { + return // we'll get this as part of its enclosing function + } + if len(fn.Body) == 0 { // Initialize ABI wrappers if necessary. ssagen.InitLSym(fn, false) @@ -45,11 +49,22 @@ func enqueueFunc(fn *ir.Func) { } errorsBefore := base.Errors() - prepareFunc(fn) + + todo := []*ir.Func{fn} + for len(todo) > 0 { + next := todo[len(todo)-1] + todo = todo[:len(todo)-1] + + prepareFunc(next) + todo = append(todo, next.Closures...) + } + if base.Errors() > errorsBefore { return } + // Enqueue just fn itself. compileFunctions will handle + // scheduling compilation of its closures after it's done. compilequeue = append(compilequeue, fn) } @@ -97,7 +112,6 @@ func compileFunctions() { return } - types.CalcSizeDisabled = true // not safe to calculate sizes concurrently if race.Enabled { // Randomize compilation order to try to shake out races. tmp := make([]*ir.Func, len(compilequeue)) @@ -114,22 +128,37 @@ func compileFunctions() { return len(compilequeue[i].Body) > len(compilequeue[j].Body) }) } - var wg sync.WaitGroup - base.Ctxt.InParallel = true - c := make(chan *ir.Func, base.Flag.LowerC) + + // We queue up a goroutine per function that needs to be + // compiled, but require them to grab an available worker ID + // before doing any substantial work to limit parallelism. + workerIDs := make(chan int, base.Flag.LowerC) for i := 0; i < base.Flag.LowerC; i++ { + workerIDs <- i + } + + var wg sync.WaitGroup + var asyncCompile func(*ir.Func) + asyncCompile = func(fn *ir.Func) { wg.Add(1) - go func(worker int) { - for fn := range c { - ssagen.Compile(fn, worker) + go func() { + worker := <-workerIDs + ssagen.Compile(fn, worker) + workerIDs <- worker + + // Done compiling fn. Schedule it's closures for compilation. + for _, closure := range fn.Closures { + asyncCompile(closure) } wg.Done() - }(i) + }() } + + types.CalcSizeDisabled = true // not safe to calculate sizes concurrently + base.Ctxt.InParallel = true for _, fn := range compilequeue { - c <- fn + asyncCompile(fn) } - close(c) compilequeue = nil wg.Wait() base.Ctxt.InParallel = false diff --git a/src/cmd/compile/internal/ir/func.go b/src/cmd/compile/internal/ir/func.go index d660fe3b40..3fe23635f4 100644 --- a/src/cmd/compile/internal/ir/func.go +++ b/src/cmd/compile/internal/ir/func.go @@ -81,6 +81,10 @@ type Func struct { // Byval set if they're captured by value. ClosureVars []*Name + // Enclosed functions that need to be compiled. + // Populated during walk. + Closures []*Func + // Parents records the parent scope of each scope within a // function. The root scope (0) has no parent, so the i'th // scope's parent is stored at Parents[i-1]. diff --git a/src/cmd/compile/internal/ir/sizeof_test.go b/src/cmd/compile/internal/ir/sizeof_test.go index 2ada7231aa..f95f77d6a2 100644 --- a/src/cmd/compile/internal/ir/sizeof_test.go +++ b/src/cmd/compile/internal/ir/sizeof_test.go @@ -20,7 +20,7 @@ func TestSizeof(t *testing.T) { _32bit uintptr // size on 32bit platforms _64bit uintptr // size on 64bit platforms }{ - {Func{}, 184, 320}, + {Func{}, 196, 344}, {Name{}, 116, 208}, } diff --git a/src/cmd/compile/internal/walk/closure.go b/src/cmd/compile/internal/walk/closure.go index acb74b9901..7fa63ea9c7 100644 --- a/src/cmd/compile/internal/walk/closure.go +++ b/src/cmd/compile/internal/walk/closure.go @@ -86,6 +86,8 @@ func walkClosure(clo *ir.ClosureExpr, init *ir.Nodes) ir.Node { } return fn.Nname } + + ir.CurFunc.Closures = append(ir.CurFunc.Closures, fn) ir.ClosureDebugRuntimeCheck(clo) typ := typecheck.ClosureType(clo) diff --git a/src/cmd/compile/internal/walk/expr.go b/src/cmd/compile/internal/walk/expr.go index df575d6985..508cdd1d06 100644 --- a/src/cmd/compile/internal/walk/expr.go +++ b/src/cmd/compile/internal/walk/expr.go @@ -488,12 +488,15 @@ func walkCall(n *ir.CallExpr, init *ir.Nodes) ir.Node { reflectdata.MarkUsedIfaceMethod(n) } - if n.Op() == ir.OCALLFUNC && n.X.Op() == ir.OCLOSURE { + if n.Op() == ir.OCALLFUNC && n.X.Op() == ir.OCLOSURE && !ir.IsTrivialClosure(n.X.(*ir.ClosureExpr)) { // Transform direct call of a closure to call of a normal function. // transformclosure already did all preparation work. + // We leave trivial closures for walkClosure to handle. + + clo := n.X.(*ir.ClosureExpr) + ir.CurFunc.Closures = append(ir.CurFunc.Closures, clo.Func) // Prepend captured variables to argument list. - clo := n.X.(*ir.ClosureExpr) n.Args.Prepend(closureArgs(clo)...) // Replace OCLOSURE with ONAME/PFUNC. From 41352fd401f4f22eceeca375361e018ea787f0fd Mon Sep 17 00:00:00 2001 From: Matthew Dempsky Date: Tue, 12 Jan 2021 12:12:27 -0800 Subject: [PATCH 15/16] [dev.regabi] cmd/compile: transform closures during walk We used to transform directly called closures in a separate pass before walk, because we couldn't guarantee whether we'd see the closure call or the closure itself first. As of the last CL, this ordering is always guaranteed, so we can rewrite calls and the closure at the same time. Change-Id: Ia6f4d504c24795e41500108589b53395d301123b Reviewed-on: https://go-review.googlesource.com/c/go/+/283315 Run-TryBot: Matthew Dempsky TryBot-Result: Go Bot Trust: Matthew Dempsky Reviewed-by: Keith Randall --- src/cmd/compile/internal/gc/main.go | 15 ---- src/cmd/compile/internal/walk/closure.go | 103 ++++++++++++++--------- src/cmd/compile/internal/walk/expr.go | 23 +---- 3 files changed, 63 insertions(+), 78 deletions(-) diff --git a/src/cmd/compile/internal/gc/main.go b/src/cmd/compile/internal/gc/main.go index 2903d64ff8..9ecdd510b1 100644 --- a/src/cmd/compile/internal/gc/main.go +++ b/src/cmd/compile/internal/gc/main.go @@ -22,7 +22,6 @@ import ( "cmd/compile/internal/ssagen" "cmd/compile/internal/typecheck" "cmd/compile/internal/types" - "cmd/compile/internal/walk" "cmd/internal/dwarf" "cmd/internal/obj" "cmd/internal/objabi" @@ -269,20 +268,6 @@ func Main(archInit func(*ssagen.ArchInfo)) { ssagen.EnableNoWriteBarrierRecCheck() } - // Transform closure bodies to properly reference captured variables. - // This needs to happen before walk, because closures must be transformed - // before walk reaches a call of a closure. - base.Timer.Start("fe", "xclosures") - for _, n := range typecheck.Target.Decls { - if n.Op() == ir.ODCLFUNC { - n := n.(*ir.Func) - if n.OClosure != nil { - ir.CurFunc = n - walk.Closure(n) - } - } - } - // Prepare for SSA compilation. // This must be before peekitabs, because peekitabs // can trigger function compilation. diff --git a/src/cmd/compile/internal/walk/closure.go b/src/cmd/compile/internal/walk/closure.go index 7fa63ea9c7..e9b3698080 100644 --- a/src/cmd/compile/internal/walk/closure.go +++ b/src/cmd/compile/internal/walk/closure.go @@ -12,50 +12,43 @@ import ( "cmd/internal/src" ) -// Closure is called in a separate phase after escape analysis. -// It transform closure bodies to properly reference captured variables. -func Closure(fn *ir.Func) { - if len(fn.ClosureVars) == 0 { - return +// directClosureCall rewrites a direct call of a function literal into +// a normal function call with closure variables passed as arguments. +// This avoids allocation of a closure object. +// +// For illustration, the following call: +// +// func(a int) { +// println(byval) +// byref++ +// }(42) +// +// becomes: +// +// func(byval int, &byref *int, a int) { +// println(byval) +// (*&byref)++ +// }(byval, &byref, 42) +func directClosureCall(n *ir.CallExpr) { + clo := n.X.(*ir.ClosureExpr) + clofn := clo.Func + + if ir.IsTrivialClosure(clo) { + return // leave for walkClosure to handle } - if !fn.ClosureCalled() { - // The closure is not directly called, so it is going to stay as closure. - fn.SetNeedctxt(true) - return - } - - lno := base.Pos - base.Pos = fn.Pos() - - // If the closure is directly called, we transform it to a plain function call - // with variables passed as args. This avoids allocation of a closure object. - // Here we do only a part of the transformation. Walk of OCALLFUNC(OCLOSURE) - // will complete the transformation later. - // For illustration, the following closure: - // func(a int) { - // println(byval) - // byref++ - // }(42) - // becomes: - // func(byval int, &byref *int, a int) { - // println(byval) - // (*&byref)++ - // }(byval, &byref, 42) - - // f is ONAME of the actual function. - f := fn.Nname - // We are going to insert captured variables before input args. var params []*types.Field var decls []*ir.Name - for _, v := range fn.ClosureVars { + for _, v := range clofn.ClosureVars { if !v.Byval() { // If v of type T is captured by reference, // we introduce function param &v *T // and v remains PAUTOHEAP with &v heapaddr // (accesses will implicitly deref &v). - addr := typecheck.NewName(typecheck.Lookup("&" + v.Sym().Name)) + + addr := ir.NewNameAt(clofn.Pos(), typecheck.Lookup("&"+v.Sym().Name)) + addr.Curfn = clofn addr.SetType(types.NewPtr(v.Type())) v.Heapaddr = addr v = addr @@ -69,32 +62,58 @@ func Closure(fn *ir.Func) { params = append(params, fld) } - // Prepend params and decls. - f.Type().Params().SetFields(append(params, f.Type().Params().FieldSlice()...)) - fn.Dcl = append(decls, fn.Dcl...) + // f is ONAME of the actual function. + f := clofn.Nname - base.Pos = lno + // Prepend params and decls. + typ := f.Type() + typ.Params().SetFields(append(params, typ.Params().FieldSlice()...)) + clofn.Dcl = append(decls, clofn.Dcl...) + + // Rewrite call. + n.X = f + n.Args.Prepend(closureArgs(clo)...) + + // Update the call expression's type. We need to do this + // because typecheck gave it the result type of the OCLOSURE + // node, but we only rewrote the ONAME node's type. Logically, + // they're the same, but the stack offsets probably changed. + // + // TODO(mdempsky): Reuse a single type for both. + if typ.NumResults() == 1 { + n.SetType(typ.Results().Field(0).Type) + } else { + n.SetType(typ.Results()) + } + + // Add to Closures for enqueueFunc. It's no longer a proper + // closure, but we may have already skipped over it in the + // functions list as a non-trivial closure, so this just + // ensures it's compiled. + ir.CurFunc.Closures = append(ir.CurFunc.Closures, clofn) } func walkClosure(clo *ir.ClosureExpr, init *ir.Nodes) ir.Node { - fn := clo.Func + clofn := clo.Func // If no closure vars, don't bother wrapping. if ir.IsTrivialClosure(clo) { if base.Debug.Closure > 0 { base.WarnfAt(clo.Pos(), "closure converted to global") } - return fn.Nname + return clofn.Nname } - ir.CurFunc.Closures = append(ir.CurFunc.Closures, fn) + // The closure is not trivial or directly called, so it's going to stay a closure. ir.ClosureDebugRuntimeCheck(clo) + clofn.SetNeedctxt(true) + ir.CurFunc.Closures = append(ir.CurFunc.Closures, clofn) typ := typecheck.ClosureType(clo) clos := ir.NewCompLitExpr(base.Pos, ir.OCOMPLIT, ir.TypeNode(typ), nil) clos.SetEsc(clo.Esc()) - clos.List = append([]ir.Node{ir.NewUnaryExpr(base.Pos, ir.OCFUNC, fn.Nname)}, closureArgs(clo)...) + clos.List = append([]ir.Node{ir.NewUnaryExpr(base.Pos, ir.OCFUNC, clofn.Nname)}, closureArgs(clo)...) addr := typecheck.NodAddr(clos) addr.SetEsc(clo.Esc()) diff --git a/src/cmd/compile/internal/walk/expr.go b/src/cmd/compile/internal/walk/expr.go index 508cdd1d06..893a95f403 100644 --- a/src/cmd/compile/internal/walk/expr.go +++ b/src/cmd/compile/internal/walk/expr.go @@ -488,27 +488,8 @@ func walkCall(n *ir.CallExpr, init *ir.Nodes) ir.Node { reflectdata.MarkUsedIfaceMethod(n) } - if n.Op() == ir.OCALLFUNC && n.X.Op() == ir.OCLOSURE && !ir.IsTrivialClosure(n.X.(*ir.ClosureExpr)) { - // Transform direct call of a closure to call of a normal function. - // transformclosure already did all preparation work. - // We leave trivial closures for walkClosure to handle. - - clo := n.X.(*ir.ClosureExpr) - ir.CurFunc.Closures = append(ir.CurFunc.Closures, clo.Func) - - // Prepend captured variables to argument list. - n.Args.Prepend(closureArgs(clo)...) - - // Replace OCLOSURE with ONAME/PFUNC. - n.X = clo.Func.Nname - - // Update type of OCALLFUNC node. - // Output arguments had not changed, but their offsets could. - if n.X.Type().NumResults() == 1 { - n.SetType(n.X.Type().Results().Field(0).Type) - } else { - n.SetType(n.X.Type().Results()) - } + if n.Op() == ir.OCALLFUNC && n.X.Op() == ir.OCLOSURE { + directClosureCall(n) } walkCall1(n, init) From d9acf6f3a3758c3096ee5ef5a24c2bc5df9d9c8b Mon Sep 17 00:00:00 2001 From: Matthew Dempsky Date: Tue, 12 Jan 2021 12:25:33 -0800 Subject: [PATCH 16/16] [dev.regabi] cmd/compile: remove Func.ClosureType The closure's type always matches the corresponding function's type, so just use one instance rather than carrying around two. Simplifies construction of closures, rewriting them during walk, and shrinks memory usage. Passes toolstash -cmp. Change-Id: I83b8b8f435b02ab25a30fb7aa15d5ec7ad97189d Reviewed-on: https://go-review.googlesource.com/c/go/+/283152 Run-TryBot: Matthew Dempsky TryBot-Result: Go Bot Trust: Matthew Dempsky Reviewed-by: Keith Randall --- src/cmd/compile/internal/ir/func.go | 2 -- src/cmd/compile/internal/ir/sizeof_test.go | 2 +- src/cmd/compile/internal/noder/noder.go | 2 -- src/cmd/compile/internal/typecheck/func.go | 4 ++-- src/cmd/compile/internal/walk/closure.go | 10 +++++----- 5 files changed, 8 insertions(+), 12 deletions(-) diff --git a/src/cmd/compile/internal/ir/func.go b/src/cmd/compile/internal/ir/func.go index 3fe23635f4..30cddd298e 100644 --- a/src/cmd/compile/internal/ir/func.go +++ b/src/cmd/compile/internal/ir/func.go @@ -71,8 +71,6 @@ type Func struct { // Anonymous and blank PPARAMOUTs are declared as ~rNN and ~bNN Names, respectively. Dcl []*Name - ClosureType Ntype // closure representation type - // ClosureVars lists the free variables that are used within a // function literal, but formally declared in an enclosing // function. The variables in this slice are the closure function's diff --git a/src/cmd/compile/internal/ir/sizeof_test.go b/src/cmd/compile/internal/ir/sizeof_test.go index f95f77d6a2..553dc53760 100644 --- a/src/cmd/compile/internal/ir/sizeof_test.go +++ b/src/cmd/compile/internal/ir/sizeof_test.go @@ -20,7 +20,7 @@ func TestSizeof(t *testing.T) { _32bit uintptr // size on 32bit platforms _64bit uintptr // size on 64bit platforms }{ - {Func{}, 196, 344}, + {Func{}, 188, 328}, {Name{}, 116, 208}, } diff --git a/src/cmd/compile/internal/noder/noder.go b/src/cmd/compile/internal/noder/noder.go index ec0debdbbd..edd30a1fc1 100644 --- a/src/cmd/compile/internal/noder/noder.go +++ b/src/cmd/compile/internal/noder/noder.go @@ -1856,7 +1856,6 @@ func fakeRecv() *ir.Field { func (p *noder) funcLit(expr *syntax.FuncLit) ir.Node { xtype := p.typeExpr(expr.Type) - ntype := p.typeExpr(expr.Type) fn := ir.NewFunc(p.pos(expr)) fn.SetIsHiddenClosure(ir.CurFunc != nil) @@ -1867,7 +1866,6 @@ func (p *noder) funcLit(expr *syntax.FuncLit) ir.Node { fn.Nname.Defn = fn clo := ir.NewClosureExpr(p.pos(expr), fn) - fn.ClosureType = ntype fn.OClosure = clo p.funcBody(fn, expr.Body) diff --git a/src/cmd/compile/internal/typecheck/func.go b/src/cmd/compile/internal/typecheck/func.go index 8f7411daec..03a10f594a 100644 --- a/src/cmd/compile/internal/typecheck/func.go +++ b/src/cmd/compile/internal/typecheck/func.go @@ -293,20 +293,20 @@ func tcClosure(clo *ir.ClosureExpr, top int) { fn.Iota = x } - fn.ClosureType = typecheckNtype(fn.ClosureType) - clo.SetType(fn.ClosureType.Type()) fn.SetClosureCalled(top&ctxCallee != 0) // Do not typecheck fn twice, otherwise, we will end up pushing // fn to Target.Decls multiple times, causing initLSym called twice. // See #30709 if fn.Typecheck() == 1 { + clo.SetType(fn.Type()) return } fn.Nname.SetSym(closurename(ir.CurFunc)) ir.MarkFunc(fn.Nname) Func(fn) + clo.SetType(fn.Type()) // Type check the body now, but only if we're inside a function. // At top level (in a variable initialization: curfn==nil) we're not diff --git a/src/cmd/compile/internal/walk/closure.go b/src/cmd/compile/internal/walk/closure.go index e9b3698080..694aa99940 100644 --- a/src/cmd/compile/internal/walk/closure.go +++ b/src/cmd/compile/internal/walk/closure.go @@ -64,10 +64,12 @@ func directClosureCall(n *ir.CallExpr) { // f is ONAME of the actual function. f := clofn.Nname - - // Prepend params and decls. typ := f.Type() - typ.Params().SetFields(append(params, typ.Params().FieldSlice()...)) + + // Create new function type with parameters prepended, and + // then update type and declarations. + typ = types.NewSignature(typ.Pkg(), nil, append(params, typ.Params().FieldSlice()...), typ.Results().FieldSlice()) + f.SetType(typ) clofn.Dcl = append(decls, clofn.Dcl...) // Rewrite call. @@ -78,8 +80,6 @@ func directClosureCall(n *ir.CallExpr) { // because typecheck gave it the result type of the OCLOSURE // node, but we only rewrote the ONAME node's type. Logically, // they're the same, but the stack offsets probably changed. - // - // TODO(mdempsky): Reuse a single type for both. if typ.NumResults() == 1 { n.SetType(typ.Results().Field(0).Type) } else {