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 <mdempsky@google.com>
Auto-Submit: Matthew Dempsky <mdempsky@google.com>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>

diff --git a/src/cmd/compile/internal/noder/reader.go b/src/cmd/compile/internal/noder/reader.go
index cf1e1440df000b2c4d27504acefcfb85bc3e2e59..d1a8843138b89e29d410240241b40f5ec42f0165 100644 (file)
--- 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 2c050b79bd7a755dfa2926deb3de16b1c7fc2652..ebec33b6f4d97f61f8971a6769a0ee56d47a2f6a 100644 (file)
--- 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 ff70e4415101c54c4ee2256fbfe96ba32c05f9ad..5dc43ab070cd36049206c43a2645a86bca78c01f 100644 (file)
--- 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"
 }