[Qemu-devel] [PATCH] tcg: Diagnose referenced labels that have not been emitted

[Qemu-devel] [PATCH] tcg: Diagnose referenced labels that have not been emitted

[Richard Henderson]

Currently, a jump to a label that is not defined anywhere will
be emitted not be relocated.  This results in a jump to a random
jump target.  With tcg debugging, print a diagnostic to the -d op
file and abort.

This could help debug or detect errors like
c2d9644e6d ("target/arm: Fix crash on conditional instruction in an IT block")

Reported-by: Roman Kapl <code@rkapl.cz>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 tcg/tcg-op.h |  1 +
 tcg/tcg.h    | 12 +++++++++---
 tcg/tcg.c    | 23 +++++++++++++++++++++++
 3 files changed, 33 insertions(+), 3 deletions(-)
---
This detects errors earlier than the patch that Roman posted.


r~


diff --git a/tcg/tcg-op.h b/tcg/tcg-op.h
index 2d98868d8f..d3e51b15af 100644
--- a/tcg/tcg-op.h
+++ b/tcg/tcg-op.h
@@ -255,6 +255,7 @@ static inline void tcg_gen_op6ii_i64(TCGOpcode opc, TCGv_i64 a1, TCGv_i64 a2,
 
 static inline void gen_set_label(TCGLabel *l)
 {
+    l->present = 1;
     tcg_gen_op1(INDEX_op_set_label, label_arg(l));
 }
 
diff --git a/tcg/tcg.h b/tcg/tcg.h
index 045c24a357..32b7cf3489 100644
--- a/tcg/tcg.h
+++ b/tcg/tcg.h
@@ -244,16 +244,21 @@ typedef struct TCGRelocation {
     intptr_t addend;
 } TCGRelocation; 
 
-typedef struct TCGLabel {
+typedef struct TCGLabel TCGLabel;
+struct TCGLabel {
+    unsigned present : 1;
     unsigned has_value : 1;
-    unsigned id : 15;
+    unsigned id : 14;
     unsigned refs : 16;
     union {
         uintptr_t value;
         tcg_insn_unit *value_ptr;
         TCGRelocation *first_reloc;
     } u;
-} TCGLabel;
+#ifdef CONFIG_DEBUG_TCG
+    QSIMPLEQ_ENTRY(TCGLabel) next;
+#endif
+};
 
 typedef struct TCGPool {
     struct TCGPool *next;
@@ -685,6 +690,7 @@ struct TCGContext {
 #endif
 
 #ifdef CONFIG_DEBUG_TCG
+    QSIMPLEQ_HEAD(, TCGLabel) labels;
     int temps_in_use;
     int goto_tb_issue_mask;
 #endif
diff --git a/tcg/tcg.c b/tcg/tcg.c
index 20a5d8f315..9b2bf7f439 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -305,6 +305,9 @@ TCGLabel *gen_new_label(void)
     *l = (TCGLabel){
         .id = s->nb_labels++
     };
+#ifdef CONFIG_DEBUG_TCG
+    QSIMPLEQ_INSERT_TAIL(&s->labels, l, next);
+#endif
 
     return l;
 }
@@ -1092,6 +1095,9 @@ void tcg_func_start(TCGContext *s)
 
     QTAILQ_INIT(&s->ops);
     QTAILQ_INIT(&s->free_ops);
+#ifdef CONFIG_DEBUG_TCG
+    QSIMPLEQ_INIT(&s->labels);
+#endif
 }
 
 static inline TCGTemp *tcg_temp_alloc(TCGContext *s)
@@ -3841,6 +3847,23 @@ int tcg_gen_code(TCGContext *s, TranslationBlock *tb)
     }
 #endif
 
+#ifdef CONFIG_DEBUG_TCG
+    /* Ensure all labels referenced have been emitted.  */
+    {
+        TCGLabel *l;
+        bool error = false;
+
+        QSIMPLEQ_FOREACH(l, &s->labels, next) {
+            if (unlikely(!l->present) && l->refs) {
+                qemu_log_mask(CPU_LOG_TB_OP,
+                              "$L%d referenced but not present.\n", l->id);
+                error = true;
+            }
+        }
+        assert(!error);
+    }
+#endif
+
 #ifdef CONFIG_PROFILER
     atomic_set(&prof->opt_time, prof->opt_time - profile_getclock());
 #endif
-- 
2.17.2

Re: [Qemu-devel] [PATCH] tcg: Diagnose referenced labels that have not been emitted

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190208035551.3036-1-richard.henderson@linaro.org/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [Qemu-devel] [PATCH] tcg: Diagnose referenced labels that have not been emitted
Type: series
Message-id: 20190208035551.3036-1-richard.henderson@linaro.org

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]               patchew/20190208035551.3036-1-richard.henderson@linaro.org -> patchew/20190208035551.3036-1-richard.henderson@linaro.org
Switched to a new branch 'test'
702a152f7d tcg: Diagnose referenced labels that have not been emitted

