Skip to content

Commit 78e6f2a

Browse files
prattmicgopherbot
prattmic
authored and
gopherbot
committed
runtime: rename mapiterinit and mapiternext
mapiterinit allows external linkname. These users must allocate their own iter struct for initialization by mapiterinit. Since the type is unexported, they also must define the struct themselves. As a result, they of course define the struct matching the old hiter definition (in map_noswiss.go). The old definition is smaller on 32-bit platforms. On those platforms, mapiternext will clobber memory outside of the caller's allocation. On all platforms, the pointer layout between the old hiter and new maps.Iter does not match. Thus the GC may miss pointers and free reachable objects early, or it may see non-pointers that look like heap pointers and throw due to invalid references to free objects. To avoid these issues, we must keep mapiterinit and mapiternext with the old hiter definition. The most straightforward way to do this is to use mapiterinit and mapiternext as a compatibility layer between the old and new iter types. The first step to that is to move normal map use off of these functions, which is what this CL does. Introduce new mapIterStart and mapIterNext functions that replace the former functions everywhere in the toolchain. These have the same behavior as the old functions. This CL temporarily makes the old functions throw to ensure we don't have hidden dependencies on them. We cannot remove them entirely because GOEXPERIMENT=noswissmap still uses the old names, and internal/goobj requires all builtins to exist regardless of GOEXPERIMENT. The next CL will introduce the compatibility layer. I want to avoid using linkname between runtime and reflect, as that would also allow external linknames. So mapIterStart and mapIterNext are duplicated in reflect, which can be done trivially, as it imports internal/runtime/maps. For #71408. Change-Id: I6a6a636c6d4bd1392618c67ca648d3f061afe669 Reviewed-on: https://go-review.googlesource.com/c/go/+/643898 Auto-Submit: Michael Pratt <[email protected]> Reviewed-by: Keith Randall <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-by: Keith Randall <[email protected]>
1 parent 4ebd5bf commit 78e6f2a

File tree

11 files changed

+115
-51
lines changed

11 files changed

+115
-51
lines changed

