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 <mpratt@google.com>
Reviewed-by: Keith Randall <khr@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Keith Randall <khr@golang.org>

diff --git a/src/cmd/compile/internal/typecheck/_builtin/runtime.go b/src/cmd/compile/internal/typecheck/_builtin/runtime.go
index 9a83911487830a96890561b877b9b886ccc793af..cf07f31e31b83ed3da95674a437385ef428e53d9 100644 (file)
--- a/src/cmd/compile/internal/typecheck/_builtin/runtime.go
+++ b/src/cmd/compile/internal/typecheck/_builtin/runtime.go
@@ -152,12 +152,14 @@ func mapassign_fast32ptr(mapType *byte, hmap map[any]any, key unsafe.Pointer) (v
 func mapassign_fast64(mapType *byte, hmap map[any]any, key uint64) (val *any)
 func mapassign_fast64ptr(mapType *byte, hmap map[any]any, key unsafe.Pointer) (val *any)
 func mapassign_faststr(mapType *byte, hmap map[any]any, key string) (val *any)
-func mapiterinit(mapType *byte, hmap map[any]any, hiter *any)
+func mapiterinit(mapType *byte, hmap map[any]any, hiter *any)  // old maps
+func mapIterStart(mapType *byte, hmap map[any]any, hiter *any) // swiss maps
 func mapdelete(mapType *byte, hmap map[any]any, key *any)
 func mapdelete_fast32(mapType *byte, hmap map[any]any, key uint32)
 func mapdelete_fast64(mapType *byte, hmap map[any]any, key uint64)
 func mapdelete_faststr(mapType *byte, hmap map[any]any, key string)
-func mapiternext(hiter *any)
+func mapiternext(hiter *any) // old maps
+func mapIterNext(hiter *any) // swiss maps
 func mapclear(mapType *byte, hmap map[any]any)
 
 // *byte is really *runtime.Type

diff --git a/src/cmd/compile/internal/typecheck/builtin.go b/src/cmd/compile/internal/typecheck/builtin.go
index 6860d78b2e4024b2b332343a657c5122f1b3fc57..be08d0b40329a12a601685059b900599c6e151ff 100644 (file)
--- a/src/cmd/compile/internal/typecheck/builtin.go
+++ b/src/cmd/compile/internal/typecheck/builtin.go
@@ -131,11 +131,13 @@ var runtimeDecls = [...]struct {
        {"mapassign_fast64ptr", funcTag, 96},
        {"mapassign_faststr", funcTag, 89},
        {"mapiterinit", funcTag, 97},
+       {"mapIterStart", funcTag, 97},
        {"mapdelete", funcTag, 97},
        {"mapdelete_fast32", funcTag, 98},
        {"mapdelete_fast64", funcTag, 99},
        {"mapdelete_faststr", funcTag, 100},
        {"mapiternext", funcTag, 101},
+       {"mapIterNext", funcTag, 101},
        {"mapclear", funcTag, 102},
        {"makechan64", funcTag, 104},
        {"makechan", funcTag, 105},

diff --git a/src/cmd/compile/internal/walk/range.go b/src/cmd/compile/internal/walk/range.go
index 27e71425c1b015ae831d76d6898981455b3f2f46..a51b218ae57d20c07b83a710f53c01e533f46b27 100644 (file)
--- a/src/cmd/compile/internal/walk/range.go
+++ b/src/cmd/compile/internal/walk/range.go
@@ -244,19 +244,24 @@ func walkRange(nrange *ir.RangeStmt) ir.Node {
                // depends on layout of iterator struct.
                // See cmd/compile/internal/reflectdata/reflect.go:MapIterType
                var keysym, elemsym *types.Sym
+               var iterInit, iterNext string
                if buildcfg.Experiment.SwissMap {
                        keysym = th.Field(0).Sym
                        elemsym = th.Field(1).Sym // ditto
+                       iterInit = "mapIterStart"
+                       iterNext = "mapIterNext"
                } else {
                        keysym = th.Field(0).Sym
                        elemsym = th.Field(1).Sym // ditto
+                       iterInit = "mapiterinit"
+                       iterNext = "mapiternext"
                }
 
-               fn := typecheck.LookupRuntime("mapiterinit", t.Key(), t.Elem(), th)
+               fn := typecheck.LookupRuntime(iterInit, t.Key(), t.Elem(), th)
                init = append(init, mkcallstmt1(fn, reflectdata.RangeMapRType(base.Pos, nrange), ha, typecheck.NodAddr(hit)))
                nfor.Cond = ir.NewBinaryExpr(base.Pos, ir.ONE, ir.NewSelectorExpr(base.Pos, ir.ODOT, hit, keysym), typecheck.NodNil())
 
-               fn = typecheck.LookupRuntime("mapiternext", th)
+               fn = typecheck.LookupRuntime(iterNext, th)
                nfor.Post = mkcallstmt1(fn, typecheck.NodAddr(hit))
 
                key := ir.NewStarExpr(base.Pos, typecheck.ConvNop(ir.NewSelectorExpr(base.Pos, ir.ODOT, hit, keysym), types.NewPtr(t.Key())))

diff --git a/src/cmd/internal/goobj/builtinlist.go b/src/cmd/internal/goobj/builtinlist.go
index c133c60427f59855c281e658d53dfaebb98f61ae..3e550d8dd9763b8f3df6e881f974ecb3148fc872 100644 (file)
--- a/src/cmd/internal/goobj/builtinlist.go
+++ b/src/cmd/internal/goobj/builtinlist.go
@@ -110,11 +110,13 @@ var builtins = [...]struct {
        {"runtime.mapassign_fast64ptr", 1},
        {"runtime.mapassign_faststr", 1},
        {"runtime.mapiterinit", 1},
+       {"runtime.mapIterStart", 1},
        {"runtime.mapdelete", 1},
        {"runtime.mapdelete_fast32", 1},
        {"runtime.mapdelete_fast64", 1},
        {"runtime.mapdelete_faststr", 1},
        {"runtime.mapiternext", 1},
+       {"runtime.mapIterNext", 1},
        {"runtime.mapclear", 1},
        {"runtime.makechan64", 1},
        {"runtime.makechan", 1},

diff --git a/src/reflect/map_noswiss.go b/src/reflect/map_noswiss.go
index eb0a52a3902f6f963e91c9ac3028f87834795fb7..19696a4f4bd858bff83e93718022adfda118cd5e 100644 (file)
--- a/src/reflect/map_noswiss.go
+++ b/src/reflect/map_noswiss.go
@@ -17,6 +17,14 @@ type mapType struct {
        abi.OldMapType
 }
 
+// Pushed from runtime.
+
+//go:noescape
+func mapiterinit(t *abi.Type, m unsafe.Pointer, it *hiter)
+
+//go:noescape
+func mapiternext(it *hiter)
+
 func (t *rtype) Key() Type {
        if t.Kind() != Map {
                panic("reflect: Key of non-map type " + t.String())

diff --git a/src/reflect/map_swiss.go b/src/reflect/map_swiss.go
index 75dcb117dfd2926a6a7a894132bb17ad9106675e..2eac51e57dba5df282e507c4700f6be891580ae8 100644 (file)
--- a/src/reflect/map_swiss.go
+++ b/src/reflect/map_swiss.go
@@ -8,14 +8,16 @@ package reflect
 
 import (
        "internal/abi"
+       "internal/race"
        "internal/runtime/maps"
+       "internal/runtime/sys"
        "unsafe"
 )
 
 // mapType represents a map type.
-type mapType struct {
-       abi.SwissMapType
-}
+//
+// TODO(prattmic): Only used within this file, could be cleaned up.
+type mapType = abi.SwissMapType
 
 func (t *rtype) Key() Type {
        if t.Kind() != Map {
@@ -176,6 +178,31 @@ func (v Value) MapIndex(key Value) Value {
        return copyVal(typ, fl, e)
 }
 
+// Equivalent to runtime.mapIterStart.
+//
+//go:noinline
+func mapIterStart(t *abi.SwissMapType, m *maps.Map, it *maps.Iter) {
+       if race.Enabled && m != nil {
+               callerpc := sys.GetCallerPC()
+               race.ReadPC(unsafe.Pointer(m), callerpc, abi.FuncPCABIInternal(mapIterStart))
+       }
+
+       it.Init(t, m)
+       it.Next()
+}
+
+// Equivalent to runtime.mapIterNext.
+//
+//go:noinline
+func mapIterNext(it *maps.Iter) {
+       if race.Enabled {
+               callerpc := sys.GetCallerPC()
+               race.ReadPC(unsafe.Pointer(it.Map()), callerpc, abi.FuncPCABIInternal(mapIterNext))
+       }
+
+       it.Next()
+}
+
 // MapKeys returns a slice containing all the keys present in the map,
 // in unspecified order.
 // It panics if v's Kind is not [Map].
@@ -187,13 +214,17 @@ func (v Value) MapKeys() []Value {
 
        fl := v.flag.ro() | flag(keyType.Kind())
 
-       m := v.pointer()
+       // Escape analysis can't see that the map doesn't escape. It sees an
+       // escape from maps.IterStart, via assignment into it, even though it
+       // doesn't escape this function.
+       mptr := abi.NoEscape(v.pointer())
+       m := (*maps.Map)(mptr)
        mlen := int(0)
        if m != nil {
-               mlen = maplen(m)
+               mlen = maplen(mptr)
        }
        var it maps.Iter
-       mapiterinit(v.typ(), m, &it)
+       mapIterStart(tt, m, &it)
        a := make([]Value, mlen)
        var i int
        for i = 0; i < len(a); i++ {
@@ -205,7 +236,7 @@ func (v Value) MapKeys() []Value {
                        break
                }
                a[i] = copyVal(keyType, fl, key)
-               mapiternext(&it)
+               mapIterNext(&it)
        }
        return a[:i]
 }
@@ -317,12 +348,14 @@ func (iter *MapIter) Next() bool {
                panic("MapIter.Next called on an iterator that does not have an associated map Value")
        }
        if !iter.hiter.Initialized() {
-               mapiterinit(iter.m.typ(), iter.m.pointer(), &iter.hiter)
+               t := (*mapType)(unsafe.Pointer(iter.m.typ()))
+               m := (*maps.Map)(iter.m.pointer())
+               mapIterStart(t, m, &iter.hiter)
        } else {
                if iter.hiter.Key() == nil {
                        panic("MapIter.Next called on exhausted iterator")
                }
-               mapiternext(&iter.hiter)
+               mapIterNext(&iter.hiter)
        }
        return iter.hiter.Key() != nil
 }

diff --git a/src/reflect/value.go b/src/reflect/value.go
index 4ed94addf99a57c9c99404283584e2b319d7bc74..ba5b106c18c970357d76445e0494a90cc7ec78c1 100644 (file)
--- a/src/reflect/value.go
+++ b/src/reflect/value.go
@@ -3603,12 +3603,6 @@ func mapdelete(t *abi.Type, m unsafe.Pointer, key unsafe.Pointer)
 //go:noescape
 func mapdelete_faststr(t *abi.Type, m unsafe.Pointer, key string)
 
-//go:noescape
-func mapiterinit(t *abi.Type, m unsafe.Pointer, it *hiter)
-
-//go:noescape
-func mapiternext(it *hiter)
-
 //go:noescape
 func maplen(m unsafe.Pointer) int
 

diff --git a/src/runtime/map_swiss.go b/src/runtime/map_swiss.go
index e6e29bcfb800bb3ef7a09684205a0ae4e0a0185e..f4b4062dd9d44ef35c85a46581a16ec5cdabf6a6 100644 (file)
--- a/src/runtime/map_swiss.go
+++ b/src/runtime/map_swiss.go
@@ -176,11 +176,21 @@ func mapdelete(t *abi.SwissMapType, m *maps.Map, key unsafe.Pointer) {
 // Do not remove or change the type signature.
 // See go.dev/issue/67401.
 //
-//go:linkname mapiterinit
+// TODO go:linkname mapiterinit
 func mapiterinit(t *abi.SwissMapType, m *maps.Map, it *maps.Iter) {
+       // N.B. This is required by the builtin list in internal/goobj because
+       // it is a builtin for old maps.
+       throw("unreachable")
+}
+
+// mapIterStart initializes the Iter struct used for ranging over maps and
+// performs the first step of iteration. The Iter struct pointed to by 'it' is
+// allocated on the stack by the compilers order pass or on the heap by
+// reflect. Both need to have zeroed it since the struct contains pointers.
+func mapIterStart(t *abi.SwissMapType, m *maps.Map, it *maps.Iter) {
        if raceenabled && m != nil {
                callerpc := sys.GetCallerPC()
-               racereadpc(unsafe.Pointer(m), callerpc, abi.FuncPCABIInternal(mapiterinit))
+               racereadpc(unsafe.Pointer(m), callerpc, abi.FuncPCABIInternal(mapIterStart))
        }
 
        it.Init(t, m)
@@ -199,11 +209,19 @@ func mapiterinit(t *abi.SwissMapType, m *maps.Map, it *maps.Iter) {
 // Do not remove or change the type signature.
 // See go.dev/issue/67401.
 //
-//go:linkname mapiternext
+// TODO go:linkname mapiternext
 func mapiternext(it *maps.Iter) {
+       // N.B. This is required by the builtin list in internal/goobj because
+       // it is a builtin for old maps.
+       throw("unreachable")
+}
+
+// mapIterNext performs the next step of iteration. Afterwards, the next
+// key/elem are in it.Key()/it.Elem().
+func mapIterNext(it *maps.Iter) {
        if raceenabled {
                callerpc := sys.GetCallerPC()
-               racereadpc(unsafe.Pointer(it.Map()), callerpc, abi.FuncPCABIInternal(mapiternext))
+               racereadpc(unsafe.Pointer(it.Map()), callerpc, abi.FuncPCABIInternal(mapIterNext))
        }
 
        it.Next()
@@ -317,10 +335,10 @@ func reflect_mapdelete_faststr(t *abi.SwissMapType, m *maps.Map, key string) {
 // Do not remove or change the type signature.
 // See go.dev/issue/67401.
 //
-//go:linkname reflect_mapiterinit reflect.mapiterinit
-func reflect_mapiterinit(t *abi.SwissMapType, m *maps.Map, it *maps.Iter) {
-       mapiterinit(t, m, it)
-}
+// TODO go:linkname reflect_mapiterinit reflect.mapiterinit
+//func reflect_mapiterinit(t *abi.SwissMapType, m *maps.Map, it *maps.Iter) {
+//     mapiterinit(t, m, it)
+//}
 
 // reflect_mapiternext is for package reflect,
 // but widely used packages access it using linkname.
@@ -334,10 +352,10 @@ func reflect_mapiterinit(t *abi.SwissMapType, m *maps.Map, it *maps.Iter) {
 // Do not remove or change the type signature.
 // See go.dev/issue/67401.
 //
-//go:linkname reflect_mapiternext reflect.mapiternext
-func reflect_mapiternext(it *maps.Iter) {
-       mapiternext(it)
-}
+// TODO go:linkname reflect_mapiternext reflect.mapiternext
+//func reflect_mapiternext(it *maps.Iter) {
+//     mapiternext(it)
+//}
 
 // reflect_mapiterkey was for package reflect,
 // but widely used packages access it using linkname.
@@ -348,10 +366,10 @@ func reflect_mapiternext(it *maps.Iter) {
 // Do not remove or change the type signature.
 // See go.dev/issue/67401.
 //
-//go:linkname reflect_mapiterkey reflect.mapiterkey
-func reflect_mapiterkey(it *maps.Iter) unsafe.Pointer {
-       return it.Key()
-}
+// TODO go:linkname reflect_mapiterkey reflect.mapiterkey
+//func reflect_mapiterkey(it *maps.Iter) unsafe.Pointer {
+//     return it.Key()
+//}
 
 // reflect_mapiterelem was for package reflect,
 // but widely used packages access it using linkname.
@@ -362,10 +380,10 @@ func reflect_mapiterkey(it *maps.Iter) unsafe.Pointer {
 // Do not remove or change the type signature.
 // See go.dev/issue/67401.
 //
-//go:linkname reflect_mapiterelem reflect.mapiterelem
-func reflect_mapiterelem(it *maps.Iter) unsafe.Pointer {
-       return it.Elem()
-}
+// TODO go:linkname reflect_mapiterelem reflect.mapiterelem
+//func reflect_mapiterelem(it *maps.Iter) unsafe.Pointer {
+//     return it.Elem()
+//}
 
 // reflect_maplen is for package reflect,
 // but widely used packages access it using linkname.

diff --git a/test/codegen/maps.go b/test/codegen/maps.go
index 25505799e9351db7057457267833b0d6f6734e73..c4aed3354514f6fef3be62b48f869a83b1cca4d3 100644 (file)
--- a/test/codegen/maps.go
+++ b/test/codegen/maps.go
@@ -74,7 +74,7 @@ func LookupStringConversionKeyedArrayLit(m map[[2]string]int, bytes []byte) int
 
 func MapClearReflexive(m map[int]int) {
        // amd64:`.*runtime\.mapclear`
-       // amd64:-`.*runtime\.mapiterinit`
+       // amd64:-`.*runtime\.(mapiterinit|mapIterStart)`
        for k := range m {
                delete(m, k)
        }
@@ -83,7 +83,7 @@ func MapClearReflexive(m map[int]int) {
 func MapClearIndirect(m map[int]int) {
        s := struct{ m map[int]int }{m: m}
        // amd64:`.*runtime\.mapclear`
-       // amd64:-`.*runtime\.mapiterinit`
+       // amd64:-`.*runtime\.(mapiterinit|mapIterStart)`
        for k := range s.m {
                delete(s.m, k)
        }
@@ -91,14 +91,14 @@ func MapClearIndirect(m map[int]int) {
 
 func MapClearPointer(m map[*byte]int) {
        // amd64:`.*runtime\.mapclear`
-       // amd64:-`.*runtime\.mapiterinit`
+       // amd64:-`.*runtime\.(mapiterinit|mapIterStart)`
        for k := range m {
                delete(m, k)
        }
 }
 
 func MapClearNotReflexive(m map[float64]int) {
-       // amd64:`.*runtime\.mapiterinit`
+       // amd64:`.*runtime\.(mapiterinit|mapIterStart)`
        // amd64:-`.*runtime\.mapclear`
        for k := range m {
                delete(m, k)
@@ -106,7 +106,7 @@ func MapClearNotReflexive(m map[float64]int) {
 }
 
 func MapClearInterface(m map[interface{}]int) {
-       // amd64:`.*runtime\.mapiterinit`
+       // amd64:`.*runtime\.(mapiterinit|mapIterStart)`
        // amd64:-`.*runtime\.mapclear`
        for k := range m {
                delete(m, k)
@@ -115,7 +115,7 @@ func MapClearInterface(m map[interface{}]int) {
 
 func MapClearSideEffect(m map[int]int) int {
        k := 0
-       // amd64:`.*runtime\.mapiterinit`
+       // amd64:`.*runtime\.(mapiterinit|mapIterStart)`
        // amd64:-`.*runtime\.mapclear`
        for k = range m {
                delete(m, k)

diff --git a/test/live.go b/test/live.go
index 250a77cdac46144e54dca6a440f6a835a4a332c0..c0b0fcd274a0cac048b28a1470d6073216dbdca0 100644 (file)
--- a/test/live.go
+++ b/test/live.go
@@ -458,14 +458,14 @@ func f28(b bool) {
 
 func f29(b bool) {
        if b {
-               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)$"
+               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)$"
                        printstring(k) // ERROR "live at call to printstring: .autotmp_[0-9]+$"
                }
        }
-       for k := range m { // ERROR "live at call to mapiterinit: .autotmp_[0-9]+$" "live at call to mapiternext: .autotmp_[0-9]+$"
+       for k := range m { // ERROR "live at call to (mapiterinit|mapIterStart): .autotmp_[0-9]+$" "live at call to (mapiternext|mapIterNext): .autotmp_[0-9]+$"
                printstring(k) // ERROR "live at call to printstring: .autotmp_[0-9]+$"
        }
-       for k := range m { // ERROR "live at call to mapiterinit: .autotmp_[0-9]+$" "live at call to mapiternext: .autotmp_[0-9]+$"
+       for k := range m { // ERROR "live at call to (mapiterinit|mapIterStart): .autotmp_[0-9]+$" "live at call to (mapiternext|mapIterNext): .autotmp_[0-9]+$"
                printstring(k) // ERROR "live at call to printstring: .autotmp_[0-9]+$"
        }
 }

diff --git a/test/live_regabi.go b/test/live_regabi.go
index 090e2ec57776fe6915d5340920fbc41beac52a16..35f874ecc365f679bf0723920026eedba498c977 100644 (file)
--- a/test/live_regabi.go
+++ b/test/live_regabi.go
@@ -456,14 +456,14 @@ func f28(b bool) {
 
 func f29(b bool) {
        if b {
-               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)$"
+               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)$"
                        printstring(k) // ERROR "live at call to printstring: .autotmp_[0-9]+$"
                }
        }
-       for k := range m { // ERROR "live at call to mapiterinit: .autotmp_[0-9]+$" "live at call to mapiternext: .autotmp_[0-9]+$"
+       for k := range m { // ERROR "live at call to (mapiterinit|mapIterStart): .autotmp_[0-9]+$" "live at call to (mapiternext|mapIterNext): .autotmp_[0-9]+$"
                printstring(k) // ERROR "live at call to printstring: .autotmp_[0-9]+$"
        }
-       for k := range m { // ERROR "live at call to mapiterinit: .autotmp_[0-9]+$" "live at call to mapiternext: .autotmp_[0-9]+$"
+       for k := range m { // ERROR "live at call to (mapiterinit|mapIterStart): .autotmp_[0-9]+$" "live at call to (mapiternext|mapIterNext): .autotmp_[0-9]+$"
                printstring(k) // ERROR "live at call to printstring: .autotmp_[0-9]+$"
        }
 }