[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 01/13] configure: add --enable-tsan flag + fiber annotatio
From: |
Alex Bennée |
Subject: |
Re: [PATCH v2 01/13] configure: add --enable-tsan flag + fiber annotations for coroutine-ucontext |
Date: |
Mon, 08 Jun 2020 14:39:36 +0100 |
User-agent: |
mu4e 1.5.2; emacs 28.0.50 |
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
- [PATCH v2 00/13] Add Thread Sanitizer support to QEMU, Robert Foley, 2020/06/05
- [PATCH v2 01/13] configure: add --enable-tsan flag + fiber annotations for coroutine-ucontext, Robert Foley, 2020/06/05
- Re: [PATCH v2 01/13] configure: add --enable-tsan flag + fiber annotations for coroutine-ucontext,
Alex Bennée <=
- [PATCH v2 02/13] cpu: convert queued work to a QSIMPLEQ, Robert Foley, 2020/06/05
- [PATCH v2 03/13] thread: add qemu_spin_destroy, Robert Foley, 2020/06/05
- [PATCH v2 04/13] cputlb: destroy CPUTLB with tlb_destroy, Robert Foley, 2020/06/05
- [PATCH v2 05/13] qht: call qemu_spin_destroy for head buckets, Robert Foley, 2020/06/05
- [PATCH v2 06/13] tcg: call qemu_spin_destroy for tb->jmp_lock, Robert Foley, 2020/06/05
- [PATCH v2 07/13] translate-all: call qemu_spin_destroy for PageDesc, Robert Foley, 2020/06/05