[Qemu-devel] [PATCH 0/2] Always enable TCGv type checking

[Qemu-devel] [PATCH 0/2] Always enable TCGv type checking

[Richard Henderson]

We've had a number of bugs slip through recently that would have
been caught by --enable-tcg-debug.


r~


Richard Henderson (2):
  qemu/compiler: Define QEMU_ARTIFICIAL
  tcg: Always enable TCGv type checking

 include/qemu/compiler.h |  6 ++++
 tcg/tcg.h               | 89 +++++++++++++++++++------------------------------
 2 files changed, 40 insertions(+), 55 deletions(-)

-- 
1.9.3

[Qemu-devel] [PATCH 1/2] qemu/compiler: Define QEMU_ARTIFICIAL

[Richard Henderson]

The combination of always_inline + artificial allows tiny inline
functions to be written that do not interfere with debugging.
In particular, gdb will not step into an artificial function.

The always_inline attribute was introduced in gcc 4.2,
and the artificial attribute was introduced in gcc 4.3.

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 include/qemu/compiler.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/qemu/compiler.h b/include/qemu/compiler.h
index 155b358..ac7c4c4 100644
--- a/include/qemu/compiler.h
+++ b/include/qemu/compiler.h
@@ -24,6 +24,12 @@
 #define QEMU_WARN_UNUSED_RESULT
 #endif
 
+#if QEMU_GNUC_PREREQ(4, 3)
+#define QEMU_ARTIFICIAL __attribute__((always_inline, artificial))
+#else
+#define QEMU_ARTIFICIAL
+#endif
+
 #if defined(_WIN32)
 # define QEMU_PACKED __attribute__((gcc_struct, packed))
 #else
-- 
1.9.3

Re: [Qemu-devel] [PATCH 1/2] qemu/compiler: Define QEMU_ARTIFICIAL

[Peter Maydell]

On 16 September 2014 10:46, Richard Henderson <rth@twiddle.net> wrote:
> The combination of always_inline + artificial allows tiny inline
> functions to be written that do not interfere with debugging.
> In particular, gdb will not step into an artificial function.
>
> The always_inline attribute was introduced in gcc 4.2,
> and the artificial attribute was introduced in gcc 4.3.
>
> Signed-off-by: Richard Henderson <rth@twiddle.net>

clang can't handle the "artificial" attribute but it reports
itself as gcc 4.2 so that's OK.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM

[Qemu-devel] [PATCH 2/2] tcg: Always enable TCGv type checking

[Richard Henderson]

Instead of using structures, which imply some amount of overhead
on certain ABIs, use pointer types.

This actually reduces the size of the binaries vs a NON-debug
build on ppc64 and x86_64, due to a reduction in the number of
sign-extension insns.

Signed-off-by: Richard Henderson <rth@twiddle.net>
---
 tcg/tcg.h | 89 ++++++++++++++++++++++++---------------------------------------
 1 file changed, 34 insertions(+), 55 deletions(-)

diff --git a/tcg/tcg.h b/tcg/tcg.h
index 997a704..7285f71 100644
--- a/tcg/tcg.h
+++ b/tcg/tcg.h
@@ -274,75 +274,54 @@ typedef enum TCGMemOp {
 
 typedef tcg_target_ulong TCGArg;
 
-/* Define a type and accessor macros for variables.  Using a struct is
-   nice because it gives some level of type safely.  Ideally the compiler
-   be able to see through all this.  However in practice this is not true,
-   especially on targets with braindamaged ABIs (e.g. i386).
-   We use plain int by default to avoid this runtime overhead.
-   Users of tcg_gen_* don't need to know about any of this, and should
-   treat TCGv as an opaque type.
+/* Define a type and accessor macros for variables.  Using pointer types
+   is nice because it gives some level of type safely.  Converting to and
+   from intptr_t rather than int reduces the number of sign-extension
+   instructions that get implied on 64-bit hosts.  Users of tcg_gen_* don't
+   need to know about any of this, and should treat TCGv as an opaque type.
    In addition we do typechecking for different types of variables.  TCGv_i32
    and TCGv_i64 are 32/64-bit variables respectively.  TCGv and TCGv_ptr
-   are aliases for target_ulong and host pointer sized values respectively.
- */
+   are aliases for target_ulong and host pointer sized values respectively.  */
 
-#ifdef CONFIG_DEBUG_TCG
-#define DEBUG_TCGV 1
-#endif
+typedef struct TCGv_i32_d *TCGv_i32;
+typedef struct TCGv_i64_d *TCGv_i64;
+typedef struct TCGv_ptr_d *TCGv_ptr;
 
-#ifdef DEBUG_TCGV
+static inline TCGv_i32 QEMU_ARTIFICIAL MAKE_TCGV_I32(intptr_t i)
+{
+    return (TCGv_i32)i;
+}
 
-typedef struct
+static inline TCGv_i64 QEMU_ARTIFICIAL MAKE_TCGV_I64(intptr_t i)
 {
-    int i32;
-} TCGv_i32;
+    return (TCGv_i64)i;
+}
 
