From 6a801d3082c6ab28372c115d13ef0c238e3535ae Mon Sep 17 00:00:00 2001 From: Matthew Dempsky Date: Wed, 24 Aug 2022 15:03:50 -0700 Subject: [PATCH] cmd/compile/internal/noder: fix inlined function literal positions When inlining function calls, we rewrite the position information on all of the nodes to keep track of the inlining context. This is necessary so that at runtime, we can synthesize additional stack frames so that the inlining is transparent to the user. However, for function literals, we *don't* want to apply this rewriting to the underlying function. Because within the function literal (when it's not itself inlined), the inlining context (if any) will have already be available at the caller PC instead. Unified IR was already getting this right in the case of user-written statements within the function literal, which is what the unit test for #46234 tested. However, it was still using inline-adjusted positions for the function declaration and its parameters, which occasionally end up getting used for generated code (e.g., loading captured values from the closure record). I've manually verified that this fixes the hang in https://go.dev/play/p/avQ0qgRzOgt, and spot-checked the -d=pctab=pctoinline output for kube-apiserver and kubelet and they seem better. However, I'm still working on a more robust test for this (hence "Updates" not "Fixes") and internal assertions to verify that we're emitting correct inline trees. In particular, there are still other cases (even in the non-unified frontend) where we're producing corrupt (but at least acyclic) inline trees. Updates #54625. Change-Id: Iacfd2e1eb06ae8dc299c0679f377461d3d46c15a Reviewed-on: https://go-review.googlesource.com/c/go/+/425395 Run-TryBot: Matthew Dempsky Auto-Submit: Matthew Dempsky Reviewed-by: Cuong Manh Le TryBot-Result: Gopher Robot Reviewed-by: David Chase --- src/cmd/compile/internal/noder/reader.go | 55 +++++++++++++++++++----- src/cmd/compile/internal/noder/writer.go | 2 +- test/inline_unified.go | 4 +- 3 files changed, 48 insertions(+), 13 deletions(-) diff --git a/src/cmd/compile/internal/noder/reader.go b/src/cmd/compile/internal/noder/reader.go index cf1e1440df..d1a8843138 100644 --- a/src/cmd/compile/internal/noder/reader.go +++ b/src/cmd/compile/internal/noder/reader.go @@ -136,6 +136,10 @@ type reader struct { inlTreeIndex int inlPosBases map[*src.PosBase]*src.PosBase + // suppressInlPos tracks whether position base rewriting for + // inlining should be suppressed. See funcLit. + suppressInlPos int + delayResults bool // Label to return to. @@ -286,9 +290,15 @@ func (pr *pkgReader) posBaseIdx(idx pkgbits.Index) *src.PosBase { return b } -// TODO(mdempsky): Document this. +// inlPosBase returns the inlining-adjusted src.PosBase corresponding +// to oldBase, which must be a non-inlined position. When not +// inlining, this is just oldBase. func (r *reader) inlPosBase(oldBase *src.PosBase) *src.PosBase { - if r.inlCall == nil { + if index := oldBase.InliningIndex(); index >= 0 { + base.Fatalf("oldBase %v already has inlining index %v", oldBase, index) + } + + if r.inlCall == nil || r.suppressInlPos != 0 { return oldBase } @@ -301,8 +311,10 @@ func (r *reader) inlPosBase(oldBase *src.PosBase) *src.PosBase { return newBase } -// TODO(mdempsky): Document this. -func (r *reader) updatePos(xpos src.XPos) src.XPos { +// inlPos returns the inlining-adjusted src.XPos corresponding to +// xpos, which must be a non-inlined position. When not inlining, this +// is just xpos. +func (r *reader) inlPos(xpos src.XPos) src.XPos { pos := base.Ctxt.PosTable.Pos(xpos) pos.SetBase(r.inlPosBase(pos.Base())) return base.Ctxt.PosTable.XPos(pos) @@ -1472,7 +1484,7 @@ func (r *reader) funcarg(param *types.Field, sym *types.Sym, ctxt ir.Class) { return } - name := ir.NewNameAt(r.updatePos(param.Pos), sym) + name := ir.NewNameAt(r.inlPos(param.Pos), sym) setType(name, param.Type) r.addLocal(name, ctxt) @@ -2715,7 +2727,13 @@ func syntheticSig(sig *types.Type) (params, results []*types.Field) { if sym == nil || sym.Name == "_" { sym = typecheck.LookupNum(".anon", i) } - res[i] = types.NewField(param.Pos, sym, param.Type) + // TODO(mdempsky): It would be nice to preserve the original + // parameter positions here instead, but at least + // typecheck.NewMethodType replaces them with base.Pos, making + // them useless. Worse, the positions copied from base.Pos may + // have inlining contexts, which we definitely don't want here + // (e.g., #54625). + res[i] = types.NewField(base.AutogeneratedPos, sym, param.Type) res[i].SetIsDDD(param.IsDDD()) } return res @@ -2756,7 +2774,7 @@ func (r *reader) optExpr() ir.Node { // otherwise, they need to create their own wrapper. func (r *reader) methodExpr() (wrapperFn, baseFn, dictPtr ir.Node) { recv := r.typ() - sig0 := r.signature(types.LocalPkg, nil) + sig0 := r.typ() pos := r.pos() _, sym := r.selector() @@ -3019,13 +3037,30 @@ func wrapName(pos src.XPos, x ir.Node) ir.Node { func (r *reader) funcLit() ir.Node { r.Sync(pkgbits.SyncFuncLit) + // The underlying function declaration (including its parameters' + // positions, if any) need to remain the original, uninlined + // positions. This is because we track inlining-context on nodes so + // we can synthesize the extra implied stack frames dynamically when + // generating tracebacks, whereas those stack frames don't make + // sense *within* the function literal. (Any necessary inlining + // adjustments will have been applied to the call expression + // instead.) + // + // This is subtle, and getting it wrong leads to cycles in the + // inlining tree, which lead to infinite loops during stack + // unwinding (#46234, #54625). + // + // Note that we *do* want the inline-adjusted position for the + // OCLOSURE node, because that position represents where any heap + // allocation of the closure is credited (#49171). + r.suppressInlPos++ pos := r.pos() xtype2 := r.signature(types.LocalPkg, nil) + r.suppressInlPos-- - opos := pos - - fn := ir.NewClosureFunc(opos, r.curfn != nil) + fn := ir.NewClosureFunc(pos, r.curfn != nil) clo := fn.OClosure + clo.SetPos(r.inlPos(pos)) // see comment above ir.NameClosure(clo, r.curfn) setType(fn.Nname, xtype2) diff --git a/src/cmd/compile/internal/noder/writer.go b/src/cmd/compile/internal/noder/writer.go index 2c050b79bd..ebec33b6f4 100644 --- a/src/cmd/compile/internal/noder/writer.go +++ b/src/cmd/compile/internal/noder/writer.go @@ -1973,7 +1973,7 @@ func (w *writer) methodExpr(expr *syntax.SelectorExpr, recv types2.Type, sel *ty sig := fun.Type().(*types2.Signature) w.typ(recv) - w.signature(sig) + w.typ(sig) w.pos(expr) w.selector(fun) diff --git a/test/inline_unified.go b/test/inline_unified.go index ff70e44151..5dc43ab070 100644 --- a/test/inline_unified.go +++ b/test/inline_unified.go @@ -13,9 +13,9 @@ func r(z int) int { return x + z } bar := func(x int) int { // ERROR "func literal does not escape" "can inline r.func2" - return x + func(y int) int { // ERROR "can inline r.func2.1" + return x + func(y int) int { // ERROR "can inline r.func2.1" "can inline r.func3" return 2*y + x*z }(x) // ERROR "inlining call to r.func2.1" } - return foo(42) + bar(42) // ERROR "inlining call to r.func1" "inlining call to r.func2" "can inline r.func3" "inlining call to r.func3" + return foo(42) + bar(42) // ERROR "inlining call to r.func1" "inlining call to r.func2" "inlining call to r.func3" }