Skip to content

Commit e283473

Browse files
committed
cmd/compile: avoid using destination pointer base type in memmove optimization
The type of the source and destination of a memmove call isn't always accurate. It will always be a pointer (or an unsafe.Pointer), but the base type might not be accurate. This comes about because multiple copies of a pointer with different base types are coalesced into a single value. In the failing example, the IData selector of the input argument is a *[32]byte in one branch of the type switch, and a *[]byte in the other branch. During the expand_calls pass both IDatas become just copies of the input register. Those copies are deduped and an arbitrary one wins (in this case, *[]byte is the unfortunate winner). Generally an op v can rely on v.Type during rewrite rules. But relying on v.Args[i].Type is discouraged. Fixes #55122 Change-Id: I348fd9accf2058a87cd191eec01d39cda612f120 Reviewed-on: https://go-review.googlesource.com/c/go/+/431496 TryBot-Result: Gopher Robot <[email protected]> Reviewed-by: Cherry Mui <[email protected]> Run-TryBot: Keith Randall <[email protected]> Reviewed-by: Cuong Manh Le <[email protected]> Reviewed-by: Keith Randall <[email protected]>
1 parent 0053ec4 commit e283473

File tree

4 files changed

+119
-33
lines changed

4 files changed

+119
-33
lines changed

src/cmd/compile/internal/ssa/gen/generic.rules

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2112,39 +2112,42 @@
21122112

21132113
// Inline small or disjoint runtime.memmove calls with constant length.
21142114
// See the comment in op Move in genericOps.go for discussion of the type.
2115-
2115+
//
2116+
// Note that we've lost any knowledge of the type and alignment requirements
2117+
// of the source and destination. We only know the size, and that the type
2118+
// contains no pointers.
2119+
// The type of the move is not necessarily v.Args[0].Type().Elem()!
2120+
// See issue 55122 for details.
2121+
//
21162122
// Because expand calls runs after prove, constants useful to this pattern may not appear.
21172123
// Both versions need to exist; the memory and register variants.
21182124
//
21192125
// Match post-expansion calls, memory version.
21202126
(SelectN [0] call:(StaticCall {sym} s1:(Store _ (Const(64|32) [sz]) s2:(Store _ src s3:(Store {t} _ dst mem)))))
21212127
&& sz >= 0
21222128
&& isSameCall(sym, "runtime.memmove")
2123-
&& t.IsPtr() // avoids TUNSAFEPTR, see issue 30061
21242129
&& s1.Uses == 1 && s2.Uses == 1 && s3.Uses == 1
21252130
&& isInlinableMemmove(dst, src, int64(sz), config)
21262131
&& clobber(s1, s2, s3, call)
2127-
=> (Move {t.Elem()} [int64(sz)] dst src mem)
2132+
=> (Move {types.Types[types.TUINT8]} [int64(sz)] dst src mem)
21282133

21292134
// Match post-expansion calls, register version.
21302135
(SelectN [0] call:(StaticCall {sym} dst src (Const(64|32) [sz]) mem))
21312136
&& sz >= 0
21322137
&& call.Uses == 1 // this will exclude all calls with results
21332138
&& isSameCall(sym, "runtime.memmove")
2134-
&& dst.Type.IsPtr() // avoids TUNSAFEPTR, see issue 30061
21352139
&& isInlinableMemmove(dst, src, int64(sz), config)
21362140
&& clobber(call)
2137-
=> (Move {dst.Type.Elem()} [int64(sz)] dst src mem)
2141+
=> (Move {types.Types[types.TUINT8]} [int64(sz)] dst src mem)
21382142

21392143
// Match pre-expansion calls.
21402144
(SelectN [0] call:(StaticLECall {sym} dst src (Const(64|32) [sz]) mem))
21412145
&& sz >= 0
21422146
&& call.Uses == 1 // this will exclude all calls with results
21432147
&& isSameCall(sym, "runtime.memmove")
2144-
&& dst.Type.IsPtr() // avoids TUNSAFEPTR, see issue 30061
21452148
&& isInlinableMemmove(dst, src, int64(sz), config)
21462149
&& clobber(call)
2147-
=> (Move {dst.Type.Elem()} [int64(sz)] dst src mem)
2150+
=> (Move {types.Types[types.TUINT8]} [int64(sz)] dst src mem)
21482151

21492152
// De-virtualize late-expanded interface calls into late-expanded static calls.
21502153
// Note that (ITab (IMake)) doesn't get rewritten until after the first opt pass,

src/cmd/compile/internal/ssa/rewritegeneric.go

Lines changed: 24 additions & 26 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

test/fixedbugs/issue55122.go

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
// run
2+
3+
// Copyright 2022 The Go Authors. All rights reserved.
4+
// Use of this source code is governed by a BSD-style
5+
// license that can be found in the LICENSE file.
6+
7+
package main
8+
9+
func main() {
10+
for i := 0; i < 10000; i++ {
11+
h(i)
12+
sink = make([]byte, 1024) // generate some garbage
13+
}
14+
}
15+
16+
func h(iter int) {
17+
var x [32]byte
18+
for i := 0; i < 32; i++ {
19+
x[i] = 99
20+
}
21+
g(&x)
22+
if x == ([32]byte{}) {
23+
return
24+
}
25+
for i := 0; i < 32; i++ {
26+
println(x[i])
27+
}
28+
panic(iter)
29+
}
30+
31+
//go:noinline
32+
func g(x interface{}) {
33+
switch e := x.(type) {
34+
case *[32]byte:
35+
var c [32]byte
36+
*e = c
37+
case *[]byte:
38+
*e = nil
39+
}
40+
}
41+
42+
var sink []byte

test/fixedbugs/issue55122b.go

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
// run
2+
3+
// Copyright 2022 The Go Authors. All rights reserved.
4+
// Use of this source code is governed by a BSD-style
5+
// license that can be found in the LICENSE file.
6+
7+
package main
8+
9+
func main() {
10+
for i := 0; i < 10000; i++ {
11+
h(i)
12+
sink = make([]byte, 1024) // generate some garbage
13+
}
14+
}
15+
16+
func h(iter int) {
17+
var x [32]byte
18+
for i := 0; i < 32; i++ {
19+
x[i] = 99
20+
}
21+
g(&x)
22+
if x == ([32]byte{}) {
23+
return
24+
}
25+
for i := 0; i < 32; i++ {
26+
println(x[i])
27+
}
28+
panic(iter)
29+
}
30+
31+
//go:noinline
32+
func g(x interface{}) {
33+
switch e := x.(type) {
34+
case *[32]byte:
35+
var c [32]byte
36+
*e = c
37+
case *[3]*byte:
38+
var c [3]*byte
39+
*e = c
40+
}
41+
}
42+
43+
var sink []byte

0 commit comments

Comments
 (0)