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 <[email protected]>
Auto-Submit: Matthew Dempsky <[email protected]>
Reviewed-by: Cuong Manh Le <[email protected]>
TryBot-Result: Gopher Robot <[email protected]>
Reviewed-by: David Chase <[email protected]>

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)

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)
 

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"
 }