diff mbox series

tcg: Diagnose referenced labels that have not been emitted

Message ID 20190208035551.3036-1-richard.henderson@linaro.org
State Superseded
Headers show
Series tcg: Diagnose referenced labels that have not been emitted | expand

Commit Message

Richard Henderson Feb. 8, 2019, 3:55 a.m. UTC 
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~


-- 
2.17.2

Comments

no-reply@patchew.org Feb. 8, 2019, 4 a.m. UTC | #1 
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
no-reply@patchew.org Feb. 8, 2019, 4 a.m. UTC | #2 
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
no-reply@patchew.org Feb. 8, 2019, 4:05 a.m. UTC | #3 
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
Philippe Mathieu-Daudé Feb. 8, 2019, 10:41 a.m. UTC | #4 
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>
Richard Henderson Feb. 8, 2019, 4:52 p.m. UTC | #5 
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~
Philippe Mathieu-Daudé Feb. 8, 2019, 4:58 p.m. UTC | #6 
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.
diff mbox series

Patch

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