[Qemu-devel] [PATCH 0/2] Add TCG sanity checks (goto_tb related)

[Qemu-devel] [PATCH 0/2] Add TCG sanity checks (goto_tb related)

[Max Filippov]

Does this look sane or should it better be merged with e.g. tcg_dump_ops?

Max Filippov (2):
  tcg/README: document tcg_gen_goto_tb restrictions
  tcg: add TB sanity checking

 tcg/README |    3 +-
 tcg/tcg.c  |   69 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 71 insertions(+), 1 deletions(-)

-- 
1.7.7.6

[Qemu-devel] [PATCH 1/2] tcg/README: document tcg_gen_goto_tb restrictions

[Max Filippov]

See
http://lists.nongnu.org/archive/html/qemu-devel/2012-09/msg03196.html
for the whole story.

Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
---
 tcg/README |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/tcg/README b/tcg/README
index cfdfd96..86b43f1 100644
--- a/tcg/README
+++ b/tcg/README
@@ -386,7 +386,8 @@ Exit the current TB and return the value t0 (word type).
 
 Exit the current TB and jump to the TB index 'index' (constant) if the
 current TB was linked to this TB. Otherwise execute the next
-instructions.
+instructions. Only indices 0 and 1 are valid and tcg_gen_goto_tb may be issued
+at most once with each slot index per TB.
 
 * qemu_ld8u t0, t1, flags
 qemu_ld8s t0, t1, flags
-- 
1.7.7.6

[Qemu-devel] [PATCH 2/2] tcg: add TB sanity checking

[Max Filippov]

Do a sanity checking pass on the intermediate code.
Check that goto_tb indices are either 0 or 1 and used at most once per
TB.

Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
---
 tcg/tcg.c |   69 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 69 insertions(+), 0 deletions(-)

diff --git a/tcg/tcg.c b/tcg/tcg.c
index b8a1bec..cdd1975 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -1454,6 +1454,71 @@ static void check_regs(TCGContext *s)
 }
 #endif
 
