[PATCH] D54889: Fiber support for thread sanitizer

Mon Dec 17 07:39:00 PST 2018

yuri added a comment.

In D54889#1326913 <https://reviews.llvm.org/D54889#1326913>, @dvyukov wrote:

> In D54889#1325232 <https://reviews.llvm.org/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 standalone thread we create temporary context and switch into into it. Then original thread context is posted into event loop thread, where it runs for some time together with other fibers. At some point code decides that it should migrate to original thread. Context is removed from list of fibers and orginal thread (running temporart context) is signaled. Thread switches into its own context and destroys temporary context.

This trick significantly simplifies implementation of bindings for library that uses fibers internally and improves debugability because stack traces are preserved after switch.

>>> 4. What if a fiber calls pthread_exit?
>> 
>> I think, it's enough to add check and abort until someone will really need support for it
> 
> Let's add a check and a test then. These things tend to lead to very obscure failure modes that take days to debug on large user programs.

Currently there is no interceptor for pthread_exit(). Do you suggest to add it?

>>> 5. Can we combine _tsan_set_fiber_name with _tsan_create_fiber?
>> 
>> It will not solve our requirements. It is convenient to set fiber name from the fiber itself and even change it sometimes, analogously to pthread_setname_np().
> 
> I see. Let's leave it as separate function. I guess it's more flexible and will cover all possible use cases.
> 
>>> 7. Do we want to support nested fibers/tasks? Does GCD have nested tasks? I think TBB/OpenMP does. Won't begin/end fiber/task be a more generic API for this? E.g. fiber switching can be expressed as stop running the prev fiber (which switches back to the physical thread context), then start running new fiber.
>> 
>> GCD has no nested task. GCD tasks can start other tasks, but can't switch or wait.
>> 
>> For me current API looks more generic then what you suggest.
>>  Also, switch to physical thread context may be problematic in some case (see answer to question 3).
> 
> Let's leave tasks aside for now. It's quite complex without tasks already.
> 
>>> 8. How does this play with setjmp/longjmp? In particular with fiber switching based on setjmp/longjmp? jmp_bufs is part of ThreadState, won't we always try to do longjmp from a wrong ThreadState in such case?
>> 
>> My idea is when setjmp/longjmp is used for fiber switch, client should disable interceptors before call and enable them after it.
> 
> What do you mean by "disable interceptors"? Do we have such thing?
> 
>> This procedure can be simplified by flag of _tsan_switch_to_fiber().
> 
> How? setjmp/longjmp happen outside of __tsan_switch_to_fiber.

Isn't it enough to place __tsan_ignore_thread_begin()/__tsan_ignore_thread_end() around setjmp/longjmp calls?

Repository:
  rCRT Compiler Runtime

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

https://reviews.llvm.org/D54889