3 ms·
Just looking at this initially, thread_safe_var_init is not correct. There is no guarantee whatsoever that "*vpp = vp" actually does anything, the compiler is p
by slaymaker1907 3y ago
Just looking at this initially, thread_safe_var_init is not correct. There is no guarantee whatsoever that "*vpp = vp" actually does anything, the compiler is perfectly free to just never allocate vp and just use the memory at vpp (thus allowing partial initialization) and the situation is even worse on ARM. You need some sort of memory barrier between that last assignment and it should probably be a volatile/std::atomic pointer.
- cryptonector 3y agoHmmm, good point, the init function does need a memory barrier. Thanks for the review!
- cryptonector 3y agoI fixed that. TSAN also found a pair of data races involving assert()s (oof). All three are fixed.
- cryptonector 3y agoI've added use of Helgrind's `ANNOTATE_HAPPENS_BEFORE()` and `ANNOTATE_HAPPENS_AFTER()` macros to the functions in `atomics.c` and now this is Helgrind-clean as well as TSAN-clean. The only bugs found by TSAN (as noted in another reply) were the one you found and two assert()s that didn't use atomics. Those two asserts could be a big problem if one ran a non-NDEBUG build in production. So now this is TSAN- and Helgrind-clean. Thanks for prompting me to do this extra work!