On Fri, May 22, 2020 at 12:07:37PM -0400, Robert Foley wrote: > +#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
QEMU has tracing support, see docs/devel/tracing.txt. These fprintfs
should be trace events defined in the util/trace-events file.
> +
> /**
> * 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))
Please document why always_inline is necessary here and in other
functions. Is it for performance or because the __tsan_*() functions
need to be called from a the parent function?
> +void on_new_fiber(CoroutineUContext *co)
> +{
> +#ifdef CONFIG_TSAN
> + co->tsan_co_fiber = __tsan_create_fiber(0); /* flags: sync on switch */
> + co->tsan_caller_fiber = __tsan_get_current_fiber();
> + UC_TRACE("Create new TSAN co fiber. co: %p co fiber: %p caller fiber: %p
> ",
> + co, co->tsan_co_fiber, co->tsan_caller_fiber);
> +#endif
> +}
> +
> +static inline __attribute__((always_inline))
> +void finish_switch_fiber(void *fake_stack_save)
> {
> #ifdef CONFIG_ASAN
> const void *bottom_old;
> @@ -78,18 +106,40 @@ static void finish_switch_fiber(void *fake_stack_save)
> leader.stack_size = size_old;
> }
> #endif
> +#ifdef CONFIG_TSAN
> + if (fake_stack_save) {
> + __tsan_release(fake_stack_save);
> + __tsan_switch_to_fiber(fake_stack_save, 0); /* 0=synchronize */
> + }
> +#endif
> }
>
> -static void start_switch_fiber(void **fake_stack_save,
> - const void *bottom, size_t size)
> +static inline __attribute__((always_inline)) void start_switch_fiber(
> + CoroutineAction action, void **fake_stack_save,
> + const void *bottom, size_t size, void *new_fiber)
> {
> #ifdef CONFIG_ASAN
> - __sanitizer_start_switch_fiber(fake_stack_save, bottom, size);
> + if (action == COROUTINE_TERMINATE) {
> + __sanitizer_start_switch_fiber(
> + action == COROUTINE_TERMINATE ? NULL : fake_stack_save,
The if statement already checks action == COROUTINE_TERMINATE, why is it
being checked again?
I think the old behavior can be retained by dropping the if statement
like this:
__sanitizer_start_switch_fiber(action == COROUTINE_TERMINATE ?
NULL : fake_stack_save,
bottom, size);
> + bottom, size);
> + }
> +#endif
> +#ifdef CONFIG_TSAN
> + void *curr_fiber =
> + __tsan_get_current_fiber();
> + __tsan_acquire(curr_fiber);
> +
> + UC_TRACE("Current fiber: %p.", curr_fiber);
> + *fake_stack_save = curr_fiber;
> + UC_TRACE("Switch to fiber %p", new_fiber);
> + __tsan_switch_to_fiber(new_fiber, 0); /* 0=synchronize */
> #endif
> }
Please split start_switch_fiber() into two functions:
start_switch_fiber_asan() and start_switch_fiber_tsan(). That way the
asan- and tsan-specific arguments can be kept separate and the
co->tsan_* fields only need to be compiled in when CONFIG_TSAN is
defined.
For example:
static inline __attribute__((always_inline))
void start_switch_fiber_tsan(void **fake_stack_save,
CoroutineUContext *co,
bool caller)
{
#ifdef CONFIG_TSAN
void *new_fiber = caller ?
co->tsan_caller_fiber :
co->tsan_co_fiber;
void *curr_fiber = __tsan_get_current_fiber();
__tsan_acquire(curr_fiber);
UC_TRACE("Current fiber: %p.", curr_fiber);
*fake_stack_save = curr_fiber;
UC_TRACE("Switch to fiber %p", new_fiber);
__tsan_switch_to_fiber(new_fiber, 0); /* 0=synchronize */
#endif
}
This does two things:
1. Unrelated ASAN and TSAN code is separate and each function only
has arguments that are actually needed.
2. The co->tsan_caller_fiber and co->tsan_co_fiber fields are only
access from within #ifdef CONFIG_TSAN.
signature.asc
Description: PGP signature
