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

[Robert Foley <robert.foley@linaro.org>]

Hi,

On Wed, 17 Jun 2020 at 10:24, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>
> 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.

Thanks for the details.  We removed this tracing in a later patch.
https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg02506.html
>
> > +
> >  /**
> >   * 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?

We will look into this and add documentation here or (if it is no
longer needed),
remove the inline.

<snip>
> > -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);

Good point.  We did change this by dropping the if in a later patch.
https://lists.gnu.org/archive/html/qemu-devel/2020-06/msg02506.html


> > +    }
> > +#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.

Makes sense, we will make these changes and submit a cleanup patch.

Thanks & Regards,
-Rob