internal/concurrent: hashtriemap uses abi.NoEscape cause incorrect inlining decision #68511
Comments
|
Most likely a dup of #68415. Can you try the fix for that issue and see if it fixes your example? |
|
We probably want to disable a nocheckptr function (including runtime functions) into a checkptr function. I think we already do something similar for norace. |
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("")
+}
|
|
Ok, then this should be fixed in the next rc or the final release. |
|
Change https://go.dev/cl/599356 mentions this issue: |
|
Change https://go.dev/cl/599435 mentions this issue: |
gopherbot
pushed a commit
that referenced
this issue
Jul 22, 2024
Runtime functions, e.g. internal/abi.NoEscape, should not be instrumented with checkptr. But if they are inlined into a checkptr-enabled function, they will be instrumented, and may result in a check failure. Let the compiler not inline runtime functions into checkptr- enabled functions. Also undo the change in the strings package in CL 598295, as the compiler handles it now. Fixes #68511. Updates #68415. Change-Id: I78eb380855ac9dd53c1a1a628ec0da75c3e5a1a0 Reviewed-on: https://go-review.googlesource.com/c/go/+/599435 LUCI-TryBot-Result: Go LUCI <[email protected]> Reviewed-by: Keith Randall <[email protected]> Reviewed-by: Cuong Manh Le <[email protected]> Reviewed-by: Keith Randall <[email protected]>
dmitshur
added
the
NeedsFix
This was referenced Sep 26, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Go version
go version go1.23rc2 darwin/arm64
Output of
go envin your module/workspace: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=checkptrflag:What did you see happen?
What did you expect to see?
Normal exit.
The text was updated successfully, but these errors were encountered: