[PATCH] D95184: [ASan] Stop blocking child thread progress from parent thread in `pthread_create` interceptor.

[Dan Liew via Phabricator via llvm-commits]

delcypher marked an inline comment as done.
delcypher added inline comments.


================
Comment at: compiler-rt/lib/asan/asan_allocator.cpp:1197
+      reinterpret_cast<__asan::AsanThreadContext *>(tctx);
+  __asan::AsanThread *asanThread = atctx->thread;
+
----------------
vitalybuka wrote:
> historically sanitizers use snake case for local variable, from Google coding style
Okay. I'll change the style.


================
Comment at: compiler-rt/lib/asan/asan_allocator.cpp:1202-1204
+  if (!(atctx->status == ThreadStatusCreated ||
+        atctx->status == ThreadStatusRunning))
+    return;
----------------
vitalybuka wrote:
>   if (atctx->status != ThreadStatusCreated && atctx->status != ThreadStatusRunning)
>     return;
> 
Sure. I'll rewrite it that way.


================
Comment at: compiler-rt/lib/asan/asan_interceptors.cpp:228
+    // Note AsanThreadContexts never get destroyed so that still leaks.
+    t->Destroy();
   }
----------------
vitalybuka wrote:
> delcypher wrote:
> > @vitalybuka **Is `AsanThread::Destroy()` safe to call from this thread and in this context where the corresponding thread was never started.?** I wasn't really sure. 
> > 
> > I see that calling this will call `DTLS_Destroy()` and I'm not sure if that needs to be called from a different thread (i.e. called from the thread before its destroyed). It looks like this function is a no-op on macOS which is where I'm testing.
> > 
> > 
> I think DTLS_Destroy should be fine here. I assume this thread will never showup in suspended_threads so lsan will not try to scan it.
> 
> As I understand Ctx is not leaked but pushed into quarantine by ThreadRegistry::FinishThread
Sorry "leaks" is a poor choice of words. What I meant is we've wasted an AsanThreadContext because created it but then never used it. Because in ASan we never re-use AsanThreadContexts its wasted.

I'll update the comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D95184