diff --git a/src/cmd/compile/internal/ir/symtab.go b/src/cmd/compile/internal/ir/symtab.go index b204a1d544..148edb2c88 100644 --- a/src/cmd/compile/internal/ir/symtab.go +++ b/src/cmd/compile/internal/ir/symtab.go @@ -26,6 +26,7 @@ var Syms struct { GCWriteBarrier *obj.LSym Goschedguarded *obj.LSym Growslice *obj.LSym + Memmove *obj.LSym Msanread *obj.LSym Msanwrite *obj.LSym Msanmove *obj.LSym diff --git a/src/cmd/compile/internal/ssa/gen/genericOps.go b/src/cmd/compile/internal/ssa/gen/genericOps.go index da386b9dff..eb508afe30 100644 --- a/src/cmd/compile/internal/ssa/gen/genericOps.go +++ b/src/cmd/compile/internal/ssa/gen/genericOps.go @@ -355,7 +355,9 @@ var genericOps = []opData{ {name: "Load", argLength: 2}, // Load from arg0. arg1=memory {name: "Dereference", argLength: 2}, // Load from arg0. arg1=memory. Helper op for arg/result passing, result is an otherwise not-SSA-able "value". {name: "Store", argLength: 3, typ: "Mem", aux: "Typ"}, // Store arg1 to arg0. arg2=memory, aux=type. Returns memory. - // The source and destination of Move may overlap in some cases. See e.g. + // Normally we require that the source and destination of Move do not overlap. + // There is an exception when we know all the loads will happen before all + // the stores. In that case, overlap is ok. See // memmove inlining in generic.rules. When inlineablememmovesize (in ../rewrite.go) // returns true, we must do all loads before all stores, when lowering Move. // The type of Move is used for the write barrier pass to insert write barriers diff --git a/src/cmd/compile/internal/ssa/rewrite.go b/src/cmd/compile/internal/ssa/rewrite.go index 58f1fe9249..13eb86ade1 100644 --- a/src/cmd/compile/internal/ssa/rewrite.go +++ b/src/cmd/compile/internal/ssa/rewrite.go @@ -1362,7 +1362,8 @@ func zeroUpper56Bits(x *Value, depth int) bool { // isInlinableMemmove reports whether the given arch performs a Move of the given size // faster than memmove. It will only return true if replacing the memmove with a Move is -// safe, either because Move is small or because the arguments are disjoint. +// safe, either because Move will do all of its loads before any of its stores, or +// because the arguments are known to be disjoint. // This is used as a check for replacing memmove with Move ops. func isInlinableMemmove(dst, src *Value, sz int64, c *Config) bool { // It is always safe to convert memmove into Move when its arguments are disjoint. @@ -1381,6 +1382,9 @@ func isInlinableMemmove(dst, src *Value, sz int64, c *Config) bool { } return false } +func IsInlinableMemmove(dst, src *Value, sz int64, c *Config) bool { + return isInlinableMemmove(dst, src, sz, c) +} // logLargeCopy logs the occurrence of a large copy. // The best place to do this is in the rewrite rules where the size of the move is easy to find. @@ -1394,6 +1398,14 @@ func logLargeCopy(v *Value, s int64) bool { } return true } +func LogLargeCopy(funcName string, pos src.XPos, s int64) { + if s < 128 { + return + } + if logopt.Enabled() { + logopt.LogOpt(pos, "copy", "lower", funcName, fmt.Sprintf("%d bytes", s)) + } +} // hasSmallRotate reports whether the architecture has rotate instructions // for sizes < 32-bit. This is used to decide whether to promote some rotations. diff --git a/src/cmd/compile/internal/ssagen/ssa.go b/src/cmd/compile/internal/ssagen/ssa.go index 1fa905bcc9..a06bb2a98f 100644 --- a/src/cmd/compile/internal/ssagen/ssa.go +++ b/src/cmd/compile/internal/ssagen/ssa.go @@ -105,6 +105,7 @@ func InitConfig() { ir.Syms.GCWriteBarrier = typecheck.LookupRuntimeFunc("gcWriteBarrier") ir.Syms.Goschedguarded = typecheck.LookupRuntimeFunc("goschedguarded") ir.Syms.Growslice = typecheck.LookupRuntimeFunc("growslice") + ir.Syms.Memmove = typecheck.LookupRuntimeFunc("memmove") ir.Syms.Msanread = typecheck.LookupRuntimeFunc("msanread") ir.Syms.Msanwrite = typecheck.LookupRuntimeFunc("msanwrite") ir.Syms.Msanmove = typecheck.LookupRuntimeFunc("msanmove") @@ -1371,7 +1372,47 @@ func (s *state) zero(t *types.Type, dst *ssa.Value) { } func (s *state) move(t *types.Type, dst, src *ssa.Value) { + s.moveWhichMayOverlap(t, dst, src, false) +} +func (s *state) moveWhichMayOverlap(t *types.Type, dst, src *ssa.Value, mayOverlap bool) { s.instrumentMove(t, dst, src) + if mayOverlap && t.IsArray() && t.NumElem() > 1 && !ssa.IsInlinableMemmove(dst, src, t.Size(), s.f.Config) { + // Normally, when moving Go values of type T from one location to another, + // we don't need to worry about partial overlaps. The two Ts must either be + // in disjoint (nonoverlapping) memory or in exactly the same location. + // There are 2 cases where this isn't true: + // 1) Using unsafe you can arrange partial overlaps. + // 2) Since Go 1.17, you can use a cast from a slice to a ptr-to-array. + // https://go.dev/ref/spec#Conversions_from_slice_to_array_pointer + // This feature can be used to construct partial overlaps of array types. + // var a [3]int + // p := (*[2]int)(a[:]) + // q := (*[2]int)(a[1:]) + // *p = *q + // We don't care about solving 1. Or at least, we haven't historically + // and no one has complained. + // For 2, we need to ensure that if there might be partial overlap, + // then we can't use OpMove; we must use memmove instead. + // (memmove handles partial overlap by copying in the correct + // direction. OpMove does not.) + // + // Note that we have to be careful here not to introduce a call when + // we're marshaling arguments to a call or unmarshaling results from a call. + // Cases where this is happening must pass mayOverlap to false. + // (Currently this only happens when unmarshaling results of a call.) + if t.HasPointers() { + s.rtcall(ir.Syms.Typedmemmove, true, nil, s.reflectType(t), dst, src) + // We would have otherwise implemented this move with straightline code, + // including a write barrier. Pretend we issue a write barrier here, + // so that the write barrier tests work. (Otherwise they'd need to know + // the details of IsInlineableMemmove.) + s.curfn.SetWBPos(s.peekPos()) + } else { + s.rtcall(ir.Syms.Memmove, true, nil, dst, src, s.constInt(types.Types[types.TUINTPTR], t.Size())) + } + ssa.LogLargeCopy(s.f.Name, s.peekPos(), t.Size()) + return + } store := s.newValue3I(ssa.OpMove, types.TypeMem, t.Size(), dst, src, s.mem()) store.Aux = t s.vars[memVar] = store @@ -1547,6 +1588,36 @@ func (s *state) stmt(n ir.Node) { return } + // mayOverlap keeps track of whether the LHS and RHS might + // refer to overlapping memory. + mayOverlap := true + if n.Y == nil { + // Not a move at all, mayOverlap is not relevant. + } else if n.Def { + // A variable being defined cannot overlap anything else. + mayOverlap = false + } else if n.X.Op() == ir.ONAME && n.Y.Op() == ir.ONAME { + // Two named things never overlap. + // (Or they are identical, which we treat as nonoverlapping.) + mayOverlap = false + } else if n.Y.Op() == ir.ODEREF { + p := n.Y.(*ir.StarExpr).X + for p.Op() == ir.OCONVNOP { + p = p.(*ir.ConvExpr).X + } + if p.Op() == ir.OSPTR && p.(*ir.UnaryExpr).X.Type().IsString() { + // Pointer fields of strings point to unmodifiable memory. + // That memory can't overlap with the memory being written. + mayOverlap = false + } + } else if n.Y.Op() == ir.ORESULT || n.Y.Op() == ir.OCALLFUNC || n.Y.Op() == ir.OCALLINTER { + // When copying values out of the return area of a call, we know + // the source and destination don't overlap. Importantly, we must + // set mayOverlap so we don't introduce a call to memmove while + // we still have live data in the argument area. + mayOverlap = false + } + // Evaluate RHS. rhs := n.Y if rhs != nil { @@ -1647,7 +1718,7 @@ func (s *state) stmt(n ir.Node) { } } - s.assign(n.X, r, deref, skip) + s.assignWhichMayOverlap(n.X, r, deref, skip, mayOverlap) case ir.OIF: n := n.(*ir.IfStmt) @@ -3529,7 +3600,11 @@ const ( // If deref is true, then we do left = *right instead (and right has already been nil-checked). // If deref is true and right == nil, just do left = 0. // skip indicates assignments (at the top level) that can be avoided. +// mayOverlap indicates whether left&right might partially overlap in memory. Default is false. func (s *state) assign(left ir.Node, right *ssa.Value, deref bool, skip skipMask) { + s.assignWhichMayOverlap(left, right, deref, skip, false) +} +func (s *state) assignWhichMayOverlap(left ir.Node, right *ssa.Value, deref bool, skip skipMask, mayOverlap bool) { if left.Op() == ir.ONAME && ir.IsBlank(left) { return } @@ -3630,7 +3705,7 @@ func (s *state) assign(left ir.Node, right *ssa.Value, deref bool, skip skipMask if right == nil { s.zero(t, addr) } else { - s.move(t, addr, right) + s.moveWhichMayOverlap(t, addr, right, mayOverlap) } return } diff --git a/test/fixedbugs/issue54467.go b/test/fixedbugs/issue54467.go new file mode 100644 index 0000000000..42e221c954 --- /dev/null +++ b/test/fixedbugs/issue54467.go @@ -0,0 +1,26 @@ +// run + +// Copyright 2022 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 + +import "fmt" + +func main() { + var x [64]byte + for i := range x { + x[i] = byte(i) + } + y := x + + copy(x[4:36], x[2:34]) + *(*[32]byte)(y[4:36]) = *(*[32]byte)(y[2:34]) + + for i := range x { + if x[i] != y[i] { + fmt.Printf("x[%v] = %v; y[%v] = %v\n", i, x[i], i, y[i]) + } + } +} diff --git a/test/nilptr5.go b/test/nilptr5.go index 2c48c0b261..118746e4aa 100644 --- a/test/nilptr5.go +++ b/test/nilptr5.go @@ -1,7 +1,7 @@ // errorcheck -0 -d=nil -// +build !wasm -// +build !aix +//go:build !wasm && !aix +// +build !wasm,!aix // Copyright 2018 The Go Authors. All rights reserved. // Use of this source code is governed by a BSD-style @@ -20,7 +20,7 @@ func f5(p *float32, q *float64, r *float32, s *float64) float64 { return x + y } -type T [29]byte +type T struct{ b [29]byte } func f6(p, q *T) { x := *p // ERROR "removed nil check" @@ -28,6 +28,6 @@ func f6(p, q *T) { } // make sure to remove nil check for memory move (issue #18003) -func f8(t *[8]int) [8]int { +func f8(t *struct{ b [8]int }) struct{ b [8]int } { return *t // ERROR "removed nil check" }