In D58110#1408455, @dvyukov wrote:

Kuba, please submit this. I can't test/build this.

In D58110#1405299, @yuri wrote:

If everything is OK, can you commit this patch?

If everything is OK, can you commit this patch?

Disable tests on tvOS and watchOS

In D58110#1401436, @kubamracek wrote:

I think the tests are still going to fail on watchOS and tvOS, they need to be marked as UNSUPPORTED

Do not include <ucontext.h> and declare getcontext/setcontext manually

Added

#if !SANITIZER_WATCHOS && !SANITIZER_TVOS

around swapcontext interceptor

In D58110#1398292, @kubamracek wrote:

@yln Can you please try to build this for watchOS/tvOS?

In D58110#1396514, @kubamracek wrote:

For the pragma diagnostic push/ignore stuff, we might be better off just redeclaring the function ourselves without including ucontext.h.

Cleanup tests

In D54889#1396430, @dvyukov wrote:

I see a similar failure was already reported:
https://reviews.llvm.org/D54889#1394632
There was no fix for it, right? Or did I forgot to squash something else?

In D54889#1396412, @dvyukov wrote:

• Forwarded message --------- From: Yi-Hong Lyu via Phabricator <reviews@reviews.llvm.org> Date: Wed, Feb 13, 2019 at 5:33 PM Subject: [Diffusion] rL353817: tsan: add fiber support
  
  Yi-Hong.Lyu added a comment. This patch seems cause failure of buildbot http://lab.llvm.org:8011/builders/clang-ppc64le-linux-multistage/builds/9150

Patch with context

In D54889#1395985, @rovka wrote:

Hi Yuri,

I think this might be breaking our aarch64-full buildbot [1]. I ran compiler-rt/test/sanitizer_common/tsan-aarch64-Linux/Linux/Output/clock_gettime.c.tmp and get this stack trace [2]:

Any ideas?

@lei, please help to investigate what happened in http://lab.llvm.org:8011/builders/sanitizer-ppc64be-linux/builds/11413

Final patch (macOS support): D58110

Add more calls of cur_thread_init()

Patch with in_symbolizer() function: D58104

In D54889#1392721, @dvyukov wrote:

However, for thread initialization we rely on the fact that pthread's thread entry function (start_thread) calls _setjmp which we intercept. If start_thread does not call _setjmp then we risk to hit NULL cur_thread in secondary threads. I think we need to add cur_thread_init also to:

1. SCOPED_INTERCEPTOR_RAW (this will ensure that if start_thread calls any intercepted libc function, we will init at that point)

In D57876#1392414, @dvyukov wrote:

I've seen some fixes has landed over the weekend. Is this fixed now? Or some breakages still remain?

Rebased on current trunk

In D57876#1391757, @vitalybuka wrote:

It still crashes even without interceptor

Fix for macOS is in D57963

In D54889#1389066, @dvyukov wrote:

The cur_thread/cur_thread_fast separation still looks confusing to me. It's a convoluted way to do lazy initialization. If one adds any new calls to these functions, which one to choose is non-obvious. I think we should do lazy initialization explicitly. Namely, leave cur_thread alone, don't introduce cur_thread_fast, don't change any call sites. Instead, add init_cur_thread call that does lazy initialization to interceptor entry point and any other points that we expect can be the first call into tsan runtime overall or within a new thread. I think interceptors and tsan_init should be enough (no tsan_func_entry). We call __tsan_init from .preinit_array, instrumented code can't be executed before .preinit_array, only interceptors from dynamic loader can precede .preinit_array callbacks.

• Replaced cur_thread1 with ThreadState::current
• Removed cur_thread_fast() and replaced calls with cur_thread()
• Added cur_thread_init()

In D57876#1390150, @dvyukov wrote:

One thing that I noticed is that pthread_exit is a noreturn function and SCOPED_TSAN_INTERCEPTOR creates a local object with destructor. At the very least this leads to confusing code where readers assumption is that the destructor will run, but it will not.
Perhaps we should try to remove the SCOPED_TSAN_INTERCEPTOR entirely as the CHECK that we want to add here does not need it.

In D57876#1390146, @dvyukov wrote:

It looks like your change has caused a failure of the Darwin/dispatch_main.mm TSAN test on our CI:

