False positive on atomic write vs free

[dvyukov]

ThreadSanitizer currently reports a data race on the following program:

void *Thread(void *a) {
  __atomic_store_n((int*)a, 1, __ATOMIC_RELAXED);
  return 0;
}

int main() {
  int *a = new int(0);
  pthread_t t;
  pthread_create(&t, 0, Thread, a);
  while (__atomic_load_n(a, __ATOMIC_RELAXED) == 0)
    pthread_yield();
  delete a;
  pthread_join(t, 0);
}

WARNING: ThreadSanitizer: data race (pid=49350)
  Write of size 8 at 0x7d040000f7f0 by main thread:
    #0 operator delete(void*) lib/tsan/rtl/tsan_new_delete.cc:69
    #1 main test/tsan/./atomic_free3.cc:15:3

  Previous atomic write of size 4 at 0x7d040000f7f0 by thread T1:
    #0 __tsan_atomic32_store lib/tsan/rtl/tsan_interface_atomic.cc:560
    #1 Thread(void*) test/tsan/./atomic_free3.cc:5:3

This is false positive.
However, nobody ever complained and fixing it will lead to false negatives for races that actually happen.

[dvyukov]

On second though, it can actually be a race according to C/C++ memory model. delete a can read/write the object and delete is not synchronized with the atomic write.

Test is added in revision 248522.

[dvyukov]

Not a bug.

[conversy]

came across this ^^
This comment for other people trying to fix such a data race: swapping 'delete' and 'join' fixes the data race, supposedly because there is now a synchronization (join), before the deletion. I said 'supposedly' because I do not consider myself as a specialist, take this with a grain of salt.

[dvyukov]

An update on this: this is now considered a defect in the standard. If the proposal is accepted, there is no race and TSAN shouldn't produce a race report.

On first glance precise modelling is problematic. TSAN is all based on vector clocks and transitive HB. Everything is timestamped with per-thread clock and then checked against vector clocks.

If we make the store HB the load, then it may cause massive false negatives, since now everything in the first HB the load (effectively promote all relaxed store/load to release/acquire). This is probably worse overall.

Precise modelling will require some new per-pair-of-thread/per-location relation, which looks extremely expensive to maintain and does not worth it for this obscure corner case.

A possible practical solution may be to wipe the store after the value was loaded (pretend the store did not happen anymore), or make it appear to happen at a much earlier point in time (at the time that HB the load). This should prevent this particular FP w/o introducing massive side effects.