cmd/compile: do not change field offset in ABI analysis

Currently, the ABI analysis assigns parameter/result offsets
to the fields of function *Type. In some cases, we may have
an ABI0 function reference and an ABIInternal reference share
the same function *Type. For example, for an ABI0 function F,
"f := F" will make f and (ABI0) F having the same *Type. But f,
as a func value, should use ABIInternal. Analyses on F and f will
collide and cause ICE.

Also, changing field offsets in ABI analysis has to be done very
carefully to avoid data races. It has been causing
trickiness/difficulty.

This CL removes the change of field offsets in ABI analysis
altogether. The analysis result is stored in ABIParamAssignment,
which is the only way to access parameter/result stack offset now.

Fixes #47317.
Fixes #47227.

Change-Id: I23a3e081a6cf327ac66855da222daaa636ed1ead
Reviewed-on: https://go-review.googlesource.com/c/go/+/336629
Trust: Cherry Mui <cherryyz@google.com>
Reviewed-by: Cuong Manh Le <cuong.manhle.vn@gmail.com>
Reviewed-by: Than McIntosh <thanm@google.com>

diff --git a/src/cmd/compile/internal/abi/abiutils.go b/src/cmd/compile/internal/abi/abiutils.go
index b8ea1955d13be363dbb500dc1152bff9b6a52ae3..d657ddc867bad00f44a00a3e9af54b7507b99683 100644 (file)
--- a/src/cmd/compile/internal/abi/abiutils.go
+++ b/src/cmd/compile/internal/abi/abiutils.go
@@ -446,35 +446,20 @@ func (config *ABIConfig) ABIAnalyze(t *types.Type, setNname bool) *ABIParamResul
        return result
 }
 
-// parameterUpdateMu protects the Offset field of function/method parameters (a subset of structure Fields)
-var parameterUpdateMu sync.Mutex
-
-// FieldOffsetOf returns a concurrency-safe version of f.Offset
-func FieldOffsetOf(f *types.Field) int64 {
-       parameterUpdateMu.Lock()
-       defer parameterUpdateMu.Unlock()
-       return f.Offset
-}
-
 func (config *ABIConfig) updateOffset(result *ABIParamResultInfo, f *types.Field, a ABIParamAssignment, isReturn, setNname bool) {
        // Everything except return values in registers has either a frame home (if not in a register) or a frame spill location.
        if !isReturn || len(a.Registers) == 0 {
                // The type frame offset DOES NOT show effects of minimum frame size.
                // Getting this wrong breaks stackmaps, see liveness/plive.go:WriteFuncMap and typebits/typebits.go:Set
-               parameterUpdateMu.Lock()
-               defer parameterUpdateMu.Unlock()
                off := a.FrameOffset(result)
                fOffset := f.Offset
                if fOffset == types.BOGUS_FUNARG_OFFSET {
-                       // Set the Offset the first time. After that, we may recompute it, but it should never change.
-                       f.Offset = off
-                       if f.Nname != nil {
-                               // always set it in this case.
+                       if setNname && f.Nname != nil {
                                f.Nname.(*ir.Name).SetFrameOffset(off)
                                f.Nname.(*ir.Name).SetIsOutputParamInRegisters(false)
                        }
-               } else if fOffset != off {
-                       base.Fatalf("offset for %s at %s changed from %d to %d", f.Sym.Name, base.FmtPos(f.Pos), fOffset, off)
+               } else {
+                       base.Fatalf("field offset for %s at %s has been set to %d", f.Sym.Name, base.FmtPos(f.Pos), fOffset)
                }
        } else {
                if setNname && f.Nname != nil {

diff --git a/src/cmd/compile/internal/ssagen/ssa.go b/src/cmd/compile/internal/ssagen/ssa.go
index a5cb0857b31a1f6cc7035e82dedb26b145d4a88b..dfa76006de790110b05def67fe3d8a4451584245 100644 (file)
--- a/src/cmd/compile/internal/ssagen/ssa.go
+++ b/src/cmd/compile/internal/ssagen/ssa.go
@@ -1296,7 +1296,7 @@ func (s *state) instrumentFields(t *types.Type, addr *ssa.Value, kind instrument
                if f.Sym.IsBlank() {
                        continue
                }
-               offptr := s.newValue1I(ssa.OpOffPtr, types.NewPtr(f.Type), abi.FieldOffsetOf(f), addr)
+               offptr := s.newValue1I(ssa.OpOffPtr, types.NewPtr(f.Type), f.Offset, addr)
                s.instrumentFields(f.Type, offptr, kind)
        }
 }
@@ -5053,19 +5053,23 @@ func (s *state) call(n *ir.CallExpr, k callKind, returnResultAddr bool) *ssa.Val
                ft := fn.Type()
                off := t.FieldOff(12) // TODO register args: be sure this isn't a hardcoded param stack offset.
                args := n.Args
+               i0 := 0
 
                // Set receiver (for interface calls). Always a pointer.
                if rcvr != nil {
                        p := s.newValue1I(ssa.OpOffPtr, ft.Recv().Type.PtrTo(), off, addr)
                        s.store(types.Types[types.TUINTPTR], p, rcvr)
+                       i0 = 1
                }
                // Set receiver (for method calls).
                if n.Op() == ir.OCALLMETH {
                        base.Fatalf("OCALLMETH missed by walkCall")
                }
                // Set other args.
-               for _, f := range ft.Params().Fields().Slice() {
-                       s.storeArgWithBase(args[0], f.Type, addr, off+abi.FieldOffsetOf(f))
+               // This code is only used when RegabiDefer is not enabled, and arguments are always
+               // passed on stack.
+               for i, f := range ft.Params().Fields().Slice() {
+                       s.storeArgWithBase(args[0], f.Type, addr, off+params.InParam(i+i0).FrameOffset(params))
                        args = args[1:]
                }
 
@@ -5078,7 +5082,6 @@ func (s *state) call(n *ir.CallExpr, k callKind, returnResultAddr bool) *ssa.Val
                if stksize < int64(types.PtrSize) {
                        // We need room for both the call to deferprocStack and the call to
                        // the deferred function.
-                       // TODO(register args) Revisit this if/when we pass args in registers.
                        stksize = int64(types.PtrSize)
                }
                call.AuxInt = stksize

diff --git a/test/fixedbugs/issue47317.dir/a.s b/test/fixedbugs/issue47317.dir/a.s
new file mode 100644 (file)
index 0000000..b969ddb
--- /dev/null
+++ b/test/fixedbugs/issue47317.dir/a.s
@@ -0,0 +1,6 @@
+// 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.
+
+TEXT   ·G(SB),4,$0
+       RET

diff --git a/test/fixedbugs/issue47317.dir/x.go b/test/fixedbugs/issue47317.dir/x.go
new file mode 100644 (file)
index 0000000..83b5542
--- /dev/null
+++ b/test/fixedbugs/issue47317.dir/x.go
@@ -0,0 +1,17 @@
+// 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.
+
+// Issue 47317: ICE when calling ABI0 function via func value.
+
+package main
+
+func main() { F() }
+
+func F() interface{} {
+       g := G
+       g(1)
+       return G
+}
+
+func G(x int) [2]int

diff --git a/test/fixedbugs/issue47317.go b/test/fixedbugs/issue47317.go
new file mode 100644 (file)
index 0000000..3548e90
--- /dev/null
+++ b/test/fixedbugs/issue47317.go
@@ -0,0 +1,7 @@
+// builddir
+
+// 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 ignored