Can someone with access to ppc64be system help to investigate the issue?

Rebased on current trunk

Next patch (performance optimizations): D57882

One more patch (pthread_exit): D57876

More detailed comment

In D54889#1387062, @dvyukov wrote:

Going forward I think we should get in all unrelated/preparatory changed first: thread type (creates lots of diffs), pthread_exit interceptor/test and spot optimizations to memory access functions.

Hi Dmitry,
Now, when all performance issues are resolved, what else should be done to complete the review?

I have found out which optimization is applied by gcc but not by clang and did it manually. Executions times for new version:

Optimized MemoryAccessImpl1()

In D54889#1349383, @dvyukov wrote:

"current" is compiler-rt HEAD with no local changes, or with the previous version of this change? This makes clang-compiled runtime 12% faster?

I improved test execution time. On my system I got following execution times (compared to original version of code):

Improved performance

Fix typo

In D54889#1334579, @dvyukov wrote:

I added default synchronization and flag to opt-out.

Can you swicth to it in your codebase? I would expect that you need all (or almost all) of this synchronization anyway?

In D54889#1334562, @dvyukov wrote:

I don't completely follow logic behind cur_thread/cur_thread_fast/cur_thread1 and how it does not introduce slowdown.
cur_thread_fast still includes an indirection, so it looks exactly the same as the first version.
cur_thread_fast does not check for NULL, so I would expect it to be used only in few carefully chosen functions. But it's used in most entry functions. Which makes me wonder: when cur_thread_fast cannot be used if it can be used in all these places?
It seems that you use cur_thread only to lazily initialize cur_thread1 in Initialize or ThreadStart. But then the question is: why don't we explicitly intialize cur_thread1 in these functions, and then just have a single cur_thread function as before with no 2 subtly different accessors?

• introduce ThreadType enum
• cleanup internal interfaces
• add pthread_exit() interceptor
• more tests

In D54889#1334678, @741g wrote:

https://android-review.googlesource.com/c/platform/external/qemu/+/844675

Latest version of patch doesn't work with QEMU anymore, at least with those annotations. Error log:

Do you know what I'm doing wrong here?

Please check if adding __attribute__((always_inline)) for start_switch_fiber() and finish_switch_fiber() helps

In D54889#1326913, @dvyukov wrote:

In D54889#1325232, @yuri wrote:

3. I think instead of _tsan_get_current_fiber we could use pointer 0. It prevents things like switching to context of another thread, which won't end well. And just simplifies API surface.

For us possibility to use context of another thread is required feature. We use it to call into library which use fibers from external finber-unaware code.
It is on-par with Windows API for fibers - ConvertThreadToFiber().

Tell me more.
You have context for all fibers, tsan_create_fiber returns it.
_tsan_get_current_fiber is only useful for the physical thread. Do you use tsan_switch_to_fiber to switch from a fiber to a non-current physical thread? I don't see how this can work. Or what do you do?

In D54889#1326910, @dvyukov wrote:

In D54889#1325232, @yuri wrote:

2. This introduces slowdown into the hottest runtime functions (branch and indirection in cur_thread). I think check_analyze.sh test will fail. Kinda hard to say, because it's broken already at the moment, but this perturbs runtime functions enough:

I have checked parts of the patch separately and found:

• Part of patch related to HACKY_CALL does not affect code of critical functions and benchmark results
• Part of patch related to proc() does not affect code of critical functions and benchmark results
• The only part that affects critical functions is implementation cur_thread(). I tried to minimize its effect in new version of a patch.

Yes, cur_thread() is the problem. It's used in all the hottest functions.
As far as I see the current code still contains an indirection in cur_thread(). We need to think of a way to not affect hot paths.

In D54889#1326850, @dvyukov wrote:

In D54889#1325232, @yuri wrote:

Hi Dmitry,
I will try to answer your questions

In D54889#1316889, @dvyukov wrote:

1. Do we want to synchronize fibers on switch? If fibers use TLS data, or cooperative scheduling (e.g. mutex unlock directly schedules the next critical section owner), or pass some data from prev fiber to next fiber, in all these cases not synchronizing fibers on switch will lead to false positives. As far as I remember in my old experiments I concluded that it's almost impossible to get rid of false positives it tasks/fibers are not synchronized on switch.

