diff mbox

[1/8] tls: require compiler support for __thread

Message ID 1421171537-19118-2-git-send-email-pbonzini@redhat.com
State New
Headers show

Commit Message

Paolo Bonzini Jan. 13, 2015, 5:52 p.m. UTC
The block layer is now using __thread unconditionally.  Remove the
"fake" TLS wrappers (that actually aren't TLS on !Linux) in
include/qemu/tls.h, and add a testcase.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 configure          |  9 +-----
 exec.c             |  2 +-
 include/qemu/tls.h | 52 ----------------------------------
 include/qom/cpu.h  |  4 +--
 tests/Makefile     |  7 ++++-
 tests/test-tls.c   | 83 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 92 insertions(+), 65 deletions(-)
 delete mode 100644 include/qemu/tls.h
 create mode 100644 tests/test-tls.c

Comments

Peter Maydell Jan. 13, 2015, 6:40 p.m. UTC | #1
On 13 January 2015 at 17:52, Paolo Bonzini <pbonzini@redhat.com> wrote:
> The block layer is now using __thread unconditionally.

Where? I did a quick grep for __thread and (other than stuff in
the *-user code and some Win32 specific files) there's no use of
__thread outside the DEFINE_TLS wrappers.

>  Remove the
> "fake" TLS wrappers (that actually aren't TLS on !Linux) in
> include/qemu/tls.h, and add a testcase.

What platforms have you tested on? We definitely shouldn't
widen our use of __thread without testing it on all the
platforms we support...

thanks
-- PMM
Paolo Bonzini Jan. 13, 2015, 7:48 p.m. UTC | #2
On 13/01/2015 19:40, Peter Maydell wrote:
> On 13 January 2015 at 17:52, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> The block layer is now using __thread unconditionally.
> 
> Where? I did a quick grep for __thread and (other than stuff in
> the *-user code and some Win32 specific files) there's no use of
> __thread outside the DEFINE_TLS wrappers.

It's in the pull request.

>>  Remove the
>> "fake" TLS wrappers (that actually aren't TLS on !Linux) in
>> include/qemu/tls.h, and add a testcase.
> 
> What platforms have you tested on? We definitely shouldn't
> widen our use of __thread without testing it on all the
> platforms we support...

Native TLS is supported by all platforms except Windows and, I think,
OpenBSD, which will have to use GCC's emulated TLS.  For Windows we
already do.  OpenBSD ports will have to use a new-enough GCC (basically
depend on the GPLv3 GCC port).

Paolo
Peter Maydell Jan. 13, 2015, 8 p.m. UTC | #3
On 13 January 2015 at 19:48, Paolo Bonzini <pbonzini@redhat.com> wrote:
> It's in the pull request.

That was sneaky :-)

> Native TLS is supported by all platforms except Windows and, I think,
> OpenBSD, which will have to use GCC's emulated TLS.  For Windows we
> already do.  OpenBSD ports will have to use a new-enough GCC (basically
> depend on the GPLv3 GCC port).

Hmm. That's a chunk of users who are now going to have to
change the way they've been building QEMU. Does configure
at least blow up on the old gcc, or will we just silently
build a non-working QEMU?

(I have an OpenBSD VM at home, I should test it...)

Presumably we've also just effectively dropped support for
some of the obscure stuff we have ifdefs for in configure?

-- PMM
Paolo Bonzini Jan. 13, 2015, 8:27 p.m. UTC | #4
On 13/01/2015 21:00, Peter Maydell wrote:
> Hmm. That's a chunk of users who are now going to have to
> change the way they've been building QEMU. Does configure
> at least blow up on the old gcc, or will we just silently
> build a non-working QEMU?

At least the build, if not configure, should blow up.

Paolo
Peter Maydell Jan. 13, 2015, 10:07 p.m. UTC | #5
On 13 January 2015 at 20:27, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 13/01/2015 21:00, Peter Maydell wrote:
>> Hmm. That's a chunk of users who are now going to have to
>> change the way they've been building QEMU. Does configure
>> at least blow up on the old gcc, or will we just silently
>> build a non-working QEMU?
>
> At least the build, if not configure, should blow up.

Yeah, it does:
qemu-coroutine.c:29: error: thread-local storage not supported for this target
(that's gcc 4.2.1 20070719 on OpenBSD 5.5.)

As a new requirement it might be polite to make configure
insist on it rather than failing the build later, maybe.

-- PMM
diff mbox

Patch

diff --git a/configure b/configure
index 7539645..0580606 100755
--- a/configure
+++ b/configure
@@ -1554,14 +1554,7 @@  fi
 
 if test "$pie" != "no" ; then
   cat > $TMPC << EOF
-
-#ifdef __linux__
-#  define THREAD __thread
-#else
-#  define THREAD
-#endif
-
-static THREAD int tls_var;
+static __thread int tls_var;
 
 int main(void) { return tls_var; }
 
diff --git a/exec.c b/exec.c
index 081818e..9046203 100644
--- a/exec.c
+++ b/exec.c
@@ -85,7 +85,7 @@  static MemoryRegion io_mem_unassigned;
 struct CPUTailQ cpus = QTAILQ_HEAD_INITIALIZER(cpus);
 /* current CPU in the current thread. It is only valid inside
    cpu_exec() */
-DEFINE_TLS(CPUState *, current_cpu);
+__thread CPUState *current_cpu;
 /* 0 = Do not count executed instructions.
    1 = Precise instruction counting.
    2 = Adaptive rate instruction counting.  */
diff --git a/include/qemu/tls.h b/include/qemu/tls.h
deleted file mode 100644
index b92ea9d..0000000
--- a/include/qemu/tls.h
+++ /dev/null
@@ -1,52 +0,0 @@ 
-/*
- * Abstraction layer for defining and using TLS variables
- *
- * Copyright (c) 2011 Red Hat, Inc
- * Copyright (c) 2011 Linaro Limited
- *
- * Authors:
- *  Paolo Bonzini <pbonzini@redhat.com>
- *  Peter Maydell <peter.maydell@linaro.org>
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of the GNU General Public License as
- * published by the Free Software Foundation; either version 2 of
- * the License, or (at your option) any later version.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- *
- * You should have received a copy of the GNU General Public License along
- * with this program; if not, see <http://www.gnu.org/licenses/>.
- */
-
-#ifndef QEMU_TLS_H
-#define QEMU_TLS_H
-
-/* Per-thread variables. Note that we only have implementations
- * which are really thread-local on Linux; the dummy implementations
- * define plain global variables.
- *
- * This means that for the moment use should be restricted to
- * per-VCPU variables, which are OK because:
- *  - the only -user mode supporting multiple VCPU threads is linux-user
- *  - TCG system mode is single-threaded regarding VCPUs
- *  - KVM system mode is multi-threaded but limited to Linux
- *
- * TODO: proper implementations via Win32 .tls sections and
- * POSIX pthread_getspecific.
- */
-#ifdef __linux__
-#define DECLARE_TLS(type, x) extern DEFINE_TLS(type, x)
-#define DEFINE_TLS(type, x)  __thread __typeof__(type) tls__##x
-#define tls_var(x)           tls__##x
-#else
-/* Dummy implementations which define plain global variables */
-#define DECLARE_TLS(type, x) extern DEFINE_TLS(type, x)
-#define DEFINE_TLS(type, x)  __typeof__(type) tls__##x
-#define tls_var(x)           tls__##x
-#endif
-
-#endif
diff --git a/include/qom/cpu.h b/include/qom/cpu.h
index 2098f1c..a93466d 100644
--- a/include/qom/cpu.h
+++ b/include/qom/cpu.h
@@ -26,7 +26,6 @@ 
 #include "exec/hwaddr.h"
 #include "qemu/queue.h"
 #include "qemu/thread.h"
-#include "qemu/tls.h"
 #include "qemu/typedefs.h"
 
 typedef int (*WriteCoreDumpFunction)(const void *buf, size_t size,
@@ -310,8 +309,7 @@  extern struct CPUTailQ cpus;
     QTAILQ_FOREACH_SAFE(cpu, &cpus, node, next_cpu)
 #define first_cpu QTAILQ_FIRST(&cpus)
 
-DECLARE_TLS(CPUState *, current_cpu);
-#define current_cpu tls_var(current_cpu)
+extern __thread CPUState *current_cpu;
 
 /**
  * cpu_paging_enabled:
diff --git a/tests/Makefile b/tests/Makefile
index e4ddb6a..47ba143 100644
--- a/tests/Makefile
+++ b/tests/Makefile
@@ -57,6 +57,9 @@  check-unit-y += tests/test-cutils$(EXESUF)
 gcov-files-test-cutils-y += util/cutils.c
 check-unit-y += tests/test-mul64$(EXESUF)
 gcov-files-test-mul64-y = util/host-utils.c
+check-unit-y += tests/test-tls$(EXESUF)
+# all code tested by test-tls is inside test-tls.c
+gcov-files-test-tls-y =
 check-unit-y += tests/test-int128$(EXESUF)
 # all code tested by test-int128 is inside int128.h
 gcov-files-test-int128-y =
@@ -223,7 +226,7 @@  test-obj-y = tests/check-qint.o tests/check-qstring.o tests/check-qdict.o \
 	tests/test-qmp-input-visitor.o tests/test-qmp-input-strict.o \
 	tests/test-qmp-commands.o tests/test-visitor-serialization.o \
 	tests/test-x86-cpuid.o tests/test-mul64.o tests/test-int128.o \
-	tests/test-opts-visitor.o tests/test-qmp-event.o
+	tests/test-opts-visitor.o tests/test-qmp-event.o tests/test-tls.o
 
 test-qapi-obj-y = tests/test-qapi-visit.o tests/test-qapi-types.o \
 		  tests/test-qapi-event.o
@@ -252,6 +255,8 @@  tests/test-x86-cpuid$(EXESUF): tests/test-x86-cpuid.o
 tests/test-xbzrle$(EXESUF): tests/test-xbzrle.o migration/xbzrle.o page_cache.o libqemuutil.a
 tests/test-cutils$(EXESUF): tests/test-cutils.o util/cutils.o
 tests/test-int128$(EXESUF): tests/test-int128.o
+tests/test-tls$(EXESUF): tests/test-tls.o libqemuutil.a
+
 tests/test-qdev-global-props$(EXESUF): tests/test-qdev-global-props.o \
 	hw/core/qdev.o hw/core/qdev-properties.o hw/core/hotplug.o\
 	hw/core/irq.o \
diff --git a/tests/test-tls.c b/tests/test-tls.c
new file mode 100644
index 0000000..f798f74
--- /dev/null
+++ b/tests/test-tls.c
@@ -0,0 +1,83 @@ 
+/*
+ * Unit-tests for compiler-provided TLS
+ *
+ * Copyright (C) 2015 Red Hat Inc.
+ *
+ * Authors:
+ *  Paolo Bonzini <pbonzini@redhat.com>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a copy
+ * of this software and associated documentation files (the "Software"), to deal
+ * in the Software without restriction, including without limitation the rights
+ * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+ * copies of the Software, and to permit persons to whom the Software is
+ * furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+ * THE SOFTWARE.
+ */
+
+#include <glib.h>
+#include <errno.h>
+#include <string.h>
+
+#include "qemu-common.h"
+#include "qemu/atomic.h"
+#include "qemu/thread.h"
+
+__thread volatile long long cnt;
+
+#define NUM_THREADS 10
+
+int stop;
+
+static void *test_thread(void *arg)
+{
+    volatile long long **p_ret = arg;
+    long long exp = 0;
+
+    *p_ret = &cnt;
+    g_assert(cnt == 0);
+    while (atomic_mb_read(&stop) == 0) {
+        exp++;
+        cnt++;
+        g_assert(cnt == exp);
+    }
+
+    return NULL;
+}
+
+static void test_tls(void)
+{
+    volatile long long *addr[NUM_THREADS];
+    QemuThread t[NUM_THREADS];
+    int i;
+
+    for (i = 0; i < NUM_THREADS; i++) {
+        qemu_thread_create(&t[i], "test", test_thread, &addr[i], QEMU_THREAD_JOINABLE);
+    }
+    g_usleep(1000000);
+    atomic_mb_set(&stop, 1);
+    for (i = 0; i < NUM_THREADS; i++) {
+        qemu_thread_join(&t[i]);
+    }
+    for (i = 1; i < NUM_THREADS; i++) {
+        g_assert(addr[i] != addr[i - 1]);
+    }
+}
+
+int main(int argc, char **argv)
+{
+    g_test_init(&argc, &argv, NULL);
+
+    g_test_add_func("/tls", test_tls);
+    return g_test_run();
+}