+#ifdef CONFIG_DEBUG_TCG
+static void tcg_sanity_check(TCGContext *s)
+{
+    const uint16_t *opc_ptr;
+    const TCGArg *args;
+    TCGArg arg;
+    TCGOpcode c;
+    int nb_oargs, nb_iargs, nb_cargs, error_count = 0;
+    const TCGOpDef *def;
+    unsigned goto_tb_slots[2] = {0};
+
+    opc_ptr = gen_opc_buf;
+    args = gen_opparam_buf;
+    while (opc_ptr < gen_opc_ptr) {
+        c = *opc_ptr++;
+        def = &tcg_op_defs[c];
+        if (c == INDEX_op_call) {
+            TCGArg arg;
+
+            /* variable number of arguments */
+            arg = *args++;
+            nb_oargs = arg >> 16;
+            nb_iargs = arg & 0xffff;
+            nb_cargs = def->nb_cargs;
+        } else {
+            if (c == INDEX_op_nopn) {
+                /* variable number of arguments */
+                nb_cargs = *args;
+                nb_oargs = 0;
+                nb_iargs = 0;
+            } else {
+                nb_oargs = def->nb_oargs;
+                nb_iargs = def->nb_iargs;
+                nb_cargs = def->nb_cargs;
+            }
+        }
+
+        switch (c) {
+        case INDEX_op_goto_tb:
+            arg = args[0];
+            if (arg != 0 && arg != 1) {
+                qemu_log("TB ERROR: wrong goto_tb slot index: %"TCG_PRIlx"\n",
+                        arg);
+                ++error_count;
+            } else {
+                ++goto_tb_slots[arg];
+                if (goto_tb_slots[arg] > 1) {
+                    qemu_log("TB ERROR: multiple goto_tb(%"TCG_PRIlx")\n", arg);
+                    ++error_count;
+                }
+            }
+            break;
+
+        default:
+            break;
+        }
+
+        args += nb_iargs + nb_oargs + nb_cargs;
+    }
+    if (error_count) {
+        qemu_log("\n");
+    }
+}
+#endif
+
 static void temp_allocate_frame(TCGContext *s, int temp)
 {
     TCGTemp *ts;
@@ -2082,6 +2147,10 @@ static inline int tcg_gen_code_common(TCGContext *s, uint8_t *gen_code_buf,
     }
 #endif
 
+#ifdef CONFIG_DEBUG_TCG
+    tcg_sanity_check(s);
+#endif
+
     tcg_reg_alloc_start(s);
 
     s->code_buf = gen_code_buf;
-- 
1.7.7.6

Re: [Qemu-devel] [PATCH 2/2] tcg: add TB sanity checking

[Michael Tokarev]

On 21.09.2012 04:18, Max Filippov wrote:

> diff --git a/tcg/tcg.c b/tcg/tcg.c

> +#ifdef CONFIG_DEBUG_TCG
> +static void tcg_sanity_check(TCGContext *s)

#ifndef CONFIG_DEBUG_TCG
#define tcg_sanity_check(s) /*empty*/
#else
static void tcg_sanity_check(TCGContext *s)

> +{
[]
> +}
> +#endif

> @@ -2082,6 +2147,10 @@ static inline int tcg_gen_code_common(TCGContext *s, uint8_t *gen_code_buf,
> +#ifdef CONFIG_DEBUG_TCG
> +    tcg_sanity_check(s);
> +#endif

And here we can drop the #ifdef.  FWIW.

/mjt

Re: [Qemu-devel] [PATCH 1/2] tcg/README: document tcg_gen_goto_tb restrictions

[Aurelien Jarno]

On Fri, Sep 21, 2012 at 04:18:07AM +0400, Max Filippov wrote:
> See
> http://lists.nongnu.org/archive/html/qemu-devel/2012-09/msg03196.html
> for the whole story.
> 
> Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
> ---
>  tcg/README |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/tcg/README b/tcg/README
> index cfdfd96..86b43f1 100644
> --- a/tcg/README
> +++ b/tcg/README
> @@ -386,7 +386,8 @@ Exit the current TB and return the value t0 (word type).
>  
>  Exit the current TB and jump to the TB index 'index' (constant) if the
>  current TB was linked to this TB. Otherwise execute the next
> -instructions.
> +instructions. Only indices 0 and 1 are valid and tcg_gen_goto_tb may be issued
> +at most once with each slot index per TB.
>  
>  * qemu_ld8u t0, t1, flags
>  qemu_ld8s t0, t1, flags
> -- 
> 1.7.7.6
> 

Thanks, applied.


-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH 2/2] tcg: add TB sanity checking

[Aurelien Jarno]

On Fri, Sep 21, 2012 at 04:18:08AM +0400, Max Filippov wrote:
> Do a sanity checking pass on the intermediate code.
> Check that goto_tb indices are either 0 or 1 and used at most once per
> TB.
> 
> Signed-off-by: Max Filippov <jcmvbkbc@gmail.com>
> ---
>  tcg/tcg.c |   69 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 69 insertions(+), 0 deletions(-)
> 
> diff --git a/tcg/tcg.c b/tcg/tcg.c
> index b8a1bec..cdd1975 100644
> --- a/tcg/tcg.c
> +++ b/tcg/tcg.c
> @@ -1454,6 +1454,71 @@ static void check_regs(TCGContext *s)
>  }
>  #endif
>  
> +#ifdef CONFIG_DEBUG_TCG
> +static void tcg_sanity_check(TCGContext *s)
> +{
> +    const uint16_t *opc_ptr;
> +    const TCGArg *args;
> +    TCGArg arg;
> +    TCGOpcode c;
> +    int nb_oargs, nb_iargs, nb_cargs, error_count = 0;
> +    const TCGOpDef *def;
> +    unsigned goto_tb_slots[2] = {0};
> +
> +    opc_ptr = gen_opc_buf;
> +    args = gen_opparam_buf;
> +    while (opc_ptr < gen_opc_ptr) {
> +        c = *opc_ptr++;
> +        def = &tcg_op_defs[c];
> +        if (c == INDEX_op_call) {
> +            TCGArg arg;
> +
> +            /* variable number of arguments */
> +            arg = *args++;
> +            nb_oargs = arg >> 16;
> +            nb_iargs = arg & 0xffff;
> +            nb_cargs = def->nb_cargs;
> +        } else {
> +            if (c == INDEX_op_nopn) {
> +                /* variable number of arguments */
> +                nb_cargs = *args;
> +                nb_oargs = 0;
> +                nb_iargs = 0;
> +            } else {
> +                nb_oargs = def->nb_oargs;
> +                nb_iargs = def->nb_iargs;
> +                nb_cargs = def->nb_cargs;
> +            }
> +        }
> +
> +        switch (c) {
> +        case INDEX_op_goto_tb:
> +            arg = args[0];
> +            if (arg != 0 && arg != 1) {
> +                qemu_log("TB ERROR: wrong goto_tb slot index: %"TCG_PRIlx"\n",
> +                        arg);
> +                ++error_count;
> +            } else {
> +                ++goto_tb_slots[arg];
> +                if (goto_tb_slots[arg] > 1) {
> +                    qemu_log("TB ERROR: multiple goto_tb(%"TCG_PRIlx")\n", arg);
> +                    ++error_count;
> +                }
> +            }
> +            break;
> +
> +        default:
> +            break;
> +        }
> +
> +        args += nb_iargs + nb_oargs + nb_cargs;
> +    }
> +    if (error_count) {
> +        qemu_log("\n");
> +    }
> +}
> +#endif
> +
>  static void temp_allocate_frame(TCGContext *s, int temp)
>  {
>      TCGTemp *ts;
> @@ -2082,6 +2147,10 @@ static inline int tcg_gen_code_common(TCGContext *s, uint8_t *gen_code_buf,
>      }
>  #endif
>  
> +#ifdef CONFIG_DEBUG_TCG
> +    tcg_sanity_check(s);
> +#endif
> +
>      tcg_reg_alloc_start(s);
>  
>      s->code_buf = gen_code_buf;

I think this is better address in the patch from Richard Henderson.


-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net