Repository: puzpuzpuz/xsync
Pull request #47 (2022-08-12 18:44:04)

Fix MapOf struct value pointer uniqueness
Also fixes nil value uniqueness in Map

codecov-commenter: # [Codecov](https://codecov.io/gh/puzpuzpuz/xsync/pull/47?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Andrey+Pechkurov) Report
> Merging [#47](https://codecov.io/gh/puzpuzpuz/xsync/pull/47?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Andrey+Pechkurov) (030e421) into [main](https://codecov.io/gh/puzpuzpuz/xsync/commit/9dba66eac2b45a5b5f193323573df0722290b858?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Andrey+Pechkurov) (9dba66e) will **decrease** coverage by `3.43%`.
> The diff coverage is `69.23%`.

```diff
@@            Coverage Diff             @@
##             main      #47      +/-   ##
==========================================
- Coverage   94.32%   90.89%   -3.44%     
==========================================
  Files           6        6              
  Lines         723      725       +2     
==========================================
- Hits          682      659      -23     
- Misses         34       52      +18     
- Partials        7       14       +7     
```


| [Impacted Files](https://codecov.io/gh/puzpuzpuz/xsync/pull/47?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Andrey+Pechkurov) | Coverage Δ | |
|---|---|---|
| [util.go](https://codecov.io/gh/puzpuzpuz/xsync/pull/47/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Andrey+Pechkurov#diff-dXRpbC5nbw==) | `100.00% <ø> (ø)` | |
| [map.go](https://codecov.io/gh/puzpuzpuz/xsync/pull/47/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Andrey+Pechkurov#diff-bWFwLmdv) | `89.76% <60.00%> (-2.99%)` | :arrow_down: |
| [mapof.go](https://codecov.io/gh/puzpuzpuz/xsync/pull/47/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Andrey+Pechkurov#diff-bWFwb2YuZ28=) | `87.76% <75.00%> (-6.21%)` | :arrow_down: |

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Andrey+Pechkurov). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Andrey+Pechkurov)


Patch:
diff --git a/map.go b/map.go
index a2c2074..f75fea4 100644
--- a/map.go
+++ b/map.go
@@ -87,11 +87,6 @@ type rangeEntry struct {
 	value unsafe.Pointer
 }
 
-// special type to mark nil values
-type nilValue struct{}
-
-var nilVal = new(nilValue)
-
 // NewMap creates a new Map instance.
 func NewMap() *Map {
 	m := &Map{}
@@ -169,9 +164,6 @@ func (m *Map) LoadAndStore(key string, value interface{}) (actual interface{}, l
 }
 
 func (m *Map) doStore(key string, value interface{}, loadIfExists bool) (interface{}, bool) {
-	if value == nil {
-		value = nilVal
-	}
 	// Read-only path.
 	if loadIfExists {
 		if v, ok := m.Load(key); ok {
@@ -222,7 +214,11 @@ func (m *Map) doStore(key string, value interface{}, loadIfExists bool) (interfa
 				// interface{} on each call, thus the live value pointers are
 				// unique. Otherwise atomic snapshot won't be correct in case
 				// of multiple Store calls using the same value.
-				atomic.StorePointer(&b.values[i], unsafe.Pointer(&value))
+				nvp := unsafe.Pointer(&value)
+				if assertionsEnabled && vp == nvp {
+					panic("non-unique value pointer")
+				}
+				atomic.StorePointer(&b.values[i], nvp)
 				b.mu.Unlock()
 				return derefValue(vp), true
 			}
@@ -477,11 +473,7 @@ func derefKey(keyPtr unsafe.Pointer) string {
 }
 
 func derefValue(valuePtr unsafe.Pointer) interface{} {
-	value := *(*interface{})(valuePtr)
-	if _, ok := value.(*nilValue); ok {
-		return nil
-	}
-	return value
+	return *(*interface{})(valuePtr)
 }
 
 func bucketIdx(table *mapTable, hash uint64) uint64 {
diff --git a/mapof.go b/mapof.go
index dfa58b6..6427f3f 100644
--- a/mapof.go
+++ b/mapof.go
@@ -146,11 +146,16 @@ func (m *MapOf[V]) doStore(key string, value V, loadIfExists bool) (V, bool) {
 					b.mu.Unlock()
 					return derefTypedValue[V](vp), true
 				}
-				// In-place update case. Luckily we get a copy of the value
+				// In-place update case. We get a copy of the value via an
 				// interface{} on each call, thus the live value pointers are
 				// unique. Otherwise atomic snapshot won't be correct in case
 				// of multiple Store calls using the same value.
-				atomic.StorePointer(&b.values[i], unsafe.Pointer(&value))
+				var wv interface{} = value
+				nvp := unsafe.Pointer(&wv)
+				if assertionsEnabled && vp == nvp {
+					panic("non-unique value pointer")
+				}
+				atomic.StorePointer(&b.values[i], nvp)
 				b.mu.Unlock()
 				return derefTypedValue[V](vp), true
 			}
@@ -159,7 +164,8 @@ func (m *MapOf[V]) doStore(key string, value V, loadIfExists bool) (V, bool) {
 			// Insertion case. First we update the value, then the key.
 			// This is important for atomic snapshot states.
 			atomic.StoreUint64(&b.topHashes, storeTopHash(hash, b.topHashes, emptyidx))
-			atomic.StorePointer(emptyvp, unsafe.Pointer(&value))
+			var wv interface{} = value
+			atomic.StorePointer(emptyvp, unsafe.Pointer(&wv))
 			atomic.StorePointer(emptykp, unsafe.Pointer(&key))
 			b.mu.Unlock()
 			addSize(table, bidx, 1)
@@ -336,7 +342,7 @@ func (m *MapOf[V]) Size() int {
 }
 
 func derefTypedValue[V any](valuePtr unsafe.Pointer) (val V) {
-	return *(*V)(valuePtr)
+	return (*(*interface{})(valuePtr)).(V)
 }
 
 // O(N) operation; use for debug purposes only
diff --git a/util.go b/util.go
index 693ff73..3145262 100644
--- a/util.go
+++ b/util.go
@@ -5,6 +5,9 @@ import (
 	"unsafe"
 )
 
+// test-only assert()-like flag
+var assertionsEnabled = false
+
 const (
 	// used in paddings to prevent false sharing;
 	// 64B are used instead of 128B as a compromise between


Test patch:
diff --git a/export_test.go b/export_test.go
index 6bfcc82..ebd63c9 100644
--- a/export_test.go
+++ b/export_test.go
@@ -29,3 +29,11 @@ func StoreTopHash(hash, topHashes uint64, idx int) uint64 {
 func EraseTopHash(topHashes uint64, idx int) uint64 {
 	return eraseTopHash(topHashes, idx)
 }
+
+func EnableAssertions() {
+	assertionsEnabled = true
+}
+
+func DisableAssertions() {
+	assertionsEnabled = false
+}
diff --git a/map_test.go b/map_test.go
index 30e7ca8..4195936 100644
--- a/map_test.go
+++ b/map_test.go
@@ -50,6 +50,61 @@ func TestMap_BucketStructSize(t *testing.T) {
 	}
 }
 
+func TestMap_UniqueValuePointers_Int(t *testing.T) {
+	EnableAssertions()
+	m := NewMap()
+	v := 42
+	m.Store("foo", v)
+	m.Store("foo", v)
+	DisableAssertions()
+}
+
+func TestMap_UniqueValuePointers_Struct(t *testing.T) {
+	type foo struct{}
+	EnableAssertions()
+	m := NewMap()
+	v := foo{}
+	m.Store("foo", v)
+	m.Store("foo", v)
+	DisableAssertions()
+}
+
+func TestMap_UniqueValuePointers_Pointer(t *testing.T) {
+	type foo struct{}
+	EnableAssertions()
+	m := NewMap()
+	v := &foo{}
+	m.Store("foo", v)
+	m.Store("foo", v)
+	DisableAssertions()
+}
+
+func TestMap_UniqueValuePointers_Slice(t *testing.T) {
+	EnableAssertions()
+	m := NewMap()
+	v := make([]int, 13)
+	m.Store("foo", v)
+	m.Store("foo", v)
+	DisableAssertions()
+}
+
+func TestMap_UniqueValuePointers_String(t *testing.T) {
+	EnableAssertions()
+	m := NewMap()
+	v := "bar"
+	m.Store("foo", v)
+	m.Store("foo", v)
+	DisableAssertions()
+}
+
+func TestMap_UniqueValuePointers_Nil(t *testing.T) {
+	EnableAssertions()
+	m := NewMap()
+	m.Store("foo", nil)
+	m.Store("foo", nil)
+	DisableAssertions()
+}
+
 func TestMap_MissingEntry(t *testing.T) {
 	m := NewMap()
 	v, ok := m.Load("foo")
diff --git a/mapof_test.go b/mapof_test.go
index 86f2d9a..2d3b9ed 100644
--- a/mapof_test.go
+++ b/mapof_test.go
@@ -12,6 +12,61 @@ import (
 	. "github.com/puzpuzpuz/xsync"
 )
 
+func TestMapOf_UniqueValuePointers_Int(t *testing.T) {
+	EnableAssertions()
+	m := NewMapOf[int]()
+	v := 42
+	m.Store("foo", v)
+	m.Store("foo", v)
+	DisableAssertions()
+}
+
+func TestMapOf_UniqueValuePointers_Struct(t *testing.T) {
+	type foo struct{}
+	EnableAssertions()
+	m := NewMapOf[foo]()
+	v := foo{}
+	m.Store("foo", v)
+	m.Store("foo", v)
+	DisableAssertions()
+}
+
+func TestMapOf_UniqueValuePointers_Pointer(t *testing.T) {
+	type foo struct{}
+	EnableAssertions()
+	m := NewMapOf[*foo]()
+	v := &foo{}
+	m.Store("foo", v)
+	m.Store("foo", v)
+	DisableAssertions()
+}
+
+func TestMapOf_UniqueValuePointers_Slice(t *testing.T) {
+	EnableAssertions()
+	m := NewMapOf[[]int]()
+	v := make([]int, 13)
+	m.Store("foo", v)
+	m.Store("foo", v)
+	DisableAssertions()
+}
+
+func TestMapOf_UniqueValuePointers_String(t *testing.T) {
+	EnableAssertions()
+	m := NewMapOf[string]()
+	v := "bar"
+	m.Store("foo", v)
+	m.Store("foo", v)
+	DisableAssertions()
+}
+
+func TestMapOf_UniqueValuePointers_Nil(t *testing.T) {
+	EnableAssertions()
+	m := NewMapOf[*struct{}]()
+	m.Store("foo", nil)
+	m.Store("foo", nil)
+	DisableAssertions()
+}
+
 func TestMapOf_MissingEntry(t *testing.T) {
 	m := NewMapOf[string]()
 	v, ok := m.Load("foo")