Hi, On Wed, 17 Jun 2020 at 10:24, Stefan Hajnoczi <stefa...@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
