Re: [PATCH v2 01/13] configure: add --enable-tsan flag + fiber annotations for coroutine-ucontext

[Alex Bennée]

Robert Foley <robert.foley@linaro.org> writes:

> From: Lingfeng Yang <lfy@google.com>
>
> We tried running QEMU under tsan in 2016, but tsan's lack of support for
> longjmp-based fibers was a blocker:
>   https://groups.google.com/forum/#!topic/thread-sanitizer/se0YuzfWazw
>
> Fortunately, thread sanitizer gained fiber support in early 2019:
>   https://reviews.llvm.org/D54889
>
> This patch brings tsan support upstream by importing the patch that annotated
> QEMU's coroutines as tsan fibers in Android's QEMU fork:
>   https://android-review.googlesource.com/c/platform/external/qemu/+/844675
>
> Tested with '--enable-tsan --cc=clang-9 --cxx=clang++-9 --disable-werror'
> configure flags.
>
> Signed-off-by: Lingfeng Yang <lfy@google.com>
> Signed-off-by: Emilio G. Cota <cota@braap.org>
> [cota: minor modifications + configure changes]
> Signed-off-by: Robert Foley <robert.foley@linaro.org>
> [RF: configure changes for warnings, erorr handling + minor modifications]
<snip>
>  
> +#define UC_DEBUG 0
> +#if UC_DEBUG && defined(CONFIG_TSAN)
> +#define UC_TRACE(fmt, ...) fprintf(stderr, "%s:%d:%p " fmt "\n", \
> +    __func__, __LINE__, __tsan_get_current_fiber(), ##__VA_ARGS__);
> +#else
> +#define UC_TRACE(fmt, ...)
> +#endif
> +

We shouldn't be introducing new debug printfs if we can avoid it. I
suspect a separate patch could introduce some relevant trace points that
are outside the #if CONFIG_TSAN chunks.

>  /**
>   * Per-thread coroutine bookkeeping
>   */
> @@ -65,7 +80,20 @@ union cc_arg {
>      int i[2];
>  };
>  
> -static void finish_switch_fiber(void *fake_stack_save)
> +/* QEMU_ALWAYS_INLINE only does so if __OPTIMIZE__, so we cannot use it. */
> +static inline __attribute__((always_inline))
> +void on_new_fiber(CoroutineUContext *co)
> +{

We could put a tracepoint here at something like trace_new_fibre() but I
suspect for following what's going on you could probably just have
tracepoints in the higher coroutine functions and leave the fibre stuff
as purely a CONFIG_TSAN detail.

Please we wouldn't have to ague about American vs British spelling for
the tracepoints ;-)

<snip>

Otherwise without the UC_TRACE verbiage:

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

-- 
Alex Bennée