Skip to content

Commit 4a1a783

Browse files
committed
cmd/compile: fix static initializer
staticcopy of a struct or array should recursively call itself, not staticassign. This fixes an issue where a struct with a slice in it is copied during static initialization. In this case, the backing array for the slice is duplicated, and each copy of the slice refers to a different backing array, which is incorrect. That issue has existed since at least Go 1.2. I'm not sure why this was never noticed. It seems like a pretty obvious bug if anyone modifies the resulting slice. In any case, we started to notice when the optimization in CL 140301 landed. Here is basically what happens in issue29013b.go: 1) The error above happens, so we get two backing stores for what should be the same slice. 2) The code for initializing those backing stores is reused. But not duplicated: they are the same Node structure. 3) The order pass allocates temporaries for the map operations. For the first instance, things work fine and two temporaries are allocated and stored in the OKEY nodes. For the second instance, the order pass decides new temporaries aren't needed, because the OKEY nodes already have temporaries in them. But the order pass also puts a VARKILL of the temporaries between the two instance initializations. 4) In this state, the code is technically incorrect. But before CL 140301 it happens to work because the temporaries are still correctly initialized when they are used for the second time. But then... 5) The new CL 140301 sees the VARKILLs and decides to reuse the temporary for instance 1 map 2 to initialize the instance 2 map 1 map. Because the keys aren't re-initialized, instance 2 map 1 gets the wrong key inserted into it. Fixes #29013 Change-Id: I840ce1b297d119caa706acd90e1517a5e47e9848 Reviewed-on: https://go-review.googlesource.com/c/152081 Run-TryBot: Keith Randall <[email protected]> TryBot-Result: Gobot Gobot <[email protected]> Reviewed-by: Josh Bleecher Snyder <[email protected]>
1 parent 5bd7e9c commit 4a1a783

File tree

4 files changed

+68
-7
lines changed

4 files changed

+68
-7
lines changed

src/cmd/compile/internal/gc/sinit.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -350,7 +350,7 @@ func staticcopy(l *Node, r *Node, out *[]*Node) bool {
350350
continue
351351
}
352352
ll := n.sepcopy()
353-
if staticassign(ll, e.Expr, out) {
353+
if staticcopy(ll, e.Expr, out) {
354354
continue
355355
}
356356
// Requires computation, but we're

test/fixedbugs/issue29013a.go

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
// run
2+
3+
// Copyright 2018 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+
type TestSuite struct {
10+
Tests []int
11+
}
12+
13+
var Suites = []TestSuite{
14+
Dicts,
15+
}
16+
var Dicts = TestSuite{
17+
Tests: []int{0},
18+
}
19+
20+
func main() {
21+
if &Dicts.Tests[0] != &Suites[0].Tests[0] {
22+
panic("bad")
23+
}
24+
}

test/fixedbugs/issue29013b.go

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
// run
2+
3+
// Copyright 2018 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+
type TestSuite struct {
10+
Tests []Test
11+
}
12+
type Test struct {
13+
Want interface{}
14+
}
15+
type Int struct {
16+
i int
17+
}
18+
19+
func NewInt(v int) Int {
20+
return Int{i: v}
21+
}
22+
23+
var Suites = []TestSuite{
24+
Dicts,
25+
}
26+
var Dicts = TestSuite{
27+
Tests: []Test{
28+
{
29+
Want: map[Int]bool{NewInt(1): true},
30+
},
31+
{
32+
Want: map[Int]string{
33+
NewInt(3): "3",
34+
},
35+
},
36+
},
37+
}
38+
39+
func main() {
40+
if Suites[0].Tests[0].Want.(map[Int]bool)[NewInt(3)] {
41+
panic("bad")
42+
}
43+
}

test/sinit.go

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -43,15 +43,12 @@ var c = []int{1201, 1202, 1203}
4343

4444
var aa = [3][3]int{[3]int{2001, 2002, 2003}, [3]int{2004, 2005, 2006}, [3]int{2007, 2008, 2009}}
4545
var as = [3]S{S{2101, 2102, 2103}, S{2104, 2105, 2106}, S{2107, 2108, 2109}}
46-
var ac = [3][]int{[]int{2201, 2202, 2203}, []int{2204, 2205, 2206}, []int{2207, 2208, 2209}}
4746

4847
var sa = SA{[3]int{3001, 3002, 3003}, [3]int{3004, 3005, 3006}, [3]int{3007, 3008, 3009}}
4948
var ss = SS{S{3101, 3102, 3103}, S{3104, 3105, 3106}, S{3107, 3108, 3109}}
50-
var sc = SC{[]int{3201, 3202, 3203}, []int{3204, 3205, 3206}, []int{3207, 3208, 3209}}
5149

5250
var ca = [][3]int{[3]int{4001, 4002, 4003}, [3]int{4004, 4005, 4006}, [3]int{4007, 4008, 4009}}
5351
var cs = []S{S{4101, 4102, 4103}, S{4104, 4105, 4106}, S{4107, 4108, 4109}}
54-
var cc = [][]int{[]int{4201, 4202, 4203}, []int{4204, 4205, 4206}, []int{4207, 4208, 4209}}
5552

5653
var answers = [...]int{
5754
// s
@@ -135,15 +132,12 @@ var copy_c = c
135132

136133
var copy_aa = aa
137134
var copy_as = as
138-
var copy_ac = ac
139135

140136
var copy_sa = sa
141137
var copy_ss = ss
142-
var copy_sc = sc
143138

144139
var copy_ca = ca
145140
var copy_cs = cs
146-
var copy_cc = cc
147141

148142
var copy_answers = answers
149143

0 commit comments

Comments
 (0)