Golang internal/concurrent: hashtriemap uses abi.NoEscape cause incorrect inlining decision

Go version

go version go1.23rc2 darwin/arm64

Output of `go env` in your module/workspace:

GO111MODULE=''
GOARCH='arm64'
GOBIN=''
GOCACHE='/Users/xxx/Library/Caches/go-build'
GOENV='/Users/xxx/Library/Application Support/go/env'
GOEXE=''
GOEXPERIMENT='arenas'
GOFLAGS=''
GOHOSTARCH='arm64'
GOHOSTOS='darwin'
GOINSECURE=''
GOMODCACHE='/Users/xxx/go/pkg/mod'
GOOS='darwin'
GOPATH='/Users/xxx/go'
GOROOT='/Users/xxx/sdk/go1.23rc2'
GOSUMDB='sum.golang.org'
GOTMPDIR=''
GOTOOLCHAIN='auto'
GOTOOLDIR='/Users/xxx/sdk/go1.23rc2/pkg/tool/darwin_arm64'
GOVCS=''
GOVERSION='go1.23rc2'
GODEBUG=''
GOTELEMETRY='local'
GOTELEMETRYDIR='/Users/xxx/Library/Application Support/go/telemetry'
GCCGO='gccgo'
GOARM64='v8.0'
AR='ar'
CC='clang'
CXX='clang++'
CGO_ENABLED='1'
GOMOD='/dev/null'
GOWORK=''
CGO_CFLAGS='-O2 -g'
CGO_CPPFLAGS=''
CGO_CXXFLAGS='-O2 -g'
CGO_FFLAGS='-O2 -g'
CGO_LDFLAGS='-O2 -g'
PKG_CONFIG='pkg-config'
GOGCCFLAGS='-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -ffile-prefix-map=/var/folders/df/2vy1kgkn1_98kqwtn0zgdyj80000gn/T/go-build3156128764=/tmp/go-build -gno-record-gcc-switches -fno-common'

What did you do?

Related #68415

Also, a code search found internal/reflectlite/value.go also uses abi.NoEscape, I'm not sure whether it needs to be fixed as well.

Run this program with -gcflags=all=-d=checkptr flag:

package main

import "unique"

func main() {
    unique.Make("")
}

What did you see happen?

fatal error: checkptr: pointer arithmetic result points to invalid allocation

goroutine 1 gp=0x140000021c0 m=0 mp=0x104adcb40 [running]:
runtime.throw({0x104a4a3c6?, 0x0?})
    /Users/xxx/sdk/go1.23rc2/src/runtime/panic.go:1067 +0x38 fp=0x14000108dc0 sp=0x14000108d90 pc=0x104a36208
runtime.checkptrArithmetic(0x104a6abaf?, {0x0, 0x0, 0x104a6abaf?})
    /Users/xxx/sdk/go1.23rc2/src/runtime/checkptr.go:69 +0xa8 fp=0x14000108df0 sp=0x14000108dc0 pc=0x1049dd308
internal/abi.NoEscape(...)
    /Users/xxx/sdk/go1.23rc2/src/internal/abi/escape.go:21
internal/concurrent.(*HashTrieMap[...]).Load(0x104a7d6c0, 0x104a6ab80)
    /Users/xxx/sdk/go1.23rc2/src/internal/concurrent/hashtriemap.go:49 +0x38 fp=0x14000108e40 sp=0x14000108df0 pc=0x104a41468
unique.Make[...]({0x0, 0x0})
    /Users/xxx/sdk/go1.23rc2/src/unique/handle.go:35 +0x98 fp=0x14000108f10 sp=0x14000108e40 pc=0x104a43088
main.main()
    /Users/xxx/Projects/guance/local-dev/cmd/checkptr/main.go:6 +0x2c fp=0x14000108f40 sp=0x14000108f10 pc=0x104a41b4c
runtime.main()
    /Users/xxx/sdk/go1.23rc2/src/runtime/proc.go:272 +0x288 fp=0x14000108fd0 sp=0x14000108f40 pc=0x104a08858
runtime.goexit({})
    /Users/xxx/sdk/go1.23rc2/src/runtime/asm_arm64.s:1223 +0x4 fp=0x14000108fd0 sp=0x14000108fd0 pc=0x104a3c804

What did you expect to see?

Normal exit.

Comment From: gabyhelp

Related Issues and Documentation

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

Comment From: randall77

Most likely a dup of #68415. Can you try the fix for that issue and see if it fixes your example?

Comment From: cherrymui

We probably want to disable a nocheckptr function (including runtime functions) into a checkptr function. I think we already do something similar for norace.

Comment From: callthingsoff

Most likely a dup of #68415. Can you try the fix for that issue and see if it fixes your example?

Replacing with the wrapper function, it passed the run.