=== OUTPUT BEGIN ===
ERROR: spaces prohibited around that ':' (ctx:WxW)
#90: FILE: tcg/tcg.h:249:
+    unsigned present : 1;
                      ^

ERROR: spaces prohibited around that ':' (ctx:WxW)
#93: FILE: tcg/tcg.h:251:
+    unsigned id : 14;
                 ^

total: 2 errors, 0 warnings, 79 lines checked

Commit 702a152f7d06 (tcg: Diagnose referenced labels that have not been emitted) has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20190208035551.3036-1-richard.henderson@linaro.org/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH] tcg: Diagnose referenced labels that have not been emitted

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190208035551.3036-1-richard.henderson@linaro.org/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Subject: [Qemu-devel] [PATCH] tcg: Diagnose referenced labels that have not been emitted
Message-id: 20190208035551.3036-1-richard.henderson@linaro.org

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20190208035551.3036-1-richard.henderson@linaro.org -> patchew/20190208035551.3036-1-richard.henderson@linaro.org
Switched to a new branch 'test'
702a152 tcg: Diagnose referenced labels that have not been emitted

=== OUTPUT BEGIN ===
ERROR: spaces prohibited around that ':' (ctx:WxW)
#90: FILE: tcg/tcg.h:249:
+    unsigned present : 1;
                      ^

ERROR: spaces prohibited around that ':' (ctx:WxW)
#93: FILE: tcg/tcg.h:251:
+    unsigned id : 14;
                 ^

total: 2 errors, 0 warnings, 79 lines checked

Commit 702a152f7d06 (tcg: Diagnose referenced labels that have not been emitted) has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20190208035551.3036-1-richard.henderson@linaro.org/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH] tcg: Diagnose referenced labels that have not been emitted

[no-reply]

Patchew URL: https://patchew.org/QEMU/20190208035551.3036-1-richard.henderson@linaro.org/



Hi,

This series seems to have some coding style problems. See output below for
more information:

Subject: [Qemu-devel] [PATCH] tcg: Diagnose referenced labels that have not been emitted
Message-id: 20190208035551.3036-1-richard.henderson@linaro.org
Type: series

