* [PATCH v1 01/12] configure: add --enable-tsan flag + fiber annotations for coroutine-ucontext
@ 2020-05-29 13:23 Robert Foley
2020-05-29 13:23 ` [PATCH v1 02/12] cpu: convert queued work to a QSIMPLEQ Robert Foley
` (11 more replies)
0 siblings, 12 replies; 22+ messages in thread
From: Robert Foley @ 2020-05-29 13:23 UTC (permalink / raw)
To: qemu-devel
Cc: Kevin Wolf, robert.foley, cota, Stefan Hajnoczi, Lingfeng Yang,
peter.puhov, alex.bennee
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: Error out in configure if tsan not available, fix checkpatch warnings]
---
configure | 41 +++++++++++++++++
util/coroutine-ucontext.c | 97 +++++++++++++++++++++++++++++++++++----
2 files changed, 129 insertions(+), 9 deletions(-)
diff --git a/configure b/configure
index b969dee675..c18efae65e 100755
--- a/configure
+++ b/configure
@@ -395,6 +395,7 @@ gprof="no"
debug_tcg="no"
debug="no"
sanitizers="no"
+tsan="no"
fortify_source=""
strip_opt="yes"
tcg_interpreter="no"
@@ -1150,6 +1151,10 @@ for opt do
;;
--disable-sanitizers) sanitizers="no"
;;
+ --enable-tsan) tsan="yes"
+ ;;
+ --disable-tsan) tsan="no"
+ ;;
--enable-sparse) sparse="yes"
;;
--disable-sparse) sparse="no"
@@ -1750,6 +1755,7 @@ Advanced options (experts only):
--with-pkgversion=VERS use specified string as sub-version of the package
--enable-debug enable common debug build options
--enable-sanitizers enable default sanitizers
+ --enable-tsan enable thread sanitizer
--disable-strip disable stripping binaries
--disable-werror disable compilation abort on warning
--disable-stack-protector disable compiler-provided stack protection
@@ -6192,6 +6198,27 @@ if test "$fuzzing" = "yes" ; then
fi
fi
+# Thread sanitizer is, for now, much noisier than the other sanitizers;
+# keep it separate until that is not the case.
+have_tsan=no
+have_tsan_iface_fiber=no
+if test "$tsan" = "yes" ; then
+ write_c_skeleton
+ if compile_prog "$CPU_CFLAGS -Werror -fsanitize=thread" "" ; then
+ have_tsan=yes
+ fi
+ cat > $TMPC << EOF
+#include <sanitizer/tsan_interface.h>
+int main(void) {
+ __tsan_create_fiber(0);
+ return 0;
+}
+EOF
+ if compile_prog "$CPU_CFLAGS -Werror -fsanitize=thread" "" ; then
+ have_tsan_iface_fiber=yes
+ fi
+fi
+
##########################################
# check for libpmem
@@ -6293,6 +6320,16 @@ if test "$have_asan" = "yes"; then
"Without code annotation, the report may be inferior."
fi
fi
+if test "$have_tsan" = "yes" ; then
+ if test "$have_tsan_iface_fiber" = "yes" ; then
+ QEMU_CFLAGS="-fsanitize=thread $QEMU_CFLAGS"
+ QEMU_LDFLAGS="-fsanitize=thread $QEMU_LDFLAGS"
+ else
+ error_exit "Cannot enable TSAN due to missing fiber annotation interface."
+ fi
+elif test "$tsan" = "yes" ; then
+ error_exit "Cannot enable TSAN due to missing sanitize thread interface."
+fi
if test "$have_ubsan" = "yes"; then
QEMU_CFLAGS="-fsanitize=undefined $QEMU_CFLAGS"
QEMU_LDFLAGS="-fsanitize=undefined $QEMU_LDFLAGS"
@@ -7382,6 +7419,10 @@ if test "$have_asan_iface_fiber" = "yes" ; then
echo "CONFIG_ASAN_IFACE_FIBER=y" >> $config_host_mak
fi
+if test "$have_tsan" = "yes" && test "$have_tsan_iface_fiber" = "yes" ; then
+ echo "CONFIG_TSAN=y" >> $config_host_mak
+fi
+
if test "$has_environ" = "yes" ; then
echo "CONFIG_HAS_ENVIRON=y" >> $config_host_mak
fi
diff --git a/util/coroutine-ucontext.c b/util/coroutine-ucontext.c
index bd593e61bc..a3dc78e67a 100644
--- a/util/coroutine-ucontext.c
+++ b/util/coroutine-ucontext.c
@@ -37,18 +37,33 @@
#endif
#endif
+#ifdef CONFIG_TSAN
+#include <sanitizer/tsan_interface.h>
+#endif
+
typedef struct {
Coroutine base;
void *stack;
size_t stack_size;
sigjmp_buf env;
+ void *tsan_co_fiber;
+ void *tsan_caller_fiber;
+
#ifdef CONFIG_VALGRIND_H
unsigned int valgrind_stack_id;
#endif
} CoroutineUContext;
+#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
+
/**
* 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)
+{
+#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,
+ 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
}
static void coroutine_trampoline(int i0, int i1)
{
+ UC_TRACE("Start trampoline");
union cc_arg arg;
CoroutineUContext *self;
Coroutine *co;
@@ -104,21 +154,34 @@ static void coroutine_trampoline(int i0, int i1)
/* Initialize longjmp environment and switch back the caller */
if (!sigsetjmp(self->env, 0)) {
- start_switch_fiber(&fake_stack_save,
- leader.stack, leader.stack_size);
+ UC_TRACE("Current fiber: %p. Set co %p to env 0x%lx",
+ __tsan_get_current_fiber(), self, (unsigned long)self->env);
+ start_switch_fiber(
+ COROUTINE_YIELD,
+ &fake_stack_save,
+ leader.stack,
+ leader.stack_size,
+ self->tsan_caller_fiber);
+ UC_TRACE("Jump to co %p caller fiber %p env 0x%lx",
+ co, self->tsan_caller_fiber, *(unsigned long *)co->entry_arg);
siglongjmp(*(sigjmp_buf *)co->entry_arg, 1);
}
+ UC_TRACE("After first siglongjmp");
+
finish_switch_fiber(fake_stack_save);
while (true) {
co->entry(co->entry_arg);
+ UC_TRACE("switch from co %p to caller co %p fiber %p\n",
+ co, co->caller, self->tsan_caller_fiber);
qemu_coroutine_switch(co, co->caller, COROUTINE_TERMINATE);
}
}
Coroutine *qemu_coroutine_new(void)
{
+ UC_TRACE("Start new coroutine");
CoroutineUContext *co;
ucontext_t old_uc, uc;
sigjmp_buf old_env;
@@ -154,12 +217,16 @@ Coroutine *qemu_coroutine_new(void)
arg.p = co;
+ on_new_fiber(co);
makecontext(&uc, (void (*)(void))coroutine_trampoline,
2, arg.i[0], arg.i[1]);
/* swapcontext() in, siglongjmp() back out */
if (!sigsetjmp(old_env, 0)) {
- start_switch_fiber(&fake_stack_save, co->stack, co->stack_size);
+ start_switch_fiber(
+ COROUTINE_YIELD,
+ &fake_stack_save,
+ co->stack, co->stack_size, co->tsan_co_fiber);
swapcontext(&old_uc, &uc);
}
@@ -185,6 +252,7 @@ static inline void valgrind_stack_deregister(CoroutineUContext *co)
void qemu_coroutine_delete(Coroutine *co_)
{
+ UC_TRACE("Nuking co %p from orbit", co_);
CoroutineUContext *co = DO_UPCAST(CoroutineUContext, base, co_);
#ifdef CONFIG_VALGRIND_H
@@ -209,6 +277,10 @@ qemu_coroutine_switch(Coroutine *from_, Coroutine *to_,
{
CoroutineUContext *from = DO_UPCAST(CoroutineUContext, base, from_);
CoroutineUContext *to = DO_UPCAST(CoroutineUContext, base, to_);
+ UC_TRACE("from to: %p %p uc: %p %p. fibers: %p %p caller fibers: %p %p\n",
+ from_, to_, from, to,
+ from->tsan_co_fiber, to->tsan_co_fiber,
+ from->tsan_caller_fiber, to->tsan_caller_fiber);
int ret;
void *fake_stack_save = NULL;
@@ -216,8 +288,8 @@ qemu_coroutine_switch(Coroutine *from_, Coroutine *to_,
ret = sigsetjmp(from->env, 0);
if (ret == 0) {
- start_switch_fiber(action == COROUTINE_TERMINATE ?
- NULL : &fake_stack_save, to->stack, to->stack_size);
+ start_switch_fiber(action, &fake_stack_save,
+ to->stack, to->stack_size, to->tsan_co_fiber);
siglongjmp(to->env, action);
}
@@ -231,6 +303,13 @@ Coroutine *qemu_coroutine_self(void)
if (!current) {
current = &leader.base;
}
+#ifdef CONFIG_TSAN
+ if (!leader.tsan_co_fiber) {
+ leader.tsan_co_fiber = __tsan_get_current_fiber();
+ UC_TRACE("For co %p set leader co fiber to %p",
+ current, leader.tsan_co_fiber);
+ }
+#endif
return current;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v1 02/12] cpu: convert queued work to a QSIMPLEQ
2020-05-29 13:23 [PATCH v1 01/12] configure: add --enable-tsan flag + fiber annotations for coroutine-ucontext Robert Foley
@ 2020-05-29 13:23 ` Robert Foley
2020-05-29 13:23 ` [PATCH v1 03/12] thread: add qemu_spin_destroy Robert Foley
` (10 subsequent siblings)
11 siblings, 0 replies; 22+ messages in thread
From: Robert Foley @ 2020-05-29 13:23 UTC (permalink / raw)
To: qemu-devel
Cc: robert.foley, Eduardo Habkost, cota, Paolo Bonzini, peter.puhov,
alex.bennee, Richard Henderson
From: "Emilio G. Cota" <cota@braap.org>
We convert queued work to a QSIMPLEQ, instead of
open-coding it.
While at it, make sure that all accesses to the list are
performed while holding the list's lock.
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Emilio G. Cota <cota@braap.org>
Signed-off-by: Robert Foley <robert.foley@linaro.org>
---
cpus-common.c | 25 ++++++++-----------------
cpus.c | 14 ++++++++++++--
hw/core/cpu.c | 1 +
include/hw/core/cpu.h | 6 +++---
4 files changed, 24 insertions(+), 22 deletions(-)
diff --git a/cpus-common.c b/cpus-common.c
index 55d5df8923..210fc7fc39 100644
--- a/cpus-common.c
+++ b/cpus-common.c
@@ -97,7 +97,7 @@ void cpu_list_remove(CPUState *cpu)
}
struct qemu_work_item {
- struct qemu_work_item *next;
+ QSIMPLEQ_ENTRY(qemu_work_item) node;
run_on_cpu_func func;
run_on_cpu_data data;
bool free, exclusive, done;
@@ -106,13 +106,7 @@ struct qemu_work_item {
static void queue_work_on_cpu(CPUState *cpu, struct qemu_work_item *wi)
{
qemu_mutex_lock(&cpu->work_mutex);
- if (cpu->queued_work_first == NULL) {
- cpu->queued_work_first = wi;
- } else {
- cpu->queued_work_last->next = wi;
- }
- cpu->queued_work_last = wi;
- wi->next = NULL;
+ QSIMPLEQ_INSERT_TAIL(&cpu->work_list, wi, node);
wi->done = false;
qemu_mutex_unlock(&cpu->work_mutex);
@@ -306,17 +300,14 @@ void process_queued_cpu_work(CPUState *cpu)
{
struct qemu_work_item *wi;
- if (cpu->queued_work_first == NULL) {
+ qemu_mutex_lock(&cpu->work_mutex);
+ if (QSIMPLEQ_EMPTY(&cpu->work_list)) {
+ qemu_mutex_unlock(&cpu->work_mutex);
return;
}
-
- qemu_mutex_lock(&cpu->work_mutex);
- while (cpu->queued_work_first != NULL) {
- wi = cpu->queued_work_first;
- cpu->queued_work_first = wi->next;
- if (!cpu->queued_work_first) {
- cpu->queued_work_last = NULL;
- }
+ while (!QSIMPLEQ_EMPTY(&cpu->work_list)) {
+ wi = QSIMPLEQ_FIRST(&cpu->work_list);
+ QSIMPLEQ_REMOVE_HEAD(&cpu->work_list, node);
qemu_mutex_unlock(&cpu->work_mutex);
if (wi->exclusive) {
/* Running work items outside the BQL avoids the following deadlock:
diff --git a/cpus.c b/cpus.c
index 5670c96bcf..af44027549 100644
--- a/cpus.c
+++ b/cpus.c
@@ -97,9 +97,19 @@ bool cpu_is_stopped(CPUState *cpu)
return cpu->stopped || !runstate_is_running();
}
+static inline bool cpu_work_list_empty(CPUState *cpu)
+{
+ bool ret;
+
+ qemu_mutex_lock(&cpu->work_mutex);
+ ret = QSIMPLEQ_EMPTY(&cpu->work_list);
+ qemu_mutex_unlock(&cpu->work_mutex);
+ return ret;
+}
+
static bool cpu_thread_is_idle(CPUState *cpu)
{
- if (cpu->stop || cpu->queued_work_first) {
+ if (cpu->stop || !cpu_work_list_empty(cpu)) {
return false;
}
if (cpu_is_stopped(cpu)) {
@@ -1498,7 +1508,7 @@ static void *qemu_tcg_rr_cpu_thread_fn(void *arg)
cpu = first_cpu;
}
- while (cpu && !cpu->queued_work_first && !cpu->exit_request) {
+ while (cpu && cpu_work_list_empty(cpu) && !cpu->exit_request) {
atomic_mb_set(&tcg_current_rr_cpu, cpu);
current_cpu = cpu;
diff --git a/hw/core/cpu.c b/hw/core/cpu.c
index 5284d384fb..77703d62b7 100644
--- a/hw/core/cpu.c
+++ b/hw/core/cpu.c
@@ -368,6 +368,7 @@ static void cpu_common_initfn(Object *obj)
cpu->nr_threads = 1;
qemu_mutex_init(&cpu->work_mutex);
+ QSIMPLEQ_INIT(&cpu->work_list);
QTAILQ_INIT(&cpu->breakpoints);
QTAILQ_INIT(&cpu->watchpoints);
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index 07f7698155..d78ff1d165 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -331,8 +331,8 @@ struct qemu_work_item;
* @opaque: User data.
* @mem_io_pc: Host Program Counter at which the memory was accessed.
* @kvm_fd: vCPU file descriptor for KVM.
- * @work_mutex: Lock to prevent multiple access to queued_work_*.
- * @queued_work_first: First asynchronous work pending.
+ * @work_mutex: Lock to prevent multiple access to @work_list.
+ * @work_list: List of pending asynchronous work.
* @trace_dstate_delayed: Delayed changes to trace_dstate (includes all changes
* to @trace_dstate).
* @trace_dstate: Dynamic tracing state of events for this vCPU (bitmask).
@@ -376,7 +376,7 @@ struct CPUState {
sigjmp_buf jmp_env;
QemuMutex work_mutex;
- struct qemu_work_item *queued_work_first, *queued_work_last;
+ QSIMPLEQ_HEAD(, qemu_work_item) work_list;
CPUAddressSpace *cpu_ases;
int num_ases;
--
2.17.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v1 03/12] thread: add qemu_spin_destroy
2020-05-29 13:23 [PATCH v1 01/12] configure: add --enable-tsan flag + fiber annotations for coroutine-ucontext Robert Foley
2020-05-29 13:23 ` [PATCH v1 02/12] cpu: convert queued work to a QSIMPLEQ Robert Foley
@ 2020-05-29 13:23 ` Robert Foley
2020-06-02 19:23 ` Alex Bennée
2020-05-29 13:23 ` [PATCH v1 04/12] cputlb: destroy CPUTLB with tlb_destroy Robert Foley
` (9 subsequent siblings)
11 siblings, 1 reply; 22+ messages in thread
From: Robert Foley @ 2020-05-29 13:23 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.puhov, cota, alex.bennee, robert.foley
From: "Emilio G. Cota" <cota@braap.org>
It will be used for TSAN annotations.
Signed-off-by: Emilio G. Cota <cota@braap.org>
Signed-off-by: Robert Foley <robert.foley@linaro.org>
---
include/qemu/thread.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/include/qemu/thread.h b/include/qemu/thread.h
index d22848138e..e50a073889 100644
--- a/include/qemu/thread.h
+++ b/include/qemu/thread.h
@@ -215,6 +215,9 @@ static inline void qemu_spin_init(QemuSpin *spin)
__sync_lock_release(&spin->value);
}
+static inline void qemu_spin_destroy(QemuSpin *spin)
+{ }
+
static inline void qemu_spin_lock(QemuSpin *spin)
{
while (unlikely(__sync_lock_test_and_set(&spin->value, true))) {
--
2.17.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v1 04/12] cputlb: destroy CPUTLB with tlb_destroy
2020-05-29 13:23 [PATCH v1 01/12] configure: add --enable-tsan flag + fiber annotations for coroutine-ucontext Robert Foley
2020-05-29 13:23 ` [PATCH v1 02/12] cpu: convert queued work to a QSIMPLEQ Robert Foley
2020-05-29 13:23 ` [PATCH v1 03/12] thread: add qemu_spin_destroy Robert Foley
@ 2020-05-29 13:23 ` Robert Foley
2020-06-02 19:25 ` Alex Bennée
2020-05-29 13:23 ` [PATCH v1 05/12] qht: call qemu_spin_destroy for head buckets Robert Foley
` (8 subsequent siblings)
11 siblings, 1 reply; 22+ messages in thread
From: Robert Foley @ 2020-05-29 13:23 UTC (permalink / raw)
To: qemu-devel
Cc: robert.foley, cota, Paolo Bonzini, peter.puhov, alex.bennee,
Richard Henderson
From: "Emilio G. Cota" <cota@braap.org>
I was after adding qemu_spin_destroy calls, but while at
it I noticed that we are leaking some memory.
Signed-off-by: Emilio G. Cota <cota@braap.org>
Signed-off-by: Robert Foley <robert.foley@linaro.org>
---
accel/tcg/cputlb.c | 15 +++++++++++++++
exec.c | 1 +
include/exec/exec-all.h | 8 ++++++++
3 files changed, 24 insertions(+)
diff --git a/accel/tcg/cputlb.c b/accel/tcg/cputlb.c
index eb2cf9de5e..1e815357c7 100644
--- a/accel/tcg/cputlb.c
+++ b/accel/tcg/cputlb.c
@@ -270,6 +270,21 @@ void tlb_init(CPUState *cpu)
}
}
+void tlb_destroy(CPUState *cpu)
+{
+ CPUArchState *env = cpu->env_ptr;
+ int i;
+
+ qemu_spin_destroy(&env_tlb(env)->c.lock);
+ for (i = 0; i < NB_MMU_MODES; i++) {
+ CPUTLBDesc *desc = &env_tlb(env)->d[i];
+ CPUTLBDescFast *fast = &env_tlb(env)->f[i];
+
+ g_free(fast->table);
+ g_free(desc->iotlb);
+ }
+}
+
/* flush_all_helper: run fn across all cpus
*
* If the wait flag is set then the src cpu's helper will be queued as
diff --git a/exec.c b/exec.c
index 5162f0d12f..da3d60b034 100644
--- a/exec.c
+++ b/exec.c
@@ -892,6 +892,7 @@ void cpu_exec_unrealizefn(CPUState *cpu)
{
CPUClass *cc = CPU_GET_CLASS(cpu);
+ tlb_destroy(cpu);
cpu_list_remove(cpu);
if (cc->vmsd != NULL) {
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index 8792bea07a..3cf88272df 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -124,6 +124,11 @@ void cpu_address_space_init(CPUState *cpu, int asidx,
* @cpu: CPU whose TLB should be initialized
*/
void tlb_init(CPUState *cpu);
+/**
+ * tlb_destroy - destroy a CPU's TLB
+ * @cpu: CPU whose TLB should be destroyed
+ */
+void tlb_destroy(CPUState *cpu);
/**
* tlb_flush_page:
* @cpu: CPU whose TLB should be flushed
@@ -284,6 +289,9 @@ void tlb_set_page(CPUState *cpu, target_ulong vaddr,
static inline void tlb_init(CPUState *cpu)
{
}
+static inline void tlb_destroy(CPUState *cpu)
+{
+}
static inline void tlb_flush_page(CPUState *cpu, target_ulong addr)
{
}
--
2.17.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v1 05/12] qht: call qemu_spin_destroy for head buckets
2020-05-29 13:23 [PATCH v1 01/12] configure: add --enable-tsan flag + fiber annotations for coroutine-ucontext Robert Foley
` (2 preceding siblings ...)
2020-05-29 13:23 ` [PATCH v1 04/12] cputlb: destroy CPUTLB with tlb_destroy Robert Foley
@ 2020-05-29 13:23 ` Robert Foley
2020-06-02 19:26 ` Alex Bennée
2020-05-29 13:23 ` [PATCH v1 06/12] tcg: call qemu_spin_destroy for tb->jmp_lock Robert Foley
` (7 subsequent siblings)
11 siblings, 1 reply; 22+ messages in thread
From: Robert Foley @ 2020-05-29 13:23 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.puhov, cota, alex.bennee, robert.foley
From: "Emilio G. Cota" <cota@braap.org>
Signed-off-by: Robert Foley <robert.foley@linaro.org>
---
util/qht.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/util/qht.c b/util/qht.c
index aa51be3c52..67e5d5b916 100644
--- a/util/qht.c
+++ b/util/qht.c
@@ -348,6 +348,7 @@ static inline void qht_chain_destroy(const struct qht_bucket *head)
struct qht_bucket *curr = head->next;
struct qht_bucket *prev;
+ qemu_spin_destroy(&head->lock);
while (curr) {
prev = curr;
curr = curr->next;
--
2.17.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v1 06/12] tcg: call qemu_spin_destroy for tb->jmp_lock
2020-05-29 13:23 [PATCH v1 01/12] configure: add --enable-tsan flag + fiber annotations for coroutine-ucontext Robert Foley
` (3 preceding siblings ...)
2020-05-29 13:23 ` [PATCH v1 05/12] qht: call qemu_spin_destroy for head buckets Robert Foley
@ 2020-05-29 13:23 ` Robert Foley
2020-05-29 13:23 ` [PATCH v1 07/12] translate-all: call qemu_spin_destroy for PageDesc Robert Foley
` (6 subsequent siblings)
11 siblings, 0 replies; 22+ messages in thread
From: Robert Foley @ 2020-05-29 13:23 UTC (permalink / raw)
To: qemu-devel
Cc: robert.foley, cota, Paolo Bonzini, peter.puhov, alex.bennee,
Richard Henderson
From: "Emilio G. Cota" <cota@braap.org>
Signed-off-by: Emilio G. Cota <cota@braap.org>
Signed-off-by: Robert Foley <robert.foley@linaro.org>
[RF: Minor changes to fix some checkpatch errors]
---
accel/tcg/translate-all.c | 10 +++++++++-
include/tcg/tcg.h | 3 ++-
tcg/tcg.c | 19 ++++++++++++++++---
3 files changed, 27 insertions(+), 5 deletions(-)
diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index 42ce1dfcff..3708aab36b 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -384,6 +384,11 @@ static int cpu_restore_state_from_tb(CPUState *cpu, TranslationBlock *tb,
return 0;
}
+static void tb_destroy(TranslationBlock *tb)
+{
+ qemu_spin_destroy(&tb->jmp_lock);
+}
+
bool cpu_restore_state(CPUState *cpu, uintptr_t host_pc, bool will_exit)
{
TranslationBlock *tb;
@@ -413,6 +418,7 @@ bool cpu_restore_state(CPUState *cpu, uintptr_t host_pc, bool will_exit)
/* one-shot translation, invalidate it immediately */
tb_phys_invalidate(tb, -1);
tcg_tb_remove(tb);
+ tb_destroy(tb);
}
r = true;
}
@@ -1230,7 +1236,7 @@ static void do_tb_flush(CPUState *cpu, run_on_cpu_data tb_flush_count)
qht_reset_size(&tb_ctx.htable, CODE_GEN_HTABLE_SIZE);
page_flush_tb();
- tcg_region_reset_all();
+ tcg_region_reset_all(tb_destroy);
/* XXX: flush processor icache at this point if cache flush is
expensive */
atomic_mb_set(&tb_ctx.tb_flush_count, tb_ctx.tb_flush_count + 1);
@@ -1886,6 +1892,7 @@ TranslationBlock *tb_gen_code(CPUState *cpu,
orig_aligned -= ROUND_UP(sizeof(*tb), qemu_icache_linesize);
atomic_set(&tcg_ctx->code_gen_ptr, (void *)orig_aligned);
+ tb_destroy(tb);
return existing_tb;
}
tcg_tb_insert(tb);
@@ -2235,6 +2242,7 @@ void cpu_io_recompile(CPUState *cpu, uintptr_t retaddr)
tb_phys_invalidate(tb->orig_tb, -1);
}
tcg_tb_remove(tb);
+ tb_destroy(tb);
}
/* TODO: If env->pc != tb->pc (i.e. the faulting instruction was not
diff --git a/include/tcg/tcg.h b/include/tcg/tcg.h
index c48bd76b0a..2c5997e14e 100644
--- a/include/tcg/tcg.h
+++ b/include/tcg/tcg.h
@@ -815,8 +815,9 @@ void *tcg_malloc_internal(TCGContext *s, int size);
void tcg_pool_reset(TCGContext *s);
TranslationBlock *tcg_tb_alloc(TCGContext *s);
+typedef void (*tb_destroy_func)(TranslationBlock *tb);
void tcg_region_init(void);
-void tcg_region_reset_all(void);
+void tcg_region_reset_all(tb_destroy_func tb_destroy);
size_t tcg_code_size(void);
size_t tcg_code_capacity(void);
diff --git a/tcg/tcg.c b/tcg/tcg.c
index a2268d9db0..2680968683 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -502,7 +502,16 @@ size_t tcg_nb_tbs(void)
return nb_tbs;
}
-static void tcg_region_tree_reset_all(void)
+static gboolean tcg_region_tree_traverse(gpointer k, gpointer v, gpointer data)
+{
+ TranslationBlock *tb = v;
+ tb_destroy_func tb_destroy = data;
+
+ tb_destroy(tb);
+ return FALSE;
+}
+
+static void tcg_region_tree_reset_all(tb_destroy_func tb_destroy)
{
size_t i;
@@ -510,6 +519,10 @@ static void tcg_region_tree_reset_all(void)
for (i = 0; i < region.n; i++) {
struct tcg_region_tree *rt = region_trees + i * tree_size;
+ if (tb_destroy != NULL) {
+ g_tree_foreach(rt->tree, tcg_region_tree_traverse, tb_destroy);
+ }
+
/* Increment the refcount first so that destroy acts as a reset */
g_tree_ref(rt->tree);
g_tree_destroy(rt->tree);
@@ -586,7 +599,7 @@ static inline bool tcg_region_initial_alloc__locked(TCGContext *s)
}
/* Call from a safe-work context */
-void tcg_region_reset_all(void)
+void tcg_region_reset_all(tb_destroy_func tb_destroy)
{
unsigned int n_ctxs = atomic_read(&n_tcg_ctxs);
unsigned int i;
@@ -603,7 +616,7 @@ void tcg_region_reset_all(void)
}
qemu_mutex_unlock(®ion.lock);
- tcg_region_tree_reset_all();
+ tcg_region_tree_reset_all(tb_destroy);
}
#ifdef CONFIG_USER_ONLY
--
2.17.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v1 07/12] translate-all: call qemu_spin_destroy for PageDesc
2020-05-29 13:23 [PATCH v1 01/12] configure: add --enable-tsan flag + fiber annotations for coroutine-ucontext Robert Foley
` (4 preceding siblings ...)
2020-05-29 13:23 ` [PATCH v1 06/12] tcg: call qemu_spin_destroy for tb->jmp_lock Robert Foley
@ 2020-05-29 13:23 ` Robert Foley
2020-05-29 13:23 ` [PATCH v1 08/12] thread: add tsan annotations to QemuSpin Robert Foley
` (5 subsequent siblings)
11 siblings, 0 replies; 22+ messages in thread
From: Robert Foley @ 2020-05-29 13:23 UTC (permalink / raw)
To: qemu-devel
Cc: robert.foley, cota, Paolo Bonzini, peter.puhov, alex.bennee,
Richard Henderson
From: "Emilio G. Cota" <cota@braap.org>
The radix tree is append-only, but we can fail to insert
a PageDesc if the insertion races with another thread.
Signed-off-by: Emilio G. Cota <cota@braap.org>
Signed-off-by: Robert Foley <robert.foley@linaro.org>
---
accel/tcg/translate-all.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/accel/tcg/translate-all.c b/accel/tcg/translate-all.c
index 3708aab36b..3fb71a1503 100644
--- a/accel/tcg/translate-all.c
+++ b/accel/tcg/translate-all.c
@@ -547,6 +547,15 @@ static PageDesc *page_find_alloc(tb_page_addr_t index, int alloc)
#endif
existing = atomic_cmpxchg(lp, NULL, pd);
if (unlikely(existing)) {
+#ifndef CONFIG_USER_ONLY
+ {
+ int i;
+
+ for (i = 0; i < V_L2_SIZE; i++) {
+ qemu_spin_destroy(&pd[i].lock);
+ }
+ }
+#endif
g_free(pd);
pd = existing;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v1 08/12] thread: add tsan annotations to QemuSpin
2020-05-29 13:23 [PATCH v1 01/12] configure: add --enable-tsan flag + fiber annotations for coroutine-ucontext Robert Foley
` (5 preceding siblings ...)
2020-05-29 13:23 ` [PATCH v1 07/12] translate-all: call qemu_spin_destroy for PageDesc Robert Foley
@ 2020-05-29 13:23 ` Robert Foley
2020-06-02 20:14 ` Alex Bennée
2020-05-29 13:23 ` [PATCH v1 09/12] tests/docker: Added docker build support for TSan Robert Foley
` (4 subsequent siblings)
11 siblings, 1 reply; 22+ messages in thread
From: Robert Foley @ 2020-05-29 13:23 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.puhov, cota, alex.bennee, robert.foley
From: "Emilio G. Cota" <cota@braap.org>
Signed-off-by: Emilio G. Cota <cota@braap.org>
Signed-off-by: Robert Foley <robert.foley@linaro.org>
---
include/qemu/thread.h | 39 ++++++++++++++++++++++++++++++++++++---
1 file changed, 36 insertions(+), 3 deletions(-)
diff --git a/include/qemu/thread.h b/include/qemu/thread.h
index e50a073889..43fc094b96 100644
--- a/include/qemu/thread.h
+++ b/include/qemu/thread.h
@@ -206,6 +206,10 @@ void qemu_thread_atexit_add(struct Notifier *notifier);
*/
void qemu_thread_atexit_remove(struct Notifier *notifier);
+#ifdef CONFIG_TSAN
+#include <sanitizer/tsan_interface.h>
+#endif
+
struct QemuSpin {
int value;
};
@@ -213,23 +217,46 @@ struct QemuSpin {
static inline void qemu_spin_init(QemuSpin *spin)
{
__sync_lock_release(&spin->value);
+#ifdef CONFIG_TSAN
+ __tsan_mutex_create(spin, __tsan_mutex_not_static);
+#endif
}
-static inline void qemu_spin_destroy(QemuSpin *spin)
-{ }
+/* const parameter because the only purpose here is the TSAN annotation */
+static inline void qemu_spin_destroy(const QemuSpin *spin)
+{
+#ifdef CONFIG_TSAN
+ __tsan_mutex_destroy((void *)spin, __tsan_mutex_not_static);
+#endif
+}
static inline void qemu_spin_lock(QemuSpin *spin)
{
+#ifdef CONFIG_TSAN
+ __tsan_mutex_pre_lock(spin, 0);
+#endif
while (unlikely(__sync_lock_test_and_set(&spin->value, true))) {
while (atomic_read(&spin->value)) {
cpu_relax();
}
}
+#ifdef CONFIG_TSAN
+ __tsan_mutex_post_lock(spin, 0, 0);
+#endif
}
static inline bool qemu_spin_trylock(QemuSpin *spin)
{
- return __sync_lock_test_and_set(&spin->value, true);
+#ifdef CONFIG_TSAN
+ __tsan_mutex_pre_lock(spin, __tsan_mutex_try_lock);
+#endif
+ bool busy = __sync_lock_test_and_set(&spin->value, true);
+#ifdef CONFIG_TSAN
+ unsigned flags = __tsan_mutex_try_lock;
+ flags |= busy ? __tsan_mutex_try_lock_failed : 0;
+ __tsan_mutex_post_lock(spin, flags, 0);
+#endif
+ return busy;
}
static inline bool qemu_spin_locked(QemuSpin *spin)
@@ -239,7 +266,13 @@ static inline bool qemu_spin_locked(QemuSpin *spin)
static inline void qemu_spin_unlock(QemuSpin *spin)
{
+#ifdef CONFIG_TSAN
+ __tsan_mutex_pre_unlock(spin, 0);
+#endif
__sync_lock_release(&spin->value);
+#ifdef CONFIG_TSAN
+ __tsan_mutex_post_unlock(spin, 0);
+#endif
}
struct QemuLockCnt {
--
2.17.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v1 09/12] tests/docker: Added docker build support for TSan.
2020-05-29 13:23 [PATCH v1 01/12] configure: add --enable-tsan flag + fiber annotations for coroutine-ucontext Robert Foley
` (6 preceding siblings ...)
2020-05-29 13:23 ` [PATCH v1 08/12] thread: add tsan annotations to QemuSpin Robert Foley
@ 2020-05-29 13:23 ` Robert Foley
2020-06-02 20:21 ` Alex Bennée
2020-05-29 13:23 ` [PATCH v1 10/12] include/qemu: Added tsan.h for annotations Robert Foley
` (3 subsequent siblings)
11 siblings, 1 reply; 22+ messages in thread
From: Robert Foley @ 2020-05-29 13:23 UTC (permalink / raw)
To: qemu-devel
Cc: Fam Zheng, robert.foley, Philippe Mathieu-Daudé,
cota, peter.puhov, alex.bennee
Added a new docker for ubuntu 20.04.
This docker has support for Thread Sanitizer
including one patch we need in one of the header files.
https://github.com/llvm/llvm-project/commit/a72dc86cd
This command will build with tsan enabled:
make docker-test-build-ubuntu2004 V=1 TSAN=1
Also added the TSAN suppresion file to disable certain
cases of TSAN warnings.
Cc: Fam Zheng <fam@euphon.net>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Signed-off-by: Robert Foley <robert.foley@linaro.org>
---
tests/docker/Makefile.include | 2 +
tests/docker/common.rc | 19 +++++++
tests/docker/dockerfiles/ubuntu2004.docker | 65 ++++++++++++++++++++++
tests/tsan/blacklist.tsan | 10 ++++
tests/tsan/suppressions.tsan | 14 +++++
5 files changed, 110 insertions(+)
create mode 100644 tests/docker/dockerfiles/ubuntu2004.docker
create mode 100644 tests/tsan/blacklist.tsan
create mode 100644 tests/tsan/suppressions.tsan
diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
index 43a8678688..e029e54b42 100644
--- a/tests/docker/Makefile.include
+++ b/tests/docker/Makefile.include
@@ -202,6 +202,7 @@ endif
@echo ' (default is 1)'
@echo ' DEBUG=1 Stop and drop to shell in the created container'
@echo ' before running the command.'
+ @echo ' TSAN=1 Enable use of tsan during the build/test.'
@echo ' NETWORK=1 Enable virtual network interface with default backend.'
@echo ' NETWORK=$$BACKEND Enable virtual network interface with $$BACKEND.'
@echo ' NOUSER Define to disable adding current user to containers passwd.'
@@ -239,6 +240,7 @@ docker-run: docker-qemu-src
-e EXTRA_CONFIGURE_OPTS="$(EXTRA_CONFIGURE_OPTS)" \
-e V=$V -e J=$J -e DEBUG=$(DEBUG) \
-e SHOW_ENV=$(SHOW_ENV) \
+ $(if $(TSAN),,-e TSAN=$(TSAN)) \
$(if $(NOUSER),, \
-e CCACHE_DIR=/var/tmp/ccache \
-v $(DOCKER_CCACHE_DIR):/var/tmp/ccache:z \
diff --git a/tests/docker/common.rc b/tests/docker/common.rc
index 02cd67a8c5..5df93c6326 100755
--- a/tests/docker/common.rc
+++ b/tests/docker/common.rc
@@ -27,6 +27,25 @@ requires()
configure_qemu()
{
+ if test -z "$TSAN"; then
+ requires clang tsan
+ echo "Including TSan Support"
+ tsan_log_dir="/tmp/qemu-test/build/tsan"
+ mkdir -p $tsan_log_dir > /dev/null || true
+ EXTRA_CONFIGURE_OPTS="${EXTRA_CONFIGURE_OPTS} --enable-tsan \
+ --cc=clang-10 --cxx=clang++-10 \
+ --disable-werror --extra-cflags=-O0"
+ # detect deadlocks is false currently simply because
+ # TSan crashes immediately with deadlock detecter enabled.
+ # We have maxed out the history size to get the best chance of finding
+ # warnings during testing.
+ # Note, to get tsan to fail on warning, use exitcode=66 below.
+ tsan_opts="suppressions=/tmp/qemu-test/src/tests/tsan/suppressions.tsan\
+ detect_deadlocks=false history_size=7\
+ halt_on_error=0 exitcode=0 verbose=5\
+ log_path=$tsan_log_dir/tsan_warnings.txt"
+ export TSAN_OPTIONS="$tsan_opts"
+ fi
config_opts="--enable-werror \
${TARGET_LIST:+--target-list=${TARGET_LIST}} \
--prefix=$INSTALL_DIR \
diff --git a/tests/docker/dockerfiles/ubuntu2004.docker b/tests/docker/dockerfiles/ubuntu2004.docker
new file mode 100644
index 0000000000..6050ce7e8a
--- /dev/null
+++ b/tests/docker/dockerfiles/ubuntu2004.docker
@@ -0,0 +1,65 @@
+FROM ubuntu:20.04
+ENV PACKAGES flex bison \
+ ccache \
+ clang-10\
+ gcc \
+ gettext \
+ git \
+ glusterfs-common \
+ libaio-dev \
+ libattr1-dev \
+ libbrlapi-dev \
+ libbz2-dev \
+ libcacard-dev \
+ libcap-ng-dev \
+ libcurl4-gnutls-dev \
+ libdrm-dev \
+ libepoxy-dev \
+ libfdt-dev \
+ libgbm-dev \
+ libgtk-3-dev \
+ libibverbs-dev \
+ libiscsi-dev \
+ libjemalloc-dev \
+ libjpeg-turbo8-dev \
+ liblzo2-dev \
+ libncurses5-dev \
+ libncursesw5-dev \
+ libnfs-dev \
+ libnss3-dev \
+ libnuma-dev \
+ libpixman-1-dev \
+ librados-dev \
+ librbd-dev \
+ librdmacm-dev \
+ libsasl2-dev \
+ libsdl2-dev \
+ libseccomp-dev \
+ libsnappy-dev \
+ libspice-protocol-dev \
+ libspice-server-dev \
+ libssh-dev \
+ libusb-1.0-0-dev \
+ libusbredirhost-dev \
+ libvdeplug-dev \
+ libvte-2.91-dev \
+ libxen-dev \
+ libzstd-dev \
+ make \
+ python3-yaml \
+ python3-sphinx \
+ sparse \
+ texinfo \
+ xfslibs-dev\
+ vim
+RUN apt-get update && \
+ DEBIAN_FRONTEND=noninteractive apt-get -y install $PACKAGES
+RUN dpkg -l $PACKAGES | sort > /packages.txt
+ENV FEATURES clang tsan pyyaml sdl2
+
+# https://bugs.launchpad.net/qemu/+bug/1838763
+ENV QEMU_CONFIGURE_OPTS --disable-libssh
+
+# Apply patch https://reviews.llvm.org/D75820
+# This is required for TSan in clang-10 to compile with QEMU.
+RUN sed -i 's/^const/static const/g' /usr/lib/llvm-10/lib/clang/10.0.0/include/sanitizer/tsan_interface.h
diff --git a/tests/tsan/blacklist.tsan b/tests/tsan/blacklist.tsan
new file mode 100644
index 0000000000..75e444f5dc
--- /dev/null
+++ b/tests/tsan/blacklist.tsan
@@ -0,0 +1,10 @@
+# This is an example blacklist.
+# To enable use of the blacklist add this to configure:
+# "--extra-cflags=-fsanitize-blacklist=<src path>/tests/tsan/blacklist.tsan"
+# The eventual goal would be to fix these warnings.
+
+# TSan is not happy about setting/getting of dirty bits,
+# for example, cpu_physical_memory_set_dirty_range,
+# and cpu_physical_memory_get_dirty.
+src:bitops.c
+src:bitmap.c
diff --git a/tests/tsan/suppressions.tsan b/tests/tsan/suppressions.tsan
new file mode 100644
index 0000000000..73414b9ebd
--- /dev/null
+++ b/tests/tsan/suppressions.tsan
@@ -0,0 +1,14 @@
+# This is the set of runtime suppressions of TSan warnings.
+# The goal would be to have here only items we do not
+# plan to fix, and to explain why for each item.
+
+# TSan reports a double lock on RECURSIVE mutexes.
+# Since the recursive lock is intentional, we choose to ignore it.
+mutex:aio_context_acquire
+mutex:pthread_mutex_lock
+
+# TSan reports a race betwen pthread_mutex_init() and
+# pthread_mutex_lock(). Since this is outside of QEMU,
+# we choose to ignore it.
+race:pthread_mutex_init
+race:pthread_mutex_lock
--
2.17.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v1 10/12] include/qemu: Added tsan.h for annotations.
2020-05-29 13:23 [PATCH v1 01/12] configure: add --enable-tsan flag + fiber annotations for coroutine-ucontext Robert Foley
` (7 preceding siblings ...)
2020-05-29 13:23 ` [PATCH v1 09/12] tests/docker: Added docker build support for TSan Robert Foley
@ 2020-05-29 13:23 ` Robert Foley
2020-05-29 13:23 ` [PATCH v1 11/12] util: Added tsan annotate for thread name Robert Foley
` (2 subsequent siblings)
11 siblings, 0 replies; 22+ messages in thread
From: Robert Foley @ 2020-05-29 13:23 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.puhov, cota, alex.bennee, robert.foley
These annotations will allow us to give tsan
additional hints. For example, we can inform
tsan about reads/writes to ignore to silence certain
classes of warnings.
We can also annotate threads so that the proper thread
naming shows up in tsan warning results.
Signed-off-by: Robert Foley <robert.foley@linaro.org>
Reviewed-by: Emilio G. Cota <cota@braap.org>
---
include/qemu/tsan.h | 71 +++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 71 insertions(+)
create mode 100644 include/qemu/tsan.h
diff --git a/include/qemu/tsan.h b/include/qemu/tsan.h
new file mode 100644
index 0000000000..09cc665f91
--- /dev/null
+++ b/include/qemu/tsan.h
@@ -0,0 +1,71 @@
+#ifndef QEMU_TSAN_H
+#define QEMU_TSAN_H
+/*
+ * tsan.h
+ *
+ * This file defines macros used to give ThreadSanitizer
+ * additional information to help suppress warnings.
+ * This is necessary since TSan does not provide a header file
+ * for these annotations. The standard way to include these
+ * is via the below macros.
+ *
+ * Annotation examples can be found here:
+ * https://github.com/llvm/llvm-project/tree/master/compiler-rt/test/tsan
+ * annotate_happens_before.cpp or ignore_race.cpp are good places to start.
+ *
+ * The full set of annotations can be found here in tsan_interface_ann.cpp.
+ * https://github.com/llvm/llvm-project/blob/master/compiler-rt/lib/tsan/rtl/
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifdef CONFIG_TSAN
+/*
+ * Informs TSan of a happens before/after relationship.
+ */
+#define QEMU_TSAN_ANNOTATE_HAPPENS_BEFORE(addr) \
+ AnnotateHappensBefore(__FILE__, __LINE__, (void *)(addr))
+#define QEMU_TSAN_ANNOTATE_HAPPENS_AFTER(addr) \
+ AnnotateHappensAfter(__FILE__, __LINE__, (void *)(addr))
+/*
+ * Gives TSan more information about thread names it can report the
+ * name of the thread in the warning report.
+ */
+#define QEMU_TSAN_ANNOTATE_THREAD_NAME(name) \
+ AnnotateThreadName(__FILE__, __LINE__, (void *)(name))
+/*
+ * Allows defining a region of code on which TSan will not record memory READS.
+ * This has the effect of disabling race detection for this section of code.
+ */
+#define QEMU_TSAN_ANNOTATE_IGNORE_READS_BEGIN() \
+ AnnotateIgnoreReadsBegin(__FILE__, __LINE__)
+#define QEMU_TSAN_ANNOTATE_IGNORE_READS_END() \
+ AnnotateIgnoreReadsEnd(__FILE__, __LINE__)
+/*
+ * Allows defining a region of code on which TSan will not record memory
+ * WRITES. This has the effect of disabling race detection for this
+ * section of code.
+ */
+#define QEMU_TSAN_ANNOTATE_IGNORE_WRITES_BEGIN() \
+ AnnotateIgnoreWritesBegin(__FILE__, __LINE__)
+#define QEMU_TSAN_ANNOTATE_IGNORE_WRITES_END() \
+ AnnotateIgnoreWritesEnd(__FILE__, __LINE__)
+#else
+#define QEMU_TSAN_ANNOTATE_HAPPENS_BEFORE(addr)
+#define QEMU_TSAN_ANNOTATE_HAPPENS_AFTER(addr)
+#define QEMU_TSAN_ANNOTATE_THREAD_NAME(name)
+#define QEMU_TSAN_ANNOTATE_IGNORE_READS_BEGIN()
+#define QEMU_TSAN_ANNOTATE_IGNORE_READS_END()
+#define QEMU_TSAN_ANNOTATE_IGNORE_WRITES_BEGIN()
+#define QEMU_TSAN_ANNOTATE_IGNORE_WRITES_END()
+#endif
+
+void AnnotateHappensBefore(const char *f, int l, void *addr);
+void AnnotateHappensAfter(const char *f, int l, void *addr);
+void AnnotateThreadName(const char *f, int l, char *name);
+void AnnotateIgnoreReadsBegin(const char *f, int l);
+void AnnotateIgnoreReadsEnd(const char *f, int l);
+void AnnotateIgnoreWritesBegin(const char *f, int l);
+void AnnotateIgnoreWritesEnd(const char *f, int l);
+#endif
--
2.17.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH v1 11/12] util: Added tsan annotate for thread name.
2020-05-29 13:23 [PATCH v1 01/12] configure: add --enable-tsan flag + fiber annotations for coroutine-ucontext Robert Foley
` (8 preceding siblings ...)
2020-05-29 13:23 ` [PATCH v1 10/12] include/qemu: Added tsan.h for annotations Robert Foley
@ 2020-05-29 13:23 ` Robert Foley
2020-06-01 3:56 ` Emilio G. Cota
2020-05-29 13:51 ` [PATCH v1 01/12] configure: add --enable-tsan flag + fiber annotations for coroutine-ucontext Eric Blake
2020-06-02 19:22 ` Alex Bennée
11 siblings, 1 reply; 22+ messages in thread
From: Robert Foley @ 2020-05-29 13:23 UTC (permalink / raw)
To: qemu-devel; +Cc: peter.puhov, cota, alex.bennee, robert.foley, Paolo Bonzini
This allows us to see the name of the thread in tsan
warning reports such as this:
Thread T7 'CPU 1/TCG' (tid=24317, running) created by main thread at:
Signed-off-by: Robert Foley <robert.foley@linaro.org>
---
util/qemu-thread-posix.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
index 838980aaa5..b4c2359272 100644
--- a/util/qemu-thread-posix.c
+++ b/util/qemu-thread-posix.c
@@ -15,6 +15,7 @@
#include "qemu/atomic.h"
#include "qemu/notify.h"
#include "qemu-thread-common.h"
+#include "qemu/tsan.h"
static bool name_threads;
@@ -513,6 +514,7 @@ static void *qemu_thread_start(void *args)
# endif
}
#endif
+ QEMU_TSAN_ANNOTATE_THREAD_NAME(qemu_thread_args->name);
g_free(qemu_thread_args->name);
g_free(qemu_thread_args);
pthread_cleanup_push(qemu_thread_atexit_notify, NULL);
--
2.17.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH v1 01/12] configure: add --enable-tsan flag + fiber annotations for coroutine-ucontext
2020-05-29 13:23 [PATCH v1 01/12] configure: add --enable-tsan flag + fiber annotations for coroutine-ucontext Robert Foley
` (9 preceding siblings ...)
2020-05-29 13:23 ` [PATCH v1 11/12] util: Added tsan annotate for thread name Robert Foley
@ 2020-05-29 13:51 ` Eric Blake
2020-05-29 14:56 ` Robert Foley
2020-06-02 19:22 ` Alex Bennée
11 siblings, 1 reply; 22+ messages in thread
From: Eric Blake @ 2020-05-29 13:51 UTC (permalink / raw)
To: Robert Foley, qemu-devel
Cc: Kevin Wolf, cota, Stefan Hajnoczi, Lingfeng Yang, peter.puhov,
alex.bennee
On 5/29/20 8:23 AM, Robert Foley wrote:
> 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
[meta-comment]
This message lacked References: and In-Reply-To: headers, making it the
start of a new thread for patches 1-11. But I also see you sent a 0/12
message with Message-Id: <20200529132143.702-1-robert.foley@linaro.org>,
as well as an unthreaded 12/12 with Message-Id:
<20200529132438.837-1-robert.foley@linaro.org>. You may want to figure
out why your sending workflow is not preserving desired threading.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3226
Virtualization: qemu.org | libvirt.org
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v1 01/12] configure: add --enable-tsan flag + fiber annotations for coroutine-ucontext
2020-05-29 13:51 ` [PATCH v1 01/12] configure: add --enable-tsan flag + fiber annotations for coroutine-ucontext Eric Blake
@ 2020-05-29 14:56 ` Robert Foley
0 siblings, 0 replies; 22+ messages in thread
From: Robert Foley @ 2020-05-29 14:56 UTC (permalink / raw)
To: Eric Blake
Cc: Kevin Wolf, QEMU Developers, Emilio G. Cota, Stefan Hajnoczi,
Lingfeng Yang, Peter Puhov, Alex Bennée
On Fri, 29 May 2020 at 09:51, Eric Blake <eblake@redhat.com> wrote:
>
> On 5/29/20 8:23 AM, Robert Foley wrote:
> > 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
>
> [meta-comment]
>
> This message lacked References: and In-Reply-To: headers, making it the
> start of a new thread for patches 1-11. But I also see you sent a 0/12
> message with Message-Id: <20200529132143.702-1-robert.foley@linaro.org>,
> as well as an unthreaded 12/12 with Message-Id:
> <20200529132438.837-1-robert.foley@linaro.org>. You may want to figure
> out why your sending workflow is not preserving desired threading.
Thanks for pointing this out. I see what happened here and will fix
it for next time.
Thanks,
-Rob
> --
> Eric Blake, Principal Software Engineer
> Red Hat, Inc. +1-919-301-3226
> Virtualization: qemu.org | libvirt.org
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v1 11/12] util: Added tsan annotate for thread name.
2020-05-29 13:23 ` [PATCH v1 11/12] util: Added tsan annotate for thread name Robert Foley
@ 2020-06-01 3:56 ` Emilio G. Cota
0 siblings, 0 replies; 22+ messages in thread
From: Emilio G. Cota @ 2020-06-01 3:56 UTC (permalink / raw)
To: Robert Foley; +Cc: peter.puhov, alex.bennee, qemu-devel, Paolo Bonzini
On Fri, May 29, 2020 at 09:23:41 -0400, Robert Foley wrote:
> This allows us to see the name of the thread in tsan
> warning reports such as this:
>
> Thread T7 'CPU 1/TCG' (tid=24317, running) created by main thread at:
>
> Signed-off-by: Robert Foley <robert.foley@linaro.org>
Reviewed-by: Emilio G. Cota <cota@braap.org>
Thanks,
Emilio
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v1 01/12] configure: add --enable-tsan flag + fiber annotations for coroutine-ucontext
2020-05-29 13:23 [PATCH v1 01/12] configure: add --enable-tsan flag + fiber annotations for coroutine-ucontext Robert Foley
` (10 preceding siblings ...)
2020-05-29 13:51 ` [PATCH v1 01/12] configure: add --enable-tsan flag + fiber annotations for coroutine-ucontext Eric Blake
@ 2020-06-02 19:22 ` Alex Bennée
2020-06-03 16:51 ` Robert Foley
11 siblings, 1 reply; 22+ messages in thread
From: Alex Bennée @ 2020-06-02 19:22 UTC (permalink / raw)
To: Robert Foley
Cc: Kevin Wolf, qemu-devel, cota, Stefan Hajnoczi, Lingfeng Yang,
peter.puhov
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: Error out in configure if tsan not available, fix checkpatch warnings]
> ---
> configure | 41 +++++++++++++++++
> util/coroutine-ucontext.c | 97 +++++++++++++++++++++++++++++++++++----
> 2 files changed, 129 insertions(+), 9 deletions(-)
>
> diff --git a/configure b/configure
> index b969dee675..c18efae65e 100755
> --- a/configure
> +++ b/configure
> @@ -395,6 +395,7 @@ gprof="no"
> debug_tcg="no"
> debug="no"
> sanitizers="no"
> +tsan="no"
> fortify_source=""
> strip_opt="yes"
> tcg_interpreter="no"
> @@ -1150,6 +1151,10 @@ for opt do
> ;;
> --disable-sanitizers) sanitizers="no"
> ;;
> + --enable-tsan) tsan="yes"
> + ;;
> + --disable-tsan) tsan="no"
> + ;;
> --enable-sparse) sparse="yes"
> ;;
> --disable-sparse) sparse="no"
> @@ -1750,6 +1755,7 @@ Advanced options (experts only):
> --with-pkgversion=VERS use specified string as sub-version of the package
> --enable-debug enable common debug build options
> --enable-sanitizers enable default sanitizers
> + --enable-tsan enable thread sanitizer
> --disable-strip disable stripping binaries
> --disable-werror disable compilation abort on warning
> --disable-stack-protector disable compiler-provided stack protection
> @@ -6192,6 +6198,27 @@ if test "$fuzzing" = "yes" ; then
> fi
> fi
>
> +# Thread sanitizer is, for now, much noisier than the other sanitizers;
> +# keep it separate until that is not the case.
I think we also need to stop both being enabled at once. On my clang-9
setup I get:
make: *** [qapi/qobject-output-visitor.o] Error 1
clang: error: invalid argument '-fsanitize=address' not allowed with '-fsanitize=thread'
clang: error: invalid argument '-fsanitize=address' not allowed with '-fsanitize=thread'
clang: errorclang: : errorinvalid argument '-fsanitize=address' not allowed with '-fsanitize=thread':
invalid argument '-fsanitize=address' not allowed with '-fsanitize=thread'
clang: error: invalid argument '-fsanitize=address' not allowed with '-fsanitize=thread'
> +have_tsan=no
> +have_tsan_iface_fiber=no
> +if test "$tsan" = "yes" ; then
> + write_c_skeleton
> + if compile_prog "$CPU_CFLAGS -Werror -fsanitize=thread" "" ; then
> + have_tsan=yes
> + fi
> + cat > $TMPC << EOF
> +#include <sanitizer/tsan_interface.h>
> +int main(void) {
> + __tsan_create_fiber(0);
> + return 0;
> +}
> +EOF
> + if compile_prog "$CPU_CFLAGS -Werror -fsanitize=thread" "" ; then
> + have_tsan_iface_fiber=yes
> + fi
> +fi
> +
> ##########################################
> # check for libpmem
>
> @@ -6293,6 +6320,16 @@ if test "$have_asan" = "yes"; then
> "Without code annotation, the report may be inferior."
> fi
> fi
> +if test "$have_tsan" = "yes" ; then
> + if test "$have_tsan_iface_fiber" = "yes" ; then
> + QEMU_CFLAGS="-fsanitize=thread $QEMU_CFLAGS"
> + QEMU_LDFLAGS="-fsanitize=thread $QEMU_LDFLAGS"
> + else
> + error_exit "Cannot enable TSAN due to missing fiber annotation interface."
> + fi
> +elif test "$tsan" = "yes" ; then
> + error_exit "Cannot enable TSAN due to missing sanitize thread interface."
> +fi
> if test "$have_ubsan" = "yes"; then
> QEMU_CFLAGS="-fsanitize=undefined $QEMU_CFLAGS"
> QEMU_LDFLAGS="-fsanitize=undefined $QEMU_LDFLAGS"
> @@ -7382,6 +7419,10 @@ if test "$have_asan_iface_fiber" = "yes" ; then
> echo "CONFIG_ASAN_IFACE_FIBER=y" >> $config_host_mak
> fi
Are we missing any LDFLAGS? On Ubuntu 18.04 with clang-9 I'm seeing:
LINK qemu-ga
/usr/lib/llvm-9/lib/clang/9.0.0/lib/linux/libclang_rt.tsan-x86_64.a(tsan_rtl_amd64.S.o): warning: common of `__interception::real_setjmp' overridden by definition
/usr/lib/llvm-9/lib/clang/9.0.0/lib/linux/libclang_rt.tsan-x86_64.a(tsan_interceptors.cc.o): warning: defined here
/usr/lib/llvm-9/lib/clang/9.0.0/lib/linux/libclang_rt.tsan-x86_64.a(tsan_rtl_amd64.S.o): warning: common of `__interception::real__setjmp' overridden by definition
/usr/lib/llvm-9/lib/clang/9.0.0/lib/linux/libclang_rt.tsan-x86_64.a(tsan_interceptors.cc.o): warning: defined here
/usr/lib/llvm-9/lib/clang/9.0.0/lib/linux/libclang_rt.tsan-x86_64.a(tsan_rtl_amd64.S.o): warning: common of `__interception::real_sigsetjmp' overridden by definition
/usr/lib/llvm-9/lib/clang/9.0.0/lib/linux/libclang_rt.tsan-x86_64.a(tsan_interceptors.cc.o): warning: defined here
/usr/lib/llvm-9/lib/clang/9.0.0/lib/linux/libclang_rt.tsan-x86_64.a(tsan_rtl_amd64.S.o): warning: common of `__interception::real___sigsetjmp' overridden by definition
/usr/lib/llvm-9/lib/clang/9.0.0/lib/linux/libclang_rt.tsan-x86_64.a(tsan_interceptors.cc.o): warning: defined here
libqemuutil.a(osdep.o):/usr/lib/llvm-9/lib/clang/9.0.0/include/sanitizer/tsan_interface.h:41: multiple definition of `__tsan_mutex_linker_init'
libqemuutil.a(control.o):/usr/lib/llvm-9/lib/clang/9.0.0/include/sanitizer/tsan_interface.h:41: first defined here
libqemuutil.a(osdep.o):/usr/lib/llvm-9/lib/clang/9.0.0/include/sanitizer/tsan_interface.h:50: multiple definition of `__tsan_mutex_not_static'
libqemuutil.a(control.o):/usr/lib/llvm-9/lib/clang/9.0.0/include/sanitizer/tsan_interface.h:50: first defined here
libqemuutil.a(osdep.o):/usr/lib/llvm-9/lib/clang/9.0.0/include/sanitizer/tsan_interface.h:55: multiple definition of `__tsan_mutex_read_lock'
libqemuutil.a(control.o):/usr/lib/llvm-9/lib/clang/9.0.0/include/sanitizer/tsan_interface.h:55: first defined here
libqemuutil.a(osdep.o):/usr/lib/llvm-9/lib/clang/9.0.0/include/sanitizer/tsan_interface.h:45: multiple definition of `__tsan_mutex_read_reentrant'
libqemuutil.a(control.o):/usr/lib/llvm-9/lib/clang/9.0.0/include/sanitizer/tsan_interface.h:45: first defined here
libqemuutil.a(osdep.o):/usr/lib/llvm-9/lib/clang/9.0.0/include/sanitizer/tsan_interface.h:64: multiple definition of `__tsan_mutex_recursive_lock'
libqemuutil.a(control.o):/usr/lib/llvm-9/lib/clang/9.0.0/include/sanitizer/tsan_interface.h:64: first defined here
libqemuutil.a(osdep.o):/usr/lib/llvm-9/lib/clang/9.0.0/include/sanitizer/tsan_interface.h:68: multiple definition of `__tsan_mutex_recursive_unlock'
<snip>
--
Alex Bennée
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v1 03/12] thread: add qemu_spin_destroy
2020-05-29 13:23 ` [PATCH v1 03/12] thread: add qemu_spin_destroy Robert Foley
@ 2020-06-02 19:23 ` Alex Bennée
0 siblings, 0 replies; 22+ messages in thread
From: Alex Bennée @ 2020-06-02 19:23 UTC (permalink / raw)
To: Robert Foley; +Cc: peter.puhov, cota, qemu-devel
Robert Foley <robert.foley@linaro.org> writes:
> From: "Emilio G. Cota" <cota@braap.org>
>
> It will be used for TSAN annotations.
>
> Signed-off-by: Emilio G. Cota <cota@braap.org>
> Signed-off-by: Robert Foley <robert.foley@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> ---
> include/qemu/thread.h | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/include/qemu/thread.h b/include/qemu/thread.h
> index d22848138e..e50a073889 100644
> --- a/include/qemu/thread.h
> +++ b/include/qemu/thread.h
> @@ -215,6 +215,9 @@ static inline void qemu_spin_init(QemuSpin *spin)
> __sync_lock_release(&spin->value);
> }
>
> +static inline void qemu_spin_destroy(QemuSpin *spin)
> +{ }
> +
> static inline void qemu_spin_lock(QemuSpin *spin)
> {
> while (unlikely(__sync_lock_test_and_set(&spin->value, true))) {
--
Alex Bennée
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v1 04/12] cputlb: destroy CPUTLB with tlb_destroy
2020-05-29 13:23 ` [PATCH v1 04/12] cputlb: destroy CPUTLB with tlb_destroy Robert Foley
@ 2020-06-02 19:25 ` Alex Bennée
0 siblings, 0 replies; 22+ messages in thread
From: Alex Bennée @ 2020-06-02 19:25 UTC (permalink / raw)
To: Robert Foley
Cc: peter.puhov, cota, qemu-devel, Paolo Bonzini, Richard Henderson
Robert Foley <robert.foley@linaro.org> writes:
> From: "Emilio G. Cota" <cota@braap.org>
>
> I was after adding qemu_spin_destroy calls, but while at
> it I noticed that we are leaking some memory.
>
> Signed-off-by: Emilio G. Cota <cota@braap.org>
> Signed-off-by: Robert Foley <robert.foley@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
--
Alex Bennée
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v1 05/12] qht: call qemu_spin_destroy for head buckets
2020-05-29 13:23 ` [PATCH v1 05/12] qht: call qemu_spin_destroy for head buckets Robert Foley
@ 2020-06-02 19:26 ` Alex Bennée
0 siblings, 0 replies; 22+ messages in thread
From: Alex Bennée @ 2020-06-02 19:26 UTC (permalink / raw)
To: Robert Foley; +Cc: peter.puhov, cota, qemu-devel
Robert Foley <robert.foley@linaro.org> writes:
> From: "Emilio G. Cota" <cota@braap.org>
>
> Signed-off-by: Robert Foley <robert.foley@linaro.org>
> ---
> util/qht.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/util/qht.c b/util/qht.c
> index aa51be3c52..67e5d5b916 100644
> --- a/util/qht.c
> +++ b/util/qht.c
> @@ -348,6 +348,7 @@ static inline void qht_chain_destroy(const struct qht_bucket *head)
> struct qht_bucket *curr = head->next;
> struct qht_bucket *prev;
>
> + qemu_spin_destroy(&head->lock);
> while (curr) {
> prev = curr;
> curr = curr->next;
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
--
Alex Bennée
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v1 08/12] thread: add tsan annotations to QemuSpin
2020-05-29 13:23 ` [PATCH v1 08/12] thread: add tsan annotations to QemuSpin Robert Foley
@ 2020-06-02 20:14 ` Alex Bennée
0 siblings, 0 replies; 22+ messages in thread
From: Alex Bennée @ 2020-06-02 20:14 UTC (permalink / raw)
To: Robert Foley; +Cc: peter.puhov, cota, qemu-devel
Robert Foley <robert.foley@linaro.org> writes:
> From: "Emilio G. Cota" <cota@braap.org>
>
> Signed-off-by: Emilio G. Cota <cota@braap.org>
> Signed-off-by: Robert Foley <robert.foley@linaro.org>
Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
> ---
> include/qemu/thread.h | 39 ++++++++++++++++++++++++++++++++++++---
> 1 file changed, 36 insertions(+), 3 deletions(-)
>
> diff --git a/include/qemu/thread.h b/include/qemu/thread.h
> index e50a073889..43fc094b96 100644
> --- a/include/qemu/thread.h
> +++ b/include/qemu/thread.h
> @@ -206,6 +206,10 @@ void qemu_thread_atexit_add(struct Notifier *notifier);
> */
> void qemu_thread_atexit_remove(struct Notifier *notifier);
>
> +#ifdef CONFIG_TSAN
> +#include <sanitizer/tsan_interface.h>
> +#endif
> +
> struct QemuSpin {
> int value;
> };
> @@ -213,23 +217,46 @@ struct QemuSpin {
> static inline void qemu_spin_init(QemuSpin *spin)
> {
> __sync_lock_release(&spin->value);
> +#ifdef CONFIG_TSAN
> + __tsan_mutex_create(spin, __tsan_mutex_not_static);
> +#endif
> }
>
> -static inline void qemu_spin_destroy(QemuSpin *spin)
> -{ }
> +/* const parameter because the only purpose here is the TSAN annotation */
> +static inline void qemu_spin_destroy(const QemuSpin *spin)
> +{
> +#ifdef CONFIG_TSAN
> + __tsan_mutex_destroy((void *)spin, __tsan_mutex_not_static);
> +#endif
> +}
>
> static inline void qemu_spin_lock(QemuSpin *spin)
> {
> +#ifdef CONFIG_TSAN
> + __tsan_mutex_pre_lock(spin, 0);
> +#endif
> while (unlikely(__sync_lock_test_and_set(&spin->value, true))) {
> while (atomic_read(&spin->value)) {
> cpu_relax();
> }
> }
> +#ifdef CONFIG_TSAN
> + __tsan_mutex_post_lock(spin, 0, 0);
> +#endif
> }
>
> static inline bool qemu_spin_trylock(QemuSpin *spin)
> {
> - return __sync_lock_test_and_set(&spin->value, true);
> +#ifdef CONFIG_TSAN
> + __tsan_mutex_pre_lock(spin, __tsan_mutex_try_lock);
> +#endif
> + bool busy = __sync_lock_test_and_set(&spin->value, true);
> +#ifdef CONFIG_TSAN
> + unsigned flags = __tsan_mutex_try_lock;
> + flags |= busy ? __tsan_mutex_try_lock_failed : 0;
> + __tsan_mutex_post_lock(spin, flags, 0);
> +#endif
> + return busy;
> }
>
> static inline bool qemu_spin_locked(QemuSpin *spin)
> @@ -239,7 +266,13 @@ static inline bool qemu_spin_locked(QemuSpin *spin)
>
> static inline void qemu_spin_unlock(QemuSpin *spin)
> {
> +#ifdef CONFIG_TSAN
> + __tsan_mutex_pre_unlock(spin, 0);
> +#endif
> __sync_lock_release(&spin->value);
> +#ifdef CONFIG_TSAN
> + __tsan_mutex_post_unlock(spin, 0);
> +#endif
> }
>
> struct QemuLockCnt {
--
Alex Bennée
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v1 09/12] tests/docker: Added docker build support for TSan.
2020-05-29 13:23 ` [PATCH v1 09/12] tests/docker: Added docker build support for TSan Robert Foley
@ 2020-06-02 20:21 ` Alex Bennée
2020-06-03 15:46 ` Robert Foley
0 siblings, 1 reply; 22+ messages in thread
From: Alex Bennée @ 2020-06-02 20:21 UTC (permalink / raw)
To: Robert Foley
Cc: Fam Zheng, peter.puhov, cota, Philippe Mathieu-Daudé, qemu-devel
Robert Foley <robert.foley@linaro.org> writes:
> Added a new docker for ubuntu 20.04.
> This docker has support for Thread Sanitizer
> including one patch we need in one of the header files.
> https://github.com/llvm/llvm-project/commit/a72dc86cd
>
> This command will build with tsan enabled:
> make docker-test-build-ubuntu2004 V=1 TSAN=1
>
> Also added the TSAN suppresion file to disable certain
> cases of TSAN warnings.
>
> Cc: Fam Zheng <fam@euphon.net>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Signed-off-by: Robert Foley <robert.foley@linaro.org>
> ---
> tests/docker/Makefile.include | 2 +
> tests/docker/common.rc | 19 +++++++
> tests/docker/dockerfiles/ubuntu2004.docker | 65 ++++++++++++++++++++++
> tests/tsan/blacklist.tsan | 10 ++++
> tests/tsan/suppressions.tsan | 14 +++++
> 5 files changed, 110 insertions(+)
> create mode 100644 tests/docker/dockerfiles/ubuntu2004.docker
> create mode 100644 tests/tsan/blacklist.tsan
> create mode 100644 tests/tsan/suppressions.tsan
>
> diff --git a/tests/docker/Makefile.include b/tests/docker/Makefile.include
> index 43a8678688..e029e54b42 100644
> --- a/tests/docker/Makefile.include
> +++ b/tests/docker/Makefile.include
> @@ -202,6 +202,7 @@ endif
> @echo ' (default is 1)'
> @echo ' DEBUG=1 Stop and drop to shell in the created container'
> @echo ' before running the command.'
> + @echo ' TSAN=1 Enable use of tsan during the
> build/test.'
I'd rather not pollute the build with another env flag, rather...
> @echo ' NETWORK=1 Enable virtual network interface with default backend.'
> @echo ' NETWORK=$$BACKEND Enable virtual network interface with $$BACKEND.'
> @echo ' NOUSER Define to disable adding current user to containers passwd.'
> @@ -239,6 +240,7 @@ docker-run: docker-qemu-src
> -e EXTRA_CONFIGURE_OPTS="$(EXTRA_CONFIGURE_OPTS)" \
> -e V=$V -e J=$J -e DEBUG=$(DEBUG) \
> -e SHOW_ENV=$(SHOW_ENV) \
> + $(if $(TSAN),,-e TSAN=$(TSAN)) \
> $(if $(NOUSER),, \
> -e CCACHE_DIR=/var/tmp/ccache \
> -v $(DOCKER_CCACHE_DIR):/var/tmp/ccache:z \
> diff --git a/tests/docker/common.rc b/tests/docker/common.rc
> index 02cd67a8c5..5df93c6326 100755
> --- a/tests/docker/common.rc
> +++ b/tests/docker/common.rc
> @@ -27,6 +27,25 @@ requires()
>
> configure_qemu()
> {
> + if test -z "$TSAN"; then
> + requires clang tsan
> + echo "Including TSan Support"
> + tsan_log_dir="/tmp/qemu-test/build/tsan"
> + mkdir -p $tsan_log_dir > /dev/null || true
> + EXTRA_CONFIGURE_OPTS="${EXTRA_CONFIGURE_OPTS} --enable-tsan \
> + --cc=clang-10 --cxx=clang++-10 \
> + --disable-werror --extra-cflags=-O0"
> + # detect deadlocks is false currently simply because
> + # TSan crashes immediately with deadlock detecter enabled.
> + # We have maxed out the history size to get the best chance of finding
> + # warnings during testing.
> + # Note, to get tsan to fail on warning, use exitcode=66 below.
> + tsan_opts="suppressions=/tmp/qemu-test/src/tests/tsan/suppressions.tsan\
> + detect_deadlocks=false history_size=7\
> + halt_on_error=0 exitcode=0 verbose=5\
> + log_path=$tsan_log_dir/tsan_warnings.txt"
> + export TSAN_OPTIONS="$tsan_opts"
> + fi
...I think it would be better to put this in it's own test (test-tsan?)
--
Alex Bennée
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v1 09/12] tests/docker: Added docker build support for TSan.
2020-06-02 20:21 ` Alex Bennée
@ 2020-06-03 15:46 ` Robert Foley
0 siblings, 0 replies; 22+ messages in thread
From: Robert Foley @ 2020-06-03 15:46 UTC (permalink / raw)
To: Alex Bennée
Cc: Fam Zheng, Peter Puhov, Emilio G. Cota,
Philippe Mathieu-Daudé,
QEMU Developers
On Tue, 2 Jun 2020 at 16:21, Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Robert Foley <robert.foley@linaro.org> writes:
<snip>
> >
> > configure_qemu()
> > {
> > + if test -z "$TSAN"; then
> > + requires clang tsan
> > + echo "Including TSan Support"
> > + tsan_log_dir="/tmp/qemu-test/build/tsan"
> > + mkdir -p $tsan_log_dir > /dev/null || true
> > + EXTRA_CONFIGURE_OPTS="${EXTRA_CONFIGURE_OPTS} --enable-tsan \
> > + --cc=clang-10 --cxx=clang++-10 \
> > + --disable-werror --extra-cflags=-O0"
> > + # detect deadlocks is false currently simply because
> > + # TSan crashes immediately with deadlock detecter enabled.
> > + # We have maxed out the history size to get the best chance of finding
> > + # warnings during testing.
> > + # Note, to get tsan to fail on warning, use exitcode=66 below.
> > + tsan_opts="suppressions=/tmp/qemu-test/src/tests/tsan/suppressions.tsan\
> > + detect_deadlocks=false history_size=7\
> > + halt_on_error=0 exitcode=0 verbose=5\
> > + log_path=$tsan_log_dir/tsan_warnings.txt"
> > + export TSAN_OPTIONS="$tsan_opts"
> > + fi
>
> ...I think it would be better to put this in it's own test (test-tsan?)
>
Makes sense, we will put this TSan code in its own separate test.
Sure, test-tsan seems like a good name for this.
Thanks & Regards,
-Rob
> --
> Alex Bennée
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH v1 01/12] configure: add --enable-tsan flag + fiber annotations for coroutine-ucontext
2020-06-02 19:22 ` Alex Bennée
@ 2020-06-03 16:51 ` Robert Foley
0 siblings, 0 replies; 22+ messages in thread
From: Robert Foley @ 2020-06-03 16:51 UTC (permalink / raw)
To: Alex Bennée
Cc: Kevin Wolf, QEMU Developers, Emilio G. Cota, Stefan Hajnoczi,
Lingfeng Yang, Peter Puhov
On Tue, 2 Jun 2020 at 15:22, Alex Bennée <alex.bennee@linaro.org> wrote:
>
>
> Robert Foley <robert.foley@linaro.org> writes:
>
> > From: Lingfeng Yang <lfy@google.com>
<snip>
> >
> > +# Thread sanitizer is, for now, much noisier than the other sanitizers;
> > +# keep it separate until that is not the case.
>
> I think we also need to stop both being enabled at once. On my clang-9
> setup I get:
>
> make: *** [qapi/qobject-output-visitor.o] Error 1
> clang: error: invalid argument '-fsanitize=address' not allowed with '-fsanitize=thread'
> clang: error: invalid argument '-fsanitize=address' not allowed with '-fsanitize=thread'
> clang: errorclang: : errorinvalid argument '-fsanitize=address' not allowed with '-fsanitize=thread':
> invalid argument '-fsanitize=address' not allowed with '-fsanitize=thread'
> clang: error: invalid argument '-fsanitize=address' not allowed with '-fsanitize=thread'
Good point. I see a similar error if I use --enable-sanitizers and
--enable-tsan.
Will look to add an error check for this. Wondering if there are any
other interactions
we need to avoid? Will poke around a bit here.
<snip>
>
> Are we missing any LDFLAGS? On Ubuntu 18.04 with clang-9 I'm seeing:
>
> LINK qemu-ga
> /usr/lib/llvm-9/lib/clang/9.0.0/lib/linux/libclang_rt.tsan-x86_64.a(tsan_rtl_amd64.S.o): warning: common of `__interception::real_setjmp' overridden by definition
> /usr/lib/llvm-9/lib/clang/9.0.0/lib/linux/libclang_rt.tsan-x86_64.a(tsan_interceptors.cc.o): warning: defined here
Looks like these warnings come from use of --warn-common, so I think we need to
exclude this when using TSan if we want to silence these warnings from
the TSan libraries.
Thanks & Regards,
-Rob
> /usr/lib/llvm-9/lib/clang/9.0.0/lib/linux/libclang_rt.tsan-x86_64.a(tsan_rtl_amd64.S.o): warning: common of `__interception::real__setjmp' overridden by definition
> /usr/lib/llvm-9/lib/clang/9.0.0/lib/linux/libclang_rt.tsan-x86_64.a(tsan_interceptors.cc.o): warning: defined here
> /usr/lib/llvm-9/lib/clang/9.0.0/lib/linux/libclang_rt.tsan-x86_64.a(tsan_rtl_amd64.S.o): warning: common of `__interception::real_sigsetjmp' overridden by definition
> /usr/lib/llvm-9/lib/clang/9.0.0/lib/linux/libclang_rt.tsan-x86_64.a(tsan_interceptors.cc.o): warning: defined here
> /usr/lib/llvm-9/lib/clang/9.0.0/lib/linux/libclang_rt.tsan-x86_64.a(tsan_rtl_amd64.S.o): warning: common of `__interception::real___sigsetjmp' overridden by definition
> /usr/lib/llvm-9/lib/clang/9.0.0/lib/linux/libclang_rt.tsan-x86_64.a(tsan_interceptors.cc.o): warning: defined here
> libqemuutil.a(osdep.o):/usr/lib/llvm-9/lib/clang/9.0.0/include/sanitizer/tsan_interface.h:41: multiple definition of `__tsan_mutex_linker_init'
> libqemuutil.a(control.o):/usr/lib/llvm-9/lib/clang/9.0.0/include/sanitizer/tsan_interface.h:41: first defined here
> libqemuutil.a(osdep.o):/usr/lib/llvm-9/lib/clang/9.0.0/include/sanitizer/tsan_interface.h:50: multiple definition of `__tsan_mutex_not_static'
> libqemuutil.a(control.o):/usr/lib/llvm-9/lib/clang/9.0.0/include/sanitizer/tsan_interface.h:50: first defined here
> libqemuutil.a(osdep.o):/usr/lib/llvm-9/lib/clang/9.0.0/include/sanitizer/tsan_interface.h:55: multiple definition of `__tsan_mutex_read_lock'
> libqemuutil.a(control.o):/usr/lib/llvm-9/lib/clang/9.0.0/include/sanitizer/tsan_interface.h:55: first defined here
> libqemuutil.a(osdep.o):/usr/lib/llvm-9/lib/clang/9.0.0/include/sanitizer/tsan_interface.h:45: multiple definition of `__tsan_mutex_read_reentrant'
> libqemuutil.a(control.o):/usr/lib/llvm-9/lib/clang/9.0.0/include/sanitizer/tsan_interface.h:45: first defined here
> libqemuutil.a(osdep.o):/usr/lib/llvm-9/lib/clang/9.0.0/include/sanitizer/tsan_interface.h:64: multiple definition of `__tsan_mutex_recursive_lock'
> libqemuutil.a(control.o):/usr/lib/llvm-9/lib/clang/9.0.0/include/sanitizer/tsan_interface.h:64: first defined here
> libqemuutil.a(osdep.o):/usr/lib/llvm-9/lib/clang/9.0.0/include/sanitizer/tsan_interface.h:68: multiple definition of `__tsan_mutex_recursive_unlock'
>
> <snip>
>
> --
> Alex Bennée
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2020-06-03 16:52 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-29 13:23 [PATCH v1 01/12] configure: add --enable-tsan flag + fiber annotations for coroutine-ucontext Robert Foley
2020-05-29 13:23 ` [PATCH v1 02/12] cpu: convert queued work to a QSIMPLEQ Robert Foley
2020-05-29 13:23 ` [PATCH v1 03/12] thread: add qemu_spin_destroy Robert Foley
2020-06-02 19:23 ` Alex Bennée
2020-05-29 13:23 ` [PATCH v1 04/12] cputlb: destroy CPUTLB with tlb_destroy Robert Foley
2020-06-02 19:25 ` Alex Bennée
2020-05-29 13:23 ` [PATCH v1 05/12] qht: call qemu_spin_destroy for head buckets Robert Foley
2020-06-02 19:26 ` Alex Bennée
2020-05-29 13:23 ` [PATCH v1 06/12] tcg: call qemu_spin_destroy for tb->jmp_lock Robert Foley
2020-05-29 13:23 ` [PATCH v1 07/12] translate-all: call qemu_spin_destroy for PageDesc Robert Foley
2020-05-29 13:23 ` [PATCH v1 08/12] thread: add tsan annotations to QemuSpin Robert Foley
2020-06-02 20:14 ` Alex Bennée
2020-05-29 13:23 ` [PATCH v1 09/12] tests/docker: Added docker build support for TSan Robert Foley
2020-06-02 20:21 ` Alex Bennée
2020-06-03 15:46 ` Robert Foley
2020-05-29 13:23 ` [PATCH v1 10/12] include/qemu: Added tsan.h for annotations Robert Foley
2020-05-29 13:23 ` [PATCH v1 11/12] util: Added tsan annotate for thread name Robert Foley
2020-06-01 3:56 ` Emilio G. Cota
2020-05-29 13:51 ` [PATCH v1 01/12] configure: add --enable-tsan flag + fiber annotations for coroutine-ucontext Eric Blake
2020-05-29 14:56 ` Robert Foley
2020-06-02 19:22 ` Alex Bennée
2020-06-03 16:51 ` Robert Foley
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).