| The point of enumerating all of those specific issues, wasn't to say "here are some bugs" and if you fix them you're good. It was to say "here are some examples of the much more fundamental problem", which seemed to be a fundamental misunderstanding of the language memory model and the guarantees offered by assignments, atomic.CompareAndSwap, etc., and those operations' interactions with package unsafe. For example this code https://github.com/sirgallo/cmapv2/blob/6bcaa0253b1b0b261e8a... and in particular its use of this code https://github.com/sirgallo/cmapv2/blob/6bcaa0253b1b0b261e8a... is still completely unsound. Looking at *only this code path* -- and there are *many more* -- --- Put - Snapshots the current root pointer with atomic.LoadPointer - Makes an updated root pointer via putRecursive, given the snapshotted root pointer - Spins on a CAS of the root ptr and the updated ptr with runtime.Gosched() between attempts --- atomic.LoadPointer isn't a real snapshot - It's atomic only over the root ptr, not any interior field - Those interior fields are mutated in-place via e.g. setBitmap, setChild, etc. - Any goroutine can see partial data, violating the memory model, etc. --- putRecursive is unsound - copyNode performs a shallow copy, child pointers are shared, subsequent setChild, extendTable, etc. mutate nodes other goroutines can still hold -- this is a fundamental bug that seems to remain un-addressed from the previous review - Those mutations use plain writes (no atomics/locks/etc.) -- data race, memory model violation, etc. - Get later returns the internal []byte slice directly -- data race, memory model violation, etc. - Newly created nodes are cast to unsafe.Pointer without an atomic store, bypassing the write barrier required by the GC --- That compareAndSwap is unsound - It compares only the root pointer, a shallow copy - After a successful CAS other writers can still mutate any shared children (see above), so readers following any shared path see data races, memory model violations, etc. - The retry loop can livelock, details elided --- The implementation still seems to confuse "atomic pointer swap" with "atomic update of a complex, shared value", misunderstands the requirements of the Go memory model, and consistently mis-uses unsafe.Pointer. tl;dr here is probably to just stop using package unsafe altogether, until you have some time to properly understand its semantics, requirements, and limitations... |