=== TEST SCRIPT BEGIN ===
#!/bin/bash
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
./scripts/checkpatch.pl --mailback base..
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/20190208035551.3036-1-richard.henderson@linaro.org -> patchew/20190208035551.3036-1-richard.henderson@linaro.org
Submodule 'capstone' (https://git.qemu.org/git/capstone.git) registered for path 'capstone'
Submodule 'dtc' (https://git.qemu.org/git/dtc.git) registered for path 'dtc'
Submodule 'roms/QemuMacDrivers' (https://git.qemu.org/git/QemuMacDrivers.git) registered for path 'roms/QemuMacDrivers'
Submodule 'roms/SLOF' (https://git.qemu.org/git/SLOF.git) registered for path 'roms/SLOF'
Submodule 'roms/ipxe' (https://git.qemu.org/git/ipxe.git) registered for path 'roms/ipxe'
Submodule 'roms/openbios' (https://git.qemu.org/git/openbios.git) registered for path 'roms/openbios'
Submodule 'roms/openhackware' (https://git.qemu.org/git/openhackware.git) registered for path 'roms/openhackware'
Submodule 'roms/qemu-palcode' (https://git.qemu.org/git/qemu-palcode.git) registered for path 'roms/qemu-palcode'
Submodule 'roms/seabios' (https://git.qemu.org/git/seabios.git/) registered for path 'roms/seabios'
Submodule 'roms/seabios-hppa' (https://github.com/hdeller/seabios-hppa.git) registered for path 'roms/seabios-hppa'
Submodule 'roms/sgabios' (https://git.qemu.org/git/sgabios.git) registered for path 'roms/sgabios'
Submodule 'roms/skiboot' (https://git.qemu.org/git/skiboot.git) registered for path 'roms/skiboot'
Submodule 'roms/u-boot' (https://git.qemu.org/git/u-boot.git) registered for path 'roms/u-boot'
Submodule 'roms/u-boot-sam460ex' (https://git.qemu.org/git/u-boot-sam460ex.git) registered for path 'roms/u-boot-sam460ex'
Submodule 'tests/fp/berkeley-softfloat-3' (https://github.com/cota/berkeley-softfloat-3) registered for path 'tests/fp/berkeley-softfloat-3'
Submodule 'tests/fp/berkeley-testfloat-3' (https://github.com/cota/berkeley-testfloat-3) registered for path 'tests/fp/berkeley-testfloat-3'
Submodule 'ui/keycodemapdb' (https://git.qemu.org/git/keycodemapdb.git) registered for path 'ui/keycodemapdb'
Cloning into 'capstone'...
Submodule path 'capstone': checked out '22ead3e0bfdb87516656453336160e0a37b066bf'
Cloning into 'dtc'...
Submodule path 'dtc': checked out '88f18909db731a627456f26d779445f84e449536'
Cloning into 'roms/QemuMacDrivers'...
Submodule path 'roms/QemuMacDrivers': checked out '90c488d5f4a407342247b9ea869df1c2d9c8e266'
Cloning into 'roms/SLOF'...
Submodule path 'roms/SLOF': checked out 'a5b428e1c1eae703bdd62a3f527223c291ee3fdc'
Cloning into 'roms/ipxe'...
Submodule path 'roms/ipxe': checked out 'de4565cbe76ea9f7913a01f331be3ee901bb6e17'
Cloning into 'roms/openbios'...
Submodule path 'roms/openbios': checked out '441a84d3a642a10b948369c63f32367e8ff6395b'
Cloning into 'roms/openhackware'...
Submodule path 'roms/openhackware': checked out 'c559da7c8eec5e45ef1f67978827af6f0b9546f5'
Cloning into 'roms/qemu-palcode'...
Submodule path 'roms/qemu-palcode': checked out '51c237d7e20d05100eacadee2f61abc17e6bc097'
Cloning into 'roms/seabios'...
Submodule path 'roms/seabios': checked out 'a698c8995ffb2838296ec284fe3c4ad33dfca307'
Cloning into 'roms/seabios-hppa'...
Submodule path 'roms/seabios-hppa': checked out '1ef99a01572c2581c30e16e6fe69e9ea2ef92ce0'
Cloning into 'roms/sgabios'...
Submodule path 'roms/sgabios': checked out 'cbaee52287e5f32373181cff50a00b6c4ac9015a'
Cloning into 'roms/skiboot'...
Submodule path 'roms/skiboot': checked out 'e0ee24c27a172bcf482f6f2bc905e6211c134bcc'
Cloning into 'roms/u-boot'...
Submodule path 'roms/u-boot': checked out 'd85ca029f257b53a96da6c2fb421e78a003a9943'
Cloning into 'roms/u-boot-sam460ex'...
Submodule path 'roms/u-boot-sam460ex': checked out '60b3916f33e617a815973c5a6df77055b2e3a588'
Cloning into 'tests/fp/berkeley-softfloat-3'...
Submodule path 'tests/fp/berkeley-softfloat-3': checked out 'b64af41c3276f97f0e181920400ee056b9c88037'
Cloning into 'tests/fp/berkeley-testfloat-3'...
Submodule path 'tests/fp/berkeley-testfloat-3': checked out '5a59dcec19327396a011a17fd924aed4fec416b3'
Cloning into 'ui/keycodemapdb'...
Submodule path 'ui/keycodemapdb': checked out '6b3d716e2b6472eb7189d3220552280ef3d832ce'
Switched to a new branch 'test'
702a152 tcg: Diagnose referenced labels that have not been emitted

=== OUTPUT BEGIN ===
ERROR: spaces prohibited around that ':' (ctx:WxW)
#90: FILE: tcg/tcg.h:249:
+    unsigned present : 1;
                      ^

ERROR: spaces prohibited around that ':' (ctx:WxW)
#93: FILE: tcg/tcg.h:251:
+    unsigned id : 14;
                 ^

total: 2 errors, 0 warnings, 79 lines checked

Commit 702a152f7d06 (tcg: Diagnose referenced labels that have not been emitted) has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===

Test command exited with code: 1


The full log is available at
http://patchew.org/logs/20190208035551.3036-1-richard.henderson@linaro.org/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [Qemu-devel] [PATCH] tcg: Diagnose referenced labels that have not been emitted

[Philippe Mathieu-Daudé]

Hi Richard,

On 2/8/19 4:55 AM, Richard Henderson wrote:
> Currently, a jump to a label that is not defined anywhere will
> be emitted not be relocated.  This results in a jump to a random
> jump target.  With tcg debugging, print a diagnostic to the -d op
> file and abort.
> 
> This could help debug or detect errors like
> c2d9644e6d ("target/arm: Fix crash on conditional instruction in an IT block")
> 
> Reported-by: Roman Kapl <code@rkapl.cz>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  tcg/tcg-op.h |  1 +
>  tcg/tcg.h    | 12 +++++++++---
>  tcg/tcg.c    | 23 +++++++++++++++++++++++
>  3 files changed, 33 insertions(+), 3 deletions(-)
> ---
> This detects errors earlier than the patch that Roman posted.
> 
> 
> r~
> 
> 
> diff --git a/tcg/tcg-op.h b/tcg/tcg-op.h
> index 2d98868d8f..d3e51b15af 100644
> --- a/tcg/tcg-op.h
> +++ b/tcg/tcg-op.h
> @@ -255,6 +255,7 @@ static inline void tcg_gen_op6ii_i64(TCGOpcode opc, TCGv_i64 a1, TCGv_i64 a2,
>  
>  static inline void gen_set_label(TCGLabel *l)
>  {
> +    l->present = 1;
>      tcg_gen_op1(INDEX_op_set_label, label_arg(l));
>  }
>  
> diff --git a/tcg/tcg.h b/tcg/tcg.h
> index 045c24a357..32b7cf3489 100644
> --- a/tcg/tcg.h
> +++ b/tcg/tcg.h
> @@ -244,16 +244,21 @@ typedef struct TCGRelocation {
>      intptr_t addend;
>  } TCGRelocation; 
>  
> -typedef struct TCGLabel {
> +typedef struct TCGLabel TCGLabel;
> +struct TCGLabel {
> +    unsigned present : 1;
>      unsigned has_value : 1;
> -    unsigned id : 15;
> +    unsigned id : 14;
>      unsigned refs : 16;
>      union {
>          uintptr_t value;
>          tcg_insn_unit *value_ptr;
>          TCGRelocation *first_reloc;
>      } u;
> -} TCGLabel;
> +#ifdef CONFIG_DEBUG_TCG
> +    QSIMPLEQ_ENTRY(TCGLabel) next;
> +#endif
> +};
>  
>  typedef struct TCGPool {
>      struct TCGPool *next;
> @@ -685,6 +690,7 @@ struct TCGContext {
>  #endif
>  
>  #ifdef CONFIG_DEBUG_TCG
> +    QSIMPLEQ_HEAD(, TCGLabel) labels;
>      int temps_in_use;
>      int goto_tb_issue_mask;
>  #endif
> diff --git a/tcg/tcg.c b/tcg/tcg.c
> index 20a5d8f315..9b2bf7f439 100644
> --- a/tcg/tcg.c
> +++ b/tcg/tcg.c
> @@ -305,6 +305,9 @@ TCGLabel *gen_new_label(void)

Not related to this patch content, but I'm wondering why, TCGLabels
never get freed?

>      *l = (TCGLabel){
>          .id = s->nb_labels++
>      };
> +#ifdef CONFIG_DEBUG_TCG
> +    QSIMPLEQ_INSERT_TAIL(&s->labels, l, next);
> +#endif
>  
>      return l;
>  }
> @@ -1092,6 +1095,9 @@ void tcg_func_start(TCGContext *s)
>  
>      QTAILQ_INIT(&s->ops);
>      QTAILQ_INIT(&s->free_ops);
> +#ifdef CONFIG_DEBUG_TCG
> +    QSIMPLEQ_INIT(&s->labels);
> +#endif
>  }
>  
>  static inline TCGTemp *tcg_temp_alloc(TCGContext *s)
> @@ -3841,6 +3847,23 @@ int tcg_gen_code(TCGContext *s, TranslationBlock *tb)
>      }
>  #endif
>  
> +#ifdef CONFIG_DEBUG_TCG
> +    /* Ensure all labels referenced have been emitted.  */
> +    {
> +        TCGLabel *l;
> +        bool error = false;
> +
> +        QSIMPLEQ_FOREACH(l, &s->labels, next) {
> +            if (unlikely(!l->present) && l->refs) {
> +                qemu_log_mask(CPU_LOG_TB_OP,
> +                              "$L%d referenced but not present.\n", l->id);
> +                error = true;
> +            }
> +        }
> +        assert(!error);
> +    }
> +#endif
> +
>  #ifdef CONFIG_PROFILER
>      atomic_set(&prof->opt_time, prof->opt_time - profile_getclock());
>  #endif
> 

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Re: [Qemu-devel] [PATCH] tcg: Diagnose referenced labels that have not been emitted

[Richard Henderson]

On 2/8/19 2:41 AM, Philippe Mathieu-Daudé wrote:
> Not related to this patch content, but I'm wondering why, TCGLabels
> never get freed?

They are allocated with tcg_malloc, which is a pool allocator.  The entire pool
is freed after each TB is compiled.


r~

Re: [Qemu-devel] [PATCH] tcg: Diagnose referenced labels that have not been emitted

[Philippe Mathieu-Daudé]

On 2/8/19 5:52 PM, Richard Henderson wrote:
> On 2/8/19 2:41 AM, Philippe Mathieu-Daudé wrote:
>> Not related to this patch content, but I'm wondering why, TCGLabels
>> never get freed?
> 
> They are allocated with tcg_malloc, which is a pool allocator.  The entire pool
> is freed after each TB is compiled.

Got it now, thanks!

Phil.