From 052da5717e02659da49707873b3868fe36f2aaf0 Mon Sep 17 00:00:00 2001 From: Cherry Mui Date: Thu, 22 Jul 2021 12:42:09 -0400 Subject: 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 Reviewed-by: Cuong Manh Le Reviewed-by: Than McIntosh --- src/cmd/compile/internal/abi/abiutils.go | 21 +++------------------ src/cmd/compile/internal/ssagen/ssa.go | 11 +++++++---- test/fixedbugs/issue47317.dir/a.s | 6 ++++++ test/fixedbugs/issue47317.dir/x.go | 17 +++++++++++++++++ test/fixedbugs/issue47317.go | 7 +++++++ 5 files changed, 40 insertions(+), 22 deletions(-) create mode 100644 test/fixedbugs/issue47317.dir/a.s create mode 100644 test/fixedbugs/issue47317.dir/x.go create mode 100644 test/fixedbugs/issue47317.go 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 -- cgit v1.2.3-54-g00ecf