]> Cypherpunks repositories - gostls13.git/commitdiff
hash/maphash, cmd/compile: make Comparable[string] not escape its argument
authorCherry Mui <cherryyz@google.com>
Mon, 2 Dec 2024 04:53:23 +0000 (23:53 -0500)
committerCherry Mui <cherryyz@google.com>
Mon, 2 Dec 2024 21:27:06 +0000 (21:27 +0000)
Currently, maphash.Comparable forces its argument to escape if it
contains a pointer, as we cannot hash stack pointers, which will
change when the stack moves. However, for a string, it is actually
okay if its data pointer points to the stack, as the hash depends
on only the content, not the pointer.

Currently there is no way to write this type-dependent escape
logic in Go code. So we implement it in the compiler as an
intrinsic. The compiler can also recognize not just the string
type, but types whose pointers are all string pointers, and make
them not escape.

Fixes #70560.

Change-Id: I3bf219ad71a238d2e35f0ea33de96487bc8cc231
Reviewed-on: https://go-review.googlesource.com/c/go/+/632715
Reviewed-by: David Chase <drchase@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

src/cmd/compile/internal/escape/call.go
src/cmd/compile/internal/inline/inl.go
src/cmd/compile/internal/walk/expr.go
src/hash/maphash/maphash.go
src/hash/maphash/maphash_purego.go
src/hash/maphash/maphash_runtime.go
src/hash/maphash/maphash_test.go

index 4a3753ada9cc91226a498d904bed201229e20cc8..1d7a0c90893c67afec28b188dfd6bc4f31dd329c 100644 (file)
@@ -10,6 +10,7 @@ import (
        "cmd/compile/internal/typecheck"
        "cmd/compile/internal/types"
        "cmd/internal/src"
+       "strings"
 )
 
 // call evaluates a call expressions, including builtin calls. ks
@@ -82,6 +83,29 @@ func (e *escape) call(ks []hole, call ir.Node) {
                        argument(e.tagHole(ks, fn, param), arg)
                }
 
+               // hash/maphash.escapeForHash forces its argument to be on
+               // the heap, if it contains a non-string pointer. We cannot
+               // hash pointers to local variables, as the address of the
+               // local variable might change on stack growth.
+               // Strings are okay as the hash depends on only the content,
+               // not the pointer.
+               // The actual call we match is
+               //   hash/maphash.escapeForHash[go.shape.T](dict, go.shape.T)
+               if fn != nil && fn.Sym().Pkg.Path == "hash/maphash" && strings.HasPrefix(fn.Sym().Name, "escapeForHash[") {
+                       ps := fntype.Params()
+                       if len(ps) == 2 && ps[1].Type.IsShape() {
+                               if !hasNonStringPointers(ps[1].Type) {
+                                       argumentParam = func(param *types.Field, arg ir.Node) {
+                                               argument(e.discardHole(), arg)
+                                       }
+                               } else {
+                                       argumentParam = func(param *types.Field, arg ir.Node) {
+                                               argument(e.heapHole(), arg)
+                                       }
+                               }
+                       }
+               }
+
                args := call.Args
                if recvParam := fntype.Recv(); recvParam != nil {
                        if recvArg == nil {
@@ -359,3 +383,23 @@ func (e *escape) tagHole(ks []hole, fn *ir.Name, param *types.Field) hole {
 
        return e.teeHole(tagKs...)
 }
+
+func hasNonStringPointers(t *types.Type) bool {
+       if !t.HasPointers() {
+               return false
+       }
+       switch t.Kind() {
+       case types.TSTRING:
+               return false
+       case types.TSTRUCT:
+               for _, f := range t.Fields() {
+                       if hasNonStringPointers(f.Type) {
+                               return true
+                       }
+               }
+               return false
+       case types.TARRAY:
+               return hasNonStringPointers(t.Elem())
+       }
+       return true
+}
index d64ab6b487b623dd728e2c47b9868a18ee29a948..f298f69ec1b6a7a0a3cecd06b9d82da35eef225c 100644 (file)
@@ -31,6 +31,7 @@ import (
        "go/constant"
        "internal/buildcfg"
        "strconv"
+       "strings"
 
        "cmd/compile/internal/base"
        "cmd/compile/internal/inline/inlheur"
@@ -481,6 +482,12 @@ opSwitch:
                                        case "panicrangestate":
                                                cheap = true
                                        }
+                               case "hash/maphash":
+                                       if strings.HasPrefix(fn, "escapeForHash[") {
+                                               // hash/maphash.escapeForHash[T] is a compiler intrinsic
+                                               // implemented in the escape analysis phase.
+                                               cheap = true
+                                       }
                                }
                        }
                        // Special case for coverage counter updates; although
@@ -803,6 +810,14 @@ func inlineCallCheck(callerfn *ir.Func, call *ir.CallExpr) (bool, bool) {
                        }
                }
        }
+
+       // hash/maphash.escapeForHash[T] is a compiler intrinsic implemented
+       // in the escape analysis phase.
+       if fn := ir.StaticCalleeName(call.Fun); fn != nil && fn.Sym().Pkg.Path == "hash/maphash" &&
+               strings.HasPrefix(fn.Sym().Name, "escapeForHash[") {
+               return false, true
+       }
+
        if ir.IsIntrinsicCall(call) {
                return false, true
        }