In case of single-threaded scheduler, it is worth to synchronize fibers on each switch. Currently I do it in client code using _tsan_release()/_tsan_acquire(), but it is possible to add flag for _tsan_switch_to_fiber() that will do it.

In case of multithreaded scheduler, client probably has own sychronization primitives (for example, custom mutex), and it is client's responsibility to call corresponding TSAN functions.

I think that synchronization will be exactly in the wrong place.
Consider, a thread pushes a fiber for execution on a thread pool. It locks a mutex and adds the fiber onto run queue. Then a thread pool thread dequeues the fiber from the run queue under the same mutex. Now the first thread and the thread pool thread are synchronized. Now the thread pool switches to the fiber context, no synchronization happens, so the fiber is _not_ synchronized with the submitting thread. This means that any passed data will cause false positives.
Even if the fiber is just created, in the above scheme even reading fiber entry function arguments (and even just writing to the stack) will cause false positives.

If fiber switch will synchronize parent thread and the fiber, then we will get the desired transitive synchronization.

Looking at the race reports produces by the qemu prototype, it seems there are actually all kinds of these false reports. There are false races on TLS storage. There are races on fiber start function. There are numerous races on arguments passed to fibers.

You said that for your case you need the synchronization, and it seems that it's also needed for all other cases too. I think we need to do it. People will get annotations wrong, or don't think about acquire/release at all, and then come to mailing list with questions about all these races. If it will prove to be needed, we can always add an opt-out of synchronization with a new flag for the annotations.

• By default sync fibers with each other on switch
• Faster cur_thread(), no visible difference in benchmark results
• Moved implementation to tsan_rtl_thread.cc

Problem solved differently in D54889

• Temporary switch into fiber context for start and stop instead of avoiding HACKY_CALL
• Minimized perfomance impact of cur_thread() change
• Added (currently unused) flags to _tsan_create_fiber() and _tsan_switch_to_fiber()

Hi Dmitry,
I will try to answer your questions

In D54889#1309646, @dvyukov wrote:

I think single threaded programs would also benefit from swapcontext support. At the very least it will give correct stacks. I think tsan shadow stack can also overflow on some fibers patterns (if a fiber has uneven number of function entries and exits).

• Excluded macOS support in order to reduce patch size
• Added comment in tsan_interface.h
• Added tests

In D54889#1309542, @dvyukov wrote:

In D54889#1309507, @yuri wrote:

In D54889#1309474, @dvyukov wrote:

It's unclear how this is supposed to be used. I would expect to see an interceptor for swapcontext that will make use of this, but there is none...

Interceptor for swapcontext without additional sanitizer API is not enough because it is not known when context is created and destroyed. Actually, I am not sure if swapcontext is actually used by someone because it is slow and has strange limitations.

My interface assumes is that users should call __tsan_switch_to_fiber() immediately before switching context and then call swapcontext or whatever he want to perform actual switch.

What do people use nowadays?

In D54889#1309475, @dvyukov wrote:

I'd like to make sure that the work can be re-used to implement proper understanding of Darwin GCD queues.

What kind of API is required for GCD queues?
A while ago I sketched something for tasking support:
https://codereview.appspot.com/6904068/diff/6001/rtl/tsan_interface_task.h
It would be nice to have tasking support too (nobody programs in threads today), but I don't yet know how close this fibers support to tasking support.

In D54889#1309474, @dvyukov wrote:

It's unclear how this is supposed to be used. I would expect to see an interceptor for swapcontext that will make use of this, but there is none...

In D54889#1308510, @kubamracek wrote:

Can you explain what this patch does a bit? The way I see it, it's just adding an extra level of indirection to each cur_thread() call. Is that it? Does that affect the performance of TSan?

In D54835#1306574, @dvyukov wrote:

Fibers support sounds great.
Regardless of how we proceed with submitting patches, I would like to see the whole change first. Otherwise it's hard to see reasoning behind individual changes and understand if it's right approach or not.
Please upload whole change into a separate review.

I have working implemented of fibers (coroutines) support for thread sanitizer.
Taking in the account that fibers are supported by address sanitizer, support in thread sanitizer may be useful as well.

In D54521#1305057, @dvyukov wrote:

Do you want me to merge this or you have commit access?

Improved tests

Added tests and fixed code, now it really works