From 6f02578f9cff92e6c0fae4d86df01dcf99673c61 Mon Sep 17 00:00:00 2001 From: David Chase Date: Fri, 25 Sep 2020 13:30:51 -0400 Subject: cmd/compile: fix logopt log directory naming for windows Allow Windows absolute paths, also fixed URI decoding on Windows. Added a test, reorganized to make the test cleaner. Also put some doc comments on exported functions that did not have them. Fixes #41614. Change-Id: I2871be0e5183fbd53ffb309896d6fe56c15a7727 Reviewed-on: https://go-review.googlesource.com/c/go/+/257677 Trust: David Chase Run-TryBot: David Chase TryBot-Result: Go Bot Reviewed-by: Rebecca Stambler --- src/cmd/compile/internal/logopt/log_opts.go | 89 ++++++++++++++++++-------- src/cmd/compile/internal/logopt/logopt_test.go | 28 ++++++++ 2 files changed, 89 insertions(+), 28 deletions(-) (limited to 'src/cmd/compile/internal/logopt') diff --git a/src/cmd/compile/internal/logopt/log_opts.go b/src/cmd/compile/internal/logopt/log_opts.go index 22a94b0f2d..37a049d640 100644 --- a/src/cmd/compile/internal/logopt/log_opts.go +++ b/src/cmd/compile/internal/logopt/log_opts.go @@ -19,6 +19,7 @@ import ( "strconv" "strings" "sync" + "unicode" ) // This implements (non)optimization logging for -json option to the Go compiler @@ -223,11 +224,11 @@ type Diagnostic struct { // A LoggedOpt is what the compiler produces and accumulates, // to be converted to JSON for human or IDE consumption. type LoggedOpt struct { - pos src.XPos // Source code position at which the event occurred. If it is inlined, outer and all inlined locations will appear in JSON. - pass string // For human/adhoc consumption; does not appear in JSON (yet) - fname string // For human/adhoc consumption; does not appear in JSON (yet) - what string // The (non) optimization; "nilcheck", "boundsCheck", "inline", "noInline" - target []interface{} // Optional target(s) or parameter(s) of "what" -- what was inlined, why it was not, size of copy, etc. 1st is most important/relevant. + pos src.XPos // Source code position at which the event occurred. If it is inlined, outer and all inlined locations will appear in JSON. + compilerPass string // Compiler pass. For human/adhoc consumption; does not appear in JSON (yet) + functionName string // Function name. For human/adhoc consumption; does not appear in JSON (yet) + what string // The (non) optimization; "nilcheck", "boundsCheck", "inline", "noInline" + target []interface{} // Optional target(s) or parameter(s) of "what" -- what was inlined, why it was not, size of copy, etc. 1st is most important/relevant. } type logFormat uint8 @@ -240,12 +241,13 @@ const ( var Format = None var dest string +// LogJsonOption parses and validates the version,directory value attached to the -json compiler flag. func LogJsonOption(flagValue string) { version, directory := parseLogFlag("json", flagValue) if version != 0 { log.Fatal("-json version must be 0") } - checkLogPath("json", directory) + dest = checkLogPath(directory) Format = Json0 } @@ -268,51 +270,80 @@ func parseLogFlag(flag, value string) (version int, directory string) { return } -// checkLogPath does superficial early checking of the string specifying -// the directory to which optimizer logging is directed, and if -// it passes the test, stores the string in LO_dir -func checkLogPath(flag, destination string) { - sep := string(os.PathSeparator) - if strings.HasPrefix(destination, "/") || strings.HasPrefix(destination, sep) { - err := os.MkdirAll(destination, 0755) - if err != nil { - log.Fatalf("optimizer logging destination ',' but could not create : err=%v", err) - } - } else if strings.HasPrefix(destination, "file://") { // IKWIAD, or Windows C:\foo\bar\baz +// isWindowsDriveURI returns true if the file URI is of the format used by +// Windows URIs. The url.Parse package does not specially handle Windows paths +// (see golang/go#6027), so we check if the URI path has a drive prefix (e.g. "/C:"). +// (copied from tools/internal/span/uri.go) +// this is less comprehensive that the processing in filepath.IsAbs on Windows. +func isWindowsDriveURIPath(uri string) bool { + if len(uri) < 4 { + return false + } + return uri[0] == '/' && unicode.IsLetter(rune(uri[1])) && uri[2] == ':' +} + +func parseLogPath(destination string) (string, string) { + if filepath.IsAbs(destination) { + return filepath.Clean(destination), "" + } + if strings.HasPrefix(destination, "file://") { // IKWIAD, or Windows C:\foo\bar\baz uri, err := url.Parse(destination) if err != nil { - log.Fatalf("optimizer logging destination looked like file:// URI but failed to parse: err=%v", err) + return "", fmt.Sprintf("optimizer logging destination looked like file:// URI but failed to parse: err=%v", err) } destination = uri.Host + uri.Path - err = os.MkdirAll(destination, 0755) - if err != nil { - log.Fatalf("optimizer logging destination ',' but could not create %s: err=%v", destination, err) + if isWindowsDriveURIPath(destination) { + // strip leading / from /C: + // unlike tools/internal/span/uri.go, do not uppercase the drive letter -- let filepath.Clean do what it does. + destination = destination[1:] } - } else { - log.Fatalf("optimizer logging destination %s was neither %s-prefixed directory nor file://-prefixed file URI", destination, sep) + return filepath.Clean(destination), "" + } + return "", fmt.Sprintf("optimizer logging destination %s was neither %s-prefixed directory nor file://-prefixed file URI", destination, string(filepath.Separator)) +} + +// checkLogPath does superficial early checking of the string specifying +// the directory to which optimizer logging is directed, and if +// it passes the test, stores the string in LO_dir +func checkLogPath(destination string) string { + path, complaint := parseLogPath(destination) + if complaint != "" { + log.Fatalf(complaint) + } + err := os.MkdirAll(path, 0755) + if err != nil { + log.Fatalf("optimizer logging destination ',' but could not create : err=%v", err) } - dest = destination + return path } var loggedOpts []*LoggedOpt var mu = sync.Mutex{} // mu protects loggedOpts. -func NewLoggedOpt(pos src.XPos, what, pass, fname string, args ...interface{}) *LoggedOpt { +// NewLoggedOpt allocates a new LoggedOpt, to later be passed to either NewLoggedOpt or LogOpt as "args". +// Pos is the source position (including inlining), what is the message, pass is which pass created the message, +// funcName is the name of the function +// A typical use for this to accumulate an explanation for a missed optimization, for example, why did something escape? +func NewLoggedOpt(pos src.XPos, what, pass, funcName string, args ...interface{}) *LoggedOpt { pass = strings.Replace(pass, " ", "_", -1) - return &LoggedOpt{pos, pass, fname, what, args} + return &LoggedOpt{pos, pass, funcName, what, args} } -func LogOpt(pos src.XPos, what, pass, fname string, args ...interface{}) { +// Logopt logs information about a (usually missed) optimization performed by the compiler. +// Pos is the source position (including inlining), what is the message, pass is which pass created the message, +// funcName is the name of the function +func LogOpt(pos src.XPos, what, pass, funcName string, args ...interface{}) { if Format == None { return } - lo := NewLoggedOpt(pos, what, pass, fname, args...) + lo := NewLoggedOpt(pos, what, pass, funcName, args...) mu.Lock() defer mu.Unlock() // Because of concurrent calls from back end, no telling what the order will be, but is stable-sorted by outer Pos before use. loggedOpts = append(loggedOpts, lo) } +// Enabled returns whether optimization logging is enabled. func Enabled() bool { switch Format { case None: @@ -459,11 +490,13 @@ func FlushLoggedOpts(ctxt *obj.Link, slashPkgPath string) { } } +// newPointRange returns a single-position Range for the compiler source location p. func newPointRange(p src.Pos) Range { return Range{Start: Position{p.Line(), p.Col()}, End: Position{p.Line(), p.Col()}} } +// newLocation returns the Location for the compiler source location p func newLocation(p src.Pos) Location { loc := Location{URI: uriIfy(uprootedPath(p.Filename())), Range: newPointRange(p)} return loc diff --git a/src/cmd/compile/internal/logopt/logopt_test.go b/src/cmd/compile/internal/logopt/logopt_test.go index df3e70a614..b57a07f12c 100644 --- a/src/cmd/compile/internal/logopt/logopt_test.go +++ b/src/cmd/compile/internal/logopt/logopt_test.go @@ -55,6 +55,34 @@ func wantN(t *testing.T, out string, desired string, n int) { } } +func TestPathStuff(t *testing.T) { + sep := string(filepath.Separator) + if path, whine := parseLogPath("file:///c:foo"); path != "c:foo" || whine != "" { // good path + t.Errorf("path='%s', whine='%s'", path, whine) + } + if path, whine := parseLogPath("file:///foo"); path != sep+"foo" || whine != "" { // good path + t.Errorf("path='%s', whine='%s'", path, whine) + } + if path, whine := parseLogPath("foo"); path != "" || whine == "" { // BAD path + t.Errorf("path='%s', whine='%s'", path, whine) + } + if sep == "\\" { // On WINDOWS ONLY + if path, whine := parseLogPath("C:/foo"); path != "C:\\foo" || whine != "" { // good path + t.Errorf("path='%s', whine='%s'", path, whine) + } + if path, whine := parseLogPath("c:foo"); path != "" || whine == "" { // BAD path + t.Errorf("path='%s', whine='%s'", path, whine) + } + if path, whine := parseLogPath("/foo"); path != "" || whine == "" { // BAD path + t.Errorf("path='%s', whine='%s'", path, whine) + } + } else { // ON UNIX ONLY + if path, whine := parseLogPath("/foo"); path != sep+"foo" || whine != "" { // good path + t.Errorf("path='%s', whine='%s'", path, whine) + } + } +} + func TestLogOpt(t *testing.T) { t.Parallel() -- cgit v1.3 From cced777026e1fc094ed21d99ae1efa4cf19146d2 Mon Sep 17 00:00:00 2001 From: Matthew Dempsky Date: Mon, 21 Sep 2020 21:24:00 -0700 Subject: cmd/compile: set n.Name.Defn for inlined parameters Normally, when variables are declared and initialized using ":=", we set the variable's n.Name.Defn to point to the initialization assignment node (i.e., OAS or OAS2). Further, some frontend optimizations look for variables that are initialized but never reassigned. However, when inl.go inlines calls, it was declaring the inlined variables, and then separately assigning to them. This CL changes inl.go tweaks the AST to fit the combined declaration+initialization pattern. This isn't terribly useful by itself, but it allows further followup optimizations. Updates #41474. Change-Id: I62a9752c60414305679e0ed15a6563baa0224efa Reviewed-on: https://go-review.googlesource.com/c/go/+/256457 Run-TryBot: Matthew Dempsky TryBot-Result: Go Bot Trust: Matthew Dempsky Reviewed-by: Cuong Manh Le --- src/cmd/compile/internal/gc/inl.go | 74 +++++++++++--------------- src/cmd/compile/internal/logopt/logopt_test.go | 2 +- 2 files changed, 33 insertions(+), 43 deletions(-) (limited to 'src/cmd/compile/internal/logopt') diff --git a/src/cmd/compile/internal/gc/inl.go b/src/cmd/compile/internal/gc/inl.go index fa5b3ec698..5740864b12 100644 --- a/src/cmd/compile/internal/gc/inl.go +++ b/src/cmd/compile/internal/gc/inl.go @@ -831,16 +831,19 @@ func (v *reassignVisitor) visitList(l Nodes) *Node { return nil } -func tinlvar(t *types.Field, inlvars map[*Node]*Node) *Node { - if n := asNode(t.Nname); n != nil && !n.isBlank() { - inlvar := inlvars[n] - if inlvar == nil { - Fatalf("missing inlvar for %v\n", n) - } - return inlvar +func inlParam(t *types.Field, as *Node, inlvars map[*Node]*Node) *Node { + n := asNode(t.Nname) + if n == nil || n.isBlank() { + return nblank } - return typecheck(nblank, ctxExpr|ctxAssign) + inlvar := inlvars[n] + if inlvar == nil { + Fatalf("missing inlvar for %v", n) + } + as.Ninit.Append(nod(ODCL, inlvar, nil)) + inlvar.Name.Defn = as + return inlvar } var inlgen int @@ -970,14 +973,15 @@ func mkinlcall(n, fn *Node, maxCost int32, inlMap map[*Node]bool) *Node { continue } if ln.isParamStackCopy() { // ignore the on-stack copy of a parameter that moved to the heap - continue - } - inlvars[ln] = typecheck(inlvar(ln), ctxExpr) - if ln.Class() == PPARAM || ln.Name.Param.Stackcopy != nil && ln.Name.Param.Stackcopy.Class() == PPARAM { - ninit.Append(nod(ODCL, inlvars[ln], nil)) + // TODO(mdempsky): Remove once I'm confident + // this never actually happens. We currently + // perform inlining before escape analysis, so + // nothing should have moved to the heap yet. + Fatalf("impossible: %v", ln) } + inlf := typecheck(inlvar(ln), ctxExpr) + inlvars[ln] = inlf if genDwarfInline > 0 { - inlf := inlvars[ln] if ln.Class() == PPARAM { inlf.Name.SetInlFormal(true) } else { @@ -1019,56 +1023,42 @@ func mkinlcall(n, fn *Node, maxCost int32, inlMap map[*Node]bool) *Node { // Assign arguments to the parameters' temp names. as := nod(OAS2, nil, nil) - as.Rlist.Set(n.List.Slice()) + as.SetColas(true) + if n.Op == OCALLMETH { + if n.Left.Left == nil { + Fatalf("method call without receiver: %+v", n) + } + as.Rlist.Append(n.Left.Left) + } + as.Rlist.Append(n.List.Slice()...) // For non-dotted calls to variadic functions, we assign the // variadic parameter's temp name separately. var vas *Node - if fn.IsMethod() { - rcv := fn.Type.Recv() - - if n.Left.Op == ODOTMETH { - // For x.M(...), assign x directly to the - // receiver parameter. - if n.Left.Left == nil { - Fatalf("method call without receiver: %+v", n) - } - ras := nod(OAS, tinlvar(rcv, inlvars), n.Left.Left) - ras = typecheck(ras, ctxStmt) - ninit.Append(ras) - } else { - // For T.M(...), add the receiver parameter to - // as.List, so it's assigned by the normal - // arguments. - if as.Rlist.Len() == 0 { - Fatalf("non-method call to method without first arg: %+v", n) - } - as.List.Append(tinlvar(rcv, inlvars)) - } + if recv := fn.Type.Recv(); recv != nil { + as.List.Append(inlParam(recv, as, inlvars)) } - for _, param := range fn.Type.Params().Fields().Slice() { // For ordinary parameters or variadic parameters in // dotted calls, just add the variable to the // assignment list, and we're done. if !param.IsDDD() || n.IsDDD() { - as.List.Append(tinlvar(param, inlvars)) + as.List.Append(inlParam(param, as, inlvars)) continue } // Otherwise, we need to collect the remaining values // to pass as a slice. - numvals := n.List.Len() - x := as.List.Len() - for as.List.Len() < numvals { + for as.List.Len() < as.Rlist.Len() { as.List.Append(argvar(param.Type, as.List.Len())) } varargs := as.List.Slice()[x:] - vas = nod(OAS, tinlvar(param, inlvars), nil) + vas = nod(OAS, nil, nil) + vas.Left = inlParam(param, vas, inlvars) if len(varargs) == 0 { vas.Right = nodnil() vas.Right.Type = param.Type diff --git a/src/cmd/compile/internal/logopt/logopt_test.go b/src/cmd/compile/internal/logopt/logopt_test.go index b57a07f12c..fb71e142e3 100644 --- a/src/cmd/compile/internal/logopt/logopt_test.go +++ b/src/cmd/compile/internal/logopt/logopt_test.go @@ -213,7 +213,7 @@ func s15a8(x *[15]int64) [15]int64 { want(t, slogged, `{"range":{"start":{"line":7,"character":13},"end":{"line":7,"character":13}},"severity":3,"code":"leak","source":"go compiler","message":"parameter z leaks to ~r2 with derefs=0",`+ `"relatedInformation":[`+ `{"location":{"uri":"file://tmpdir/file.go","range":{"start":{"line":9,"character":13},"end":{"line":9,"character":13}}},"message":"escflow: flow: y = z:"},`+ - `{"location":{"uri":"file://tmpdir/file.go","range":{"start":{"line":9,"character":13},"end":{"line":9,"character":13}}},"message":"escflow: from y = \u003cN\u003e (assign-pair)"},`+ + `{"location":{"uri":"file://tmpdir/file.go","range":{"start":{"line":9,"character":13},"end":{"line":9,"character":13}}},"message":"escflow: from y := z (assign-pair)"},`+ `{"location":{"uri":"file://tmpdir/file.go","range":{"start":{"line":9,"character":13},"end":{"line":9,"character":13}}},"message":"escflow: flow: ~r1 = y:"},`+ `{"location":{"uri":"file://tmpdir/file.go","range":{"start":{"line":4,"character":11},"end":{"line":4,"character":11}}},"message":"inlineLoc"},`+ `{"location":{"uri":"file://tmpdir/file.go","range":{"start":{"line":9,"character":13},"end":{"line":9,"character":13}}},"message":"escflow: from y.b (dot of pointer)"},`+ -- cgit v1.3 From 1bcf6beec53ae811490fcd0ac29328b12b53702c Mon Sep 17 00:00:00 2001 From: Matthew Dempsky Date: Tue, 22 Sep 2020 02:19:14 -0700 Subject: cmd/compile: use staticValue for inlining logic This CL replaces the ad hoc and duplicated logic for detecting inlinable calls with a single "inlCallee" function, which uses the "staticValue" helper function introduced in an earlier commit. Updates #41474. Change-Id: I103d4091b10366fce1344ef2501222b7df68f21d Reviewed-on: https://go-review.googlesource.com/c/go/+/256460 Reviewed-by: David Chase Reviewed-by: Cuong Manh Le Trust: Matthew Dempsky --- src/cmd/compile/internal/gc/inl.go | 92 +++++++------------------- src/cmd/compile/internal/logopt/logopt_test.go | 1 - test/inline.go | 9 +++ 3 files changed, 34 insertions(+), 68 deletions(-) (limited to 'src/cmd/compile/internal/logopt') diff --git a/src/cmd/compile/internal/gc/inl.go b/src/cmd/compile/internal/gc/inl.go index 8630560a9a..ba12cf40b5 100644 --- a/src/cmd/compile/internal/gc/inl.go +++ b/src/cmd/compile/internal/gc/inl.go @@ -325,18 +325,10 @@ func (v *hairyVisitor) visit(n *Node) bool { break } - if fn := n.Left.Func; fn != nil && fn.Inl != nil { - v.budget -= fn.Inl.Cost + if fn := inlCallee(n.Left); fn != nil && fn.Func.Inl != nil { + v.budget -= fn.Func.Inl.Cost break } - if n.Left.isMethodExpression() { - if d := asNode(n.Left.Sym.Def); d != nil && d.Func.Inl != nil { - v.budget -= d.Func.Inl.Cost - break - } - } - // TODO(mdempsky): Budget for OCLOSURE calls if we - // ever allow that. See #15561 and #23093. // Call cost for non-leaf inlining. v.budget -= v.extraCallCost @@ -679,53 +671,11 @@ func inlnode(n *Node, maxCost int32, inlMap map[*Node]bool) *Node { if Debug['m'] > 3 { fmt.Printf("%v:call to func %+v\n", n.Line(), n.Left) } - if n.Left.Func != nil && n.Left.Func.Inl != nil && !isIntrinsicCall(n) { // normal case - n = mkinlcall(n, n.Left, maxCost, inlMap) - } else if n.Left.isMethodExpression() && asNode(n.Left.Sym.Def) != nil { - n = mkinlcall(n, asNode(n.Left.Sym.Def), maxCost, inlMap) - } else if n.Left.Op == OCLOSURE { - if f := inlinableClosure(n.Left); f != nil { - n = mkinlcall(n, f, maxCost, inlMap) - } - } else if n.Left.Op == ONAME && n.Left.Name != nil && n.Left.Name.Defn != nil { - if d := n.Left.Name.Defn; d.Op == OAS && d.Right.Op == OCLOSURE { - if f := inlinableClosure(d.Right); f != nil { - // NB: this check is necessary to prevent indirect re-assignment of the variable - // having the address taken after the invocation or only used for reads is actually fine - // but we have no easy way to distinguish the safe cases - if d.Left.Name.Addrtaken() { - if Debug['m'] > 1 { - fmt.Printf("%v: cannot inline escaping closure variable %v\n", n.Line(), n.Left) - } - if logopt.Enabled() { - logopt.LogOpt(n.Pos, "cannotInlineCall", "inline", Curfn.funcname(), - fmt.Sprintf("%v cannot be inlined (escaping closure variable)", n.Left)) - } - break - } - - // ensure the variable is never re-assigned - if unsafe, a := reassigned(n.Left); unsafe { - if Debug['m'] > 1 { - if a != nil { - fmt.Printf("%v: cannot inline re-assigned closure variable at %v: %v\n", n.Line(), a.Line(), a) - if logopt.Enabled() { - logopt.LogOpt(n.Pos, "cannotInlineCall", "inline", Curfn.funcname(), - fmt.Sprintf("%v cannot be inlined (re-assigned closure variable)", a)) - } - } else { - fmt.Printf("%v: cannot inline global closure variable %v\n", n.Line(), n.Left) - if logopt.Enabled() { - logopt.LogOpt(n.Pos, "cannotInlineCall", "inline", Curfn.funcname(), - fmt.Sprintf("%v cannot be inlined (global closure variable)", n.Left)) - } - } - } - break - } - n = mkinlcall(n, f, maxCost, inlMap) - } - } + if isIntrinsicCall(n) { + break + } + if fn := inlCallee(n.Left); fn != nil && fn.Func.Inl != nil { + n = mkinlcall(n, fn, maxCost, inlMap) } case OCALLMETH: @@ -749,16 +699,22 @@ func inlnode(n *Node, maxCost int32, inlMap map[*Node]bool) *Node { return n } -// inlinableClosure takes an OCLOSURE node and follows linkage to the matching ONAME with -// the inlinable body. Returns nil if the function is not inlinable. -func inlinableClosure(n *Node) *Node { - c := n.Func.Closure - caninl(c) - f := c.Func.Nname - if f == nil || f.Func.Inl == nil { - return nil +// inlCallee takes a function-typed expression and returns the underlying function ONAME +// that it refers to if statically known. Otherwise, it returns nil. +func inlCallee(fn *Node) *Node { + fn = staticValue(fn) + switch { + case fn.Op == ONAME && fn.Class() == PFUNC: + if fn.isMethodExpression() { + return asNode(fn.Sym.Def) + } + return fn + case fn.Op == OCLOSURE: + c := fn.Func.Closure + caninl(c) + return c.Func.Nname } - return f + return nil } func staticValue(n *Node) *Node { @@ -771,7 +727,9 @@ func staticValue(n *Node) *Node { } } -// staticValue1 implements a simple SSA-like optimization. +// staticValue1 implements a simple SSA-like optimization. If n is a local variable +// that is initialized and never reassigned, staticValue1 returns the initializer +// expression. Otherwise, it returns nil. func staticValue1(n *Node) *Node { if n.Op != ONAME || n.Class() != PAUTO || n.Name.Addrtaken() { return nil diff --git a/src/cmd/compile/internal/logopt/logopt_test.go b/src/cmd/compile/internal/logopt/logopt_test.go index fb71e142e3..fca85c10fb 100644 --- a/src/cmd/compile/internal/logopt/logopt_test.go +++ b/src/cmd/compile/internal/logopt/logopt_test.go @@ -208,7 +208,6 @@ func s15a8(x *[15]int64) [15]int64 { `"relatedInformation":[{"location":{"uri":"file://tmpdir/file.go","range":{"start":{"line":4,"character":11},"end":{"line":4,"character":11}}},"message":"inlineLoc"}]}`) want(t, slogged, `{"range":{"start":{"line":11,"character":6},"end":{"line":11,"character":6}},"severity":3,"code":"isInBounds","source":"go compiler","message":""}`) want(t, slogged, `{"range":{"start":{"line":7,"character":6},"end":{"line":7,"character":6}},"severity":3,"code":"canInlineFunction","source":"go compiler","message":"cost: 35"}`) - want(t, slogged, `{"range":{"start":{"line":21,"character":21},"end":{"line":21,"character":21}},"severity":3,"code":"cannotInlineCall","source":"go compiler","message":"foo cannot be inlined (escaping closure variable)"}`) // escape analysis explanation want(t, slogged, `{"range":{"start":{"line":7,"character":13},"end":{"line":7,"character":13}},"severity":3,"code":"leak","source":"go compiler","message":"parameter z leaks to ~r2 with derefs=0",`+ `"relatedInformation":[`+ diff --git a/test/inline.go b/test/inline.go index 2f6fc0fe88..0e41873de4 100644 --- a/test/inline.go +++ b/test/inline.go @@ -49,6 +49,12 @@ func j(x int) int { // ERROR "can inline j" } } +func _() int { // ERROR "can inline _" + tmp1 := h + tmp2 := tmp1 + return tmp2(0) // ERROR "inlining call to h" +} + var somethingWrong error // local closures can be inlined @@ -58,6 +64,9 @@ func l(x, y int) (int, int, error) { } if x == y { e(somethingWrong) // ERROR "inlining call to l.func1" + } else { + f := e + f(nil) // ERROR "inlining call to l.func1" } return y, x, nil } -- cgit v1.3 From 063a91c0abef445154df1ba34ffb500eeccfe8bc Mon Sep 17 00:00:00 2001 From: Matthew Dempsky Date: Fri, 30 Oct 2020 12:58:28 -0700 Subject: cmd/compile: fix recognition of unnamed return variables In golang.org/cl/266199, I reused the existing code in inlining that recognizes anonymous variables. However, it turns out that code mistakenly recognizes anonymous return parameters as named when inlining a function from the same package. The issue is funcargs (which is only used for functions parsed from source) synthesizes ~r names for anonymous return parameters, but funcargs2 (which is only used for functions imported from export data) does not. This CL fixes the behavior so that anonymous return parameters are handled identically whether a function is inlined within the same package or across packages. It also adds a proper cross-package test case demonstrating #33160 is fixed in both cases. Change-Id: Iaa39a23f5666979a1f5ca6d09fc8c398e55b784c Reviewed-on: https://go-review.googlesource.com/c/go/+/266719 Run-TryBot: Matthew Dempsky TryBot-Result: Go Bot Reviewed-by: Cuong Manh Le Reviewed-by: David Chase Trust: Matthew Dempsky --- src/cmd/compile/internal/gc/inl.go | 2 +- src/cmd/compile/internal/logopt/logopt_test.go | 8 ++++---- test/fixedbugs/issue42284.dir/a.go | 9 ++++++++- test/fixedbugs/issue42284.dir/b.go | 5 ++++- 4 files changed, 17 insertions(+), 7 deletions(-) (limited to 'src/cmd/compile/internal/logopt') diff --git a/src/cmd/compile/internal/gc/inl.go b/src/cmd/compile/internal/gc/inl.go index 8a5c6d8666..253036fea6 100644 --- a/src/cmd/compile/internal/gc/inl.go +++ b/src/cmd/compile/internal/gc/inl.go @@ -1054,7 +1054,7 @@ func mkinlcall(n, fn *Node, maxCost int32, inlMap map[*Node]bool) *Node { var retvars []*Node for i, t := range fn.Type.Results().Fields().Slice() { var m *Node - if n := asNode(t.Nname); n != nil && !n.isBlank() { + if n := asNode(t.Nname); n != nil && !n.isBlank() && !strings.HasPrefix(n.Sym.Name, "~r") { m = inlvar(n) m = typecheck(m, ctxExpr) inlvars[n] = m diff --git a/src/cmd/compile/internal/logopt/logopt_test.go b/src/cmd/compile/internal/logopt/logopt_test.go index fca85c10fb..51bab49518 100644 --- a/src/cmd/compile/internal/logopt/logopt_test.go +++ b/src/cmd/compile/internal/logopt/logopt_test.go @@ -213,15 +213,15 @@ func s15a8(x *[15]int64) [15]int64 { `"relatedInformation":[`+ `{"location":{"uri":"file://tmpdir/file.go","range":{"start":{"line":9,"character":13},"end":{"line":9,"character":13}}},"message":"escflow: flow: y = z:"},`+ `{"location":{"uri":"file://tmpdir/file.go","range":{"start":{"line":9,"character":13},"end":{"line":9,"character":13}}},"message":"escflow: from y := z (assign-pair)"},`+ - `{"location":{"uri":"file://tmpdir/file.go","range":{"start":{"line":9,"character":13},"end":{"line":9,"character":13}}},"message":"escflow: flow: ~r1 = y:"},`+ + `{"location":{"uri":"file://tmpdir/file.go","range":{"start":{"line":9,"character":13},"end":{"line":9,"character":13}}},"message":"escflow: flow: ~R0 = y:"},`+ `{"location":{"uri":"file://tmpdir/file.go","range":{"start":{"line":4,"character":11},"end":{"line":4,"character":11}}},"message":"inlineLoc"},`+ `{"location":{"uri":"file://tmpdir/file.go","range":{"start":{"line":9,"character":13},"end":{"line":9,"character":13}}},"message":"escflow: from y.b (dot of pointer)"},`+ `{"location":{"uri":"file://tmpdir/file.go","range":{"start":{"line":4,"character":11},"end":{"line":4,"character":11}}},"message":"inlineLoc"},`+ `{"location":{"uri":"file://tmpdir/file.go","range":{"start":{"line":9,"character":13},"end":{"line":9,"character":13}}},"message":"escflow: from \u0026y.b (address-of)"},`+ `{"location":{"uri":"file://tmpdir/file.go","range":{"start":{"line":4,"character":9},"end":{"line":4,"character":9}}},"message":"inlineLoc"},`+ - `{"location":{"uri":"file://tmpdir/file.go","range":{"start":{"line":9,"character":13},"end":{"line":9,"character":13}}},"message":"escflow: from ~r1 = \u003cN\u003e (assign-pair)"},`+ - `{"location":{"uri":"file://tmpdir/file.go","range":{"start":{"line":9,"character":3},"end":{"line":9,"character":3}}},"message":"escflow: flow: ~r2 = ~r1:"},`+ - `{"location":{"uri":"file://tmpdir/file.go","range":{"start":{"line":9,"character":3},"end":{"line":9,"character":3}}},"message":"escflow: from return (*int)(~r1) (return)"}]}`) + `{"location":{"uri":"file://tmpdir/file.go","range":{"start":{"line":9,"character":13},"end":{"line":9,"character":13}}},"message":"escflow: from ~R0 = \u003cN\u003e (assign-pair)"},`+ + `{"location":{"uri":"file://tmpdir/file.go","range":{"start":{"line":9,"character":3},"end":{"line":9,"character":3}}},"message":"escflow: flow: ~r2 = ~R0:"},`+ + `{"location":{"uri":"file://tmpdir/file.go","range":{"start":{"line":9,"character":3},"end":{"line":9,"character":3}}},"message":"escflow: from return (*int)(~R0) (return)"}]}`) }) } diff --git a/test/fixedbugs/issue42284.dir/a.go b/test/fixedbugs/issue42284.dir/a.go index e1271af32d..ffe9310be3 100644 --- a/test/fixedbugs/issue42284.dir/a.go +++ b/test/fixedbugs/issue42284.dir/a.go @@ -9,12 +9,19 @@ type T int func (T) M() {} // ERROR "can inline T.M" +func E() I { // ERROR "can inline E" + return T(0) // ERROR "T\(0\) escapes to heap" +} + func F(i I) I { // ERROR "can inline F" "leaking param: i to result ~r1 level=0" i = nil return i } -func g() { // ERROR "can inline g" +func g() { + h := E() // ERROR "inlining call to E" "T\(0\) does not escape" + h.M() // ERROR "devirtualizing h.M to T" + // BAD: T(0) could be stack allocated. i := F(T(0)) // ERROR "inlining call to F" "T\(0\) escapes to heap" diff --git a/test/fixedbugs/issue42284.dir/b.go b/test/fixedbugs/issue42284.dir/b.go index 3305166db0..652aa32122 100644 --- a/test/fixedbugs/issue42284.dir/b.go +++ b/test/fixedbugs/issue42284.dir/b.go @@ -6,7 +6,10 @@ package b import "./a" -func g() { // ERROR "can inline g" +func g() { + h := a.E() // ERROR "inlining call to a.E" "a.I\(a.T\(0\)\) does not escape" + h.M() // ERROR "devirtualizing h.M to a.T" + // BAD: T(0) could be stack allocated. i := a.F(a.T(0)) // ERROR "inlining call to a.F" "a.T\(0\) escapes to heap" -- cgit v1.3 From db6032dd0cbce3e4feff0160287cbe3d9234a540 Mon Sep 17 00:00:00 2001 From: Ikko Ashimine Date: Wed, 9 Dec 2020 14:17:56 +0000 Subject: cmd/compile: fix message typo occurences -> occurrences Change-Id: Ia81671f5de8a24ddd303a77b4580e8c726f29122 GitHub-Last-Rev: 11f9ab9f8c2c9acd70bcf170930426547d9b63eb GitHub-Pull-Request: golang/go#43097 Reviewed-on: https://go-review.googlesource.com/c/go/+/276612 Reviewed-by: Robert Griesemer Reviewed-by: Keith Randall --- src/cmd/compile/internal/logopt/logopt_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/cmd/compile/internal/logopt') diff --git a/src/cmd/compile/internal/logopt/logopt_test.go b/src/cmd/compile/internal/logopt/logopt_test.go index 51bab49518..e121c1abd2 100644 --- a/src/cmd/compile/internal/logopt/logopt_test.go +++ b/src/cmd/compile/internal/logopt/logopt_test.go @@ -51,7 +51,7 @@ func want(t *testing.T, out string, desired string) { func wantN(t *testing.T, out string, desired string, n int) { if strings.Count(out, desired) != n { - t.Errorf("expected exactly %d occurences of %s in \n%s", n, desired, out) + t.Errorf("expected exactly %d occurrences of %s in \n%s", n, desired, out) } } -- cgit v1.3