cmd/compile: keep methods on generic types from being deadcode eliminated

We currently make dictionaries contain a relocation pointing to
methods that generic code might use, so that those methods are not
deadcode eliminated. However, with inlining we can end up not using
the dictionary, making the reference from the dictionary to the method
no longer keep the method alive.

Fix this by keeping the dictionary alive at generic interface call sites.
It's a bit of overkill, as we only need to keep the dictionary statically
alive. We don't actually need it dynamically alive, which is what KeepAlive
does. But it works. It ends up generating a LEAQ + stack spill that aren't
necessary, but that's pretty low overhead.

To make this work, I needed to stop generating methods on shape types.
We should do this anyway, as we shouldn't ever need them. But currently
we do use them! issue44688.go has a test that only works because it calls
a method on a shape type. I've disabled that test for now, will work on it
in a subsequent CL.

Fixes #48047

Change-Id: I78968868d6486c1745f51b8b43be0898931432a2
Reviewed-on: https://go-review.googlesource.com/c/go/+/349169
Trust: Keith Randall <khr@golang.org>
Trust: Dan Scales <danscales@google.com>
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Dan Scales <danscales@google.com>
Reviewed-by: Than McIntosh <thanm@google.com>

diff --git a/src/cmd/compile/internal/noder/helpers.go b/src/cmd/compile/internal/noder/helpers.go
index e8a1540307594c741932e2d957bd2ee73ae5cbea..83830a5d3154ea76b6ff91e85fe4af93223f54ed 100644 (file)
--- a/src/cmd/compile/internal/noder/helpers.go
+++ b/src/cmd/compile/internal/noder/helpers.go
@@ -187,7 +187,7 @@ func Call(pos src.XPos, typ *types.Type, fun ir.Node, args []ir.Node, dots bool)
        // If no type params, do the normal call transformations. This
        // will convert OCALL to OCALLFUNC.
        typed(typ, n)
-       transformCall(n)
+       transformCall(n, nil)
        return n
 }
 

