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

[Dan Liew via Phabricator via llvm-commits]

delcypher created this revision.
delcypher added reviewers: kubamracek, yln, kcc, dvyukov, eugenis, vitalybuka, cryptoad, earthdok.
Herald added a subscriber: jfb.
delcypher requested review of this revision.
Herald added a project: Sanitizers.
Herald added a subscriber: Sanitizers.

Previously in ASan's `pthread_create` interceptor we would block in the
`pthread_create` interceptor waiting for the child thread to start.

Unfortunately this has bad performance characteristics because the OS
scheduler doesn't know the relationship between the parent and child
thread (i.e. the parent thread cannot make progress until the child
thread makes progress) and may make the wrong scheduling decision which
stalls progress.

It turns out that ASan didn't use to block in this interceptor but was
changed to do so to try to address
http://llvm.org/bugs/show_bug.cgi?id=21621/.

In that bug the problem being addressed was a LeakSanitizer false
positive. That bug concerns a heap object being passed
as `arg` to `pthread_create`. If:

- The calling thread loses a live reference to the object (e.g. `pthread_create` finishes and the thread no longer has a live reference to the object).
- Leak checking is triggered.
- The child thread has not yet started (once it starts it will have a live reference).

then the heap object will incorrectly appear to be leaked.

This bug is covered by the `lsan/TestCases/leak_check_before_thread_started.cpp` test case.

In b029c5101fb49b3577a1c322f42ef9fc616f25bf ASan was changed to block
in `pthread_create()` until the child thread starts so that `arg` is
kept alive for the purposes of leaking check.

While this change "works" its problematic due to the performance
problems it causes. The change is also completely unnecessary if leak
checking is disabled (via detect_leaks runtime option or
CAN_SANITIZE_LEAKS compile time config).

This patch takes a different approach to solving the leak false positive
telling LSan to ignore the `arg` object in between the time the child
thread is created and started. Because the object is being ignored its
no longer necessary to wait for the child thread to start and so the
blocking behaviour can be removed.

In more detail:

1. Change `pthread_create` interceptor and `asan_thread_start` to no longer have a blocking relationship. This is similar to reverting b029c5101fb49b3577a1c322f42ef9fc616f25bf except we
  - Keep the test case added.
  - Leave the `__lsan_thread_start_func` changes in place because we haven't changed the blocking behaviour in standalone LSan. We could but I think its best we start with making this change to ASan first.
  - Adapt to source code changes made since the patch was landed.

2. In the `pthread_create` interceptor call `__lsan::IgnoreObjectLocked(arg)` to cause the object to be ignored for the purposes of leak detection. This is only done if leak checking is enabled. The interceptor then calls a new method (`set_unignore_arg_on_start`) to tell the AsanThread that it should unignore the object when it starts.

3. In `AsanThread::ThreadStart` call `__lsan::UnIgnoredObjectLocked(arg)` if `unignore_arg_on_start_` is true.

rdar://problem/63537240


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D94210

Files:
  compiler-rt/lib/asan/asan_interceptors.cpp
  compiler-rt/lib/asan/asan_thread.cpp
  compiler-rt/lib/asan/asan_thread.h

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D94210.315033.patch
Type: text/x-patch
Size: 5629 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210107/9e38a95f/attachment.bin>