-typedef struct
+static inline TCGv_ptr QEMU_ARTIFICIAL MAKE_TCGV_PTR(intptr_t i)
 {
-    int i64;
-} TCGv_i64;
-
-typedef struct {
-    int iptr;
-} TCGv_ptr;
-
-#define MAKE_TCGV_I32(i) __extension__                  \
-    ({ TCGv_i32 make_tcgv_tmp = {i}; make_tcgv_tmp;})
-#define MAKE_TCGV_I64(i) __extension__                  \
-    ({ TCGv_i64 make_tcgv_tmp = {i}; make_tcgv_tmp;})
-#define MAKE_TCGV_PTR(i) __extension__                  \
-    ({ TCGv_ptr make_tcgv_tmp = {i}; make_tcgv_tmp; })
-#define GET_TCGV_I32(t) ((t).i32)
-#define GET_TCGV_I64(t) ((t).i64)
-#define GET_TCGV_PTR(t) ((t).iptr)
-#if TCG_TARGET_REG_BITS == 32
-#define TCGV_LOW(t) MAKE_TCGV_I32(GET_TCGV_I64(t))
-#define TCGV_HIGH(t) MAKE_TCGV_I32(GET_TCGV_I64(t) + 1)
-#endif
+    return (TCGv_ptr)i;
+}
+
+static inline intptr_t QEMU_ARTIFICIAL GET_TCGV_I32(TCGv_i32 t)
+{
+    return (intptr_t)t;
+}
 
-#else /* !DEBUG_TCGV */
+static inline intptr_t QEMU_ARTIFICIAL GET_TCGV_I64(TCGv_i64 t)
+{
+    return (intptr_t)t;
+}
 
-typedef int TCGv_i32;
-typedef int TCGv_i64;
-#if TCG_TARGET_REG_BITS == 32
-#define TCGv_ptr TCGv_i32
-#else
-#define TCGv_ptr TCGv_i64
-#endif
-#define MAKE_TCGV_I32(x) (x)
-#define MAKE_TCGV_I64(x) (x)
-#define MAKE_TCGV_PTR(x) (x)
-#define GET_TCGV_I32(t) (t)
-#define GET_TCGV_I64(t) (t)
-#define GET_TCGV_PTR(t) (t)
+static inline intptr_t QEMU_ARTIFICIAL GET_TCGV_PTR(TCGv_ptr t)
+{
+    return (intptr_t)t;
+}
 
 #if TCG_TARGET_REG_BITS == 32
-#define TCGV_LOW(t) (t)
-#define TCGV_HIGH(t) ((t) + 1)
+#define TCGV_LOW(t) MAKE_TCGV_I32(GET_TCGV_I64(t))
+#define TCGV_HIGH(t) MAKE_TCGV_I32(GET_TCGV_I64(t) + 1)
 #endif
 
-#endif /* DEBUG_TCGV */
-
 #define TCGV_EQUAL_I32(a, b) (GET_TCGV_I32(a) == GET_TCGV_I32(b))
 #define TCGV_EQUAL_I64(a, b) (GET_TCGV_I64(a) == GET_TCGV_I64(b))
 #define TCGV_EQUAL_PTR(a, b) (GET_TCGV_PTR(a) == GET_TCGV_PTR(b))
-- 
1.9.3

Re: [Qemu-devel] [PATCH 2/2] tcg: Always enable TCGv type checking

[Peter Maydell]

On 16 September 2014 10:46, Richard Henderson <rth@twiddle.net> wrote:
> Instead of using structures, which imply some amount of overhead
> on certain ABIs, use pointer types.
>
> This actually reduces the size of the binaries vs a NON-debug
> build on ppc64 and x86_64, due to a reduction in the number of
> sign-extension insns.

It's good to have this typechecking enabled unconditionally.

> Signed-off-by: Richard Henderson <rth@twiddle.net>
> ---
>  tcg/tcg.h | 89 ++++++++++++++++++++++++---------------------------------------
>  1 file changed, 34 insertions(+), 55 deletions(-)
>
> diff --git a/tcg/tcg.h b/tcg/tcg.h
> index 997a704..7285f71 100644
> --- a/tcg/tcg.h
> +++ b/tcg/tcg.h
> @@ -274,75 +274,54 @@ typedef enum TCGMemOp {
>
>  typedef tcg_target_ulong TCGArg;
>
> -/* Define a type and accessor macros for variables.  Using a struct is
> -   nice because it gives some level of type safely.  Ideally the compiler
> -   be able to see through all this.  However in practice this is not true,
> -   especially on targets with braindamaged ABIs (e.g. i386).
> -   We use plain int by default to avoid this runtime overhead.
> -   Users of tcg_gen_* don't need to know about any of this, and should
> -   treat TCGv as an opaque type.
> +/* Define a type and accessor macros for variables.  Using pointer types
> +   is nice because it gives some level of type safely.  Converting to and

"safety".

Otherwise
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

-- PMM