[PATCH] D54889: Fiber support for thread sanitizer

[Dmitry Vyukov via Phabricator via llvm-commits]

dvyukov added inline comments.


================
Comment at: lib/tsan/rtl/tsan_interface.cc:141
+  ThreadState *fiber = static_cast<ThreadState *>(mem);
+  internal_memset(fiber, 0, sizeof(*fiber));
+  int tid = ThreadCreate(thr, pc, 0, true);
----------------
Bulk of this logic should go into tsan_rtl_thread.cc
This file is not meant to contain any significant logic, it's only interface part.
Please add a set of runtime functions, following current naming convention (e.g. FiberCreate FiberSwitch, etc), and forward to these functions.



================
Comment at: lib/tsan/rtl/tsan_rtl.h:472
 INLINE ThreadState *cur_thread() {
-  return reinterpret_cast<ThreadState *>(&cur_thread_placeholder);
+  return reinterpret_cast<ThreadState *>(cur_thread_placeholder.buf +
+                                         cur_thread_placeholder.cur);
----------------
This looks even more expensive then before.
Previously we had just 1 pointer indirection. Now we also add a base to it, so it's another load and another add.


================
Comment at: lib/tsan/rtl/tsan_rtl_thread.cc:249
 #if !SANITIZER_GO
-  GetThreadStackAndTls(tid == 0, &stk_addr, &stk_size, &tls_addr, &tls_size);
+  if (os_id)
+    GetThreadStackAndTls(tid == 0, &stk_addr, &stk_size, &tls_addr, &tls_size);
----------------
Let's not abuse an existing variable for this. I am not sure no OS passes 0 here today, or in future somebody will decide to pass 0 as they don't have anything meaningful to pass as os_id. Let's add another flag for this.


================
Comment at: test/tsan/fiber_asm.cc:62
+void ucontext_switch(ucontext *save, ucontext *load) {
+  save->fiber = __tsan_get_current_fiber();
+  __tsan_switch_to_fiber(load->fiber, 0);
----------------
We save the current fiber, which is the main thread in this test. But if we ever try to switch to it on another thread, everything will explode in a bad way, right?


================
Comment at: test/tsan/fiber_simple.cc:17
+int main() {
+  orig_fiber = __tsan_get_current_fiber();
+  fiber = __tsan_create_fiber(0);
----------------
If we move this 1 line below, we will get a race on orig_fiber, right? If yes, this is another argument towards synchronizing all fiber switches.


Repository:
  rCRT Compiler Runtime

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D54889/new/

https://reviews.llvm.org/D54889