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

[Stefan Hajnoczi <stefanha@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4358 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]