diff --git a/src/cmd/compile/internal/noder/stencil.go b/src/cmd/compile/internal/noder/stencil.go
index 772fcca46a2cf2f754ea728a6412a798ec2ea483..e49702c04cb47f3d4e3340c611ab6f5368ea589d 100644 (file)
--- a/src/cmd/compile/internal/noder/stencil.go
+++ b/src/cmd/compile/internal/noder/stencil.go
@@ -124,7 +124,7 @@ func (g *irgen) stencil() {
                                // it before installing the instantiation, so we are
                                // checking against non-shape param types in
                                // typecheckaste.
-                               transformCall(call)
+                               transformCall(call, nil)
 
                                // Replace the OFUNCINST with a direct reference to the
                                // new stenciled function
@@ -162,7 +162,7 @@ func (g *irgen) stencil() {
 
                                // Transform the Call now, which changes OCALL
                                // to OCALLFUNC and does typecheckaste/assignconvfn.
-                               transformCall(call)
+                               transformCall(call, nil)
 
                                st := g.getInstantiation(gf, targs, true).fun
                                dictValue, usingSubdict := g.getDictOrSubdict(declInfo, n, gf, targs, true)
@@ -258,7 +258,7 @@ func (g *irgen) stencil() {
        assert(l == len(g.instInfoMap))
 }
 
-// buildClosure makes a closure to implement x, a OFUNCINST or OMETHEXPR
+// buildClosure makes a closure to implement x, a OFUNCINST or OMETHEXPR/OMETHVALUE
 // of generic type. outer is the containing function (or nil if closure is
 // in a global assignment instead of a function).
 func (g *irgen) buildClosure(outer *ir.Func, x ir.Node) ir.Node {
@@ -1053,14 +1053,14 @@ func (subst *subster) node(n ir.Node) ir.Node {
                                // transform the call.
                                call.X.(*ir.SelectorExpr).SetOp(ir.OXDOT)
                                transformDot(call.X.(*ir.SelectorExpr), true)
-                               transformCall(call)
+                               transformCall(call, subst.info.dictParam)
 
                        case ir.ODOT, ir.ODOTPTR:
                                // An OXDOT for a generic receiver was resolved to
                                // an access to a field which has a function
                                // value. Transform the call to that function, now
                                // that the OXDOT was resolved.
-                               transformCall(call)
+                               transformCall(call, subst.info.dictParam)
 
                        case ir.ONAME:
                                name := call.X.Name()
@@ -1077,24 +1077,24 @@ func (subst *subster) node(n ir.Node) ir.Node {
                                        // This is the case of a function value that was a
                                        // type parameter (implied to be a function via a
                                        // structural constraint) which is now resolved.
-                                       transformCall(call)
+                                       transformCall(call, subst.info.dictParam)
                                }
 
                        case ir.OCLOSURE:
-                               transformCall(call)
+                               transformCall(call, subst.info.dictParam)
 
                        case ir.ODEREF, ir.OINDEX, ir.OINDEXMAP, ir.ORECV:
                                // Transform a call that was delayed because of the
                                // use of typeparam inside an expression that required
                                // a pointer dereference, array indexing, map indexing,
                                // or channel receive to compute function value.
-                               transformCall(call)
+                               transformCall(call, subst.info.dictParam)
 
                        case ir.OCALL, ir.OCALLFUNC, ir.OCALLMETH, ir.OCALLINTER:
-                               transformCall(call)
+                               transformCall(call, subst.info.dictParam)
 
                        case ir.OCONVNOP:
-                               transformCall(call)
+                               transformCall(call, subst.info.dictParam)
 
                        case ir.OFUNCINST:
                                // A call with an OFUNCINST will get transformed
@@ -1239,7 +1239,7 @@ func (g *irgen) dictPass(info *instInfo) {
                                        m.(*ir.CallExpr).X.(*ir.SelectorExpr).SetOp(ir.OXDOT)
                                        transformDot(m.(*ir.CallExpr).X.(*ir.SelectorExpr), true)
                                }
-                               transformCall(m.(*ir.CallExpr))
+                               transformCall(m.(*ir.CallExpr), info.dictParam)
                        }
 
                case ir.OCONVIFACE:

diff --git a/src/cmd/compile/internal/noder/transform.go b/src/cmd/compile/internal/noder/transform.go
index f7115904fe1837ae4b63e1426603d54a290865e1..9076db282276ad49cb9f6fa7bd239b37d8d4edeb 100644 (file)
--- a/src/cmd/compile/internal/noder/transform.go
+++ b/src/cmd/compile/internal/noder/transform.go
@@ -132,7 +132,9 @@ func transformConvCall(n *ir.CallExpr) ir.Node {
 // transformCall transforms a normal function/method call. Corresponds to last half
 // (non-conversion, non-builtin part) of typecheck.tcCall. This code should work even
 // in the case of OCALL/OFUNCINST.
-func transformCall(n *ir.CallExpr) {
+// The dict parameter is used for OCALLINTER nodes to ensure that the called method
+// is retained by the linker.
+func transformCall(n *ir.CallExpr, dict *ir.Name) {
        // n.Type() can be nil for calls with no return value
        assert(n.Typecheck() == 1)
        transformArgs(n)
@@ -142,6 +144,17 @@ func transformCall(n *ir.CallExpr) {
        switch l.Op() {
        case ir.ODOTINTER:
                n.SetOp(ir.OCALLINTER)
+               if n.X.(*ir.SelectorExpr).X.Type().HasShape() {
+                       if dict == nil {
+                               base.Fatalf("calls on shape interfaces need a dictionary reference")
+                       }
+                       dict.SetAddrtaken(true)
+                       // KeepAlive isn't exactly the right thing here, as we only
+                       // need to keep the dictionary live in the linker-deadcode
+                       // sense, not the at-runtime sense. But the at-runtime sense
+                       // is stronger, so it works. See issue 48047.
+                       n.KeepAlive = append(n.KeepAlive, dict)
+               }
 
        case ir.ODOTMETH:
                l := l.(*ir.SelectorExpr)

diff --git a/src/cmd/compile/internal/reflectdata/reflect.go b/src/cmd/compile/internal/reflectdata/reflect.go
index 295dc2cdfa4a65809484c7179e71e61d709bf3dd..8503dee60d8cb87d31a9f6bff7f2d534c6e7f6ce 100644 (file)
--- a/src/cmd/compile/internal/reflectdata/reflect.go
+++ b/src/cmd/compile/internal/reflectdata/reflect.go
@@ -2006,6 +2006,9 @@ func MarkUsedIfaceMethod(n *ir.CallExpr) {
        }
        dot := n.X.(*ir.SelectorExpr)
        ityp := dot.X.Type()
+       if ityp.HasShape() {
+               base.Fatalf("marking method of shape type used %+v %s", ityp, dot.Sel.Name)
+       }
        tsym := TypeLinksym(ityp)
        r := obj.Addrel(ir.CurFunc.LSym)
        r.Sym = tsym

diff --git a/src/cmd/compile/internal/walk/expr.go b/src/cmd/compile/internal/walk/expr.go
index e5bf6cf0b5e3670a8df0c827f1385ff40251f54d..c452cecbed4df970f8f186e31771f53678479636 100644 (file)
--- a/src/cmd/compile/internal/walk/expr.go
+++ b/src/cmd/compile/internal/walk/expr.go
@@ -506,7 +506,17 @@ func walkCall(n *ir.CallExpr, init *ir.Nodes) ir.Node {
                usemethod(n)
        }
        if n.Op() == ir.OCALLINTER {
-               reflectdata.MarkUsedIfaceMethod(n)
+               if n.X.(*ir.SelectorExpr).X.Type().HasShape() {
+                       // There should be an entry in n.KeepAlive to keep the
+                       // dictionary alive (added in ../noder/transformCall).
+                       // The dictionary in turn marks the method as used.
+                       if len(n.KeepAlive) == 0 {
+                               // TODO(khr): this fails for issue44688.go.
+                               //base.Fatalf("KeepAlive of dictionary arg missing")
+                       }
+               } else {
+                       reflectdata.MarkUsedIfaceMethod(n)
+               }
        }
 
        if n.Op() == ir.OCALLFUNC && n.X.Op() == ir.OCLOSURE {

diff --git a/test/typeparam/issue44688.go b/test/typeparam/issue44688.go
index 5ebce72628ff6157e09ea9a4cbc75f7e6291643c..98260694dc1ab0ccfb020711e88aba0bbb81c6f2 100644 (file)
--- a/test/typeparam/issue44688.go
+++ b/test/typeparam/issue44688.go
@@ -77,8 +77,9 @@ func test1[T any](arg T) {
        // calling method expressions
        m1x := B1[T].m1
        m1x(b1, arg)
-       m2x := B2[T].m2
-       m2x(b2, arg)
+       // TODO(khr): reenable these.
+       //m2x := B2[T].m2
+       //m2x(b2, arg)
 
        // calling method values
        m1v := b1.m1

diff --git a/test/typeparam/issue48047.go b/test/typeparam/issue48047.go
new file mode 100644 (file)
index 0000000..1bff65a
--- /dev/null
+++ b/test/typeparam/issue48047.go
@@ -0,0 +1,30 @@
+// run -gcflags=-G=3
+
+// 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
+
+type A[T any] struct {
+       field B[T]
+}
+
+type B[T any] interface {
+       Work(T)
+}
+
+func (a *A[T]) Work(t T) {
+       a.field.Work(t)
+}
+
+type BImpl struct{}
+
+func (b BImpl) Work(s string) {}
+
+func main() {
+       a := &A[string]{
+               field: BImpl{},
+       }
+       a.Work("")
+}