‎src/cmd/compile/internal/typecheck/_builtin/runtime.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -152,12 +152,14 @@ func mapassign_fast32ptr(mapType *byte, hmap map[any]any, key unsafe.Pointer) (v
152152
func mapassign_fast64(mapType *byte, hmap map[any]any, key uint64) (val *any)
153153
func mapassign_fast64ptr(mapType *byte, hmap map[any]any, key unsafe.Pointer) (val *any)
154154
func mapassign_faststr(mapType *byte, hmap map[any]any, key string) (val *any)
155-
func mapiterinit(mapType *byte, hmap map[any]any, hiter *any)
155+
func mapiterinit(mapType *byte, hmap map[any]any, hiter *any) // old maps
156+
func mapIterStart(mapType *byte, hmap map[any]any, hiter *any) // swiss maps
156157
func mapdelete(mapType *byte, hmap map[any]any, key *any)
157158
func mapdelete_fast32(mapType *byte, hmap map[any]any, key uint32)
158159
func mapdelete_fast64(mapType *byte, hmap map[any]any, key uint64)
159160
func mapdelete_faststr(mapType *byte, hmap map[any]any, key string)
160-
func mapiternext(hiter *any)
161+
func mapiternext(hiter *any) // old maps
162+
func mapIterNext(hiter *any) // swiss maps
161163
func mapclear(mapType *byte, hmap map[any]any)
162164

163165
// *byte is really *runtime.Type

‎src/cmd/compile/internal/typecheck/builtin.go

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

‎src/cmd/compile/internal/walk/range.go

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -244,19 +244,24 @@ func walkRange(nrange *ir.RangeStmt) ir.Node {
244244
// depends on layout of iterator struct.
245245
// See cmd/compile/internal/reflectdata/reflect.go:MapIterType
246246
var keysym, elemsym *types.Sym
247+
var iterInit, iterNext string
247248
if buildcfg.Experiment.SwissMap {
248249
keysym = th.Field(0).Sym
249250
elemsym = th.Field(1).Sym // ditto
251+
iterInit = "mapIterStart"
252+
iterNext = "mapIterNext"
250253
} else {
251254
keysym = th.Field(0).Sym
252255
elemsym = th.Field(1).Sym // ditto
256+
iterInit = "mapiterinit"
257+
iterNext = "mapiternext"
253258
}
254259

255-
fn := typecheck.LookupRuntime("mapiterinit", t.Key(), t.Elem(), th)
260+
fn := typecheck.LookupRuntime(iterInit, t.Key(), t.Elem(), th)
256261
init = append(init, mkcallstmt1(fn, reflectdata.RangeMapRType(base.Pos, nrange), ha, typecheck.NodAddr(hit)))
257262
nfor.Cond = ir.NewBinaryExpr(base.Pos, ir.ONE, ir.NewSelectorExpr(base.Pos, ir.ODOT, hit, keysym), typecheck.NodNil())
258263

259-
fn = typecheck.LookupRuntime("mapiternext", th)
264+
fn = typecheck.LookupRuntime(iterNext, th)
260265
nfor.Post = mkcallstmt1(fn, typecheck.NodAddr(hit))
261266

262267
key := ir.NewStarExpr(base.Pos, typecheck.ConvNop(ir.NewSelectorExpr(base.Pos, ir.ODOT, hit, keysym), types.NewPtr(t.Key())))

‎src/cmd/internal/goobj/builtinlist.go

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

‎src/reflect/map_noswiss.go

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,14 @@ type mapType struct {
1717
abi.OldMapType
1818
}
1919

20+
// Pushed from runtime.
21+
22+
//go:noescape
23+
func mapiterinit(t *abi.Type, m unsafe.Pointer, it *hiter)
24+
25+
//go:noescape
26+
func mapiternext(it *hiter)
27+
2028
func (t *rtype) Key() Type {
2129
if t.Kind() != Map {
2230
panic("reflect: Key of non-map type " + t.String())

‎src/reflect/map_swiss.go

Lines changed: 42 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -8,14 +8,16 @@ package reflect
88

99
import (
1010
"internal/abi"
11+
"internal/race"
1112
"internal/runtime/maps"
13+
"internal/runtime/sys"
1214
"unsafe"
1315
)
1416

1517
// mapType represents a map type.
16-
type mapType struct {
17-
abi.SwissMapType
18-
}
18+
//
19+
// TODO(prattmic): Only used within this file, could be cleaned up.
20+
type mapType = abi.SwissMapType
1921

2022
func (t *rtype) Key() Type {
2123
if t.Kind() != Map {
@@ -176,6 +178,31 @@ func (v Value) MapIndex(key Value) Value {
176178
return copyVal(typ, fl, e)
177179
}
178180

181+
// Equivalent to runtime.mapIterStart.
182+
//
183+
//go:noinline
184+
func mapIterStart(t *abi.SwissMapType, m *maps.Map, it *maps.Iter) {
185+
if race.Enabled && m != nil {
186+
callerpc := sys.GetCallerPC()
187+
race.ReadPC(unsafe.Pointer(m), callerpc, abi.FuncPCABIInternal(mapIterStart))
188+
}
189+
190+
it.Init(t, m)
191+
it.Next()
192+
}
193+
194+
// Equivalent to runtime.mapIterNext.
195+
//
196+
//go:noinline
197+
func mapIterNext(it *maps.Iter) {
198+
if race.Enabled {
199+
callerpc := sys.GetCallerPC()
200+
race.ReadPC(unsafe.Pointer(it.Map()), callerpc, abi.FuncPCABIInternal(mapIterNext))
201+
}
202+
203+
it.Next()
204+
}
205+
179206
// MapKeys returns a slice containing all the keys present in the map,
180207
// in unspecified order.
181208
// It panics if v's Kind is not [Map].
@@ -187,13 +214,17 @@ func (v Value) MapKeys() []Value {
187214

188215
fl := v.flag.ro() | flag(keyType.Kind())
189216

190-
m := v.pointer()
217+
// Escape analysis can't see that the map doesn't escape. It sees an
218+
// escape from maps.IterStart, via assignment into it, even though it
219+
// doesn't escape this function.
220+
mptr := abi.NoEscape(v.pointer())
221+
m := (*maps.Map)(mptr)
191222
mlen := int(0)
192223
if m != nil {
193-
mlen = maplen(m)
224+
mlen = maplen(mptr)
194225
}
195226
var it maps.Iter
196-
mapiterinit(v.typ(), m, &it)
227+
mapIterStart(tt, m, &it)
197228
a := make([]Value, mlen)
198229
var i int
199230
for i = 0; i < len(a); i++ {
@@ -205,7 +236,7 @@ func (v Value) MapKeys() []Value {
205236
break
206237
}
207238
a[i] = copyVal(keyType, fl, key)
208-
mapiternext(&it)
239+
mapIterNext(&it)
209240
}
210241
return a[:i]
211242
}
@@ -317,12 +348,14 @@ func (iter *MapIter) Next() bool {
317348
panic("MapIter.Next called on an iterator that does not have an associated map Value")
318349
}
319350
if !iter.hiter.Initialized() {
320-
mapiterinit(iter.m.typ(), iter.m.pointer(), &iter.hiter)
351+
t := (*mapType)(unsafe.Pointer(iter.m.typ()))
352+
m := (*maps.Map)(iter.m.pointer())
353+
mapIterStart(t, m, &iter.hiter)
321354
} else {
322355
if iter.hiter.Key() == nil {
323356
panic("MapIter.Next called on exhausted iterator")
324357
}
325-
mapiternext(&iter.hiter)
358+
mapIterNext(&iter.hiter)
326359
}
327360
return iter.hiter.Key() != nil
328361
}

‎src/reflect/value.go

Lines changed: 0 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3603,12 +3603,6 @@ func mapdelete(t *abi.Type, m unsafe.Pointer, key unsafe.Pointer)
36033603
//go:noescape
36043604
func mapdelete_faststr(t *abi.Type, m unsafe.Pointer, key string)
36053605

3606-
//go:noescape
3607-
func mapiterinit(t *abi.Type, m unsafe.Pointer, it *hiter)
3608-
3609-
//go:noescape
3610-
func mapiternext(it *hiter)
3611-
36123606
//go:noescape
36133607
func maplen(m unsafe.Pointer) int
36143608

‎src/runtime/map_swiss.go

Lines changed: 38 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -176,11 +176,21 @@ func mapdelete(t *abi.SwissMapType, m *maps.Map, key unsafe.Pointer) {
176176
// Do not remove or change the type signature.
177177
// See go.dev/issue/67401.
178178
//
179-
//go:linkname mapiterinit
179+
// TODO go:linkname mapiterinit
180180
func mapiterinit(t *abi.SwissMapType, m *maps.Map, it *maps.Iter) {
181+
// N.B. This is required by the builtin list in internal/goobj because
182+
// it is a builtin for old maps.
183+
throw("unreachable")
184+
}
185+
186+
// mapIterStart initializes the Iter struct used for ranging over maps and
187+
// performs the first step of iteration. The Iter struct pointed to by 'it' is
188+
// allocated on the stack by the compilers order pass or on the heap by
189+
// reflect. Both need to have zeroed it since the struct contains pointers.
190+
func mapIterStart(t *abi.SwissMapType, m *maps.Map, it *maps.Iter) {
181191
if raceenabled && m != nil {
182192
callerpc := sys.GetCallerPC()
183-
racereadpc(unsafe.Pointer(m), callerpc, abi.FuncPCABIInternal(mapiterinit))
193+
racereadpc(unsafe.Pointer(m), callerpc, abi.FuncPCABIInternal(mapIterStart))
184194
}
185195

186196
it.Init(t, m)
@@ -199,11 +209,19 @@ func mapiterinit(t *abi.SwissMapType, m *maps.Map, it *maps.Iter) {
199209
// Do not remove or change the type signature.
200210
// See go.dev/issue/67401.
201211
//
202-
//go:linkname mapiternext
212+
// TODO go:linkname mapiternext
203213
func mapiternext(it *maps.Iter) {
214+
// N.B. This is required by the builtin list in internal/goobj because
215+
// it is a builtin for old maps.
216+
throw("unreachable")
217+
}
218+
219+
// mapIterNext performs the next step of iteration. Afterwards, the next
220+
// key/elem are in it.Key()/it.Elem().
221+
func mapIterNext(it *maps.Iter) {
204222
if raceenabled {
205223
callerpc := sys.GetCallerPC()
206-
racereadpc(unsafe.Pointer(it.Map()), callerpc, abi.FuncPCABIInternal(mapiternext))
224+
racereadpc(unsafe.Pointer(it.Map()), callerpc, abi.FuncPCABIInternal(mapIterNext))
207225
}
208226

209227
it.Next()
@@ -317,10 +335,10 @@ func reflect_mapdelete_faststr(t *abi.SwissMapType, m *maps.Map, key string) {
317335
// Do not remove or change the type signature.
318336
// See go.dev/issue/67401.
319337
//
320-
//go:linkname reflect_mapiterinit reflect.mapiterinit
321-
func reflect_mapiterinit(t *abi.SwissMapType, m *maps.Map, it *maps.Iter) {
322-
mapiterinit(t, m, it)
323-
}
338+
// TODO go:linkname reflect_mapiterinit reflect.mapiterinit
339+
//func reflect_mapiterinit(t *abi.SwissMapType, m *maps.Map, it *maps.Iter) {
340+
// mapiterinit(t, m, it)
341+
//}
324342

325343
// reflect_mapiternext is for package reflect,
326344
// but widely used packages access it using linkname.
@@ -334,10 +352,10 @@ func reflect_mapiterinit(t *abi.SwissMapType, m *maps.Map, it *maps.Iter) {
334352
// Do not remove or change the type signature.
335353
// See go.dev/issue/67401.
336354
//
337-
//go:linkname reflect_mapiternext reflect.mapiternext
338-
func reflect_mapiternext(it *maps.Iter) {
339-
mapiternext(it)
340-
}
355+
// TODO go:linkname reflect_mapiternext reflect.mapiternext
356+
//func reflect_mapiternext(it *maps.Iter) {
357+
// mapiternext(it)
358+
//}
341359

342360
// reflect_mapiterkey was for package reflect,
343361
// but widely used packages access it using linkname.
@@ -348,10 +366,10 @@ func reflect_mapiternext(it *maps.Iter) {
348366
// Do not remove or change the type signature.
349367
// See go.dev/issue/67401.
350368
//
351-
//go:linkname reflect_mapiterkey reflect.mapiterkey
352-
func reflect_mapiterkey(it *maps.Iter) unsafe.Pointer {
353-
return it.Key()
354-
}
369+
// TODO go:linkname reflect_mapiterkey reflect.mapiterkey
370+
//func reflect_mapiterkey(it *maps.Iter) unsafe.Pointer {
371+
// return it.Key()
372+
//}
355373

356374
// reflect_mapiterelem was for package reflect,
357375
// but widely used packages access it using linkname.
@@ -362,10 +380,10 @@ func reflect_mapiterkey(it *maps.Iter) unsafe.Pointer {
362380
// Do not remove or change the type signature.
363381
// See go.dev/issue/67401.
364382
//
365-
//go:linkname reflect_mapiterelem reflect.mapiterelem
366-
func reflect_mapiterelem(it *maps.Iter) unsafe.Pointer {
367-
return it.Elem()
368-
}
383+
// TODO go:linkname reflect_mapiterelem reflect.mapiterelem
384+
//func reflect_mapiterelem(it *maps.Iter) unsafe.Pointer {
385+
// return it.Elem()
386+
//}
369387

370388
// reflect_maplen is for package reflect,
371389
// but widely used packages access it using linkname.

‎test/codegen/maps.go

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -74,7 +74,7 @@ func LookupStringConversionKeyedArrayLit(m map[[2]string]int, bytes []byte) int
7474

7575
func MapClearReflexive(m map[int]int) {
7676
// amd64:`.*runtime\.mapclear`
77-
// amd64:-`.*runtime\.mapiterinit`
77+
// amd64:-`.*runtime\.(mapiterinit|mapIterStart)`
7878
for k := range m {
7979
delete(m, k)
8080
}
@@ -83,30 +83,30 @@ func MapClearReflexive(m map[int]int) {
8383
func MapClearIndirect(m map[int]int) {
8484
s := struct{ m map[int]int }{m: m}
8585
// amd64:`.*runtime\.mapclear`
86-
// amd64:-`.*runtime\.mapiterinit`
86+
// amd64:-`.*runtime\.(mapiterinit|mapIterStart)`
8787
for k := range s.m {
8888
delete(s.m, k)
8989
}
9090
}
9191

9292
func MapClearPointer(m map[*byte]int) {
9393
// amd64:`.*runtime\.mapclear`
94-
// amd64:-`.*runtime\.mapiterinit`
94+
// amd64:-`.*runtime\.(mapiterinit|mapIterStart)`
9595
for k := range m {
9696
delete(m, k)
9797
}
9898
}
9999

100100
func MapClearNotReflexive(m map[float64]int) {
101-
// amd64:`.*runtime\.mapiterinit`
101+
// amd64:`.*runtime\.(mapiterinit|mapIterStart)`
102102
// amd64:-`.*runtime\.mapclear`
103103
for k := range m {
104104
delete(m, k)
105105
}
106106
}
107107

108108
func MapClearInterface(m map[interface{}]int) {
109-
// amd64:`.*runtime\.mapiterinit`
109+
// amd64:`.*runtime\.(mapiterinit|mapIterStart)`
110110
// amd64:-`.*runtime\.mapclear`
111111
for k := range m {
112112
delete(m, k)
@@ -115,7 +115,7 @@ func MapClearInterface(m map[interface{}]int) {
115115

116116
func MapClearSideEffect(m map[int]int) int {
117117
k := 0
118-
// amd64:`.*runtime\.mapiterinit`
118+
// amd64:`.*runtime\.(mapiterinit|mapIterStart)`
119119
// amd64:-`.*runtime\.mapclear`
120120
for k = range m {
121121
delete(m, k)

‎test/live.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -458,14 +458,14 @@ func f28(b bool) {
458458

459459
func f29(b bool) {
460460
if b {
461-
for k := range m { // ERROR "live at call to mapiterinit: .autotmp_[0-9]+$" "live at call to mapiternext: .autotmp_[0-9]+$" "stack object .autotmp_[0-9]+ (runtime.hiter|internal/runtime/maps.Iter)$"
461+
for k := range m { // ERROR "live at call to (mapiterinit|mapIterStart): .autotmp_[0-9]+$" "live at call to (mapiternext|mapIterNext): .autotmp_[0-9]+$" "stack object .autotmp_[0-9]+ (runtime.hiter|internal/runtime/maps.Iter)$"
462462
printstring(k) // ERROR "live at call to printstring: .autotmp_[0-9]+$"
463463
}
464464
}
465-
for k := range m { // ERROR "live at call to mapiterinit: .autotmp_[0-9]+$" "live at call to mapiternext: .autotmp_[0-9]+$"
465+
for k := range m { // ERROR "live at call to (mapiterinit|mapIterStart): .autotmp_[0-9]+$" "live at call to (mapiternext|mapIterNext): .autotmp_[0-9]+$"
466466
printstring(k) // ERROR "live at call to printstring: .autotmp_[0-9]+$"
467467
}
468-
for k := range m { // ERROR "live at call to mapiterinit: .autotmp_[0-9]+$" "live at call to mapiternext: .autotmp_[0-9]+$"
468+
for k := range m { // ERROR "live at call to (mapiterinit|mapIterStart): .autotmp_[0-9]+$" "live at call to (mapiternext|mapIterNext): .autotmp_[0-9]+$"
469469
printstring(k) // ERROR "live at call to printstring: .autotmp_[0-9]+$"
470470
}
471471
}

‎test/live_regabi.go

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -456,14 +456,14 @@ func f28(b bool) {
456456

457457
func f29(b bool) {
458458
if b {
459-
for k := range m { // ERROR "live at call to mapiterinit: .autotmp_[0-9]+$" "live at call to mapiternext: .autotmp_[0-9]+$" "stack object .autotmp_[0-9]+ (runtime.hiter|internal/runtime/maps.Iter)$"
459+
for k := range m { // ERROR "live at call to (mapiterinit|mapIterStart): .autotmp_[0-9]+$" "live at call to (mapiternext|mapIterNext): .autotmp_[0-9]+$" "stack object .autotmp_[0-9]+ (runtime.hiter|internal/runtime/maps.Iter)$"
460460
printstring(k) // ERROR "live at call to printstring: .autotmp_[0-9]+$"
461461
}
462462
}
463-
for k := range m { // ERROR "live at call to mapiterinit: .autotmp_[0-9]+$" "live at call to mapiternext: .autotmp_[0-9]+$"
463+
for k := range m { // ERROR "live at call to (mapiterinit|mapIterStart): .autotmp_[0-9]+$" "live at call to (mapiternext|mapIterNext): .autotmp_[0-9]+$"
464464
printstring(k) // ERROR "live at call to printstring: .autotmp_[0-9]+$"
465465
}
466-
for k := range m { // ERROR "live at call to mapiterinit: .autotmp_[0-9]+$" "live at call to mapiternext: .autotmp_[0-9]+$"
466+
for k := range m { // ERROR "live at call to (mapiterinit|mapIterStart): .autotmp_[0-9]+$" "live at call to (mapiternext|mapIterNext): .autotmp_[0-9]+$"
467467
printstring(k) // ERROR "live at call to printstring: .autotmp_[0-9]+$"
468468
}
469469
}

0 commit comments

Comments
 (0)