diff --git a/src/internal/concurrent/hashtriemap.go b/src/internal/concurrent/hashtriemap.go
index 4f7e730d4f..b44c25eb70 100644
--- a/src/internal/concurrent/hashtriemap.go
+++ b/src/internal/concurrent/hashtriemap.go
@@ -46,7 +46,7 @@ type equalFunc func(unsafe.Pointer, unsafe.Pointer) bool
 // value is present.
 // The ok result indicates whether value was found in the map.
 func (ht *HashTrieMap[K, V]) Load(key K) (value V, ok bool) {
-   hash := ht.keyHash(abi.NoEscape(unsafe.Pointer(&key)), ht.seed)
+   hash := ht.keyHash(noescape(unsafe.Pointer(&key)), ht.seed)

    i := ht.root
    hashShift := 8 * goarch.PtrSize
@@ -69,7 +69,7 @@ func (ht *HashTrieMap[K, V]) Load(key K) (value V, ok bool) {
 // Otherwise, it stores and returns the given value.
 // The loaded result is true if the value was loaded, false if stored.
 func (ht *HashTrieMap[K, V]) LoadOrStore(key K, value V) (result V, loaded bool) {
-   hash := ht.keyHash(abi.NoEscape(unsafe.Pointer(&key)), ht.seed)
+   hash := ht.keyHash(noescape(unsafe.Pointer(&key)), ht.seed)
    var i *indirect[K, V]
    var hashShift uint
    var slot *atomic.Pointer[node[K, V]]
@@ -182,7 +182,7 @@ func (ht *HashTrieMap[K, V]) expand(oldEntry, newEntry *entry[K, V], newHash uin
 // If there is no current value for key in the map, CompareAndDelete returns false
 // (even if the old value is the nil interface value).
 func (ht *HashTrieMap[K, V]) CompareAndDelete(key K, old V) (deleted bool) {
-   hash := ht.keyHash(abi.NoEscape(unsafe.Pointer(&key)), ht.seed)
+   hash := ht.keyHash(noescape(unsafe.Pointer(&key)), ht.seed)
    var i *indirect[K, V]
    var hashShift uint
    var slot *atomic.Pointer[node[K, V]]
@@ -355,7 +355,7 @@ func newEntryNode[K, V comparable](key K, value V) *entry[K, V] {

 func (e *entry[K, V]) lookup(key K, equal equalFunc) (V, bool) {
    for e != nil {
-       if equal(unsafe.Pointer(&e.key), abi.NoEscape(unsafe.Pointer(&key))) {
+       if equal(unsafe.Pointer(&e.key), noescape(unsafe.Pointer(&key))) {
            return e.value, true
        }
        e = e.overflow.Load()
@@ -368,16 +368,16 @@ func (e *entry[K, V]) lookup(key K, equal equalFunc) (V, bool) {
 //
 // compareAndDelete must be called under the mutex of the indirect node which e is a child of.
 func (head *entry[K, V]) compareAndDelete(key K, value V, keyEqual, valEqual equalFunc) (*entry[K, V], bool) {
-   if keyEqual(unsafe.Pointer(&head.key), abi.NoEscape(unsafe.Pointer(&key))) &&
-       valEqual(unsafe.Pointer(&head.value), abi.NoEscape(unsafe.Pointer(&value))) {
+   if keyEqual(unsafe.Pointer(&head.key), noescape(unsafe.Pointer(&key))) &&
+       valEqual(unsafe.Pointer(&head.value), noescape(unsafe.Pointer(&value))) {
        // Drop the head of the list.
        return head.overflow.Load(), true
    }
    i := &head.overflow
    e := i.Load()
    for e != nil {
-       if keyEqual(unsafe.Pointer(&e.key), abi.NoEscape(unsafe.Pointer(&key))) &&
-           valEqual(unsafe.Pointer(&e.value), abi.NoEscape(unsafe.Pointer(&value))) {
+       if keyEqual(unsafe.Pointer(&e.key), noescape(unsafe.Pointer(&key))) &&
+           valEqual(unsafe.Pointer(&e.value), noescape(unsafe.Pointer(&value))) {
            i.Store(e.overflow.Load())
            return head, true
        }
@@ -387,6 +387,18 @@ func (head *entry[K, V]) compareAndDelete(key K, value V, keyEqual, valEqual equ
    return head, false
 }

+// This is just a wrapper around abi.NoEscape.
+//
+// This wrapper is necessary because internal/abi is a runtime package,
+// so it can not be built with -d=checkptr, causing incorrect inlining
+// decision when building with checkptr enabled, see issue #68511.
+//
+//go:nosplit
+//go:nocheckptr
+func noescape(p unsafe.Pointer) unsafe.Pointer {
+   return abi.NoEscape(p)
+}
+
 // node is the header for a node. It's polymorphic and
 // is actually either an entry or an indirect.
 type node[K, V comparable] struct {
diff --git a/test/fixedbugs/issue68511.go b/test/fixedbugs/issue68511.go
new file mode 100644
index 0000000000..9ef090ecb9
--- /dev/null
+++ b/test/fixedbugs/issue68511.go
@@ -0,0 +1,13 @@
+// run -gcflags=all=-d=checkptr
+
+// Copyright 2024 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package main
+
+import "unique"
+
+func main() {
+   unique.Make("")
+}

Comment From: randall77

Ok, then this should be fixed in the next rc or the final release.

Comment From: gopherbot

Change https://go.dev/cl/599356 mentions this issue: internal/concurrent: introduce noescape wrapper function