index 7afbc110420317ba5321cfca1ce6623e947902af..8cb38031908ad8a891bbe213531814f2ca75203f 100644 (file)
@@ -582,6 +582,20 @@ func walkCall(n *ir.CallExpr, init *ir.Nodes) ir.Node {
                return walkExpr(n, init)
        }
 
+       if n.Op() == ir.OCALLFUNC {
+               fn := ir.StaticCalleeName(n.Fun)
+               if fn != nil && fn.Sym().Pkg.Path == "hash/maphash" && strings.HasPrefix(fn.Sym().Name, "escapeForHash[") {
+                       // hash/maphash.escapeForHash[T] is a compiler intrinsic
+                       // for the escape analysis to escape its argument based on
+                       // the type. The call itself is no-op. Just walk the
+                       // argument.
+                       ps := fn.Type().Params()
+                       if len(ps) == 2 && ps[1].Type.IsShape() {
+                               return walkExpr(n.Args[1], init)
+                       }
+               }
+       }
+
        if name, ok := n.Fun.(*ir.Name); ok {
                sym := name.Sym()
                if sym.Pkg.Path == "go.runtime" && sym.Name == "deferrangefunc" {
index 20735671a7c089238adbc3d8c5f39712952d91e7..a8872d72a5aa77204e2f3000170eb5713ce92e33 100644 (file)
@@ -13,7 +13,6 @@
 package maphash
 
 import (
-       "internal/abi"
        "internal/byteorder"
        "math"
 )
@@ -286,21 +285,26 @@ func (h *Hash) BlockSize() int { return len(h.buf) }
 // such that Comparable(s, v1) == Comparable(s, v2) if v1 == v2.
 // If v != v, then the resulting hash is randomly distributed.
 func Comparable[T comparable](seed Seed, v T) uint64 {
-       comparableReady(v)
+       escapeForHash(v)
        return comparableHash(v, seed)
 }
 
-func comparableReady[T comparable](v T) {
-       // Force v to be on the heap.
-       // We cannot hash pointers to local variables,
-       // as the address of the local variable
-       // might change on stack growth.
-       abi.Escape(v)
-}
+// escapeForHash forces v to be on the heap, if v contains a
+// non-string pointer. We cannot hash pointers to local variables,
+// as the address of the local variable might change on stack growth.
+// Strings are okay as the hash depends on only the content, not
+// the pointer.
+//
+// This is essentially
+//
+//     if hasNonStringPointers(T) { abi.Escape(v) }
+//
+// Implemented as a compiler intrinsic.
+func escapeForHash[T comparable](v T) { panic("intrinsic") }
 
 // WriteComparable adds x to the data hashed by h.
 func WriteComparable[T comparable](h *Hash, x T) {
-       comparableReady(x)
+       escapeForHash(x)
        // writeComparable (not in purego mode) directly operates on h.state
        // without using h.buf. Mix in the buffer length so it won't
        // commute with a buffered write, which either changes h.n or changes
index 687626a8a2d23657cfa751e53ed315d448676f26..53636a48ca21c9ddb18d94d1f35f4fb7dc37548c 100644 (file)
@@ -14,6 +14,8 @@ import (
        "reflect"
 )
 
+const purego = true
+
 var hashkey [4]uint64
 
 func init() {
index 3f049a99243a9ff415c9d544d2da803128bd23f2..91e7d49e2c3e07718215d9de113ef6a52798f185 100644 (file)
@@ -13,6 +13,8 @@ import (
        "unsafe"
 )
 
+const purego = false
+
 //go:linkname runtime_rand runtime.rand
 func runtime_rand() uint64
 
index f5bccdaca83b63994bd0b8403e6472082bcfefef..4a85c8a6acadf41a986f4a149c3506670534798b 100644 (file)
@@ -8,6 +8,7 @@ import (
        "bytes"
        "fmt"
        "hash"
+       "internal/asan"
        "math"
        "reflect"
        "strings"
@@ -420,6 +421,36 @@ func TestWriteComparableNoncommute(t *testing.T) {
        }
 }
 
+func TestComparableAllocations(t *testing.T) {
+       if purego {
+               t.Skip("skip allocation test in purego mode - reflect-based implementation allocates more")
+       }
+       if asan.Enabled {
+               t.Skip("skip allocation test under -asan")
+       }
+       seed := MakeSeed()
+       x := heapStr(t)
+       allocs := testing.AllocsPerRun(10, func() {
+               s := "s" + x
+               Comparable(seed, s)
+       })
+       if allocs > 0 {
+               t.Errorf("got %v allocs, want 0", allocs)
+       }
+
+       type S struct {
+               a int
+               b string
+       }
+       allocs = testing.AllocsPerRun(10, func() {
+               s := S{123, "s" + x}
+               Comparable(seed, s)
+       })
+       if allocs > 0 {
+               t.Errorf("got %v allocs, want 0", allocs)
+       }
+}
+
 // Make sure a Hash implements the hash.Hash and hash.Hash64 interfaces.
 var _ hash.Hash = &Hash{}
 var _ hash.Hash64 = &Hash{}