[PATCH] D51064: [tsan] Adjust setjmp/longjmp handling on Darwin for macOS Mojave

[Kuba (Brecka) Mracek via Phabricator via llvm-commits]

kubamracek added inline comments.


================
Comment at: lib/tsan/rtl/tsan_platform_mac.cc:260
+    __tsan_darwin_setjmp_xor_key =
+        (uptr)pthread_getspecific(kPthreadSetjmpXorKeySlot);
+  }
----------------
delcypher wrote:
> This seems slightly odd. Using `pthread_getspecific` suggests the value you're requested could be different between threads. However the value you read here is global is being written to a global in the TSan runtime and thus will be shared by all threads.
> 
> At this point during init there is only one thread so there's only one value you can retrieve this early on but I do wonder if you'd get a different value on another thread. Perhaps this key should be stored per thread instead and initialized on thread creation?
> 
> If we expect the key to not change between threads perhaps we should add a `CHECK_EQ()` call that on thread creation checks that the key retrieved for the thread matches the global value stored during init?
You're right, this is a bit weird, but I've confirmed with the Libc folks that currently, the value is process-global and doesn't change between threads. This is something that can potentially change in the future.

Adding the check sounds reasonable, I'll try to do that.


================
Comment at: lib/tsan/rtl/tsan_platform_mac.cc:261
+        (uptr)pthread_getspecific(kPthreadSetjmpXorKeySlot);
+  }
 }
----------------
delcypher wrote:
> On the else branch here we could add `CHECK_EQ(__tsan_darwin_setjmp_xor_key, 0);` because we require this value for the key so that the xor operation on older OS versions is a no-op.
Hm, that seems superfluous. `__tsan_darwin_setjmp_xor_key` is initialized to zero a couple lines above.


================
Comment at: lib/tsan/rtl/tsan_rtl_amd64.S:199
   mov %rdi, %rsi
+  xorq ___tsan_darwin_setjmp_xor_key(%rip), %rsi
 #elif defined(__linux__)
----------------
delcypher wrote:
> Could you jog my memory. Why is `%rip` involved in the xor calculation? `___tsan_darwin_setjmp_xor_key` is being used as an offset here so presumably the offset is instruction relative?
Sure, this is to emit `%rip`-relative reference to `___tsan_darwin_setjmp_xor_key`. Position-independent code requires that, IMHO.


Repository:
  rL LLVM

https://reviews.llvm.org/D51064