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>

5 files changed, 40 insertions, 22 deletions

diff --git a/src/cmd/compile/internal/abi/abiutils.go b/src/cmd/compile/internal/abi/abiutils.go
index b8ea1955d1..d657ddc867 100644
--- 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 a5cb0857b3..dfa76006de 100644
--- 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
index 0000000000..b969ddb972
--- /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
index 0000000000..83b5542144
--- /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
index 0000000000..3548e90d02
--- /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