[PATCH] D45646: [tsan] Zero out the shadow memory for the stack and TLS in ThreadFinish

[Dmitry Vyukov via Phabricator via llvm-commits]

dvyukov added a comment.

> I'm not too worried about the first variation. It's very unlikely that user code will try to mmap(MAP_FIXED)

Are you sure that the first variation requires MAP_FIXED? On linux all mmaps go to the same memory range (0x7ff...) and as far as I remember it was the case for mac too. Maybe you tested only gcd threads or pthread threads? But stacks for the other one are allocated from normal mmap range?

> and the thread destruction event is not sent "late enough", i.e. interceptors get called after that event.

This means that we have another bug. Late interceptors will recreate thread descriptor, right? Which means that than a new thread will reuse this descriptor and this can lead to both false negatives (as these 2 threads are now effectivetely a single thread tsan point of view) and false positives (as for the second thread we does not synchronize it with thread creation point).

> Is moving the backing store of our "fake TLS" to the metamap going to help?

There are other good reasons to clean meta shadow. If we don't clean it, we leak sync objects and/or induce false negatives (as new sync objects created at the same address reuse the old one). We already pay the cost of cleaning it in munmap. And we should clean it on thread destruction too.
So if we use meta map, then we don't pay double price for cleaning both shadows and any bug fixes for mac will also help all other OSes, and vise versa -- any fixes that clean run away meta shadow to prevent sync object leaks will also help mac.
If we use normal shadow, then we pay double price, and you are on your own with long tail of cases where we forgot to clean normal shadow.

> Doesn't that cause some problems already?

No. I don't think there is anything that requires it now.

But both normal shadow and meta shadow does not solve problem with early thread destruction callback...


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D45646